bug: make canary groups account for host mappingSelector labels (#5215)

If two mappings have different host labels but share the same prefix, then they should not be placed in the same canary group. Previously, labels were not taken into account when grouping mappings into canary groups, which caused mappings sharing the same prefix to be part of the same canary group when using host labels (hostname or host field was not specified in the mapping).

In order to solve this, we update the mapping group_id logic to incorporate host selector labels. This means that the mappings are grouped based on the following criteria:
- HTTP method
- Prefix
- Headers
- Query parameters
- Host labels
- Precedence

Because this affects only how mappings are grouped, the following behavior is retained:
- hostname, host, and :authority header fields take precendence over host labels for the purposes of grouping.
- If hostname, host, and/or :authority header fields are specified along with host labels, then they must match before they can be associated.
- A canary group can be associated with multiple Hosts based on selector labels.

Fixes #4170.
---------

Signed-off-by: Hamzah Qudsi <[email protected]>

CHANGELOG.md

@@ -90,6 +90,15 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
 
 ### Emissary-ingress and Ambassador Edge Stack
 
+- Bugfix: As of v2.2.2, if two mappings were associated with different Hosts through host
+  mappingSelector labels but share the same prefix, the labels were not taken into account which
+  would cause one Mapping to be correctly routed but the other not.
+  This change fixes this issue so
+  that Mappings sharing the same prefix but associated with different Hosts will be correctly
+  routed. ([Canary grouping must take labels into account])
+
+[Canary grouping must take labels into account]: https://github.com/emissary-ingress/emissary/issues/4170
+
 ## [3.7.2] July 25, 2023
 [3.7.2]: https://github.com/emissary-ingress/emissary/compare/v3.7.1...v3.7.2
 

docs/releaseNotes.yml

@@ -35,7 +35,19 @@ items:
   - version: 3.8.0
     prevVersion: 3.7.2
     date: TBD
-    notes: []
+    notes:
+      - title: Account for matchLabels when associating mappings with the same prefix to different Hosts
+        type: bugfix
+        body: >-
+          As of v2.2.2, if two mappings were associated with different Hosts through host
+          mappingSelector labels but share the same prefix, the labels were not taken into
+          account which would cause one Mapping to be correctly routed but the other not.
+
+          This change fixes this issue so that Mappings sharing the same prefix but associated
+          with different Hosts will be correctly routed.
+        github:
+          - title: "Canary grouping must take labels into account"
+            link: https://github.com/emissary-ingress/emissary/issues/4170
 
   - version: 3.7.2
     prevVersion: 3.7.1

python/ambassador/ir/irhttpmapping.py

@@ -11,6 +11,7 @@
 from .irerrorresponse import IRErrorResponse
 from .irhttpmappinggroup import IRHTTPMappingGroup
 from .irretrypolicy import IRRetryPolicy
+from .irutils import selector_matches
 
 if TYPE_CHECKING:
     from .ir import IR  # pragma: no cover
@@ -203,39 +204,39 @@ def __init__(
 
         # OK. Start by looking for a :authority header match.
         if "headers" in kwargs:
-            for name, value in kwargs.get("headers", {}).items():
-                if value is True:
-                    hdrs.append(KeyValueDecorator(name))
+            for hdr_name, hdr_value in kwargs.get("headers", {}).items():
+                if hdr_value is True:
+                    hdrs.append(KeyValueDecorator(hdr_name))
                 else:
                     # An exact match on the :authority header is special -- treat it like
                     # they set the "host" element (but note that we'll allow the actual
                     # "host" element to override it later).
-                    if name.lower() == ":authority":
+                    if hdr_name.lower() == ":authority":
                         # This is an _exact_ match, so it mustn't contain a "*" -- that's illegal in the DNS.
-                        if "*" in value:
+                        if "*" in hdr_value:
                             # We can't call self.post_error() yet, because we're not initialized yet. So we cheat a bit
                             # and defer the error for later.
                             new_args[
                                 "_deferred_error"
-                            ] = f":authority exact-match '{value}' contains *, which cannot match anything."
+                            ] = f":authority exact-match '{hdr_value}' contains *, which cannot match anything."
                             ir.logger.debug(
                                 "IRHTTPMapping %s: self.host contains * (%s, :authority)",
                                 name,
-                                value,
+                                hdr_value,
                             )
                         else:
                             # No globs, just save it. (We'll end up using it as a glob later, in the Envoy
                             # config part of the world, but that's OK -- a glob with no "*" in it will always
                             # match only itself.)
-                            host = value
+                            host = hdr_value
                             ir.logger.debug(
                                 "IRHTTPMapping %s: self.host == %s (:authority)", name, self.host
                             )
                             # DO NOT save the ':authority' match here -- we'll pick it up after we've checked
                             # for hostname, too.
                     else:
                         # It's not an :authority match, so we're good.
-                        hdrs.append(KeyValueDecorator(name, value))
+                        hdrs.append(KeyValueDecorator(hdr_name, hdr_value))
 
         if "regex_headers" in kwargs:
             # DON'T do anything special with a regex :authority match: we can't
@@ -503,36 +504,63 @@ def validate_load_balancer(load_balancer) -> bool:
 
         return is_valid
 
+    # Mappings are grouped by:
+    #  - HTTP Method
+    #  - Prefix
+    #  - Headers
+    #  - Query Parameters
+    #  - Mapping Label Selectors
+    #  - Precedence
     def _group_id(self) -> str:
-        # Yes, we're using a cryptographic hash here. Cope. [ :) ]
-
-        h = hashlib.new("sha1")
-
         # This is an HTTP mapping.
-        h.update("HTTP-".encode("utf-8"))
-
-        # method first, but of course method might be None. For calculating the
-        # group_id, 'method' defaults to 'GET' (for historical reasons).
+        group_id = "HTTP-".encode("utf-8")
 
+        # Method
         method = self.get("method") or "GET"
-        h.update(method.encode("utf-8"))
-        h.update(self.prefix.encode("utf-8"))
+        group_id = group_id + method.encode("utf-8")
 
+        # Prefix
+        group_id = group_id + self.prefix.encode("utf-8")
+
+        # Headers
         for hdr in self.headers:
-            h.update(hdr.name.encode("utf-8"))
+            group_id = group_id + hdr.name.encode("utf-8")
 
             if hdr.value is not None:
-                h.update(hdr.value.encode("utf-8"))
+                group_id = group_id + hdr.value.encode("utf-8")
 
+        # Query Parameters
         for query_parameter in self.query_parameters:
-            h.update(query_parameter.name.encode("utf-8"))
+            group_id = group_id + query_parameter.name.encode("utf-8")
 
             if query_parameter.value is not None:
-                h.update(query_parameter.value.encode("utf-8"))
-
+                group_id = group_id + query_parameter.value.encode("utf-8")
+
+        # Host Mapping Selector Labels
+        if not self.host and self.metadata_labels is not None:
+            for host in self.ir.hosts.values():
+                mapsel = host.get("mappingSelector")
+                if not mapsel:
+                    continue
+
+                if selector_matches(self.ir.logger, mapsel, self.metadata_labels):
+                    # We care only about the labels that are part of the Host mappingSelector.
+                    # For example, let's say there are two Mappings with labels
+                    # host=foo;irrelevant-label=1 for one Mapping and host=foo;irrelevant-label=2
+                    # for the other Mapping. There exists a Host that contains a mappingSelector
+                    # for host=foo. We would only want to group based on the host label and not
+                    # the irrelevant label. In this case the two Mappings are part of the same group
+                    # assumming method, prefix, headers, etc. all match.
+                    for key, val in mapsel.get("matchLabels", {}).items():
+                        group_id = group_id + key.encode("utf-8")
+                        group_id = group_id + val.encode("utf-8")
+
+        # Precedence
         if self.precedence != 0:
-            h.update(str(self.precedence).encode("utf-8"))
+            group_id = group_id + str(self.precedence).encode("utf-8")
 
+        h = hashlib.new("sha1")
+        h.update(group_id)
         return h.hexdigest()
 
     def _route_weight(self) -> List[Union[str, int]]:

python/ambassador/ir/irhttpmappinggroup.py

@@ -366,8 +366,6 @@ def finalize(self, ir: "IR", aconf: Config) -> List[IRCluster]:
             add_request_headers.update(mapping.get("add_request_headers", {}))
             add_response_headers.update(mapping.get("add_response_headers", {}))
 
-            # Should we have higher weights win over lower if there are conflicts?
-            # Should we disallow conflicts?
             metadata_labels.update(mapping.get("metadata_labels") or {})
 
         if add_request_headers:

python/tests/unit/test_mapping_canary_group.py

@@ -0,0 +1,65 @@
+import json
+import os
+from dataclasses import dataclass
+from typing import List, Optional
+
+import pytest
+import yaml
+
+from tests.utils import compile_with_cachecheck
+
+
+@dataclass
+class MappingGroupTestOutput:
+    group_id: str
+    host: Optional[str]
+    prefix: str
+    mappings: List[str]
+
+
+test_cases = [
+    "mapping_selector",
+    "mapping_selector_and_authority",
+    "mapping_selector_and_hostname",
+    "mapping_selector_and_host",
+    "mapping_selector_to_multiple_hosts",
+    "mapping_selector_irrelevant_labels",
+]
+
+
+@pytest.mark.compilertest
+@pytest.mark.parametrize("test_case", test_cases)
+def test_mapping_canary_group_selectors(test_case):
+    testdata_dir = os.path.join(
+        os.path.dirname(os.path.abspath(__file__)), "testdata", "canary_groups"
+    )
+
+    with open(os.path.join(testdata_dir, f"{test_case}_in.yaml"), "r") as f:
+        test_yaml = f.read()
+
+    r = compile_with_cachecheck(test_yaml, errors_ok=True)
+
+    ir = r["ir"]
+
+    errors = ir.aconf.errors
+    assert len(errors) == 0, "Expected no errors but got %s" % (
+        json.dumps(errors, sort_keys=True, indent=4)
+    )
+
+    mapping_groups = []
+    for g in ir.groups.values():
+        if g.prefix.startswith("/ambassador"):
+            continue
+
+        mg = MappingGroupTestOutput(
+            group_id=g.group_id, host=g.host, prefix=g.prefix, mappings=[m.name for m in g.mappings]
+        )
+        mapping_groups.append(mg)
+
+    with open(os.path.join(testdata_dir, f"{test_case}_out.yaml"), "r") as f:
+        out = yaml.safe_load(f)
+
+    expected_output = [MappingGroupTestOutput(**group_yaml) for group_yaml in out["mapping_groups"]]
+    assert sorted(mapping_groups, key=lambda g: g.group_id) == sorted(
+        expected_output, key=lambda g: g.group_id
+    )

python/tests/unit/testdata/canary_groups/mapping_selector_and_authority_in.yaml

@@ -0,0 +1,47 @@
+apiVersion: getambassador.io/v3alpha1
+kind: Host
+metadata:
+  name: foo-host
+  namespace: default
+spec:
+  hostname: foo.example.com
+  mappingSelector:
+    matchLabels:
+      host: foo
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Host
+metadata:
+  name: bar-host
+  namespace: default
+spec:
+  hostname: bar.example.com
+  mappingSelector:
+    matchLabels:
+      host: bar
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: star-backend-foo
+  namespace: default
+  labels:
+    host: foo
+spec:
+  prefix: /test/
+  service: star
+  headers:
+    ":authority": foo.example.com
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: star-backend-bar
+  namespace: default
+  labels:
+    host: bar
+spec:
+  prefix: /test/
+  service: star
+  headers:
+    ":authority": bar.example.com

python/tests/unit/testdata/canary_groups/mapping_selector_and_authority_out.yaml

@@ -0,0 +1,12 @@
+mapping_groups:
+- group_id: '1a5a176cc92a69ab669ac332644324fe7813e889'
+  host: 'foo.example.com'
+  prefix: /test/
+  mappings:
+  - star-backend-foo
+
+- group_id: '71cf9a8de7cec920d057da6ae1a1d304dc599a05'
+  host: 'bar.example.com'
+  prefix: /test/
+  mappings:
+  - star-backend-bar

python/tests/unit/testdata/canary_groups/mapping_selector_and_host_in.yaml

@@ -0,0 +1,45 @@
+apiVersion: getambassador.io/v3alpha1
+kind: Host
+metadata:
+  name: foo-host
+  namespace: default
+spec:
+  hostname: foo.example.com
+  mappingSelector:
+    matchLabels:
+      host: foo
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Host
+metadata:
+  name: bar-host
+  namespace: default
+spec:
+  hostname: bar.example.com
+  mappingSelector:
+    matchLabels:
+      host: bar
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: star-backend-foo
+  namespace: default
+  labels:
+    host: foo
+spec:
+  prefix: /test/
+  service: star
+  host: foo.example.com
+---
+apiVersion: getambassador.io/v3alpha1
+kind: Mapping
+metadata:
+  name: star-backend-bar
+  namespace: default
+  labels:
+    host: bar
+spec:
+  prefix: /test/
+  service: star
+  host: bar.example.com

python/tests/unit/testdata/canary_groups/mapping_selector_and_host_out.yaml

@@ -0,0 +1,12 @@
+mapping_groups:
+- group_id: '1a5a176cc92a69ab669ac332644324fe7813e889'
+  host: foo.example.com
+  prefix: /test/
+  mappings:
+  - star-backend-foo
+
+- group_id: '71cf9a8de7cec920d057da6ae1a1d304dc599a05'
+  host: bar.example.com
+  prefix: /test/
+  mappings:
+  - star-backend-bar