Merge pull request #635 from monopole/nameTransformerDocs

Improve name transformer docs.

pkg/target/kusttarget.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package target implements state for the set of all resources being customized.
+// Package target implements state for the set of all resources to customize.
 package target
 
 import (
@@ -97,9 +97,10 @@ func unmarshal(y []byte, o interface{}) error {
 	return dec.Decode(o)
 }
 
-// makeTransformerConfig returns a complete TransformerConfig object from either files
-// or the default configs
-func makeTransformerConfig(ldr ifc.Loader, paths []string) (*config.TransformerConfig, error) {
+// makeTransformerConfig returns a complete TransformerConfig object
+// from either files or the default configs
+func makeTransformerConfig(
+	ldr ifc.Loader, paths []string) (*config.TransformerConfig, error) {
 	if paths == nil || len(paths) == 0 {
 		return config.NewFactory(nil).DefaultConfig(), nil
 	}
@@ -113,13 +114,7 @@ func (kt *KustTarget) MakeCustomizedResMap() (resmap.ResMap, error) {
 	if err != nil {
 		return nil, err
 	}
-	return kt.resolveRefsToGeneratedResources(m)
-}
-
-// resolveRefsToGeneratedResources fixes all name references.
-func (kt *KustTarget) resolveRefsToGeneratedResources(m resmap.ResMap) (resmap.ResMap, error) {
-	if kt.kustomization.GeneratorOptions == nil ||
-		!kt.kustomization.GeneratorOptions.DisableNameSuffixHash {
+	if kt.shouldAddHashSuffixesToGeneratedResources() {
 		// This effects only generated resources.
 		// It changes only the Name field in the
 		// resource held in the ResMap's value, not
@@ -129,25 +124,28 @@ func (kt *KustTarget) resolveRefsToGeneratedResources(m resmap.ResMap) (resmap.R
 			return nil, err
 		}
 	}
-	var r []transformers.Transformer
-	t, err := transformers.NewNameReferenceTransformer(kt.tConfig.NameReference)
-	if err != nil {
-		return nil, err
-	}
-	r = append(r, t)
 
-	refVars, err := kt.resolveRefVars(m)
+	// Given that names have changed (prefixs/suffixes added),
+	// fix all the back references to those names.
+	err = transformers.NewNameReferenceTransformer(
+		kt.tConfig.NameReference).Transform(m)
 	if err != nil {
 		return nil, err
 	}
-	t = transformers.NewRefVarTransformer(refVars, kt.tConfig.VarReference)
-	r = append(r, t)
 
-	err = transformers.NewMultiTransformer(r).Transform(m)
+	// With all the back references fixed, it's OK to resolve Vars.
+	refVars, err := kt.resolveVars(m)
 	if err != nil {
 		return nil, err
 	}
-	return m, nil
+	err = transformers.NewRefVarTransformer(
+		refVars, kt.tConfig.VarReference).Transform(m)
+	return m, err
+}
+
+func (kt *KustTarget) shouldAddHashSuffixesToGeneratedResources() bool {
+	return kt.kustomization.GeneratorOptions == nil ||
+		!kt.kustomization.GeneratorOptions.DisableNameSuffixHash
 }
 
 // loadCustomizedResMap loads and customizes resources to build a ResMap.
@@ -268,7 +266,7 @@ func (kt *KustTarget) loadCustomizedBases() (resmap.ResMap, *interror.Kustomizat
 	return result, errs
 }
 
-func (kt *KustTarget) loadBasesAsFlatList() ([]*KustTarget, error) {
+func (kt *KustTarget) loadBasesAsKustTargets() ([]*KustTarget, error) {
 	var result []*KustTarget
 	errs := &interror.KustomizationErrors{}
 	for _, path := range kt.kustomization.Bases {
@@ -291,8 +289,10 @@ func (kt *KustTarget) loadBasesAsFlatList() ([]*KustTarget, error) {
 	return result, nil
 }
 
-// newTransformer makes a Transformer that does everything except resolve generated names.
-func (kt *KustTarget) newTransformer(patches []*resource.Resource) (transformers.Transformer, error) {
+// newTransformer makes a Transformer that does a collection
+// of object transformations.
+func (kt *KustTarget) newTransformer(
+	patches []*resource.Resource) (transformers.Transformer, error) {
 	var r []transformers.Transformer
 	t, err := kt.tFactory.MakePatchTransformer(patches, kt.rFactory.RF())
 	if err != nil {
@@ -325,7 +325,10 @@ func (kt *KustTarget) newTransformer(patches []*resource.Resource) (transformers
 	return transformers.NewMultiTransformer(r), nil
 }
 
-func (kt *KustTarget) resolveRefVars(m resmap.ResMap) (map[string]string, error) {
+// resolveVars returns a map of Var names to their final values.
+// The values are strings intended for substitution wherever
+// the $(var.Name) occurs.
+func (kt *KustTarget) resolveVars(m resmap.ResMap) (map[string]string, error) {
 	result := map[string]string{}
 	vars, err := kt.getAllVars()
 	if err != nil {
@@ -336,11 +339,11 @@ func (kt *KustTarget) resolveRefVars(m resmap.ResMap) (map[string]string, error)
 		if r, found := m.DemandOneMatchForId(id); found {
 			s, err := r.GetFieldValue(v.FieldRef.FieldPath)
 			if err != nil {
-				return nil, fmt.Errorf("failed to resolve referred var: %+v", v)
+				return nil, fmt.Errorf("field path err for var: %+v", v)
 			}
 			result[v.Name] = s
 		} else {
-			log.Printf("couldn't resolve v: %v", v)
+			log.Printf("couldn't resolve var: %v", v)
 		}
 	}
 	return result, nil
@@ -351,7 +354,7 @@ func (kt *KustTarget) getAllVars() ([]types.Var, error) {
 	var result []types.Var
 	errs := &interror.KustomizationErrors{}
 
-	bases, err := kt.loadBasesAsFlatList()
+	bases, err := kt.loadBasesAsKustTargets()
 	if err != nil {
 		return nil, err
 	}
@@ -377,7 +380,9 @@ func (kt *KustTarget) getAllVars() ([]types.Var, error) {
 }
 
 func loadKustFile(ldr ifc.Loader) ([]byte, error) {
-	for _, kf := range []string{constants.KustomizationFileName, constants.SecondaryKustomizationFileName} {
+	for _, kf := range []string{
+		constants.KustomizationFileName,
+		constants.SecondaryKustomizationFileName} {
 		content, err := ldr.Load(kf)
 		if err == nil {
 			return content, nil

pkg/transformers/namereference.go

@@ -17,8 +17,8 @@ limitations under the License.
 package transformers
 
 import (
-	"errors"
 	"fmt"
+	"log"
 
 	"sigs.k8s.io/kustomize/pkg/gvk"
 	"sigs.k8s.io/kustomize/pkg/resmap"
@@ -33,18 +33,31 @@ var _ Transformer = &nameReferenceTransformer{}
 
 // NewNameReferenceTransformer constructs a nameReferenceTransformer
 // with a given slice of NameBackReferences.
-func NewNameReferenceTransformer(
-	br []config.NameBackReferences) (Transformer, error) {
+func NewNameReferenceTransformer(br []config.NameBackReferences) Transformer {
 	if br == nil {
-		return nil, errors.New("backrefs not expected to be nil")
+		log.Fatal("backrefs not expected to be nil")
 	}
-	return &nameReferenceTransformer{backRefs: br}, nil
+	return &nameReferenceTransformer{backRefs: br}
 }
 
-// Transform does the field update according to fieldSpecs.
-// The old name is in the key in the map and the new name is in the object
-// associated with the key. e.g. if <k, v> is one of the key-value pair in the map,
-// then the old name is k.Name and the new name is v.GetName()
+// 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.
+//
+// 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).
+//
+// 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
+// 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 FilterBy,

pkg/transformers/namereference_test.go

@@ -417,11 +417,8 @@ func TestNameReferenceHappyRun(t *testing.T) {
 				},
 			},
 		})
-	nrt, err := NewNameReferenceTransformer(defaultTransformerConfig.NameReference)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	err = nrt.Transform(m)
+	nrt := NewNameReferenceTransformer(defaultTransformerConfig.NameReference)
+	err := nrt.Transform(m)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -483,19 +480,16 @@ func TestNameReferenceUnhappyRun(t *testing.T) {
 			expectedErr: "is expected to be either a string or a []interface{}"},
 	}
 
-	nrt, err := NewNameReferenceTransformer(defaultTransformerConfig.NameReference)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
+	nrt := NewNameReferenceTransformer(defaultTransformerConfig.NameReference)
 	for _, test := range tests {
-		err = nrt.Transform(test.resMap)
+		err := nrt.Transform(test.resMap)
 		if err == nil {
 			t.Fatalf("expected error to happen")
 		}
 
 		if !strings.Contains(err.Error(), test.expectedErr) {
-			t.Fatalf("Incorrect error.\nExpected: %s, but got %v", test.expectedErr, err)
+			t.Fatalf("Incorrect error.\nExpected: %s, but got %v",
+				test.expectedErr, err)
 		}
 	}
 }