allow vars of the same name that refer to the same concrete value

pkg/accumulator/resaccumulator.go

@@ -94,7 +94,36 @@ func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) {
 	if err != nil {
 		return err
 	}
-	return ra.varSet.MergeSet(other.varSet)
+	err = ra.varSet.MergeSet(other.varSet)
+	if errs, ok := err.(types.MergeSetError); ok {
+		return ra.resolveVarConflicts(other, errs)
+	}
+	return nil
+}
+
+// resolveVarConflicts iterates over all variable name conflicts found while
+// merging VarSets from two ResAccumulator instances. Some apparent conflicts
+// can be ignored, such as variables that appear more than once but which
+// reference the same concrete value.
+func (ra *ResAccumulator) resolveVarConflicts(other *ResAccumulator, conflict types.MergeSetError) error {
+	var badVars []string
+	for _, mergeErr := range conflict.Errs {
+		incomingValue, ierr := ra.findVarValueFromResources(mergeErr.Incoming)
+		if ierr != nil {
+			return ierr
+		}
+		conflictValue, cerr := other.findVarValueFromResources(mergeErr.Conflict)
+		if cerr != nil {
+			return cerr
+		}
+		if incomingValue != conflictValue {
+			badVars = append(badVars, mergeErr.Incoming.Name)
+		}
+	}
+	if len(badVars) == 0 {
+		return nil
+	}
+	return fmt.Errorf("found conflicting variable(s) named: %s", strings.Join(badVars, ", "))
 }
 
 func (ra *ResAccumulator) findVarValueFromResources(v types.Var) (interface{}, error) {

pkg/accumulator/resaccumulator_test.go

@@ -179,6 +179,87 @@ func TestResolveVarsVarNeedsDisambiguation(t *testing.T) {
 	}
 }
 
+func makeNamespacedConfigMapWithDataProviderValue(
+	namespace string,
+	value string,
+) map[string]interface{} {
+	return map[string]interface{}{
+		"apiVersion": "v1",
+		"kind":       "ConfigMap",
+		"metadata": map[string]interface{}{
+			"name":      "environment",
+			"namespace": namespace,
+		},
+		"data": map[string]interface{}{
+			"provider": value,
+		},
+	}
+}
+
+func makeVarToNamepaceAndPath(
+	name string,
+	namespace string,
+	path string,
+) types.Var {
+	return types.Var{
+		Name: name,
+		ObjRef: types.Target{
+			Gvk:       gvk.Gvk{Version: "v1", Kind: "ConfigMap"},
+			Name:      "environment",
+			Namespace: namespace,
+		},
+		FieldRef: types.FieldSelector{FieldPath: path},
+	}
+}
+
+func TestResolveVarConflicts(t *testing.T) {
+	rf := resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl())
+
+	// create configmaps in foo and bar namespaces with `data.provider` values.
+	fooAws := makeNamespacedConfigMapWithDataProviderValue("foo", "aws")
+	barAws := makeNamespacedConfigMapWithDataProviderValue("bar", "aws")
+	barGcp := makeNamespacedConfigMapWithDataProviderValue("bar", "gcp")
+
+	// create two variables with (apparently) conflicting names that point to
+	// fieldpaths that could be generalized.
+	varFoo := makeVarToNamepaceAndPath("PROVIDER", "foo", "data.provider")
+	varBar := makeVarToNamepaceAndPath("PROVIDER", "bar", "data.provider")
+
+	// create accumulators holding apparently conflicting vars that are not
+	// actually in conflict because they point to the same concrete value.
+	rm0 := resmap.New()
+	rm0.Append(rf.FromMap(fooAws))
+	ac0 := MakeEmptyAccumulator()
+	ac0.AppendAll(rm0)
+	ac0.MergeVars([]types.Var{varFoo})
+
+	rm1 := resmap.New()
+	rm1.Append(rf.FromMap(barAws))
+	ac1 := MakeEmptyAccumulator()
+	ac1.AppendAll(rm1)
+	ac1.MergeVars([]types.Var{varBar})
+
+	// validate that two vars of the same name which reference the same concrete
+	// value do not produce a conflict.
+	err := ac0.MergeAccumulator(ac1)
+	if err != nil {
+		t.Fatalf("dupe var names w/ same concrete val should not conflict: %v", err)
+	}
+
+	// create an accumulator will have an actually conflicting value with the
+	// two above (because it contains a variable whose name is used in the other
+	// accumulators AND whose concrete values are different).
+	rm2 := resmap.New()
+	rm2.Append(rf.FromMap(barGcp))
+	ac2 := MakeEmptyAccumulator()
+	ac2.AppendAll(rm2)
+	ac2.MergeVars([]types.Var{varBar})
+	err = ac1.MergeAccumulator(ac2)
+	if err == nil {
+		t.Fatalf("dupe vars w/ different concrete values should conflict")
+	}
+}
+
 func TestResolveVarsGoodResIdBadField(t *testing.T) {
 	ra, _ := makeResAccumulator(t)
 	err := ra.MergeVars([]types.Var{

pkg/types/var.go

@@ -65,7 +65,34 @@ type FieldSelector struct {
 	FieldPath string `json:"fieldPath,omitempty" yaml:"fieldPath,omitempty"`
 }
 
-// defaulting sets reference to field used by default.
+// VarMergeError annotates a variable merge conflict with a reference to the
+// variable that encountered the conflict. This is used at higher levels of the
+// API to determine if the conflict can be ignored (e.g. two sets of resources
+// using the same variable name are not in conflict if the concrete values they
+// reference are the same).
+type VarMergeError struct {
+	msg      string
+	Incoming Var
+	Conflict Var
+}
+
+func (vme VarMergeError) Error() string { return vme.msg }
+
+// MergeSetError contains the aggregation of all errors encountered while
+// trying to merge two VarSets.
+type MergeSetError struct {
+	Errs []VarMergeError
+}
+
+func (mse MergeSetError) Error() string {
+	var result []string
+	for _, err := range mse.Errs {
+		result = append(result, err.Error())
+	}
+	return strings.Join(result, "\n")
+}
+
+// Defaulting sets reference to field used by default.
 func (v *Var) Defaulting() {
 	if v.FieldRef.FieldPath == "" {
 		v.FieldRef.FieldPath = defaultFieldPath
@@ -117,11 +144,15 @@ func (vs *VarSet) Copy() VarSet {
 
 // MergeSet absorbs other vars with error on name collision.
 func (vs *VarSet) MergeSet(incoming VarSet) error {
+	var errs []VarMergeError
 	for _, incomingVar := range incoming.set {
-		if err := vs.Merge(incomingVar); err != nil {
-			return err
+		if mergeErr, ok := vs.Merge(incomingVar).(VarMergeError); ok {
+			errs = append(errs, mergeErr)
 		}
 	}
+	if len(errs) != 0 {
+		return MergeSetError{errs}
+	}
 	return nil
 }
 
@@ -140,8 +171,11 @@ func (vs *VarSet) MergeSlice(incoming []Var) error {
 // Empty fields in incoming Var is defaulted.
 func (vs *VarSet) Merge(v Var) error {
 	if vs.Contains(v) {
-		return fmt.Errorf(
-			"var '%s' already encountered", v.Name)
+		return VarMergeError{
+			msg:      fmt.Sprintf("var '%s' already encountered", v.Name),
+			Incoming: v,
+			Conflict: *vs.Get(v.Name),
+		}
 	}
 	v.Defaulting()
 	vs.set[v.Name] = v