diff --git a/pkg/accumulator/resaccumulator.go b/pkg/accumulator/resaccumulator.go index cf5825bc9..e7e460c59 100644 --- a/pkg/accumulator/resaccumulator.go +++ b/pkg/accumulator/resaccumulator.go @@ -64,6 +64,18 @@ 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) if len(matched) > 1 { diff --git a/pkg/target/namespaces_test.go b/pkg/target/namespaces_test.go index 0ce86eea1..481cdf2cc 100644 --- a/pkg/target/namespaces_test.go +++ b/pkg/target/namespaces_test.go @@ -5,6 +5,7 @@ package target_test import ( "sigs.k8s.io/kustomize/v3/pkg/kusttest" + "strings" "testing" ) @@ -93,3 +94,244 @@ rules: - get `) } + +// This serie of constants is used to prove the need of +// the namespace field in the objref field of the var declaration. +// The following tests demonstrate that it creates umbiguous variable +// declaration if two entities of the kind with the same name +// but in different namespaces are declared. +// This is tracking the following issue: +// https://github.com/kubernetes-sigs/kustomize/issues/1298 +const namespaceNeedInVarMyApp string = ` +resources: +- elasticsearch-test-service.yaml +- elasticsearch-dev-service.yaml +vars: +- name: elasticsearch-test-service-name + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: metadata.name +- name: elasticsearch-test-protocol + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: spec.ports[0].protocol +- name: elasticsearch-dev-service-name + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: metadata.name +- name: elasticsearch-dev-protocol + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: spec.ports[0].protocol +` + +const namespaceNeedInVarDevResources string = ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: elasticsearch + namespace: dev +spec: + template: + spec: + containers: + - name: elasticsearch + env: + - name: DISCOVERY_SERVICE + value: "$(elasticsearch-dev-service-name).monitoring.svc.cluster.local" + - name: DISCOVERY_PROTOCOL + value: "$(elasticsearch-dev-protocol)" +--- +apiVersion: v1 +kind: Service +metadata: + name: elasticsearch + namespace: dev +spec: + ports: + - name: transport + port: 9300 + protocol: TCP + clusterIP: None +` + +const namespaceNeedInVarTestResources string = ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: elasticsearch + namespace: test +spec: + template: + spec: + containers: + - name: elasticsearch + env: + - name: DISCOVERY_SERVICE + value: "$(elasticsearch-test-service-name).monitoring.svc.cluster.local" + - name: DISCOVERY_PROTOCOL + value: "$(elasticsearch-test-protocol)" +--- +apiVersion: v1 +kind: Service +metadata: + name: elasticsearch + namespace: test +spec: + ports: + - name: transport + port: 9300 + protocol: UDP + clusterIP: None +` + +const namespaceNeedInVarExpectedOutput string = ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: elasticsearch + namespace: dev +spec: + template: + spec: + containers: + - env: + - name: DISCOVERY_SERVICE + value: elasticsearch.monitoring.svc.cluster.local + - name: DISCOVERY_PROTOCOL + value: TCP + name: elasticsearch +--- +apiVersion: v1 +kind: Service +metadata: + name: elasticsearch + namespace: dev +spec: + clusterIP: None + ports: + - name: transport + port: 9300 + protocol: TCP +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: elasticsearch + namespace: test +spec: + template: + spec: + containers: + - env: + - name: DISCOVERY_SERVICE + value: elasticsearch.monitoring.svc.cluster.local + - name: DISCOVERY_PROTOCOL + value: UDP + name: elasticsearch +--- +apiVersion: v1 +kind: Service +metadata: + name: elasticsearch + namespace: test +spec: + clusterIP: None + ports: + - name: transport + port: 9300 + protocol: UDP +` + +// 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. +func TestVariablesAmbiguous(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/namespaceNeedInVar/myapp") + th.WriteK("/namespaceNeedInVar/myapp", namespaceNeedInVarMyApp) + th.WriteF("/namespaceNeedInVar/myapp/elasticsearch-dev-service.yaml", namespaceNeedInVarDevResources) + th.WriteF("/namespaceNeedInVar/myapp/elasticsearch-test-service.yaml", namespaceNeedInVarTestResources) + _, err := th.MakeKustTarget().MakeCustomizedResMap() + if err == nil { + t.Fatalf("expected error") + } + if !strings.Contains(err.Error(), "unable to disambiguate") { + t.Fatalf("unexpected error %v", err) + } +} + +const namespaceNeedInVarDevFolder string = ` +resources: +- elasticsearch-dev-service.yaml +vars: +- name: elasticsearch-dev-service-name + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: metadata.name +- name: elasticsearch-dev-protocol + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: spec.ports[0].protocol +` + +const namespaceNeedInVarTestFolder string = ` +resources: +- elasticsearch-test-service.yaml +vars: +- name: elasticsearch-test-service-name + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: metadata.name +- name: elasticsearch-test-protocol + objref: + kind: Service + name: elasticsearch + apiVersion: v1 + fieldref: + fieldpath: spec.ports[0].protocol +` + +// TestVariablesAmbiguousWorkaround demonstrates a possible workaround +// to TestVariablesAmbiguous problem. It requires to separate the variables +// and resources into multiple kustomization context/folders instead of one. +func TestVariablesAmbiguousWorkaround(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/namespaceNeedInVar/workaround") + th.WriteK("/namespaceNeedInVar/dev", namespaceNeedInVarDevFolder) + th.WriteF("/namespaceNeedInVar/dev/elasticsearch-dev-service.yaml", namespaceNeedInVarDevResources) + th.WriteK("/namespaceNeedInVar/test", namespaceNeedInVarTestFolder) + th.WriteF("/namespaceNeedInVar/test/elasticsearch-test-service.yaml", namespaceNeedInVarTestResources) + th.WriteK("/namespaceNeedInVar/workaround", ` +resources: +- ../dev +- ../test +`) + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) + } + th.AssertActualEqualsExpected(m, namespaceNeedInVarExpectedOutput) +}