Merge pull request #631 from monopole/addKustTargetGetAllVarsTest

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.
-	err = transformers.NewNameReferenceTransformer(
+	if kt.tConfig.NameReference != nil {
-		kt.tConfig.NameReference).Transform(m)
+		err = transformers.NewNameReferenceTransformer(
-	if err != nil {
+			kt.tConfig.NameReference).Transform(m)
-		return nil, err
+		if err != nil {
+			return nil, err
+		}
 	}
-
 	// With all the back references fixed, it's OK to resolve Vars.
-	refVars, err := kt.resolveVars(m)
+	err = kt.doVarReplacement(m)
-	if err != nil {
-		return nil, err
-	}
-	err = transformers.NewRefVarTransformer(
-		refVars, kt.tConfig.VarReference).Transform(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
 	}
-	if len(errs.Get()) > 0 {
+	return result, nil
-		return nil, errs
+}
+
+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
+		}
 	}
 	return result, nil
 }

pkg/target/kusttarget_test.go

@@ -20,6 +20,7 @@ import (
 	"encoding/base64"
 	"path/filepath"
 	"reflect"
+	"sort"
 	"strings"
 	"testing"
 
@@ -350,3 +351,172 @@ bases:
 		t.Fatalf("Empty map.")
 	}
 }
+
+// To simplify tests, these vars specified in alphabetical order.
+var someVars = []types.Var{
+	{
+		Name: "AWARD",
+		ObjRef: types.Target{
+			APIVersion: "v7",
+			Gvk:        gvk.Gvk{Kind: "Service"},
+			Name:       "nobelPrize"},
+		FieldRef: types.FieldSelector{FieldPath: "some.arbitrary.path"},
+	},
+	{
+		Name: "BIRD",
+		ObjRef: types.Target{
+			APIVersion: "v300",
+			Gvk:        gvk.Gvk{Kind: "Service"},
+			Name:       "heron"},
+		FieldRef: types.FieldSelector{FieldPath: "metadata.name"},
+	},
+	{
+		Name: "FRUIT",
+		ObjRef: types.Target{
+			Gvk:  gvk.Gvk{Kind: "Service"},
+			Name: "apple"},
+		FieldRef: types.FieldSelector{FieldPath: "metadata.name"},
+	},
+	{
+		Name: "VEGETABLE",
+		ObjRef: types.Target{
+			Gvk:  gvk.Gvk{Kind: "Leafy"},
+			Name: "kale"},
+		FieldRef: types.FieldSelector{FieldPath: "metadata.name"},
+	},
+}
+
+func TestGetAllVarsSimple(t *testing.T) {
+	ldr := loadertest.NewFakeLoader(
+		"/app")
+	writeK(t, ldr, "/app", `
+vars:
+  - name: AWARD
+    objref:
+      kind: Service
+      name: nobelPrize
+      apiVersion: v7
+    fieldref:
+      fieldpath: some.arbitrary.path
+  - name: BIRD
+    objref:
+      kind: Service
+      name: heron
+      apiVersion: v300
+`)
+	vars, err := makeKustTarget(t, ldr).getAllVars()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	if len(vars) != 2 {
+		t.Fatalf("unexpected size %d", len(vars))
+	}
+	for i, k := range sortedKeys(vars)[:2] {
+		if !reflect.DeepEqual(vars[k], someVars[i]) {
+			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")
+	writeK(t, ldr, "/app/base", `
+vars:
+  - name: AWARD
+    objref:
+      kind: Service
+      name: nobelPrize
+      apiVersion: v7
+    fieldref:
+      fieldpath: some.arbitrary.path
+  - name: BIRD
+    objref:
+      kind: Service
+      name: heron
+      apiVersion: v300
+`)
+	writeK(t, ldr, "/app/overlays/o1", `
+vars:
+  - name: FRUIT
+    objref:
+      kind: Service
+      name: apple
+bases:
+- ../../base
+`)
+	writeK(t, ldr, "/app/overlays/o2", `
+vars:
+  - name: VEGETABLE
+    objref:
+      kind: Leafy
+      name: kale
+bases:
+- ../o1
+`)
+	vars, err := makeKustTarget(t, ldr).getAllVars()
+	if err != nil {
+		t.Fatalf("Err: %v", err)
+	}
+	if len(vars) != 4 {
+		t.Fatalf("unexpected size %d", len(vars))
+	}
+	for i, k := range sortedKeys(vars) {
+		if !reflect.DeepEqual(vars[k], someVars[i]) {
+			t.Fatalf("unexpected var[%d]:\n  %v\n  %v", i, vars[k], someVars[i])
+		}
+	}
+}
+
+func TestVarCollisionsForbidden(t *testing.T) {
+	ldr := loadertest.NewFakeLoader(
+		"/app/overlays/o2")
+	writeK(t, ldr, "/app/base", `
+vars:
+  - name: AWARD
+    objref:
+      kind: Service
+      name: nobelPrize
+      apiVersion: v7
+    fieldref:
+      fieldpath: some.arbitrary.path
+  - name: BIRD
+    objref:
+      kind: Service
+      name: heron
+      apiVersion: v300
+`)
+	writeK(t, ldr, "/app/overlays/o1", `
+vars:
+  - name: AWARD
+    objref:
+      kind: Service
+      name: academy
+bases:
+- ../../base
+`)
+	writeK(t, ldr, "/app/overlays/o2", `
+vars:
+  - name: VEGETABLE
+    objref:
+      kind: Leafy
+      name: kale
+bases:
+- ../o1
+`)
+	_, err := makeKustTarget(t, ldr).getAllVars()
+	if err == nil {
+		t.Fatalf("expected var collision")
+	}
+	if _, ok := err.(ErrVarCollision); !ok {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}