Tests and comments for MakeDefaultConfig perf work

Document updated code in MakeDefaultConfig.
Add unit tests to ensure DeepCopy works.
Add hints to other code to ensure DeepCopy is kept up to date.

api/internal/plugins/builtinconfig/namebackreferences.go

@@ -47,6 +47,8 @@ type NameBackReferences struct {
 	// 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"`
+
+	// Note: If any new pointer based members are added, DeepCopy needs to be updated
 }
 
 func (n NameBackReferences) String() string {
@@ -66,9 +68,14 @@ func (s nbrSlice) Less(i, j int) bool {
 	return s[i].Gvk.IsLessThan(s[j].Gvk)
 }
 
+// DeepCopy returns a new copy of nbrSlice
 func (s nbrSlice) DeepCopy() nbrSlice {
 	ret := make(nbrSlice, len(s))
 	copy(ret, s)
+	for i, slice := range ret {
+		ret[i].Referrers = slice.Referrers.DeepCopy()
+	}
+
 	return ret
 }
 

api/internal/plugins/builtinconfig/namebackreferences_test.go

@@ -95,3 +95,29 @@ func TestMergeAll(t *testing.T) {
 		t.Fatalf("expected\n %v\n but got\n %v\n", expected, actual)
 	}
 }
+
+func TestNbrSlice_DeepCopy(t *testing.T) {
+	original := make(nbrSlice, 2, 4)
+	original[0] = NameBackReferences{Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}}
+	original[1] = NameBackReferences{Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}}
+
+	copied := original.DeepCopy()
+
+	original, _ = original.mergeOne(NameBackReferences{Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}})
+
+	// perform mutations which should not affect original
+	copied.Swap(0, 1)
+	copied[0].Referrers[0].Path = "very b" // ensure Referrers are not shared
+	_, _ = copied.mergeOne(NameBackReferences{Gvk: resid.FromKind("D"), Referrers: types.FsSlice{{Path: "d"}}})
+
+	// if DeepCopy does not work, original would be {very b,a,d} instead of {a,b,c}
+	expected := nbrSlice{
+		{Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}},
+		{Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}},
+		{Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}},
+	}
+
+	if !reflect.DeepEqual(original, expected) {
+		t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected)
+	}
+}

api/internal/plugins/builtinconfig/transformerconfig.go

@@ -16,6 +16,7 @@ import (
 
 // TransformerConfig holds the data needed to perform transformations.
 type TransformerConfig struct {
+	// if any fields are added, update the DeepCopy implementation
 	NamePrefix        types.FsSlice `json:"namePrefix,omitempty" yaml:"namePrefix,omitempty"`
 	NameSuffix        types.FsSlice `json:"nameSuffix,omitempty" yaml:"nameSuffix,omitempty"`
 	NameSpace         types.FsSlice `json:"namespace,omitempty" yaml:"namespace,omitempty"`
@@ -33,6 +34,7 @@ func MakeEmptyConfig() *TransformerConfig {
 	return &TransformerConfig{}
 }
 
+// DeepCopy returns a new copy of TransformerConfig
 func (t *TransformerConfig) DeepCopy() *TransformerConfig {
 	return &TransformerConfig{
 		NamePrefix:        t.NamePrefix.DeepCopy(),
@@ -48,13 +50,16 @@ func (t *TransformerConfig) DeepCopy() *TransformerConfig {
 	}
 }
 
+// the default transformer config is initialized by MakeDefaultConfig,
+// and must only be accessed via that function.
 var (
-	initDefaultConfig sync.Once
-	defaultConfig     *TransformerConfig
+	initDefaultConfig sync.Once          //nolint:gochecknoglobals
+	defaultConfig     *TransformerConfig //nolint:gochecknoglobals
 )
 
 // MakeDefaultConfig returns a default TransformerConfig.
 func MakeDefaultConfig() *TransformerConfig {
+	// parsing is expensive when having a large tree with many kustomization modules, so only do it once
 	initDefaultConfig.Do(func() {
 		var err error
 		defaultConfig, err = makeTransformerConfigFromBytes(
@@ -64,6 +69,7 @@ func MakeDefaultConfig() *TransformerConfig {
 		}
 	})
 
+	// return a copy to avoid any mutations to protect the reference copy
 	return defaultConfig.DeepCopy()
 }
 

api/internal/plugins/builtinconfig/transformerconfig_test.go

@@ -173,3 +173,23 @@ func TestMerge(t *testing.T) {
 		t.Fatalf("expected: %v\n but got: %v\n", cfga, actual)
 	}
 }
+
+func TestMakeDefaultConfig_mutation(t *testing.T) {
+	a := MakeDefaultConfig()
+
+	// mutate
+	a.NameReference[0].Kind = "mutated"
+	a.NameReference = a.NameReference[:1]
+
+	clean := MakeDefaultConfig()
+	if clean.NameReference[0].Kind == "mutated" {
+		t.Errorf("MakeDefaultConfig() did not return a clean copy: %+v", clean.NameReference)
+	}
+
+}
+
+func BenchmarkMakeDefaultConfig(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		_ = MakeDefaultConfig()
+	}
+}

api/types/fieldspec.go

@@ -30,6 +30,8 @@ type FieldSpec struct {
 	resid.Gvk          `json:",inline,omitempty" yaml:",inline,omitempty"`
 	Path               string `json:"path,omitempty" yaml:"path,omitempty"`
 	CreateIfNotPresent bool   `json:"create,omitempty" yaml:"create,omitempty"`
+
+	// Note: If any new pointer based members are added, FsSlice.DeepCopy needs to be updated
 }
 
 func (fs FieldSpec) String() string {
@@ -50,6 +52,7 @@ func (s FsSlice) Less(i, j int) bool {
 	return s[i].Gvk.IsLessThan(s[j].Gvk)
 }
 
+// DeepCopy returns a new copy of FsSlice
 func (s FsSlice) DeepCopy() FsSlice {
 	ret := make(FsSlice, len(s))
 	copy(ret, s)

api/types/fieldspec_test.go

@@ -142,3 +142,27 @@ func TestFsSlice_MergeAll(t *testing.T) {
 		}
 	}
 }
+
+func TestFsSlice_DeepCopy(t *testing.T) {
+	original := make(FsSlice, 2, 4)
+	original[0] = FieldSpec{Path: "a"}
+	original[1] = FieldSpec{Path: "b"}
+
+	copied := original.DeepCopy()
+
+	original, _ = original.MergeOne(FieldSpec{Path: "c"})
+
+	// perform mutations which should not affect original
+	copied.Swap(0, 1)
+	_, _ = copied.MergeOne(FieldSpec{Path: "d"})
+
+	// if DeepCopy does not work, original would be {b,a,d} instead of {a,b,c}
+	expected := FsSlice{
+		{Path: "a"},
+		{Path: "b"},
+		{Path: "c"},
+	}
+	if !reflect.DeepEqual(original, expected) {
+		t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected)
+	}
+}