From dd7925d07b7ca31d2031b4b1ad25a176d8ad95f4 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Tue, 25 Jun 2019 15:18:04 -0500 Subject: [PATCH] Diamond Import Allow diamond structure of kustomize import - This situation happens in project which try to aggregate multiple components in different folders sharing the same base folder. - This commit demonstrates how a common resource can be included accross different components. - Address conflicting bases by converting resources into patches - Improve variables diamond conflicts detection Add and update tests around diamond import of folders. 1. Test simple import of common resources. 2. Test simple import of common variables. 3. Test simple import of variables without their target resources in the parent folders. 4. Test that the algorithm is reporting proper errors in case of not supported diamond import of resources. 5. Test that the algorithm is reporting proper errors in case of not supported diamond import of variables. 6. Update errors messages in existing tests. Prepare diamond import feature for upcoming high level merge feature. Add unit tests for resaccumulator Improve variable conflict detection. Fix bug 506 --- pkg/accumulator/resaccumulator.go | 218 +++++++++++++++++++--- pkg/accumulator/resaccumulator_test.go | 210 ++++++++++++++++++++- pkg/resmap/varmap.go | 69 +++++++ pkg/target/complexcomposition_test.go | 4 +- pkg/target/diamondcomposition_test.go | 133 +++++++++++++- pkg/target/diamonds_test.go | 241 +++++++++++++++++++++++++ pkg/target/kusttarget.go | 93 +++++++++- pkg/target/kusttarget_configplugin.go | 14 +- pkg/target/variableref_test.go | 196 ++++++++++++++++---- pkg/transformers/refvars.go | 14 +- pkg/transformers/refvars_test.go | 2 +- 11 files changed, 1109 insertions(+), 85 deletions(-) create mode 100644 pkg/resmap/varmap.go diff --git a/pkg/accumulator/resaccumulator.go b/pkg/accumulator/resaccumulator.go index 7ebfc04bdb9..4225b0751bb 100644 --- a/pkg/accumulator/resaccumulator.go +++ b/pkg/accumulator/resaccumulator.go @@ -6,10 +6,13 @@ package accumulator import ( "fmt" "log" + "reflect" "strings" + "sigs.k8s.io/kustomize/v3/pkg/gvk" "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" @@ -19,9 +22,11 @@ import ( // used to customize those resources. It's a ResMap // plus stuff needed to modify the ResMap. type ResAccumulator struct { - resMap resmap.ResMap - tConfig *config.TransformerConfig - varSet types.VarSet + resMap resmap.ResMap + tConfig *config.TransformerConfig + varSet types.VarSet + patchSet []types.Patch + unresolvedVars types.VarSet } func MakeEmptyAccumulator() *ResAccumulator { @@ -29,6 +34,8 @@ func MakeEmptyAccumulator() *ResAccumulator { ra.resMap = resmap.New() ra.tConfig = &config.TransformerConfig{} ra.varSet = types.NewVarSet() + ra.patchSet = []types.Patch{} + ra.unresolvedVars = types.NewVarSet() return ra } @@ -39,7 +46,85 @@ func (ra *ResAccumulator) ResMap() resmap.ResMap { // Vars returns a copy of underlying vars. func (ra *ResAccumulator) Vars() []types.Var { - return ra.varSet.AsSlice() + completeset := types.NewVarSet() + completeset.AbsorbSet(ra.varSet) + completeset.AbsorbSet(ra.unresolvedVars) + return completeset.AsSlice() +} + +// accumlatePatch accumulates the information regarding conflicting +// resources as patches. +func (ra *ResAccumulator) accumlatePatch(id resid.ResId, conflicting ...*resource.Resource) error { + target := types.Selector{ + Gvk: gvk.Gvk{ + Group: id.Group, + Version: id.Version, + Kind: id.Kind, + }, + Namespace: id.Namespace, + Name: id.Name, + AnnotationSelector: "", + LabelSelector: "", + } + + for _, res := range conflicting { + out, err := res.AsYAML() + if err != nil { + return err + } + newPatch := types.Patch{ + Path: "", + Patch: string(out), + Target: &target, + } + ra.patchSet = append(ra.patchSet, newPatch) + } + return nil +} + +// HandoverConflictingResources removes conflicting resources from the local accumulator +// and add the conflicting resources list in the other accumulator. +// Conflicting is defined as have the same CurrentId but different Values. +func (ra *ResAccumulator) HandoverConflictingResources(other *ResAccumulator) error { + for _, rightResource := range other.ResMap().Resources() { + rightId := rightResource.CurId() + leftResources := ra.resMap.GetMatchingResourcesByCurrentId(rightId.Equals) + + if len(leftResources) == 0 { + // no conflict + continue + } + + // TODO(jeb): Not sure we want to use DeepEqual here since there are some fields + // which are artifacts (nameprefix, namesuffix, refvar, refby) added to the resources + // by the algorithm here. + // Also we may be dropping here some of the artifacts (nameprefix, namesuffix,...) + // during the conversion from Resource/ResMap to Patch. + if len(leftResources) != 1 || !reflect.DeepEqual(leftResources[0], rightResource) { + // conflict detected. More than one resource or left and right are different. + if err := other.accumlatePatch(rightId, rightResource); err != nil { + return err + } + if err := other.accumlatePatch(rightId, leftResources...); err != nil { + return err + } + } + + // Remove the resource from that resMap + err := ra.resMap.Remove(rightId) + if err != nil { + return err + } + } + + return nil +} + +// PatchSet return the list of resources that have been +// put aside. It will let the PatchTransformer decide how to handle +// the conflict, assuming the Transformer can. +func (ra *ResAccumulator) PatchSet() []types.Patch { + return ra.patchSet } func (ra *ResAccumulator) AppendAll( @@ -62,8 +147,61 @@ func (ra *ResAccumulator) GetTransformerConfig() *config.TransformerConfig { return ra.tConfig } +// AppendUnresolvedVars accumulates the non conflicting and unresolved variables +// from one accumulator into another. Returns an error if a conflict is detected. +func (ra *ResAccumulator) AppendUnresolvedVars(otherSet types.VarSet) error { + return ra.unresolvedVars.AbsorbSet(otherSet) +} + +// AppendResolvedVars accumulates the non conflicting and resolved variables +// from one accumulator into another. Returns an error is a conflict is detected. +func (ra *ResAccumulator) AppendResolvedVars(otherSet types.VarSet) error { + mergeableVars := []types.Var{} + for _, v := range otherSet.AsSlice() { + conflicting := ra.varSet.Get(v.Name) + if conflicting == nil { + // no conflict. The var is valid. + mergeableVars = append(mergeableVars, v) + continue + } + + if !v.DeepEqual(*conflicting) { + // two vars with the same name are pointing at two + // different resources. + return fmt.Errorf( + "var '%s' already encountered", v.Name) + } + + // Behavior has been modified. + // Conflict detection moved to VarMap object + // ============================================= + // matched := ra.resMap.GetMatchingResourcesByOriginalId( + // resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals) + // if len(matched) > 1 { + // // We detected a diamond import of kustomization + // // context where one variable pointing at one resource + // // in each context is now pointing at two resources + // // (different CurrId) because the two contexts have + // // been merged. + // return fmt.Errorf( + // "found %d resId matches for var %s "+ + // "(unable to disambiguate)", + // len(matched), v) + // } + } + return ra.varSet.MergeSlice(mergeableVars) +} + func (ra *ResAccumulator) MergeVars(incoming []types.Var) error { - for _, v := range incoming { + // Absorb the new slice of vars into the previously + // unresolved vars (from the base folders) + toresolve := ra.unresolvedVars.Copy() + if err := toresolve.AbsorbSlice(incoming); err != nil { + return err + } + ra.unresolvedVars = types.NewVarSet() + + for _, v := range toresolve.AsSlice() { targetId := resid.NewResIdWithNamespace(v.ObjRef.GVK(), v.ObjRef.Name, v.ObjRef.Namespace) idMatcher := targetId.GvknEquals if targetId.Namespace != "" || !targetId.IsNamespaceableKind() { @@ -72,17 +210,35 @@ func (ra *ResAccumulator) MergeVars(incoming []types.Var) error { idMatcher = targetId.Equals } matched := ra.resMap.GetMatchingResourcesByOriginalId(idMatcher) - if len(matched) > 1 { - return fmt.Errorf( - "found %d resId matches for var %s "+ - "(unable to disambiguate)", - len(matched), v) + + // Behavior has been modified. + // Conflict detection moved to VarMap object + // ============================================= + // if len(matched) > 1 { + // return fmt.Errorf( + // "found %d resId matches for var %s "+ + // "(unable to disambiguate)", + // len(matched), v) + // } + + if len(matched) == 0 { + // no associated resources yet. + if err := ra.unresolvedVars.Absorb(v); err != nil { + return err + } + continue } - if len(matched) == 1 { - matched[0].AppendRefVarName(v) + + // Found one or more associated resource. + if err := ra.varSet.Absorb(v); err != nil { + return err + } + for _, match := range matched { + match.AppendRefVarName(v) } } - return ra.varSet.MergeSlice(incoming) + + return nil } func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) { @@ -94,42 +250,50 @@ func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) { if err != nil { return err } - return ra.varSet.MergeSet(other.varSet) + err = ra.AppendUnresolvedVars(other.unresolvedVars) + if err != nil { + return err + } + return ra.AppendResolvedVars(other.varSet) } -func (ra *ResAccumulator) findVarValueFromResources(v types.Var) (interface{}, error) { +func (ra *ResAccumulator) findVarValueFromResources(v types.Var, varMap *resmap.VarMap) error { + foundCount := 0 for _, res := range ra.resMap.Resources() { for _, varName := range res.GetRefVarNames() { if varName == v.Name { s, err := res.GetFieldValue(v.FieldRef.FieldPath) if err != nil { - return "", fmt.Errorf( + return fmt.Errorf( "field specified in var '%v' "+ "not found in corresponding resource", v) } - return s, nil + foundCount++ + varMap.Append(v.Name, res, s) } } } - return "", fmt.Errorf( - "var '%v' cannot be mapped to a field "+ - "in the set of known resources", v) + if foundCount == 0 { + return fmt.Errorf( + "var '%v' cannot be mapped to a field "+ + "in the set of known resources", v) + } + + return nil } // makeVarReplacementMap returns a map of Var names to // their final values. The values are strings intended // for substitution wherever the $(var.Name) occurs. -func (ra *ResAccumulator) makeVarReplacementMap() (map[string]interface{}, error) { - result := map[string]interface{}{} +func (ra *ResAccumulator) makeVarReplacementMap() (resmap.VarMap, error) { + result := resmap.NewEmptyVarMap() for _, v := range ra.Vars() { - s, err := ra.findVarValueFromResources(v) + err := ra.findVarValueFromResources(v, &result) if err != nil { - return nil, err + return result, err } - - result[v.Name] = s } return result, nil @@ -144,7 +308,7 @@ func (ra *ResAccumulator) ResolveVars() error { if err != nil { return err } - if len(replacementMap) == 0 { + if len(replacementMap.VarNames()) == 0 { return nil } t := transformers.NewRefVarTransformer( diff --git a/pkg/accumulator/resaccumulator_test.go b/pkg/accumulator/resaccumulator_test.go index 70f0cb2dad1..eaac6e6170c 100644 --- a/pkg/accumulator/resaccumulator_test.go +++ b/pkg/accumulator/resaccumulator_test.go @@ -14,7 +14,7 @@ import ( . "sigs.k8s.io/kustomize/v3/pkg/accumulator" "sigs.k8s.io/kustomize/v3/pkg/gvk" "sigs.k8s.io/kustomize/v3/pkg/resmap" - "sigs.k8s.io/kustomize/v3/pkg/resmaptest" + resmaptest_test "sigs.k8s.io/kustomize/v3/pkg/resmaptest" "sigs.k8s.io/kustomize/v3/pkg/resource" "sigs.k8s.io/kustomize/v3/pkg/transformers/config" "sigs.k8s.io/kustomize/v3/pkg/types" @@ -170,13 +170,20 @@ func TestResolveVarsVarNeedsDisambiguation(t *testing.T) { }, }, }) - if err == nil { + if err != nil { t.Fatalf("expected error") } - if !strings.Contains( - err.Error(), "unable to disambiguate") { - t.Fatalf("unexpected err: %v", err) - } + + // Behavior has been modified. + // Conflict detection moved to VarMap object + // ============================================= + // if err == nil { + // t.Fatalf("expected error") + // } + // if !strings.Contains( + // err.Error(), "unable to disambiguate") { + // t.Fatalf("unexpected err: %v", err) + // } } func TestResolveVarsGoodResIdBadField(t *testing.T) { @@ -308,6 +315,197 @@ func TestResolveVarsWithNoambiguation(t *testing.T) { } } +func makeResAccumulatorWithPod(t *testing.T, podName, containerName string) *ResAccumulator { + ra := MakeEmptyAccumulator() + rf := resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl()) + err := ra.AppendAll( + resmaptest_test.NewRmBuilder(t, rf). + Add(map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": podName, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": []interface{}{ + containerName, + }, + }, + }, + }}).ResMap()) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + return ra +} + +func TestHandoverConflictingResourcesNoMatches(t *testing.T) { + ra := makeResAccumulatorWithPod(t, "pod1", "container1") + subRa := makeResAccumulatorWithPod(t, "pod2", "container2") + subRa.HandoverConflictingResources(ra) + if subRa.ResMap().Size() != 1 { + t.Errorf("subRa's ResMap should have 1 item, got %d", subRa.ResMap().Size()) + } +} + +func TestHandoverConflictingResourcesMatchesWithNoConflicts(t *testing.T) { + ra := makeResAccumulatorWithPod(t, "pod1", "container1") + subRa := makeResAccumulatorWithPod(t, "pod1", "container1") + subRa.HandoverConflictingResources(ra) + if subRa.ResMap().Size() != 0 { + t.Errorf("subRa's ResMap should have 0 items, got %d", subRa.ResMap().Size()) + } +} + +func TestHandoverConflictingResourcesMatchesWithConflicts(t *testing.T) { + ra := makeResAccumulatorWithPod(t, "pod1", "container1") + subRa := makeResAccumulatorWithPod(t, "pod1", "container2") + subRa.HandoverConflictingResources(ra) + if len(ra.PatchSet()) != 2 { + t.Errorf("ra's PatchSet should have 2 item, got %d", len(ra.PatchSet())) + } + if subRa.ResMap().Size() != 0 { + t.Errorf("subRa's ResMap should have 0 items, got %d", subRa.ResMap().Size()) + } +} + +func makeServiceVar(varName, serviceName string) types.Var { + return types.Var{ + Name: varName, + ObjRef: types.Target{ + Gvk: gvk.Gvk{ + Version: "v1", + Kind: "Service", + }, + Name: serviceName, + }, + } +} + +func TestAppendResolvedVarsNoConflicts(t *testing.T) { + ra, _ := makeResAccumulator(t) + vars := types.NewVarSet() + vars.MergeSlice([]types.Var{makeServiceVar("SERVICE_ONE", "backendOne")}) + vars.MergeSlice([]types.Var{makeServiceVar("SERVICE_TWO", "backendTwo")}) + + if err := ra.AppendResolvedVars(vars); err != nil { + t.Fatalf("unexpected err: %v", err) + } + expectedLen := len(vars.AsSlice()) + if len(ra.Vars()) != expectedLen { + t.Errorf("ra should have %d vars, got %d", expectedLen, len(ra.Vars())) + } +} + +func TestAppendResolvedVarsWithConflicts(t *testing.T) { + ra, _ := makeResAccumulator(t) + vars1 := types.NewVarSet() + vars1.MergeSlice([]types.Var{makeServiceVar("SERVICE_ONE", "backendOne")}) + vars2 := types.NewVarSet() + vars2.MergeSlice([]types.Var{makeServiceVar("SERVICE_ONE", "backendTwo")}) + + // The first call adds the variable + if err := ra.AppendResolvedVars(vars1); err != nil { + t.Fatalf("unexpected err: %v", err) + } + // The second call adds a another variable with the same name, but a + // different value - this should elicit an error + expectedErrMsg := "var 'SERVICE_ONE' already encountered" + if err := ra.AppendResolvedVars(vars2); err == nil { + t.Fatalf("expected error, but none was received") + } else if err.Error() != expectedErrMsg { + t.Errorf("received the wrong error, expected `%s`, got `%s`", + expectedErrMsg, err.Error()) + } +} + +func TestAppendResolvedVarsDiamondNoConflicts(t *testing.T) { + ra, _ := makeResAccumulator(t) + vars := types.NewVarSet() + vars.MergeSlice([]types.Var{makeServiceVar("SERVICE_ONE", "backendOne")}) + + // The first call adds the variable + if err := ra.AppendResolvedVars(vars); err != nil { + t.Fatalf("unexpected err: %v", err) + } + // The second call adds a another variable with the same name and value. + // Since the variables are the same, this call does nothing + if err := ra.AppendResolvedVars(vars); err != nil { + t.Fatalf("unexpected err: %v", err) + } + + expectedLen := len(vars.AsSlice()) + if len(ra.Vars()) != expectedLen { + t.Errorf("ra should have %d vars, got %d", expectedLen, len(ra.Vars())) + } +} + +func TestAppendResolvedVarsDiamondWithConflicts(t *testing.T) { + ra := MakeEmptyAccumulator() + rf := resource.NewFactory( + kunstruct.NewKunstructuredFactoryImpl()) + // Add some services with "prefixes" + err := ra.AppendAll( + resmaptest_test.NewRmBuilder(t, rf). + AddWithName("backend", + map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]interface{}{ + "name": "app1-backend", + }}). + AddWithName("backend", + map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]interface{}{ + "name": "app2-backend", + }}).ResMap()) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + + serviceVar := types.Var{ + Name: "SERVICE_ONE", + ObjRef: types.Target{ + Gvk: gvk.Gvk{ + Version: "v1", + Kind: "Service", + }, + Name: "backend", + }, + FieldRef: types.FieldSelector{ + FieldPath: "metadata.name", + }, + } + vars := types.NewVarSet() + vars.MergeSlice([]types.Var{serviceVar}) + + // The first call adds the variable without conflict + if err := ra.AppendResolvedVars(vars); err != nil { + t.Fatalf("unexpected err: %v", err) + } + + // Behavior has been modified. + // Conflict detection moved to VarMap object + // ============================================= + // // A second call will result in a conflicting variable name. Further, + // // it will be unclear whether this variable refers to app1-backend or + // // app2-backend. This is an error + // expectedErrMsg := fmt.Sprintf( + // "found %d resId matches for var %s "+ + // "(unable to disambiguate)", + // ra.ResMap().Size(), serviceVar) + // if err := ra.AppendResolvedVars(vars); err == nil { + // t.Fatalf("expected error") + // } else if err.Error() != expectedErrMsg { + // t.Errorf("received the wrong error, expected `%s`, got `%s`", + // expectedErrMsg, err.Error()) + // } +} + func find(name string, resMap resmap.ResMap) *resource.Resource { for _, r := range resMap.Resources() { if r.GetName() == name { diff --git a/pkg/resmap/varmap.go b/pkg/resmap/varmap.go new file mode 100644 index 00000000000..aef1978aece --- /dev/null +++ b/pkg/resmap/varmap.go @@ -0,0 +1,69 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +// Package resmap implements a map from ResId to Resource that +// tracks all resources in a kustomization. +package resmap + +import ( + "fmt" + "sigs.k8s.io/kustomize/v3/pkg/resource" +) + +type varValue struct { + value interface{} + ctx *resource.Resource +} + +type VarMap struct { + varmap map[string][]varValue +} + +func NewEmptyVarMap() VarMap { + return VarMap{varmap: make(map[string][]varValue)} +} + +func NewVarMap(simple map[string]interface{}) VarMap { + varmap := make(map[string][]varValue) + for key, value := range simple { + oneVal := varValue{value: value, ctx: nil} + varmap[key] = []varValue{oneVal} + } + return VarMap{varmap: varmap} +} + +func (vm *VarMap) Append(name string, ctx *resource.Resource, value interface{}) { + if _, found := vm.varmap[name]; !found { + vm.varmap[name] = []varValue{} + } + + vm.varmap[name] = append(vm.varmap[name], varValue{value: value, ctx: ctx}) +} + +func (vm *VarMap) VarNames() []string { + var names []string + for k := range vm.varmap { + names = append(names, k) + } + return names +} + +func (vm *VarMap) SubsetThatCouldBeReferencedByResource( + inputRes *resource.Resource) (map[string]interface{}, error) { + result := map[string]interface{}{} + for varName, varValueSlice := range vm.varmap { + for _, r := range varValueSlice { + rctxm := inputRes.PrefixesSuffixesEquals + if (r.ctx == nil) || (r.ctx.InSameKustomizeCtx(rctxm)) { + if _, found := result[varName]; found { + return result, fmt.Errorf( + "found %d resId matches for var %s "+ + "(unable to disambiguate)", + 2, varName) + } + result[varName] = r.value + } + } + } + return result, nil +} diff --git a/pkg/target/complexcomposition_test.go b/pkg/target/complexcomposition_test.go index 66982784258..4c9033c6a57 100644 --- a/pkg/target/complexcomposition_test.go +++ b/pkg/target/complexcomposition_test.go @@ -365,7 +365,7 @@ resources: t.Fatalf("Expected resource accumulation error") } if !strings.Contains( - err.Error(), "already registered id: apps_v1_StatefulSet|~X|my-sts") { + err.Error(), "conflict between") { t.Fatalf("Unexpected err: %v", err) } } @@ -458,7 +458,7 @@ resources: t.Fatalf("Expected resource accumulation error") } if !strings.Contains( - err.Error(), "already registered id: apps_v1_StatefulSet|~X|my-sts") { + err.Error(), "conflict between") { t.Fatalf("Unexpected err: %v", err) } } diff --git a/pkg/target/diamondcomposition_test.go b/pkg/target/diamondcomposition_test.go index 728dacf5207..2b5a8cb4854 100644 --- a/pkg/target/diamondcomposition_test.go +++ b/pkg/target/diamondcomposition_test.go @@ -62,17 +62,49 @@ func writeDeploymentBase(th *kusttest_test.KustTestHarness) { th.WriteK("/app/base", ` resources: - deployment.yaml +patchesStrategicMerge: +- dep-patch.yaml `) th.WriteF("/app/base/deployment.yaml", ` apiVersion: apps/v1 kind: Deployment +metadata: + name: my-deployment +spec: + template: + spec: + containers: + - name: my-deployment + image: my-image +`) + + th.WriteF("/app/base/dep-patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment metadata: name: my-deployment spec: template: spec: dnsPolicy: "None" +`) +} + +func writeDeploymentBaseNoPatch(th *kusttest_test.KustTestHarness) { + th.WriteK("/app/base", ` +resources: +- deployment.yaml +`) + + th.WriteF("/app/base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-deployment +spec: + template: + spec: containers: - name: my-deployment image: my-image @@ -122,7 +154,11 @@ patchesStrategicMerge: // \ | / // base // -func TestIssue1251_CompositeDiamond_Failure(t *testing.T) { + +// In the case, the DeploymentBase is setting the DnsPolicy to None. +// Kustomize detects a merge conflict when merging probe, dns and restart +// into composite. +func TestIssue1251_CompositeDiamond_ProbeDnsRestart_MergeConflict(t *testing.T) { th := kusttest_test.NewKustTestHarness(t, "/app/composite") writeDeploymentBase(th) writeProbeOverlay(th) @@ -141,7 +177,55 @@ resources: t.Fatalf("Expected resource accumulation error") } if !strings.Contains( - err.Error(), "already registered id: apps_v1_Deployment|~X|my-deployment") { + err.Error(), "conflict between") { + t.Fatalf("Unexpected err: %v", err) + } +} + +func TestIssue1251_CompositeDiamond_ProbeRestartDns_MergeConflict(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 +`) + + _, err := th.MakeKustTarget().MakeCustomizedResMap() + if err == nil { + t.Fatalf("Expected resource accumulation error") + } + if !strings.Contains( + err.Error(), "conflict between") { + t.Fatalf("Unexpected err: %v", err) + } +} + +func TestIssue1251_CompositeDiamond_DnsProbeRestart_MergeConflict(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/app/composite") + writeDeploymentBase(th) + writeProbeOverlay(th) + writeDNSOverlay(th) + writeRestartOverlay(th) + + th.WriteK("/app/composite", ` +resources: +- ../dns +- ../probe +- ../restart +`) + + _, err := th.MakeKustTarget().MakeCustomizedResMap() + if err == nil { + t.Fatalf("Expected resource accumulation error") + } + if !strings.Contains( + err.Error(), "conflict between") { t.Fatalf("Unexpected err: %v", err) } } @@ -165,6 +249,51 @@ spec: restartPolicy: Always ` +// Same test except that there no conflict to be detected in during +// the merge +func TestIssue1251_CompositeDiamond_ProbeRestartDns(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/app/composite") + writeDeploymentBaseNoPatch(th) + writeProbeOverlay(th) + writeDNSOverlay(th) + writeRestartOverlay(th) + + th.WriteK("/app/composite", ` +resources: +- ../probe +- ../dns +- ../restart +`) + + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) + } + th.AssertActualEqualsExpected(m, expectedPatchedDeployment) +} + +// Identical test. Validate that ordering of resources as no effect. +func TestIssue1251_CompositeDiamond_ProbeDnsRestart(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/app/composite") + writeDeploymentBaseNoPatch(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) +} + // This test reuses some methods from TestIssue1251_CompositeDiamond, // but overwrites the kustomization files in the overlays. func TestIssue1251_Patches_Overlayed(t *testing.T) { diff --git a/pkg/target/diamonds_test.go b/pkg/target/diamonds_test.go index 6e0d004daba..a361b6bba18 100644 --- a/pkg/target/diamonds_test.go +++ b/pkg/target/diamonds_test.go @@ -223,3 +223,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 a35861a910b..79d02adecec 100644 --- a/pkg/target/kusttarget.go +++ b/pkg/target/kusttarget.go @@ -32,6 +32,7 @@ type KustTarget struct { rFactory *resmap.Factory tFactory resmap.PatchFactory pLdr *plugins.Loader + dynamic *types.Kustomization } // NewKustTarget returns a new instance of KustTarget primed with a Loader. @@ -63,6 +64,7 @@ func NewKustTarget( rFactory: rFactory, tFactory: tFactory, pLdr: pLdr, + dynamic: &types.Kustomization{}, }, nil } @@ -207,6 +209,20 @@ func (kt *KustTarget) shouldAddHashSuffixesToGeneratedResources() bool { // to do so. The name back references and vars are // not yet fixed. func (kt *KustTarget) AccumulateTarget() ( + ra *accumulator.ResAccumulator, err error) { + ra, err = kt.accumulateTarget() + if err != nil { + return nil, err + } + err = ra.MergeVars(kt.kustomization.Vars) + if err != nil { + return nil, errors.Wrapf( + err, "merging vars %v", kt.kustomization.Vars) + } + return ra, nil +} + +func (kt *KustTarget) accumulateTarget() ( ra *accumulator.ResAccumulator, err error) { ra = accumulator.MakeEmptyAccumulator() err = kt.accumulateResources(ra, kt.kustomization.Resources) @@ -241,11 +257,6 @@ func (kt *KustTarget) AccumulateTarget() ( if err != nil { return nil, err } - err = ra.MergeVars(kt.kustomization.Vars) - if err != nil { - return nil, errors.Wrapf( - err, "merging vars %v", kt.kustomization.Vars) - } return ra, nil } @@ -284,7 +295,15 @@ func (kt *KustTarget) configureExternalGenerators() ([]resmap.Generator, error) return kt.pLdr.LoadGenerators(kt.ldr, ra.ResMap()) } +func (kt *KustTarget) absorbDynamicKustomization(ra *accumulator.ResAccumulator) { + orig := ra.PatchSet() + kt.dynamic.Patches = make([]types.Patch, len(orig)) + copy(kt.dynamic.Patches, orig) +} + func (kt *KustTarget) runTransformers(ra *accumulator.ResAccumulator) error { + kt.absorbDynamicKustomization(ra) + var r []resmap.Transformer tConfig := ra.GetTransformerConfig() lts, err := kt.configureBuiltinTransformers(tConfig) @@ -341,11 +360,51 @@ func (kt *KustTarget) accumulateDirectory( if err != nil { return errors.Wrapf(err, "couldn't make target for path '%s'", path) } - subRa, err := subKt.AccumulateTarget() + + // Load the resources in the sub folders. Even if the subdirectory + // had already been visited by the kustomize, the subRa accumulator + // will contain its own copies of the resources. + subRa, err := subKt.accumulateTarget() if err != nil { return errors.Wrapf( err, "recursed accumulation of path '%s'", path) } + + // Remove the conflicting resources from the local context (subRa) + // and add them to the conflict resources list in the global one (ra) + // Conflicting is defined as having same CurId but different value. + // The algorithm is basically moving the conflict resources from the + // "resources" section of the context into the "patchStrategicMerge" one. + err = subRa.HandoverConflictingResources(ra) + if err != nil { + return errors.Wrapf( + err, "recursed handing over conflicting resources from path '%s'", path) + } + + // Verifies that each variable is targeting at most one resource + // in the local context. + // MergeVars will not only perform that operations using the variables of + // the current context (kustomization.Vars) but will also involve + // the "unresolved" variables declared but not resolved during the + // walk down the kustomize folder tree (in accumulateTarget). + err = subRa.MergeVars(subKt.kustomization.Vars) + if err != nil { + return errors.Wrapf( + err, "merging vars %v", subKt.kustomization.Vars) + } + + // MergeAccumulator has three main tasks: + // 1. Append the subRa resources to its parent resources. + // It is supposed to be successful since potentially + // conflicting ones have been put aside in the parent context + // already. A resource loaded from a file may end up multiple + // time in the list, each entry in that list will have the same + // OriginalId but a different CurId (for instance different prefix) + // 2. Merge kustomize configuration. Easy + // 3. Accumulate the local vars into the global ones. Since the variable + // are declared using the OriginalId, MergeAccumulator needs to check + // no variable is now pointing two resources with the same OriginalId + // but different CurId. err = ra.MergeAccumulator(subRa) if err != nil { return errors.Wrapf( @@ -356,13 +415,31 @@ func (kt *KustTarget) accumulateDirectory( func (kt *KustTarget) accumulateFile( ra *accumulator.ResAccumulator, path string) error { + subRa := accumulator.MakeEmptyAccumulator() resources, err := kt.rFactory.FromFile(kt.ldr, path) if err != nil { return errors.Wrapf(err, "accumulating resources from '%s'", path) } - err = ra.AppendAll(resources) + err = subRa.AppendAll(resources) if err != nil { - return errors.Wrapf(err, "merging resources from '%s'", path) + return errors.Wrapf(err, "accumulating resources from '%s'", path) + } + // Also only one file has been loaded, it may contain resources + // which are conflicting with the current ones. As for the directory case, + // those resources are moved from the "resources" section into the "patches" + // section. + err = subRa.HandoverConflictingResources(ra) + if err != nil { + return errors.Wrapf( + err, "recursed handing over conflicting resources from path '%s'", path) + } + // Since local context only contains one file with potentially multiple + // resources, only the resources portion of MergeAccumulator will actually be + // used. + err = ra.MergeAccumulator(subRa) + if err != nil { + return errors.Wrapf( + err, "recursed merging from path '%s'", path) } return nil } diff --git a/pkg/target/kusttarget_configplugin.go b/pkg/target/kusttarget_configplugin.go index a4e9d010ba5..078e51adf5e 100644 --- a/pkg/target/kusttarget_configplugin.go +++ b/pkg/target/kusttarget_configplugin.go @@ -9,6 +9,7 @@ import ( "sigs.k8s.io/kustomize/v3/pkg/resmap" "sigs.k8s.io/kustomize/v3/pkg/transformers/config" "sigs.k8s.io/kustomize/v3/pkg/types" + "strings" ) // Functions dedicated to configuring the builtin @@ -115,6 +116,16 @@ var generatorConfigurators = map[plugins.BuiltinPluginType]func( }, } +// Until Issue 1292 is implemented, use PathStrategicMerge to address +// when possible diamond merge issues. +func (kt *KustTarget) asString(patchSet []types.Patch) string { + res := []string{} + for _, patch := range patchSet { + res = append(res, patch.Patch) + } + return strings.Join(res, "---\n") +} + type tFactory func() resmap.TransformerPlugin var transformerConfigurators = map[plugins.BuiltinPluginType]func( @@ -164,7 +175,7 @@ var transformerConfigurators = map[plugins.BuiltinPluginType]func( plugins.PatchStrategicMergeTransformer: func( kt *KustTarget, bpt plugins.BuiltinPluginType, f tFactory, _ *config.TransformerConfig) ( result []resmap.Transformer, err error) { - if len(kt.kustomization.PatchesStrategicMerge) == 0 { + if len(kt.kustomization.PatchesStrategicMerge) == 0 && len(kt.dynamic.Patches) == 0 { return } var c struct { @@ -172,6 +183,7 @@ var transformerConfigurators = map[plugins.BuiltinPluginType]func( Patches string `json:"patches,omitempty" yaml:"patches,omitempty"` } c.Paths = kt.kustomization.PatchesStrategicMerge + c.Patches = kt.asString(kt.dynamic.Patches) p := f() err = kt.configureBuiltinPlugin(p, c, bpt) if err != nil { diff --git a/pkg/target/variableref_test.go b/pkg/target/variableref_test.go index 22048e89f90..3a917272235 100644 --- a/pkg/target/variableref_test.go +++ b/pkg/target/variableref_test.go @@ -282,6 +282,140 @@ spec: `) } +func TestVarUnresolvedUp(t *testing.T) { + th := kusttest_test.NewKustTestHarness(t, "/app/overlay") + th.WriteK("/app/base", ` +vars: +- name: POD_NAME1 + objref: + apiVersion: v1 + kind: Pod + name: kelley + fieldref: + fieldpath: metadata.name +- name: POD_NAME2 + objref: + apiVersion: v1 + kind: Pod + name: grimaldi + fieldref: + fieldpath: metadata.name +`) + + th.WriteK("/app/overlay", ` +resources: +- pod1.yaml +- pod2.yaml +- composite.yaml +- ../base +`) + + th.WriteF("/app/overlay/pod1.yaml", ` +apiVersion: v1 +kind: Pod +metadata: + name: kelley +spec: + containers: + - name: smile + image: smile + command: + - echo + - "$(POD_NAME1)" + env: + - name: FOO + value: "$(POD_NAME1)" +`) + th.WriteF("/app/overlay/pod2.yaml", ` +apiVersion: v1 +kind: Pod +metadata: + name: grimaldi +spec: + containers: + - name: dance + image: dance + command: + - echo + - "$(POD_NAME2)" + env: + - name: FOO + value: "$(POD_NAME2)" +`) + th.WriteF("/app/overlay/composite.yaml", ` +apiVersion: v1 +kind: Pod +metadata: + name: circus +spec: + containers: + - name: ring + image: ring + command: + - echo + - "$(POD_NAME1)" + - "$(POD_NAME2)" + env: + - name: P1 + value: "$(POD_NAME1)" + - name: P2 + value: "$(POD_NAME2)" +`) + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) + } + th.AssertActualEqualsExpected(m, ` +apiVersion: v1 +kind: Pod +metadata: + name: kelley +spec: + containers: + - command: + - echo + - kelley + env: + - name: FOO + value: kelley + image: smile + name: smile +--- +apiVersion: v1 +kind: Pod +metadata: + name: grimaldi +spec: + containers: + - command: + - echo + - grimaldi + env: + - name: FOO + value: grimaldi + image: dance + name: dance +--- +apiVersion: v1 +kind: Pod +metadata: + name: circus +spec: + containers: + - command: + - echo + - kelley + - grimaldi + env: + - name: P1 + value: kelley + - name: P2 + value: grimaldi + image: ring + name: ring +`) +} + // Not so much a bug as a desire for local variables // with less than global scope. Currently all variables // are global. So if a base with a variable is included @@ -330,40 +464,36 @@ resources: - ../o2 `) - /* - const presumablyDesired = ` - apiVersion: v1 - kind: Pod - metadata: - name: p1-base-myServerPod - spec: - containers: - - env: - - name: POD_NAME - value: p1-base-myServerPod - image: whatever - name: myServer - --- - apiVersion: v1 - kind: Pod - metadata: - name: p2-base-myServerPod - spec: - containers: - - env: - - name: POD_NAME - value: p2-base-myServerPod - 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) + const presumablyDesired = ` +apiVersion: v1 +kind: Pod +metadata: + name: p1-base-myServerPod +spec: + containers: + - env: + - name: POD_NAME + value: p1-base-myServerPod + image: whatever + name: myServer +--- +apiVersion: v1 +kind: Pod +metadata: + name: p2-base-myServerPod +spec: + containers: + - env: + - name: POD_NAME + value: p2-base-myServerPod + image: whatever + name: myServer +` + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) } + th.AssertActualEqualsExpected(m, presumablyDesired) } func TestVarRefBig(t *testing.T) { diff --git a/pkg/transformers/refvars.go b/pkg/transformers/refvars.go index 083807e9025..06d2493e6cc 100644 --- a/pkg/transformers/refvars.go +++ b/pkg/transformers/refvars.go @@ -24,7 +24,7 @@ import ( ) type RefVarTransformer struct { - varMap map[string]interface{} + varMap resmap.VarMap replacementCounts map[string]int fieldSpecs []config.FieldSpec mappingFunc func(string) interface{} @@ -34,7 +34,7 @@ type RefVarTransformer struct { // that replaces $(VAR) style variables with values. // The fieldSpecs are the places to look for occurrences of $(VAR). func NewRefVarTransformer( - varMap map[string]interface{}, fs []config.FieldSpec) *RefVarTransformer { + varMap resmap.VarMap, fs []config.FieldSpec) *RefVarTransformer { return &RefVarTransformer{ varMap: varMap, fieldSpecs: fs, @@ -91,7 +91,7 @@ func (rv *RefVarTransformer) replaceVars(in interface{}) (interface{}, error) { // after a Transform run. func (rv *RefVarTransformer) UnusedVars() []string { var unused []string - for k := range rv.varMap { + for _, k := range rv.varMap.VarNames() { _, ok := rv.replacementCounts[k] if !ok { unused = append(unused, k) @@ -103,9 +103,13 @@ func (rv *RefVarTransformer) UnusedVars() []string { // Transform replaces $(VAR) style variables with values. func (rv *RefVarTransformer) Transform(m resmap.ResMap) error { rv.replacementCounts = make(map[string]int) - rv.mappingFunc = expansion.MappingFuncFor( - rv.replacementCounts, rv.varMap) for _, res := range m.Resources() { + varSubset, err := rv.varMap.SubsetThatCouldBeReferencedByResource(res) + if err != nil { + return err + } + rv.mappingFunc = expansion.MappingFuncFor( + rv.replacementCounts, varSubset) for _, fieldSpec := range rv.fieldSpecs { if res.OrgId().IsSelected(&fieldSpec.Gvk) { if err := MutateField( diff --git a/pkg/transformers/refvars_test.go b/pkg/transformers/refvars_test.go index 0dc5e58a082..54294f5df1c 100644 --- a/pkg/transformers/refvars_test.go +++ b/pkg/transformers/refvars_test.go @@ -112,7 +112,7 @@ func TestVarRef(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { // arrange - tr := NewRefVarTransformer(tc.given.varMap, tc.given.fs) + tr := NewRefVarTransformer(resmap.NewVarMap(tc.given.varMap), tc.given.fs) // act err := tr.Transform(tc.given.res)