From 8c994725cb2e62371dc69bc2d81eebe22e78d2d2 Mon Sep 17 00:00:00 2001 From: jregan Date: Sat, 29 Dec 2018 08:19:37 -0800 Subject: [PATCH] Check for config merge conflicts and duplication. --- pkg/gvk/gvk.go | 16 +-- pkg/resource/factory.go | 2 +- pkg/target/customconfig_test.go | 15 +- pkg/target/kusttarget.go | 7 +- pkg/transformers/config/factory.go | 5 +- pkg/transformers/config/factorycrd.go | 32 ++++- pkg/transformers/config/fieldspec.go | 46 +++++- pkg/transformers/config/fieldspec_test.go | 134 ++++++++++++++++++ pkg/transformers/config/namebackreferences.go | 23 +-- .../config/namebackreferences_test.go | 10 +- pkg/transformers/config/transformerconfig.go | 69 ++++++--- .../config/transformerconfig_test.go | 35 ++++- 12 files changed, 325 insertions(+), 69 deletions(-) diff --git a/pkg/gvk/gvk.go b/pkg/gvk/gvk.go index 0bc515a35..00e44f7ea 100644 --- a/pkg/gvk/gvk.go +++ b/pkg/gvk/gvk.go @@ -116,16 +116,16 @@ func (x Gvk) IsLessThan(o Gvk) bool { // If `selector` and `x` are the same, return true. // If `selector` is nil, it is considered a wildcard match, returning true. // If selector fields are empty, they are considered wildcards matching -// anything in the corresponding fields, .g. -// selector -// -// selects -// . +// anything in the corresponding fields, e.g. // -// while selector +// this item: +// +// +// is selected by +// +// +// but rejected by // -// rejects -// . // func (x Gvk) IsSelected(selector *Gvk) bool { if selector == nil { diff --git a/pkg/resource/factory.go b/pkg/resource/factory.go index 3c0dbab07..2f287ecbd 100644 --- a/pkg/resource/factory.go +++ b/pkg/resource/factory.go @@ -87,7 +87,7 @@ func (rf *Factory) SliceFromBytes(in []byte) ([]*Resource, error) { items := u.Map()["items"] itemsSlice, ok := items.([]interface{}) if !ok { - return nil, fmt.Errorf("items in List is type %T, expected array.", items) + return nil, fmt.Errorf("items in List is type %T, expected array", items) } for _, item := range itemsSlice { itemJSON, err := json.Marshal(item) diff --git a/pkg/target/customconfig_test.go b/pkg/target/customconfig_test.go index 9c0c6b0e3..a2f0cde56 100644 --- a/pkg/target/customconfig_test.go +++ b/pkg/target/customconfig_test.go @@ -150,9 +150,6 @@ spec: `) } -// TODO: this test demonstrates #658 -// The prefix "x-" is applied twice (search for x-x-sandiego), because the -// config from the base isn't properly merged with the config in the overlay. func TestCustomConfigWithDefaultOverspecification(t *testing.T) { th := NewKustTestHarness(t, "/app/base") makeBaseReferencingCustomConfig(th) @@ -184,21 +181,21 @@ kind: AnimalPark metadata: labels: app: myApp - name: x-x-sandiego + name: x-sandiego spec: food: - mimosa - bambooshoots giraffeRef: - name: x-x-april + name: x-april gorillaRef: - name: x-x-koko + name: x-koko --- kind: Giraffe metadata: labels: app: myApp - name: x-x-april + name: x-april spec: diet: mimosa location: NE @@ -207,7 +204,7 @@ kind: Giraffe metadata: labels: app: myApp - name: x-x-may + name: x-may spec: diet: acacia location: SE @@ -216,7 +213,7 @@ kind: Gorilla metadata: labels: app: myApp - name: x-x-koko + name: x-koko spec: diet: bambooshoots location: SW diff --git a/pkg/target/kusttarget.go b/pkg/target/kusttarget.go index 02a094709..ac315006a 100644 --- a/pkg/target/kusttarget.go +++ b/pkg/target/kusttarget.go @@ -134,7 +134,7 @@ func mergeCustomConfigWithDefaults( if err != nil { return nil, err } - return t1.Merge(t2), nil + return t1.Merge(t2) } // MakeCustomizedResMap creates a ResMap per kustomization instructions. @@ -194,10 +194,13 @@ func (kt *KustTarget) loadCustomizedResMap() (resmap.ResMap, error) { errs.Append(errors.Wrap(err, "loadResMapFromBasesAndResources")) } crdTc, err := config.NewFactory(kt.ldr).LoadCRDs(kt.kustomization.Crds) - kt.tConfig = kt.tConfig.Merge(crdTc) if err != nil { errs.Append(errors.Wrap(err, "LoadCRDs")) } + kt.tConfig, err = kt.tConfig.Merge(crdTc) + if err != nil { + errs.Append(errors.Wrap(err, "merge CRDs")) + } resMap, err := kt.generateConfigMapsAndSecrets(errs) if err != nil { errs.Append(errors.Wrap(err, "generateConfigMapsAndSecrets")) diff --git a/pkg/transformers/config/factory.go b/pkg/transformers/config/factory.go index 0bf842c64..644fbcae6 100644 --- a/pkg/transformers/config/factory.go +++ b/pkg/transformers/config/factory.go @@ -53,7 +53,10 @@ func (tf *Factory) FromFiles( if err != nil { return nil, err } - result = result.Merge(t) + result, err = result.Merge(t) + if err != nil { + return nil, err + } } return result, nil } diff --git a/pkg/transformers/config/factorycrd.go b/pkg/transformers/config/factorycrd.go index 0742ab5db..24e0d5614 100644 --- a/pkg/transformers/config/factorycrd.go +++ b/pkg/transformers/config/factorycrd.go @@ -33,7 +33,10 @@ func (tf *Factory) LoadCRDs(paths []string) (*TransformerConfig, error) { if err != nil { return nil, err } - tc = tc.Merge(otherTc) + tc, err = tc.Merge(otherTc) + if err != nil { + return nil, err + } } return tc, nil } @@ -63,7 +66,10 @@ func (tf *Factory) loadCRD(path string) (*TransformerConfig, error) { if err != nil { return result, err } - result = result.Merge(tc) + result, err = result.Merge(tc) + if err != nil { + return result, err + } } return result, nil @@ -92,7 +98,7 @@ func getCRDs(types map[string]common.OpenAPIDefinition) map[string]gvk.Gvk { func loadCrdIntoConfig( types map[string]common.OpenAPIDefinition, atype string, crd string, in gvk.Gvk, - path []string, config *TransformerConfig) error { + path []string, config *TransformerConfig) (err error) { if _, ok := types[crd]; !ok { return nil } @@ -100,33 +106,42 @@ func loadCrdIntoConfig( for propname, property := range types[atype].Schema.SchemaProps.Properties { _, annotate := property.Extensions.GetString(Annotation) if annotate { - config.AddAnnotationFieldSpec( + err = config.AddAnnotationFieldSpec( FieldSpec{ CreateIfNotPresent: false, Gvk: in, Path: strings.Join(append(path, propname), "/"), }, ) + if err != nil { + return err + } } _, label := property.Extensions.GetString(LabelSelector) if label { - config.AddLabelFieldSpec( + err = config.AddLabelFieldSpec( FieldSpec{ CreateIfNotPresent: false, Gvk: in, Path: strings.Join(append(path, propname), "/"), }, ) + if err != nil { + return err + } } _, identity := property.Extensions.GetString(Identity) if identity { - config.AddPrefixFieldSpec( + err = config.AddPrefixFieldSpec( FieldSpec{ CreateIfNotPresent: false, Gvk: in, Path: strings.Join(append(path, propname), "/"), }, ) + if err != nil { + return err + } } version, ok := property.Extensions.GetString(Version) if ok { @@ -136,7 +151,7 @@ func loadCrdIntoConfig( if !ok { nameKey = "name" } - config.AddNamereferenceFieldSpec(NameBackReferences{ + err = config.AddNamereferenceFieldSpec(NameBackReferences{ Gvk: gvk.Gvk{Kind: kind, Version: version}, FieldSpecs: []FieldSpec{ { @@ -146,6 +161,9 @@ func loadCrdIntoConfig( }, }, }) + if err != nil { + return err + } } } diff --git a/pkg/transformers/config/fieldspec.go b/pkg/transformers/config/fieldspec.go index 7f759d2a0..5b0f6ee33 100644 --- a/pkg/transformers/config/fieldspec.go +++ b/pkg/transformers/config/fieldspec.go @@ -57,6 +57,11 @@ func (fs FieldSpec) String() string { "%s:%v:%s", fs.Gvk.String(), fs.CreateIfNotPresent, fs.Path) } +// If true, the primary key is the same, but other fields might not be. +func (fs FieldSpec) effectivelyEquals(other FieldSpec) bool { + return fs.IsSelected(&other.Gvk) && fs.Path == other.Path +} + // PathSlice converts the path string to a slice of strings, // separated by a '/'. Forward slash can be contained in a // fieldname. such as ingress.kubernetes.io/auth-secret in @@ -76,7 +81,6 @@ func (fs FieldSpec) PathSlice() []string { if !strings.Contains(fs.Path, escapedForwardSlash) { return strings.Split(fs.Path, "/") } - s := strings.Replace(fs.Path, escapedForwardSlash, tempSlashReplacement, -1) paths := strings.Split(s, "/") var result []string @@ -93,3 +97,43 @@ func (s fsSlice) Swap(i, j int) { s[i], s[j] = s[j], s[i] } func (s fsSlice) Less(i, j int) bool { return s[i].Gvk.IsLessThan(s[j].Gvk) } + +// mergeAll merges the argument into this, returning the result. +// Items already present are ignored. +// Items that conflict (primary key matches, but remain data differs) +// result in an error. +func (s fsSlice) mergeAll(incoming fsSlice) (result fsSlice, err error) { + result = s + for _, x := range incoming { + result, err = result.mergeOne(x) + if err != nil { + return nil, err + } + } + return result, nil +} + +// mergeOne merges the argument into this, returning the result. +// If the item's primary key is already present, and there are no +// conflicts, it is ignored (we don't want duplicates). +// If there is a conflict, the merge fails. +func (s fsSlice) mergeOne(x FieldSpec) (fsSlice, error) { + i := s.index(x) + if i > -1 { + // It's already there. + if s[i].CreateIfNotPresent != x.CreateIfNotPresent { + return nil, fmt.Errorf("conflicting fieldspecs") + } + return s, nil + } + return append(s, x), nil +} + +func (s fsSlice) index(fs FieldSpec) int { + for i, x := range s { + if x.effectivelyEquals(fs) { + return i + } + } + return -1 +} diff --git a/pkg/transformers/config/fieldspec_test.go b/pkg/transformers/config/fieldspec_test.go index 47366317d..7279e0abe 100644 --- a/pkg/transformers/config/fieldspec_test.go +++ b/pkg/transformers/config/fieldspec_test.go @@ -17,8 +17,12 @@ limitations under the License. package config import ( + "fmt" "reflect" + "strings" "testing" + + "sigs.k8s.io/kustomize/pkg/gvk" ) func TestPathSlice(t *testing.T) { @@ -44,3 +48,133 @@ func TestPathSlice(t *testing.T) { } } } + +var mergeTests = []struct { + name string + original fsSlice + incoming fsSlice + err error + result fsSlice +}{ + { + "normal", + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "pear"}, + CreateIfNotPresent: false, + }, + }, + fsSlice{ + { + Path: "home", + Gvk: gvk.Gvk{Group: "beans"}, + CreateIfNotPresent: false, + }, + }, + nil, + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "pear"}, + CreateIfNotPresent: false, + }, + { + Path: "home", + Gvk: gvk.Gvk{Group: "beans"}, + CreateIfNotPresent: false, + }, + }, + }, + { + "ignore copy", + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "pear"}, + CreateIfNotPresent: false, + }, + }, + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + }, + nil, + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "pear"}, + CreateIfNotPresent: false, + }, + }, + }, + { + "error on conflict", + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: false, + }, + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "pear"}, + CreateIfNotPresent: false, + }, + }, + fsSlice{ + { + Path: "whatever", + Gvk: gvk.Gvk{Group: "apple"}, + CreateIfNotPresent: true, + }, + }, + fmt.Errorf("hey"), + fsSlice{}, + }, +} + +func TestFsSlice_MergeAll(t *testing.T) { + for _, item := range mergeTests { + result, err := item.original.mergeAll(item.incoming) + if item.err == nil { + if err != nil { + t.Fatalf("test %s: unexpected err %v", item.name, err) + } + if !reflect.DeepEqual(item.result, result) { + t.Fatalf("test %s: expected: %v\n but got: %v\n", + item.name, item.result, result) + } + } else { + if err == nil { + t.Fatalf("test %s: expected err: %v", item.name, err) + } + if !strings.Contains(err.Error(), "conflicting fieldspecs") { + t.Fatalf("test %s: unexpected err: %v", item.name, err) + } + } + } +} diff --git a/pkg/transformers/config/namebackreferences.go b/pkg/transformers/config/namebackreferences.go index d0a6e8d1e..172e4b3ca 100644 --- a/pkg/transformers/config/namebackreferences.go +++ b/pkg/transformers/config/namebackreferences.go @@ -52,7 +52,7 @@ import ( // } type NameBackReferences struct { gvk.Gvk `json:",inline,omitempty" yaml:",inline,omitempty"` - FieldSpecs []FieldSpec `json:"FieldSpecs,omitempty" yaml:"FieldSpecs,omitempty"` + FieldSpecs fsSlice `json:"FieldSpecs,omitempty" yaml:"FieldSpecs,omitempty"` } func (n NameBackReferences) String() string { @@ -72,20 +72,27 @@ func (s nbrSlice) Less(i, j int) bool { return s[i].Gvk.IsLessThan(s[j].Gvk) } -func (s nbrSlice) mergeAll(o nbrSlice) nbrSlice { - result := s +func (s nbrSlice) mergeAll(o nbrSlice) (result nbrSlice, err error) { + result = s for _, r := range o { - result = result.mergeOne(r) + result, err = result.mergeOne(r) + if err != nil { + return nil, err + } } - return result + return result, nil } -func (s nbrSlice) mergeOne(other NameBackReferences) nbrSlice { +func (s nbrSlice) mergeOne(other NameBackReferences) (nbrSlice, error) { var result nbrSlice + var err error found := false for _, c := range s { if c.Gvk.Equals(other.Gvk) { - c.FieldSpecs = append(c.FieldSpecs, other.FieldSpecs...) + c.FieldSpecs, err = c.FieldSpecs.mergeAll(other.FieldSpecs) + if err != nil { + return nil, err + } found = true } result = append(result, c) @@ -94,5 +101,5 @@ func (s nbrSlice) mergeOne(other NameBackReferences) nbrSlice { if !found { result = append(result, other) } - return result + return result, nil } diff --git a/pkg/transformers/config/namebackreferences_test.go b/pkg/transformers/config/namebackreferences_test.go index 59eb07761..a872a74d9 100644 --- a/pkg/transformers/config/namebackreferences_test.go +++ b/pkg/transformers/config/namebackreferences_test.go @@ -89,17 +89,19 @@ func TestMergeAll(t *testing.T) { Gvk: gvk.Gvk{ Kind: "ConfigMap", }, - // Current behavior allows repeats of FieldSpec - FieldSpecs: append(fsSlice1, fsSlice1...), + FieldSpecs: fsSlice1, }, { Gvk: gvk.Gvk{ Kind: "Secret", }, - FieldSpecs: append(fsSlice2, fsSlice2...), + FieldSpecs: fsSlice2, }, } - actual := nbrsSlice1.mergeAll(nbrsSlice2) + actual, err := nbrsSlice1.mergeAll(nbrsSlice2) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if !reflect.DeepEqual(actual, expected) { t.Fatalf("expected\n %v\n but got\n %v\n", expected, actual) } diff --git a/pkg/transformers/config/transformerconfig.go b/pkg/transformers/config/transformerconfig.go index 5de589e19..6d9e6d2e4 100644 --- a/pkg/transformers/config/transformerconfig.go +++ b/pkg/transformers/config/transformerconfig.go @@ -44,43 +44,70 @@ func (t *TransformerConfig) sortFields() { } // AddPrefixFieldSpec adds a FieldSpec to NamePrefix -func (t *TransformerConfig) AddPrefixFieldSpec(fs FieldSpec) { - t.NamePrefix = append(t.NamePrefix, fs) +func (t *TransformerConfig) AddPrefixFieldSpec(fs FieldSpec) (err error) { + t.NamePrefix, err = t.NamePrefix.mergeOne(fs) + return err } // AddSuffixFieldSpec adds a FieldSpec to NameSuffix -func (t *TransformerConfig) AddSuffixFieldSpec(fs FieldSpec) { - t.NameSuffix = append([]FieldSpec{fs}, t.NameSuffix...) +func (t *TransformerConfig) AddSuffixFieldSpec(fs FieldSpec) (err error) { + t.NameSuffix, err = t.NameSuffix.mergeOne(fs) + return err } // AddLabelFieldSpec adds a FieldSpec to CommonLabels -func (t *TransformerConfig) AddLabelFieldSpec(fs FieldSpec) { - t.CommonLabels = append(t.CommonLabels, fs) +func (t *TransformerConfig) AddLabelFieldSpec(fs FieldSpec) (err error) { + t.CommonLabels, err = t.CommonLabels.mergeOne(fs) + return err } // AddAnnotationFieldSpec adds a FieldSpec to CommonAnnotations -func (t *TransformerConfig) AddAnnotationFieldSpec(fs FieldSpec) { - t.CommonAnnotations = append(t.CommonAnnotations, fs) +func (t *TransformerConfig) AddAnnotationFieldSpec(fs FieldSpec) (err error) { + t.CommonAnnotations, err = t.CommonAnnotations.mergeOne(fs) + return err } // AddNamereferenceFieldSpec adds a NameBackReferences to NameReference -func (t *TransformerConfig) AddNamereferenceFieldSpec(nbrs NameBackReferences) { - t.NameReference = t.NameReference.mergeOne(nbrs) +func (t *TransformerConfig) AddNamereferenceFieldSpec(nbrs NameBackReferences) (err error) { + t.NameReference, err = t.NameReference.mergeOne(nbrs) + return err } // Merge merges two TransformerConfigs objects into a new TransformerConfig object -func (t *TransformerConfig) Merge(input *TransformerConfig) *TransformerConfig { +func (t *TransformerConfig) Merge(input *TransformerConfig) ( + merged *TransformerConfig, err error) { if input == nil { - return t + return t, nil + } + merged = &TransformerConfig{} + merged.NamePrefix, err = t.NamePrefix.mergeAll(input.NamePrefix) + if err != nil { + return nil, err + } + merged.NameSuffix, err = t.NameSuffix.mergeAll(input.NameSuffix) + if err != nil { + return nil, err + } + merged.NameSpace, err = t.NameSpace.mergeAll(input.NameSpace) + if err != nil { + return nil, err + } + merged.CommonAnnotations, err = t.CommonAnnotations.mergeAll(input.CommonAnnotations) + if err != nil { + return nil, err + } + merged.CommonLabels, err = t.CommonLabels.mergeAll(input.CommonLabels) + if err != nil { + return nil, err + } + merged.VarReference, err = t.VarReference.mergeAll(input.VarReference) + if err != nil { + return nil, err + } + merged.NameReference, err = t.NameReference.mergeAll(input.NameReference) + if err != nil { + return nil, err } - merged := &TransformerConfig{} - merged.NamePrefix = append(t.NamePrefix, input.NamePrefix...) - merged.NameSuffix = append(input.NameSuffix, t.NameSuffix...) - merged.NameSpace = append(t.NameSpace, input.NameSpace...) - merged.CommonAnnotations = append(t.CommonAnnotations, input.CommonAnnotations...) - merged.CommonLabels = append(t.CommonLabels, input.CommonLabels...) - merged.VarReference = append(t.VarReference, input.VarReference...) - merged.NameReference = t.NameReference.mergeAll(input.NameReference) merged.sortFields() - return merged + return merged, nil } diff --git a/pkg/transformers/config/transformerconfig_test.go b/pkg/transformers/config/transformerconfig_test.go index a12fa6e38..40d8b05e4 100644 --- a/pkg/transformers/config/transformerconfig_test.go +++ b/pkg/transformers/config/transformerconfig_test.go @@ -42,7 +42,10 @@ func TestAddNamereferenceFieldSpec(t *testing.T) { }, } - cfg.AddNamereferenceFieldSpec(nbrs) + err := cfg.AddNamereferenceFieldSpec(nbrs) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(cfg.NameReference) != 1 { t.Fatal("failed to add namereference FieldSpec") } @@ -57,19 +60,31 @@ func TestAddFieldSpecs(t *testing.T) { CreateIfNotPresent: true, } - cfg.AddPrefixFieldSpec(fieldSpec) + err := cfg.AddPrefixFieldSpec(fieldSpec) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(cfg.NamePrefix) != 1 { t.Fatalf("failed to add nameprefix FieldSpec") } - cfg.AddSuffixFieldSpec(fieldSpec) + err = cfg.AddSuffixFieldSpec(fieldSpec) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(cfg.NameSuffix) != 1 { t.Fatalf("failed to add namesuffix FieldSpec") } - cfg.AddLabelFieldSpec(fieldSpec) + err = cfg.AddLabelFieldSpec(fieldSpec) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(cfg.CommonLabels) != 1 { t.Fatalf("failed to add nameprefix FieldSpec") } - cfg.AddAnnotationFieldSpec(fieldSpec) + err = cfg.AddAnnotationFieldSpec(fieldSpec) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(cfg.CommonAnnotations) != 1 { t.Fatalf("failed to add nameprefix FieldSpec") } @@ -128,7 +143,10 @@ func TestMerge(t *testing.T) { cfgb.AddPrefixFieldSpec(fieldSpecs[1]) cfga.AddSuffixFieldSpec(fieldSpecs[1]) - actual := cfga.Merge(cfgb) + actual, err := cfga.Merge(cfgb) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if len(actual.NamePrefix) != 2 { t.Fatal("merge failed for namePrefix FieldSpec") @@ -154,7 +172,10 @@ func TestMerge(t *testing.T) { t.Fatalf("expected: %v\n but got: %v\n", expected, actual) } - actual = cfga.Merge(nil) + actual, err = cfga.Merge(nil) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if !reflect.DeepEqual(actual, cfga) { t.Fatalf("expected: %v\n but got: %v\n", cfga, actual) }