Merge pull request #3518 from monopole/nits

Improve name reference transformer testing.

api/filters/nameref/nameref.go

@@ -18,18 +18,25 @@ import (
 
 // Filter updates a name references.
 type Filter struct {
-	// Referrer is the object that refers to something else by a name,
+	// Referrer refers to another resource X by X's name.
-	// a name that this filter seeks to update.
+	// E.g. A Deployment can refer to a ConfigMap.
+	// The Deployment is the Referrer,
+	// the ConfigMap is the ReferralTarget.
+	// This filter seeks to repair the reference in Deployment, given
+	// that the ConfigMap's name may have changed.
 	Referrer *resource.Resource
 
-	// NameFieldToUpdate is the field in the Referrer that holds the
+	// NameFieldToUpdate is the field in the Referrer
-	// name requiring an update.
+	// that holds the name requiring an update.
-	NameFieldToUpdate types.FieldSpec `json:"nameFieldToUpdate,omitempty" yaml:"nameFieldToUpdate,omitempty"`
+	// This is the field to write.
+	NameFieldToUpdate types.FieldSpec
 
-	// Source of the new value for the name (in its name field).
+	// ReferralTarget is the source of the new value for
+	// the name, always in the 'metadata/name' field.
+	// This is the field to read.
 	ReferralTarget resid.Gvk
 
-	// Set of resources to hunt through to find the ReferralTarget.
+	// Set of resources to scan to find the ReferralTarget.
 	ReferralCandidates resmap.ResMap
 }
 
@@ -47,6 +54,10 @@ func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 // 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) {
+	if err := f.confirmNodeMatchesReferrer(node); err != nil {
+		// sanity check.
+		return nil, err
+	}
 	if err := node.PipeE(fieldspec.Filter{
 		FieldSpec: f.NameFieldToUpdate,
 		SetValue:  f.set,
@@ -221,7 +232,7 @@ func (f Filter) filterReferralCandidates(
 
 // selectReferral picks the referral among a subset of candidates.
 // The content of the candidateSubset slice is most of the time
-// identical to the ReferralCandidates resmap. Still in some cases, such
+// identical to the ReferralCandidates ResMap. Still in some cases, such
 // as ClusterRoleBinding, the subset only contains the resources of a specific
 // namespace.
 func (f Filter) selectReferral(
@@ -278,3 +289,37 @@ func getIds(rs []*resource.Resource) string {
 	}
 	return strings.Join(result, ", ")
 }
+
+func checkEqual(k, a, b string) error {
+	if a != b {
+		return fmt.Errorf(
+			"node-referrerOriginal '%s' mismatch '%s' != '%s'",
+			k, a, b)
+	}
+	return nil
+}
+
+func (f Filter) confirmNodeMatchesReferrer(node *yaml.RNode) error {
+	meta, err := node.GetMeta()
+	if err != nil {
+		return err
+	}
+	gvk := f.Referrer.GetGvk()
+	if err = checkEqual(
+		"APIVersion", meta.APIVersion, gvk.ApiVersion()); err != nil {
+		return err
+	}
+	if err = checkEqual(
+		"Kind", meta.Kind, gvk.Kind); err != nil {
+		return err
+	}
+	if err = checkEqual(
+		"Name", meta.Name, f.Referrer.GetName()); err != nil {
+		return err
+	}
+	if err = checkEqual(
+		"Namespace", meta.Namespace, f.Referrer.GetNamespace()); err != nil {
+		return err
+	}
+	return nil
+}

api/internal/accumulator/loadconfigfromcrds.go

@@ -162,7 +162,7 @@ func loadCrdIntoConfig(
 				err = theConfig.AddNamereferenceFieldSpec(
 					builtinconfig.NameBackReferences{
 						Gvk: resid.Gvk{Kind: kind, Version: version},
-						FieldSpecs: []types.FieldSpec{
+						Referrers: []types.FieldSpec{
 							makeFs(theGvk, append(path, propName, nameKey))},
 					})
 				if err != nil {

api/internal/accumulator/loadconfigfromcrds_test.go

@@ -8,10 +8,9 @@ import (
 	"testing"
 
 	"sigs.k8s.io/kustomize/api/filesys"
-	"sigs.k8s.io/kustomize/api/loader"
-
 	. "sigs.k8s.io/kustomize/api/internal/accumulator"
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig"
+	"sigs.k8s.io/kustomize/api/loader"
 	"sigs.k8s.io/kustomize/api/resid"
 	"sigs.k8s.io/kustomize/api/types"
 )
@@ -140,21 +139,19 @@ func TestLoadCRDs(t *testing.T) {
 	nbrs := []builtinconfig.NameBackReferences{
 		{
 			Gvk: resid.Gvk{Kind: "Secret", Version: "v1"},
-			FieldSpecs: []types.FieldSpec{
+			Referrers: []types.FieldSpec{
 				{
-					CreateIfNotPresent: false,
+					Gvk:  resid.Gvk{Kind: "MyKind"},
-					Gvk:                resid.Gvk{Kind: "MyKind"},
+					Path: "spec/secretRef/name",
-					Path:               "spec/secretRef/name",
 				},
 			},
 		},
 		{
 			Gvk: resid.Gvk{Kind: "Bee", Version: "v1beta1"},
-			FieldSpecs: []types.FieldSpec{
+			Referrers: []types.FieldSpec{
 				{
-					CreateIfNotPresent: false,
+					Gvk:  resid.Gvk{Kind: "MyKind"},
-					Gvk:                resid.Gvk{Kind: "MyKind"},
+					Path: "spec/beeRef/name",
-					Path:               "spec/beeRef/name",
 				},
 			},
 		},

api/internal/accumulator/namereferencetransformer.go

@@ -4,11 +4,13 @@
 package accumulator
 
 import (
+	"fmt"
 	"log"
 
 	"sigs.k8s.io/kustomize/api/filters/nameref"
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig"
 	"sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/resource"
 )
 
 type nameReferenceTransformer struct {
@@ -17,6 +19,8 @@ type nameReferenceTransformer struct {
 
 var _ resmap.Transformer = &nameReferenceTransformer{}
 
+type filterMap map[*resource.Resource][]nameref.Filter
+
 // newNameReferenceTransformer constructs a nameReferenceTransformer
 // with a given slice of NameBackReferences.
 func newNameReferenceTransformer(
@@ -29,16 +33,62 @@ func newNameReferenceTransformer(
 
 // Transform updates name references in resource A that
 // refer to resource B, given that B's name may have
-// changed.  A is the referrer, B is the referralTarget.
+// changed.
 //
 // For example, a HorizontalPodAutoscaler (HPA)
 // necessarily refers to a Deployment, the thing that
-// the HPA scales. The Deployment's name might change
+// an HPA scales. In this case:
-// (e.g. prefix added), and the reference in the HPA
-// has to be fixed.
 //
-// In the outer loop over the ResMap below, say we
+//   - the HPA instance is the Referrer,
-// encounter a specific HPA. Then, in scanning the set
+//   - the Deployment instance is the ReferralTarget.
+//
+// If the Deployment's name changes, e.g. a prefix is added,
+// then the HPA's reference to the Deployment must be fixed.
+//
+func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error {
+	fMap := t.determineFilters(m.Resources())
+	// debug(fMap)
+	for r, fList := range fMap {
+		c := m.SubsetThatCouldBeReferencedByResource(r)
+		for _, f := range fList {
+			f.Referrer = r
+			f.ReferralCandidates = c
+			if err := f.Referrer.ApplyFilter(f); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+//nolint: deadcode, unused
+func debug(fMap filterMap) {
+	fmt.Printf("filterMap has %d entries:\n", len(fMap))
+	rCount := 0
+	for r, fList := range fMap {
+		yml, _ := r.AsYAML()
+		rCount++
+		fmt.Printf(`
+---- %3d. possible referrer -------------
+%s
+---------`, rCount, string(yml),
+		)
+		for i, f := range fList {
+			fmt.Printf(`
+%3d/%3d update: %s
+          from: %s
+`, rCount, i+1, f.NameFieldToUpdate.Path, f.ReferralTarget,
+			)
+		}
+	}
+}
+
+// Produce a map from referrer resources that might need to be fixed
+// to filters that might fix them.  The keys to this map are potential
+// referrers, so won't include resources like ConfigMap or Secret.
+//
+// In the inner loop over the resources below, say we
+// encounter an HPA instance. Then, in scanning the set
 // of all known backrefs, we encounter an entry like
 //
 //   - kind: Deployment
@@ -48,64 +98,50 @@ func newNameReferenceTransformer(
 //
 // This entry says that an HPA, via its
 // 'spec/scaleTargetRef/name' field, may refer to a
-// Deployment.  This match to HPA means we may need to
+// Deployment.
-// modify the value in its 'spec/scaleTargetRef/name'
-// field, by searching for the thing it refers to,
-// and getting its new name.
 //
-// As a filter, and search optimization, we compute a
+// This means that a filter will need to hunt for the right Deployment,
-// subset of all resources that the HPA could refer to,
+// obtain it's new name, and write that name into the HPA's
-// by excluding objects from other namespaces, and
+// 'spec/scaleTargetRef/name' field. Return a filter that can do that.
-// excluding objects that don't have the same prefix-
+func (t *nameReferenceTransformer) determineFilters(
-// suffix mods as the HPA.
+	resources []*resource.Resource) (fMap filterMap) {
-//
+	fMap = make(filterMap)
-// We look in this subset for all Deployment objects
+	for _, backReference := range t.backRefs {
-// with a resId that has a Name matching the field value
+		for _, referrerSpec := range backReference.Referrers {
-// present in the HPA.  If no match do nothing; if more
+			for _, res := range resources {
-// than one match, it's an error.
+				if res.OrgId().IsSelected(&referrerSpec.Gvk) {
-//
+					// If this is true, the res might be a referrer, and if
-// We overwrite the HPA name field with the value found
+					// so, the name reference it holds might need an update.
-// in the Deployment's name field (the name in the raw
+					if resHasField(res, referrerSpec.Path) {
-// object - the modified name - not the unmodified name
+						// Optimization - the referrer has the field
-// in the Deployment's resId).
+						// that might need updating.
-//
+						fMap[res] = append(fMap[res], nameref.Filter{
-// This process assumes that the name stored in a ResId
+							// Name field to write in the Referrer.
-// (the ResMap key) isn't modified by name transformers.
+							NameFieldToUpdate: referrerSpec,
-// Name transformers should only modify the name in the
+							// Specification of object class to read from.
-// body of the resource object (the value in the ResMap).
+							// Always read from metadata/name field.
-//
+							ReferralTarget: backReference.Gvk,
-func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error {
+						})
-	// TODO: Too much looping, here and in transitive calls.
-	for _, referrer := range m.Resources() {
-		var candidates resmap.ResMap
-		for _, referralTarget := range t.backRefs {
-			for _, fSpec := range referralTarget.FieldSpecs {
-				if referrer.OrgId().IsSelected(&fSpec.Gvk) {
-					if candidates == nil {
-						// This excludes objects from other namespaces.
-						// In most realistic uses, it returns all elements of m,
-						// (since they're all in the same namespace).
-						candidates = m.SubsetThatCouldBeReferencedByResource(referrer)
-					}
-					// One way to get here is with, say, a referrer that's an
-					// HPA, and a target that's a Deployment (one of the
-					// Deployment's fieldSpecs selects an HPA).  Now we look
-					// through the candidates to see if one is a Deployment
-					// (the target), and if so, get the Deployment's name and
-					// write it into the referrer, at the field specfied in
-					// fSpec.
-					err := referrer.ApplyFilter(nameref.Filter{
-						Referrer:           referrer,
-						NameFieldToUpdate:  fSpec,
-						ReferralTarget:     referralTarget.Gvk,
-						ReferralCandidates: candidates,
-					})
-					if err != nil {
-						return err
 					}
 				}
 			}
 		}
 	}
-	return nil
+	return fMap
+}
+
+// TODO: check res for field existence here to avoid extra work.
+// res.GetFieldValue, which uses yaml.Lookup under the hood, doesn't know
+// how to parse fieldspec-style paths that make no distinction
+// between maps and sequences.  This means it cannot lookup commonly
+// used "indeterminate" paths like
+//    spec/containers/env/valueFrom/configMapKeyRef/name
+// ('containers' is a list, not a map).
+// However, the fieldspec filter does know how to handle this;
+// extract that code and call it here?
+func resHasField(res *resource.Resource, path string) bool {
+	return true
+	// fld := strings.Join(utils.PathSplitter(path), ".")
+	// _, e := res.GetFieldValue(fld)
+	// return e == nil
 }

api/internal/accumulator/namereferencetransformer_test.go

@@ -7,15 +7,16 @@ import (
 	"strings"
 	"testing"
 
-	"sigs.k8s.io/kustomize/api/konfig"
-
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig"
+	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/provider"
 	"sigs.k8s.io/kustomize/api/resid"
 	"sigs.k8s.io/kustomize/api/resmap"
 	resmaptest_test "sigs.k8s.io/kustomize/api/testutils/resmaptest"
 )
 
+const notEqualErrFmt = "expected (self) doesn't match actual (other): %v"
+
 func TestNameReferenceHappyRun(t *testing.T) {
 	m := resmaptest_test.NewRmBuilderDefault(t).AddWithName(
 		"cm1",
@@ -472,7 +473,7 @@ func TestNameReferenceHappyRun(t *testing.T) {
 	}
 
 	if err = expected.ErrorIfNotEqualLists(m); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }
 
@@ -609,7 +610,7 @@ func TestNameReferencePersistentVolumeHappyRun(t *testing.T) {
 	v2.AppendRefBy(c2.CurId())
 
 	if err := m1.ErrorIfNotEqualLists(m2); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }
 
@@ -745,7 +746,7 @@ func TestNameReferenceNamespace(t *testing.T) {
 
 	m.RemoveBuildAnnotations()
 	if err = expected.ErrorIfNotEqualLists(m); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }
 
@@ -907,7 +908,7 @@ func TestNameReferenceClusterWide(t *testing.T) {
 
 	m.RemoveBuildAnnotations()
 	if err = expected.ErrorIfNotEqualLists(m); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }
 
@@ -1034,7 +1035,7 @@ func TestNameReferenceNamespaceTransformation(t *testing.T) {
 
 	m.RemoveBuildAnnotations()
 	if err = expected.ErrorIfNotEqualLists(m); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }
 
@@ -1071,6 +1072,6 @@ func TestNameReferenceCandidateSelection(t *testing.T) {
 
 	m.RemoveBuildAnnotations()
 	if err = expected.ErrorIfNotEqualLists(m); err != nil {
-		t.Fatalf("actual doesn't match expected: %v", err)
+		t.Fatalf(notEqualErrFmt, err)
 	}
 }

api/internal/plugins/builtinconfig/namebackreferences.go

@@ -10,8 +10,8 @@ import (
 	"sigs.k8s.io/kustomize/api/types"
 )
 
-// NameBackReferences is an association between a gvk.GVK and a list
+// NameBackReferences is an association between a gvk.GVK (a ReferralTarget)
-// of FieldSpec instances that could refer to it.
+// and a list of Referrers that could refer to it.
 //
 // It is used to handle name changes, and can be thought of as a
 // a contact list.  If you change your own contact info (name,
@@ -19,16 +19,20 @@ import (
 // know about the change.
 //
 // For example, ConfigMaps can be used by Pods and everything that
-// contains a Pod; Deployment, Job, StatefulSet, etc.  To change
+// contains a Pod; Deployment, Job, StatefulSet, etc.
-// the name of a ConfigMap instance from 'alice' to 'bob', one
+// The ConfigMap is the ReferralTarget, the others are Referrers.
-// must visit all objects that could refer to the ConfigMap, see if
+//
-// they mention 'alice', and if so, change the reference to 'bob'.
+// If the the name of a ConfigMap instance changed from 'alice' to 'bob',
+// one must
+//  - visit all objects that could refer to the ConfigMap (the Referrers)
+//  - see if they mention 'alice',
+//  - if so, change the Referrer's name reference to 'bob'.
 //
 // The NameBackReferences instance to aid in this could look like
 //   {
 //     kind: ConfigMap
 //     version: v1
-//     FieldSpecs:
+//     fieldSpecs:
 //     - kind: Pod
 //       version: v1
 //       path: spec/volumes/configMap/name
@@ -39,13 +43,15 @@ import (
 //       (etc.)
 //   }
 type NameBackReferences struct {
-	resid.Gvk  `json:",inline,omitempty" yaml:",inline,omitempty"`
+	resid.Gvk `json:",inline,omitempty" yaml:",inline,omitempty"`
-	FieldSpecs types.FsSlice `json:"FieldSpecs,omitempty" yaml:"FieldSpecs,omitempty"`
+	// TODO: rename json 'fieldSpecs' to 'referrers' for clarity.
+	// This will, however, break anyone using a custom config.
+	Referrers types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"`
 }
 
 func (n NameBackReferences) String() string {
 	var r []string
-	for _, f := range n.FieldSpecs {
+	for _, f := range n.Referrers {
 		r = append(r, f.String())
 	}
 	return n.Gvk.String() + ":  (\n" +
@@ -77,7 +83,7 @@ func (s nbrSlice) mergeOne(other NameBackReferences) (nbrSlice, error) {
 	found := false
 	for _, c := range s {
 		if c.Gvk.Equals(other.Gvk) {
-			c.FieldSpecs, err = c.FieldSpecs.MergeAll(other.FieldSpecs)
+			c.Referrers, err = c.Referrers.MergeAll(other.Referrers)
 			if err != nil {
 				return nil, err
 			}

api/internal/plugins/builtinconfig/namebackreferences_test.go

@@ -50,13 +50,13 @@ func TestMergeAll(t *testing.T) {
 			Gvk: resid.Gvk{
 				Kind: "ConfigMap",
 			},
-			FieldSpecs: fsSlice1,
+			Referrers: fsSlice1,
 		},
 		{
 			Gvk: resid.Gvk{
 				Kind: "Secret",
 			},
-			FieldSpecs: fsSlice2,
+			Referrers: fsSlice2,
 		},
 	}
 	nbrsSlice2 := nbrSlice{
@@ -64,13 +64,13 @@ func TestMergeAll(t *testing.T) {
 			Gvk: resid.Gvk{
 				Kind: "ConfigMap",
 			},
-			FieldSpecs: fsSlice1,
+			Referrers: fsSlice1,
 		},
 		{
 			Gvk: resid.Gvk{
 				Kind: "Secret",
 			},
-			FieldSpecs: fsSlice2,
+			Referrers: fsSlice2,
 		},
 	}
 	expected := nbrSlice{
@@ -78,13 +78,13 @@ func TestMergeAll(t *testing.T) {
 			Gvk: resid.Gvk{
 				Kind: "ConfigMap",
 			},
-			FieldSpecs: fsSlice1,
+			Referrers: fsSlice1,
 		},
 		{
 			Gvk: resid.Gvk{
 				Kind: "Secret",
 			},
-			FieldSpecs: fsSlice2,
+			Referrers: fsSlice2,
 		},
 	}
 	actual, err := nbrsSlice1.mergeAll(nbrsSlice2)

api/internal/plugins/builtinconfig/transformerconfig_test.go

@@ -24,7 +24,7 @@ func TestAddNamereferenceFieldSpec(t *testing.T) {
 		Gvk: resid.Gvk{
 			Kind: "KindA",
 		},
-		FieldSpecs: []types.FieldSpec{
+		Referrers: []types.FieldSpec{
 			{
 				Gvk: resid.Gvk{
 					Kind: "KindB",
@@ -89,7 +89,7 @@ func TestMerge(t *testing.T) {
 			Gvk: resid.Gvk{
 				Kind: "KindA",
 			},
-			FieldSpecs: []types.FieldSpec{
+			Referrers: []types.FieldSpec{
 				{
 					Gvk: resid.Gvk{
 						Kind: "KindB",
@@ -103,7 +103,7 @@ func TestMerge(t *testing.T) {
 			Gvk: resid.Gvk{
 				Kind: "KindA",
 			},
-			FieldSpecs: []types.FieldSpec{
+			Referrers: []types.FieldSpec{
 				{
 					Gvk: resid.Gvk{
 						Kind: "KindC",

api/internal/wrappy/wnode_test.go

@@ -33,6 +33,38 @@ const (
       "area": "51",
       "greeting": "Take me to your leader."
     }
+  },
+  "spec": {
+    "template": {
+      "spec": {
+        "containers": [
+          {
+            "env": [
+              {
+                "name": "CM_FOO",
+                "valueFrom": {
+                  "configMapKeyRef": {
+                    "key": "somekey",
+                    "name": "myCm"
+                  }
+                }
+              },
+              {
+                "name": "SECRET_FOO",
+                "valueFrom": {
+                  "secretKeyRef": {
+                    "key": "someKey",
+                    "name": "mySecret"
+                  }
+                }
+              }
+            ],
+            "image": "nginx:1.7.9",
+            "name": "nginx"
+          }
+        ]
+      }
+    }
   }
 }
 `
@@ -358,6 +390,51 @@ func TestGetFieldValueReturnsMap(t *testing.T) {
 	}
 }
 
+func TestGetFieldValueReturnsStuff(t *testing.T) {
+	wn := NewWNode()
+	if err := wn.UnmarshalJSON([]byte(deploymentBiggerJson)); err != nil {
+		t.Fatalf("unexpected unmarshaljson err: %v", err)
+	}
+	expected := []interface{}{
+		map[string]interface{}{
+			"env": []interface{}{
+				map[string]interface{}{
+					"name": "CM_FOO",
+					"valueFrom": map[string]interface{}{
+						"configMapKeyRef": map[string]interface{}{
+							"key":  "somekey",
+							"name": "myCm",
+						},
+					},
+				},
+				map[string]interface{}{
+					"name": "SECRET_FOO",
+					"valueFrom": map[string]interface{}{
+						"secretKeyRef": map[string]interface{}{
+							"key":  "someKey",
+							"name": "mySecret",
+						},
+					},
+				},
+			},
+			"image": string("nginx:1.7.9"),
+			"name":  string("nginx"),
+		},
+	}
+	actual, err := wn.GetFieldValue("spec.template.spec.containers")
+	if err != nil {
+		t.Fatalf("error getting field value: %v", err)
+	}
+	if diff := cmp.Diff(expected, actual); diff != "" {
+		t.Fatalf("actual map does not deep equal expected map:\n%v", diff)
+	}
+	// Cannot go deeper yet.
+	_, err = wn.GetFieldValue("spec.template.spec.containers.env")
+	if err == nil {
+		t.Fatalf("expected err %v", err)
+	}
+}
+
 func TestGetFieldValueReturnsSlice(t *testing.T) {
 	bytes, err := yaml.Marshal(makeBigMap())
 	if err != nil {

api/konfig/builtinpluginconsts/namereference.go

@@ -3,6 +3,9 @@
 
 package builtinpluginconsts
 
+// TODO: rename 'fieldSpecs' to 'referrers' for clarity.
+// This will, however, break anyone using a custom config.
+
 const (
 	nameReferenceFieldSpecs = `
 nameReference:

api/krusty/namespaces_test.go

@@ -50,7 +50,7 @@ resources:
 - secrets.yaml
 - role.yaml
 `)
-	// This validates Fix #1444. This should not be an error anymore -
+	// This validates fix for Issue #1044. This should not be an error anymore -
 	// the secrets have the same name but are in different namespaces.
 	// The ClusterRole (by def) is not in a namespace,
 	// an in this case applies to *any* Secret resource
@@ -91,13 +91,71 @@ rules:
 `)
 }
 
+func TestNameReferenceDeployment(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	th.WriteK("base", `
+resources:
+- cm.yaml
+- dep.yaml
+`)
+	th.WriteF("base/cm.yaml", `
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: myMap
+`)
+	th.WriteF("base/dep.yaml", `
+apiVersion: v1
+group: apps
+kind: Deployment
+metadata:
+  name: myDep
+spec:
+  template:
+    spec:
+      containers:
+      - env:
+        - name: CM_FOO
+          valueFrom:
+            configMapKeyRef:
+              key: foo
+              name: myMap
+`)
+	th.WriteK("ov1", `
+resources:
+- ../base
+namePrefix: pp-
+`)
+	th.WriteK("ov2", `
+resources:
+- ../base
+nameSuffix: -ss
+`)
+	th.WriteK("ov3", `
+resources:
+- ../base
+namespace: fred
+nameSuffix: -xx
+`)
+	th.WriteK(".", `
+resources:
+- ../ov1
+- ../ov2
+- ../ov3
+`)
+	err := th.RunWithErr(".", th.MakeDefaultOptions())
+	if err == nil {
+		t.Fatal("TODO(3489): this should work")
+	}
+}
+
 // TestNameAndNsTransformation validates that NamespaceTransformer,
 // PrefixSuffixTransformer and namereference transformers are
 // able to deal with simultaneous change of namespace and name.
 func TestNameAndNsTransformation(t *testing.T) {
 	th := kusttest_test.MakeHarness(t)
 
-	th.WriteK("/nameandns", `
+	th.WriteK(".", `
 namePrefix: p1-
 nameSuffix: -s1
 namespace: newnamespace
@@ -105,7 +163,7 @@ resources:
 - resources.yaml
 `)
 
-	th.WriteF("/nameandns/resources.yaml", `
+	th.WriteF("resources.yaml", `
 apiVersion: v1
 kind: ConfigMap
 metadata:
@@ -204,7 +262,7 @@ kind: PersistentVolume
 metadata:
   name: pv1
 `)
-	m := th.Run("/nameandns", th.MakeDefaultOptions())
+	m := th.Run(".", th.MakeDefaultOptions())
 	th.AssertActualEqualsExpected(m, `
 apiVersion: v1
 kind: ConfigMap

api/resmap/reswrangler.go

@@ -377,52 +377,59 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap {
 
 // SubsetThatCouldBeReferencedByResource implements ResMap.
 func (m *resWrangler) SubsetThatCouldBeReferencedByResource(
-	inputRes *resource.Resource) ResMap {
+	referrer *resource.Resource) ResMap {
+	referrerId := referrer.CurId()
+	if !referrerId.IsNamespaceableKind() {
+		// A cluster scoped resource can refer to anything.
+		return m
+	}
 	result := newOne()
-	inputId := inputRes.CurId()
+	roleBindingNamespaces := getNamespacesForRoleBinding(referrer)
-	isInputIdNamespaceable := inputId.IsNamespaceableKind()
+	for _, possibleTarget := range m.Resources() {
-	subjectNamespaces := getNamespacesForRoleBinding(inputRes)
+		id := possibleTarget.CurId()
-	for _, r := range m.Resources() {
+		if !id.IsNamespaceableKind() {
-		// Need to match more accuratly both at the time of selection and transformation.
+			// A cluster-scoped resource can be referred to by anything.
-		// OutmostPrefixSuffixEquals is not accurate enough since it is only using
+			result.append(possibleTarget)
-		// the outer most suffix and the last prefix. Use PrefixedSuffixesEquals instead.
+			continue
-		resId := r.CurId()
+		}
-		if !isInputIdNamespaceable || !resId.IsNamespaceableKind() || resId.IsNsEquals(inputId) ||
+		if id.IsNsEquals(referrerId) {
-			isRoleBindingNamespace(&subjectNamespaces, r.GetNamespace()) {
+			// The two objects are in the same namespace.
-			result.append(r)
+			result.append(possibleTarget)
+			continue
+		}
+		// The two objects are namespaced (not cluster-scoped), AND
+		// are in different namespaces.
+		// There's still a chance they can refer to each other.
+		ns := possibleTarget.GetNamespace()
+		if roleBindingNamespaces[ns] {
+			result.append(possibleTarget)
 		}
 	}
 	return result
 }
 
-// isRoleBindingNamespace returns true is the namespace `ns` is in role binding
+// getNamespacesForRoleBinding returns referenced ServiceAccount namespaces
-// namespaces `m`
+// if the resource is a RoleBinding
-func isRoleBindingNamespace(m *map[string]bool, ns string) bool {
+func getNamespacesForRoleBinding(r *resource.Resource) map[string]bool {
-	return (*m)[ns]
+	result := make(map[string]bool)
-}
+	if r.GetKind() != "RoleBinding" {
-
+		return result
-// getNamespacesForRoleBinding returns referenced ServiceAccount namespaces if the inputRes is
-// a RoleBinding
-func getNamespacesForRoleBinding(inputRes *resource.Resource) map[string]bool {
-	res := make(map[string]bool)
-	if inputRes.GetKind() != "RoleBinding" {
-		return res
 	}
-	subjects, err := inputRes.GetSlice("subjects")
+	subjects, err := r.GetSlice("subjects")
 	if err != nil || subjects == nil {
-		return res
+		return result
 	}
-
 	for _, s := range subjects {
 		subject := s.(map[string]interface{})
-		if subject["namespace"] == nil || subject["kind"] == nil ||
+		if ns, ok1 := subject["namespace"]; ok1 {
-			subject["kind"].(string) != "ServiceAccount" {
+			if kind, ok2 := subject["kind"]; ok2 {
-			continue
+				if kind.(string) == "ServiceAccount" {
+					result[ns.(string)] = true
+				}
+			}
 		}
-		res[subject["namespace"].(string)] = true
 	}
-
+	return result
-	return res
 }
 
 func (m *resWrangler) append(res *resource.Resource) {

api/resource/resource.go

@@ -195,21 +195,27 @@ func (r *Resource) ErrIfNotEquals(o *Resource) error {
 		return err
 	}
 	if !r.ReferencesEqual(o) {
-		return fmt.Errorf("references unequal")
+		return fmt.Errorf(
+			`unequal references - self:
+%sreferenced by: %s
+--- other:
+%sreferenced by: %s
+`, meYaml, r.GetRefBy(), otherYaml, o.GetRefBy())
 	}
 	if string(meYaml) != string(otherYaml) {
-		return fmt.Errorf("---  self:\n"+
+		return fmt.Errorf(`---  self:
-			"%s\n"+
+%s
-			"--- other:\n"+
+--- other:
-			"%s\n", meYaml, otherYaml)
+%s
+`, meYaml, otherYaml)
 	}
 	return nil
 }
 
-func (r *Resource) ReferencesEqual(o *Resource) bool {
+func (r *Resource) ReferencesEqual(other *Resource) bool {
 	setSelf := make(map[resid.ResId]bool)
 	setOther := make(map[resid.ResId]bool)
-	for _, ref := range o.refBy {
+	for _, ref := range other.refBy {
 		setOther[ref] = true
 	}
 	for _, ref := range r.refBy {