From 331a8a197aae8722f74dea4f59c73042358c0008 Mon Sep 17 00:00:00 2001 From: Amim Knabben Date: Wed, 4 Jan 2023 12:12:40 -0300 Subject: [PATCH] Drop ClusterTopologyManagedFieldsAnnotation field from v1beta1 --- .golangci.yml | 2 +- api/v1beta1/common_types.go | 16 ----- .../src/developer/providers/v1.3-to-v1.4.md | 7 ++- .../structuredmerge/serversidepathhelper.go | 62 ------------------- .../serversidepathhelper_test.go | 51 --------------- 5 files changed, 5 insertions(+), 133 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6407fffa069e..986fe81eb870 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -222,7 +222,7 @@ issues: # Specific exclude rules for deprecated fields that are still part of the codebase. These should be removed as the referenced deprecated item is removed from the project. - linters: - staticcheck - text: "SA1019: (bootstrapv1.ClusterStatus|clusterv1.ClusterTopologyManagedFieldsAnnotation|scope.Config.Spec.UseExperimentalRetryJoin|DockerMachine.Spec.Bootstrapped|machineStatus.Bootstrapped) is deprecated" + text: "SA1019: (bootstrapv1.ClusterStatus|scope.Config.Spec.UseExperimentalRetryJoin|DockerMachine.Spec.Bootstrapped|machineStatus.Bootstrapped) is deprecated" - linters: - revive text: "exported: exported method .*\\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported" diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index 421e444692fe..c355d9d3b9e1 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -29,22 +29,6 @@ const ( // ClusterTopologyOwnedLabel is the label set on all the object which are managed as part of a ClusterTopology. ClusterTopologyOwnedLabel = "topology.cluster.x-k8s.io/owned" - // ClusterTopologyManagedFieldsAnnotation is the annotation used to store the list of paths managed - // by the topology controller; changes to those paths will be considered authoritative. - // NOTE: Managed field depends on the last reconciliation of a managed object; this list can - // change during the lifecycle of an object, depending on how the corresponding template + patch/variable - // changes over time. - // NOTE: The topology controller is only concerned about managed paths in the spec; given that - // we are dropping spec. from the result to reduce verbosity of the generated annotation. - // NOTE: Managed paths are relevant only for unstructured objects where it is not possible - // to easily discover which fields have been set by templates + patches/variables at a given reconcile; - // instead, it is not necessary to store managed paths for typed objets (e.g. Cluster, MachineDeployments) - // given that the topology controller explicitly sets a well-known, immutable list of fields at every reconcile. - // - // Deprecated: Topology controller is now using server side apply and this annotation will be removed in a future release. - // When removing also remove from staticcheck exclude-rules for SA1019 in golangci.yml. - ClusterTopologyManagedFieldsAnnotation = "topology.cluster.x-k8s.io/managed-field-paths" - // ClusterTopologyMachineDeploymentNameLabel is the label set on the generated MachineDeployment objects // to track the name of the MachineDeployment topology it represents. ClusterTopologyMachineDeploymentNameLabel = "topology.cluster.x-k8s.io/deployment-name" diff --git a/docs/book/src/developer/providers/v1.3-to-v1.4.md b/docs/book/src/developer/providers/v1.3-to-v1.4.md index 48970c561b6d..fd19f69e2389 100644 --- a/docs/book/src/developer/providers/v1.3-to-v1.4.md +++ b/docs/book/src/developer/providers/v1.3-to-v1.4.md @@ -21,11 +21,12 @@ maintainers of providers and consumers of our Go API. ### Removals - `clusterctl backup` has been removed. -- `MachineHealthCheckSuccededCondition` condition type has been removed. -- `CloneTemplate` and `CloneTemplateInput` has been removed. -- The option `--list-images` from `init` subcommand has been removed. +- `api/v1beta1.MachineHealthCheckSuccededCondition` condition type has been removed. +- `controller/external/util.CloneTemplate` and `controllers/external/util.CloneTemplateInput` has been removed. +- The option `--list-images` from `clusterctl init` subcommand has been removed. - `exp/runtime/server.NewServer` has been removed. - `--disable-no-echo` option from `clusterctl describe cluster` subcommand has been removed +- `api/v1beta1.ClusterTopologyManagedFieldsAnnotation` field has been removed. ### API Changes diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index b94dd92487c5..a60203c43f62 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -18,15 +18,12 @@ package structuredmerge import ( "context" - "encoding/json" "github.com/pkg/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util" ) @@ -59,13 +56,6 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj return nil, errors.Wrap(err, "failed to convert original object to Unstructured") } } - - // If the object has been created with previous custom approach for tracking managed fields, cleanup the object. - if _, ok := original.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation]; ok { - if err := cleanupLegacyManagedFields(ctx, originalUnstructured, c); err != nil { - return nil, errors.Wrap(err, "failed to cleanup legacy managed fields from original object") - } - } } modifiedUnstructured := &unstructured.Unstructured{} @@ -149,55 +139,3 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error { } return h.client.Patch(ctx, h.modified, client.Apply, options...) } - -// cleanupLegacyManagedFields cleanups managed field management in place before introducing SSA. -// NOTE: this operation can trigger a machine rollout, but this is considered acceptable given that ClusterClass is still alpha -// and SSA adoption align the topology controller with K8s recommended solution for many controllers authoring the same object. -func cleanupLegacyManagedFields(ctx context.Context, obj *unstructured.Unstructured, c client.Client) error { - base := obj.DeepCopyObject().(*unstructured.Unstructured) - - // Remove the topology.cluster.x-k8s.io/managed-field-paths annotation - annotations := obj.GetAnnotations() - delete(annotations, clusterv1.ClusterTopologyManagedFieldsAnnotation) - obj.SetAnnotations(annotations) - - // Remove managedFieldEntry for manager=manager and operation=update to prevent having two managers holding values set by the topology controller. - originalManagedFields := obj.GetManagedFields() - managedFields := make([]metav1.ManagedFieldsEntry, 0, len(originalManagedFields)) - for i := range originalManagedFields { - if originalManagedFields[i].Manager == "manager" && - originalManagedFields[i].Operation == metav1.ManagedFieldsOperationUpdate { - continue - } - managedFields = append(managedFields, originalManagedFields[i]) - } - - // Add a seeding managedFieldEntry for SSA executed by the management controller, to prevent SSA to create/infer - // a default managedFieldEntry when the first SSA is applied. - // More specifically, if an existing object doesn't have managedFields when applying the first SSA the API server - // creates an entry with operation=Update (kind of guessing where the object comes from), but this entry ends up - // acting as a co-ownership and we want to prevent this. - // NOTE: fieldV1Map cannot be empty, so we add metadata.name which will be cleaned up at the first SSA patch. - fieldV1Map := map[string]interface{}{ - "f:metadata": map[string]interface{}{ - "f:name": map[string]interface{}{}, - }, - } - fieldV1, err := json.Marshal(fieldV1Map) - if err != nil { - return errors.Wrap(err, "failed to create seeding fieldV1Map for cleaning up legacy managed fields") - } - now := metav1.Now() - managedFields = append(managedFields, metav1.ManagedFieldsEntry{ - Manager: TopologyManagerName, - Operation: metav1.ManagedFieldsOperationApply, - APIVersion: obj.GetAPIVersion(), - Time: &now, - FieldsType: "FieldsV1", - FieldsV1: &metav1.FieldsV1{Raw: fieldV1}, - }) - - obj.SetManagedFields(managedFields) - - return c.Patch(ctx, obj, client.MergeFrom(base)) -} diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 71b254908832..9872dd6aac13 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -487,57 +487,6 @@ func TestServerSideApply(t *testing.T) { }) } -func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { - g := NewWithT(t) - // Create a namespace for running the test - ns, err := env.CreateNamespace(ctx, "ssa") - g.Expect(err).ToNot(HaveOccurred()) - - // Build the test object to work with. - obj := builder.TestInfrastructureCluster(ns.Name, "obj1").WithSpecFields(map[string]interface{}{ - "spec.foo": "", - }).Build() - obj.SetAnnotations(map[string]string{clusterv1.ClusterTopologyManagedFieldsAnnotation: "foo"}) - - t.Run("Server side apply cleanups legacy managed fields", func(t *testing.T) { - g := NewWithT(t) - - // Create the object simulating reconcile with legacy managed fields - g.Expect(env.CreateAndWait(ctx, obj.DeepCopy(), client.FieldOwner("manager"))) - - // Gets the object and create SSA patch helper triggering cleanup. - original := builder.TestInfrastructureCluster("", "").Build() - g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(obj), original)).To(Succeed()) - - modified := obj.DeepCopy() - _, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient()) - g.Expect(err).ToNot(HaveOccurred()) - - // Get created object after cleanup - got := builder.TestInfrastructureCluster("", "").Build() - g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(obj), got)).To(Succeed()) - - // Check annotation has been removed - g.Expect(got.GetAnnotations()).ToNot(HaveKey(clusterv1.ClusterTopologyManagedFieldsAnnotation)) - - // Check managed fields has been fixed - gotManagedFields := got.GetManagedFields() - gotLegacyManager, gotSSAManager := false, false - for i := range gotManagedFields { - if gotManagedFields[i].Manager == "manager" && - gotManagedFields[i].Operation == metav1.ManagedFieldsOperationUpdate { - gotLegacyManager = true - } - if gotManagedFields[i].Manager == TopologyManagerName && - gotManagedFields[i].Operation == metav1.ManagedFieldsOperationApply { - gotSSAManager = true - } - } - g.Expect(gotLegacyManager).To(BeFalse()) - 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{} {