Improve error handling during var resolution.

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 {