Cache: fix an issue where the cache key for mappings is wrong when mu…

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

docs/releaseNotes.yml

@@ -54,6 +54,13 @@ items:
           When CORS is specified (either in a <code>Mapping</code> or in the <code>Ambassador</code>
           <code>Module</code>), 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 <code>Mapping</code>s that have the same
+          <code>metadata.name</code> 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 <code>Mapping</code>s that share the same name until the $productName$ pods restarted.
 
   - version: 2.2.2
     date: '2022-02-25'

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:

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)