diff --git a/pkg/accumulator/resaccumulator.go b/pkg/accumulator/resaccumulator.go index cf5825bc99..b689136e88 100644 --- a/pkg/accumulator/resaccumulator.go +++ b/pkg/accumulator/resaccumulator.go @@ -6,10 +6,12 @@ package accumulator import ( "fmt" "log" + "reflect" "strings" "sigs.k8s.io/kustomize/v3/pkg/resid" "sigs.k8s.io/kustomize/v3/pkg/resmap" + "sigs.k8s.io/kustomize/v3/pkg/resource" "sigs.k8s.io/kustomize/v3/pkg/transformers" "sigs.k8s.io/kustomize/v3/pkg/transformers/config" "sigs.k8s.io/kustomize/v3/pkg/types" @@ -42,6 +44,36 @@ func (ra *ResAccumulator) Vars() []types.Var { return ra.varSet.AsSlice() } +// RemoveIds removes resources of the internal resMap which are member +// of the resId slice past in parameter. +// The removed Resources are returned by the method. +func (ra *ResAccumulator) RemoveConflicts(rightResources []*resource.Resource) ([]*resource.Resource, error) { + conflicting := []*resource.Resource{} + for _, rightResource := range rightResources { + rightId := rightResource.CurId() + leftResources := ra.resMap.GetMatchingResourcesByCurrentId(rightId.Equals) + + if len(leftResources) == 0 { + // no conflict + continue + } + + if len(leftResources) != 1 || !reflect.DeepEqual(leftResources[0], rightResource) { + // conflict detected. More than one resource or left and right are different. + conflicting = append(conflicting, leftResources...) + } + + // Remove the resource from that resMap + err := ra.resMap.Remove(rightId) + if err != nil { + return nil, err + } + } + return conflicting, nil +} + +// AllIds returns all CurrentIds. + func (ra *ResAccumulator) AppendAll( resources resmap.ResMap) error { return ra.resMap.AppendAll(resources) diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index 9c9e194fc7..129f10f42f 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -215,8 +215,16 @@ func (m *resWrangler) Resources() []*resource.Resource { func (m *resWrangler) Append(res *resource.Resource) error { id := res.CurId() if r := m.GetMatchingResourcesByCurrentId(id.Equals); len(r) > 0 { - return fmt.Errorf( - "may not add resource with an already registered id: %s", id) + // This allows diamond import of resources. + // We can assert the same resource as been loaded because there is + // only one resource in the list with the same name and + // value of the resource we intend to append is identical to the one + // we already added to the list. + if len(r) > 1 || !res.Equals(r[0]) { + return fmt.Errorf( + "may not add resource with an already registered id: %s", id) + } + return nil } m.rList = append(m.rList, res) return nil diff --git a/pkg/resmap/resmap_test.go b/pkg/resmap/resmap_test.go index babe8cb20b..5616ea96ad 100644 --- a/pkg/resmap/resmap_test.go +++ b/pkg/resmap/resmap_test.go @@ -45,6 +45,21 @@ func makeCm(i int) *resource.Resource { }) } +// Make a resource with a predictable name and label. +func makeCmWithLabel(i int, label string) *resource.Resource { + return rf.FromMap( + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": fmt.Sprintf("cm%03d", i), + "labels": map[string]interface{}{ + "key1": label, + }, + }, + }) +} + func TestAppendRemove(t *testing.T) { w1 := New() doAppend(t, w1, makeCm(1)) @@ -69,7 +84,7 @@ func TestAppendRemove(t *testing.T) { t.Fatalf("mismatch") } - err := w2.Append(makeCm(6)) + err := w2.Append(makeCmWithLabel(6, "trigger-a-conflict")) if err == nil { t.Fatalf("expected error") } @@ -98,6 +113,7 @@ func TestRemove(t *testing.T) { func TestReplace(t *testing.T) { cm5 := makeCm(5) + cm5WithLabel := makeCmWithLabel(5, "trigger-a-conflict") cm700 := makeCm(700) otherCm5 := makeCm(5) @@ -121,7 +137,10 @@ func TestReplace(t *testing.T) { if r, err := w.GetByCurrentId(cm5.OrgId()); err != nil || r != otherCm5 { t.Fatalf("unexpected result r=%s, err=%v", r.CurId(), err) } - if err := w.Append(cm5); err == nil { + if err := w.Append(cm5); err != nil { + t.Fatalf("unexpected err: %v", err) + } + if err := w.Append(cm5WithLabel); err == nil { t.Fatalf("expected id already there error") } if err := w.Remove(cm5.OrgId()); err != nil { diff --git a/pkg/target/diamondcomposition_test.go b/pkg/target/diamondcomposition_test.go index 137f2e21cb..40afa83083 100644 --- a/pkg/target/diamondcomposition_test.go +++ b/pkg/target/diamondcomposition_test.go @@ -116,13 +116,19 @@ patchesStrategicMerge: // of it, without using any of the `namePrefix`, `nameSuffix` or `namespace` // kustomization keywords. // -// composite -// / | \ -// probe dns restart -// \ | / -// base +// composite +// / | \ +// 1.probe 2.dns 3.restart +// \ | / +// base // -func TestIssue1251_CompositeDiamond_Failure(t *testing.T) { +// +// The generation flow relies on the order of the resources in the resources field +// of the kustomization.yaml +// 1. Customize base (no-op), customize probe deployment (patch) and add it to composite deployment +// 2. Customise base again (no-op), customize dns deployment (patch) and merge it into composite deployment +// 3. Customize base again (no-op), customize restart deployment (patch) and merge it into composite deployment +func TestIssue1251_CompositeDiamond_ProbeRestartDns_Order(t *testing.T) { th := kusttest_test.NewKustTestHarness(t, "/app/composite") writeDeploymentBase(th) writeProbeOverlay(th) @@ -136,14 +142,59 @@ resources: - ../restart `) - _, err := th.MakeKustTarget().MakeCustomizedResMap() - if err == nil { - t.Fatalf("Expected resource accumulation error") + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) } - if !strings.Contains( - err.Error(), "already registered id: apps_v1_Deployment|~X|my-deployment") { - t.Fatalf("Unexpected err: %v", err) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-deployment +spec: + template: + spec: + containers: + - image: my-image + livenessProbe: + httpGet: + path: /healthz + port: 8080 + name: my-deployment + dnsPolicy: None + restartPolicy: Always +`) +} + +// +// composite +// / | \ +// 1.probe 2.restart 3.dns +// \ | / +// base +// +// 1. Customize base (no-op), customize probe deployment (patch) and add it to composite deployment +// 2. Customise base again (no-op), customize dns deployment (patch) and merge it into composite deployment +// 3. Customize base again (no-op), customize restart deployment (patch) and merge it into composite deployment +func TestIssue1251_CompositeDiamond_ProbeDnsRestart_Order(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/app/composite") + writeDeploymentBase(th) + writeProbeOverlay(th) + writeDNSOverlay(th) + writeRestartOverlay(th) + + th.WriteK("/app/composite", ` +resources: +- ../probe +- ../restart +- ../dns +`) + + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) } + th.AssertActualEqualsExpected(m, expectedPatchedDeployment) } const expectedPatchedDeployment = ` diff --git a/pkg/target/diamonds_test.go b/pkg/target/diamonds_test.go index be67553654..4ab821f45d 100644 --- a/pkg/target/diamonds_test.go +++ b/pkg/target/diamonds_test.go @@ -222,3 +222,244 @@ metadata: name: prod-t-federation `) } + +// This example demonstrate a simple sharing +// of a configmap and variables between +// component1 and component2 before being +// aggregated into myapp +// +// myapp +// / | \ +// component1 | component2 +// \ | / +// common +// + +type diamonImportTest struct{} + +func (ut *diamonImportTest) writeKustCommon(th *kusttest_test.KustTestHarness) { + th.WriteK("/diamondimport/common/", ` +resources: +- configmap.yaml + +vars: +- name: ConfigMap.global.data.user + objref: + apiVersion: v1 + kind: ConfigMap + name: global + fieldref: + fieldpath: data.user +`) +} +func (ut *diamonImportTest) writeKustComponent2(th *kusttest_test.KustTestHarness) { + th.WriteK("/diamondimport/component2/", ` +resources: +- ../common +- deployment.yaml +`) +} +func (ut *diamonImportTest) writeKustComponent1(th *kusttest_test.KustTestHarness) { + th.WriteK("/diamondimport/component1/", ` +resources: +- ../common +- deployment.yaml +`) +} +func (ut *diamonImportTest) writeKustMyApp(th *kusttest_test.KustTestHarness) { + th.WriteK("/diamondimport/myapp/", ` +resources: +- ../common +- ../component1 +- ../component2 +`) +} +func (ut *diamonImportTest) writeCommonConfigMap(th *kusttest_test.KustTestHarness) { + th.WriteF("/diamondimport/common/configmap.yaml", ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: global +data: + settings: | + database: mydb + port: 3000 + user: myuser +`) +} +func (ut *diamonImportTest) writeComponent2(th *kusttest_test.KustTestHarness) { + th.WriteF("/diamondimport/component2/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: component2 + labels: + app: component2 +spec: + replicas: 1 + selector: + matchLabels: + app: component2 + template: + metadata: + labels: + app: component2 + spec: + containers: + - name: component2 + image: k8s.gcr.io/busybox + env: + - name: APP_USER + value: $(ConfigMap.global.data.user) + command: [ "/bin/sh", "-c", "cat /etc/config/component2 && sleep 60" ] + volumeMounts: + - name: config-volume + mountPath: /etc/config + volumes: + - name: config-volume + configMap: + name: global + items: + - key: settings + path: component2 +`) +} +func (ut *diamonImportTest) writeComponent1(th *kusttest_test.KustTestHarness) { + th.WriteF("/diamondimport/component1/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: component1 + labels: + app: component1 +spec: + replicas: 1 + selector: + matchLabels: + app: component1 + template: + metadata: + labels: + app: component1 + spec: + containers: + - name: component1 + image: k8s.gcr.io/busybox + env: + - name: APP_USER + value: $(ConfigMap.global.data.user) + command: [ "/bin/sh", "-c", "cat /etc/config/component1 && sleep 60" ] + volumeMounts: + - name: config-volume + mountPath: /etc/config + volumes: + - name: config-volume + configMap: + name: global + items: + - key: settings + path: component1 +`) +} +func TestSimpleDiamondImport(t *testing.T) { + ut := &diamonImportTest{} + th := kusttest_test.NewKustTestHarness(t, "/diamondimport/myapp") + ut.writeKustCommon(th) + ut.writeKustComponent1(th) + ut.writeKustComponent2(th) + ut.writeKustMyApp(th) + ut.writeCommonConfigMap(th) + ut.writeComponent1(th) + ut.writeComponent2(th) + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + // Error before this Resource.Append fix. + // may not add resource with an already registered id: ~G_v1_ConfigMap|~X|global + t.Fatalf("Err: %v", err) + } + th.AssertActualEqualsExpected(m, ` +apiVersion: v1 +data: + settings: | + database: mydb + port: 3000 + user: myuser +kind: ConfigMap +metadata: + name: global +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: component1 + name: component1 +spec: + replicas: 1 + selector: + matchLabels: + app: component1 + template: + metadata: + labels: + app: component1 + spec: + containers: + - command: + - /bin/sh + - -c + - cat /etc/config/component1 && sleep 60 + env: + - name: APP_USER + value: myuser + image: k8s.gcr.io/busybox + name: component1 + volumeMounts: + - mountPath: /etc/config + name: config-volume + volumes: + - configMap: + items: + - key: settings + path: component1 + name: global + name: config-volume +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: component2 + name: component2 +spec: + replicas: 1 + selector: + matchLabels: + app: component2 + template: + metadata: + labels: + app: component2 + spec: + containers: + - command: + - /bin/sh + - -c + - cat /etc/config/component2 && sleep 60 + env: + - name: APP_USER + value: myuser + image: k8s.gcr.io/busybox + name: component2 + volumeMounts: + - mountPath: /etc/config + name: config-volume + volumes: + - configMap: + items: + - key: settings + path: component2 + name: global + name: config-volume +`) +} diff --git a/pkg/target/kusttarget.go b/pkg/target/kusttarget.go index 42215cc928..5d78142489 100644 --- a/pkg/target/kusttarget.go +++ b/pkg/target/kusttarget.go @@ -330,6 +330,24 @@ func (kt *KustTarget) configureExternalTransformers() ([]transformers.Transforme return kt.pLdr.LoadTransformers(kt.ldr, ra.ResMap()) } +func (kt *KustTarget) solveConflicts(ra *accumulator.ResAccumulator, subRa *accumulator.ResAccumulator) error { + conflicting, err := subRa.RemoveConflicts(ra.ResMap().Resources()) + if err != nil { + return errors.Wrapf( + err, "converting diamond imported resources into patches %v", + subRa.ResMap().AllIds()) + } + + if len(conflicting) == 0 { + return nil + } + t, err := kt.tFactory.MakePatchTransformer((conflicting), kt.rFactory.RF()) + if err != nil { + return err + } + return ra.Transform(t) +} + // accumulateResources fills the given resourceAccumulator // with resources read from the given list of paths. func (kt *KustTarget) accumulateResources( @@ -364,6 +382,11 @@ func (kt *KustTarget) accumulateDirectory( return errors.Wrapf( err, "recursed accumulation of path '%s'", path) } + err = subKt.solveConflicts(ra, subRa) + if err != nil { + return errors.Wrapf( + err, "recursed merging from path '%s'", path) + } err = ra.MergeAccumulator(subRa) if err != nil { return errors.Wrapf( diff --git a/pkg/target/variableref_test.go b/pkg/target/variableref_test.go index e4ba55aa4b..9608e789bb 100644 --- a/pkg/target/variableref_test.go +++ b/pkg/target/variableref_test.go @@ -313,6 +313,13 @@ vars: name: myServerPod fieldref: fieldpath: metadata.name +- name: IMAGE_NAME + objref: + apiVersion: v1 + kind: Pod + name: myServerPod + fieldref: + fieldpath: spec.containers[0].image `) th.WriteF("/app/base/pod.yaml", ` apiVersion: v1 @@ -326,6 +333,8 @@ spec: env: - name: POD_NAME value: $(POD_NAME) + - name: IMAGE_NAME + value: $(IMAGE_NAME) `) th.WriteK("/app/o1", ` nameprefix: p1- @@ -343,7 +352,7 @@ resources: - ../o2 `) - const presumablyDesired = ` + const pod2StillBoggus = ` apiVersion: v1 kind: Pod metadata: @@ -353,6 +362,8 @@ spec: - env: - name: POD_NAME value: p1-base-myServerPod + - name: IMAGE_NAME + value: whatever image: whatever name: myServer --- @@ -364,17 +375,17 @@ spec: containers: - env: - name: POD_NAME - value: p2-base-myServerPod + value: p1-base-myServerPod + - name: IMAGE_NAME + value: whatever image: whatever name: myServer ` - _, err := th.MakeKustTarget().MakeCustomizedResMap() - if err == nil { - t.Fatalf("should have an error") - } - if !strings.Contains(err.Error(), "var 'POD_NAME' already encountered") { - t.Fatalf("unexpected err: %v", err) + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) } + th.AssertActualEqualsExpected(m, pod2StillBoggus) } func TestVarRefBig(t *testing.T) { diff --git a/pkg/types/var.go b/pkg/types/var.go index 4b70caf7fd..9a48a81110 100644 --- a/pkg/types/var.go +++ b/pkg/types/var.go @@ -18,6 +18,7 @@ package types import ( "fmt" + "reflect" "sort" "strings" @@ -125,11 +126,16 @@ func (vs *VarSet) MergeSlice(incoming []Var) error { // Merge absorbs another Var with error on name collision. // Empty fields in incoming Var is defaulted. func (vs *VarSet) Merge(v Var) error { + v.defaulting() if vs.Contains(v) { - return fmt.Errorf( - "var '%s' already encountered", v.Name) + // Only return an error if a variable with the same + // name already exists + if !reflect.DeepEqual(v, vs.set[v.Name]) { + return fmt.Errorf( + "var '%s' already encountered", v.Name) + } + return nil } - v.defaulting() vs.set[v.Name] = v return nil } diff --git a/pkg/types/var_test.go b/pkg/types/var_test.go index 95f799e020..463ec7cb88 100644 --- a/pkg/types/var_test.go +++ b/pkg/types/var_test.go @@ -123,16 +123,16 @@ func TestVarSet(t *testing.T) { t.Fatalf("set %v should contain var %v", set.AsSlice(), v) } } + set2 := NewVarSet() err = set2.MergeSet(set) if err != nil { t.Fatalf("unexpected err: %v", err) } + // Attempt to merge variables that have already been imported + // This happens during diamond kustomize imports err = set2.MergeSlice(vars) - if err == nil { - t.Fatalf("expected err") - } - if !strings.Contains(err.Error(), "var 'SHELLVARS' already encountered") { + if err != nil { t.Fatalf("unexpected err: %v", err) } v := set2.Get("BACKEND") @@ -152,6 +152,87 @@ func TestVarSet(t *testing.T) { } } +func TestVarSetMergeConflicts(t *testing.T) { + set := NewVarSet() + vars := []Var{ + { + Name: "SHELLVARS", + ObjRef: Target{ + APIVersion: "v7", + Gvk: gvk.Gvk{Kind: "ConfigMap"}, + Name: "bash"}, + }, + { + Name: "AWARD", + ObjRef: Target{ + APIVersion: "v7", + Gvk: gvk.Gvk{Kind: "Service"}, + Name: "nobelPrize"}, + FieldRef: FieldSelector{FieldPath: "some.arbitrary.path"}, + }, + } + nodefault := []Var{ + { + Name: "SHELLVARS", + ObjRef: Target{ + APIVersion: "v7", + Gvk: gvk.Gvk{Kind: "ConfigMap"}, + Name: "bash"}, + FieldRef: FieldSelector{FieldPath: "metadata.name"}, + }, + } + conflict1 := []Var{ + { + Name: "SHELLVARS", + ObjRef: Target{ + APIVersion: "v7", + Gvk: gvk.Gvk{Kind: "ConfigMap"}, + Name: "zsh"}, + }, + } + conflict2 := []Var{ + { + Name: "AWARD", + ObjRef: Target{ + APIVersion: "v7", + Gvk: gvk.Gvk{Kind: "Deployment"}, + Name: "nobelPrize"}, + FieldRef: FieldSelector{FieldPath: "some.arbitrary.path"}, + }, + } + err := set.MergeSlice(vars) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + set2 := NewVarSet() + err = set2.MergeSet(set) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + // Check that the defaulting for the FieldRef field during variable loading + // does not interfere with the Merging behavior + err = set2.MergeSlice(nodefault) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + // Attempt to merge a variable with different Target.Name and default FieldRef + err = set2.MergeSlice(conflict1) + if err == nil { + t.Fatalf("expected err") + } + if !strings.Contains(err.Error(), "var 'SHELLVARS' already encountered") { + t.Fatalf("unexpected err: %v", err) + } + // Attempt to merge a variable with different Target.Name and a same FieldRef + err = set2.MergeSlice(conflict2) + if err == nil { + t.Fatalf("expected err") + } + if !strings.Contains(err.Error(), "var 'AWARD' already encountered") { + t.Fatalf("unexpected err: %v", err) + } +} + func TestVarSetCopy(t *testing.T) { set1 := NewVarSet() vars := []Var{