From f214f57e2f4e62431bc23361736507e5123ba3a6 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Thu, 23 Jun 2022 12:02:15 +0200 Subject: [PATCH] Improve dry run for server side apply using an annotation --- api/v1beta1/common_types.go | 4 + cmd/clusterctl/client/cluster/mover.go | 38 - internal/contract/types.go | 14 + .../topology/cluster/cluster_controller.go | 12 +- .../topology/cluster/reconcile_state.go | 22 +- .../topology/cluster/reconcile_state_test.go | 62 +- .../structuredmerge/diff/annotation.go | 106 + .../cluster/structuredmerge/diff/diff.go | 472 ++++ .../cluster/structuredmerge/diff/diff_test.go | 1996 +++++++++++++++++ .../cluster/structuredmerge/diff/doc.go | 18 + .../cluster/structuredmerge/diff/schema.go | 133 ++ .../structuredmerge/diff/schema_cache.go | 215 ++ .../structuredmerge/diff/schema_cache_test.go | 382 ++++ .../cluster/structuredmerge/dryrun.go | 302 --- .../cluster/structuredmerge/dryrun_test.go | 888 -------- .../cluster/structuredmerge/interfaces.go | 2 +- .../structuredmerge/serversidepathhelper.go | 37 +- .../serversidepathhelper_test.go | 74 +- 18 files changed, 3489 insertions(+), 1288 deletions(-) create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/annotation.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/diff.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/diff_test.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/doc.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/schema.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/schema_cache.go create mode 100644 internal/controllers/topology/cluster/structuredmerge/diff/schema_cache_test.go delete mode 100644 internal/controllers/topology/cluster/structuredmerge/dryrun.go delete mode 100644 internal/controllers/topology/cluster/structuredmerge/dryrun_test.go diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index f9d89ef75125..f5dc3eeffa88 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -43,6 +43,10 @@ const ( // Deprecated: Topology controller is now using server side apply and this annotation will be removed in a future release. ClusterTopologyManagedFieldsAnnotation = "topology.cluster.x-k8s.io/managed-field-paths" + // ClusterTopologyLastAppliedIntentAnnotation is the annotation used to store the last-applied-intent + // by the topology controller. + ClusterTopologyLastAppliedIntentAnnotation = "topology.cluster.x-k8s.io/last-applied-intent" + // ClusterTopologyMachineDeploymentLabelName is the label set on the generated MachineDeployment objects // to track the name of the MachineDeployment topology it represents. ClusterTopologyMachineDeploymentLabelName = "topology.cluster.x-k8s.io/deployment-name" 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/contract/types.go b/internal/contract/types.go index febd30e05a48..49d9937e488d 100644 --- a/internal/contract/types.go +++ b/internal/contract/types.go @@ -34,6 +34,20 @@ func (p Path) Append(k string) Path { return append(p, k) } +// Item return a path for an item element, represented as + []. item elements are used as +// a placeholder for schema definitions for list items of for map items. +func (p Path) Item() Path { + item := Path{} + for i, x := range p { + if i < len(p)-1 { + item = append(item, x) + continue + } + item = append(item, x+"[]") + } + return item +} + // IsParentOf check if one path is Parent of the other. func (p Path) IsParentOf(other Path) bool { if len(p) >= len(other) { diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index a40862b12106..7ee38191642f 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge/diff" runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog" runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" "sigs.k8s.io/cluster-api/util" @@ -114,7 +115,8 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt r.patchEngine = patches.NewEngine(r.RuntimeClient) r.recorder = mgr.GetEventRecorderFor("topology/cluster") if r.patchHelperFactory == nil { - r.patchHelperFactory = serverSideApplyPatchHelperFactory(r.Client) + crdSchemaCache := diff.NewCRDSchemaCache(r.Client) + r.patchHelperFactory = serverSideApplyPatchHelperFactory(r.Client, crdSchemaCache) } return nil } @@ -338,15 +340,15 @@ 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...) +func serverSideApplyPatchHelperFactory(c client.Client, crdSchemaCache diff.CRDSchemaCache) structuredmerge.PatchHelperFactoryFunc { + return func(ctx context.Context, original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { + return structuredmerge.NewServerSidePatchHelper(ctx, original, modified, c, crdSchemaCache, 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/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 3fca565f4a22..ae1f15d6b3f5 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge/diff" "sigs.k8s.io/cluster-api/internal/hooks" runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog" fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake" @@ -52,9 +53,14 @@ var ( IgnoreNameGenerated = IgnorePaths{ {"metadata", "name"}, } + IgnoreLastAppliedIntentAnnotation = IgnorePaths{ + {"metadata", "annotations", clusterv1.ClusterTopologyLastAppliedIntentAnnotation}, + } ) func TestReconcileShim(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() @@ -86,7 +92,7 @@ func TestReconcileShim(t *testing.T) { r := Reconciler{ Client: env, APIReader: env.GetAPIReader(), - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -129,7 +135,7 @@ func TestReconcileShim(t *testing.T) { r := Reconciler{ Client: env, APIReader: env.GetAPIReader(), - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -179,7 +185,7 @@ func TestReconcileShim(t *testing.T) { r := Reconciler{ Client: env, APIReader: env.GetAPIReader(), - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -226,7 +232,7 @@ func TestReconcileShim(t *testing.T) { r := Reconciler{ Client: env, APIReader: env.GetAPIReader(), - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -267,7 +273,7 @@ func TestReconcileShim(t *testing.T) { r := Reconciler{ Client: nil, APIReader: env.GetAPIReader(), - patchHelperFactory: serverSideApplyPatchHelperFactory(nil), + patchHelperFactory: serverSideApplyPatchHelperFactory(nil, crdSchemaCache), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -854,6 +860,8 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { } func TestReconcileCluster(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + cluster1 := builder.Cluster(metav1.NamespaceDefault, "cluster1"). Build() cluster1WithReferences := builder.Cluster(metav1.NamespaceDefault, "cluster1"). @@ -915,7 +923,7 @@ func TestReconcileCluster(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } err = r.reconcileCluster(ctx, s) @@ -941,6 +949,7 @@ func TestReconcileCluster(t *testing.T) { func TestReconcileInfrastructureCluster(t *testing.T) { g := NewWithT(t) + crdSchemaCache := diff.NewCRDSchemaCache(env) // build an infrastructure cluster with a field managed by the topology controller (derived from the template). clusterInfrastructure1 := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1"). @@ -1040,7 +1049,7 @@ func TestReconcileInfrastructureCluster(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } err = r.reconcileInfrastructureCluster(ctx, s) @@ -1075,6 +1084,7 @@ func TestReconcileInfrastructureCluster(t *testing.T) { func TestReconcileControlPlane(t *testing.T) { g := NewWithT(t) + crdSchemaCache := diff.NewCRDSchemaCache(env) // Objects for testing reconciliation of a control plane without machines. @@ -1294,7 +1304,7 @@ func TestReconcileControlPlane(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } @@ -1406,6 +1416,8 @@ func TestReconcileControlPlane(t *testing.T) { } func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + // Create InfrastructureMachineTemplates for test cases infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build() @@ -1554,7 +1566,7 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } @@ -1579,13 +1591,14 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) - g.Expect(gotMHC).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{{"kind"}, {"apiVersion"}})) + g.Expect(gotMHC).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnoreLastAppliedIntentAnnotation, IgnorePaths{{"kind"}, {"apiVersion"}})) }) } } func TestReconcileMachineDeployments(t *testing.T) { g := NewWithT(t) + crdSchemaCache := diff.NewCRDSchemaCache(env) // Write the config file to access the test env for debugging. // g.Expect(os.WriteFile("test.conf", kubeconfig.FromEnvTestConfig(env.Config, &clusterv1.Cluster{ @@ -1803,7 +1816,7 @@ func TestReconcileMachineDeployments(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } err = r.reconcileMachineDeployments(ctx, s) @@ -1849,7 +1862,7 @@ func TestReconcileMachineDeployments(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) - g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata, IgnoreNameGenerated)) + g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata, IgnoreLastAppliedIntentAnnotation, IgnoreNameGenerated)) // Check BootstrapTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.BootstrapTemplate != nil { @@ -1873,7 +1886,7 @@ func TestReconcileMachineDeployments(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) - g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata, IgnoreNameGenerated)) + g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata, IgnoreLastAppliedIntentAnnotation, IgnoreNameGenerated)) // Check InfrastructureMachineTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.InfrastructureMachineTemplate != nil { @@ -1894,6 +1907,8 @@ func TestReconcileMachineDeployments(t *testing.T) { // NOTE: by Extension this tests validates managed field handling in mergePatches, and thus its usage in other parts of the // codebase. func TestReconcileReferencedObjectSequences(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + // 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{ @@ -2282,7 +2297,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } @@ -2395,6 +2410,8 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { } func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + md := builder.MachineDeployment(metav1.NamespaceDefault, "md-1").WithLabels( map[string]string{ clusterv1.ClusterTopologyMachineDeploymentLabelName: "machine-deployment-one", @@ -2520,6 +2537,13 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { uidsByName[mdts.Object.Name] = mdts.Object.GetUID() if mdts.MachineHealthCheck != nil { + // Adds last applied intent annotation assuming the current object has been originated from the topology controller, without external changes. + currentIntent := &unstructured.Unstructured{} + g.Expect(env.Scheme().Convert(mdts.MachineHealthCheck, currentIntent, nil)).To(Succeed()) + g.Expect(diff.AddCurrentIntentAnnotation(currentIntent)).To(Succeed()) + g.Expect(env.Scheme().Convert(currentIntent, mdts.MachineHealthCheck, nil)).To(Succeed()) + mdts.MachineHealthCheck.SetGroupVersionKind(currentIntent.GroupVersionKind()) + for i, ref := range mdts.MachineHealthCheck.OwnerReferences { ref.UID = mdts.Object.GetUID() mdts.MachineHealthCheck.OwnerReferences[i] = ref @@ -2548,7 +2572,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } @@ -2570,7 +2594,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { ref.UID = "" actual.OwnerReferences[i] = ref } - g.Expect(wantMHC).To(EqualObject(&actual, IgnoreAutogeneratedMetadata)) + g.Expect(&actual).To(EqualObject(wantMHC, IgnoreAutogeneratedMetadata, IgnoreLastAppliedIntentAnnotation)) } } } @@ -2603,6 +2627,8 @@ func toMachineDeploymentTopologyStateMap(states []*scope.MachineDeploymentState) } func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { + crdSchemaCache := diff.NewCRDSchemaCache(env) + // create a controlPlane object with enough information to be used as an OwnerReference for the MachineHealthCheck. cp := builder.ControlPlane(metav1.NamespaceDefault, "cp1").Build() mhcBuilder := builder.MachineHealthCheck(metav1.NamespaceDefault, "cp1"). @@ -2694,7 +2720,7 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { r := Reconciler{ Client: env, - patchHelperFactory: serverSideApplyPatchHelperFactory(env), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, crdSchemaCache), recorder: env.GetEventRecorderFor("test"), } if tt.current != nil { @@ -2717,7 +2743,7 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { } } - g.Expect(got).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{{"kind"}, {"apiVersion"}})) + g.Expect(got).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnoreLastAppliedIntentAnnotation, IgnorePaths{{"kind"}, {"apiVersion"}})) }) } } diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/annotation.go b/internal/controllers/topology/cluster/structuredmerge/diff/annotation.go new file mode 100644 index 000000000000..2ac0312b897f --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/annotation.go @@ -0,0 +1,106 @@ +/* +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 diff + +import ( + "bytes" + "compress/gzip" + "encoding/base64" + "encoding/json" + "io" + + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +// AddCurrentIntentAnnotation adds to the object a last applied intent annotation using the object itself as an intent. +// NOTE: this func is designed to be used in the server side helper, where the modified object that has +// been generated in compute desired state + patches is already trimmed down to a server side apply intent - +// the set of field the controller express an opinion on - by considering allowed and ignore paths. +func AddCurrentIntentAnnotation(obj *unstructured.Unstructured) error { + currentIntentAnnotation, err := toCurrentIntentAnnotation(obj) + if err != nil { + return err + } + + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string, 1) + } + annotations[clusterv1.ClusterTopologyLastAppliedIntentAnnotation] = currentIntentAnnotation + obj.SetAnnotations(annotations) + + return nil +} + +func toCurrentIntentAnnotation(currentIntent *unstructured.Unstructured) (string, error) { + // Converts to json. + currentIntentJSON, err := json.Marshal(currentIntent.Object) + if err != nil { + return "", errors.Wrap(err, "failed to marshal current intent") + } + + // gzip and base64 encode + var currentIntentJSONGZIP bytes.Buffer + zw := gzip.NewWriter(¤tIntentJSONGZIP) + if _, err := zw.Write(currentIntentJSON); err != nil { + return "", errors.Wrap(err, "failed to write current intent to gzip writer") + } + if err := zw.Close(); err != nil { + return "", errors.Wrap(err, "failed to close gzip writer for current intent") + } + currentIntentAnnotation := base64.StdEncoding.EncodeToString(currentIntentJSONGZIP.Bytes()) + return currentIntentAnnotation, nil +} + +// GetPreviousIntent returns the previous intent stored in the object's the last applied annotation. +func GetPreviousIntent(obj client.Object) (*unstructured.Unstructured, error) { + previousIntentAnnotation := obj.GetAnnotations()[clusterv1.ClusterTopologyLastAppliedIntentAnnotation] + + if previousIntentAnnotation == "" { + return &unstructured.Unstructured{}, nil + } + + previousIntentJSONGZIP, err := base64.StdEncoding.DecodeString(previousIntentAnnotation) + if err != nil { + return nil, errors.Wrap(err, "failed to decode previous intent") + } + + var previousIntentJSON bytes.Buffer + zr, err := gzip.NewReader(bytes.NewReader(previousIntentJSONGZIP)) + if err != nil { + return nil, errors.Wrap(err, "failed to create gzip reader forprevious intent") + } + + if _, err := io.Copy(&previousIntentJSON, zr); err != nil { //nolint:gosec + return nil, errors.Wrap(err, "failed to copy from gzip reader") + } + + if err := zr.Close(); err != nil { + return nil, errors.Wrap(err, "failed to close gzip reader for managed fields") + } + + previousIntentMap := make(map[string]interface{}) + if err := json.Unmarshal(previousIntentJSON.Bytes(), &previousIntentMap); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal managed fields") + } + + return &unstructured.Unstructured{Object: previousIntentMap}, nil +} diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/diff.go b/internal/controllers/topology/cluster/structuredmerge/diff/diff.go new file mode 100644 index 000000000000..3bff4fef3ba0 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/diff.go @@ -0,0 +1,472 @@ +/* +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 diff + +import ( + "fmt" + "reflect" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +// NOTE: We can assume that current intent and current object always have the same gvk, +// given that we are always calling UpdateReferenceAPIContract when reading both of them. + +// DryRunDiff determine if the current intent is going to determine changes in the object. +func DryRunDiff(in *DryRunDiffInput) (hasChanges, hasSpecChanges bool) { + // Check if the current intent is going to actually change something in the current object + hasChanges, hasSpecChanges = isChangingAnything(&isChangingAnythingInput{ + path: contract.Path{}, + currentIntent: in.CurrentIntent.Object, + currentObject: in.CurrentObject.Object, + schema: in.Schema, + }) + + // If we already found a diff, we can return here without processing following checks. + if hasChanges && hasSpecChanges { + return hasChanges, hasSpecChanges + } + + // compares previous intent and current intent to determine if the topology + // controller stopped to express opinion on some fields. + hasDeletion, hasSpecDeletion := isDroppingAnyIntent(&isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: in.PreviousIntent.Object, + currentIntent: in.CurrentIntent.Object, + currentObject: in.CurrentObject.Object, + schema: in.Schema, + }) + hasChanges = hasChanges || hasDeletion + hasSpecChanges = hasSpecChanges || hasSpecDeletion + + return hasChanges, hasSpecChanges +} + +// DryRunDiffInput is the input of the DryRunDiff func. +type DryRunDiffInput struct { + // the previous intent, that is the values the topology controller enforced at the previous reconcile. + PreviousIntent *unstructured.Unstructured + // the current intent, that is the values the topology controller is enforcing at this reconcile. + CurrentIntent *unstructured.Unstructured + + // the current object read at the beginning of the reconcile process. + CurrentObject *unstructured.Unstructured + + Schema CRDSchema +} + +// isChangingAnything checks if the current intent is going to actually change something in the current object. +// NOTE: We can assume that current intent and current object always have the same gvk, +// given that we are always calling UpdateReferenceAPIContract when reading both of them. +func isChangingAnything(in *isChangingAnythingInput) (hasChanges, hasSpecChanges bool) { + typeDef, ok := in.schema[in.path.String()] + if !ok { + // TODO: log that the operation is done assuming atomic value. + } + + if typeDef.Struct != nil || typeDef.Map != nil { + if (typeDef.Struct != nil && typeDef.Struct.Type == GranularStructType) || (typeDef.Map != nil && typeDef.Map.Type == GranularMapType) { + currentIntentMap, ok := in.currentIntent.(map[string]interface{}) + if !ok { + // NOTE: this condition should never happen because this func is called while walking through values in the current intent. + // However, for extra caution, it is handled nevertheless. + return hasChanges, hasSpecChanges + } + + // Get the corresponding struct/map in the current object. + // If the corresponding struct/map does not exist in the current object, it means it is being added by the intent (it is a change). + currentObjectMap, ok := in.currentObject.(map[string]interface{}) + if !ok { + return pathToResult(in.path) + } + + // For each field in the current intent. + for field, currentIntentValue := range currentIntentMap { + // Compute the path to get access to the field schema; in case of struct, it is defined by the current path + the field name. + // For maps instead every item has the same schema defined at current path + []. + fieldPath := in.path.Append(field) + if typeDef.Map != nil { + fieldPath = in.path.Item() + } + + // Get the corresponding value in the current object. + // If the corresponding value does not exist in the current object, it means it is being added by the intent (it is a change). + currentObjectValue, ok := currentObjectMap[field] + if !ok { + fieldHasChanges, fieldHasSpecChanges := pathToResult(fieldPath) + hasChanges = hasChanges || fieldHasChanges + hasSpecChanges = hasSpecChanges || fieldHasSpecChanges + + // If we already detected a spec change, no need to look further, otherwise + // we should keep going to process remaining entries in the struct/map. + if hasSpecChanges { + return hasChanges, hasSpecChanges + } + continue + } + + // Otherwise, if there is a corresponding value, check for changes recursively. + fieldHasChanges, fieldHasSpecChanges := isChangingAnything(&isChangingAnythingInput{ + path: fieldPath, + currentIntent: currentIntentValue, + currentObject: currentObjectValue, + schema: in.schema, + }) + hasChanges = hasChanges || fieldHasChanges + hasSpecChanges = hasSpecChanges || fieldHasSpecChanges + + // If we already detected a spec change, no need to look further, otherwise + // we should keep going to process remaining entries in the struct/map. + if hasSpecChanges { + return hasChanges, hasSpecChanges + } + } + + return hasChanges, hasSpecChanges + } + + // NOTE: struct/maps of Type atomic are treated below. + } + + if typeDef.List != nil { + // Get the corresponding list in the current intent. + currentIntentList, ok := in.currentIntent.([]interface{}) + if !ok { + // NOTE: this condition should never happen, because entire list deletion is caught when processing fields in a map, and the list is always a field in a map. + // However, for extra caution, it is handled nevertheless. + return hasChanges, hasSpecChanges + } + + // Get the corresponding list in the current object. + // If the corresponding list does not exist in the current object, it means it is being added by the intent (it is a change). + currentObjectList, ok := in.currentObject.([]interface{}) + if !ok { + return pathToResult(in.path) + } + + if typeDef.List.Type == ListMapType { + // If it is ListMap, we know that the list includes only map items and those items must have a set of unique listMapKeys values. + // Accordingly, we detect changes by looking for items (maps identified by a listMapKeys values) both in current object and current intent and checking for differences. + // e.g. previous intent [{"id": "1", "foo": "foo", ...}, {...}], current intent [{"id": "1", "foo": "foo-changed", ...}, {...}], item with key "id": "1" has been changed. + + // Check if items in the current intent have changes from the current object. + for _, currentIntentItem := range currentIntentList { + // Get the corresponding item from the current object, which is the item with the same ListMap keys values. + // If the corresponding item does not exist in the current object, it means it is being added by the intent (it is a change). + currentObjectItem := typeDef.List.GetSameItem(currentObjectList, currentIntentItem) + if currentObjectItem == nil { + return pathToResult(in.path) + } + + // Otherwise, if there is a corresponding item, check for deletion recursively. + if itemHasChanges, itemHasSpecChanges := isChangingAnything(&isChangingAnythingInput{ + path: in.path.Item(), + currentIntent: currentIntentItem, + currentObject: currentObjectItem, + schema: in.schema, + }); itemHasChanges || itemHasSpecChanges { + return itemHasChanges, itemHasSpecChanges + } + } + + return hasChanges, hasSpecChanges + } + + if typeDef.List.Type == ListSetType { + // If it is ListSet, we know that the list includes only scalar items and those items must be unique. + // Accordingly, we detect changes by looking for items (scalar values) present in the current intent but not in the current object. + // e.g. previous intent ["a", "b"], current intent ["a"], item "b" has been deleted. + + // Get the corresponding set of list items from the current object. + currentObjectSet := sets.NewString() + for _, currentObjectItem := range currentObjectList { + currentObjectSet.Insert(fmt.Sprintf("%s", currentObjectItem)) + } + + // Check if items in the current intent have changes from the current object; + // being the List a set of scalar values, we are only concerned about additions. + for _, currentIntentItem := range currentIntentList { + // If the corresponding item in the corresponding does not exist in the current object, it means it is being added by the intent (it is a change). + if !currentObjectSet.Has(fmt.Sprintf("%s", currentIntentItem)) { + return pathToResult(in.path) + } + } + + return hasChanges, hasSpecChanges + } + + // NOTE: lists of Type atomic are treated below. + } + + // Otherwise we consider the field a Scalar or an atomic value. + + // Compare current intent and current object, if they are different it means it is being modified by the intent (it is a change). + // NOTE: we use reflect.DeepEqual because the value can be a nested struct. + if !reflect.DeepEqual(in.currentIntent, in.currentObject) { + return pathToResult(in.path) + } + + return hasChanges, hasSpecChanges +} + +// isChangingAnythingInput is the input of the isChangingAnything func. +type isChangingAnythingInput struct { + path contract.Path + + currentIntent interface{} + currentObject interface{} + + schema CRDSchema +} + +// isDroppingAnyIntent compares previous intent and current intent to determine if the topology +// controller stopped to express opinion on some fields; as further optimization, +// detected deletion from the intent are ignored in case the corresponding value +// has been already removed from the current object. +// NOTE: We can assume that current intent and current object always have the same gvk, +// given that we are always calling UpdateReferenceAPIContract when reading both of them. +// NOTE: Previous intent instead could have an older gvk, and in that case we look for dropped +// intent at best effort, using the latest gvk schema. +// A possible improvements will be for the providers to convert the content of the previous intent +// while converting the object to a newer version. +func isDroppingAnyIntent(in *isDroppingAnyIntentInput) (hasDeletion, hasSpecDeletion bool) { + typeDef, ok := in.schema[in.path.String()] + if !ok { + // TODO: log that the operation is done assuming atomic value. + } + + if typeDef.Struct != nil || typeDef.Map != nil { + if (typeDef.Struct != nil && typeDef.Struct.Type == GranularStructType) || (typeDef.Map != nil && typeDef.Map.Type == GranularMapType) { + // Get the corresponding struct/map in the current object. + // NOTE: The conversion error is ignored because the struct/map could have been already removed from the current object. + currentObjectStruct, _ := in.currentObject.(map[string]interface{}) + + // Get the struct/map from the previous intent. + previousIntentStruct, ok := in.previousIntent.(map[string]interface{}) + if !ok { + // NOTE: this condition should never happen because this func is called while walking through values in the previous intent. + // However, for extra caution, it is handled nevertheless. + return hasDeletion, hasSpecDeletion + } + + // Get the corresponding struct/map from the current intent. + currentIntentStruct, ok := in.currentIntent.(map[string]interface{}) + if !ok { + // NOTE: this condition should never happen, because entire struct/map deletion is caught when processing fields in a struct, and the struct/map is always a field in a parent struct. + // However, for extra caution, it is handled nevertheless. + + // If the struct/map still is in the current object, then there is a deletion the topology controller should reconcile. + if currentObjectStruct != nil { + return pathToResult(in.path) + } + return hasDeletion, hasSpecDeletion + } + + // For each field in the previous intent. + for field, previousIntentValue := range previousIntentStruct { + // Compute the path to get access to the field schema; in case of struct, it is defined by the current path + the field name. + // For maps instead every item has the same schema defined at current path + []. + fieldPath := in.path.Append(field) + if typeDef.Map != nil { + fieldPath = in.path.Item() + } + + // Get the corresponding value in the current object. + // NOTE: The item could have been already removed from the current object. + currentObjectValue := currentObjectStruct[field] + + // Get the corresponding value in the current intent; if there is no corresponding value, + // the field has been deleted from the current intent. + currentIntentValue, ok := currentIntentStruct[field] + if !ok { + if currentObjectValue != nil { + fieldHasDeletion, fieldHasSpecDeletion := pathToResult(fieldPath) + hasDeletion = hasDeletion || fieldHasDeletion + hasSpecDeletion = hasSpecDeletion || fieldHasSpecDeletion + + // If we already detected a spec deletion, no need to look further, otherwise + // we should keep going to process remaining entries in the struct/map. + if hasSpecDeletion { + return hasDeletion, hasSpecDeletion + } + } + continue + } + + // Otherwise, there is a corresponding value in the current intent, then check for deletion recursively. + fieldHasDeletion, fieldHasSpecDeletion := isDroppingAnyIntent(&isDroppingAnyIntentInput{ + path: fieldPath, + previousIntent: previousIntentValue, + currentIntent: currentIntentValue, + currentObject: currentObjectValue, + schema: in.schema, + }) + hasDeletion = hasDeletion || fieldHasDeletion + hasSpecDeletion = hasSpecDeletion || fieldHasSpecDeletion + + // If we already detected a spec deletion, no need to look further, otherwise + // we should keep going to process remaining entries in the struct/map. + if hasSpecDeletion { + return hasDeletion, hasSpecDeletion + } + } + + return hasDeletion, hasSpecDeletion + } + + // NOTE: struct of Type atomic are treated below. + } + + if typeDef.List != nil { + // Get the corresponding list in the current object. + // NOTE: The conversion error is ignored because the map could have been already removed from the current object. + currentObjectList, _ := in.currentObject.([]interface{}) + + // Get the corresponding list in the current intent; if there is no corresponding list, + // the list has been deleted from the current intent. + currentIntentList, ok := in.currentIntent.([]interface{}) + if !ok { + // NOTE: this condition should never happen, because entire list deletion is caught when processing fields in a map, and the list is always a field in a map. + // However, for extra caution, it is handled nevertheless. + + // If the list still is in the current object, then there is a deletion the topology controller should reconcile. + if currentObjectList != nil { + return pathToResult(in.path) + } + return hasDeletion, hasSpecDeletion + } + + // Get the corresponding value in the current intent; if there is no corresponding value, + previousIntentList, ok := in.previousIntent.([]interface{}) + if !ok { + // NOTE: this condition should never happen because this func is called while walking through values in the previous intent. + // However, for extra caution, it is handled nevertheless. + return hasDeletion, hasSpecDeletion + } + + if typeDef.List.Type == ListMapType { + // If it is ListMap, we know that the list includes only map items and those items must have a set of unique listMapKeys values. + // Accordingly, we detect deletions by looking for items (maps identified by a listMapKeys values) present in the previous intent but not in the current intent. + // e.g. previous intent [{"id": "1", ...}, {"id": "2", ...}], current intent [{"id": "1", ...}], item with key "id": "2" has been deleted. + // If instead an item exists both in previousIntent and current intent, we look for deletions recursively inside the item. + + // Check if items in the previous intent has been removed from the current intent. + for _, previousIntentItem := range previousIntentList { + // Get the corresponding item from the current object, which is the item with the same ListMap keys values. + currentObjectItem := typeDef.List.GetSameItem(currentObjectList, previousIntentItem) + + // Get the corresponding item from the current intent, which is the item with the same ListMap keys values. + currentIntentItem := typeDef.List.GetSameItem(currentIntentList, previousIntentItem) + + // If there is no corresponding value in the current intent, the item has been deleted. + if currentIntentItem == nil { + // If the list item still is in the current object, then there is a deletion the topology controller should reconcile, + // otherwise we can continue processing remaining items. + if currentObjectItem != nil { + return pathToResult(in.path) + } + continue + } + + // Otherwise, if there is a corresponding item, check for deletion recursively. + if itemHasDeletion, itemHasSpecDeletion := isDroppingAnyIntent(&isDroppingAnyIntentInput{ + path: in.path.Item(), + previousIntent: previousIntentItem, + currentIntent: currentIntentItem, + currentObject: currentObjectItem, + schema: in.schema, + }); itemHasDeletion || itemHasSpecDeletion { + return itemHasDeletion, itemHasSpecDeletion + } + } + + return hasDeletion, hasSpecDeletion + } + + if typeDef.List.Type == ListSetType { + // If it is ListSet, we know that the list includes only scalar items and those items must be unique. + // Accordingly, we detect deletions by looking for items (scalar values) present in the previous intent but not in the current intent. + // e.g. previous intent ["a", "b"], current intent ["a"], item "b" has been deleted. + + // Get the corresponding set of list items from the current object. + currentObjectSet := sets.NewString() + for _, currentObjectItem := range currentObjectList { + currentObjectSet.Insert(fmt.Sprintf("%s", currentObjectItem)) + } + + // Get the corresponding set of list items from the current intent. + currentIntentSet := sets.NewString() + for _, currentIntentItem := range currentIntentList { + currentIntentSet.Insert(fmt.Sprintf("%s", currentIntentItem)) + } + + // Check if items in the previous intent has been removed from the current intent. + for _, previousIntentItem := range previousIntentList { + // if there is no corresponding item in the current intent, the item has been deleted. + if !currentIntentSet.Has(fmt.Sprintf("%s", previousIntentItem)) { + // If the item still is in the current object, then there is a deletion the topology controller should reconcile, + // otherwise we can continue processing the remaining items. + if currentObjectSet.Has(fmt.Sprintf("%s", previousIntentItem)) { + return pathToResult(in.path) + } + continue + } + } + + return hasDeletion, hasSpecDeletion + } + + // NOTE: lists of Type atomic are treated below. + } + + // Otherwise we consider the field a Scalar or an atomic value. + + // If there is no corresponding field in the current intent, and it still is in the current object, + // then there is a deletion the topology controller should reconcile. + if in.currentIntent == nil && in.currentObject != nil { + return pathToResult(in.path) + } + + return hasDeletion, hasSpecDeletion +} + +// isDroppingAnyIntentInput is the input of the isDroppingAnyIntent func. +type isDroppingAnyIntentInput struct { + // the path of the field being processed. + path contract.Path + + // the previous intent, that is the values the topology controller enforced at the previous reconcile. + previousIntent interface{} + // the current intent, that is the values the topology controller is enforcing at this reconcile. + currentIntent interface{} + + // the current object read at the beginning of the reconcile process. + currentObject interface{} + + schema CRDSchema +} + +// 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") +} diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/diff_test.go b/internal/controllers/topology/cluster/structuredmerge/diff/diff_test.go new file mode 100644 index 000000000000..f4ef623953f4 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/diff_test.go @@ -0,0 +1,1996 @@ +/* +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 diff + +import ( + "testing" + + . "github.com/onsi/gomega" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +var testSchema = CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, // Root object schema + "metadata": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + + "spec.struct": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.struct.foo": TypeDef{Scalar: &ScalarDef{}}, + "spec.struct.bar": TypeDef{Scalar: &ScalarDef{}}, + "spec.struct.baz": TypeDef{Scalar: &ScalarDef{}}, + "spec.atomicStruct": TypeDef{Struct: &StructDef{Type: AtomicStructType}}, + + "metadata.labels": TypeDef{Map: &MapDef{Type: GranularMapType}}, // mapOfScalars + "metadata.labels[]": TypeDef{Scalar: &ScalarDef{}}, + "spec.mapOfStruct": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.mapOfStruct[]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.mapOfStruct[].foo": TypeDef{Scalar: &ScalarDef{}}, + "spec.mapOfMap": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.mapOfMap[]": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.mapOfMap[][]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.mapOfMap[][].foo": TypeDef{Scalar: &ScalarDef{}}, + "spec.mapOfList": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.mapOfList[]": TypeDef{List: &ListDef{Type: AtomicListType}}, + "spec.atomicMap": TypeDef{Map: &MapDef{Type: AtomicMapType}}, + + "spec.listMap": TypeDef{List: &ListDef{Type: ListMapType, Keys: []string{"id1", "id2"}}}, + "spec.listMap[]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, // List item schema + "spec.listMap[].id1": TypeDef{Scalar: &ScalarDef{}}, + "spec.listMap[].id2": TypeDef{Scalar: &ScalarDef{}}, + "spec.listMap[].foo": TypeDef{Scalar: &ScalarDef{}}, + "spec.listMap[].bar": TypeDef{Scalar: &ScalarDef{}}, + "spec.listMap[].baz": TypeDef{Scalar: &ScalarDef{}}, + "spec.listSet": TypeDef{List: &ListDef{Type: ListSetType}}, + "spec.atomicList": TypeDef{List: &ListDef{Type: AtomicListType}}, +} + +// Test_isChangingAnything_basic checks if isChangingAnything handles basic combination of JSONSchemaProps that can be found in a CRD definition. +func Test_isChangingAnything(t *testing.T) { + tests := []struct { + name string + ctx *isChangingAnythingInput + wantHasChanges bool + wantHasSpecChanges bool + }{ + // Test struct & struct variants (atomic struct) + + { + name: "No changes in struct", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", // field existing only in current object should not trigger change. + }, + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + }, + { + name: "Struct field changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo-changed", // foo has been changed in the current intent. + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "Struct field changed in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo-changed", // foo has been changed in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "Field does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + // foo has been deleted from current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicStruct changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicStruct changed in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", // bar has been added in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "(Atomic) Struct does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + + // Test map & map variants (map of scalar, map of struct, map of map, map of list, atomic map) + + { + name: "No map changes", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", + "bar": "bar", // items existing only in current object should not trigger change. + }, + }, + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + "bar": map[string]interface{}{ // items existing only in current object should not trigger change. + "foo": "bar", + }, + }, + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "bar": map[string]interface{}{ // items existing only in current object should not trigger change. + "foo": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + "bar": []interface{}{ // items existing only in current object should not trigger change. + "a", + }, + }, + "atomicMap": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", + }, + }, + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + "atomicMap": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + }, + { + name: "item in map of scalar item/metadata only changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo-changed", // foo has been changed in the current intent. + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + }, + { + name: "item in map of scalar item/metadata only added in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + "bar": "bar", // bar has been added in the current intent. + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + }, + { + name: "item in map of scalar item/metadata only changed in the object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo-changed", // foo has been changed in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + }, + { + name: "item in map of struct changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo-changed", // foo has been changed in the current intent. + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of struct added in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + "bar": map[string]interface{}{ // bar has been added in the current intent. + "foo": "bar", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of struct changed in the object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo-changed", // foo has been changed in the current object. + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of maps changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ // foo has been changed in the current intent. + "foo": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of maps added in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "bar": map[string]interface{}{ // bar has been added in the current intent. + "foo": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of maps changed in the object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ // foo has been changed in the current object. + "foo": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of list changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ // foo has been changed in the current intent. + "a-changed", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of list added in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + "bar": []interface{}{ // bar has been changed in the current intent. + "a", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "item in map of list changed in the object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ // foo has been changed in the current object. + "a-changed", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicMap changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicMap changed in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "(Atomic) Map does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + + // Test list & list variants (atomic list, list set, list map) + + { + name: "No list changes", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + "bar": "bar", // field existing only in current object should not trigger change. + }, + map[string]interface{}{ // item existing only in current object should not trigger change. + "id1": "B1", + "id2": "B2", + "foo": "foo", + }, + }, + "listSet": []interface{}{ + "a", + "b", + "c", // item existing only in current object should not trigger change. + }, + "atomicList": []interface{}{ + "a", + "b", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + "listSet": []interface{}{ + "a", + "b", + }, + "atomicList": []interface{}{ + "a", + "b", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + }, + + { + name: "List map item changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo-changed", // foo has been changed in the current intent. + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "List map item changed in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo-changed", // foo has been changed in the current intent. + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "List map item does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + // item with key id1:A1, id2:A2 does not exist in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "List set item does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + // item "b" does not exist in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + "b", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicList changed in the intent", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + "b", // b added in the current intent. + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "AtomicList changed in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + "b", // b added in the current object. + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + { + name: "(Atomic) List does not exists in the current object", + ctx: &isChangingAnythingInput{ + path: contract.Path{}, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + // atomic list does not exist in the current object. + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + schema: testSchema, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + gotHasChanges, gotHasSpecChanges := isChangingAnything(tt.ctx) + + g.Expect(gotHasChanges).To(Equal(tt.wantHasChanges)) + g.Expect(gotHasSpecChanges).To(Equal(tt.wantHasSpecChanges)) + }) + } +} + +// Test_isDroppingAnyIntent_basic checks if isDroppingAnyIntent handles basic combination of JSONSchemaProps that can be found in a CRD definition. +func Test_isDroppingAnyIntent_basic(t *testing.T) { + tests := []struct { + name string + ctx *isDroppingAnyIntentInput + wantHasDeletion bool + wantHasSpecDeletion bool + }{ + // Test struct & struct variants (atomic struct) + + { + name: "No deletion in struct", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + "atomicStruct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + "bar": "bar-changed", // field changed should not be detected as a deletion. + "baz": "baz", // field added should not be detected as a deletion. + }, + "atomicStruct": map[string]interface{}{ + // field deleted should not be detected as a deletion + "bar": "bar-changed", // field changed should not be detected as a deletion. + "baz": "baz", // field added should not be detected as a deletion. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + "atomicStruct": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects field deletion in struct", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + // foo has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore field deletion in struct if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "struct": map[string]interface{}{ + "foo": "foo", + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + // foo has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "struct": map[string]interface{}{ + // The field has been already deleted from the current object. + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects (atomic) struct deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // struct has been deleted. + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore (atomic) struct deletion if the struct has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicStruct": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // struct has been deleted. + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + // The struct has been already deleted from the current object. + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + + // Test map & map variants (map of scalar, map of struct, map of map, map of list, atomic map) + + { + name: "No deletion in map", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + "bar": "bar", + }, + }, + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + "bar": map[string]interface{}{ + "foo": "bar", + }, + }, + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "bar": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + "bar": []interface{}{ + "a", + }, + }, + "atomicMap": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + "bar": "bar-changed", // field changed should not be detected as a deletion. + "baz": "baz", // field added should not be detected as a deletion. + }, + }, + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + "bar": map[string]interface{}{ // item changed should not be detected as a deletion. + "foo": "bar-changed", + }, + "baz": map[string]interface{}{ // item added should not be detected as a deletion. + "foo": "foo", + }, + }, + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "bar": map[string]interface{}{ // item changed should not be detected as a deletion. + "foo": map[string]interface{}{ + "foo": "bar-changed", + }, + }, + "baz": map[string]interface{}{ // item added should not be detected as a deletion. + "foo": map[string]interface{}{ + "foo": "baz", + }, + }, + }, + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + "bar": []interface{}{ // item changed should not be detected as a deletion. + "a-changed", + }, + "baz": []interface{}{ // item added should not be detected as a deletion. + "a", + }, + }, + "atomicMap": map[string]interface{}{ + // item deleted should not be detected as a deletion. + "bar": "bar-changed", // item changed should not be detected as a deletion. + "baz": "baz", // item added should not be detected as a deletion. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + "bar": "bar", + }, + }, + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + "bar": map[string]interface{}{ + "foo": "bar", + }, + }, + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + "bar": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + "bar": []interface{}{ + "a", + }, + }, + "atomicMap": map[string]interface{}{ + "foo": "foo", + "bar": "bar", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detect map of scalar item/metadata only deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: false, + }, + { + name: "Ignore map of scalar item/metadata only deletion if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ // mapOfScalars + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: false, + }, + { + name: "Detect map of struct deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore map of struct deletion if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfStruct": map[string]interface{}{ + // foo item has been already removed from current object + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detect map of map deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore map of map deletion if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfMap": map[string]interface{}{ + // foo item has been already removed from current object + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detect map of list deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore map of map list if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + "foo": []interface{}{ + "a", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + // foo item has been removed + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "mapOfList": map[string]interface{}{ + // foo item has been already removed from current object + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects (atomic) map deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // map has been deleted. + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore (atomic) map deletion if the map has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicMap": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // map has been deleted. + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + // The struct map been already deleted from the current object. + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + + // Test list & list variants (atomic list, list set, list map) + + { + name: "No deletion in lists", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + "bar": "bar", + }, + }, + "listSet": []interface{}{ + "a", + "b", + }, + "atomicList": []interface{}{ + "a", + "b", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + "bar": "bar-changed", // field changed should not be detected as a deletion. + "baz": "baz", // field added should not be detected as a deletion. + }, + map[string]interface{}{ // item added should not be detected as a deletion. + "id1": "B1", + "id2": "B2", + "foo": "foo", + }, + }, + "listSet": []interface{}{ + "a", + "b", + "c", // item added should not be detected as a deletion. + }, + "atomicList": []interface{}{ + // item deleted should not be detected as a deletion + "b", + "c", // item added should not be detected as a deletion. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + "bar": "bar", + }, + }, + "listSet": []interface{}{ + "a", + "b", + }, + "atomicList": []interface{}{ + "a", + "b", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects list map item deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + // item with key A1,A2 has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore list map item deletion if the list map item has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + // item with key A1,A2 has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + // The item with key A1,A2 has been already deleted from the current object + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects field deletion inside list map item", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + // field foo in the item with key A1,A2 has been deleted. + }, + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore field deletion inside list map item if the field has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + "foo": "foo", + }, + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + // field foo in the item with key A1,A2 has been deleted. + }, + }, + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + "listMap": []interface{}{ + map[string]interface{}{ + "id1": "A1", + "id2": "A2", + // field foo in the item with key A1,A2 has been already deleted. + }, + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects list set item deleted", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + "b", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + // item b has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + "b", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore list set item deletion if the item has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + "b", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + // item b has been deleted. + }, + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "listSet": []interface{}{ + "a", + // item b has been already deleted. + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + { + name: "Detects (atomic) list deletion", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // listMap has been deleted. + }, + }, + currentObject: map[string]interface{}{ + // NOTE: It is required to keep the current object intentionally in sync with the previous intent (see note above). + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + schema: testSchema, + }, + wantHasDeletion: true, + wantHasSpecDeletion: true, + }, + { + name: "Ignore (atomic) list deletion if the list has been already deleted from the current object", + ctx: &isDroppingAnyIntentInput{ + path: contract.Path{}, + previousIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + "atomicList": []interface{}{ + "a", + }, + }, + }, + currentIntent: map[string]interface{}{ + "spec": map[string]interface{}{ + // listMap has been deleted. + }, + }, + currentObject: map[string]interface{}{ + "spec": map[string]interface{}{ + // The listMap has been already deleted from the current object. + }, + }, + schema: testSchema, + }, + wantHasDeletion: false, + wantHasSpecDeletion: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + gotHasDeletion, gotHasSpecDeletion := isDroppingAnyIntent(tt.ctx) + + g.Expect(gotHasDeletion).To(Equal(tt.wantHasDeletion)) + g.Expect(gotHasSpecDeletion).To(Equal(tt.wantHasSpecDeletion)) + }) + } +} diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/doc.go b/internal/controllers/topology/cluster/structuredmerge/diff/doc.go new file mode 100644 index 000000000000..98104a5e2042 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/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 diff implements dryRunDiff logic for server side apply used by the managed topology controllers. +package diff diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/schema.go b/internal/controllers/topology/cluster/structuredmerge/diff/schema.go new file mode 100644 index 000000000000..f0af3eaf00a1 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/schema.go @@ -0,0 +1,133 @@ +/* +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 diff + +import ( + "reflect" +) + +// CRDSchema provides a simplified view of a CRD's Open API schema. +// Schema is a represented as map of field path, TypeDef. +type CRDSchema map[string]TypeDef + +// TypeDef represents a named type in a schema. +// TypeDef contains only the info required for diff processing. +type TypeDef struct { + Scalar *ScalarDef + List *ListDef + Map *MapDef + Struct *StructDef +} + +// ScalarDef represent a scalar type. +type ScalarDef struct { +} + +// MapType enumerate the type if map. +type MapType string + +const ( + // GranularMapType defines a map where each item should be processed. + GranularMapType = MapType("granular") + + // AtomicMapType defines a map where all the items should be processed at once. + AtomicMapType = MapType("atomic") +) + +// MapDef represent a map type. +type MapDef struct { + Type MapType +} + +// StructType enumerate the type if struct. +type StructType string + +const ( + // GranularStructType defines a map where each field should be processed. + GranularStructType = StructType("granular") + + // AtomicStructType defines a map where all the field should be processed at once. + AtomicStructType = StructType("atomic") +) + +// StructDef represent a struct type. +type StructDef struct { + Type StructType +} + +// ListType enumerate the type if lists. +type ListType string + +const ( + // AtomicListType defines a list where all the items should be processed at once. + AtomicListType = ListType("atomic") + + // ListMapType defines a list where each item should be processed according to ListMapKeys. + ListMapType = ListType("map") + + // ListSetType defines a list where each item should be processed like in a set of scalar values. + ListSetType = ListType("set") +) + +// ListDef represent a list type. +type ListDef struct { + Type ListType + Keys []string +} + +// GetSameItem returns the element with the same keys of items from the given list. +func (ld *ListDef) GetSameItem(list []interface{}, item interface{}) map[string]interface{} { + keys := ld.GetItemKeys(item) + if keys == nil { + return nil + } + + return ld.GetItemWithKeys(list, keys) +} + +// GetItemKeys return the keys for a given list item. +func (ld *ListDef) GetItemKeys(item interface{}) map[string]interface{} { + itemMap, ok := item.(map[string]interface{}) + if !ok { + return nil + } + + keys := make(map[string]interface{}) + for _, k := range ld.Keys { + itemValue, ok := itemMap[k] + if !ok { + return nil + } + keys[k] = itemValue + } + return keys +} + +// GetItemWithKeys returns the element with the given keys/values from the list. +func (ld *ListDef) GetItemWithKeys(list []interface{}, keys map[string]interface{}) map[string]interface{} { + for _, item := range list { + itemMap, ok := item.(map[string]interface{}) + if !ok { + continue + } + + if reflect.DeepEqual(ld.GetItemKeys(itemMap), keys) { + return itemMap + } + } + return nil +} diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache.go b/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache.go new file mode 100644 index 000000000000..c1c0c48dd364 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache.go @@ -0,0 +1,215 @@ +/* +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 diff + +import ( + "fmt" + "strings" + "sync" + + "github.com/gobuffalo/flect" + "github.com/pkg/errors" + "golang.org/x/net/context" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +// CRDSchemaCache caches CRD's Open API schemas. +type CRDSchemaCache interface { + // LoadOrStore returns the existing value for the gvk if present. Otherwise, it stores and returns the given value. + LoadOrStore(ctx context.Context, gvk schema.GroupVersionKind) (CRDSchema, error) +} + +// NewCRDSchemaCache returns a schema caches implementation that reads schemas from the Open API schema defined in the CRDs. +func NewCRDSchemaCache(c client.Client) CRDSchemaCache { + return &crdSchemaCache{ + client: c, + m: map[schema.GroupVersionKind]CRDSchema{}, + } +} + +var _ CRDSchemaCache = &crdSchemaCache{} + +type crdSchemaCache struct { + client client.Client + + lock sync.RWMutex + m map[schema.GroupVersionKind]CRDSchema +} + +// LoadOrStore returns the CRDSchema value for the gvk, if present. +// Otherwise, if not present, it retrieves the corresponding CRD definition, infer the CRDSchema, stores it, returns it. +func (sc *crdSchemaCache) LoadOrStore(ctx context.Context, gvk schema.GroupVersionKind) (CRDSchema, error) { + if s, ok := sc.load(gvk); ok { + return s, nil + } + return sc.store(ctx, gvk) +} + +// load returns the CRDSchema value for the gvk, if present. +func (sc *crdSchemaCache) load(gvk schema.GroupVersionKind) (CRDSchema, bool) { + sc.lock.RLock() + defer sc.lock.RUnlock() + + s, ok := sc.m[gvk] + return s, ok +} + +// store retrieves the CRD definition for the gvk, infer the corresponding CRDSchema, stores it, returns it. +func (sc *crdSchemaCache) store(ctx context.Context, gvk schema.GroupVersionKind) (CRDSchema, error) { + sc.lock.Lock() + defer sc.lock.Unlock() + + // check if the CRDSchema has been added while waiting for the lock. if yes, return it. + if s, ok := sc.m[gvk]; ok { + return s, nil + } + + crd := &apiextensionsv1.CustomResourceDefinition{} + crd.SetName(fmt.Sprintf("%s.%s", flect.Pluralize(strings.ToLower(gvk.Kind)), gvk.Group)) + if err := sc.client.Get(ctx, client.ObjectKeyFromObject(crd), crd); err != nil { + return nil, errors.Wrapf(err, "failed to get CRD for %s", gvk.String()) + } + + schema := CRDSchema{} + for _, v := range crd.Spec.Versions { + if v.Name == gvk.Version && v.Schema != nil && v.Schema.OpenAPIV3Schema != nil { + addToSchema(contract.Path{}, schema, v.Schema.OpenAPIV3Schema) + } + } + + sc.m[gvk] = schema + return schema, nil +} + +// addToSchema converts JSONSchemaProps to a CRDSchema. +func addToSchema(p contract.Path, s CRDSchema, c *apiextensionsv1.JSONSchemaProps) { + if p.String() == "status" { + return + } + + switch c.Type { + case "object": + // "object" apply to both fields defined as `foo SomeStruct` (struct) or `foo map[string]Something` (maps). + + // If it is field defined as `foo SomeStruct` (struct), Properties are defined for each nested field. + if c.Properties != nil { + // NOTE: XMapType is used also for struct. + st := StructType(pointer.StringDeref(c.XMapType, string(GranularStructType))) // NOTE: by default struct are granular in CRDs. + s[p.String()] = TypeDef{ + Struct: &StructDef{Type: st}, + } + + // If the struct is defined as atomic, it is not required to process the property's schema given that the entire struct should be treated as an atomic element. + if st == AtomicStructType { + return + } + + // Process the property's schema. + for field, fieldSchema := range c.Properties { + fieldSchema := fieldSchema + addToSchema(p.Append(field), s, &fieldSchema) + } + } + + // If it is a field defined as `foo map[string]Something`, AdditionalProperties define the struct of the items. + if c.AdditionalProperties != nil { + mt := MapType(pointer.StringDeref(c.XMapType, string(GranularMapType))) // NOTE: by default map are granular in CRDs. + s[p.String()] = TypeDef{ + Map: &MapDef{Type: mt}, + } + // If the map is defined as atomic, it is not required to process the item's schema given that each item should be treated as atomic elements. + if mt == AtomicMapType { + return + } + + // Process the item's schema. + addToSchema(p.Item(), s, c.AdditionalProperties.Schema) + } + + case "array": + // "array" apply to all the fields defined as `foo []Something`; how to process such + // lists depends on +listType and +listMapKey markers. + + // Store the list definition. + lt := ListType(pointer.StringDeref(c.XListType, string(AtomicListType))) // NOTE: by default list are atomic in CRDs. + lk := c.XListMapKeys + s[p.String()] = TypeDef{ + List: &ListDef{ + Type: lt, + Keys: lk, + }, + } + + // Process item definition, if required. + switch lt { + case ListMapType: + // Add an entry defining the list item; by design, it is as a struct. + if c.Items != nil && c.Items.Schema != nil { + item := p.Item() + s[item.String()] = TypeDef{ + Struct: &StructDef{ + Type: GranularStructType, + }, + } + for field, fieldSchema := range c.Items.Schema.Properties { + fieldSchema := fieldSchema + addToSchema(item.Append(field), s, &fieldSchema) + } + } + case ListSetType: + // It is not required to process the item's schema given that each item of the list set are scalars. + default: // AtomicListType + // It is not required to process the item's schema given that items should be treated as atomic elements. + } + default: + // Otherwise, the schema applies to scalar fields, e.g. `foo string`, `foo bool`, + s[p.String()] = TypeDef{ + Scalar: &ScalarDef{}, + } + } + + // Add to level fields which are never part of a CRD's OpenAPI schema. + if p.String() == "" { + s["apiVersion"] = TypeDef{Scalar: &ScalarDef{}} + s["kind"] = TypeDef{Scalar: &ScalarDef{}} + } + // Expand metadata schema which are store only partially in CRD's OpenAPI schema. + // Ref https://github.com/kubernetes-sigs/controller-tools/blob/59485af1c1f6a664655dad49543c474bb4a0d2a2/pkg/crd/gen.go#L185 + if p.String() == "metadata" { + // NOTE: we add only fields which are relevant for the topology controller. + s["metadata"] = TypeDef{Struct: &StructDef{Type: GranularStructType}} + s["metadata.name"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.namespace"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.labels"] = TypeDef{Map: &MapDef{Type: GranularMapType}} + s["metadata.labels[]"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.annotations"] = TypeDef{Map: &MapDef{Type: GranularMapType}} + s["metadata.annotations[]"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences"] = TypeDef{List: &ListDef{Type: ListMapType, Keys: []string{"uid"}}} + s["metadata.ownerReferences[]"] = TypeDef{Struct: &StructDef{Type: GranularStructType}} + s["metadata.ownerReferences[].apiVersion"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences[].blockOwnerDeletion"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences[].controller"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences[].kind"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences[].name"] = TypeDef{Scalar: &ScalarDef{}} + s["metadata.ownerReferences[].uid"] = TypeDef{Scalar: &ScalarDef{}} + } +} diff --git a/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache_test.go b/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache_test.go new file mode 100644 index 000000000000..1a48a38c7e20 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/diff/schema_cache_test.go @@ -0,0 +1,382 @@ +/* +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 diff + +import ( + "testing" + + . "github.com/onsi/gomega" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/utils/pointer" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +// Test_addToSchema_basic test add to schema handles basic combination of JSONSchemaProps that can be found in a CRD definition. +// TODO: generate CRD definition from go types and load them, so we cover the generation process vs making assumption on the generated JSONSchemaProps. +func Test_addToSchema_basic(t *testing.T) { + tests := []struct { + name string + crd *apiextensionsv1.JSONSchemaProps + wantSchema CRDSchema + }{ + // Test struct & struct variants (atomic struct) + + { + name: "Adds struct, scalar and nested structs", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "apiVersion": {Type: "string"}, + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + "fooStruct": { + Type: "object", + // Struct are granular by default + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooScalar": TypeDef{Scalar: &ScalarDef{}}, + "spec.fooStruct": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooStruct.fooScalar": TypeDef{Scalar: &ScalarDef{}}, + }, + }, + { + name: "Adds atomic struct", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooAtomicStruct": { + Type: "object", + XMapType: pointer.StringPtr("atomic"), // XMapType applies to struct as well. + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooAtomicStruct": TypeDef{Struct: &StructDef{Type: AtomicStructType}}, + // no further detail are processed for fooAtomicStruct. + }, + }, + + // Test map & map variants (map of scalar, map of struct, map of map, map of list, atomic map) + + { + name: "Adds map with scalar items", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooMap": { + Type: "object", + // Map are granular by default + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.fooMap[]": TypeDef{Scalar: &ScalarDef{}}, + }, + }, + { + name: "Adds map with struct items", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooMap": { + Type: "object", + // Map are granular by default + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + }, + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.fooMap[]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap[].fooScalar": TypeDef{Scalar: &ScalarDef{}}, + }, + }, + { + name: "Adds map with map items", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooMap": { + Type: "object", + // Map are granular by default + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.fooMap[]": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.fooMap[][]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap[][].fooScalar": TypeDef{Scalar: &ScalarDef{}}, + }, + }, + { + name: "Adds map with list items", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooMap": { + Type: "object", + // Map are granular by default + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "array", + Items: &apiextensionsv1.JSONSchemaPropsOrArray{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooMap": TypeDef{Map: &MapDef{Type: GranularMapType}}, + "spec.fooMap[]": TypeDef{List: &ListDef{Type: AtomicListType}}, + // no further detail are processed for spec.fooMap[] (it is an atomic list). + }, + }, + { + name: "Adds atomic map", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooAtomicMap": { + Type: "object", + XMapType: pointer.StringPtr("atomic"), + AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooAtomicMap": TypeDef{Map: &MapDef{Type: AtomicMapType}}, + // no further detail are processed for fooAtomicMap. + }, + }, + + // Test list & list variants (atomic list, list set, list map) + + { + name: "Adds atomic list", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooAtomicList": { + Type: "array", + // List are atomic by default + Items: &apiextensionsv1.JSONSchemaPropsOrArray{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "bool", + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooAtomicList": TypeDef{List: &ListDef{Type: AtomicListType}}, + // no further detail are processed for fooAtomicList. + }, + }, + { + name: "Adds list set", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooListSet": { + Type: "array", + XListType: pointer.String("set"), + Items: &apiextensionsv1.JSONSchemaPropsOrArray{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooListSet": TypeDef{List: &ListDef{Type: ListSetType}}, + // no further detail are processed for fooListSet, they are scalar by default. + }, + }, + { + name: "Adds list map", + crd: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooListMap": { + Type: "array", + XListType: pointer.String("map"), + XListMapKeys: []string{"fooScalar"}, + Items: &apiextensionsv1.JSONSchemaPropsOrArray{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "fooScalar": {Type: "string"}, + "barScalar": {Type: "string"}, + }, + }, + }, + }, + }, + }, + }, + }, + wantSchema: CRDSchema{ + "": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "apiVersion": TypeDef{Scalar: &ScalarDef{}}, + "kind": TypeDef{Scalar: &ScalarDef{}}, + "spec": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooListMap": TypeDef{List: &ListDef{Type: ListMapType, Keys: []string{"fooScalar"}}}, + "spec.fooListMap[]": TypeDef{Struct: &StructDef{Type: GranularStructType}}, + "spec.fooListMap[].fooScalar": TypeDef{Scalar: &ScalarDef{}}, + "spec.fooListMap[].barScalar": TypeDef{Scalar: &ScalarDef{}}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + scheme := CRDSchema{} + addToSchema(contract.Path{}, scheme, tt.crd) + + g.Expect(scheme).To(Equal(tt.wantSchema)) + }) + } +} 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..f91e43767b73 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -28,6 +28,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/internal/contract" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge/diff" ) // TopologyManagerName is the manager name in managed fields for the topology controller. @@ -41,7 +42,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, crdSchemaCache diff.CRDSchemaCache, opts ...HelperOption) (PatchHelper, error) { helperOptions := &HelperOptions{} helperOptions = helperOptions.ApplyOptions(opts) helperOptions.allowedPaths = []contract.Path{ @@ -60,8 +61,15 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, // If required, convert the original and modified objects to unstructured and filter out all the information // not relevant for the topology controller. - var originalUnstructured *unstructured.Unstructured + var originalUnstructured, previousIntent *unstructured.Unstructured if !isNil(original) { + // Gets the previousIntent from the original object, to be used later for DryRunDiff. + var err error + previousIntent, err = diff.GetPreviousIntent(original) + if err != nil { + return nil, errors.Wrap(err, "failed to get last applied intent") + } + originalUnstructured = &unstructured.Unstructured{} switch original.(type) { case *unstructured.Unstructured: @@ -100,11 +108,20 @@ 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, + // Gets the schema for the modified object gvk. + // NOTE: this schema drives DryRunDiff operations; modified (current intent) and original (current object) + // are of the same gvk, given that we are always calling UpdateReferenceAPIContract when reading both of them. + // previousIntent instead could be of an older version, but this impacts dryRun only partially (see diff.isDroppingAnyIntent for more details) + schema, err := crdSchemaCache.LoadOrStore(ctx, modifiedUnstructured.GroupVersionKind()) + if err != nil { + return nil, errors.Wrapf(err, "failed to get schema for %s", modifiedUnstructured.GroupVersionKind().String()) + } + + hasChanges, hasSpecChanges = diff.DryRunDiff(&diff.DryRunDiffInput{ + PreviousIntent: previousIntent, + CurrentIntent: modifiedUnstructured, + CurrentObject: originalUnstructured, + Schema: schema, }) } @@ -135,6 +152,12 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error { log := ctrl.LoggerFrom(ctx) log.V(5).Info("Patching object", "Intent", h.modified) + // Stores che current intent as last applied intent. + // NOTE: we are doing this at this stage so it won't impact the dryRunDiff logic. + if err := diff.AddCurrentIntentAnnotation(h.modified); err != nil { + return errors.Wrap(err, "failed to add last applied intent annotation") + } + options := []client.PatchOption{ client.FieldOwner(TopologyManagerName), // NOTE: we are using force ownership so in case of conflicts the topology controller diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 38754108af98..801e07ccec99 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" @@ -25,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge/diff" "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/util/patch" ) @@ -35,13 +37,15 @@ func TestServerSideApply(t *testing.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()) + // ObjectMeta: metav1.ObjectMeta{Name: "test"}, + // }), 0777)).To(Succeed())0777)).To(Succeed()) // Create a namespace for running the test ns, err := env.CreateNamespace(ctx, "ssa") g.Expect(err).ToNot(HaveOccurred()) + schemaCache := diff.NewCRDSchemaCache(env.Client) + // Build the test object to work with. obj := builder.TestInfrastructureCluster(ns.Name, "obj1").WithSpecFields(map[string]interface{}{ "spec.controlPlaneEndpoint.host": "1.2.3.4", @@ -56,7 +60,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(), schemaCache) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -67,7 +71,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(), schemaCache) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -77,7 +81,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -102,6 +106,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(controlPlaneEndpointFieldV1).ToNot(BeEmpty()) g.Expect(controlPlaneEndpointFieldV1).To(HaveKey("f:host")) // topology controller should express opinions on spec.controlPlaneEndpoint.host. g.Expect(controlPlaneEndpointFieldV1).To(HaveKey("f:port")) // topology controller should express opinions on spec.controlPlaneEndpoint.port. + + g.Expect(got.GetAnnotations()).To(HaveKey(clusterv1.ClusterTopologyLastAppliedIntentAnnotation)) }) t.Run("Server side apply patch helper detects no changes", func(t *testing.T) { @@ -113,7 +119,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -130,7 +136,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -147,7 +153,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -164,7 +170,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -181,7 +187,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -201,10 +207,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -221,7 +228,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -256,17 +263,18 @@ 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(), schemaCache, 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 (should be a no-op). 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()) + g.Expect(original.GetResourceVersion()).To(Equal(got.GetResourceVersion())) v1, _, _ := unstructured.NestedString(got.Object, "spec", "foo") g.Expect(v1).To(Equal("changed")) @@ -287,6 +295,8 @@ func TestServerSideApply(t *testing.T) { 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).ToNot(HaveKey("f:bar")) // topology controller should not express opinions on fields managed by other controllers. + + g.Expect(got.GetAnnotations()).To(HaveKey(clusterv1.ClusterTopologyLastAppliedIntentAnnotation)) }) t.Run("Topology controller reconcile again with some changes on topology managed fields", func(t *testing.T) { @@ -300,7 +310,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -311,6 +321,7 @@ func TestServerSideApply(t *testing.T) { // Check the object and verify the change is applied as well as the fields set by the other controller are still preserved. got := obj.DeepCopy() g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + g.Expect(original.GetResourceVersion()).ToNot(Equal(got.GetResourceVersion())) v0, _, _ := unstructured.NestedString(got.Object, "spec", "controlPlaneEndpoint", "host") g.Expect(v0).To(Equal("changed")) @@ -322,6 +333,8 @@ func TestServerSideApply(t *testing.T) { g.Expect(v3).To(Equal("changed")) v4, _, _ := unstructured.NestedBool(got.Object, "status", "ready") g.Expect(v4).To(Equal(true)) + + g.Expect(got.GetAnnotations()).To(HaveKey(clusterv1.ClusterTopologyLastAppliedIntentAnnotation)) }) t.Run("Topology controller reconcile again with an opinion on a field managed by another controller (force ownership)", func(t *testing.T) { @@ -336,7 +349,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(), schemaCache, IgnorePaths{{"spec", "foo"}}) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) @@ -347,6 +360,7 @@ func TestServerSideApply(t *testing.T) { // 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()) + g.Expect(original.GetResourceVersion()).ToNot(Equal(got.GetResourceVersion())) v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") g.Expect(v2).To(Equal("changed-by-topology-controller")) @@ -360,6 +374,8 @@ func TestServerSideApply(t *testing.T) { 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). + + g.Expect(got.GetAnnotations()).To(HaveKey(clusterv1.ClusterTopologyLastAppliedIntentAnnotation)) }) t.Run("No-op on unstructured object having empty map[string]interface in spec", func(t *testing.T) { g := NewWithT(t) @@ -381,7 +397,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(), schemaCache) g.Expect(err).ToNot(HaveOccurred()) g.Expect(p0.HasChanges()).To(BeFalse()) g.Expect(p0.HasSpecChanges()).To(BeFalse()) @@ -400,6 +416,8 @@ func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { ns, err := env.CreateNamespace(ctx, "ssa") g.Expect(err).ToNot(HaveOccurred()) + schemaCache := diff.NewCRDSchemaCache(env.Client) + // Build the test object to work with. obj := builder.TestInfrastructureCluster(ns.Name, "obj1").WithSpecFields(map[string]interface{}{ "spec.foo": "", @@ -417,7 +435,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(), schemaCache) g.Expect(err).ToNot(HaveOccurred()) // Get created object after cleanup @@ -444,3 +462,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 +}