Forbid Var name collisions in a kustomization stack

pkg/target/kusttarget.go

@@ -124,22 +124,17 @@ func (kt *KustTarget) MakeCustomizedResMap() (resmap.ResMap, error) {
 			return nil, err
 		}
 	}
-
 	// Given that names have changed (prefixs/suffixes added),
 	// fix all the back references to those names.
+	if kt.tConfig.NameReference != nil {
 		err = transformers.NewNameReferenceTransformer(
 			kt.tConfig.NameReference).Transform(m)
 		if err != nil {
 			return nil, err
 		}
-
-	// With all the back references fixed, it's OK to resolve Vars.
-	refVars, err := kt.resolveVars(m)
-	if err != nil {
-		return nil, err
 	}
-	err = transformers.NewRefVarTransformer(
+	// With all the back references fixed, it's OK to resolve Vars.
-		refVars, kt.tConfig.VarReference).Transform(m)
+	err = kt.doVarReplacement(m)
 	return m, err
 }
 
@@ -148,6 +143,19 @@ func (kt *KustTarget) shouldAddHashSuffixesToGeneratedResources() bool {
 		!kt.kustomization.GeneratorOptions.DisableNameSuffixHash
 }
 
+func (kt *KustTarget) doVarReplacement(m resmap.ResMap) error {
+	vars, err := kt.getAllVars()
+	if err != nil {
+		return err
+	}
+	varMap, err := kt.resolveVars(m, vars)
+	if err != nil {
+		return err
+	}
+	return transformers.NewRefVarTransformer(
+		varMap, kt.tConfig.VarReference).Transform(m)
+}
+
 // loadCustomizedResMap loads and customizes resources to build a ResMap.
 func (kt *KustTarget) loadCustomizedResMap() (resmap.ResMap, error) {
 	errs := &interror.KustomizationErrors{}
@@ -235,7 +243,8 @@ func (kt *KustTarget) loadResMapFromBasesAndResources() (resmap.ResMap, error) {
 
 // Loop through the Bases of this kustomization recursively loading resources.
 // Combine into one ResMap, demanding unique Ids for each resource.
-func (kt *KustTarget) loadCustomizedBases() (resmap.ResMap, *interror.KustomizationErrors) {
+func (kt *KustTarget) loadCustomizedBases() (
+	resmap.ResMap, *interror.KustomizationErrors) {
 	var list []resmap.ResMap
 	errs := &interror.KustomizationErrors{}
 	for _, path := range kt.kustomization.Bases {
@@ -268,24 +277,18 @@ func (kt *KustTarget) loadCustomizedBases() (resmap.ResMap, *interror.Kustomizat
 
 func (kt *KustTarget) loadBasesAsKustTargets() ([]*KustTarget, error) {
 	var result []*KustTarget
-	errs := &interror.KustomizationErrors{}
 	for _, path := range kt.kustomization.Bases {
 		ldr, err := kt.ldr.New(path)
 		if err != nil {
-			errs.Append(err)
+			return nil, err
-			continue
 		}
 		target, err := NewKustTarget(
 			ldr, kt.fSys, kt.rFactory, kt.tFactory)
 		if err != nil {
-			errs.Append(err)
+			return nil, err
-			continue
 		}
 		result = append(result, target)
 	}
-	if len(errs.Get()) > 0 {
-		return nil, errs
-	}
 	return result, nil
 }
 
@@ -328,12 +331,9 @@ func (kt *KustTarget) newTransformer(
 // resolveVars returns a map of Var names to their final values.
 // The values are strings intended for substitution wherever
 // the $(var.Name) occurs.
-func (kt *KustTarget) resolveVars(m resmap.ResMap) (map[string]string, error) {
+func (kt *KustTarget) resolveVars(
+	m resmap.ResMap, vars map[string]types.Var) (map[string]string, error) {
 	result := map[string]string{}
-	vars, err := kt.getAllVars()
-	if err != nil {
-		return result, err
-	}
 	for _, v := range vars {
 		id := resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name)
 		if r, found := m.DemandOneMatchForId(id); found {
@@ -349,32 +349,53 @@ func (kt *KustTarget) resolveVars(m resmap.ResMap) (map[string]string, error) {
 	return result, nil
 }
 
-// getAllVars returns all the "environment" style Var instances defined in the app.
+type ErrVarCollision struct {
-func (kt *KustTarget) getAllVars() ([]types.Var, error) {
+	name string
-	var result []types.Var
+	path string
-	errs := &interror.KustomizationErrors{}
+}
 
-	bases, err := kt.loadBasesAsKustTargets()
+func (e ErrVarCollision) Error() string {
+	return fmt.Sprintf(
+		"var %s in %s defined in some other kustomization",
+		e.name, e.path)
+}
+
+// getAllVars returns a map of Var names to Var instances, pulled from
+// this kustomization and the kustomizations it depends on.
+// An error is returned on Var name collision.
+func (kt *KustTarget) getAllVars() (map[string]types.Var, error) {
+	result, err := kt.getVarsFromBases()
 	if err != nil {
 		return nil, err
 	}
-
-	// TODO: computing vars and resources for bases can be combined
-	for _, b := range bases {
-		vars, err := b.getAllVars()
-		if err != nil {
-			errs.Append(err)
-			continue
-		}
-		b.ldr.Cleanup()
-		result = append(result, vars...)
-	}
 	for _, v := range kt.kustomization.Vars {
 		v.Defaulting()
-		result = append(result, v)
+		if _, oops := result[v.Name]; oops {
+			return nil, ErrVarCollision{v.Name, kt.ldr.Root()}
+		}
+		result[v.Name] = v
+	}
+	return result, nil
+}
+
+func (kt *KustTarget) getVarsFromBases() (map[string]types.Var, error) {
+	targets, err := kt.loadBasesAsKustTargets()
+	if err != nil {
+		return nil, err
+	}
+	result := make(map[string]types.Var)
+	for _, subKt := range targets {
+		vars, err := subKt.getAllVars()
+		if err != nil {
+			return nil, err
+		}
+		subKt.ldr.Cleanup()
+		for k, v := range vars {
+			if _, oops := result[k]; oops {
+				return nil, ErrVarCollision{v.Name, subKt.ldr.Root()}
+			}
+			result[k] = v
 		}
-	if len(errs.Get()) > 0 {
-		return nil, errs
 	}
 	return result, nil
 }

pkg/target/kusttarget_test.go

@@ -20,6 +20,7 @@ import (
 	"encoding/base64"
 	"path/filepath"
 	"reflect"
+	"sort"
 	"strings"
 	"testing"
 
@@ -351,6 +352,7 @@ bases:
 	}
 }
 
+// To simplify tests, these vars specified in alphabetical order.
 var someVars = []types.Var{
 	{
 		Name: "AWARD",
@@ -387,7 +389,7 @@ var someVars = []types.Var{
 func TestGetAllVarsSimple(t *testing.T) {
 	ldr := loadertest.NewFakeLoader(
 		"/app")
-	write(t, ldr, "/app", `
+	writeK(t, ldr, "/app", `
 vars:
   - name: AWARD
     objref:
@@ -409,17 +411,25 @@ vars:
 	if len(vars) != 2 {
 		t.Fatalf("unexpected size %d", len(vars))
 	}
-	for i, v := range someVars[:2] {
+	for i, k := range sortedKeys(vars)[:2] {
-		if !reflect.DeepEqual(vars[i], v) {
+		if !reflect.DeepEqual(vars[k], someVars[i]) {
-			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[i], v)
+			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[k], someVars[i])
 		}
 	}
 }
 
+func sortedKeys(m map[string]types.Var) (result []string) {
+	for k := range m {
+		result = append(result, k)
+	}
+	sort.Strings(result)
+	return
+}
+
 func TestGetAllVarsNested(t *testing.T) {
 	ldr := loadertest.NewFakeLoader(
 		"/app/overlays/o2")
-	write(t, ldr, "/app/base", `
+	writeK(t, ldr, "/app/base", `
 vars:
   - name: AWARD
     objref:
@@ -434,7 +444,7 @@ vars:
       name: heron
       apiVersion: v300
 `)
-	write(t, ldr, "/app/overlays/o1", `
+	writeK(t, ldr, "/app/overlays/o1", `
 vars:
   - name: FRUIT
     objref:
@@ -443,7 +453,7 @@ vars:
 bases:
 - ../../base
 `)
-	write(t, ldr, "/app/overlays/o2", `
+	writeK(t, ldr, "/app/overlays/o2", `
 vars:
   - name: VEGETABLE
     objref:
@@ -459,18 +469,17 @@ bases:
 	if len(vars) != 4 {
 		t.Fatalf("unexpected size %d", len(vars))
 	}
-	for i, v := range someVars {
+	for i, k := range sortedKeys(vars) {
-		if !reflect.DeepEqual(vars[i], v) {
+		if !reflect.DeepEqual(vars[k], someVars[i]) {
-			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[i], v)
+			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[k], someVars[i])
 		}
 	}
 }
 
-// TODO(monopole): Fix #634
+func TestVarCollisionsForbidden(t *testing.T) {
-func TestGetAllVarsCollisionsOkay_ReproIssue634(t *testing.T) {
 	ldr := loadertest.NewFakeLoader(
 		"/app/overlays/o2")
-	write(t, ldr, "/app/base", `
+	writeK(t, ldr, "/app/base", `
 vars:
   - name: AWARD
     objref:
@@ -485,7 +494,7 @@ vars:
       name: heron
       apiVersion: v300
 `)
-	write(t, ldr, "/app/overlays/o1", `
+	writeK(t, ldr, "/app/overlays/o1", `
 vars:
   - name: AWARD
     objref:
@@ -494,7 +503,7 @@ vars:
 bases:
 - ../../base
 `)
-	write(t, ldr, "/app/overlays/o2", `
+	writeK(t, ldr, "/app/overlays/o2", `
 vars:
   - name: VEGETABLE
     objref:
@@ -503,20 +512,11 @@ vars:
 bases:
 - ../o1
 `)
-	vars, err := makeKustTarget(t, ldr).getAllVars()
+	_, err := makeKustTarget(t, ldr).getAllVars()
-	if err != nil {
+	if err == nil {
-		t.Fatalf("Err: %v", err)
+		t.Fatalf("expected var collision")
-	}
-	if len(vars) != 4 {
-		t.Fatalf("unexpected size %d", len(vars))
-	}
-	v2 := someVars[2]
-	v2.Name = "AWARD"
-	v2.ObjRef.Name = "academy"
-	expected := []types.Var{someVars[0], someVars[1], v2, someVars[3]}
-	for i, v := range expected {
-		if !reflect.DeepEqual(vars[i], v) {
-			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[i], v)
 	}
+	if _, ok := err.(ErrVarCollision); !ok {
+		t.Fatalf("unexpected error: %v", err)
 	}
 }