diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go index e6172feb0..f7e683b5a 100644 --- a/api/filters/nameref/nameref.go +++ b/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, - // a name that this filter seeks to update. + // Referrer refers to another resource X by X's name. + // 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 - // name requiring an update. - NameFieldToUpdate types.FieldSpec `json:"nameFieldToUpdate,omitempty" yaml:"nameFieldToUpdate,omitempty"` + // NameFieldToUpdate is the field in the Referrer + // that holds the name requiring an update. + // 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 +} diff --git a/api/internal/accumulator/loadconfigfromcrds.go b/api/internal/accumulator/loadconfigfromcrds.go index 2eac72325..c00ed478e 100644 --- a/api/internal/accumulator/loadconfigfromcrds.go +++ b/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 { diff --git a/api/internal/accumulator/loadconfigfromcrds_test.go b/api/internal/accumulator/loadconfigfromcrds_test.go index e98b0b900..cae18898d 100644 --- a/api/internal/accumulator/loadconfigfromcrds_test.go +++ b/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"}, - Path: "spec/secretRef/name", + Gvk: resid.Gvk{Kind: "MyKind"}, + Path: "spec/secretRef/name", }, }, }, { Gvk: resid.Gvk{Kind: "Bee", Version: "v1beta1"}, - FieldSpecs: []types.FieldSpec{ + Referrers: []types.FieldSpec{ { - CreateIfNotPresent: false, - Gvk: resid.Gvk{Kind: "MyKind"}, - Path: "spec/beeRef/name", + Gvk: resid.Gvk{Kind: "MyKind"}, + Path: "spec/beeRef/name", }, }, }, diff --git a/api/internal/accumulator/namereferencetransformer.go b/api/internal/accumulator/namereferencetransformer.go index d929e3560..324511e88 100644 --- a/api/internal/accumulator/namereferencetransformer.go +++ b/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 -// (e.g. prefix added), and the reference in the HPA -// has to be fixed. +// an HPA scales. In this case: // -// In the outer loop over the ResMap below, say we -// encounter a specific HPA. Then, in scanning the set +// - the HPA instance is the Referrer, +// - 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 -// modify the value in its 'spec/scaleTargetRef/name' -// field, by searching for the thing it refers to, -// and getting its new name. +// Deployment. // -// As a filter, and search optimization, we compute a -// subset of all resources that the HPA could refer to, -// by excluding objects from other namespaces, and -// excluding objects that don't have the same prefix- -// suffix mods as the HPA. -// -// We look in this subset for all Deployment objects -// with a resId that has a Name matching the field value -// present in the HPA. If no match do nothing; if more -// than one match, it's an error. -// -// We overwrite the HPA name field with the value found -// in the Deployment's name field (the name in the raw -// object - the modified name - not the unmodified name -// in the Deployment's resId). -// -// This process assumes that the name stored in a ResId -// (the ResMap key) isn't modified by name transformers. -// Name transformers should only modify the name in the -// body of the resource object (the value in the ResMap). -// -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 +// This means that a filter will need to hunt for the right Deployment, +// obtain it's new name, and write that name into the HPA's +// 'spec/scaleTargetRef/name' field. Return a filter that can do that. +func (t *nameReferenceTransformer) determineFilters( + resources []*resource.Resource) (fMap filterMap) { + fMap = make(filterMap) + for _, backReference := range t.backRefs { + for _, referrerSpec := range backReference.Referrers { + for _, res := range resources { + if res.OrgId().IsSelected(&referrerSpec.Gvk) { + // If this is true, the res might be a referrer, and if + // so, the name reference it holds might need an update. + if resHasField(res, referrerSpec.Path) { + // Optimization - the referrer has the field + // that might need updating. + fMap[res] = append(fMap[res], nameref.Filter{ + // Name field to write in the Referrer. + NameFieldToUpdate: referrerSpec, + // Specification of object class to read from. + // Always read from metadata/name field. + ReferralTarget: backReference.Gvk, + }) } } } } } - 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 } diff --git a/api/internal/accumulator/namereferencetransformer_test.go b/api/internal/accumulator/namereferencetransformer_test.go index c9fedeb4b..1097fdb45 100644 --- a/api/internal/accumulator/namereferencetransformer_test.go +++ b/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) } } diff --git a/api/internal/plugins/builtinconfig/namebackreferences.go b/api/internal/plugins/builtinconfig/namebackreferences.go index 47511ebc6..e458786c4 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences.go +++ b/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 -// of FieldSpec instances that could refer to it. +// NameBackReferences is an association between a gvk.GVK (a ReferralTarget) +// 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 -// the name of a ConfigMap instance from 'alice' to 'bob', one -// must visit all objects that could refer to the ConfigMap, see if -// they mention 'alice', and if so, change the reference to 'bob'. +// contains a Pod; Deployment, Job, StatefulSet, etc. +// The ConfigMap is the ReferralTarget, the others are Referrers. +// +// 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"` - FieldSpecs types.FsSlice `json:"FieldSpecs,omitempty" yaml:"FieldSpecs,omitempty"` + resid.Gvk `json:",inline,omitempty" yaml:",inline,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 } diff --git a/api/internal/plugins/builtinconfig/namebackreferences_test.go b/api/internal/plugins/builtinconfig/namebackreferences_test.go index 58968f729..7333f173b 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences_test.go +++ b/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) diff --git a/api/internal/plugins/builtinconfig/transformerconfig_test.go b/api/internal/plugins/builtinconfig/transformerconfig_test.go index 905cb9b1b..cdd882960 100644 --- a/api/internal/plugins/builtinconfig/transformerconfig_test.go +++ b/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", diff --git a/api/internal/wrappy/wnode_test.go b/api/internal/wrappy/wnode_test.go index 7330a3d35..692fefa7e 100644 --- a/api/internal/wrappy/wnode_test.go +++ b/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 { diff --git a/api/konfig/builtinpluginconsts/namereference.go b/api/konfig/builtinpluginconsts/namereference.go index ca9292698..13e523c4a 100644 --- a/api/konfig/builtinpluginconsts/namereference.go +++ b/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: diff --git a/api/krusty/namespaces_test.go b/api/krusty/namespaces_test.go index 8e5c310cb..0a3966e65 100644 --- a/api/krusty/namespaces_test.go +++ b/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 diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index e9e8b165d..79ee617c9 100644 --- a/api/resmap/reswrangler.go +++ b/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() - isInputIdNamespaceable := inputId.IsNamespaceableKind() - subjectNamespaces := getNamespacesForRoleBinding(inputRes) - for _, r := range m.Resources() { - // Need to match more accuratly both at the time of selection and transformation. - // OutmostPrefixSuffixEquals is not accurate enough since it is only using - // the outer most suffix and the last prefix. Use PrefixedSuffixesEquals instead. - resId := r.CurId() - if !isInputIdNamespaceable || !resId.IsNamespaceableKind() || resId.IsNsEquals(inputId) || - isRoleBindingNamespace(&subjectNamespaces, r.GetNamespace()) { - result.append(r) + roleBindingNamespaces := getNamespacesForRoleBinding(referrer) + for _, possibleTarget := range m.Resources() { + id := possibleTarget.CurId() + if !id.IsNamespaceableKind() { + // A cluster-scoped resource can be referred to by anything. + result.append(possibleTarget) + continue + } + if id.IsNsEquals(referrerId) { + // The two objects are in the same namespace. + 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 -// namespaces `m` -func isRoleBindingNamespace(m *map[string]bool, ns string) bool { - return (*m)[ns] -} - -// 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 +// getNamespacesForRoleBinding returns referenced ServiceAccount namespaces +// if the resource is a RoleBinding +func getNamespacesForRoleBinding(r *resource.Resource) map[string]bool { + result := make(map[string]bool) + if r.GetKind() != "RoleBinding" { + return result } - 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 || - subject["kind"].(string) != "ServiceAccount" { - continue + if ns, ok1 := subject["namespace"]; ok1 { + if kind, ok2 := subject["kind"]; ok2 { + if kind.(string) == "ServiceAccount" { + result[ns.(string)] = true + } + } } - res[subject["namespace"].(string)] = true } - - return res + return result } func (m *resWrangler) append(res *resource.Resource) { diff --git a/api/resource/resource.go b/api/resource/resource.go index a2703300b..8bed9953c 100644 --- a/api/resource/resource.go +++ b/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"+ - "%s\n"+ - "--- other:\n"+ - "%s\n", meYaml, otherYaml) + return fmt.Errorf(`--- self: +%s +--- other: +%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 {