diff --git a/CHANGELOG.md b/CHANGELOG.md index 66ff7162d0..00b63d6454 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,11 @@ Please see the [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest processing will happen before authentication. This corrects a problem where XHR to authenticated endpoints would fail. +- Bugfix: In 2.x releases of Emissary-ingress when there are multiple `Mapping`s that have the same + `metadata.name` across multiple namespaces, their old config would not properly be removed from + the cache when their config was updated. This resulted in an inability to update configuration for + groups of `Mapping`s that share the same name until the Emissary-ingress pods restarted. + [#4179]: https://github.com/emissary-ingress/emissary/pull/4179 ## [2.2.2] February 25, 2022 diff --git a/docs/releaseNotes.yml b/docs/releaseNotes.yml index cd55af026e..5a10385bf7 100644 --- a/docs/releaseNotes.yml +++ b/docs/releaseNotes.yml @@ -54,6 +54,13 @@ items: When CORS is specified (either in a Mapping or in the Ambassador Module), CORS processing will happen before authentication. This corrects a problem where XHR to authenticated endpoints would fail. + - title: Correctly handle caching Mappings with the same name in different namespaces + type: bugfix + body: >- + In 2.x releases of $productName$ when there are multiple Mappings that have the same + metadata.name across multiple namespaces, their old config would not properly be removed + from the cache when their config was updated. This resulted in an inability to update configuration + for groups of Mappings that share the same name until the $productName$ pods restarted. - version: 2.2.2 date: '2022-02-25' diff --git a/python/ambassador/ir/irbasemapping.py b/python/ambassador/ir/irbasemapping.py index 594f1f5e8a..cd7ab8832f 100644 --- a/python/ambassador/ir/irbasemapping.py +++ b/python/ambassador/ir/irbasemapping.py @@ -114,6 +114,20 @@ def __init__(self, ir: 'IR', aconf: Config, @classmethod def make_cache_key(cls, kind: str, name: str, namespace: str, version: str="v2") -> str: + # Why is this split on the name necessary? + # the name of a Mapping when we fetch it from the aconf will match the metadata.name of + # the Mapping that the config comes from _only if_ it is the only Mapping with that exact name. + # If there are multiple Mappings with the same name in different namespaces then the name + # becomes `name.namespace` for all mappings of the same name after the first one. + # The first one just gets to be `name` for "reasons". + # + # This behaviour is needed by other places in the code, but for the cache key, we need it to match the + # below format regardless of how many Mappings there are with that name. This is necessary for the cache + # specifically because there are places where we interact with the cache that have access to the + # metadata.name and metadata.namespace of the Mapping, but do not have access to the aconf representation + # of the Mapping name and thus have no way of knowing whether a specific name is mangled due to multiple + # Mappings sharing the same name or not. + name = name.split(".")[0] return f"{kind}-{version}-{name}-{namespace}" def setup(self, ir: 'IR', aconf: Config) -> bool: diff --git a/python/tests/unit/test_cache.py b/python/tests/unit/test_cache.py index 1623b62934..3ef6e7854d 100644 --- a/python/tests/unit/test_cache.py +++ b/python/tests/unit/test_cache.py @@ -30,7 +30,7 @@ class Builder: def __init__(self, logger: logging.Logger, yaml_file: str, enable_cache=True) -> None: self.logger = logger - + self.test_dir = os.path.join( os.path.dirname(os.path.abspath(__file__)), "test_cache_data" @@ -149,7 +149,7 @@ def build(self, version='V2') -> Tuple[IR, EnvoyConfig]: if kind not in watt['Kubernetes']: watt['Kubernetes'][kind] = [] - + watt['Kubernetes'][kind].append(rsrc) watt_json = json.dumps(watt, sort_keys=True, indent=4) @@ -263,6 +263,7 @@ def strip_cache_keys(self, node: Any) -> Any: return node +@pytest.mark.compilertest def test_circular_link(): builder = Builder(logger, "cache_test_1.yaml") builder.build() @@ -300,6 +301,7 @@ def test_circular_link(): builder.check_last("after invalidating circular links") +@pytest.mark.compilertest def test_multiple_rebuilds(): builder = Builder(logger, "cache_test_1.yaml") @@ -310,6 +312,7 @@ def test_multiple_rebuilds(): builder.check_last(f"rebuild {i-1} -> {i}") +@pytest.mark.compilertest def test_simple_targets(): builder = Builder(logger, "cache_test_1.yaml") @@ -325,6 +328,7 @@ def test_simple_targets(): builder.check_last("after delete foo-4") +@pytest.mark.compilertest def test_smashed_targets(): builder = Builder(logger, "cache_test_2.yaml") @@ -342,6 +346,7 @@ def test_smashed_targets(): builder.check_last("after invalidating foo-4 and foo-6") +@pytest.mark.compilertest def test_delta_1(): builder1 = Builder(logger, "cache_test_1.yaml") builder2 = Builder(logger, "cache_test_1.yaml", enable_cache=False) @@ -365,6 +370,7 @@ def test_delta_1(): builder3.check("final", b3, b1) +@pytest.mark.compilertest def test_delta_2(): builder1 = Builder(logger, "cache_test_2.yaml") builder2 = Builder(logger, "cache_test_2.yaml", enable_cache=False) @@ -388,6 +394,7 @@ def test_delta_2(): builder3.check("final", b3, b1) +@pytest.mark.compilertest def test_delta_3(): builder1 = Builder(logger, "cache_test_1.yaml") builder2 = Builder(logger, "cache_test_1.yaml", enable_cache=False) @@ -416,6 +423,7 @@ def test_delta_3(): builder3.check("final", b3, b1) +@pytest.mark.compilertest def test_delete_4(): builder1 = Builder(logger, "cache_test_1.yaml") builder2 = Builder(logger, "cache_test_1.yaml", enable_cache=False) @@ -439,6 +447,8 @@ def test_delete_4(): builder3.check("final", b3, b1) + +@pytest.mark.compilertest def test_long_cluster_1(): # Create a cache for Mappings whose cluster names are too long # to be envoy cluster names and must be truncated. @@ -463,6 +473,57 @@ def test_long_cluster_1(): print("test_long_cluster_1 done") + +@pytest.mark.compilertest +def test_mappings_same_name_delta(): + # Tests that multiple Mappings with the same name (but in different namespaces) + # are properly added/removed from the cache when they are updated. + builder = Builder(logger, "cache_test_4.yaml") + b = builder.build() + econf = b[1] + econf = econf.as_dict() + + # loop through all the clusters in the resulting envoy config and pick out two Mappings from our test set (first and lase) + # to ensure their clusters were generated properly. + cluster1_ok, cluster2_ok = False + for cluster in econf['static_resources']['clusters']: + cname = cluster.get('name', None) + assert cname is not None, \ + f"Error, cluster missing cluster name in econf" + # The 6666 in the cluster name comes from the Mapping.spec.service's port + if cname == "cluster_bar_0_example_com_6666_bar0": + cluster1_ok = True + elif cname == "cluster_bar_9_example_com_6666_bar9": + cluster2_ok = True + if cluster1_ok and cluster2_ok: + break + assert cluster1_ok and cluster2_ok, 'clusters could not be found with the correct envoy config' + + # Update the yaml for these Mappings to simulate a reconfiguration + # We should properly remove the cache entries for these clusters when that happens. + builder.apply_yaml("cache_test_5.yaml") + b = builder.build() + econf = b[1] + econf = econf.as_dict() + + cluster1_ok, cluster2_ok = False + for cluster in econf['static_resources']['clusters']: + cname = cluster.get('name', None) + assert cname is not None, \ + f"Error, cluster missing cluster name in econf" + # We can check the cluster name to identify if the clusters were updated properly + # because in the deltas for the yaml we applied, we changed the port to 7777 + # If there was an issue removing the initial ones from the cache then we should see + # 6666 in this field and not find the cluster names below. + if cname == "cluster_bar_0_example_com_7777_bar0": + cluster1_ok = True + elif cname == "cluster_bar_9_example_com_7777_bar9": + cluster2_ok = True + if cluster1_ok and cluster2_ok: + break + assert cluster1_ok and cluster2_ok, 'clusters could not be found with the correct econf after updating their config' + + MadnessVerifier = Callable[[Tuple[IR, EnvoyConfig]], bool] @@ -478,7 +539,7 @@ def __init__(self, name, pfx, svc) -> None: # This is only OK for service names without any weirdnesses. self.cluster = "cluster_" + re.sub(r'[^0-9A-Za-z_]', '_', self.service) + "_default" - + def __str__(self) -> str: return f"MadnessMapping {self.name}: {self.pfx} => {self.service}" @@ -506,7 +567,7 @@ def __init__(self, name: str, op: str, mapping: MadnessMapping, verifiers: List[ self.op = op self.mapping = mapping self.verifiers = verifiers - + def __str__(self) -> str: return self.name @@ -525,7 +586,7 @@ def exec(self, builder1: Builder, builder2: Builder, dumpfile: Optional[str]=Non verifiers.append(self._cluster_absent) else: raise Exception(f"Unknown op {self.op}") - + logger.info("======== builder1:") logger.info("INPUT: %s" % builder1.current_yaml()) @@ -570,7 +631,7 @@ def _cluster_present(self, b: Tuple[IR, EnvoyConfig]) -> bool: def _cluster_absent(self, b: Tuple[IR, EnvoyConfig]) -> bool: ir, econf = b - ir_has_cluster = ir.has_cluster(self.mapping.cluster) + ir_has_cluster = ir.has_cluster(self.mapping.cluster) assert not ir_has_cluster, f"{self.name}: needed no IR cluster {self.mapping.cluster}, but found it" return not ir_has_cluster @@ -612,7 +673,7 @@ def check_group(self, b: Tuple[IR, EnvoyConfig], current_mappings: Dict[MadnessM return match - +@pytest.mark.compilertest def test_cache_madness(): builder1 = Builder(logger, "/dev/null") builder2 = Builder(logger, "/dev/null", enable_cache=False) @@ -657,7 +718,7 @@ def test_cache_madness(): if mapping in current_mappings: del(current_mappings[mapping]) - op = MadnessOp(name=f"delete {mapping.pfx} -> {mapping.service}", op="delete", mapping=mapping, + op = MadnessOp(name=f"delete {mapping.pfx} -> {mapping.service}", op="delete", mapping=mapping, verifiers=[ lambda b: op.check_group(b, current_mappings) ]) else: current_mappings[mapping] = True @@ -670,7 +731,7 @@ def test_cache_madness(): # if not op.exec(builder1, None, dumpfile=f"ir{i}"): if not op.exec(builder1, builder2, dumpfile=f"ir{i}"): break - + if __name__ == '__main__': pytest.main(sys.argv) diff --git a/python/tests/unit/test_cache_data/cache_test_4.yaml b/python/tests/unit/test_cache_data/cache_test_4.yaml new file mode 100644 index 0000000000..2d56403973 --- /dev/null +++ b/python/tests/unit/test_cache_data/cache_test_4.yaml @@ -0,0 +1,150 @@ +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar0 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar1 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar2 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar3 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar4 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar5 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar6 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar7 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar8 +--- +apiVersion: v1 +kind: Namespace +metadata: + name: bar9 +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar0 + name: bar-cache +spec: + prefix: /bar-0/ + service: bar-0.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar1 + name: bar-cache +spec: + prefix: /bar-1/ + service: bar-1.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar2 + name: bar-cache +spec: + prefix: /bar-2/ + service: bar-2.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar3 + name: bar-cache +spec: + prefix: /bar-3/ + service: bar-3.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar4 + name: bar-cache +spec: + prefix: /bar-4/ + service: bar-4.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar5 + name: bar-cache +spec: + prefix: /bar-5/ + service: bar-5.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar6 + name: bar-cache +spec: + prefix: /bar-6/ + service: bar-6.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar7 + name: bar-cache +spec: + prefix: /bar-7/ + service: bar-7.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar8 + name: bar-cache +spec: + prefix: /bar-8/ + service: bar-8.example.com:6666 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar9 + name: bar-cache +spec: + prefix: /bar-9/ + service: bar-9.example.com:6666 + diff --git a/python/tests/unit/test_cache_data/cache_test_5.yaml b/python/tests/unit/test_cache_data/cache_test_5.yaml new file mode 100644 index 0000000000..d909f1bf67 --- /dev/null +++ b/python/tests/unit/test_cache_data/cache_test_5.yaml @@ -0,0 +1,100 @@ +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar0 + name: bar-cache +spec: + prefix: /bar-0/ + service: bar-0.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar1 + name: bar-cache +spec: + prefix: /bar-1/ + service: bar-1.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar2 + name: bar-cache +spec: + prefix: /bar-2/ + service: bar-2.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar3 + name: bar-cache +spec: + prefix: /bar-3/ + service: bar-3.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar4 + name: bar-cache +spec: + prefix: /bar-4/ + service: bar-4.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar5 + name: bar-cache +spec: + prefix: /bar-5/ + service: bar-5.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar6 + name: bar-cache +spec: + prefix: /bar-6/ + service: bar-6.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar7 + name: bar-cache +spec: + prefix: /bar-7/ + service: bar-7.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar8 + name: bar-cache +spec: + prefix: /bar-8/ + service: bar-8.example.com:7777 + +--- +apiVersion: getambassador.io/v3alpha1 +kind: Mapping +metadata: + namespace: bar9 + name: bar-cache +spec: + prefix: /bar-9/ + service: bar-9.example.com:7777 +