diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index c173b5b36..b83ecebd7 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -134,15 +134,13 @@ type ResMap interface { // Clear removes all resources and Ids. Clear() - // ResourcesThatCouldReference returns a new ResMap with - // resources that _might_ reference the resource represented - // by the argument Id, excluding resources that should - // _never_ reference the Id. E.g., if the Id - // refers to a ConfigMap, the returned set may include a - // Deployment from the same namespace and exclude Deployments - // from other namespaces. Cluster wide objects are - // never excluded. - ResourcesThatCouldReference(resid.ResId) ResMap + // SubsetThatCouldBeReferencedBy returns a ResMap subset + // of self with resources that could be referenced by the + // resource represented by the argument Id. + // This is a filter; it excludes things that cannot be + // referenced by the Id's resource, e.g. objects in other + // namespaces. Cluster wide objects are never excluded. + SubsetThatCouldBeReferencedBy(resid.ResId) ResMap // DeepCopy copies the ResMap and underlying resources. DeepCopy() ResMap @@ -456,8 +454,8 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap { return result } -// ResourcesThatCouldReference implements ResMap. -func (m *resWrangler) ResourcesThatCouldReference(inputId resid.ResId) ResMap { +// SubsetThatCouldBeReferencedBy implements ResMap. +func (m *resWrangler) SubsetThatCouldBeReferencedBy(inputId resid.ResId) ResMap { if inputId.Gvk().IsClusterKind() { return m } diff --git a/pkg/resmap/resmap_test.go b/pkg/resmap/resmap_test.go index 8e6097d6c..2b326400f 100644 --- a/pkg/resmap/resmap_test.go +++ b/pkg/resmap/resmap_test.go @@ -381,7 +381,7 @@ func TestFilterBy(t *testing.T) { for name, test := range tests { test := test t.Run(name, func(t *testing.T) { - got := test.resMap.ResourcesThatCouldReference(test.filter) + got := test.resMap.SubsetThatCouldBeReferencedBy(test.filter) err := test.expected.ErrorIfNotEqualSets(got) if err != nil { t.Fatalf("Expected %v but got back %v", test.expected, got) diff --git a/pkg/transformers/namereference.go b/pkg/transformers/namereference.go index 318d506ef..219f76fdc 100644 --- a/pkg/transformers/namereference.go +++ b/pkg/transformers/namereference.go @@ -41,38 +41,68 @@ func NewNameReferenceTransformer(br []config.NameBackReferences) Transformer { return &nameReferenceTransformer{backRefs: br} } -// Transform updates name references in resource A that refer to resource B, -// given that B's name may have changed. +// Transform updates name references in resource A that +// refer to resource B, given that B's name may have +// changed. // -// For example, a HorizontalPodAutoscaler (HPA) necessarily refers to a -// Deployment (the thing that the HPA scales). The Deployment name might change -// (e.g. prefix added), and the reference in the HPA has to be fixed. +// For example, a HorizontalPodAutoscaler (HPA) +// necessarily refers to a Deployment, the thing that +// the HPA scales. The Deployment name might change +// (e.g. prefix added), and the reference in the HPA +// has to be fixed. // -// In the outer loop below, we encounter an HPA. In scanning backrefs, we -// find that HPA refers to a Deployment. So we find all resources in the same -// namespace as the HPA (and with the same prefix and suffix), and look through -// them to find all the Deployments with a resId that has a Name matching the -// field in HPA. For each match, 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 resId). +// In the outer loop over the ResMap below, say we +// encounter a specific HPA. Then, in scanning backrefs, +// we encounter an entry like // -// This 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 +// - kind: Deployment +// fieldSpecs: +// - path: spec/scaleTargetRef/name +// kind: HorizontalPodAutoscaler +// +// saying 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. +// +// 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 (o *nameReferenceTransformer) Transform(m resmap.ResMap) error { - // TODO: Too much looping. - // Even more hidden loops in ResourcesThatCouldReference, - // updateNameReference and FindByGVKN. - for id, r := range m.AsMap() { - for _, backRef := range o.backRefs { - for _, fSpec := range backRef.FieldSpecs { - if id.Gvk().IsSelected(&fSpec.Gvk) { + // TODO: Too much looping, here and in transitive calls. + for referrer, res := range m.AsMap() { + var candidates resmap.ResMap + for _, target := range o.backRefs { + for _, fSpec := range target.FieldSpecs { + if referrer.Gvk().IsSelected(&fSpec.Gvk) { + if candidates == nil { + candidates = m.SubsetThatCouldBeReferencedBy(referrer) + } err := mutateField( - r.Map(), + res.Map(), fSpec.PathSlice(), fSpec.CreateIfNotPresent, - o.updateNameReference( - id, backRef.Gvk, m.ResourcesThatCouldReference(id))) + o.getNewName( + referrer, target.Gvk, candidates)) if err != nil { return err } @@ -83,24 +113,28 @@ func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error { return nil } -func (o *nameReferenceTransformer) updateNameReference( - rid resid.ResId, backRef gvk.Gvk, m resmap.ResMap) func(in interface{}) (interface{}, error) { +func (o *nameReferenceTransformer) getNewName( + referrer resid.ResId, + target gvk.Gvk, + referralCandidates resmap.ResMap) func(in interface{}) (interface{}, error) { return func(in interface{}) (interface{}, error) { switch in.(type) { case string: - s, _ := in.(string) - for id, res := range m.AsMap() { - if id.Gvk().IsSelected(&backRef) && id.Name() == s { - matchedIds := m.GetMatchingIds(id.GvknEquals) + oldName, _ := in.(string) + for id, res := range referralCandidates.AsMap() { + if id.Gvk().IsSelected(&target) && id.Name() == oldName { + matchedIds := referralCandidates.GetMatchingIds(id.GvknEquals) // If there's more than one match, there's no way // to know which one to pick, so emit error. if len(matchedIds) > 1 { return nil, fmt.Errorf( "Multiple matches for name %s:\n %v", id, matchedIds) } + // In the resource, note that it is referenced + // by the referrer. + res.AppendRefBy(referrer) // Return transformed name of the object, // complete with prefixes, hashes, etc. - res.AppendRefBy(rid) return res.GetName(), nil } } @@ -111,14 +145,15 @@ func (o *nameReferenceTransformer) updateNameReference( for _, item := range l { name, ok := item.(string) if !ok { - return nil, fmt.Errorf("%#v is expected to be %T", item, name) + return nil, fmt.Errorf( + "%#v is expected to be %T", item, name) } names = append(names, name) } - for id, res := range m.AsMap() { + for id, res := range referralCandidates.AsMap() { indexes := indexOf(id.Name(), names) - if id.Gvk().IsSelected(&backRef) && len(indexes) > 0 { - matchedIds := m.GetMatchingIds(id.GvknEquals) + if id.Gvk().IsSelected(&target) && len(indexes) > 0 { + matchedIds := referralCandidates.GetMatchingIds(id.GvknEquals) if len(matchedIds) > 1 { return nil, fmt.Errorf( "Multiple matches for name %s:\n %v", id, matchedIds) @@ -126,13 +161,14 @@ func (o *nameReferenceTransformer) updateNameReference( for _, index := range indexes { l[index] = res.GetName() } - res.AppendRefBy(rid) + res.AppendRefBy(referrer) return l, nil } } return in, nil default: - return nil, fmt.Errorf("%#v is expected to be either a string or a []interface{}", in) + return nil, fmt.Errorf( + "%#v is expected to be either a string or a []interface{}", in) } } }