Add Namespace to var definition to allow disambiguation

pkg/accumulator/resaccumulator.go

@@ -64,20 +64,14 @@ func (ra *ResAccumulator) GetTransformerConfig() *config.TransformerConfig {
 
 func (ra *ResAccumulator) MergeVars(incoming []types.Var) error {
 	for _, v := range incoming {
-		// Warning: Do not change GvknEquals to Equals until the
-		// namespace is part of the variable declaration.
-		// The namespace will eventually be added to the variable
-		// declaration to solve the following issue:
-		// https://github.com/kubernetes-sigs/kustomize/issues/1298
-		// If two resources of the same kind with the same name but
-		// in different namespaces, any variable aiming to put at one
-		// of those resources will fail since it is not possible to
-		// specify the namespace and use Id.Equals instead of Id.GvknEquals
-		// The change will also need to be backward compatible, i.e.
-		// no namespace specified means wildcard namespace not "default"
-		// namespace.
-		matched := ra.resMap.GetMatchingResourcesByOriginalId(
-			resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals)
+		targetId := resid.NewResIdWithNamespace(v.ObjRef.GVK(), v.ObjRef.Name, v.ObjRef.Namespace)
+		idMatcher := targetId.GvknEquals
+		if targetId.Namespace != "" || !targetId.IsNamespaceableKind() {
+			// Preserve backward compatibility. An empty namespace means
+			// wildcard search on the namespace hence we still use GvknEquals
+			idMatcher = targetId.Equals
+		}
+		matched := ra.resMap.GetMatchingResourcesByOriginalId(idMatcher)
 		if len(matched) > 1 {
 			return fmt.Errorf(
 				"found %d resId matches for var %s "+

pkg/target/namespaces_test.go

@@ -104,8 +104,8 @@ rules:
 // https://github.com/kubernetes-sigs/kustomize/issues/1298
 const namespaceNeedInVarMyApp string = `
 resources:
-- elasticsearch-test-service.yaml
 - elasticsearch-dev-service.yaml
+- elasticsearch-test-service.yaml
 vars:
 - name: elasticsearch-test-service-name
   objref:
@@ -256,11 +256,8 @@ spec:
 `
 
 // TestVariablesAmbiguous demonstrates how two variables pointing at two different resources
-// using the same name in different namespaces are treated as ambiguous
-// The fundamental reason is that objRef field in the variable does not contain a namespace
-// qualifier.
-// Once the namespace qualifer is added, as for the other resources lookup in the coder,
-// the ResId.GvknEquals method usage will have to be phased out and replaced by ResId.Equals.
+// using the same name in different namespaces are treated as ambiguous if the namespace is
+// not specified
 func TestVariablesAmbiguous(t *testing.T) {
 	th := kusttest_test.NewKustTestHarness(t, "/namespaceNeedInVar/myapp")
 	th.WriteK("/namespaceNeedInVar/myapp", namespaceNeedInVarMyApp)
@@ -335,3 +332,56 @@ resources:
 	}
 	th.AssertActualEqualsExpected(m, namespaceNeedInVarExpectedOutput)
 }
+
+const namespaceNeedInVarMyAppWithNamespace string = `
+resources:
+- elasticsearch-dev-service.yaml
+- elasticsearch-test-service.yaml
+vars:
+- name: elasticsearch-test-service-name
+  objref:
+    kind: Service
+    name: elasticsearch
+    namespace: test
+    apiVersion: v1
+  fieldref:
+    fieldpath: metadata.name
+- name: elasticsearch-test-protocol
+  objref:
+    kind: Service
+    name: elasticsearch
+    namespace: test
+    apiVersion: v1
+  fieldref:
+    fieldpath: spec.ports[0].protocol
+- name: elasticsearch-dev-service-name
+  objref:
+    kind: Service
+    name: elasticsearch
+    namespace: dev
+    apiVersion: v1
+  fieldref:
+    fieldpath: metadata.name
+- name: elasticsearch-dev-protocol
+  objref:
+    kind: Service
+    name: elasticsearch
+    namespace: dev
+    apiVersion: v1
+  fieldref:
+    fieldpath: spec.ports[0].protocol
+`
+
+// TestVariablesDisambiguatedWithNamespace demonstrates that adding the namespace
+// to the variable declarations allows to disambiguate the variables.
+func TestVariablesDisambiguatedWithNamespace(t *testing.T) {
+	th := kusttest_test.NewKustTestHarness(t, "/namespaceNeedInVar/myapp")
+	th.WriteK("/namespaceNeedInVar/myapp", namespaceNeedInVarMyAppWithNamespace)
+	th.WriteF("/namespaceNeedInVar/myapp/elasticsearch-dev-service.yaml", namespaceNeedInVarDevResources)
+	th.WriteF("/namespaceNeedInVar/myapp/elasticsearch-test-service.yaml", namespaceNeedInVarTestResources)
+	m, err := th.MakeKustTarget().MakeCustomizedResMap()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	th.AssertActualEqualsExpected(m, namespaceNeedInVarExpectedOutput)
+}

pkg/types/var.go

@@ -54,6 +54,7 @@ type Target struct {
 	APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty"`
 	gvk.Gvk    `json:",inline,omitempty" yaml:",inline,omitempty"`
 	Name       string `json:"name" yaml:"name"`
+	Namespace  string `json:"namespace,omitempty" yaml:"namespace,omitempty"`
 }
 
 // FieldSelector contains the fieldPath to an object field.