From d3daad9a84ee91ca504e5bc8e77d9a71dee1cdea Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 10 Jan 2022 21:49:43 +0100 Subject: [PATCH] Improve managed field annotation encoding --- .../cluster/mergepatch/managed_paths.go | 202 +++++--- .../cluster/mergepatch/managed_paths_test.go | 198 +++----- .../topology/cluster/mergepatch/mergepatch.go | 6 +- .../cluster/mergepatch/mergepatch_test.go | 472 ++++++++++-------- 4 files changed, 458 insertions(+), 420 deletions(-) diff --git a/internal/controllers/topology/cluster/mergepatch/managed_paths.go b/internal/controllers/topology/cluster/mergepatch/managed_paths.go index f461e4533e8e..fc459aaeaa40 100644 --- a/internal/controllers/topology/cluster/mergepatch/managed_paths.go +++ b/internal/controllers/topology/cluster/mergepatch/managed_paths.go @@ -17,8 +17,11 @@ limitations under the License. package mergepatch import ( - "sort" - "strings" + "bytes" + "compress/gzip" + "encoding/base64" + "encoding/json" + "io" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -28,12 +31,6 @@ import ( "sigs.k8s.io/cluster-api/internal/contract" ) -const ( - managedPathSeparator = "." - managedPathDotReplace = "%" - managedFieldSeparator = "," -) - // DeepCopyWithManagedFieldAnnotation returns a copy of the object with an annotation // Keeping track of the fields the object is setting. func DeepCopyWithManagedFieldAnnotation(obj client.Object) (client.Object, error) { @@ -50,33 +47,7 @@ func deepCopyWithManagedFieldAnnotation(obj client.Object, ignorePaths []contrac return objWithManagedFieldAnnotation, nil } -// getManagedPaths infers the list of paths managed by the topology controller in the previous patch operation -// by parsing the value of the managed field annotation. -// NOTE: if for any reason the annotation is missing, the patch helper will fall back on standard -// two-way merge behavior. -func getManagedPaths(obj client.Object) []contract.Path { - // Gets the managed field annotation from the object. - managedFieldAnnotation := obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] - - // Parses the managed field annotation value into a list of paths. - // NOTE: we are prepending "spec" to the paths to restore the correct absolute path inside the object. - managedFields := strings.Split(managedFieldAnnotation, managedFieldSeparator) - paths := make([]contract.Path, 0, len(managedFields)) - for _, managedField := range managedFields { - if strings.TrimSpace(managedField) == "" { - continue - } - - path := contract.Path{"spec"} - for _, f := range strings.Split(managedField, managedPathSeparator) { - path = append(path, strings.TrimSpace(strings.ReplaceAll(f, managedPathDotReplace, managedPathSeparator))) - } - paths = append(paths, path) - } - return paths -} - -// Stores the list of paths managed by the topology controller into the managed field annotation. +// storeManagedPaths stores the list of paths managed by the topology controller into the managed field annotation. // NOTE: The topology controller is only concerned about managed paths in the spec; given that // we are dropping spec. from the result to reduce verbosity of the generated annotation. // NOTE: Managed paths are relevant only for unstructured objects where it is not possible @@ -91,69 +62,154 @@ func storeManagedPaths(obj client.Object, ignorePaths []contract.Path) error { } // Gets the object spec. - spec, ok, err := unstructured.NestedMap(u.UnstructuredContent(), "spec") + spec, _, err := unstructured.NestedMap(u.UnstructuredContent(), "spec") if err != nil { return errors.Wrap(err, "failed to get object spec") } - // Build a string representation of the paths under spec which are being managed by the object (the fields - // a topology is expressing an opinion/value on, minus the ones we are explicitly ignoring). - managedFields := "" - if ok { - s := []string{} - paths := paths([]string{}, spec) - for _, p := range paths { - ignore := false - pathString := strings.Join(p, managedPathSeparator) - for _, i := range ignorePaths { - if i[0] != "spec" { - continue - } - ignorePathString := strings.Join(i[1:], managedPathSeparator) - if pathString == ignorePathString || strings.HasPrefix(pathString, ignorePathString+managedPathSeparator) { - ignore = true - break - } - } - if !ignore { - s = append(s, strings.Join(p, managedPathSeparator)) - } - } + // Gets a map with the key of the fields we are going to set into spec. + managedFieldsMap := toManagedFieldsMap(spec, specIgnorePaths(ignorePaths)) - // Sort paths to get a predictable order (useful for readability and testing, not relevant at parse time). - sort.Strings(s) - - // Concatenate all the paths in a single string. - // NOTE: add an extra space between fields for better readability (not relevant at parse time). - managedFields = strings.Join(s, managedFieldSeparator+" ") + // Gets the annotation for the given map. + managedFieldAnnotation, err := toManagedFieldAnnotation(managedFieldsMap) + if err != nil { + return err } - // Stores the list of managed paths in an annotation for + // Store the managed paths in an annotation. annotations := obj.GetAnnotations() if annotations == nil { annotations = make(map[string]string, 1) } - annotations[clusterv1.ClusterTopologyManagedFieldsAnnotation] = managedFields + annotations[clusterv1.ClusterTopologyManagedFieldsAnnotation] = managedFieldAnnotation obj.SetAnnotations(annotations) return nil } -// paths builds a slice of paths that exists in the unstructured content. -func paths(path contract.Path, unstructuredContent map[string]interface{}) []contract.Path { +// specIgnorePaths returns ignore paths that apply to spec. +func specIgnorePaths(ignorePaths []contract.Path) []contract.Path { + specPaths := make([]contract.Path, 0, len(ignorePaths)) + for _, i := range ignorePaths { + if i[0] == "spec" && len(i) > 1 { + specPaths = append(specPaths, i[1:]) + } + } + return specPaths +} + +// toManagedFieldsMap returns a map with the key of the fields we are going to set into spec. +// Note: we are dropping ignorePaths. +func toManagedFieldsMap(m map[string]interface{}, ignorePaths []contract.Path) map[string]interface{} { + r := make(map[string]interface{}) + for k, v := range m { + // Drop the key if it matches ignore paths. + ignore := false + for _, i := range ignorePaths { + if i[0] == k && len(i) == 1 { + ignore = true + } + } + if ignore { + continue + } + + // If the field has nested values, process them. + nestedV := make(map[string]interface{}) + if nestedM, ok := v.(map[string]interface{}); ok { + nestedIgnorePaths := make([]contract.Path, 0) + for _, i := range ignorePaths { + if i[0] == k && len(i) > 1 { + nestedIgnorePaths = append(nestedIgnorePaths, i[1:]) + } + } + nestedV = toManagedFieldsMap(nestedM, nestedIgnorePaths) + } + r[k] = nestedV + } + return r +} + +// managedFieldAnnotation returns a managed field annotation for a given managedFieldsMap. +func toManagedFieldAnnotation(managedFieldsMap map[string]interface{}) (string, error) { + if len(managedFieldsMap) == 0 { + return "", nil + } + + // Converts to json. + managedFieldsJSON, err := json.Marshal(managedFieldsMap) + if err != nil { + return "", errors.Wrap(err, "failed to marshal managed fields") + } + + // gzip and base64 encode + var managedFieldsJSONGZIP bytes.Buffer + zw := gzip.NewWriter(&managedFieldsJSONGZIP) + if _, err := zw.Write(managedFieldsJSON); err != nil { + return "", errors.Wrap(err, "failed to write managed fields to gzip writer") + } + if err := zw.Close(); err != nil { + return "", errors.Wrap(err, "failed to close gzip writer for managed fields") + } + managedFields := base64.StdEncoding.EncodeToString(managedFieldsJSONGZIP.Bytes()) + return managedFields, nil +} + +// getManagedPaths infers the list of paths managed by the topology controller in the previous patch operation +// by parsing the value of the managed field annotation. +// NOTE: if for any reason the annotation is missing, the patch helper will fall back on standard +// two-way merge behavior. +func getManagedPaths(obj client.Object) ([]contract.Path, error) { + // Gets the managed field annotation from the object. + managedFieldAnnotation := obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] + + if managedFieldAnnotation == "" { + return nil, nil + } + + managedFieldsJSONGZIP, err := base64.StdEncoding.DecodeString(managedFieldAnnotation) + if err != nil { + return nil, errors.Wrap(err, "failed to decode managed fields") + } + + var managedFieldsJSON bytes.Buffer + zr, err := gzip.NewReader(bytes.NewReader(managedFieldsJSONGZIP)) + if err != nil { + return nil, errors.Wrap(err, "failed to create gzip reader for managed fields") + } + + if _, err := io.Copy(&managedFieldsJSON, zr); err != nil { //nolint:gosec + return nil, errors.Wrap(err, "failed to copy from gzip reader") + } + + if err := zr.Close(); err != nil { + return nil, errors.Wrap(err, "failed to close gzip reader for managed fields") + } + + managedFieldsMap := make(map[string]interface{}) + if err := json.Unmarshal(managedFieldsJSON.Bytes(), &managedFieldsMap); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal managed fields") + } + + paths := flattenManagePaths([]string{"spec"}, managedFieldsMap) + + return paths, nil +} + +// flattenManagePaths builds a slice of paths from a managedFieldMap. +func flattenManagePaths(path contract.Path, unstructuredContent map[string]interface{}) []contract.Path { allPaths := []contract.Path{} for k, m := range unstructuredContent { - key := strings.ReplaceAll(k, managedPathSeparator, managedPathDotReplace) nested, ok := m.(map[string]interface{}) - if !ok { + if ok && len(nested) == 0 { // We have to use a copy of path, because otherwise the slice we append to // allPaths would be overwritten in another iteration. tmp := make([]string, len(path)) copy(tmp, path) - allPaths = append(allPaths, append(tmp, key)) + allPaths = append(allPaths, append(tmp, k)) continue } - allPaths = append(allPaths, paths(append(path, key), nested)...) + allPaths = append(allPaths, flattenManagePaths(append(path, k), nested)...) } return allPaths } diff --git a/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go b/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go index ae4a6ede318b..e6d294ea8844 100644 --- a/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go +++ b/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go @@ -17,6 +17,7 @@ limitations under the License. package mergepatch import ( + "fmt" "testing" . "github.com/onsi/gomega" @@ -27,118 +28,15 @@ import ( "sigs.k8s.io/cluster-api/internal/contract" ) -func Test_getManagedPaths(t *testing.T) { +func Test_ManagedFieldAnnotation(t *testing.T) { tests := []struct { - name string - obj client.Object - want []contract.Path + name string + obj client.Object + ignorePaths []contract.Path + wantPaths []contract.Path }{ { - name: "Return no paths if the annotation does not exists", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{}, - }, - want: []contract.Path{}, - }, - { - name: "Return paths from the annotation", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "foo, bar.baz", - }, - }, - }, - }, - want: []contract.Path{ - {"spec", "foo"}, - {"spec", "bar", "baz"}, - }, - }, - { - name: "Handle label names properly", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "bar.foo%bar%baz, foo", - }, - }, - }, - }, - want: []contract.Path{ - {"spec", "bar", "foo.bar.baz"}, - {"spec", "foo"}, - }, - }, - { - name: "Handle deep nesting ", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.imageRepository, " + - "kubeadmConfigSpec.clusterConfiguration.version, " + - "kubeadmConfigSpec.initConfiguration.bootstrapToken, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.criSocket, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.cgroup-driver, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.eviction-hard, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.criSocket, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.cgroup-driver, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.eviction-hard, " + - "machineTemplate.infrastructureRef.apiVersion, " + - "machineTemplate.infrastructureRef.kind, " + - "machineTemplate.infrastructureRef.name, " + - "machineTemplate.infrastructureRef.namespace, " + - "machineTemplate.metadata.labels.cluster%x-k8s%io/cluster-name, " + - "machineTemplate.metadata.labels.topology%cluster%x-k8s%io/owned, " + - "replicas, " + - "version", - }, - }, - }, - }, - want: []contract.Path{ - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "imageRepository"}, - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "bootstrapToken"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "criSocket"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "criSocket"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, - {"spec", "machineTemplate", "infrastructureRef", "apiVersion"}, - {"spec", "machineTemplate", "infrastructureRef", "kind"}, - {"spec", "machineTemplate", "infrastructureRef", "name"}, - {"spec", "machineTemplate", "infrastructureRef", "namespace"}, - {"spec", "machineTemplate", "metadata", "labels", "cluster.x-k8s.io/cluster-name"}, - {"spec", "machineTemplate", "metadata", "labels", "topology.cluster.x-k8s.io/owned"}, - {"spec", "replicas"}, - {"spec", "version"}, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - got := getManagedPaths(tt.obj) - g.Expect(got).To(Equal(tt.want)) - }) - } -} - -func Test_storeManagedPaths(t *testing.T) { - tests := []struct { - name string - obj client.Object - IgnorePaths []contract.Path - wantAnnotation string - }{ - { - name: "Does not add annotation for typed objects", + name: "Does not add managed fields annotation for typed objects", obj: &clusterv1.Cluster{ Spec: clusterv1.ClusterSpec{ ClusterNetwork: &clusterv1.ClusterNetwork{ @@ -146,10 +44,10 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - wantAnnotation: "", + wantPaths: nil, }, { - name: "Add empty annotation in case there are no changes to spec", + name: "Add empty managed fields annotation in case we are not setting fields in spec", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -159,10 +57,10 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - wantAnnotation: "", + wantPaths: []contract.Path{}, }, { - name: "Add annotation in case of changes to spec", + name: "Add managed fields annotation in case we are not setting fields in spec", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ @@ -173,7 +71,10 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - wantAnnotation: "bar.baz, foo", + wantPaths: []contract.Path{ + {"spec", "foo"}, + {"spec", "bar", "baz"}, + }, }, { name: "Handle label names properly", @@ -187,10 +88,13 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - wantAnnotation: "bar.foo%bar%baz, foo", + wantPaths: []contract.Path{ + {"spec", "foo"}, + {"spec", "bar", "foo.bar.baz"}, + }, }, { - name: "Add annotation handling properly deep nesting in spec", + name: "Add managed fields annotation handling properly deep nesting in spec", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ @@ -238,26 +142,28 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - wantAnnotation: "kubeadmConfigSpec.clusterConfiguration.imageRepository, " + - "kubeadmConfigSpec.clusterConfiguration.version, " + - "kubeadmConfigSpec.initConfiguration.bootstrapToken, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.criSocket, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.cgroup-driver, " + - "kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.eviction-hard, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.criSocket, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.cgroup-driver, " + - "kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.eviction-hard, " + - "machineTemplate.infrastructureRef.apiVersion, " + - "machineTemplate.infrastructureRef.kind, " + - "machineTemplate.infrastructureRef.name, " + - "machineTemplate.infrastructureRef.namespace, " + - "machineTemplate.metadata.labels.cluster%x-k8s%io/cluster-name, " + - "machineTemplate.metadata.labels.topology%cluster%x-k8s%io/owned, " + - "replicas, " + - "version", + wantPaths: []contract.Path{ + {"spec", "replicas"}, + {"spec", "version"}, + {"spec", "kubeadmConfigSpec", "clusterConfiguration", "imageRepository"}, + {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, + {"spec", "kubeadmConfigSpec", "initConfiguration", "bootstrapToken"}, + {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "criSocket"}, + {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, + {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, + {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "criSocket"}, + {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, + {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, + {"spec", "machineTemplate", "infrastructureRef", "namespace"}, + {"spec", "machineTemplate", "infrastructureRef", "apiVersion"}, + {"spec", "machineTemplate", "infrastructureRef", "kind"}, + {"spec", "machineTemplate", "infrastructureRef", "name"}, + {"spec", "machineTemplate", "metadata", "labels", "cluster.x-k8s.io/cluster-name"}, + {"spec", "machineTemplate", "metadata", "labels", "topology.cluster.x-k8s.io/owned"}, + }, }, { - name: "Annotation does not include ignorePaths", + name: "Managed fields annotation does not include ignorePaths", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ @@ -275,22 +181,36 @@ func Test_storeManagedPaths(t *testing.T) { }, }, }, - IgnorePaths: []contract.Path{ + ignorePaths: []contract.Path{ {"spec", "version"}, // exact match (drops a single path) {"spec", "kubeadmConfigSpec", "initConfiguration"}, // prefix match (drops everything below a path) }, - wantAnnotation: "kubeadmConfigSpec.clusterConfiguration.version, kubeadmConfigSpec.joinConfiguration, replicas", + wantPaths: []contract.Path{ + {"spec", "replicas"}, + {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, + {"spec", "kubeadmConfigSpec", "joinConfiguration"}, + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - err := storeManagedPaths(tt.obj, tt.IgnorePaths) + err := storeManagedPaths(tt.obj, tt.ignorePaths) g.Expect(err).ToNot(HaveOccurred()) - gotAnnotation := tt.obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] - g.Expect(gotAnnotation).To(Equal(tt.wantAnnotation)) + _, hasAnnotation := tt.obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] + g.Expect(hasAnnotation).To(Equal(tt.wantPaths != nil)) + + if hasAnnotation { + gotPaths, err := getManagedPaths(tt.obj) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(gotPaths).To(HaveLen(len(tt.wantPaths)), fmt.Sprintf("%v", gotPaths)) + for _, w := range tt.wantPaths { + g.Expect(gotPaths).To(ContainElement(w)) + } + } }) } } diff --git a/internal/controllers/topology/cluster/mergepatch/mergepatch.go b/internal/controllers/topology/cluster/mergepatch/mergepatch.go index 1d687915ad82..0401aceaa310 100644 --- a/internal/controllers/topology/cluster/mergepatch/mergepatch.go +++ b/internal/controllers/topology/cluster/mergepatch/mergepatch.go @@ -60,7 +60,11 @@ func NewHelper(original, modified client.Object, c client.Client, opts ...Helper // Infer the list of paths managed by the topology controller in the previous patch operation; // changes to those paths are going to be considered authoritative. - helperOptions.managedPaths = getManagedPaths(original) + managedPaths, err := getManagedPaths(original) + if err != nil { + return nil, errors.Wrap(err, "failed to marshal original object to json") + } + helperOptions.managedPaths = managedPaths // Convert the input objects to json. originalJSON, err := json.Marshal(original) diff --git a/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go b/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go index ed4c34641605..5d4d668f82ec 100644 --- a/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go +++ b/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go @@ -28,14 +28,23 @@ import ( ) func TestNewHelper(t *testing.T) { + mustManagedFieldAnnotation := func(managedFieldsMap map[string]interface{}) string { + annotation, err := toManagedFieldAnnotation(managedFieldsMap) + if err != nil { + panic(fmt.Sprintf("failed to generated managed field annotation: %v", err)) + } + return annotation + } + tests := []struct { - name string - original *unstructured.Unstructured // current - modified *unstructured.Unstructured // desired - options []HelperOption - wantHasChanges bool - wantHasSpecChanges bool - wantPatch []byte + name string + original *unstructured.Unstructured // current + modified *unstructured.Unstructured // desired + options []HelperOption + ignoreManagedFieldAnnotationChanges bool // Prevent changes to managed field annotation to be generated, so the test can focus on how values are merged. + wantHasChanges bool + wantHasSpecChanges bool + wantPatch []byte }{ // Field both in original and in modified --> align to modified @@ -43,12 +52,6 @@ func TestNewHelper(t *testing.T) { name: "Field (spec) both in original and in modified, no-op when equal", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "foo", - }, - }, "spec": map[string]interface{}{ "foo": "bar", }, @@ -61,20 +64,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Field both in original and in modified, align to modified when different", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "foo", - }, - }, "spec": map[string]interface{}{ "foo": "bar-changed", }, @@ -87,9 +85,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), }, { name: "Field (metadata.label) both in original and in modified, align to modified when different", @@ -99,10 +98,6 @@ func TestNewHelper(t *testing.T) { "labels": map[string]interface{}{ "foo": "bar-changed", }, - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, }, }, }, @@ -115,9 +110,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"bar\"}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"bar\"}}}"), }, { name: "Field (metadata.label) preserve instance specific values when path is not authoritative", @@ -128,10 +124,6 @@ func TestNewHelper(t *testing.T) { "a": "a", "b": "b-changed", }, - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, }, }, }, @@ -146,9 +138,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"b\":\"b\",\"c\":\"c\"}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"b\":\"b\",\"c\":\"c\"}}}"), }, { name: "Field (metadata.label) align to modified when path is authoritative", @@ -159,10 +152,6 @@ func TestNewHelper(t *testing.T) { "a": "a", "b": "b-changed", }, - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, }, }, }, @@ -177,10 +166,11 @@ func TestNewHelper(t *testing.T) { }, }, }, - options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}}, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"a\":null,\"b\":\"b\",\"c\":\"c\"}}}"), + options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}}, + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"a\":null,\"b\":\"b\",\"c\":\"c\"}}}"), }, { name: "IgnorePaths supersede AuthoritativePaths", @@ -191,10 +181,6 @@ func TestNewHelper(t *testing.T) { "a": "a", "b": "b-changed", }, - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, }, }, }, @@ -209,21 +195,16 @@ func TestNewHelper(t *testing.T) { }, }, }, - options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}, IgnorePaths{contract.Path{"metadata", "labels"}}}, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}, IgnorePaths{contract.Path{"metadata", "labels"}}}, + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Nested field both in original and in modified, no-op when equal", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "template.spec.A", - }, - }, "spec": map[string]interface{}{ "template": map[string]interface{}{ "spec": map[string]interface{}{ @@ -244,20 +225,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Nested field both in original and in modified, align to modified when different", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "template.spec.A", - }, - }, "spec": map[string]interface{}{ "template": map[string]interface{}{ "spec": map[string]interface{}{ @@ -278,20 +254,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), }, { name: "Value of type map, enforces entries from modified, preserve entries only in original", original: &unstructured.Unstructured{ Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "map.A, map.C", - }, - }, "spec": map[string]interface{}{ "map": map[string]interface{}{ "A": "A-changed", @@ -312,20 +283,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"C\":\"C\"}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"C\":\"C\"}}}"), }, { name: "Value of type map, enforces entries from modified if the path is authoritative", original: &unstructured.Unstructured{ Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "map.A, map.C", - }, - }, "spec": map[string]interface{}{ "map": map[string]interface{}{ "A": "A-changed", @@ -346,21 +312,16 @@ func TestNewHelper(t *testing.T) { }, }, }, - options: []HelperOption{AuthoritativePaths{contract.Path{"spec", "map"}}}, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"B\":null,\"C\":\"C\"}}}"), + options: []HelperOption{AuthoritativePaths{contract.Path{"spec", "map"}}}, + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"B\":null,\"C\":\"C\"}}}"), }, { name: "Value of type Array or Slice, align to modified", original: &unstructured.Unstructured{ Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "slice", - }, - }, "spec": map[string]interface{}{ "slice": []interface{}{ "D", @@ -381,9 +342,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"), }, // Field only in modified (not existing in original) --> align to modified @@ -391,14 +353,7 @@ func TestNewHelper(t *testing.T) { { name: "Field only in modified, align to modified", original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "foo", - }, - }, - }, + Object: map[string]interface{}{}, }, modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{ @@ -407,21 +362,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), }, { name: "Nested field only in modified, align to modified", original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "template.spec.A", - }, - }, - }, + Object: map[string]interface{}{}, }, modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{ @@ -434,9 +383,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), }, // Field only in original (not existing in modified) --> preserve original @@ -445,12 +395,6 @@ func TestNewHelper(t *testing.T) { name: "Field only in original, align to modified", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, - }, "spec": map[string]interface{}{ "foo": "bar", }, @@ -459,20 +403,15 @@ func TestNewHelper(t *testing.T) { modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{}, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Nested field only in original, align to modified", original: &unstructured.Unstructured{ // current Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, - }, "spec": map[string]interface{}{ "template": map[string]interface{}{ "spec": map[string]interface{}{ @@ -485,9 +424,10 @@ func TestNewHelper(t *testing.T) { modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{}, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, // Diff for metadata fields computed by the system or in status are discarded @@ -495,14 +435,7 @@ func TestNewHelper(t *testing.T) { { name: "Diff for metadata fields computed by the system or in status are discarded", original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, - }, - }, + Object: map[string]interface{}{}, }, modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{ @@ -518,21 +451,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Relevant Diff for metadata (labels and annotations) are preserved", original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, - }, - }, + Object: map[string]interface{}{}, }, modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{ @@ -546,9 +473,10 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"annotations\":{\"foo\":\"bar\"},\"labels\":{\"foo\":\"bar\"}}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"annotations\":{\"foo\":\"bar\"},\"labels\":{\"foo\":\"bar\"}}}"), }, // Ignore fields @@ -556,14 +484,7 @@ func TestNewHelper(t *testing.T) { { name: "Ignore fields are removed from the diff", original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "", - }, - }, - }, + Object: map[string]interface{}{}, }, modified: &unstructured.Unstructured{ // desired Object: map[string]interface{}{ @@ -575,10 +496,11 @@ func TestNewHelper(t *testing.T) { }, }, }, - options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}}, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}}, + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, // Managed fields @@ -600,7 +522,16 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"foo.bar, foo.baz\"}},\"spec\":{\"foo\":{\"bar\":\"\",\"baz\":0}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"foo\":{\"bar\":\"\",\"baz\":0}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + mustManagedFieldAnnotation(map[string]interface{}{ + "foo": map[string]interface{}{ + "bar": map[string]interface{}{}, + "baz": map[string]interface{}{}, + }, + }), + )), }, { name: "Managed field annotation is empty when modified have no spec values", @@ -619,7 +550,11 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: false, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"\"}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + "", + )), }, { name: "Managed field annotation from a previous reconcile are cleaned up when modified have no spec values", @@ -627,7 +562,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "something.from.a.previous.reconcile", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "something": map[string]interface{}{ + "from": map[string]interface{}{ + "a": map[string]interface{}{ + "previous": map[string]interface{}{ + "reconcile": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -643,7 +588,11 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"\"}},\"spec\":{\"something\":{\"from\":{\"a\":{\"previous\":{\"reconcile\":null}}}}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"something\":{\"from\":{\"a\":{\"previous\":{\"reconcile\":null}}}}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + "", + )), }, { name: "Managed field annotation does not include ignored paths - exact match", @@ -663,7 +612,15 @@ func TestNewHelper(t *testing.T) { options: []HelperOption{IgnorePaths{contract.Path{"spec", "foo", "bar"}}}, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"foo.baz\"}},\"spec\":{\"foo\":{\"baz\":0}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"foo\":{\"baz\":0}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + mustManagedFieldAnnotation(map[string]interface{}{ + "foo": map[string]interface{}{ + "baz": map[string]interface{}{}, + }, + }), + )), }, { name: "Managed field annotation does not include ignored paths - path prefix", @@ -683,7 +640,11 @@ func TestNewHelper(t *testing.T) { options: []HelperOption{IgnorePaths{contract.Path{"spec", "foo"}}}, wantHasChanges: true, wantHasSpecChanges: false, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"\"}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + "", + )), }, { name: "changes to managed field are applied", @@ -691,7 +652,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs.enable-hostpath-provisioner", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{ + "enable-hostpath-provisioner": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -733,7 +704,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs.enable-hostpath-provisioner", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{ + "enable-hostpath-provisioner": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -775,7 +756,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs.enable-hostpath-provisioner", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{ + "enable-hostpath-provisioner": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -807,7 +798,19 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"\"}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{}, + }, + }, + }, + }), + )), }, { name: "changes managed object (a field with nested fields) to null is applied; managed field is updated accordingly", @@ -815,7 +818,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs.enable-hostpath-provisioner", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{ + "enable-hostpath-provisioner": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -847,7 +860,19 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs\"}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":null}}}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":null}}}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{}, + }, + }, + }, + }), + )), }, { name: "dropping managed object (a field with nested fields) to null is applied; managed field is updated accordingly", @@ -855,7 +880,17 @@ func TestNewHelper(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: "kubeadmConfigSpec.clusterConfiguration.controllerManager.extraArgs.enable-hostpath-provisioner", + clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{ + "extraArgs": map[string]interface{}{ + "enable-hostpath-provisioner": map[string]interface{}{}, + }, + }, + }, + }, + }), }, }, "spec": map[string]interface{}{ @@ -885,7 +920,17 @@ func TestNewHelper(t *testing.T) { }, wantHasChanges: true, wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf("{\"metadata\":{\"annotations\":{%q:\"\"}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", clusterv1.ClusterTopologyManagedFieldsAnnotation)), + wantPatch: []byte(fmt.Sprintf( + "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", + clusterv1.ClusterTopologyManagedFieldsAnnotation, + mustManagedFieldAnnotation(map[string]interface{}{ + "kubeadmConfigSpec": map[string]interface{}{ + "clusterConfiguration": map[string]interface{}{ + "controllerManager": map[string]interface{}{}, + }, + }, + }), + )), }, // More tests @@ -893,12 +938,6 @@ func TestNewHelper(t *testing.T) { name: "No changes", original: &unstructured.Unstructured{ Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "A, B", - }, - }, "spec": map[string]interface{}{ "A": "A", "B": "B", @@ -914,20 +953,15 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), }, { name: "Many changes", original: &unstructured.Unstructured{ Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - // Prevent changes to managedField to be generated, so the test can focus on how values are merged. - clusterv1.ClusterTopologyManagedFieldsAnnotation: "A, B", - }, - }, "spec": map[string]interface{}{ "A": "A", // B missing @@ -943,15 +977,39 @@ func TestNewHelper(t *testing.T) { }, }, }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"B\":\"B\"}}"), + ignoreManagedFieldAnnotationChanges: true, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"B\":\"B\"}}"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + // Prevent changes to managed field annotation to be generated, so the test can focus on how values are merged. + if tt.ignoreManagedFieldAnnotationChanges { + modified := tt.modified.DeepCopy() + + var ignorePaths []contract.Path + for _, o := range tt.options { + if i, ok := o.(IgnorePaths); ok { + ignorePaths = i + } + } + + // Compute the managed field annotation for modified. + g.Expect(storeManagedPaths(modified, ignorePaths)).To(Succeed()) + + // Clone the managed field annotation back to original. + annotations := tt.original.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string, 1) + } + annotations[clusterv1.ClusterTopologyManagedFieldsAnnotation] = modified.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] + tt.original.SetAnnotations(annotations) + } + patch, err := NewHelper(tt.original, tt.modified, nil, tt.options...) g.Expect(err).ToNot(HaveOccurred())