Add test for issue 3489 and improve error messages.

api/filters/fieldspec/fieldspec.go

@@ -41,7 +41,7 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) {
 	if err := fltr.filter(obj); err != nil {
 		s, _ := obj.String()
 		return nil, errors.WrapPrefixf(err,
-			"obj '%s' at path '%v'", s, fltr.FieldSpec.Path)
+			"considering field '%s' of object\n%v", fltr.FieldSpec.Path, s)
 	}
 	return obj, nil
 }

api/filters/fieldspec/fieldspec_test.go

@@ -176,8 +176,11 @@ kind: Bar
 a:
   b: a
 `,
-		error: "obj 'kind: Bar\na:\n  b: a\n' at path 'a/b/c': " +
-			"expected sequence or mapping node",
+		error: `considering field 'a/b/c' of object
+kind: Bar
+a:
+  b: a
+: expected sequence or mapping node`,
 		filter: fieldspec.Filter{
 			SetValue: filtersutil.SetScalar("e"),
 		},

api/filters/nameref/nameref.go

@@ -5,12 +5,13 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/pkg/errors"
 	"sigs.k8s.io/kustomize/api/filters/fieldspec"
 	"sigs.k8s.io/kustomize/api/resid"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/api/types"
-	kyaml_filtersutil "sigs.k8s.io/kustomize/kyaml/filtersutil"
+	"sigs.k8s.io/kustomize/kyaml/filtersutil"
 	"sigs.k8s.io/kustomize/kyaml/kio"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
@@ -38,18 +39,28 @@ func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 	return kio.FilterAll(yaml.FilterFunc(f.run)).Filter(nodes)
 }
 
-// The node passed in here is the same node as held in Referrer, and
+// The node passed in here is the same node as held in Referrer;
 // that's how the referrer's name field is updated.
-// However, this filter still needs the extra methods on Referrer
+// Currently, however, this filter still needs the extra methods on Referrer
 // to consult things like the resource Id, its namespace, etc.
+// TODO(3455): No filter should use the Resource api; all information
+// about names should come from annotations, with helper methods
+// on the RNode object.  Resource should get stupider, RNode smarter.
 func (f Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
-	err := node.PipeE(fieldspec.Filter{
+	if err := node.PipeE(fieldspec.Filter{
 		FieldSpec: f.NameFieldToUpdate,
 		SetValue:  f.set,
-	})
-	return node, err
+	}); err != nil {
+		return nil, errors.Wrapf(
+			err, "updating name reference in '%s' field of '%s'",
+			f.NameFieldToUpdate.Path, f.Referrer.CurId().String())
+	}
+	return node, nil
 }
 
+// This function is called at many nodes in the YAML doc tree.
+// Only on first entry can one expect the argument to match the
+// top-level node backing the Referrer.
 func (f Filter) set(node *yaml.RNode) error {
 	if yaml.IsMissingOrNull(node) {
 		return nil
@@ -65,8 +76,7 @@ func (f Filter) set(node *yaml.RNode) error {
 			setMappingFn: f.setMapping,
 		}, node)
 	default:
-		return fmt.Errorf(
-			"node is expected to be either a string or a slice of string or a map of string")
+		return fmt.Errorf("node must be a scalar, sequence or map")
 	}
 }
 
@@ -76,16 +86,19 @@ func (f Filter) setMapping(node *yaml.RNode) error {
 		return fmt.Errorf("expect a mapping node")
 	}
 	nameNode, err := node.Pipe(yaml.FieldMatcher{Name: "name"})
-	if err != nil || nameNode == nil {
-		return fmt.Errorf("cannot find field 'name' in node")
+	if err != nil {
+		return errors.Wrap(err, "trying to match 'name' field")
+	}
+	if nameNode == nil {
+		return fmt.Errorf("no 'name' field in node")
 	}
 	namespaceNode, err := node.Pipe(yaml.FieldMatcher{Name: "namespace"})
 	if err != nil {
-		return fmt.Errorf("error when find field 'namespace'")
+		return errors.Wrap(err, "trying to match 'namespace' field")
 	}
 
 	// name will not be updated if the namespace doesn't match
-	subset := f.ReferralCandidates.Resources()
+	candidates := f.ReferralCandidates.Resources()
 	if namespaceNode != nil {
 		namespace := namespaceNode.YNode().Value
 		bynamespace := f.ReferralCandidates.GroupedByOriginalNamespace()
@@ -95,57 +108,57 @@ func (f Filter) setMapping(node *yaml.RNode) error {
 				return nil
 			}
 		}
-		subset = bynamespace[namespace]
+		candidates = bynamespace[namespace]
 	}
 
 	oldName := nameNode.YNode().Value
-	res, err := f.selectReferral(oldName, subset)
-	if err != nil || res == nil {
-		// Nil res means nothing to do.
+	referral, err := f.selectReferral(oldName, candidates)
+	if err != nil || referral == nil {
+		// Nil referral means nothing to do.
 		return err
 	}
-	f.recordTheReferral(res)
-	if res.GetName() == oldName && res.GetNamespace() == "" {
+	f.recordTheReferral(referral)
+	if referral.GetName() == oldName && referral.GetNamespace() == "" {
 		// The name has not changed, nothing to do.
 		return nil
 	}
 	err = node.PipeE(yaml.FieldSetter{
 		Name:        "name",
-		StringValue: res.GetName(),
+		StringValue: referral.GetName(),
 	})
 	if err != nil {
 		return err
 	}
-	if res.GetNamespace() != "" {
+	if referral.GetNamespace() != "" {
 		// We don't want value "" to replace value "default" since
 		// the empty string is handled as a wild card here not default namespace
 		// by kubernetes.
 		err = node.PipeE(yaml.FieldSetter{
 			Name:        "namespace",
-			StringValue: res.GetNamespace(),
+			StringValue: referral.GetNamespace(),
 		})
 	}
 	return err
 }
 
 func (f Filter) setScalar(node *yaml.RNode) error {
-	res, err := f.selectReferral(
+	referral, err := f.selectReferral(
 		node.YNode().Value, f.ReferralCandidates.Resources())
-	if err != nil || res == nil {
-		// Nil res means nothing to do.
+	if err != nil || referral == nil {
+		// Nil referral means nothing to do.
 		return err
 	}
-	f.recordTheReferral(res)
-	if res.GetName() == node.YNode().Value {
+	f.recordTheReferral(referral)
+	if referral.GetName() == node.YNode().Value {
 		// The name has not changed, nothing to do.
 		return nil
 	}
-	return node.PipeE(yaml.FieldSetter{StringValue: res.GetName()})
+	return node.PipeE(yaml.FieldSetter{StringValue: referral.GetName()})
 }
 
-// In the resource, make a note that it is referred to by the referrer.
-func (f Filter) recordTheReferral(res *resource.Resource) {
-	res.AppendRefBy(f.Referrer.CurId())
+// In the resource, make a note that it is referred to by the Referrer.
+func (f Filter) recordTheReferral(referral *resource.Resource) {
+	referral.AppendRefBy(f.Referrer.CurId())
 }
 
 func (f Filter) isRoleRef() bool {
@@ -155,7 +168,7 @@ func (f Filter) isRoleRef() bool {
 // getRoleRefGvk returns a Gvk in the roleRef field. Return error
 // if the roleRef, roleRef/apiGroup or roleRef/kind is missing.
 func getRoleRefGvk(res json.Marshaler) (*resid.Gvk, error) {
-	n, err := kyaml_filtersutil.GetRNode(res)
+	n, err := filtersutil.GetRNode(res)
 	if err != nil {
 		return nil, err
 	}
@@ -213,7 +226,7 @@ func (f Filter) filterReferralCandidates(
 // namespace.
 func (f Filter) selectReferral(
 	oldName string,
-	candidateSubset []*resource.Resource) (*resource.Resource, error) {
+	referralCandidates []*resource.Resource) (*resource.Resource, error) {
 	var roleRefGvk *resid.Gvk
 	if f.isRoleRef() {
 		var err error
@@ -222,11 +235,11 @@ func (f Filter) selectReferral(
 			return nil, err
 		}
 	}
-	for _, res := range candidateSubset {
-		if res.GetOriginalName() != oldName {
+	for _, candidate := range referralCandidates {
+		if candidate.GetOriginalName() != oldName {
 			continue
 		}
-		id := res.OrgId()
+		id := candidate.OrgId()
 		if !id.IsSelected(&f.ReferralTarget) {
 			continue
 		}
@@ -242,28 +255,26 @@ func (f Filter) selectReferral(
 			filteredMatches := f.filterReferralCandidates(matches)
 			if len(filteredMatches) > 1 {
 				return nil, fmt.Errorf(
-					"multiple matches for %s:\n  %v",
-					id, getIds(filteredMatches))
+					"cannot fix name in '%s' field of referrer '%s';"+
+						" found multiple possible referrals: %v",
+					f.NameFieldToUpdate.Path,
+					f.Referrer.CurId(),
+					getIds(filteredMatches))
 			}
 			// Check is the match the resource we are working on
-			if len(filteredMatches) == 0 || res != filteredMatches[0] {
+			if len(filteredMatches) == 0 || candidate != filteredMatches[0] {
 				continue
 			}
 		}
-		// In the resource, note that it is referenced
-		// by the referrer.
-		res.AppendRefBy(f.Referrer.CurId())
-		// Return transformed name of the object,
-		// complete with prefixes, hashes, etc.
-		return res, nil
+		return candidate, nil
 	}
 	return nil, nil
 }
 
-func getIds(rs []*resource.Resource) []string {
+func getIds(rs []*resource.Resource) string {
 	var result []string
 	for _, r := range rs {
-		result = append(result, r.CurId().String()+"\n")
+		result = append(result, r.CurId().String())
 	}
-	return result
+	return strings.Join(result, ", ")
 }

api/filters/nameref/nameref_test.go

@@ -14,14 +14,14 @@ import (
 
 func TestNamerefFilter(t *testing.T) {
 	testCases := map[string]struct {
-		input         string
-		candidates    string
-		expected      string
-		filter        Filter
-		originalNames []string
+		referrerOriginal string
+		candidates       string
+		referrerFinal    string
+		filter           Filter
+		originalNames    []string
 	}{
 		"simple scalar": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -41,7 +41,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", ""},
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -59,7 +59,7 @@ ref:
 			},
 		},
 		"sequence": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -80,7 +80,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName1", ""},
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -99,7 +99,7 @@ seq:
 			},
 		},
 		"mapping": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -119,7 +119,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", ""},
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -137,7 +137,7 @@ map:
 			},
 		},
 		"mapping with namespace": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -159,7 +159,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", ""},
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -178,7 +178,7 @@ map:
 			},
 		},
 		"null value": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -198,7 +198,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", ""},
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -220,7 +220,7 @@ map:
 	for tn, tc := range testCases {
 		t.Run(tn, func(t *testing.T) {
 			factory := provider.NewDefaultDepProvider().GetResourceFactory()
-			referrer, err := factory.FromBytes([]byte(tc.input))
+			referrer, err := factory.FromBytes([]byte(tc.referrerOriginal))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -237,9 +237,9 @@ map:
 			tc.filter.ReferralCandidates = candidates
 
 			if !assert.Equal(t,
-				strings.TrimSpace(tc.expected),
+				strings.TrimSpace(tc.referrerFinal),
 				strings.TrimSpace(
-					filtertest_test.RunFilter(t, tc.input, tc.filter))) {
+					filtertest_test.RunFilter(t, tc.referrerOriginal, tc.filter))) {
 				t.FailNow()
 			}
 		})
@@ -248,14 +248,14 @@ map:
 
 func TestNamerefFilterUnhappy(t *testing.T) {
 	testCases := map[string]struct {
-		input         string
-		candidates    string
-		expected      string
-		filter        Filter
-		originalNames []string
+		referrerOriginal string
+		candidates       string
+		referrerFinal    string
+		filter           Filter
+		originalNames    []string
 	}{
 		"multiple match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -275,7 +275,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", "oldName"},
-			expected:      "",
+			referrerFinal: "",
 			filter: Filter{
 				NameFieldToUpdate: types.FieldSpec{Path: "ref/name"},
 				ReferralTarget: resid.Gvk{
@@ -286,7 +286,7 @@ metadata:
 			},
 		},
 		"no name": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -306,7 +306,7 @@ metadata:
   name: newName2
 `,
 			originalNames: []string{"oldName", "oldName"},
-			expected:      "",
+			referrerFinal: "",
 			filter: Filter{
 				NameFieldToUpdate: types.FieldSpec{Path: "ref"},
 				ReferralTarget: resid.Gvk{
@@ -321,7 +321,7 @@ metadata:
 	for tn, tc := range testCases {
 		t.Run(tn, func(t *testing.T) {
 			factory := provider.NewDefaultDepProvider().GetResourceFactory()
-			referrer, err := factory.FromBytes([]byte(tc.input))
+			referrer, err := factory.FromBytes([]byte(tc.referrerOriginal))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -337,11 +337,11 @@ metadata:
 			candidates := resMapFactory.FromResourceSlice(candidatesRes)
 			tc.filter.ReferralCandidates = candidates
 
-			_, err = filtertest_test.RunFilterE(t, tc.input, tc.filter)
+			_, err = filtertest_test.RunFilterE(t, tc.referrerOriginal, tc.filter)
 			if err == nil {
 				t.Fatalf("expect an error")
 			}
-			if tc.expected != "" && !assert.EqualError(t, err, tc.expected) {
+			if tc.referrerFinal != "" && !assert.EqualError(t, err, tc.referrerFinal) {
 				t.FailNow()
 			}
 		})
@@ -350,19 +350,19 @@ metadata:
 
 func TestCandidatesWithDifferentPrefixSuffix(t *testing.T) {
 	testCases := map[string]struct {
-		input         string
-		candidates    string
-		expected      string
-		filter        Filter
-		originalNames []string
-		prefix        []string
-		suffix        []string
-		inputPrefix   string
-		inputSuffix   string
-		err           bool
+		referrerOriginal string
+		candidates       string
+		referrerFinal    string
+		filter           Filter
+		originalNames    []string
+		prefix           []string
+		suffix           []string
+		inputPrefix      string
+		inputSuffix      string
+		err              bool
 	}{
 		"prefix match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -386,7 +386,7 @@ metadata:
 			suffix:        []string{"", "suffix2"},
 			inputPrefix:   "prefix1",
 			inputSuffix:   "",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -405,7 +405,7 @@ ref:
 			err: false,
 		},
 		"suffix match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -429,7 +429,7 @@ metadata:
 			suffix:        []string{"suffix1", "suffix2"},
 			inputPrefix:   "",
 			inputSuffix:   "suffix1",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -448,7 +448,7 @@ ref:
 			err: false,
 		},
 		"prefix suffix both match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -472,7 +472,7 @@ metadata:
 			suffix:        []string{"suffix1", "suffix2"},
 			inputPrefix:   "prefix1",
 			inputSuffix:   "suffix1",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -491,7 +491,7 @@ ref:
 			err: false,
 		},
 		"multiple match: both": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -515,7 +515,7 @@ metadata:
 			suffix:        []string{"suffix", "suffix"},
 			inputPrefix:   "prefix",
 			inputSuffix:   "suffix",
-			expected:      "",
+			referrerFinal: "",
 			filter: Filter{
 				NameFieldToUpdate: types.FieldSpec{Path: "ref/name"},
 				ReferralTarget: resid.Gvk{
@@ -527,7 +527,7 @@ metadata:
 			err: true,
 		},
 		"multiple match: only prefix": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -551,7 +551,7 @@ metadata:
 			suffix:        []string{"", ""},
 			inputPrefix:   "prefix",
 			inputSuffix:   "",
-			expected:      "",
+			referrerFinal: "",
 			filter: Filter{
 				NameFieldToUpdate: types.FieldSpec{Path: "ref/name"},
 				ReferralTarget: resid.Gvk{
@@ -563,7 +563,7 @@ metadata:
 			err: true,
 		},
 		"multiple match: only suffix": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -587,7 +587,7 @@ metadata:
 			suffix:        []string{"suffix", "suffix"},
 			inputPrefix:   "",
 			inputSuffix:   "suffix",
-			expected:      "",
+			referrerFinal: "",
 			filter: Filter{
 				NameFieldToUpdate: types.FieldSpec{Path: "ref/name"},
 				ReferralTarget: resid.Gvk{
@@ -599,7 +599,7 @@ metadata:
 			err: true,
 		},
 		"no match: neither match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -623,7 +623,7 @@ metadata:
 			suffix:        []string{"suffix1", "suffix2"},
 			inputPrefix:   "prefix",
 			inputSuffix:   "suffix",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -642,7 +642,7 @@ ref:
 			err: false,
 		},
 		"no match: prefix doesn't match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -666,7 +666,7 @@ metadata:
 			suffix:        []string{"suffix", "suffix"},
 			inputPrefix:   "prefix",
 			inputSuffix:   "suffix",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -685,7 +685,7 @@ ref:
 			err: false,
 		},
 		"no match: suffix doesn't match": {
-			input: `
+			referrerOriginal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -709,7 +709,7 @@ metadata:
 			suffix:        []string{"suffix1", "suffix2"},
 			inputPrefix:   "prefix",
 			inputSuffix:   "suffix",
-			expected: `
+			referrerFinal: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -732,7 +732,7 @@ ref:
 	for tn, tc := range testCases {
 		t.Run(tn, func(t *testing.T) {
 			factory := provider.NewDefaultDepProvider().GetResourceFactory()
-			referrer, err := factory.FromBytes([]byte(tc.input))
+			referrer, err := factory.FromBytes([]byte(tc.referrerOriginal))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -764,13 +764,15 @@ ref:
 
 			if !tc.err {
 				if !assert.Equal(t,
-					strings.TrimSpace(tc.expected),
+					strings.TrimSpace(tc.referrerFinal),
 					strings.TrimSpace(
-						filtertest_test.RunFilter(t, tc.input, tc.filter))) {
+						filtertest_test.RunFilter(
+							t, tc.referrerOriginal, tc.filter))) {
 					t.FailNow()
 				}
 			} else {
-				_, err := filtertest_test.RunFilterE(t, tc.input, tc.filter)
+				_, err := filtertest_test.RunFilterE(
+					t, tc.referrerOriginal, tc.filter)
 				if err == nil {
 					t.Fatalf("an error is expected")
 				}

api/filters/refvar/refvar_test.go

@@ -243,7 +243,8 @@ metadata:
 data:
   slice:
   - false`,
-			expectedError: `obj 'apiVersion: apps/v1
+			expectedError: `considering field 'data/slice' of object
+apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: dep
@@ -252,7 +253,7 @@ metadata:
 data:
   slice:
   - false
-' at path 'data/slice': invalid value type expect a string`,
+: invalid value type expect a string`,
 			filter: Filter{
 				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),
@@ -268,7 +269,8 @@ metadata:
   name: dep
 data:
   1: str`,
-			expectedError: `obj 'apiVersion: apps/v1
+			expectedError: `considering field 'data' of object
+apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: dep
@@ -276,7 +278,7 @@ metadata:
     config.kubernetes.io/index: '0'
 data:
   1: str
-' at path 'data': invalid map key: value='1', tag='` + yaml.NodeTagInt + `'`,
+: invalid map key: value='1', tag='` + yaml.NodeTagInt + `'`,
 			filter: Filter{
 				MappingFunc: makeMf(map[string]interface{}{
 					"VAR": int64(5),