Merge pull request #2230 from concaf/concaf/fix/mappings-same-name

Change resource name to store if duplicate found

python/ambassador/config/config.py

@@ -593,10 +593,18 @@ def safe_store(self, storage_name: str, resource: ACResource, allow_log: bool=Tr
         storage = self.config.setdefault(storage_name, {})
 
         if resource.name in storage:
-            # Oooops.
-            self.post_error("%s defines %s %s, which is already defined by %s" %
-                            (resource, resource.kind, resource.name, storage[resource.name].location),
-                            resource=resource)
+            if resource.namespace == storage[resource.name].get('namespace'):
+                # If the name and namespace, both match, then it's definitely an error.
+                # Oooops.
+                self.post_error("%s defines %s %s, which is already defined by %s" %
+                                (resource, resource.kind, resource.name, storage[resource.name].location),
+                                resource=resource)
+            else:
+                # Here, we deal with the case when multiple resources have the same name but they exist in different
+                # namespaces. Our current data structure to store resources is a flat string. Till we move to
+                # identifying resources with both, name and namespace, we change names of any subsequent resources with
+                # the same name here.
+                resource.name = f'{resource.name}.{resource.namespace}'
 
         if allow_log:
             self.logger.debug("%s: saving %s %s" %

python/ambassador/config/resourcefetcher.py

@@ -605,7 +605,7 @@ def handle_k8s_ingress(self, k8s_object: AnyDict) -> HandlerResult:
             ingress_status = self.ambassador_service_raw.get('status', {})
             ingress_status_update = (k8s_object.get('kind'), ingress_namespace, ingress_status)
             self.logger.info(f"Updating Ingress {ingress_name} status to {ingress_status_update}")
-            self.aconf.k8s_status_updates[ingress_name] = ingress_status_update
+            self.aconf.k8s_status_updates[f'{ingress_name}.{ingress_namespace}'] = ingress_status_update
 
         if parsed_ambassador_annotations is not None:
             # Copy metadata_labels to parsed annotations, if need be.

python/tests/t_ingress.py

@@ -248,3 +248,88 @@ def check(self):
         ingress_out, _ = ingress_run.communicate()
         ingress_json = json.loads(ingress_out)
         assert ingress_json['status'] == self.status_update, f"Expected Ingress status to be {self.status_update}, got {ingress_json['status']} instead"
+
+
+class SameIngressMultipleNamespaces(AmbassadorTest):
+    status_update = {
+        "loadBalancer": {
+            "ingress": [{
+                "ip": "210.210.210.210"
+            }]
+        }
+    }
+
+    def init(self):
+        self.target = HTTP()
+        self.target1 = HTTP(name="target1", namespace="same-ingress-1")
+        self.target2 = HTTP(name="target2", namespace="same-ingress-2")
+
+    def manifests(self) -> str:
+        return super().manifests() + """
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: same-ingress-1
+---
+apiVersion: extensions/v1beta1
+kind: Ingress
+metadata:
+  annotations:
+    kubernetes.io/ingress.class: ambassador
+    getambassador.io/ambassador-id: {self.ambassador_id}
+  name: {self.name.k8s}
+  namespace: same-ingress-1
+spec:
+  rules:
+  - http:
+      paths:
+      - backend:
+          serviceName: {self.target.path.k8s}-target1
+          servicePort: 80
+        path: /{self.name}-target1/
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: same-ingress-2
+---
+apiVersion: extensions/v1beta1
+kind: Ingress
+metadata:
+  annotations:
+    kubernetes.io/ingress.class: ambassador
+    getambassador.io/ambassador-id: {self.ambassador_id}
+  name: {self.name.k8s}
+  namespace: same-ingress-2
+spec:
+  rules:
+  - http:
+      paths:
+      - backend:
+          serviceName: {self.target.path.k8s}-target2
+          servicePort: 80
+        path: /{self.name}-target2/
+"""
+
+    def queries(self):
+        if sys.platform != 'darwin':
+            text = json.dumps(self.status_update)
+
+            update_cmd = ['kubestatus', 'Service', '-f', f'metadata.name={self.name.k8s}', '-u', '/dev/fd/0']
+            subprocess.run(update_cmd, input=text.encode('utf-8'), timeout=5)
+
+            yield Query(self.url(self.name + "-target1/"))
+            yield Query(self.url(self.name + "-target2/"))
+
+    def check(self):
+        if sys.platform == 'darwin':
+            pytest.xfail('not supported on Darwin')
+
+        for namespace in ['same-ingress-1', 'same-ingress-2']:
+            # check for Ingress IP here
+            ingress_cmd = ["kubectl", "get", "-o", "json", "ingress", self.path.k8s, "-n", namespace]
+            ingress_run = subprocess.Popen(ingress_cmd, stdout=subprocess.PIPE)
+            ingress_out, _ = ingress_run.communicate()
+            ingress_json = json.loads(ingress_out)
+            assert ingress_json['status'] == self.status_update, f"Expected Ingress status to be {self.status_update}, got {ingress_json['status']} instead"

python/tests/t_mappingtests.py

@@ -685,3 +685,48 @@ def check(self):
         # [2]
         assert len(self.results[2].backend.request.headers['l5d-dst-override']) > 0
         assert self.results[2].backend.request.headers['l5d-dst-override'] == ["{}:80".format(self.target_add_linkerd_header_only.path.fqdn)]
+
+
+class SameMappingDifferentNamespaces(AmbassadorTest):
+    target: ServiceType
+
+    def init(self):
+        self.target = HTTP()
+
+    def manifests(self) -> str:
+        return super().manifests() + self.format('''
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: same-mapping-1
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: same-mapping-2
+---
+apiVersion: getambassador.io/v1
+kind: Mapping
+metadata:
+  name: {self.target.path.k8s}
+  namespace: same-mapping-1
+spec:
+  ambassador_id: {self.ambassador_id}
+  prefix: /{self.name}-1/
+  service: {self.target.path.fqdn}.default
+---
+apiVersion: getambassador.io/v1
+kind: Mapping
+metadata:
+  name: {self.target.path.k8s}
+  namespace: same-mapping-2
+spec:
+  ambassador_id: {self.ambassador_id}
+  prefix: /{self.name}-2/
+  service: {self.target.path.fqdn}.default
+''')
+
+    def queries(self):
+        yield Query(self.url(self.name + "-1/"))
+        yield Query(self.url(self.name + "-2/"))