Merge pull request #827 from monopole/fix826

Improve error handling during var resolution.

Author: monopole

pkg/commands/build/build_test.go

@@ -22,6 +22,12 @@ import (
 	"sigs.k8s.io/kustomize/pkg/constants"
 )
 
+func TestNewOptionsToSilenceCodeInspectionError(t *testing.T) {
+	if NewOptions("foo", "bar") == nil {
+		t.Fatal("could not make new options")
+	}
+}
+
 func TestBuildValidate(t *testing.T) {
 	var cases = []struct {
 		name  string

pkg/commands/edit/add/flagsandargs.go

@@ -94,7 +94,7 @@ func (a *flagsAndArgs) ExpandFileSource(fSys fs.FileSystem) error {
 		if key != "" {
 			if len(result) != 1 {
 				return fmt.Errorf(
-					"'pattern '%s' catches files %v, should catch only one.", pattern, result)
+					"'pattern '%s' catches files %v, should catch only one", pattern, result)
 			}
 			fileSource := fmt.Sprintf("%s=%s", key, result[0])
 			results = append(results, fileSource)

pkg/expansion/expand.go

@@ -44,7 +44,6 @@ func MappingFuncFor(context ...map[string]string) func(string) string {
 				return val
 			}
 		}
-
 		return syntaxWrap(input)
 	}
 }

pkg/expansion/expand_test.go

@@ -46,9 +46,7 @@ func TestMapReference(t *testing.T) {
 		"BLU": "$(ZOO)-2",
 	}
 
-	serviceEnv := map[string]string{}
-
-	mapping := MappingFuncFor(declaredEnv, serviceEnv)
+	mapping := MappingFuncFor(declaredEnv)
 
 	for _, env := range envs {
 		declaredEnv[env.Name] = Expand(env.Value, mapping)

pkg/resmap/resmap.go

@@ -46,15 +46,6 @@ func (m ResMap) GetMatchingIds(matches IdMatcher) []resid.ResId {
 	return result
 }
 
-// DemandOneGvknMatchForId find the matched resource by Group/Version/Kind and Name
-func (m ResMap) DemandOneGvknMatchForId(inputId resid.ResId) (*resource.Resource, bool) {
-	result := m.GetMatchingIds(inputId.GvknEquals)
-	if len(result) == 1 {
-		return m[result[0]], true
-	}
-	return nil, false
-}
-
 // EncodeAsYaml encodes a ResMap to YAML; encoded objects separated by `---`.
 func (m ResMap) EncodeAsYaml() ([]byte, error) {
 	var ids []resid.ResId

pkg/resmap/resmap_test.go

@@ -92,35 +92,34 @@ func TestDemandOneGvknMatchForId(t *testing.T) {
 			}),
 	}
 
-	_, ok := rm1.DemandOneGvknMatchForId(
-		resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix1", "ns1"))
-	if !ok {
-		t.Fatal("Expected single map entry but got none")
+	result := rm1.GetMatchingIds(
+		resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix1", "ns1").GvknEquals)
+	if len(result) != 1 {
+		t.Fatalf("Expected single map entry but got %v", result)
 	}
 
 	// confirm that ns and prefix are not included in match
-	_, ok = rm1.DemandOneGvknMatchForId(
-		resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix", "ns"))
-	if !ok {
-		t.Fatal("Expected single map entry but got none")
+	result = rm1.GetMatchingIds(
+		resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix", "ns").GvknEquals)
+	if len(result) != 1 {
+		t.Fatalf("Expected single map entry but got %v", result)
 	}
 
 	// confirm that name is matched correctly
-	result, ok := rm1.DemandOneGvknMatchForId(
-		resid.NewResIdWithPrefixNamespace(cmap, "cm3", "prefix1", "ns1"))
-	if ok {
+	result = rm1.GetMatchingIds(
+		resid.NewResIdWithPrefixNamespace(cmap, "cm3", "prefix1", "ns1").GvknEquals)
+	if len(result) > 0 {
 		t.Fatalf("Expected no map entries but got %v", result)
 	}
 
 	cmap2 := gvk.Gvk{Version: "v2", Kind: "ConfigMap"}
 
 	// confirm that gvk is matched correctly
-	result, ok = rm1.DemandOneGvknMatchForId(
-		resid.NewResIdWithPrefixNamespace(cmap2, "cm2", "prefix1", "ns1"))
-	if ok {
+	result = rm1.GetMatchingIds(
+		resid.NewResIdWithPrefixNamespace(cmap2, "cm2", "prefix1", "ns1").GvknEquals)
+	if len(result) > 0 {
 		t.Fatalf("Expected no map entries but got %v", result)
 	}
-
 }
 
 func TestFilterBy(t *testing.T) {

pkg/target/resaccumulator.go

@@ -18,7 +18,6 @@ package target
 
 import (
 	"fmt"
-	"log"
 	"sigs.k8s.io/kustomize/pkg/resid"
 	"sigs.k8s.io/kustomize/pkg/resmap"
 	"sigs.k8s.io/kustomize/pkg/transformers"
@@ -101,17 +100,27 @@ func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) {
 // for substitution wherever the $(var.Name) occurs.
 func (ra *ResAccumulator) makeVarReplacementMap() (map[string]string, error) {
 	result := map[string]string{}
-	for _, v := range ra.varSet.Set() {
-		id := resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name)
-		if r, found := ra.resMap.DemandOneGvknMatchForId(id); found {
-			s, err := r.GetFieldValue(v.FieldRef.FieldPath)
+	for _, v := range ra.Vars() {
+		matched := ra.resMap.GetMatchingIds(
+			resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals)
+		if len(matched) > 1 {
+			return nil, fmt.Errorf(
+				"found %d resId matches for var %s "+
+					"(unable to disambiguate)",
+				len(matched), v)
+		}
+		if len(matched) == 1 {
+			s, err := ra.resMap[matched[0]].GetFieldValue(v.FieldRef.FieldPath)
 			if err != nil {
-				return nil, fmt.Errorf("field path err for var: %+v", v)
+				return nil, fmt.Errorf(
+					"field specified in var '%v' "+
+						"not found in corresponding resource", v)
 			}
 			result[v.Name] = s
 		} else {
-			// Should this be an error?
-			log.Printf("var %v defined but not used", v)
+			return nil, fmt.Errorf(
+				"var '%v' cannot be mapped to a field "+
+					"in the set of known resources", v)
 		}
 	}
 	return result, nil

pkg/target/resaccumulator_test.go

@@ -30,11 +30,11 @@ import (
 	"sigs.k8s.io/kustomize/pkg/types"
 )
 
-func TestResolveVars(t *testing.T) {
+func makeResAccumulator() (*ResAccumulator, *resource.Factory, error) {
 	ra := MakeEmptyAccumulator()
 	err := ra.MergeConfig(config.MakeDefaultConfig())
 	if err != nil {
-		t.Fatalf("unexpected err: %v", err)
+		return nil, nil, err
 	}
 	rf := resource.NewFactory(
 		kunstruct.NewKunstructuredFactoryImpl())
@@ -55,8 +55,8 @@ func TestResolveVars(t *testing.T) {
 								map[string]interface{}{
 									"command": []interface{}{
 										"myserver",
-										"--somebackendService $(FOO)",
-										"--yetAnother $(BAR)",
+										"--somebackendService $(SERVICE_ONE)",
+										"--yetAnother $(SERVICE_TWO)",
 									},
 								},
 							},
@@ -85,14 +85,23 @@ func TestResolveVars(t *testing.T) {
 				},
 			}),
 	})
+	return ra, rf, nil
+}
+
+func TestResolveVarsHappy(t *testing.T) {
+	ra, _, err := makeResAccumulator()
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
 	err = ra.MergeVars([]types.Var{
 		{
-			Name: "FOO",
+			Name: "SERVICE_ONE",
 			ObjRef: types.Target{
 				Gvk:  gvk.Gvk{Version: "v1", Kind: "Service"},
 				Name: "backendOne"},
-		}, {
-			Name: "BAR",
+		},
+		{
+			Name: "SERVICE_TWO",
 			ObjRef: types.Target{
 				Gvk:  gvk.Gvk{Version: "v1", Kind: "Service"},
 				Name: "backendTwo"},
@@ -111,6 +120,101 @@ func TestResolveVars(t *testing.T) {
 	}
 }
 
+func TestResolveVarsVarNeedsDisambiguation(t *testing.T) {
+	ra, rf, err := makeResAccumulator()
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	ra.MergeResourcesWithErrorOnIdCollision(resmap.ResMap{
+		resid.NewResIdWithPrefixNamespace(
+			gvk.Gvk{Version: "v1", Kind: "Service"},
+			"backendOne", "", "fooNamespace"): rf.FromMap(
+			map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Service",
+				"metadata": map[string]interface{}{
+					"name": "backendOne",
+				},
+			}),
+	})
+
+	err = ra.MergeVars([]types.Var{
+		{
+			Name: "SERVICE_ONE",
+			ObjRef: types.Target{
+				Gvk:  gvk.Gvk{Version: "v1", Kind: "Service"},
+				Name: "backendOne",
+			},
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	err = ra.ResolveVars()
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+	if !strings.Contains(
+		err.Error(), "unable to disambiguate") {
+		t.Fatalf("unexpected err: %v", err)
+	}
+}
+
+func TestResolveVarsGoodResIdBadField(t *testing.T) {
+	ra, _, err := makeResAccumulator()
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	err = ra.MergeVars([]types.Var{
+		{
+			Name: "SERVICE_ONE",
+			ObjRef: types.Target{
+				Gvk:  gvk.Gvk{Version: "v1", Kind: "Service"},
+				Name: "backendOne"},
+			FieldRef: types.FieldSelector{FieldPath: "nope_nope_nope"},
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	err = ra.ResolveVars()
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+	if !strings.Contains(
+		err.Error(),
+		"not found in corresponding resource") {
+		t.Fatalf("unexpected err: %v", err)
+	}
+}
+
+func TestResolveVarsUnmappableVar(t *testing.T) {
+	ra, _, err := makeResAccumulator()
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	err = ra.MergeVars([]types.Var{
+		{
+			Name: "SERVICE_THREE",
+			ObjRef: types.Target{
+				Gvk:  gvk.Gvk{Version: "v1", Kind: "Service"},
+				Name: "doesNotExist"},
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	err = ra.ResolveVars()
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+	if !strings.Contains(
+		err.Error(),
+		"cannot be mapped to a field in the set of known resources") {
+		t.Fatalf("unexpected err: %v", err)
+	}
+}
+
 func find(name string, resMap resmap.ResMap) *resource.Resource {
 	for k, v := range resMap {
 		if k.Name() == name {