From 17536fd193bf2996e2a909613cd08ebd1cd092f8 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 23 Jun 2022 10:56:49 +0200 Subject: [PATCH] Improve dry run for topology changes to dry run server side apply Co-authored-by: fabriziopandini --- api/v1beta1/common_types.go | 9 + cmd/clusterctl/client/cluster/mover.go | 38 - .../topology/cluster/cluster_controller.go | 6 +- .../topology/cluster/reconcile_state.go | 22 +- .../cluster/structuredmerge/dryrun.go | 360 ++----- .../cluster/structuredmerge/dryrun_test.go | 980 +++--------------- .../cluster/structuredmerge/interfaces.go | 2 +- .../structuredmerge/serversidepathhelper.go | 28 +- .../serversidepathhelper_test.go | 122 ++- .../v1beta1/dockermachinetemplate_webhook.go | 32 +- .../dockermachinetemplate_webhook_test.go | 3 +- test/infrastructure/docker/main.go | 2 +- util/defaulting/defaulting.go | 40 +- util/defaulting/doc.go | 18 + util/topology/immutability.go | 51 + 15 files changed, 504 insertions(+), 1209 deletions(-) create mode 100644 util/defaulting/doc.go create mode 100644 util/topology/immutability.go diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index f9d89ef75125..1d8d0d633552 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -117,6 +117,15 @@ const ( // An external controller must fulfill the contract of the InfraCluster resource. // External infrastructure providers should ensure that the annotation, once set, cannot be removed. ManagedByAnnotation = "cluster.x-k8s.io/managed-by" + + // TopologyDryRunAnnotation is an annotation that gets set on objects by the topology controller + // only during a server side dry run apply operation. It is used for validating + // update webhooks for objects which get updated by template rotation (e.g. InfrastructureMachineTemplate). + // When the annotation is set and the admission request is a dry run, the webhook should + // deny validation due to immutability. By that the request will succeed (without + // any changes to the actual object because it is a dry run) and the topology controller + // will receive the resulting object. + TopologyDryRunAnnotation = "topology.cluster.x-k8s.io/dry-run" ) const ( 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 f6746e2f5320..a273a02b6a89 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -362,14 +362,14 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu // 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 a4a6eaf3f22b..b993107d1c47 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -101,7 +101,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") } @@ -384,7 +384,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}) } @@ -413,7 +413,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}) } @@ -438,7 +438,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) + patchHelper, err := r.patchHelperFactory(ctx, s.Current.Cluster, s.Desired.Cluster) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: s.Current.Cluster}) } @@ -508,7 +508,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) } @@ -563,7 +563,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}) } @@ -656,7 +656,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") } @@ -673,7 +673,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}) } @@ -733,7 +733,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") } @@ -754,7 +754,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}) } @@ -785,7 +785,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 index 37c2abff8b4c..c4c1d8ca8f6b 100644 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun.go +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun.go @@ -18,285 +18,149 @@ package structuredmerge import ( "encoding/json" - "fmt" - "reflect" - "strings" + jsonpatch "github.com/evanphx/json-patch/v5" + "github.com/pkg/errors" + "golang.org/x/net/context" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "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 +type dryRunSSAPatchInput struct { + client client.Client + // originalUnstructured contains the current state of the object + originalUnstructured *unstructured.Unstructured + // dryRunUnstructured contains the intended changes to the object and will used to + // compare to the originalUnstructured object afterwards. + dryRunUnstructured *unstructured.Unstructured + // helperOptions contains the helper options for filtering the intent. + helperOptions *HelperOptions } -// 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 - } +// dryRunSSAPatch uses server side apply dry run to determine if the operation is going to change the actual object. +func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, bool, error) { + // For dry run we use the same options as for the intent but with adding metadata.managedFields + // to ensure that changes to ownership are detected. + dryRunHelperOptions := &HelperOptions{ + allowedPaths: append(dryRunCtx.helperOptions.allowedPaths, []string{"metadata", "managedFields"}), + ignorePaths: dryRunCtx.helperOptions.ignorePaths, } - // 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. + // Add TopologyDryRunAnnotation to notify validation webhooks to skip immutability checks. + if err := unstructured.SetNestedField(dryRunCtx.dryRunUnstructured.Object, "", "metadata", "annotations", clusterv1.TopologyDryRunAnnotation); err != nil { + return false, false, errors.Wrap(err, "failed to add topology dry-run annotation") } - // Otherwise, the func is processing scalar or atomic values. + // Do a server-side apply dry-run request to get the updated object. + err := dryRunCtx.client.Patch(ctx, dryRunCtx.dryRunUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership) + if err != nil { + // This catches errors like metadata.uid changes. + return false, false, errors.Wrap(err, "failed to request dry-run server side apply") + } - // 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 + // Cleanup the dryRunUnstructured object to remove the added TopologyDryRunAnnotation + // and remove the affected managedFields for `manager=capi-topology` which would + // otherwise show the additional field ownership for the annotation we added and + // the changed managedField timestamp. + if err := cleanupTopologyDryRunAnnotation(dryRunCtx.dryRunUnstructured); err != nil { + return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on dryRunUnstructured") + } + // Also run the function for the originalUnstructured to remove the managedField + // timestamp for `manager=capi-topology`. + if err := cleanupTopologyDryRunAnnotation(dryRunCtx.originalUnstructured); err != nil { + return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on originalUnstructured") } - // 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) + // Drop the other fields which are not part of our intent. + filterObject(dryRunCtx.dryRunUnstructured, dryRunHelperOptions) + filterObject(dryRunCtx.originalUnstructured, dryRunHelperOptions) - if notManagedBefore || valueChanged { - return pathToResult(ctx.path) + // Compare the output of dry run to the original object. + originalJSON, err := json.Marshal(dryRunCtx.originalUnstructured) + if err != nil { + return false, false, err + } + dryRunJSON, err := json.Marshal(dryRunCtx.dryRunUnstructured) + if err != nil { + return false, false, err } - 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{} + rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, dryRunJSON) + if err != nil { + return false, false, err + } - // the original and the modified value for the current path. - modified interface{} - original interface{} -} + // 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 + } -// 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") -} + hasChanges := len(diff.Object) > 0 + _, hasSpecChanges := diff.Object["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 + return hasChanges, hasSpecChanges, nil } -// 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 +// cleanupTopologyDryRunAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run +// annotation as well as the field ownership reference in managedFields. It does +// also remove the timestamp of the managedField for `manager=capi-topology` because +// it is expected to change due to the additional annotation. +func cleanupTopologyDryRunAnnotation(obj *unstructured.Unstructured) error { + // Filter the topology.cluster.x-k8s.io/dry-run annotation as well as leftover empty maps. + filterIntent(&filterIntentInput{ + path: contract.Path{}, + value: obj.Object, + shouldFilter: isIgnorePath([]contract.Path{ + {"metadata", "annotations", clusterv1.TopologyDryRunAnnotation}, + }), + }) + + // Adjust the managed field for Manager=TopologyManagerName, Subresource="", Operation="Apply" + managedFields := obj.GetManagedFields() + for i, managedField := range managedFields { + if managedField.Manager != TopologyManagerName { + continue } - } - 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 { + if managedField.Subresource != "" { continue } - iKeys := getItemKeys(keys, iMap) - if reflect.DeepEqual(iKeys, keys) { - return iMap + if managedField.Operation != metav1.ManagedFieldsOperationApply { + continue } - } - 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 + // Unset the managedField timestamp because managedFields are treated as atomic map. + managedField.Time = nil + + // Unmarshal the managed fields into a map[string]interface{} + fieldsV1 := map[string]interface{}{} + if err := json.Unmarshal(managedField.FieldsV1.Raw, &fieldsV1); err != nil { + return errors.Wrap(err, "failed to unmarshal managed fields") } - } - 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 + // Filter out the annotation ownership as well as leftover empty maps. + filterIntent(&filterIntentInput{ + path: contract.Path{}, + value: fieldsV1, + shouldFilter: isIgnorePath([]contract.Path{ + {"f:metadata", "f:annotations", "f:" + clusterv1.TopologyDryRunAnnotation}, + }), + }) + + fieldsV1Raw, err := json.Marshal(fieldsV1) + if err != nil { + return errors.Wrap(err, "failed to marshal managed fields") } + managedField.FieldsV1.Raw = fieldsV1Raw + + // Replace the modified managedField entry at the slice. + managedFields[i] = managedField + obj.SetManagedFields(managedFields) } - return false + + return nil } diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go index 8b08c520335d..39dae04db6fe 100644 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go @@ -18,871 +18,149 @@ package structuredmerge import ( "testing" + "time" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/cluster-api/internal/contract" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -func Test_dryRunPatch(t *testing.T) { +func Test_cleanupTopologyDryRunAnnotation(t *testing.T) { + rawManagedFieldWithAnnotation := `{"f:metadata":{"f:annotations":{"f:topology.cluster.x-k8s.io/dry-run":{}}}}` + rawManagedFieldWithAnnotationSpecLabels := `{"f:metadata":{"f:annotations":{"f:topology.cluster.x-k8s.io/dry-run":{}},"f:labels":{}},"f:spec":{"f:foo":{}}}` + rawManagedFieldWithSpecLabels := `{"f:metadata":{"f:labels":{}},"f:spec":{"f:foo":{}}}` + tests := []struct { - name string - ctx *dryRunInput - wantHasChanges bool - wantHasSpecChanges bool + name string + obj *unstructured.Unstructured + wantErr bool + want *unstructured.Unstructured }{ { - 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, + name: "no-op", + obj: newCleanupTopologyDryRunBuilder().Build(), + wantErr: false, + }, + { + name: "filter out annotation", + obj: newCleanupTopologyDryRunBuilder(). + WithAnnotation(clusterv1.TopologyDryRunAnnotation, ""). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + Build(), + }, + { + name: "managedFields: manager does not match", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry("other", "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry("other", "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), + }, + { + name: "managedFields: subresource does not match", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "status", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "status", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), + }, + { + name: "managedFields: operation does not match", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationUpdate, []byte(`{}`), nil). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationUpdate, []byte(`{}`), nil). + Build(), + }, + { + name: "managedFields: cleanup up the managed field entry", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(rawManagedFieldWithAnnotation), nil). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), + }, + { + name: "managedFields: cleanup the managed field entry but preserve other ownership", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(rawManagedFieldWithAnnotationSpecLabels), nil). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(rawManagedFieldWithSpecLabels), nil). + Build(), + }, + { + name: "managedFields: remove time", + obj: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(`{}`), &metav1.Time{Time: time.Time{}}). + Build(), + wantErr: false, + want: newCleanupTopologyDryRunBuilder(). + WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). + Build(), }, } 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)) + if err := cleanupTopologyDryRunAnnotation(tt.obj); (err != nil) != tt.wantErr { + t.Errorf("cleanupTopologyDryRunAnnotation() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.want != nil { + g.Expect(tt.obj).To(BeEquivalentTo(tt.want)) + } }) } } + +type cleanupTopologyDryRunBuilder struct { + u *unstructured.Unstructured +} + +func newCleanupTopologyDryRunBuilder() cleanupTopologyDryRunBuilder { + return cleanupTopologyDryRunBuilder{&unstructured.Unstructured{Object: map[string]interface{}{}}} +} + +func (b cleanupTopologyDryRunBuilder) DeepCopy() cleanupTopologyDryRunBuilder { + return cleanupTopologyDryRunBuilder{b.u.DeepCopy()} +} + +func (b cleanupTopologyDryRunBuilder) Build() *unstructured.Unstructured { + return b.u.DeepCopy() +} + +func (b cleanupTopologyDryRunBuilder) WithAnnotation(k, v string) cleanupTopologyDryRunBuilder { + annotations := b.u.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[k] = v + b.u.SetAnnotations(annotations) + return b +} + +func (b cleanupTopologyDryRunBuilder) WithManagedFieldsEntry(manager, subresource string, operation metav1.ManagedFieldsOperationType, fieldsV1 []byte, time *metav1.Time) cleanupTopologyDryRunBuilder { + managedFields := append(b.u.GetManagedFields(), metav1.ManagedFieldsEntry{ + Manager: manager, + Operation: operation, + Subresource: subresource, + FieldsV1: &metav1.FieldsV1{Raw: fieldsV1}, + Time: time, + }) + b.u.SetManagedFields(managedFields) + return b +} 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 b8afc1ad32db..2bcf0a5e0334 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -27,7 +27,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/util" ) @@ -42,7 +41,7 @@ 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) { // Create helperOptions for filtering the original and modified objects to the desired intent. helperOptions := newHelperOptions(modified, opts...) @@ -63,12 +62,10 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, // If the object has been created with previous custom approach for tracking managed fields, cleanup the object. if _, ok := original.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation]; ok { - if err := cleanupLegacyManagedFields(originalUnstructured, c); err != nil { + if err := cleanupLegacyManagedFields(ctx, originalUnstructured, c); err != nil { return nil, errors.Wrap(err, "failed to cleanup legacy managed fields from original object") } } - - filterObject(originalUnstructured, helperOptions) } modifiedUnstructured := &unstructured.Unstructured{} @@ -80,6 +77,9 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, return nil, errors.Wrap(err, "failed to convert modified object to Unstructured") } } + + // Filter the modifiedUnstructured object to only contain changes intendet to be done. + // The originalUnstructured object will be filtered in dryRunSSAPatch using other options. filterObject(modifiedUnstructured, helperOptions) // Carry over uid to match the intent to: @@ -97,12 +97,16 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, case util.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, &dryRunSSAPatchInput{ + client: c, + originalUnstructured: originalUnstructured, + dryRunUnstructured: modifiedUnstructured.DeepCopy(), + helperOptions: helperOptions, }) + if err != nil { + return nil, err + } } return &serverSidePatchHelper{ @@ -144,7 +148,7 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error { // 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. -func cleanupLegacyManagedFields(obj *unstructured.Unstructured, c client.Client) error { +func cleanupLegacyManagedFields(ctx context.Context, obj *unstructured.Unstructured, c client.Client) error { base := obj.DeepCopyObject().(*unstructured.Unstructured) // Remove the topology.cluster.x-k8s.io/managed-field-paths annotation @@ -190,5 +194,5 @@ func cleanupLegacyManagedFields(obj *unstructured.Unstructured, c client.Client) obj.SetManagedFields(managedFields) - return c.Patch(context.TODO(), obj, client.MergeFrom(base)) + return c.Patch(ctx, obj, client.MergeFrom(base)) } diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index c335e390dec1..8336059216a4 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()) @@ -403,11 +453,8 @@ func TestServerSideApply(t *testing.T) { modified.SetUID("") // Create a patch helper which should fail because original's real UID changed. - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(p0.HasChanges()).To(BeTrue()) - g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Patch(ctx)).ToNot(Succeed()) + _, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) + g.Expect(err).To(HaveOccurred()) }) t.Run("Error on object which does not exist (anymore) but was expected to get updated", func(t *testing.T) { original := builder.TestInfrastructureCluster(ns.Name, "obj3").WithSpecFields(map[string]interface{}{ @@ -423,22 +470,13 @@ func TestServerSideApply(t *testing.T) { original.SetUID("does-not-exist") // Create a patch helper which should fail because original does not exist. - p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(p0.HasChanges()).To(BeTrue()) - g.Expect(p0.HasSpecChanges()).To(BeTrue()) - g.Expect(p0.Patch(ctx)).ToNot(Succeed()) + _, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) + g.Expect(err).To(HaveOccurred()) }) } func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { g := NewWithT(t) - - // Write the config file to access the test env for debugging. - // g.Expect(os.WriteFile("test.conf", kubeconfig.FromEnvTestConfig(env.Config, &clusterv1.Cluster{ - // ObjectMeta: metav1.ObjectMeta{Name: "test"}, - // }), 0777)).To(Succeed()) - // Create a namespace for running the test ns, err := env.CreateNamespace(ctx, "ssa") g.Expect(err).ToNot(HaveOccurred()) @@ -460,7 +498,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 @@ -487,3 +525,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 +} diff --git a/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook.go b/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook.go index 1301b45f8b5d..cda7f340f3ce 100644 --- a/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook.go +++ b/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta1 import ( + "context" "fmt" "reflect" @@ -25,42 +26,53 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" + + "sigs.k8s.io/cluster-api/util/topology" ) const dockerMachineTemplateImmutableMsg = "DockerMachineTemplate spec.template.spec field is immutable. Please create a new resource instead." -func (m *DockerMachineTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error { +func (m *DockerMachineTemplateWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). - For(m). + For(&DockerMachineTemplate{}). + WithValidator(m). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-dockermachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=dockermachinetemplates,versions=v1beta1,name=validation.dockermachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Validator = &DockerMachineTemplate{} +type DockerMachineTemplateWebhook struct{} + +var _ webhook.CustomValidator = &DockerMachineTemplateWebhook{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *DockerMachineTemplate) ValidateCreate() error { +func (*DockerMachineTemplateWebhook) ValidateCreate(ctx context.Context, _ runtime.Object) error { return nil } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *DockerMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error { - var allErrs field.ErrorList +func (*DockerMachineTemplateWebhook) ValidateUpdate(ctx context.Context, oldRaw runtime.Object, newRaw runtime.Object) error { + new, ok := newRaw.(*DockerMachineTemplate) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a DockerMachineTemplate but got a %T", newRaw)) + } old, ok := oldRaw.(*DockerMachineTemplate) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a DockerMachineTemplate but got a %T", oldRaw)) } - if !reflect.DeepEqual(m.Spec.Template.Spec, old.Spec.Template.Spec) { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec"), m, dockerMachineTemplateImmutableMsg)) + + var allErrs field.ErrorList + if !topology.ShouldSkipImmutabilityChecks(ctx, new) && + !reflect.DeepEqual(new.Spec.Template.Spec, old.Spec.Template.Spec) { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec"), new, dockerMachineTemplateImmutableMsg)) } if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("DockerMachineTemplate").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(GroupVersion.WithKind("DockerMachineTemplate").GroupKind(), new.Name, allErrs) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *DockerMachineTemplate) ValidateDelete() error { +func (*DockerMachineTemplateWebhook) ValidateDelete(ctx context.Context, _ runtime.Object) error { return nil } diff --git a/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook_test.go b/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook_test.go index fc7f78201af5..8fa59ceb2113 100644 --- a/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook_test.go +++ b/test/infrastructure/docker/api/v1beta1/dockermachinetemplate_webhook_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta1 import ( + "context" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -54,7 +55,7 @@ func TestDockerMachineTemplateInvalid(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.newTemplate.ValidateUpdate(tt.oldTemplate) + err := tt.newTemplate.ValidateUpdate(context.TODO(), tt.oldTemplate, tt.newTemplate) if (err != nil) != tt.wantError { t.Errorf("unexpected result - wanted %+v, got %+v", tt.wantError, err) } diff --git a/test/infrastructure/docker/main.go b/test/infrastructure/docker/main.go index e25b04b1b282..b42a64d59c63 100644 --- a/test/infrastructure/docker/main.go +++ b/test/infrastructure/docker/main.go @@ -240,7 +240,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { } func setupWebhooks(mgr ctrl.Manager) { - if err := (&infrav1.DockerMachineTemplate{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&infrav1.DockerMachineTemplateWebhook{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "DockerMachineTemplate") os.Exit(1) } diff --git a/util/defaulting/defaulting.go b/util/defaulting/defaulting.go index f3fa5eb2327c..6dbaa83faf5b 100644 --- a/util/defaulting/defaulting.go +++ b/util/defaulting/defaulting.go @@ -14,10 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package defaulting implements defaulting webhook functionality. package defaulting import ( + "context" "testing" "github.com/onsi/gomega" @@ -31,6 +31,13 @@ type DefaultingValidator interface { //nolint:revive admission.Validator } +// DefaultingCustomValidator interface is for objects that define defaulting +// and and custom validating webhooks. +type DefaultingCustomValidator interface { //nolint:revive + admission.Defaulter + admission.CustomValidator +} + // DefaultValidateTest returns a new testing function to be used in tests to // make sure defaulting webhooks also pass validation tests on create, // update and delete. @@ -61,3 +68,34 @@ func DefaultValidateTest(object DefaultingValidator) func(*testing.T) { }) } } + +// DefaultCustomValidateTest returns a new testing function to be used in tests to +// make sure defaulting webhooks also pass custom validation tests on create, +// update and delete. +func DefaultCustomValidateTest(object DefaultingCustomValidator) func(*testing.T) { + return func(t *testing.T) { + t.Helper() + + createCopy := object.DeepCopyObject().(DefaultingCustomValidator) + updateCopy := object.DeepCopyObject().(DefaultingCustomValidator) + deleteCopy := object.DeepCopyObject().(DefaultingCustomValidator) + defaultingUpdateCopy := updateCopy.DeepCopyObject().(DefaultingCustomValidator) + + t.Run("validate-on-create", func(t *testing.T) { + g := gomega.NewWithT(t) + createCopy.Default() + g.Expect(createCopy.ValidateCreate(context.TODO(), defaultingUpdateCopy)).To(gomega.Succeed()) + }) + t.Run("validate-on-update", func(t *testing.T) { + g := gomega.NewWithT(t) + defaultingUpdateCopy.Default() + updateCopy.Default() + g.Expect(defaultingUpdateCopy.ValidateUpdate(context.TODO(), updateCopy, defaultingUpdateCopy)).To(gomega.Succeed()) + }) + t.Run("validate-on-delete", func(t *testing.T) { + g := gomega.NewWithT(t) + deleteCopy.Default() + g.Expect(deleteCopy.ValidateDelete(context.TODO(), defaultingUpdateCopy)).To(gomega.Succeed()) + }) + } +} diff --git a/util/defaulting/doc.go b/util/defaulting/doc.go new file mode 100644 index 000000000000..559c824dea21 --- /dev/null +++ b/util/defaulting/doc.go @@ -0,0 +1,18 @@ +/* +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 defaulting implements defaulting webhook functionality. +package defaulting diff --git a/util/topology/immutability.go b/util/topology/immutability.go new file mode 100644 index 000000000000..45425be6a30e --- /dev/null +++ b/util/topology/immutability.go @@ -0,0 +1,51 @@ +/* +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 topology implements topology utility functions. +package topology + +import ( + "context" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +// ShouldSkipImmutabilityChecks returns true if it is a dry-run request and the object has the +// TopologyDryRunAnnotation annotation set, false otherwise. +func ShouldSkipImmutabilityChecks(ctx context.Context, obj metav1.Object) bool { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return false + } + + // Check if the request is a dry-run + if req.DryRun == nil || !*req.DryRun { + return false + } + + // Check for the TopologyDryRunAnnotation + annotations := obj.GetAnnotations() + if annotations == nil { + return false + } + if _, ok := annotations[clusterv1.TopologyDryRunAnnotation]; ok { + return true + } + return false +}