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

…ltiple mappings have the same name

Signed-off-by: AliceProxy <[email protected]>

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)
@@ -465,6 +465,55 @@ def test_long_cluster_1():
 
 MadnessVerifier = Callable[[Tuple[IR, EnvoyConfig]], bool]
 
+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('per_connection_buffer_limit_bytes', 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'
+
+    # Invalidate the cache for these two clusters and update their yaml to simulate a reconfiguration
+    # builder.cache.invalidate("Mapping-v2-bar-cache-bar0")
+    # builder.cache.invalidate("Mapping-v2-bar-cache-bar9")
+    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('per_connection_buffer_limit_bytes', 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'
+
 
 class MadnessMapping:
     name: str
@@ -478,7 +527,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 +555,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 +574,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 +619,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
@@ -657,7 +706,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 +719,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)

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
+