From dd5674fe6b8393e51e78dff989e82ca0d7e8ad02 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sun, 7 Jul 2019 11:19:20 -0500 Subject: [PATCH 1/2] ResId.Equals usable for VariableRef. - Namespace need objRef field in variable declaration - Add namespace conflict test for variables The replacement of ResId.GkvnEquals reference by ResId.Equals highligthed the fact it is no possible yet when looking for variable targets because the namespace field is not allowed yet. This commit adds two tests to the namespaces_test.go regarding that use case. --- pkg/accumulator/resaccumulator.go | 2 + pkg/target/namespaces_test.go | 235 ++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+) diff --git a/pkg/accumulator/resaccumulator.go b/pkg/accumulator/resaccumulator.go index cf5825bc9..f22b43c38 100644 --- a/pkg/accumulator/resaccumulator.go +++ b/pkg/accumulator/resaccumulator.go @@ -64,6 +64,8 @@ func (ra *ResAccumulator) GetTransformerConfig() *config.TransformerConfig { func (ra *ResAccumulator) MergeVars(incoming []types.Var) error { for _, v := range incoming { + // TODO(jeb): Do not change GvknEquals to Equals until the + // namespace is part of the variable declaration. 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..5ec603ecd 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,237 @@ rules: - get `) } + +const ambiguousvarMyApp 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 ambiguousvarDevResources 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 ambiguousvarTestResources 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 ambiguousvarExpectedOutput 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, "/ambiguousvar/myapp") + th.WriteK("/ambiguousvar/myapp", ambiguousvarMyApp) + th.WriteF("/ambiguousvar/myapp/elasticsearch-dev-service.yaml", ambiguousvarDevResources) + th.WriteF("/ambiguousvar/myapp/elasticsearch-test-service.yaml", ambiguousvarTestResources) + _, 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 ambiguousvarDevFolder 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 ambiguousvarTestFolder 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, "/ambiguousvar/workaround") + th.WriteK("/ambiguousvar/dev", ambiguousvarDevFolder) + th.WriteF("/ambiguousvar/dev/elasticsearch-dev-service.yaml", ambiguousvarDevResources) + th.WriteK("/ambiguousvar/test", ambiguousvarTestFolder) + th.WriteF("/ambiguousvar/test/elasticsearch-test-service.yaml", ambiguousvarTestResources) + th.WriteK("/ambiguousvar/workaround", ` +resources: +- ../dev +- ../test +`) + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) + } + th.AssertActualEqualsExpected(m, ambiguousvarExpectedOutput) +} From 0bec7b996bcb39ef96e6e4e3ae8db27c954a191d Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Thu, 18 Jul 2019 15:53:19 -0500 Subject: [PATCH 2/2] Code review implementation for namespace needed in vars --- pkg/accumulator/resaccumulator.go | 12 ++++++++- pkg/target/namespaces_test.go | 41 ++++++++++++++++++------------- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/pkg/accumulator/resaccumulator.go b/pkg/accumulator/resaccumulator.go index f22b43c38..e7e460c59 100644 --- a/pkg/accumulator/resaccumulator.go +++ b/pkg/accumulator/resaccumulator.go @@ -64,8 +64,18 @@ func (ra *ResAccumulator) GetTransformerConfig() *config.TransformerConfig { func (ra *ResAccumulator) MergeVars(incoming []types.Var) error { for _, v := range incoming { - // TODO(jeb): Do not change GvknEquals to Equals until the + // 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 5ec603ecd..481cdf2cc 100644 --- a/pkg/target/namespaces_test.go +++ b/pkg/target/namespaces_test.go @@ -95,7 +95,14 @@ rules: `) } -const ambiguousvarMyApp string = ` +// 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 @@ -130,7 +137,7 @@ vars: fieldpath: spec.ports[0].protocol ` -const ambiguousvarDevResources string = ` +const namespaceNeedInVarDevResources string = ` apiVersion: apps/v1 kind: StatefulSet metadata: @@ -160,7 +167,7 @@ spec: clusterIP: None ` -const ambiguousvarTestResources string = ` +const namespaceNeedInVarTestResources string = ` apiVersion: apps/v1 kind: StatefulSet metadata: @@ -190,7 +197,7 @@ spec: clusterIP: None ` -const ambiguousvarExpectedOutput string = ` +const namespaceNeedInVarExpectedOutput string = ` apiVersion: apps/v1 kind: StatefulSet metadata: @@ -255,10 +262,10 @@ spec: // 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, "/ambiguousvar/myapp") - th.WriteK("/ambiguousvar/myapp", ambiguousvarMyApp) - th.WriteF("/ambiguousvar/myapp/elasticsearch-dev-service.yaml", ambiguousvarDevResources) - th.WriteF("/ambiguousvar/myapp/elasticsearch-test-service.yaml", ambiguousvarTestResources) + 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") @@ -268,7 +275,7 @@ func TestVariablesAmbiguous(t *testing.T) { } } -const ambiguousvarDevFolder string = ` +const namespaceNeedInVarDevFolder string = ` resources: - elasticsearch-dev-service.yaml vars: @@ -288,7 +295,7 @@ vars: fieldpath: spec.ports[0].protocol ` -const ambiguousvarTestFolder string = ` +const namespaceNeedInVarTestFolder string = ` resources: - elasticsearch-test-service.yaml vars: @@ -312,12 +319,12 @@ vars: // 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, "/ambiguousvar/workaround") - th.WriteK("/ambiguousvar/dev", ambiguousvarDevFolder) - th.WriteF("/ambiguousvar/dev/elasticsearch-dev-service.yaml", ambiguousvarDevResources) - th.WriteK("/ambiguousvar/test", ambiguousvarTestFolder) - th.WriteF("/ambiguousvar/test/elasticsearch-test-service.yaml", ambiguousvarTestResources) - th.WriteK("/ambiguousvar/workaround", ` + 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 @@ -326,5 +333,5 @@ resources: if err != nil { t.Fatalf("Err: %v", err) } - th.AssertActualEqualsExpected(m, ambiguousvarExpectedOutput) + th.AssertActualEqualsExpected(m, namespaceNeedInVarExpectedOutput) }