diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go index afed66962abd..84d582dda893 100644 --- a/cmd/clusterctl/client/cluster/mover.go +++ b/cmd/clusterctl/client/cluster/mover.go @@ -17,7 +17,6 @@ limitations under the License. package cluster import ( - "context" "fmt" "os" "path/filepath" @@ -35,7 +34,6 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" @@ -853,9 +851,6 @@ func (o *objectMover) createTargetObject(nodeToCreate *node, toProxy Proxy) erro // Rebuild the owne reference chain o.buildOwnerChain(obj, nodeToCreate) - // Save the old managed fields for topology managed fields migration - oldManagedFields := obj.GetManagedFields() - // FIXME Workaround for https://github.com/kubernetes/kubernetes/issues/32220. Remove when the issue is fixed. // If the resource already exists, the API server ordinarily returns an AlreadyExists error. Due to the above issue, if the resource has a non-empty metadata.generateName field, the API server returns a ServerTimeoutError. To ensure that the API server returns an AlreadyExists error, we set the metadata.generateName field to an empty string. if len(obj.GetName()) > 0 && len(obj.GetGenerateName()) > 0 { @@ -902,10 +897,6 @@ func (o *objectMover) createTargetObject(nodeToCreate *node, toProxy Proxy) erro // Stores the newUID assigned to the newly created object. nodeToCreate.newUID = obj.GetUID() - if err := patchTopologyManagedFields(ctx, oldManagedFields, obj, cTo); err != nil { - return err - } - return nil } @@ -1173,32 +1164,3 @@ func (o *objectMover) checkTargetProviders(toInventory InventoryClient) error { return kerrors.NewAggregate(errList) } - -// patchTopologyManagedFields patches the managed fields of obj if parts of it are owned by the topology controller. -// This is necessary to ensure the managed fields created by the topology controller are still present and thus to -// prevent unnecessary machine rollouts. Without patching the managed fields, clusterctl would be the owner of the fields -// which would lead to co-ownership and also additional machine rollouts. -func patchTopologyManagedFields(ctx context.Context, oldManagedFields []metav1.ManagedFieldsEntry, obj *unstructured.Unstructured, cTo client.Client) error { - var containsTopologyManagedFields bool - // Check if the object was owned by the topology controller. - for _, m := range oldManagedFields { - if m.Operation == metav1.ManagedFieldsOperationApply && - m.Manager == structuredmerge.TopologyManagerName && - m.Subresource == "" { - containsTopologyManagedFields = true - break - } - } - // Return early if the object was not owned by the topology controller. - if !containsTopologyManagedFields { - return nil - } - base := obj.DeepCopy() - obj.SetManagedFields(oldManagedFields) - - if err := cTo.Patch(ctx, obj, client.MergeFrom(base)); err != nil { - return errors.Wrapf(err, "error patching managed fields %q %s/%s", - obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) - } - return nil -} diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index a40862b12106..eb4b6a647826 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -339,14 +339,14 @@ func (r *Reconciler) machineDeploymentToCluster(o client.Object) []ctrl.Request // serverSideApplyPatchHelperFactory makes use of managed fields provided by server side apply and is used by the controller. func serverSideApplyPatchHelperFactory(c client.Client) structuredmerge.PatchHelperFactoryFunc { - return func(original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { - return structuredmerge.NewServerSidePatchHelper(original, modified, c, opts...) + return func(ctx context.Context, original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { + return structuredmerge.NewServerSidePatchHelper(ctx, original, modified, c, opts...) } } // dryRunPatchHelperFactory makes use of a two-ways patch and is used in situations where we cannot rely on managed fields. func dryRunPatchHelperFactory(c client.Client) structuredmerge.PatchHelperFactoryFunc { - return func(original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { + return func(ctx context.Context, original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { return structuredmerge.NewTwoWaysPatchHelper(original, modified, c, opts...) } } diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 2e93a13e04f2..10de4fb222b9 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -102,7 +102,7 @@ func (r *Reconciler) reconcileClusterShim(ctx context.Context, s *scope.Scope) e // Note: we are using Patch instead of create for ensuring consistency in managedFields for the entire controller // but in this case it isn't strictly necessary given that we are not using server side apply for modifying the // object afterwards. - helper, err := r.patchHelperFactory(nil, shim) + helper, err := r.patchHelperFactory(ctx, nil, shim) if err != nil { return errors.Wrap(err, "failed to create the patch helper for the cluster shim object") } @@ -385,7 +385,7 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d // If a current MachineHealthCheck doesn't exist but there is a desired MachineHealthCheck attempt to create. if current == nil && desired != nil { log.Infof("Creating %s", tlog.KObj{Obj: desired}) - helper, err := r.patchHelperFactory(nil, desired) + helper, err := r.patchHelperFactory(ctx, nil, desired) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: desired}) } @@ -414,7 +414,7 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d // Check differences between current and desired MachineHealthChecks, and patch if required. // NOTE: we want to be authoritative on the entire spec because the users are // expected to change MHC fields from the ClusterClass only. - patchHelper, err := r.patchHelperFactory(current, desired) + patchHelper, err := r.patchHelperFactory(ctx, current, desired) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: current}) } @@ -439,7 +439,7 @@ func (r *Reconciler) reconcileCluster(ctx context.Context, s *scope.Scope) error ctx, log := tlog.LoggerFrom(ctx).WithObject(s.Desired.Cluster).Into(ctx) // Check differences between current and desired state, and eventually patch the current object. - patchHelper, err := r.patchHelperFactory(s.Current.Cluster, s.Desired.Cluster, structuredmerge.IgnorePaths{ + patchHelper, err := r.patchHelperFactory(ctx, s.Current.Cluster, s.Desired.Cluster, structuredmerge.IgnorePaths{ {"spec", "controlPlaneEndpoint"}, // this is a well known field that is managed by the Cluster controller, topology should not express opinions on it. }) if err != nil { @@ -511,7 +511,7 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust log = log.WithObject(md.Object) log.Infof(fmt.Sprintf("Creating %s", tlog.KObj{Obj: md.Object})) - helper, err := r.patchHelperFactory(nil, md.Object) + helper, err := r.patchHelperFactory(ctx, nil, md.Object) if err != nil { return createErrorWithoutObjectName(err, md.Object) } @@ -566,7 +566,7 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, cluster *clust // Check differences between current and desired MachineDeployment, and eventually patch the current object. log = log.WithObject(desiredMD.Object) - patchHelper, err := r.patchHelperFactory(currentMD.Object, desiredMD.Object) + patchHelper, err := r.patchHelperFactory(ctx, currentMD.Object, desiredMD.Object) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: currentMD.Object}) } @@ -659,7 +659,7 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile // If there is no current object, create it. if in.current == nil { log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) - helper, err := r.patchHelperFactory(nil, in.desired) + helper, err := r.patchHelperFactory(ctx, nil, in.desired) if err != nil { return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } @@ -676,7 +676,7 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile } // Check differences between current and desired state, and eventually patch the current object. - patchHelper, err := r.patchHelperFactory(in.current, in.desired, structuredmerge.IgnorePaths(in.ignorePaths)) + patchHelper, err := r.patchHelperFactory(ctx, in.current, in.desired, structuredmerge.IgnorePaths(in.ignorePaths)) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) } @@ -736,7 +736,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci // If there is no current object, create the desired object. if in.current == nil { log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) - helper, err := r.patchHelperFactory(nil, in.desired) + helper, err := r.patchHelperFactory(ctx, nil, in.desired) if err != nil { return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } @@ -757,7 +757,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci } // Check differences between current and desired objects, and if there are changes eventually start the template rotation. - patchHelper, err := r.patchHelperFactory(in.current, in.desired) + patchHelper, err := r.patchHelperFactory(ctx, in.current, in.desired) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) } @@ -788,7 +788,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName) log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) - helper, err := r.patchHelperFactory(nil, in.desired) + helper, err := r.patchHelperFactory(ctx, nil, in.desired) if err != nil { return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun.go b/internal/controllers/topology/cluster/structuredmerge/dryrun.go deleted file mode 100644 index 37c2abff8b4c..000000000000 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun.go +++ /dev/null @@ -1,302 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package structuredmerge - -import ( - "encoding/json" - "fmt" - "reflect" - "strings" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" - "sigs.k8s.io/controller-runtime/pkg/client" - - "sigs.k8s.io/cluster-api/internal/contract" -) - -// getTopologyManagedFields returns metadata.managedFields entry tracking -// server side apply operations for the topology controller. -func getTopologyManagedFields(original client.Object) map[string]interface{} { - r := map[string]interface{}{} - - for _, m := range original.GetManagedFields() { - if m.Operation == metav1.ManagedFieldsOperationApply && - m.Manager == TopologyManagerName && - m.APIVersion == original.GetObjectKind().GroupVersionKind().GroupVersion().String() { - // NOTE: API server ensures this is a valid json. - err := json.Unmarshal(m.FieldsV1.Raw, &r) - if err != nil { - continue - } - break - } - } - return r -} - -// dryRunPatch determine if the intent defined in the modified object is going to trigger -// an actual change when running server side apply, and if this change might impact the object spec or not. -// NOTE: This func checks if: -// - something previously managed is missing from intent (a field has been deleted from modified) -// - the value for a field previously managed is changing in the intent (a field has been changed in modified) -// - the intent contains something not previously managed (a field has been added to modified). -func dryRunPatch(ctx *dryRunInput) (hasChanges, hasSpecChanges bool) { - // If the func is processing a modified element of type map - if modifiedMap, ok := ctx.modified.(map[string]interface{}); ok { - // NOTE: ignoring the error in case the element wasn't in original. - originalMap, _ := ctx.original.(map[string]interface{}) - - // Process mapType/structType = granular, previously not empty. - // NOTE: mapType/structType = atomic is managed like scalar values down in the func; - // a map is atomic when the corresponding FieldV1 doesn't have info for the nested fields. - if len(ctx.fieldsV1) > 0 { - // Process all the fields the modified map - keys := sets.NewString() - hasChanges, hasSpecChanges = false, false - for field, fieldValue := range modifiedMap { - // Skip apiVersion, kind, metadata.name and metadata.namespace which are required field for a - // server side apply intent but not tracked into metadata.managedFields, otherwise they will be - // considered as a new field added to modified because not previously managed. - if len(ctx.path) == 0 && (field == "apiVersion" || field == "kind") { - continue - } - if len(ctx.path) == 1 && ctx.path[0] == "metadata" && (field == "name" || field == "namespace") { - continue - } - - keys.Insert(field) - - // If this isn't the root of the object and there are already changes detected, it is possible - // to skip processing sibling fields. - if len(ctx.path) > 0 && hasChanges { - continue - } - - // Compute the field path. - fieldPath := ctx.path.Append(field) - - // Get the managed field for this key. - // NOTE: ignoring the conversion error that could happen when modified has a field not previously managed - fieldV1, _ := ctx.fieldsV1[fmt.Sprintf("f:%s", field)].(map[string]interface{}) - - // Get the original value. - fieldOriginalValue := originalMap[field] - - // Check for changes in the field value. - fieldHasChanges, fieldHasSpecChanges := dryRunPatch(&dryRunInput{ - path: fieldPath, - fieldsV1: fieldV1, - modified: fieldValue, - original: fieldOriginalValue, - }) - hasChanges = hasChanges || fieldHasChanges - hasSpecChanges = hasSpecChanges || fieldHasSpecChanges - } - - // Process all the fields the corresponding managed field to identify fields previously managed being - // dropped from modified. - for fieldV1 := range ctx.fieldsV1 { - // Drops "." as it represent the parent field. - if fieldV1 == "." { - continue - } - field := strings.TrimPrefix(fieldV1, "f:") - if !keys.Has(field) { - fieldPath := ctx.path.Append(field) - return pathToResult(fieldPath) - } - } - return - } - } - - // If the func is processing a modified element of type list - if modifiedList, ok := ctx.modified.([]interface{}); ok { - // NOTE: ignoring the error in case the element wasn't in original. - originalList, _ := ctx.original.([]interface{}) - - // Process listType = map/set, previously not empty. - // NOTE: listType = map/set but previously empty is managed like scalar values down in the func. - if len(ctx.fieldsV1) != 0 { - // If the number of items is changed from the previous reconcile it is already clear that - // something is changed without checking all the items. - // NOTE: this assumes the root of the object isn't a list, which is true for all the K8s objects. - if len(modifiedList) != len(ctx.fieldsV1) || len(modifiedList) != len(originalList) { - return pathToResult(ctx.path) - } - - // Otherwise, check the item in the list one by one. - - // if the list is a listMap - if isListMap(ctx.fieldsV1) { - for itemKeys, itemFieldsV1 := range ctx.fieldsV1 { - // Get the keys for the current item. - keys := getFieldV1Keys(itemKeys) - - // Get the corresponding original and modified item. - modifiedItem := getItemWithKeys(modifiedList, keys) - originalItem := getItemWithKeys(originalList, keys) - - // Get the managed field for this item. - // NOTE: ignoring conversion failures because itemFieldsV1 are always of this type. - fieldV1Map, _ := itemFieldsV1.(map[string]interface{}) - - // Check for changes in the item value. - itemHasChanges, itemHasSpecChanges := dryRunPatch(&dryRunInput{ - path: ctx.path, - fieldsV1: fieldV1Map, - modified: modifiedItem, - original: originalItem, - }) - hasChanges = hasChanges || itemHasChanges - hasSpecChanges = hasSpecChanges || itemHasSpecChanges - - // If there are already changes detected, it is possible to skip processing other items. - if hasChanges { - break - } - } - return - } - - if isListSet(ctx.fieldsV1) { - s := sets.NewString() - for v := range ctx.fieldsV1 { - s.Insert(strings.TrimPrefix(v, "v:")) - } - - for _, v := range modifiedList { - // NOTE: ignoring this error because API server ensures the keys in listMap are scalars value. - vString, _ := v.(string) - if !s.Has(vString) { - return pathToResult(ctx.path) - } - } - return - } - } - - // NOTE: listType = atomic is managed like scalar values down in the func; - // a list is atomic when the corresponding FieldV1 doesn't have info for the list items. - } - - // Otherwise, the func is processing scalar or atomic values. - - // Check if the field has been added (it wasn't managed before). - // NOTE: This prevents false positive when handling metadata, because it is required to have metadata.name and metadata.namespace - // in modified, but they are not tracked as managed field. - notManagedBefore := ctx.fieldsV1 == nil - if len(ctx.path) == 1 && ctx.path[0] == "metadata" { - notManagedBefore = false - } - - // Check if the field value is changed. - // NOTE: it is required to use reflect.DeepEqual because in case of atomic map or lists the value is not a scalar value. - valueChanged := !reflect.DeepEqual(ctx.modified, ctx.original) - - if notManagedBefore || valueChanged { - return pathToResult(ctx.path) - } - return false, false -} - -type dryRunInput struct { - // the path of the field being processed. - path contract.Path - // fieldsV1 for the current path. - fieldsV1 map[string]interface{} - - // the original and the modified value for the current path. - modified interface{} - original interface{} -} - -// pathToResult determine if a change in a path impact the spec. -// We assume there is always a change when this call is called; additionally -// we determine the change impacts spec when the path is the root of the object -// or the path starts with spec. -func pathToResult(p contract.Path) (hasChanges, hasSpecChanges bool) { - return true, len(p) == 0 || (len(p) > 0 && p[0] == "spec") -} - -// getFieldV1Keys returns the keys for a listMap item in metadata.managedFields; -// e.g. given the `"k:{\"field1\":\"id1\"}": {...}` item in a ListMap it returns {field1:id1}. -func getFieldV1Keys(v string) map[string]string { - keys := map[string]string{} - keysJSON := strings.TrimPrefix(v, "k:") - // NOTE: ignoring this error because API server ensures this is a valid yaml. - _ = json.Unmarshal([]byte(keysJSON), &keys) - return keys -} - -// getItemKeys returns the keys value pairs for an item in the list. -// e.g. given keys {field1:id1} and values `"{field1:id2, foo:foo}"` it returns {field1:id2}. -// NOTE: keys comes for managedFields, while values comes from the actual object. -func getItemKeys(keys map[string]string, values map[string]interface{}) map[string]string { - keyValues := map[string]string{} - for k := range keys { - if v, ok := values[k]; ok { - // NOTE: API server ensures the keys in listMap are scalars value. - vString, ok := v.(string) - if !ok { - continue - } - keyValues[k] = vString - } - } - return keyValues -} - -// getItemWithKeys return the item in the list with the given keys or nil if any. -// e.g. given l `"[{field1:id1, foo:foo}, {field1:id2, bar:bar}]"` and keys {field1:id1} it returns {field1:id1, foo:foo}. -func getItemWithKeys(l []interface{}, keys map[string]string) map[string]interface{} { - for _, i := range l { - // NOTE: API server ensures the item in a listMap is a map. - iMap, ok := i.(map[string]interface{}) - if !ok { - continue - } - iKeys := getItemKeys(keys, iMap) - if reflect.DeepEqual(iKeys, keys) { - return iMap - } - } - return nil -} - -// isListMap returns true if the fieldsV1 value represent a listMap. -// NOTE: a listMap has elements in the form of `"k:{...}": {...}`. -func isListMap(fieldsV1 map[string]interface{}) bool { - for k := range fieldsV1 { - if strings.HasPrefix(k, "k:") { - return true - } - } - return false -} - -// isListSet returns true if the fieldsV1 value represent a listSet. -// NOTE: a listMap has elements in the form of `"v:..": {}`. -func isListSet(fieldsV1 map[string]interface{}) bool { - for k := range fieldsV1 { - if strings.HasPrefix(k, "v:") { - return true - } - } - return false -} diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go deleted file mode 100644 index 8b08c520335d..000000000000 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go +++ /dev/null @@ -1,888 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package structuredmerge - -import ( - "testing" - - . "github.com/onsi/gomega" - - "sigs.k8s.io/cluster-api/internal/contract" -) - -func Test_dryRunPatch(t *testing.T) { - tests := []struct { - name string - ctx *dryRunInput - wantHasChanges bool - wantHasSpecChanges bool - }{ - { - name: "DryRun detects no changes on managed fields", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:metadata": map[string]interface{}{ - "f:labels": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "apiVersion, kind, metadata.name and metadata.namespace fields in modified are not detected as changes", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - // apiVersion, kind, metadata.name and metadata.namespace are not tracked in managedField. - // NOTE: We are simulating a real object with something in spec and labels, so both - // the top level object and metadata are considered as granular maps. - "f:metadata": map[string]interface{}{ - "f:labels": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", - "kind": "Foo", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", - "kind": "Foo", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "apiVersion, kind, metadata.name and metadata.namespace fields in modified are not detected as changes (edge case)", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - // apiVersion, kind, metadata.name and metadata.namespace are not tracked in managedField. - // NOTE: we are simulating an edge case where we are not tracking managed fields - // in metadata or in spec; this could lead to edge case because server side applies required - // apiVersion, kind, metadata.name and metadata.namespace but those are not tracked in managedFields. - // If this case is not properly handled, dryRun could report false positives assuming those field - // have been added to modified. - }, - original: map[string]interface{}{ - "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", - "kind": "Foo", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - }, - modified: map[string]interface{}{ - "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", - "kind": "Foo", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "DryRun detects metadata only change on managed fields", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:metadata": map[string]interface{}{ - "f:labels": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar-changed", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: false, - }, - { - name: "DryRun spec only change on managed fields", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:metadata": map[string]interface{}{ - "f:labels": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "foo": "bar-changed", - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "identifies changes when modified has a value not previously managed", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - "bar": "baz", // new value not previously managed - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "identifies changes when modified drops a value previously managed", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - // foo (previously managed) has been dropped - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "No changes in an atomic map", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:atomicMap": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicMap": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicMap": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "identifies changes in an atomic map", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:atomicMap": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicMap": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicMap": map[string]interface{}{ - "foo": "bar-changed", - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "No changes on managed atomic list", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:atomicList": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicList": []interface{}{ - map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicList": []interface{}{ - map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "Identifies changes on managed atomic list", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:atomicList": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicList": []interface{}{ - map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "atomicList": []interface{}{ - map[string]interface{}{ - "foo": "bar-changed", - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "No changes on managed listMap", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{ - "k:{\"foo\":\"id1\"}": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - "f:bar": map[string]interface{}{}, - }, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - "bar": "baz", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - "bar": "baz", - }, - }, - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "Identified value added on a empty managed listMap", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{}, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified value added on a managed listMap", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{ - "k:{\"foo\":\"id1\"}": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - }, - map[string]interface{}{ - "foo": "id2", - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified value removed on a managed listMap", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{ - "k:{\"foo\":\"id1\"}": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{}, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified changes on a managed listMap", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{ - "k:{\"foo\":\"id1\"}": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - "f:bar": map[string]interface{}{}, - }, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - "bar": "baz", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - "baz": "baz-changed", - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified changes on a managed listMap (same number of items, different keys)", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listMap": map[string]interface{}{ - "k:{\"foo\":\"id1\"}": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - "f:bar": map[string]interface{}{}, - }, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id1", - "bar": "baz", - }, - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listMap": []interface{}{ - map[string]interface{}{ - "foo": "id2", - "bar": "baz", - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "no changes on a managed listSet", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listSet": map[string]interface{}{ - "v:foo": map[string]interface{}{}, - "v:bar": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar", - }, - }, - }, - }, - wantHasChanges: false, - wantHasSpecChanges: false, - }, - { - name: "Identified value added on a empty managed listSet", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listSet": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{}, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified value added on a managed listSet", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listSet": map[string]interface{}{ - "v:foo": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar", - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified value removed on a managed listSet", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listSet": map[string]interface{}{ - "v:foo": map[string]interface{}{}, - "v:bar": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - // bar removed - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified changes on a managed listSet", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:listSet": map[string]interface{}{ - "v:foo": map[string]interface{}{}, - "v:bar": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar", - }, - }, - }, - modified: map[string]interface{}{ - "spec": map[string]interface{}{ - "listSet": []interface{}{ - "foo", - "bar-changed", - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Identified nested field got added", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - // apiVersion, kind, metadata.name and metadata.namespace are not tracked in managedField. - // NOTE: We are simulating a real object with something in spec and labels, so both - // the top level object and metadata are considered as granular maps. - "f:metadata": map[string]interface{}{ - "f:labels": map[string]interface{}{ - "f:foo": map[string]interface{}{}, - }, - }, - "f:spec": map[string]interface{}{ - "f:another": map[string]interface{}{}, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "another": "value", - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - "spec": map[string]interface{}{ - "another": "value", - "foo": map[string]interface{}{ - "bar": true, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Nested type gets changed", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "f:foo": map[string]interface{}{ - "v:bar": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - "spec": map[string]interface{}{ - "foo": []interface{}{ - "bar", - }, - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": true, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - { - name: "Nested field is getting removed", - ctx: &dryRunInput{ - path: contract.Path{}, - fieldsV1: map[string]interface{}{ - "f:spec": map[string]interface{}{ - "v:keep": map[string]interface{}{}, - "f:foo": map[string]interface{}{ - "v:bar": map[string]interface{}{}, - }, - }, - }, - original: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - "spec": map[string]interface{}{ - "keep": "me", - "foo": []interface{}{ - "bar", - }, - }, - }, - modified: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "bar", - }, - "spec": map[string]interface{}{ - "keep": "me", - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - gotHasChanges, gotHasSpecChanges := dryRunPatch(tt.ctx) - - g.Expect(gotHasChanges).To(Equal(tt.wantHasChanges)) - g.Expect(gotHasSpecChanges).To(Equal(tt.wantHasSpecChanges)) - }) - } -} diff --git a/internal/controllers/topology/cluster/structuredmerge/interfaces.go b/internal/controllers/topology/cluster/structuredmerge/interfaces.go index 28e1dbc5dd2b..2577a1598d9d 100644 --- a/internal/controllers/topology/cluster/structuredmerge/interfaces.go +++ b/internal/controllers/topology/cluster/structuredmerge/interfaces.go @@ -23,7 +23,7 @@ import ( ) // PatchHelperFactoryFunc defines a func that returns a new PatchHelper. -type PatchHelperFactoryFunc func(original, modified client.Object, opts ...HelperOption) (PatchHelper, error) +type PatchHelperFactoryFunc func(ctx context.Context, original, modified client.Object, opts ...HelperOption) (PatchHelper, error) // PatchHelper define the behavior for component responsible for managing patches for Kubernetes objects // owned by the topology controller. diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index ed09aa500aa0..f3162bf16df9 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -19,6 +19,7 @@ package structuredmerge import ( "encoding/json" + jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" "golang.org/x/net/context" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,6 +34,20 @@ import ( // TopologyManagerName is the manager name in managed fields for the topology controller. const TopologyManagerName = "capi-topology" +// allowedPaths defines the list of paths that the topology controller should express opinion on. +var allowedPaths = []contract.Path{ + // apiVersion, kind, name and namespace are required field for a server side apply intent. + {"apiVersion"}, + {"kind"}, + {"metadata", "name"}, + {"metadata", "namespace"}, + // the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only. + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"metadata", "ownerReferences"}, + {"spec"}, +} + type serverSidePatchHelper struct { client client.Client modified *unstructured.Unstructured @@ -41,21 +56,10 @@ type serverSidePatchHelper struct { } // NewServerSidePatchHelper returns a new PatchHelper using server side apply. -func NewServerSidePatchHelper(original, modified client.Object, c client.Client, opts ...HelperOption) (PatchHelper, error) { +func NewServerSidePatchHelper(ctx context.Context, original, modified client.Object, c client.Client, opts ...HelperOption) (PatchHelper, error) { helperOptions := &HelperOptions{} helperOptions = helperOptions.ApplyOptions(opts) - helperOptions.allowedPaths = []contract.Path{ - // apiVersion, kind, name and namespace are required field for a server side apply intent. - {"apiVersion"}, - {"kind"}, - {"metadata", "name"}, - {"metadata", "namespace"}, - // the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only. - {"metadata", "labels"}, - {"metadata", "annotations"}, - {"metadata", "ownerReferences"}, - {"spec"}, - } + helperOptions.allowedPaths = allowedPaths // If required, convert the original and modified objects to unstructured and filter out all the information // not relevant for the topology controller. @@ -78,8 +82,6 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, return nil, errors.Wrap(err, "failed to cleanup legacy managed fields from original object") } } - - filterObject(originalUnstructured, helperOptions) } modifiedUnstructured := &unstructured.Unstructured{} @@ -100,12 +102,14 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, case isNil(original): hasChanges, hasSpecChanges = true, true default: - hasChanges, hasSpecChanges = dryRunPatch(&dryRunInput{ - path: contract.Path{}, - fieldsV1: getTopologyManagedFields(original), - original: originalUnstructured.Object, - modified: modifiedUnstructured.Object, - }) + var err error + hasChanges, hasSpecChanges, err = dryRunSSAPatch(ctx, c, + originalUnstructured, + modifiedUnstructured, + opts...) + if err != nil { + return nil, err + } } return &serverSidePatchHelper{ @@ -144,6 +148,57 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error { return h.client.Patch(ctx, h.modified, client.Apply, options...) } +// dryRunSSAPatch uses server side apply dry run to determine if the operation is going to change the actual object. +func dryRunSSAPatch(ctx context.Context, c client.Client, originalUnstructured, modifiedUnstructured *unstructured.Unstructured, opts ...HelperOption) (bool, bool, error) { + // For dry run we use the same options as for the intent but with adding metadata.managedFields + // so we can ensure that changes to ownership are detected. + dryRunHelperOptions := &HelperOptions{} + dryRunHelperOptions = dryRunHelperOptions.ApplyOptions(opts) + dryRunHelperOptions.allowedPaths = append(allowedPaths, []string{"metadata", "managedFields"}) + + dryRunUnstructured := modifiedUnstructured.DeepCopy() + + err := c.Patch(ctx, dryRunUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership) + if err != nil { + return false, false, err + } + + // Ensure it would not create a new object. + if dryRunUnstructured.GetUID() != originalUnstructured.GetUID() { + return false, false, errors.New("The uid of is not expected to change during server side apply patch dry run. It may have been deleted in the meantime.") + } + + // Drop output of dry run all the fields that are not part of our intent. + filterObject(dryRunUnstructured, dryRunHelperOptions) + filterObject(originalUnstructured, dryRunHelperOptions) + + // Compare the output of dry run to the original object. + originalJSON, err := json.Marshal(originalUnstructured) + if err != nil { + return false, false, err + } + dryRunJSON, err := json.Marshal(dryRunUnstructured) + if err != nil { + return false, false, err + } + + rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, dryRunJSON) + if err != nil { + return false, false, err + } + + // Determine if there are changes to the spec and object. + diff := &unstructured.Unstructured{} + if err := json.Unmarshal(rawDiff, &diff.Object); err != nil { + return false, false, err + } + + hasChanges := len(diff.Object) > 0 + _, hasSpecChanges := diff.Object["spec"] + + return hasChanges, hasSpecChanges, nil +} + // cleanupLegacyManagedFields cleanups managed field management in place before introducing SSA. // NOTE: this operation can trigger a machine rollout, but this is considered acceptable given that ClusterClass is still alpha // and SSA adoption align the topology controller with K8s recommended solution for many controllers authoring the same object. diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 38754108af98..a9ec1d5bda07 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -17,6 +17,7 @@ limitations under the License. package structuredmerge import ( + "encoding/json" "testing" . "github.com/onsi/gomega" @@ -56,7 +57,7 @@ func TestServerSideApply(t *testing.T) { var original *unstructured.Unstructured modified := obj.DeepCopy() - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -67,7 +68,7 @@ func TestServerSideApply(t *testing.T) { var original *clusterv1.MachineDeployment modified := obj.DeepCopy() - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -77,7 +78,7 @@ func TestServerSideApply(t *testing.T) { g := NewWithT(t) // Create a patch helper with original == nil and modified == obj, ensure this is detected as operation that triggers changes. - p0, err := NewServerSidePatchHelper(nil, obj.DeepCopy(), env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, nil, obj.DeepCopy(), env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -113,7 +114,7 @@ func TestServerSideApply(t *testing.T) { // Create a patch helper for a modified object with no changes. modified := obj.DeepCopy() - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -130,7 +131,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() g.Expect(unstructured.SetNestedField(modified.Object, "changed", "status", "foo")).To(Succeed()) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -147,7 +148,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "bar")).To(Succeed()) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -164,7 +165,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() modified.SetLabels(map[string]string{"foo": "changed"}) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -181,7 +182,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() modified.SetAnnotations(map[string]string{"foo": "changed"}) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -201,10 +202,11 @@ func TestServerSideApply(t *testing.T) { APIVersion: "foo/v1alpha1", Kind: "foo", Name: "foo", + UID: "foo", }, }) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -221,7 +223,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "foo")).To(Succeed()) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -256,18 +258,21 @@ func TestServerSideApply(t *testing.T) { // Create a patch helper for a modified object with no changes to what previously applied by th topology manager. modified := obj.DeepCopy() - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) - // Create the object using server side apply + // Change the object using server side apply g.Expect(p0.Patch(ctx)).To(Succeed()) // Check the object and verify fields set by the other controller are preserved. got := obj.DeepCopy() g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + // Check if resourceVersion stayed the same + g.Expect(got.GetResourceVersion()).To(Equal(original.GetResourceVersion())) + v1, _, _ := unstructured.NestedString(got.Object, "spec", "foo") g.Expect(v1).To(Equal("changed")) v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") @@ -300,7 +305,7 @@ func TestServerSideApply(t *testing.T) { modified := obj.DeepCopy() g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed()) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -312,6 +317,9 @@ func TestServerSideApply(t *testing.T) { got := obj.DeepCopy() g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + // Check if resourceVersion did change + g.Expect(got.GetResourceVersion()).ToNot(Equal(original.GetResourceVersion())) + v0, _, _ := unstructured.NestedString(got.Object, "spec", "controlPlaneEndpoint", "host") g.Expect(v0).To(Equal("changed")) v1, _, _ := unstructured.NestedString(got.Object, "spec", "foo") @@ -323,7 +331,46 @@ func TestServerSideApply(t *testing.T) { v4, _, _ := unstructured.NestedBool(got.Object, "status", "ready") g.Expect(v4).To(Equal(true)) }) + t.Run("Topology controller reconcile again with an opinion on a field managed by another controller (co-ownership)", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with some changes to what previously applied by th topology manager. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed()) + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "bar")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + // Create the object using server side apply + g.Expect(p0.Patch(ctx)).To(Succeed()) + + // Check the object and verify the change is applied as well as managed field updated accordingly. + got := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + + // Check if resourceVersion did change + g.Expect(got.GetResourceVersion()).ToNot(Equal(original.GetResourceVersion())) + + v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") + g.Expect(v2).To(Equal("changed")) + + fieldV1 := getTopologyManagedFields(got) + g.Expect(fieldV1).ToNot(BeEmpty()) + g.Expect(fieldV1).To(HaveKey("f:spec")) // topology controller should express opinions on spec. + + specFieldV1 := fieldV1["f:spec"].(map[string]interface{}) + g.Expect(specFieldV1).ToNot(BeEmpty()) + g.Expect(specFieldV1).To(HaveKey("f:controlPlaneEndpoint")) // topology controller should express opinions on spec.controlPlaneEndpoint. + g.Expect(specFieldV1).ToNot(HaveKey("f:foo")) // topology controller should not express opinions on ignore paths. + g.Expect(specFieldV1).To(HaveKey("f:bar")) // topology controller now has an opinion on a field previously managed by other controllers (force ownership). + }) t.Run("Topology controller reconcile again with an opinion on a field managed by another controller (force ownership)", func(t *testing.T) { g := NewWithT(t) @@ -336,7 +383,7 @@ func TestServerSideApply(t *testing.T) { g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed()) g.Expect(unstructured.SetNestedField(modified.Object, "changed-by-topology-controller", "spec", "bar")).To(Succeed()) - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -348,6 +395,9 @@ func TestServerSideApply(t *testing.T) { got := obj.DeepCopy() g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + // Check if resourceVersion did change + g.Expect(got.GetResourceVersion()).ToNot(Equal(original.GetResourceVersion())) + v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") g.Expect(v2).To(Equal("changed-by-topology-controller")) @@ -381,7 +431,7 @@ func TestServerSideApply(t *testing.T) { g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) // Create a patch helper for a modified object with which has no changes. - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + p0, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -417,7 +467,7 @@ func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(obj), original)).To(Succeed()) modified := obj.DeepCopy() - _, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + _, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) g.Expect(err).ToNot(HaveOccurred()) // Get created object after cleanup @@ -444,3 +494,23 @@ func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { g.Expect(gotSSAManager).To(BeTrue()) }) } + +// getTopologyManagedFields returns metadata.managedFields entry tracking +// server side apply operations for the topology controller. +func getTopologyManagedFields(original client.Object) map[string]interface{} { + r := map[string]interface{}{} + + for _, m := range original.GetManagedFields() { + if m.Operation == metav1.ManagedFieldsOperationApply && + m.Manager == TopologyManagerName && + m.APIVersion == original.GetObjectKind().GroupVersionKind().GroupVersion().String() { + // NOTE: API server ensures this is a valid json. + err := json.Unmarshal(m.FieldsV1.Raw, &r) + if err != nil { + continue + } + break + } + } + return r +}