From 1dbef6e109ac8e14b58c2fa543f61b9e0fc9d1af Mon Sep 17 00:00:00 2001 From: Carl Henrik Lunde Date: Sun, 12 Mar 2023 15:26:38 +0100 Subject: [PATCH] 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. --- .../builtinconfig/namebackreferences.go | 7 +++++ .../builtinconfig/namebackreferences_test.go | 26 +++++++++++++++++++ .../builtinconfig/transformerconfig.go | 10 +++++-- .../builtinconfig/transformerconfig_test.go | 20 ++++++++++++++ api/types/fieldspec.go | 3 +++ api/types/fieldspec_test.go | 24 +++++++++++++++++ 6 files changed, 88 insertions(+), 2 deletions(-) diff --git a/api/internal/plugins/builtinconfig/namebackreferences.go b/api/internal/plugins/builtinconfig/namebackreferences.go index ac9e6c45692..eb0f307338f 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences.go +++ b/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 } diff --git a/api/internal/plugins/builtinconfig/namebackreferences_test.go b/api/internal/plugins/builtinconfig/namebackreferences_test.go index b9b60369b98..0e51b80cff7 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences_test.go +++ b/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) + } +} diff --git a/api/internal/plugins/builtinconfig/transformerconfig.go b/api/internal/plugins/builtinconfig/transformerconfig.go index f763fcb30c2..3c6cd11551a 100644 --- a/api/internal/plugins/builtinconfig/transformerconfig.go +++ b/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() } diff --git a/api/internal/plugins/builtinconfig/transformerconfig_test.go b/api/internal/plugins/builtinconfig/transformerconfig_test.go index 112a2d8cde9..2db70c4332f 100644 --- a/api/internal/plugins/builtinconfig/transformerconfig_test.go +++ b/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() + } +} diff --git a/api/types/fieldspec.go b/api/types/fieldspec.go index bdb1503a186..8b78889ea01 100644 --- a/api/types/fieldspec.go +++ b/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) diff --git a/api/types/fieldspec_test.go b/api/types/fieldspec_test.go index c0518b94a9f..83edd73f169 100644 --- a/api/types/fieldspec_test.go +++ b/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) + } +}