Merge pull request #664 from monopole/fix658

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
+// anything in the corresponding fields, e.g.
+//
+// this item:
+//       <Group: "extensions", Version: "v1beta1", Kind: "Deployment">
+//
+// is selected by
 //       <Group: "",           Version: "",        Kind: "Deployment">
-//   selects
-//       <Group: "extensions", Version: "v1beta1", Kind: "Deployment">.
 //
-//   while selector
+// 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,41 +98,50 @@ 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
 	}
 
 	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
+}