Check for config merge conflicts and duplication.

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
-//       <Group: "",           Version: "",        Kind: "Deployment">
-//   selects
-//       <Group: "extensions", Version: "v1beta1", Kind: "Deployment">.
+// anything in the corresponding fields, e.g.
 //
-//   while selector
+// this item:
+//       <Group: "extensions", Version: "v1beta1", Kind: "Deployment">
+//
+// is selected by
+//       <Group: "",           Version: "",        Kind: "Deployment">
+//
+// but rejected by
 //       <Group: "apps",       Version: "",        Kind: "Deployment">
-//   rejects
-//       <Group: "extensions", Version: "v1beta1", Kind: "Deployment">.
 //
 func (x Gvk) IsSelected(selector *Gvk) bool {
 	if selector == nil {

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)

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

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"))

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
 }

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
+				}
 			}
 		}
 

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
+}

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)
+			}
+		}
+	}
+}

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
 }

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)
 	}

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
 }

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)
 	}