diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index f3e06962bf10..f9d89ef75125 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -40,6 +40,7 @@ const ( // 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. ClusterTopologyManagedFieldsAnnotation = "topology.cluster.x-k8s.io/managed-field-paths" // ClusterTopologyMachineDeploymentLabelName is the label set on the generated MachineDeployment objects diff --git a/cmd/clusterctl/client/cluster/client.go b/cmd/clusterctl/client/cluster/client.go index ad9edf3645c6..bd34d912581b 100644 --- a/cmd/clusterctl/client/cluster/client.go +++ b/cmd/clusterctl/client/cluster/client.go @@ -29,10 +29,6 @@ import ( logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log" ) -const ( - minimumKubernetesVersion = "v1.20.0" -) - var ( ctx = context.TODO() ) diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go index 84d582dda893..afed66962abd 100644 --- a/cmd/clusterctl/client/cluster/mover.go +++ b/cmd/clusterctl/client/cluster/mover.go @@ -17,6 +17,7 @@ limitations under the License. package cluster import ( + "context" "fmt" "os" "path/filepath" @@ -34,6 +35,7 @@ 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" @@ -851,6 +853,9 @@ 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 { @@ -897,6 +902,10 @@ 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 } @@ -1164,3 +1173,32 @@ 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/cmd/clusterctl/client/cluster/proxy.go b/cmd/clusterctl/client/cluster/proxy.go index dc7329a1a595..8ec4a9af43fc 100644 --- a/cmd/clusterctl/client/cluster/proxy.go +++ b/cmd/clusterctl/client/cluster/proxy.go @@ -18,6 +18,8 @@ package cluster import ( "fmt" + "os" + "strconv" "strings" "time" @@ -27,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" - utilversion "k8s.io/apimachinery/pkg/util/version" "k8s.io/client-go/discovery" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -52,7 +53,7 @@ type Proxy interface { // CurrentNamespace returns the namespace from the current context in the kubeconfig file. CurrentNamespace() (string, error) - // ValidateKubernetesVersion returns an error if management cluster version less than minimumKubernetesVersion. + // ValidateKubernetesVersion returns an error if management cluster version less than MinimumKubernetesVersion. ValidateKubernetesVersion() error // NewClient returns a new controller runtime Client object for working on the management cluster. @@ -119,22 +120,12 @@ func (k *proxy) ValidateKubernetesVersion() error { return err } - client := discovery.NewDiscoveryClientForConfigOrDie(config) - serverVersion, err := client.ServerVersion() - if err != nil { - return errors.Wrap(err, "failed to retrieve server version") - } - - compver, err := utilversion.MustParseGeneric(serverVersion.String()).Compare(minimumKubernetesVersion) - if err != nil { - return errors.Wrap(err, "failed to parse and compare server version") + minVer := version.MinimumKubernetesVersion + if clusterTopologyFeatureGate, _ := strconv.ParseBool(os.Getenv("CLUSTER_TOPOLOGY")); clusterTopologyFeatureGate { + minVer = version.MinimumKubernetesVersionClusterTopology } - if compver == -1 { - return errors.Errorf("unsupported management cluster server version: %s - minimum required version is %s", serverVersion.String(), minimumKubernetesVersion) - } - - return nil + return version.CheckKubernetesVersion(config, minVer) } // GetConfig returns the config for a kubernetes client. diff --git a/docs/book/src/clusterctl/commands/alpha-topology-plan.md b/docs/book/src/clusterctl/commands/alpha-topology-plan.md index 12c62fc23db1..563729c8b6c6 100644 --- a/docs/book/src/clusterctl/commands/alpha-topology-plan.md +++ b/docs/book/src/clusterctl/commands/alpha-topology-plan.md @@ -22,6 +22,27 @@ the input should have all the objects needed. + + ## Example use cases ### Designing a new ClusterClass diff --git a/docs/book/src/developer/providers/v1.1-to-v1.2.md b/docs/book/src/developer/providers/v1.1-to-v1.2.md index e638cef8c1b1..72883a5a6846 100644 --- a/docs/book/src/developer/providers/v1.1-to-v1.2.md +++ b/docs/book/src/developer/providers/v1.1-to-v1.2.md @@ -5,7 +5,10 @@ maintainers of providers and consumers of our Go API. ## Minimum Kubernetes version for the management cluster -* The minimum Kubernetes version that can be used for a management cluster by Cluster API is now 1.20.0 +* The minimum Kubernetes version that can be used for a management cluster is now 1.20.0 +* The minimum Kubernetes version that can be used for a management cluster with ClusterClass is now 1.22.0 + +NOTE: compliance with minimum Kubernetes version is enforced both by clusterctl and when the CAPI controller starts. ## Minimum Go version @@ -28,6 +31,7 @@ in ClusterAPI are kept in sync with the versions used by `sigs.k8s.io/controller ### Deprecation * `util.MachinesByCreationTimestamp` has been deprecated and will be removed in a future release. +* the `topology.cluster.x-k8s.io/managed-field-paths` annotation has been deprecated and it will be removed in a future release. ### Removals * The `third_party/kubernetes-drain` package has been removed, as we're now using `k8s.io/kubectl/pkg/drain` instead ([PR](https://github.com/kubernetes-sigs/cluster-api/pull/5440)). @@ -36,11 +40,34 @@ in ClusterAPI are kept in sync with the versions used by `sigs.k8s.io/controller `annotations.HasPaused` and `annotations.HasSkipRemediation` respectively instead. * `ObjectMeta.ClusterName` has been removed from `k8s.io/apimachinery/pkg/apis/meta/v1`. -### API Changes +### golang API Changes - `util.ClusterToInfrastructureMapFuncWithExternallyManagedCheck` was removed and the externally managed check was added to `util.ClusterToInfrastructureMapFunc`, which required changing its signature. Users of the former simply need to start using the latter and users of the latter need to add the new arguments to their call. +### Required API Changes for providers + +- ClusterClass and managed topologies are now using [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) + to properly manage other controllers like CAPA/CAPZ coauthoring slices, see [#6320](https://github.com/kubernetes-sigs/cluster-api/issues/6320). + In order to take advantage of this feature providers are required to add marker to their API types as described in + [merge-strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy). + NOTE: the change will cause a rollout on existing clusters created with ClusterClass + +E.g. in CAPA + +```go +// +optional +Subnets Subnets `json:"subnets,omitempty" +``` +Must be modified into: + +```go +// +optional +// +listType=map +// +listMapKey=id +Subnets Subnets `json:"subnets,omitempty" +``` + ### Other - Logging: diff --git a/docs/book/src/reference/versions.md b/docs/book/src/reference/versions.md index fa29dd7e2ca4..863dd154a068 100644 --- a/docs/book/src/reference/versions.md +++ b/docs/book/src/reference/versions.md @@ -77,6 +77,7 @@ These diagrams show the relationships between components in a Cluster API releas \* There is an issue with CRDs in Kubernetes v1.23.{0-2}. ClusterClass with patches is affected by that (for more details please see [this issue](https://github.com/kubernetes-sigs/cluster-api/issues/5990)). Therefore we recommend to use Kubernetes v1.23.3+ with ClusterClass. Previous Kubernetes **minor** versions are not affected. +\** When using CAPI v1.2 with the CLUSTER_TOPOLOGY experimental feature on, the Kubernetes Version for the management cluster must be >= 1.22.0. The Core Provider also talks to API server of every Workload Cluster. Therefore, the Workload Cluster's Kubernetes version must also be compatible. diff --git a/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md index 733444c1f031..1cbaedee4b3e 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md @@ -174,21 +174,19 @@ underlying objects like control plane and MachineDeployment act in the same way The topology reconciler enforces values defined in the ClusterClass templates into the topology owned objects in a Cluster. -A simple way to understand this is to `kubectl get -o json` templates referenced in a ClusterClass; -then you can consider the topology reconciler to be authoritative on all the values -under `spec`. Being authoritative means that the user cannot manually change those values in -the object derived from the template in a specific Cluster (and if they do so the value gets reconciled -to the value defined in the ClusterClass). +More specifically, the topology controller uses [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) +to write/patch topology owned objects; using SSA allows other controllers to co-author the generated objects, +like e.g. adding info for subnets in CAPA. -A corollary of the behaviour described above is that it is technically possible to change non-authoritative -fields in the object derived from the template in a specific Cluster, but we advise against using the possibility +A corollary of the behaviour described above is that it is technically possible to change fields in the object +which are not derived from the templates and patches, but we advise against using the possibility or making ad-hoc changes in generated objects unless otherwise needed for a workaround. It is always preferable to improve ClusterClasses by supporting new Cluster variants in a reusable way. \ No newline at end of file diff --git a/docs/book/src/tasks/experimental-features/cluster-class/index.md b/docs/book/src/tasks/experimental-features/cluster-class/index.md index 60851406abd6..59433a4b8173 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/index.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/index.md @@ -3,6 +3,14 @@ The ClusterClass feature introduces a new way to create clusters which reduces boilerplate and enables flexible and powerful customization of clusters. ClusterClass is a powerful abstraction implemented on top of existing interfaces and offers a set of tools and operations to streamline cluster lifecycle management while maintaining the same underlying API. + + + + **Feature gate name**: `ClusterTopology` **Variable name to enable/disable the feature gate**: `CLUSTER_TOPOLOGY` diff --git a/internal/contract/types.go b/internal/contract/types.go index e7ac0e4fcfd8..febd30e05a48 100644 --- a/internal/contract/types.go +++ b/internal/contract/types.go @@ -29,6 +29,47 @@ var errNotFound = errors.New("not found") // Path defines a how to access a field in an Unstructured object. type Path []string +// Append a field name to a path. +func (p Path) Append(k string) Path { + return append(p, k) +} + +// IsParentOf check if one path is Parent of the other. +func (p Path) IsParentOf(other Path) bool { + if len(p) >= len(other) { + return false + } + for i := range p { + if p[i] != other[i] { + return false + } + } + return true +} + +// Equal check if two path are equal (exact match). +func (p Path) Equal(other Path) bool { + if len(p) != len(other) { + return false + } + for i := range p { + if p[i] != other[i] { + return false + } + } + return true +} + +// Overlaps return true if two paths are Equal or one IsParentOf the other. +func (p Path) Overlaps(other Path) bool { + return other.Equal(p) || other.IsParentOf(p) || p.IsParentOf(other) +} + +// String returns the path as a dotted string. +func (p Path) String() string { + return strings.Join(p, ".") +} + // Int64 represents an accessor to an int64 path value. type Int64 struct { path Path diff --git a/internal/contract/types_test.go b/internal/contract/types_test.go new file mode 100644 index 000000000000..a8181f07cbb9 --- /dev/null +++ b/internal/contract/types_test.go @@ -0,0 +1,161 @@ +/* +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 contract + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func TestPath_Append(t *testing.T) { + g := NewWithT(t) + + got0 := Path{}.Append("foo") + g.Expect(got0).To(Equal(Path{"foo"})) + g.Expect(got0.String()).To(Equal("foo")) + + got1 := Path{"foo"}.Append("bar") + g.Expect(got1).To(Equal(Path{"foo", "bar"})) + g.Expect(got1.String()).To(Equal("foo.bar")) +} + +func TestPath_IsParenOf(t *testing.T) { + tests := []struct { + name string + p Path + other Path + want bool + }{ + { + name: "True for parent path", + p: Path{"foo"}, + other: Path{"foo", "bar"}, + want: true, + }, + { + name: "False for same path", + p: Path{"foo"}, + other: Path{"foo"}, + want: false, + }, + { + name: "False for child path", + p: Path{"foo", "bar"}, + other: Path{"foo"}, + want: false, + }, + { + name: "False for not overlapping path path", + p: Path{"foo", "bar"}, + other: Path{"baz"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := tt.p.IsParentOf(tt.other) + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func TestPath_Equal(t *testing.T) { + tests := []struct { + name string + p Path + other Path + want bool + }{ + { + name: "False for parent path", + p: Path{"foo"}, + other: Path{"foo", "bar"}, + want: false, + }, + { + name: "True for same path", + p: Path{"foo"}, + other: Path{"foo"}, + want: true, + }, + { + name: "False for child path", + p: Path{"foo", "bar"}, + other: Path{"foo"}, + want: false, + }, + { + name: "False for not overlapping path path", + p: Path{"foo", "bar"}, + other: Path{"baz"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := tt.p.Equal(tt.other) + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func TestPath_Overlaps(t *testing.T) { + tests := []struct { + name string + p Path + other Path + want bool + }{ + { + name: "True for parent path", + p: Path{"foo"}, + other: Path{"foo", "bar"}, + want: true, + }, + { + name: "True for same path", + p: Path{"foo"}, + other: Path{"foo"}, + want: true, + }, + { + name: "True for child path", + p: Path{"foo", "bar"}, + other: Path{"foo"}, + want: true, + }, + { + name: "False for not overlapping path path", + p: Path{"foo", "bar"}, + other: Path{"baz"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := tt.p.Overlaps(tt.other) + g.Expect(got).To(Equal(tt.want)) + }) + } +} diff --git a/internal/controllers/topology/cluster/blueprint_test.go b/internal/controllers/topology/cluster/blueprint_test.go index 709367426ad5..1fed82dd97dc 100644 --- a/internal/controllers/topology/cluster/blueprint_test.go +++ b/internal/controllers/topology/cluster/blueprint_test.go @@ -315,6 +315,7 @@ func TestGetBlueprint(t *testing.T) { // Calls getBlueprint. r := &Reconciler{ Client: fakeClient, + patchHelperFactory: dryRunPatchHelperFactory(fakeClient), UnstructuredCachingClient: fakeClient, } got, err := r.getBlueprint(ctx, scope.New(cluster).Current.Cluster) diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 22b5293f1b30..2d0857d26b43 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/external" "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/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/patch" @@ -70,6 +71,8 @@ type Reconciler struct { // patchEngine is used to apply patches during computeDesiredState. patchEngine patches.Engine + + patchHelperFactory structuredmerge.PatchHelperFactoryFunc } func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { @@ -102,6 +105,9 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt } r.patchEngine = patches.NewEngine() r.recorder = mgr.GetEventRecorderFor("topology/cluster") + if r.patchHelperFactory == nil { + r.patchHelperFactory = serverSideApplyPatchHelperFactory(r.Client) + } return nil } @@ -109,6 +115,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt func (r *Reconciler) SetupForDryRun(recorder record.EventRecorder) { r.patchEngine = patches.NewEngine() r.recorder = recorder + r.patchHelperFactory = dryRunPatchHelperFactory(r.Client) } func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { @@ -285,3 +292,17 @@ 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...) + } +} + +// 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 structuredmerge.NewTwoWaysPatchHelper(original, modified, c, opts...) + } +} diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index d77a0575de45..77d74056659f 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -67,7 +67,7 @@ func TestClusterReconciler_reconcileNewlyCreatedCluster(t *testing.T) { g.Eventually(func(g Gomega) error { // Get the cluster object. actualCluster := &clusterv1.Cluster{} - if err := env.Get(ctx, client.ObjectKey{Name: clusterName1, Namespace: ns.Name}, actualCluster); err != nil { + if err := env.GetAPIReader().Get(ctx, client.ObjectKey{Name: clusterName1, Namespace: ns.Name}, actualCluster); err != nil { return err } diff --git a/internal/controllers/topology/cluster/current_state_test.go b/internal/controllers/topology/cluster/current_state_test.go index ca4c64bc2045..3a102446feee 100644 --- a/internal/controllers/topology/cluster/current_state_test.go +++ b/internal/controllers/topology/cluster/current_state_test.go @@ -538,6 +538,7 @@ func TestGetCurrentState(t *testing.T) { Client: fakeClient, APIReader: fakeClient, UnstructuredCachingClient: fakeClient, + patchHelperFactory: dryRunPatchHelperFactory(fakeClient), } got, err := r.getCurrentState(ctx, s) diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 1105c409cc41..56bc213a0f42 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -71,7 +71,8 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (* desiredState.ControlPlane.Object, selectorForControlPlaneMHC(), s.Current.Cluster.Name, - s.Blueprint.ControlPlane.MachineHealthCheck) + s.Blueprint.ControlPlane.MachineHealthCheck, + s.Current.ControlPlane.MachineHealthCheck) } // Compute the desired state for the Cluster object adding a reference to the @@ -120,6 +121,16 @@ func computeInfrastructureCluster(_ context.Context, s *scope.Scope) (*unstructu if err != nil { return nil, errors.Wrapf(err, "failed to generate the InfrastructureCluster object from the %s", template.GetKind()) } + + // Carry over shim owner reference if any. + // NOTE: this prevents to the ownerRef to be deleted by server side apply. + if s.Current.InfrastructureCluster != nil { + shim := clusterShim(s.Current.Cluster) + if ref := getOwnerReferenceFrom(s.Current.InfrastructureCluster, shim); ref != nil { + infrastructureCluster.SetOwnerReferences([]metav1.OwnerReference{*ref}) + } + } + return infrastructureCluster, nil } @@ -175,6 +186,15 @@ func computeControlPlane(_ context.Context, s *scope.Scope, infrastructureMachin return nil, errors.Wrapf(err, "failed to generate the ControlPlane object from the %s", template.GetKind()) } + // Carry over shim owner reference if any. + // NOTE: this prevents to the ownerRef to be deleted by server side apply. + if s.Current.ControlPlane != nil && s.Current.ControlPlane.Object != nil { + shim := clusterShim(s.Current.Cluster) + if ref := getOwnerReferenceFrom(s.Current.ControlPlane.Object, shim); ref != nil { + controlPlane.SetOwnerReferences([]metav1.OwnerReference{*ref}) + } + } + // If the ClusterClass mandates the controlPlane has infrastructureMachines, add a reference to InfrastructureMachine // template and metadata to be used for the control plane machines. if s.Blueprint.HasControlPlaneInfrastructureMachine() { @@ -506,12 +526,17 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP // If the ClusterClass defines a MachineHealthCheck for the MachineDeployment add it to the desired state. if machineDeploymentBlueprint.MachineHealthCheck != nil { + var currentMachineHealthCheck *clusterv1.MachineHealthCheck + if currentMachineDeployment != nil { + currentMachineHealthCheck = currentMachineDeployment.MachineHealthCheck + } // Note: The MHC is going to use a selector that provides a minimal set of labels which are common to all MachineSets belonging to the MachineDeployment. desiredMachineDeployment.MachineHealthCheck = computeMachineHealthCheck( desiredMachineDeploymentObj, selectorForMachineDeploymentMHC(desiredMachineDeploymentObj), s.Current.Cluster.Name, - machineDeploymentBlueprint.MachineHealthCheck) + machineDeploymentBlueprint.MachineHealthCheck, + currentMachineHealthCheck) } return desiredMachineDeployment, nil } @@ -744,7 +769,7 @@ func ownerReferenceTo(obj client.Object) *metav1.OwnerReference { } } -func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck { +func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass, current *clusterv1.MachineHealthCheck) *clusterv1.MachineHealthCheck { // Create a MachineHealthCheck with the spec given in the ClusterClass. mhc := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ @@ -765,9 +790,19 @@ func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1 RemediationTemplate: check.RemediationTemplate, }, } + // Default all fields in the MachineHealthCheck using the same function called in the webhook. This ensures the desired // state of the object won't be different from the current state due to webhook Defaulting. mhc.Default() + + // Carry over ownerReference to the target object. + // NOTE: this prevents to the ownerRef to be deleted by server side apply. + if current != nil { + if ref := getOwnerReferenceFrom(current, healthCheckTarget); ref != nil { + mhc.SetOwnerReferences([]metav1.OwnerReference{*ref}) + } + } + return mhc } diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 972fd8ec5f14..409192d98ff8 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -124,6 +124,25 @@ func TestComputeInfrastructureCluster(t *testing.T) { obj: obj, }) }) + t.Run("Carry over the owner reference to ClusterShim, if any", func(t *testing.T) { + g := NewWithT(t) + shim := clusterShim(cluster) + + // current cluster objects for the test scenario + clusterWithInfrastructureRef := cluster.DeepCopy() + clusterWithInfrastructureRef.Spec.InfrastructureRef = fakeRef1 + + // aggregating current cluster objects into ClusterState (simulating getCurrentState) + scope := scope.New(clusterWithInfrastructureRef) + scope.Current.InfrastructureCluster = infrastructureClusterTemplate.DeepCopy() + scope.Current.InfrastructureCluster.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim)}) + scope.Blueprint = blueprint + + obj, err := computeInfrastructureCluster(ctx, scope) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(obj).ToNot(BeNil()) + g.Expect(hasOwnerReferenceFrom(obj, shim)).To(BeTrue()) + }) } func TestComputeControlPlaneInfrastructureMachineTemplate(t *testing.T) { @@ -474,6 +493,42 @@ func TestComputeControlPlane(t *testing.T) { }) } }) + t.Run("Carry over the owner reference to ClusterShim, if any", func(t *testing.T) { + g := NewWithT(t) + shim := clusterShim(cluster) + + // current cluster objects + clusterWithoutReplicas := cluster.DeepCopy() + clusterWithoutReplicas.Spec.Topology.ControlPlane.Replicas = nil + + blueprint := &scope.ClusterBlueprint{ + Topology: clusterWithoutReplicas.Spec.Topology, + ClusterClass: clusterClass, + ControlPlane: &scope.ControlPlaneBlueprint{ + Template: controlPlaneTemplate, + }, + } + + // aggregating current cluster objects into ClusterState (simulating getCurrentState) + s := scope.New(clusterWithoutReplicas) + s.Current.ControlPlane = &scope.ControlPlaneState{ + Object: builder.ControlPlane("test1", "cp1"). + WithSpecFields(map[string]interface{}{ + "spec.version": "v1.2.2", + }). + WithStatusFields(map[string]interface{}{ + "status.version": "v1.2.1", + }). + Build(), + } + s.Current.ControlPlane.Object.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim)}) + s.Blueprint = blueprint + + obj, err := computeControlPlane(ctx, s, nil) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(obj).ToNot(BeNil()) + g.Expect(hasOwnerReferenceFrom(obj, shim)).To(BeTrue()) + }) } func TestComputeControlPlaneVersion(t *testing.T) { @@ -1461,6 +1516,20 @@ func Test_computeMachineHealthCheck(t *testing.T) { }} healthCheckTarget := builder.MachineDeployment("ns1", "md1").Build() clusterName := "cluster1" + current := &clusterv1.MachineHealthCheck{ + TypeMeta: metav1.TypeMeta{ + Kind: clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind, + APIVersion: clusterv1.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "md1", + Namespace: "ns1", + // The only thing we care about in current is the owner reference to the target object + OwnerReferences: []metav1.OwnerReference{ + *ownerReferenceTo(healthCheckTarget), + }, + }, + } want := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ Kind: clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind, @@ -1471,6 +1540,9 @@ func Test_computeMachineHealthCheck(t *testing.T) { Namespace: "ns1", // Label is added by defaulting values using MachineHealthCheck.Default() Labels: map[string]string{"cluster.x-k8s.io/cluster-name": "cluster1"}, + OwnerReferences: []metav1.OwnerReference{ + *ownerReferenceTo(healthCheckTarget), + }, }, Spec: clusterv1.MachineHealthCheckSpec{ ClusterName: "cluster1", @@ -1499,7 +1571,7 @@ func Test_computeMachineHealthCheck(t *testing.T) { t.Run("set all fields correctly", func(t *testing.T) { g := NewWithT(t) - got := computeMachineHealthCheck(healthCheckTarget, selector, clusterName, mhcSpec) + got := computeMachineHealthCheck(healthCheckTarget, selector, clusterName, mhcSpec, current) g.Expect(got).To(Equal(want), cmp.Diff(got, want)) }) diff --git a/internal/controllers/topology/cluster/mergepatch/managed_paths.go b/internal/controllers/topology/cluster/mergepatch/managed_paths.go deleted file mode 100644 index 877a1ca28af0..000000000000 --- a/internal/controllers/topology/cluster/mergepatch/managed_paths.go +++ /dev/null @@ -1,224 +0,0 @@ -/* -Copyright 2021 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 mergepatch - -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" - "sigs.k8s.io/cluster-api/internal/contract" -) - -// DeepCopyWithManagedFieldAnnotation returns a copy of the object with an annotation -// Keeping track of the fields the object is setting. -func DeepCopyWithManagedFieldAnnotation(obj client.Object) (client.Object, error) { - return deepCopyWithManagedFieldAnnotation(obj, nil) -} - -func deepCopyWithManagedFieldAnnotation(obj client.Object, ignorePaths []contract.Path) (client.Object, error) { - // Store the list of paths managed by the topology controller in the current patch operation; - // this information will be used by the next patch operation. - objWithManagedFieldAnnotation := obj.DeepCopyObject().(client.Object) - if err := storeManagedPaths(objWithManagedFieldAnnotation, ignorePaths); err != nil { - return nil, err - } - return objWithManagedFieldAnnotation, nil -} - -// storeManagedPaths stores the list of paths managed by the topology controller into the managed field annotation. -// NOTE: The topology controller is only concerned about managed paths in the spec; given that -// we are dropping spec. from the result to reduce verbosity of the generated annotation. -// NOTE: Managed paths are relevant only for unstructured objects where it is not possible -// 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. -func storeManagedPaths(obj client.Object, ignorePaths []contract.Path) error { - // Return early if the object is not unstructured. - u, ok := obj.(*unstructured.Unstructured) - if !ok { - return nil - } - - // Gets the object spec. - spec, _, err := unstructured.NestedMap(u.UnstructuredContent(), "spec") - if err != nil { - return errors.Wrap(err, "failed to get object spec") - } - - // Gets a map with the key of the fields we are going to set into spec. - managedFieldsMap := toManagedFieldsMap(spec, specIgnorePaths(ignorePaths)) - - // Gets the annotation for the given map. - managedFieldAnnotation, err := toManagedFieldAnnotation(managedFieldsMap) - if err != nil { - return err - } - - // Store the managed paths in an annotation. - annotations := obj.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string, 1) - } - annotations[clusterv1.ClusterTopologyManagedFieldsAnnotation] = managedFieldAnnotation - obj.SetAnnotations(annotations) - - return nil -} - -// specIgnorePaths returns ignore paths that apply to spec. -func specIgnorePaths(ignorePaths []contract.Path) []contract.Path { - specPaths := make([]contract.Path, 0, len(ignorePaths)) - for _, i := range ignorePaths { - if i[0] == "spec" && len(i) > 1 { - specPaths = append(specPaths, i[1:]) - } - } - return specPaths -} - -// toManagedFieldsMap returns a map with the key of the fields we are going to set into spec. -// Note: we are dropping ignorePaths. -func toManagedFieldsMap(m map[string]interface{}, ignorePaths []contract.Path) map[string]interface{} { - r := make(map[string]interface{}) - for k, v := range m { - // Drop the key if it matches ignore paths. - ignore := false - for _, i := range ignorePaths { - if i[0] == k && len(i) == 1 { - ignore = true - } - } - if ignore { - continue - } - - // If the field has nested values (it is an object/map), process them. - if nestedM, ok := v.(map[string]interface{}); ok { - nestedIgnorePaths := make([]contract.Path, 0) - for _, i := range ignorePaths { - if i[0] == k && len(i) > 1 { - nestedIgnorePaths = append(nestedIgnorePaths, i[1:]) - } - } - nestedV := toManagedFieldsMap(nestedM, nestedIgnorePaths) - - // Note: we are considering the object managed only if it is setting a value for one of the nested fields. - // This prevents the topology controller to become authoritative on all the empty maps generated due to - // how serialization works. - if len(nestedV) > 0 { - r[k] = nestedV - } - continue - } - - // Otherwise, it is a "simple" field so mark it as managed - r[k] = make(map[string]interface{}) - } - return r -} - -// managedFieldAnnotation returns a managed field annotation for a given managedFieldsMap. -func toManagedFieldAnnotation(managedFieldsMap map[string]interface{}) (string, error) { - if len(managedFieldsMap) == 0 { - return "", nil - } - - // Converts to json. - managedFieldsJSON, err := json.Marshal(managedFieldsMap) - if err != nil { - return "", errors.Wrap(err, "failed to marshal managed fields") - } - - // gzip and base64 encode - var managedFieldsJSONGZIP bytes.Buffer - zw := gzip.NewWriter(&managedFieldsJSONGZIP) - if _, err := zw.Write(managedFieldsJSON); err != nil { - return "", errors.Wrap(err, "failed to write managed fields to gzip writer") - } - if err := zw.Close(); err != nil { - return "", errors.Wrap(err, "failed to close gzip writer for managed fields") - } - managedFields := base64.StdEncoding.EncodeToString(managedFieldsJSONGZIP.Bytes()) - return managedFields, nil -} - -// getManagedPaths infers the list of paths managed by the topology controller in the previous patch operation -// by parsing the value of the managed field annotation. -// NOTE: if for any reason the annotation is missing, the patch helper will fall back on standard -// two-way merge behavior. -func getManagedPaths(obj client.Object) ([]contract.Path, error) { - // Gets the managed field annotation from the object. - managedFieldAnnotation := obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] - - if managedFieldAnnotation == "" { - return nil, nil - } - - managedFieldsJSONGZIP, err := base64.StdEncoding.DecodeString(managedFieldAnnotation) - if err != nil { - return nil, errors.Wrap(err, "failed to decode managed fields") - } - - var managedFieldsJSON bytes.Buffer - zr, err := gzip.NewReader(bytes.NewReader(managedFieldsJSONGZIP)) - if err != nil { - return nil, errors.Wrap(err, "failed to create gzip reader for managed fields") - } - - if _, err := io.Copy(&managedFieldsJSON, zr); err != nil { //nolint:gosec - return nil, errors.Wrap(err, "failed to copy from gzip reader") - } - - if err := zr.Close(); err != nil { - return nil, errors.Wrap(err, "failed to close gzip reader for managed fields") - } - - managedFieldsMap := make(map[string]interface{}) - if err := json.Unmarshal(managedFieldsJSON.Bytes(), &managedFieldsMap); err != nil { - return nil, errors.Wrap(err, "failed to unmarshal managed fields") - } - - paths := flattenManagePaths([]string{"spec"}, managedFieldsMap) - - return paths, nil -} - -// flattenManagePaths builds a slice of paths from a managedFieldMap. -func flattenManagePaths(path contract.Path, unstructuredContent map[string]interface{}) []contract.Path { - allPaths := []contract.Path{} - for k, m := range unstructuredContent { - nested, ok := m.(map[string]interface{}) - if ok && len(nested) == 0 { - // We have to use a copy of path, because otherwise the slice we append to - // allPaths would be overwritten in another iteration. - tmp := make([]string, len(path)) - copy(tmp, path) - allPaths = append(allPaths, append(tmp, k)) - continue - } - allPaths = append(allPaths, flattenManagePaths(append(path, k), nested)...) - } - return allPaths -} diff --git a/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go b/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go deleted file mode 100644 index 884ce3846863..000000000000 --- a/internal/controllers/topology/cluster/mergepatch/managed_paths_test.go +++ /dev/null @@ -1,253 +0,0 @@ -/* -Copyright 2021 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 mergepatch - -import ( - "fmt" - "testing" - - . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/client" - - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/contract" -) - -func Test_ManagedFieldAnnotation(t *testing.T) { - tests := []struct { - name string - obj client.Object - ignorePaths []contract.Path - wantPaths []contract.Path - }{ - { - name: "Does not add managed fields annotation for typed objects", - obj: &clusterv1.Cluster{ - Spec: clusterv1.ClusterSpec{ - ClusterNetwork: &clusterv1.ClusterNetwork{ - ServiceDomain: "foo.bar", - }, - }, - }, - wantPaths: nil, - }, - { - name: "Add empty managed fields annotation in case we are not setting fields in spec", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - wantPaths: []contract.Path{}, - }, - { - name: "Add managed fields annotation in case we are not setting fields in spec", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "foo", - "bar": map[string]interface{}{ - "baz": "baz", - }, - }, - }, - }, - wantPaths: []contract.Path{ - {"spec", "foo"}, - {"spec", "bar", "baz"}, - }, - }, - { - name: "Handle label names properly", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "foo", - "bar": map[string]interface{}{ - "foo.bar.baz": "baz", - }, - }, - }, - }, - wantPaths: []contract.Path{ - {"spec", "foo"}, - {"spec", "bar", "foo.bar.baz"}, - }, - }, - { - name: "Add managed fields annotation handling properly deep nesting in spec", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "replicas": int64(4), - "version": "1.17.3", - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "imageRepository": "foo", - "version": "v2.0.1", - }, - "initConfiguration": map[string]interface{}{ - "bootstrapToken": []interface{}{"abcd", "defg"}, - "nodeRegistration": map[string]interface{}{ - "criSocket": "foo", - "kubeletExtraArgs": map[string]interface{}{ - "cgroup-driver": "foo", - "eviction-hard": "foo", - }, - }, - }, - "joinConfiguration": map[string]interface{}{ - "nodeRegistration": map[string]interface{}{ - "criSocket": "foo", - "kubeletExtraArgs": map[string]interface{}{ - "cgroup-driver": "foo", - "eviction-hard": "foo", - }, - }, - }, - }, - "machineTemplate": map[string]interface{}{ - "infrastructureRef": map[string]interface{}{ - "apiVersion": "foo", - "kind": "foo", - "name": "foo", - "namespace": "foo", - }, - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "cluster.x-k8s.io/cluster-name": "foo", - "topology.cluster.x-k8s.io/owned": "foo", - }, - }, - }, - }, - }, - }, - wantPaths: []contract.Path{ - {"spec", "replicas"}, - {"spec", "version"}, - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "imageRepository"}, - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "bootstrapToken"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "criSocket"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, - {"spec", "kubeadmConfigSpec", "initConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "criSocket"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "cgroup-driver"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration", "nodeRegistration", "kubeletExtraArgs", "eviction-hard"}, - {"spec", "machineTemplate", "infrastructureRef", "namespace"}, - {"spec", "machineTemplate", "infrastructureRef", "apiVersion"}, - {"spec", "machineTemplate", "infrastructureRef", "kind"}, - {"spec", "machineTemplate", "infrastructureRef", "name"}, - {"spec", "machineTemplate", "metadata", "labels", "cluster.x-k8s.io/cluster-name"}, - {"spec", "machineTemplate", "metadata", "labels", "topology.cluster.x-k8s.io/owned"}, - }, - }, - { - name: "Managed fields annotation does not include ignorePaths", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "replicas": int64(4), - "version": "1.17.3", - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "version": "v2.0.1", - }, - "initConfiguration": map[string]interface{}{ - "bootstrapToken": []interface{}{"abcd", "defg"}, - }, - "joinConfiguration": nil, - }, - }, - }, - }, - ignorePaths: []contract.Path{ - {"spec", "version"}, // exact match (drops a single path) - {"spec", "kubeadmConfigSpec", "initConfiguration"}, // prefix match (drops everything below a path) - }, - wantPaths: []contract.Path{ - {"spec", "replicas"}, - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, - {"spec", "kubeadmConfigSpec", "joinConfiguration"}, - }, - }, - { - name: "Managed fields annotation ignore empty maps", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "version": "v2.0.1", - }, - "initConfiguration": map[string]interface{}{}, - }, - }, - }, - }, - wantPaths: []contract.Path{ - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, - }, - }, - { - name: "Managed fields annotation ignore empty maps - excluding ignore paths", - obj: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "version": "v2.0.1", - }, - "initConfiguration": map[string]interface{}{}, - }, - }, - }, - }, - ignorePaths: []contract.Path{ - {"spec", "kubeadmConfigSpec", "clusterConfiguration", "version"}, - }, - wantPaths: []contract.Path{}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - err := storeManagedPaths(tt.obj, tt.ignorePaths) - g.Expect(err).ToNot(HaveOccurred()) - - _, hasAnnotation := tt.obj.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] - g.Expect(hasAnnotation).To(Equal(tt.wantPaths != nil)) - - if hasAnnotation { - gotPaths, err := getManagedPaths(tt.obj) - g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(gotPaths).To(HaveLen(len(tt.wantPaths)), fmt.Sprintf("%v", gotPaths)) - for _, w := range tt.wantPaths { - g.Expect(gotPaths).To(ContainElement(w)) - } - } - }) - } -} diff --git a/internal/controllers/topology/cluster/mergepatch/mergepatch.go b/internal/controllers/topology/cluster/mergepatch/mergepatch.go deleted file mode 100644 index b5a26637135e..000000000000 --- a/internal/controllers/topology/cluster/mergepatch/mergepatch.go +++ /dev/null @@ -1,352 +0,0 @@ -/* -Copyright 2021 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 mergepatch - -import ( - "bytes" - "context" - "encoding/json" - - jsonpatch "github.com/evanphx/json-patch/v5" - "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - - "sigs.k8s.io/cluster-api/internal/contract" -) - -// Helper helps with a patch that yields the modified document when applied to the original document. -type Helper struct { - client client.Client - - // original holds the object to which the patch should apply to, to be used in the Patch method. - original client.Object - - // patch holds the merge patch in json format. - patch []byte - - // hasSpecChanges documents if the patch impacts the object spec - hasSpecChanges bool -} - -// NewHelper will return a patch that yields the modified document when applied to the original document. -// NOTE: patch helper consider changes only in metadata.labels, metadata.annotation and spec. -// NOTE: In the case of ClusterTopologyReconciler, original is the current object, modified is the desired object, and -// the patch returns all the changes required to align current to what is defined in desired; fields not managed -// by the topology controller are going to be preserved without changes. -func NewHelper(original, modified client.Object, c client.Client, opts ...HelperOption) (*Helper, error) { - helperOptions := &HelperOptions{} - helperOptions = helperOptions.ApplyOptions(opts) - helperOptions.allowedPaths = []contract.Path{ - {"metadata", "labels"}, - {"metadata", "annotations"}, - {"spec"}, // NOTE: The handling of managed path requires/assumes spec to be within allowed path. - } - - // Infer the list of paths managed by the topology controller in the previous patch operation; - // changes to those paths are going to be considered authoritative. - managedPaths, err := getManagedPaths(original) - if err != nil { - return nil, errors.Wrap(err, "failed to get managed paths") - } - helperOptions.managedPaths = managedPaths - - // Convert the input objects to json. - originalJSON, err := json.Marshal(original) - if err != nil { - return nil, errors.Wrap(err, "failed to marshal original object to json") - } - - // Store the list of paths managed by the topology controller in the current patch operation; - // this information will be used by the next patch operation. - modifiedWithManagedFieldAnnotation, err := deepCopyWithManagedFieldAnnotation(modified, helperOptions.ignorePaths) - if err != nil { - return nil, errors.Wrap(err, "failed to create a copy of object with the managed field annotation") - } - - modifiedJSON, err := json.Marshal(modifiedWithManagedFieldAnnotation) - if err != nil { - return nil, errors.Wrap(err, "failed to marshal modified object to json") - } - - // Compute the merge patch that will align the original object to the target - // state defined above; this patch overrides the two-way merge patch for both the - // authoritative and the managed paths, if any. - var authoritativePatch []byte - if len(helperOptions.authoritativePaths) > 0 || len(helperOptions.managedPaths) > 0 { - authoritativePatch, err = jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) - if err != nil { - return nil, errors.Wrap(err, "failed to create merge patch for authoritative paths") - } - } - - // Apply the modified object to the original one, merging the values of both; - // in case of conflicts, values from the modified object are preserved. - originalWithModifiedJSON, err := jsonpatch.MergePatch(originalJSON, modifiedJSON) - if err != nil { - return nil, errors.Wrap(err, "failed to apply modified json to original json") - } - - // Compute the merge patch that will align the original object to the target - // state defined above. - twoWayPatch, err := jsonpatch.CreateMergePatch(originalJSON, originalWithModifiedJSON) - if err != nil { - return nil, errors.Wrap(err, "failed to create merge patch") - } - - // We should consider only the changes that are relevant for the topology, removing - // changes for metadata fields computed by the system or changes to the status. - ret, err := applyPathOptions(&applyPathOptionsInput{ - authoritativePatch: authoritativePatch, - modified: modifiedJSON, - twoWayPatch: twoWayPatch, - options: helperOptions, - }) - if err != nil { - return nil, errors.Wrap(err, "failed to applyPathOptions") - } - - return &Helper{ - client: c, - patch: ret.patch, - hasSpecChanges: ret.hasSpecChanges, - original: original, - }, nil -} - -type applyPathOptionsInput struct { - authoritativePatch []byte - twoWayPatch []byte - modified []byte - options *HelperOptions -} - -type applyPathOptionsOutput struct { - patch []byte - hasSpecChanges bool -} - -// applyPathOptions applies all the options acting on path level; currently it removes from the patch diffs not -// in the allowed paths, filters out path to be ignored and enforce authoritative and managed paths. -// It also returns a flag indicating if the resulting patch has spec changes or not. -func applyPathOptions(in *applyPathOptionsInput) (*applyPathOptionsOutput, error) { - twoWayPatchMap := make(map[string]interface{}) - if err := json.Unmarshal(in.twoWayPatch, &twoWayPatchMap); err != nil { - return nil, errors.Wrap(err, "failed to unmarshal two way merge patch") - } - - // Enforce changes from authoritative patch when required (authoritative and managed paths). - // This will override instance specific fields for a subset of fields, - // e.g. machine template metadata changes should be reflected into generated objects without - // accounting for instance specific changes like we do for other maps into spec. - if len(in.options.authoritativePaths) > 0 || len(in.options.managedPaths) > 0 { - authoritativePatchMap := make(map[string]interface{}) - if err := json.Unmarshal(in.authoritativePatch, &authoritativePatchMap); err != nil { - return nil, errors.Wrap(err, "failed to unmarshal authoritative merge patch") - } - - modifiedMap := make(map[string]interface{}) - if err := json.Unmarshal(in.modified, &modifiedMap); err != nil { - return nil, errors.Wrap(err, "failed to unmarshal modified") - } - - for _, path := range append(in.options.managedPaths, in.options.authoritativePaths...) { - enforcePath(authoritativePatchMap, modifiedMap, twoWayPatchMap, path) - } - } - - // Removes from diffs everything not in allowed paths. - filterPaths(twoWayPatchMap, in.options.allowedPaths) - - // Removes from diffs everything in ignore paths. - for _, path := range in.options.ignorePaths { - removePath(twoWayPatchMap, path) - } - - // check if the changes impact the spec field. - hasSpecChanges := twoWayPatchMap["spec"] != nil - - // converts Map back into the patch - filteredPatch, err := json.Marshal(&twoWayPatchMap) - if err != nil { - return nil, errors.Wrap(err, "failed to marshal merge patch") - } - return &applyPathOptionsOutput{ - patch: filteredPatch, - hasSpecChanges: hasSpecChanges, - }, nil -} - -// filterPaths removes from the patchMap diffs not in the allowed paths. -func filterPaths(patchMap map[string]interface{}, allowedPaths []contract.Path) { - // Loop through the entries in the map. - for k, m := range patchMap { - // Check if item is in the allowed paths. - allowed := false - for _, path := range allowedPaths { - if k == path[0] { - allowed = true - break - } - } - - // If the items isn't in the allowed paths, remove it from the map. - if !allowed { - delete(patchMap, k) - continue - } - - // If the item is allowed, process then nested map with the subset of - // allowed paths relevant for this context - nestedMap, ok := m.(map[string]interface{}) - if !ok { - continue - } - nestedPaths := make([]contract.Path, 0) - for _, path := range allowedPaths { - if k == path[0] && len(path) > 1 { - nestedPaths = append(nestedPaths, path[1:]) - } - } - if len(nestedPaths) == 0 { - continue - } - filterPaths(nestedMap, nestedPaths) - - // Ensure we are not leaving empty maps around. - if len(nestedMap) == 0 { - delete(patchMap, k) - } - } -} - -// removePath removes from the patchMap diffs a given path. -func removePath(patchMap map[string]interface{}, path contract.Path) { - switch len(path) { - case 0: - // If path is empty, no-op. - return - case 1: - // If we are at the end of a path, remove the corresponding entry. - delete(patchMap, path[0]) - default: - // If in the middle of a path, go into the nested map. - nestedMap, ok := patchMap[path[0]].(map[string]interface{}) - if !ok { - // If the path is not a map, return (not a full match). - return - } - removePath(nestedMap, path[1:]) - - // Ensure we are not leaving empty maps around. - if len(nestedMap) == 0 { - delete(patchMap, path[0]) - } - } -} - -// enforcePath enforces a path from authoritativeMap into the twoWayMap thus -// enforcing changes aligned to the modified object for the authoritative paths. -func enforcePath(authoritative, modified, twoWay map[string]interface{}, path contract.Path) { - switch len(path) { - case 0: - // If path is empty, no-op. - return - case 1: - // If we are at the end of a path, enforce the value. - - // If there is an authoritative change for the value, apply it. - if authoritativeChange, authoritativeHasChange := authoritative[path[0]]; authoritativeHasChange { - twoWay[path[0]] = authoritativeChange - return - } - - // Else, if there is no authoritative change but there is a twoWays change for the value, blank it out. - delete(twoWay, path[0]) - - default: - // If in the middle of a path, go into the nested map, - var nestedSimpleMap map[string]interface{} - switch v, ok := authoritative[path[0]]; { - case ok && v == nil: - // If the nested map is nil, it means that the authoritative patch is trying to delete a parent object - // in the middle of the enforced path. - - // If the parent object has been intentionally deleted (the corresponding parent object value in the modified object is null), - // then we should enforce the deletion of the parent object including everything below it. - if m, ok := modified[path[0]]; ok && m == nil { - twoWay[path[0]] = nil - return - } - - // Otherwise, we continue processing the enforced path, thus deleting only what - // is explicitly enforced. - nestedSimpleMap = map[string]interface{}{path[1]: nil} - default: - nestedSimpleMap, ok = v.(map[string]interface{}) - - // NOTE: This should never happen given how Unmarshal works - // when generating the authoritative, but adding this as an extra safety - if !ok { - return - } - } - - // Get the corresponding map in the two-way patch. - nestedTwoWayMap, ok := twoWay[path[0]].(map[string]interface{}) - if !ok { - // If the path is empty, we need to fill it with unstructured maps. - nestedTwoWayMap = map[string]interface{}{} - twoWay[path[0]] = nestedTwoWayMap - } - - // Get the corresponding value in modified. - nestedModified, _ := modified[path[0]].(map[string]interface{}) - - // Enforce the nested path. - enforcePath(nestedSimpleMap, nestedModified, nestedTwoWayMap, path[1:]) - - // Ensure we are not leaving empty maps around. - if len(nestedTwoWayMap) == 0 { - delete(twoWay, path[0]) - } - } -} - -// HasSpecChanges return true if the patch has changes to the spec field. -func (h *Helper) HasSpecChanges() bool { - return h.hasSpecChanges -} - -// HasChanges return true if the patch has changes. -func (h *Helper) HasChanges() bool { - return !bytes.Equal(h.patch, []byte("{}")) -} - -// Patch will attempt to apply the twoWaysPatch to the original object. -func (h *Helper) Patch(ctx context.Context) error { - if !h.HasChanges() { - return nil - } - - log := ctrl.LoggerFrom(ctx) - log.V(5).Info("Patching object", "Patch", string(h.patch)) - - // Note: deepcopy before patching in order to avoid modifications to the original object. - return h.client.Patch(ctx, h.original.DeepCopyObject().(client.Object), client.RawPatch(types.MergePatchType, h.patch)) -} diff --git a/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go b/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go deleted file mode 100644 index 64e826739138..000000000000 --- a/internal/controllers/topology/cluster/mergepatch/mergepatch_test.go +++ /dev/null @@ -1,1498 +0,0 @@ -/* -Copyright 2021 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 mergepatch - -import ( - "fmt" - "testing" - - . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/contract" -) - -func TestNewHelper(t *testing.T) { - mustManagedFieldAnnotation := func(managedFieldsMap map[string]interface{}) string { - annotation, err := toManagedFieldAnnotation(managedFieldsMap) - if err != nil { - panic(fmt.Sprintf("failed to generated managed field annotation: %v", err)) - } - return annotation - } - - tests := []struct { - name string - original *unstructured.Unstructured // current - modified *unstructured.Unstructured // desired - options []HelperOption - ignoreManagedFieldAnnotationChanges bool // Prevent changes to managed field annotation to be generated, so the test can focus on how values are merged. - wantHasChanges bool - wantHasSpecChanges bool - wantPatch []byte - }{ - // Field both in original and in modified --> align to modified - - { - name: "Field (spec) both in original and in modified, no-op when equal", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Field both in original and in modified, align to modified when different", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar-changed", - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), - }, - { - name: "Field (metadata.label) both in original and in modified, align to modified when different", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar-changed", - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"bar\"}}}"), - }, - { - name: "Field (metadata.label) preserve instance specific values when path is not authoritative", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "a": "a", - "b": "b-changed", - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - // a missing - "b": "b", - "c": "c", - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"b\":\"b\",\"c\":\"c\"}}}"), - }, - { - name: "Field (metadata.label) align to modified when path is authoritative", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "a": "a", - "b": "b-changed", - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - // a missing - "b": "b", - "c": "c", - }, - }, - }, - }, - options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}}, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"labels\":{\"a\":null,\"b\":\"b\",\"c\":\"c\"}}}"), - }, - { - name: "IgnorePaths supersede AuthoritativePaths", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "a": "a", - "b": "b-changed", - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - // a missing - "b": "b", - "c": "c", - }, - }, - }, - }, - options: []HelperOption{AuthoritativePaths{contract.Path{"metadata", "labels"}}, IgnorePaths{contract.Path{"metadata", "labels"}}}, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Nested field both in original and in modified, no-op when equal", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - }, - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Nested field both in original and in modified, align to modified when different", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A-Changed", - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - }, - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), - }, - { - name: "Value of type map, enforces entries from modified, preserve entries only in original", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "map": map[string]interface{}{ - "A": "A-changed", - "B": "B", - // C missing - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "map": map[string]interface{}{ - "A": "A", - // B missing - "C": "C", - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"C\":\"C\"}}}"), - }, - { - name: "Value of type map, enforces entries from modified if the path is authoritative", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "map": map[string]interface{}{ - "A": "A-changed", - "B": "B", - // C missing - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "map": map[string]interface{}{ - "A": "A", - // B missing - "C": "C", - }, - }, - }, - }, - options: []HelperOption{AuthoritativePaths{contract.Path{"spec", "map"}}}, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"B\":null,\"C\":\"C\"}}}"), - }, - { - name: "Value of type Array or Slice, align to modified", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "slice": []interface{}{ - "D", - "C", - "B", - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "slice": []interface{}{ - "A", - "B", - "C", - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"), - }, - - // Field only in modified (not existing in original) --> align to modified - - { - name: "Field only in modified, align to modified", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"), - }, - { - name: "Nested field only in modified, align to modified", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - }, - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"), - }, - - // Field only in original (not existing in modified) --> preserve original - - { - name: "Field only in original, align to modified", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{}, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Nested field only in original, align to modified", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{}, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - - // Diff for metadata fields computed by the system or in status are discarded - - { - name: "Diff for metadata fields computed by the system or in status are discarded", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "selfLink": "foo", - "uid": "foo", - "resourceVersion": "foo", - "generation": "foo", - "managedFields": "foo", - }, - "status": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Relevant Diff for metadata (labels and annotations) are preserved", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - "annotations": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte("{\"metadata\":{\"annotations\":{\"foo\":\"bar\"},\"labels\":{\"foo\":\"bar\"}}}"), - }, - - // Ignore fields - - { - name: "Ignore fields are removed from the diff", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "controlPlaneEndpoint": map[string]interface{}{ - "host": "", - "port": int64(0), - }, - }, - }, - }, - options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}}, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - - // Managed fields - - { - name: "Managed field annotation are generated when modified have spec values", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "", - "baz": int64(0), - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"foo\":{\"bar\":\"\",\"baz\":0}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - mustManagedFieldAnnotation(map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": map[string]interface{}{}, - "baz": map[string]interface{}{}, - }, - }), - )), - }, - { - name: "Managed field annotation is empty when modified have no spec values", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "", - "baz": int64(0), - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{}, - }, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - "", - )), - }, - { - name: "Managed field annotation from a previous reconcile are cleaned up when modified have no spec values", - original: &unstructured.Unstructured{ // current - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "something": map[string]interface{}{ - "from": map[string]interface{}{ - "a": map[string]interface{}{ - "previous": map[string]interface{}{ - "reconcile": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "", - "baz": int64(0), - }, - }, - }, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{}, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"something\":{\"from\":{\"a\":{\"previous\":{\"reconcile\":null}}}}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - "", - )), - }, - { - name: "Managed field annotation does not include ignored paths - exact match", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "", - "baz": int64(0), - }, - }, - }, - }, - options: []HelperOption{IgnorePaths{contract.Path{"spec", "foo", "bar"}}}, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"foo\":{\"baz\":0}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - mustManagedFieldAnnotation(map[string]interface{}{ - "foo": map[string]interface{}{ - "baz": map[string]interface{}{}, - }, - }), - )), - }, - { - name: "Managed field annotation does not include ignored paths - path prefix", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{}, - }, - modified: &unstructured.Unstructured{ // desired - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "", - "baz": int64(0), - }, - }, - }, - }, - options: []HelperOption{IgnorePaths{contract.Path{"spec", "foo"}}}, - wantHasChanges: true, - wantHasSpecChanges: false, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - "", - )), - }, - { - name: "changes to managed field are applied", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // managed field previously set by a template - "enable-garbage-collector": "false", // user added field (should not be changed) - }, - }, - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "false", - }, - }, - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":\"false\"}}}}}}"), - }, - { - name: "changes managed field to null is applied", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // managed field previously set by a template - "enable-garbage-collector": "false", // user added field (should not be changed) - }, - }, - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": nil, - }, - }, - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}"), - }, - { - name: "dropping managed field trigger deletion; field should not be managed anymore", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // managed field previously set by a template (it is going to be dropped) - "enable-garbage-collector": "false", // user added field (should not be changed) - }, - }, - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{}, - }, - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - mustManagedFieldAnnotation(map[string]interface{}{}), - )), - }, - { - name: "changes managed object (a field with nested fields) to null is applied; managed field is updated accordingly", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // managed field previously set by a template (it is going to be dropped given that modified is providing an opinion on a parent object) - "enable-garbage-collector": "false", // user added field (it is going to be dropped given that modified is providing an opinion on a parent object) - }, - }, - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": nil, - }, - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":null}}}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{}, - }, - }, - }, - }), - )), - }, - { - name: "dropping managed object (a field with nested fields) to null is applied; managed field is updated accordingly", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "annotations": map[string]interface{}{ - clusterv1.ClusterTopologyManagedFieldsAnnotation: mustManagedFieldAnnotation(map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": map[string]interface{}{}, - }, - }, - }, - }, - }), - }, - }, - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // managed field previously set by a template (it is going to be dropped given that modified is dropping the parent object) - "enable-garbage-collector": "false", // user added field (should be preserved) - }, - }, - }, - }, - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{}, - }, - }, - }, - }, - }, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte(fmt.Sprintf( - "{\"metadata\":{\"annotations\":{%q:%q}},\"spec\":{\"kubeadmConfigSpec\":{\"clusterConfiguration\":{\"controllerManager\":{\"extraArgs\":{\"enable-hostpath-provisioner\":null}}}}}}", - clusterv1.ClusterTopologyManagedFieldsAnnotation, - mustManagedFieldAnnotation(map[string]interface{}{}), - )), - }, - - // More tests - { - name: "No changes", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - "B": "B", - "C": "C", // C only in modified - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - "B": "B", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: false, - wantHasSpecChanges: false, - wantPatch: []byte("{}"), - }, - { - name: "Many changes", - original: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - // B missing - "C": "C", // C only in modified - }, - }, - }, - modified: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "A": "A", - "B": "B", - }, - }, - }, - ignoreManagedFieldAnnotationChanges: true, - wantHasChanges: true, - wantHasSpecChanges: true, - wantPatch: []byte("{\"spec\":{\"B\":\"B\"}}"), - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - // Prevent changes to managed field annotation to be generated, so the test can focus on how values are merged. - if tt.ignoreManagedFieldAnnotationChanges { - modified := tt.modified.DeepCopy() - - var ignorePaths []contract.Path - for _, o := range tt.options { - if i, ok := o.(IgnorePaths); ok { - ignorePaths = i - } - } - - // Compute the managed field annotation for modified. - g.Expect(storeManagedPaths(modified, ignorePaths)).To(Succeed()) - - // Clone the managed field annotation back to original. - annotations := tt.original.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string, 1) - } - annotations[clusterv1.ClusterTopologyManagedFieldsAnnotation] = modified.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation] - tt.original.SetAnnotations(annotations) - } - - patch, err := NewHelper(tt.original, tt.modified, nil, tt.options...) - g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(patch.HasChanges()).To(Equal(tt.wantHasChanges)) - g.Expect(patch.HasSpecChanges()).To(Equal(tt.wantHasSpecChanges)) - g.Expect(patch.patch).To(Equal(tt.wantPatch)) - }) - } -} - -func Test_filterPaths(t *testing.T) { - tests := []struct { - name string - patchMap map[string]interface{} - paths []contract.Path - want map[string]interface{} - }{ - { - name: "Allow values", - patchMap: map[string]interface{}{ - "foo": "123", - "bar": 123, - "baz": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - }, - paths: []contract.Path{ - []string{"foo"}, - []string{"baz", "foo"}, - }, - want: map[string]interface{}{ - "foo": "123", - "baz": map[string]interface{}{ - "foo": "123", - }, - }, - }, - { - name: "Allow maps", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - "bar": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - "baz": map[string]interface{}{ - "foo": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - "bar": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - }, - }, - paths: []contract.Path{ - []string{"foo"}, - []string{"baz", "foo"}, - }, - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - "baz": map[string]interface{}{ - "foo": map[string]interface{}{ - "foo": "123", - "bar": 123, - }, - }, - }, - }, - { - name: "Cleanup empty maps", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - "baz": map[string]interface{}{ - "bar": "123", - }, - }, - }, - paths: []contract.Path{}, - want: map[string]interface{}{}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - filterPaths(tt.patchMap, tt.paths) - - g.Expect(tt.patchMap).To(Equal(tt.want)) - }) - } -} - -func Test_removePath(t *testing.T) { - tests := []struct { - name string - patchMap map[string]interface{} - path contract.Path - want map[string]interface{} - }{ - { - name: "Remove value", - patchMap: map[string]interface{}{ - "foo": "123", - }, - path: contract.Path([]string{"foo"}), - want: map[string]interface{}{}, - }, - { - name: "Remove map", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - }, - }, - path: contract.Path([]string{"foo"}), - want: map[string]interface{}{}, - }, - { - name: "Remove nested value", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - "baz": "123", - }, - }, - path: contract.Path([]string{"foo", "bar"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "baz": "123", - }, - }, - }, - { - name: "Remove nested map", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": map[string]interface{}{ - "baz": "123", - }, - "baz": "123", - }, - }, - path: contract.Path([]string{"foo", "bar"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "baz": "123", - }, - }, - }, - { - name: "Ignore partial match", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - }, - }, - path: contract.Path([]string{"foo", "bar", "baz"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - }, - }, - }, - { - name: "Cleanup empty maps", - patchMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "baz": map[string]interface{}{ - "bar": "123", - }, - }, - }, - path: contract.Path([]string{"foo", "baz", "bar"}), - want: map[string]interface{}{}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - removePath(tt.patchMap, tt.path) - - g.Expect(tt.patchMap).To(Equal(tt.want)) - }) - } -} - -func Test_enforcePath(t *testing.T) { - tests := []struct { - name string - authoritativeMap map[string]interface{} - modified map[string]interface{} - twoWaysMap map[string]interface{} - path contract.Path - want map[string]interface{} - }{ - { - name: "Keep value not enforced", - authoritativeMap: map[string]interface{}{ - "foo": nil, - }, - twoWaysMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - }, - }, - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", - }, - }, - // no enforcing path - }, - { - name: "Enforce value", - authoritativeMap: map[string]interface{}{ - "foo": nil, - }, - twoWaysMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", // value enforced, it should be overridden. - }, - }, - path: contract.Path([]string{"foo"}), - want: map[string]interface{}{ - "foo": nil, - }, - }, - { - name: "Enforce nested value", - authoritativeMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - }, - }, - twoWaysMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": "123", // value enforced, it should be overridden. - "baz": "345", // value not enforced, it should be preserved. - }, - }, - path: contract.Path([]string{"foo", "bar"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - "baz": "345", - }, - }, - }, - { - name: "Enforce nested value", - authoritativeMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - }, - }, - twoWaysMap: map[string]interface{}{ - "foo": map[string]interface{}{ // value enforced, it should be overridden. - "bar": "123", - "baz": "345", - }, - }, - path: contract.Path([]string{"foo"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - }, - }, - }, - { - name: "Enforce nested value rebuilding struct if missing", - authoritativeMap: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - }, - }, - twoWaysMap: map[string]interface{}{}, // foo enforced, it should be rebuilt/overridden. - path: contract.Path([]string{"foo"}), - want: map[string]interface{}{ - "foo": map[string]interface{}{ - "bar": nil, - }, - }, - }, - { - name: "authoritative has no changes, twoWays has no changes, no changes", - authoritativeMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": nil, - }, - }, - }, - twoWaysMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": nil, - }, - }, - }, - path: contract.Path([]string{"spec", "template", "metadata"}), - want: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": nil, - }, - }, - }, - }, - { - name: "authoritative has no changes, twoWays has no changes, no changes", - authoritativeMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{}, - }, - }, - twoWaysMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{}, - }, - }, - path: contract.Path([]string{"spec", "template", "metadata"}), - want: map[string]interface{}{}, - }, - { - name: "authoritative has changes, twoWays has no changes, authoritative apply", - authoritativeMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - }, - twoWaysMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{}, - }, - }, - path: contract.Path([]string{"spec", "template", "metadata"}), - want: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - }, - }, - { - name: "authoritative has changes, twoWays has changes, authoritative apply", - authoritativeMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - }, - twoWaysMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "baz", - }, - }, - }, - }, - }, - path: contract.Path([]string{"spec", "template", "metadata"}), - want: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "bar", - }, - }, - }, - }, - }, - }, - { - name: "authoritative has no changes, twoWays has changes, twoWays changes blanked out", - authoritativeMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{}, - }, - }, - twoWaysMap: map[string]interface{}{ - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "foo": "baz", - }, - }, - }, - }, - }, - path: contract.Path([]string{"spec", "template", "metadata"}), - want: map[string]interface{}{}, - }, - { - name: "authoritative sets to null a parent object and the change is intentional (parent null in modified).", - authoritativeMap: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": nil, // extra arg is a parent object in the authoritative path - }, - }, - }, - }, - modified: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": nil, // extra arg has been explicitly set to null - }, - }, - }, - }, - twoWaysMap: map[string]interface{}{}, - path: contract.Path([]string{"kubeadmConfigSpec", "clusterConfiguration", "controllerManager", "extraArgs", "enable-hostpath-provisioner"}), - want: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": nil, - }, - }, - }, - }, - }, - { - name: "authoritative sets to null a parent object and the change is a consequence of the object being dropped (parent does not exists in modified).", - authoritativeMap: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": nil, // extra arg is a parent object in the authoritative path - }, - }, - }, - }, - modified: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - // extra arg has been dropped from modified - }, - }, - }, - }, - twoWaysMap: map[string]interface{}{}, - path: contract.Path([]string{"kubeadmConfigSpec", "clusterConfiguration", "controllerManager", "extraArgs", "enable-hostpath-provisioner"}), - want: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": nil, - }, - }, - }, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - enforcePath(tt.authoritativeMap, tt.modified, tt.twoWaysMap, tt.path) - - g.Expect(tt.twoWaysMap).To(Equal(tt.want)) - }) - } -} diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 790fff77db19..e5c3384788f8 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -33,8 +33,8 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" "sigs.k8s.io/cluster-api/internal/contract" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/mergepatch" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge" tlog "sigs.k8s.io/cluster-api/internal/log" "sigs.k8s.io/cluster-api/internal/topology/check" ) @@ -90,14 +90,21 @@ func (r *Reconciler) reconcileClusterShim(ctx context.Context, s *scope.Scope) e // creating InfrastructureCluster/ControlPlane objects and updating the Cluster with the // references to above objects. if s.Current.InfrastructureCluster == nil || s.Current.ControlPlane.Object == nil { - if err := r.Client.Create(ctx, shim); err != nil { - if !apierrors.IsAlreadyExists(err) { - return errors.Wrap(err, "failed to create the cluster shim object") - } - if err := r.Client.Get(ctx, client.ObjectKeyFromObject(shim), shim); err != nil { - return errors.Wrapf(err, "failed to read the cluster shim object") - } + // 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) + if err != nil { + return errors.Wrap(err, "failed to create the patch helper for the cluster shim object") + } + if err := helper.Patch(ctx); err != nil { + return errors.Wrap(err, "failed to create the cluster shim object") } + + if err := r.APIReader.Get(ctx, client.ObjectKeyFromObject(shim), shim); err != nil { + return errors.Wrap(err, "get shim after creation") + } + // Enforce type meta back given that it gets blanked out by Get. shim.Kind = "Secret" shim.APIVersion = corev1.SchemeGroupVersion.String() @@ -163,16 +170,23 @@ func hasOwnerReferenceFrom(obj, owner client.Object) bool { return false } +func getOwnerReferenceFrom(obj, owner client.Object) *metav1.OwnerReference { + for _, o := range obj.GetOwnerReferences() { + if o.Kind == owner.GetObjectKind().GroupVersionKind().Kind && o.Name == owner.GetName() { + return &o + } + } + return nil +} + // reconcileInfrastructureCluster reconciles the desired state of the InfrastructureCluster object. func (r *Reconciler) reconcileInfrastructureCluster(ctx context.Context, s *scope.Scope) error { ctx, _ = tlog.LoggerFrom(ctx).WithObject(s.Desired.InfrastructureCluster).Into(ctx) return r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ - cluster: s.Current.Cluster, - current: s.Current.InfrastructureCluster, - desired: s.Desired.InfrastructureCluster, - opts: []mergepatch.HelperOption{ - mergepatch.IgnorePaths(contract.InfrastructureCluster().IgnorePaths()), - }, + cluster: s.Current.Cluster, + current: s.Current.InfrastructureCluster, + desired: s.Desired.InfrastructureCluster, + ignorePaths: contract.InfrastructureCluster().IgnorePaths(), }) } @@ -215,17 +229,6 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) current: s.Current.ControlPlane.Object, desired: s.Desired.ControlPlane.Object, versionGetter: contract.ControlPlane().Version().Get, - opts: []mergepatch.HelperOption{ - mergepatch.AuthoritativePaths{ - // Note: we want to be authoritative WRT machine's metadata labels and annotations. - // This has the nice benefit that it greatly simplify the UX around ControlPlaneClass.Metadata and - // ControlPlaneTopology.Metadata, given that changes are reflected into generated objects without - // accounting for instance specific changes like we do for other maps into spec. - // Note: nested metadata have only labels and annotations, so it is possible to override the entire - // parent struct. - contract.ControlPlane().MachineTemplate().Metadata().Path(), - }, - }, }); err != nil { return err } @@ -282,7 +285,11 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d desired.OwnerReferences = refs log.Infof("Creating %s", tlog.KObj{Obj: desired}) - if err := r.Client.Create(ctx, desired); err != nil { + helper, err := r.patchHelperFactory(nil, desired) + if err != nil { + return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: desired}) + } + if err := helper.Patch(ctx); err != nil { return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desired}) } r.recorder.Eventf(desired, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: desired}) @@ -307,7 +314,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 := mergepatch.NewHelper(current, desired, r.Client, mergepatch.AuthoritativePaths{contract.Path{"spec"}}) + patchHelper, err := r.patchHelperFactory(current, desired) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: current}) } @@ -360,7 +367,9 @@ 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 := mergepatch.NewHelper(s.Current.Cluster, s.Desired.Cluster, r.Client) + patchHelper, err := r.patchHelperFactory(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 { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: s.Current.Cluster}) } @@ -430,7 +439,11 @@ 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})) - if err := r.Client.Create(ctx, md.Object.DeepCopy()); err != nil { + helper, err := r.patchHelperFactory(nil, md.Object) + if err != nil { + return createErrorWithoutObjectName(err, md.Object) + } + if err := helper.Patch(ctx); err != nil { return createErrorWithoutObjectName(err, md.Object) } r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: md.Object}) @@ -486,23 +499,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 := mergepatch.NewHelper(currentMD.Object, desiredMD.Object, r.Client, mergepatch.AuthoritativePaths{ - // Note: we want to be authoritative WRT machine's metadata labels and annotations. - // This has the nice benefit that it greatly simplify the UX around MachineDeploymentClass.Metadata and - // MachineDeploymentTopology.Metadata, given that changes are reflected into generated objects without - // accounting for instance specific changes like we do for other maps into spec. - // Note: nested metadata have only labels and annotations, so it is possible to override the entire - // parent struct. - {"spec", "template", "metadata"}, - // Note: we want to be authoritative for the selector too, because if the selector and metadata.labels - // change, the metadata.labels might not match the selector anymore, if we don't delete outdated labels - // from the selector. - {"spec", "selector"}, - // Note: We want to be authoritative for the failureDomain set in the MachineDeployment - // spec.template.spec.failureDomain. This ensures that a change to the MachineDeploymentTopology failureDomain - // is reconciled correctly. - {"spec", "template", "spec", "failureDomain"}, - }) + patchHelper, err := r.patchHelperFactory(currentMD.Object, desiredMD.Object) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: currentMD.Object}) } @@ -583,7 +580,7 @@ type reconcileReferencedObjectInput struct { current *unstructured.Unstructured desired *unstructured.Unstructured versionGetter unstructuredVersionGetter - opts []mergepatch.HelperOption + ignorePaths []contract.Path } // reconcileReferencedObject reconciles the desired state of the referenced object. @@ -595,13 +592,12 @@ 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}) - - desiredWithManagedFieldAnnotation, err := mergepatch.DeepCopyWithManagedFieldAnnotation(in.desired) + helper, err := r.patchHelperFactory(nil, in.desired) if err != nil { - return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired}) + return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } - if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil { - return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation) + if err := helper.Patch(ctx); err != nil { + return createErrorWithoutObjectName(err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired}) return nil @@ -613,7 +609,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 := mergepatch.NewHelper(in.current, in.desired, r.Client, in.opts...) + patchHelper, err := r.patchHelperFactory(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}) } @@ -673,12 +669,12 @@ 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}) - desiredWithManagedFieldAnnotation, err := mergepatch.DeepCopyWithManagedFieldAnnotation(in.desired) + helper, err := r.patchHelperFactory(nil, in.desired) if err != nil { - return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired}) + return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } - if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil { - return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation) + if err := helper.Patch(ctx); err != nil { + return createErrorWithoutObjectName(err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired}) return nil @@ -694,7 +690,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 := mergepatch.NewHelper(in.current, in.desired, r.Client) + patchHelper, err := r.patchHelperFactory(in.current, in.desired) if err != nil { return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) } @@ -725,12 +721,12 @@ 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}) - desiredWithManagedFieldAnnotation, err := mergepatch.DeepCopyWithManagedFieldAnnotation(in.desired) + helper, err := r.patchHelperFactory(nil, in.desired) if err != nil { - return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired}) + return errors.Wrap(createErrorWithoutObjectName(err, in.desired), "failed to create patch helper") } - if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil { - return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation) + if err := helper.Patch(ctx); err != nil { + return createErrorWithoutObjectName(err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q as a replacement for %q (template rotation)", tlog.KObj{Obj: in.desired}, in.ref.Name) diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 83c0777762c6..6c14652fb496 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -17,7 +17,6 @@ limitations under the License. package cluster import ( - "context" "fmt" "net/http" "regexp" @@ -30,30 +29,27 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/internal/contract" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/mergepatch" "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/test/builder" . "sigs.k8s.io/cluster-api/internal/test/matchers" - "sigs.k8s.io/cluster-api/util/patch" ) var ( - IgnoreTopologyManagedFieldAnnotation = IgnorePaths{ - {"metadata", "annotations", clusterv1.ClusterTopologyManagedFieldsAnnotation}, - } IgnoreNameGenerated = IgnorePaths{ {"metadata", "name"}, } ) func TestReconcileShim(t *testing.T) { - infrastructureCluster := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() - controlPlane := builder.ControlPlane(metav1.NamespaceDefault, "infrastructure-cluster1").Build() + infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() + controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() // cluster requires a UID because reconcileClusterShim will create a cluster shim // which has the cluster set as Owner in an OwnerReference. @@ -81,7 +77,9 @@ func TestReconcileShim(t *testing.T) { // Run reconcileClusterShim. r := Reconciler{ - Client: env, + Client: env, + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -122,7 +120,9 @@ func TestReconcileShim(t *testing.T) { // Run reconcileClusterShim. r := Reconciler{ - Client: env, + Client: env, + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -170,7 +170,9 @@ func TestReconcileShim(t *testing.T) { // Run reconcileClusterShim. r := Reconciler{ - Client: env, + Client: env, + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -215,7 +217,9 @@ func TestReconcileShim(t *testing.T) { // Run reconcileClusterShim. r := Reconciler{ - Client: env, + Client: env, + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -254,7 +258,9 @@ func TestReconcileShim(t *testing.T) { // Run reconcileClusterShim using a nil client, so an error will be triggered if any operation is attempted r := Reconciler{ - Client: nil, + Client: nil, + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(nil), } err = r.reconcileClusterShim(ctx, s) g.Expect(err).ToNot(HaveOccurred()) @@ -267,9 +273,9 @@ func TestReconcileCluster(t *testing.T) { cluster1 := builder.Cluster(metav1.NamespaceDefault, "cluster1"). Build() cluster1WithReferences := builder.Cluster(metav1.NamespaceDefault, "cluster1"). - WithInfrastructureCluster(builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1"). + WithInfrastructureCluster(builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1"). Build()). - WithControlPlane(builder.ControlPlane(metav1.NamespaceDefault, "control-plane1").Build()). + WithControlPlane(builder.TestControlPlane(metav1.NamespaceDefault, "control-plane1").Build()). Build() cluster2WithReferences := cluster1WithReferences.DeepCopy() cluster2WithReferences.SetGroupVersionKind(cluster1WithReferences.GroupVersionKind()) @@ -315,6 +321,7 @@ func TestReconcileCluster(t *testing.T) { } if tt.current != nil { + // NOTE: it is ok to use create given that the Cluster are created by user. g.Expect(env.CreateAndWait(ctx, tt.current)).To(Succeed()) } @@ -323,8 +330,9 @@ func TestReconcileCluster(t *testing.T) { s.Desired = &scope.ClusterState{Cluster: tt.desired} r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } err = r.reconcileCluster(ctx, s) if tt.wantErr { @@ -350,67 +358,66 @@ func TestReconcileCluster(t *testing.T) { func TestReconcileInfrastructureCluster(t *testing.T) { g := NewWithT(t) - clusterInfrastructure1 := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). - Build() - clusterInfrastructure2 := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster2"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). - Build() - clusterInfrastructure3 := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster3"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). - Build() - clusterInfrastructure3WithInstanceSpecificChanges := clusterInfrastructure3.DeepCopy() - clusterInfrastructure3WithInstanceSpecificChanges.SetLabels(map[string]string{"foo": "bar"}) - clusterInfrastructure4 := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster4"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). - Build() - clusterInfrastructure4WithTemplateOverridingChanges := clusterInfrastructure4.DeepCopy() - err := unstructured.SetNestedField(clusterInfrastructure4WithTemplateOverridingChanges.UnstructuredContent(), false, "spec", "fakeSetting") - g.Expect(err).ToNot(HaveOccurred()) - clusterInfrastructure5 := builder.InfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster5"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). + // build an infrastructure cluster with a field managed by the topology controller (derived from the template). + clusterInfrastructure1 := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1"). + WithSpecFields(map[string]interface{}{"spec.foo": "foo"}). Build() + // build a patch used to simulate instance specific changes made by an external controller, and build the expected cluster infrastructure object. + clusterInfrastructure1ExternalChanges := "{ \"spec\": { \"bar\": \"bar\" }}" + clusterInfrastructure1WithExternalChanges := clusterInfrastructure1.DeepCopy() + g.Expect(unstructured.SetNestedField(clusterInfrastructure1WithExternalChanges.UnstructuredContent(), "bar", "spec", "bar")).To(Succeed()) + + // build a patch used to simulate an external controller overriding a field managed by the topology controller. + clusterInfrastructure1TemplateOverridingChanges := "{ \"spec\": { \"foo\": \"foo-override\" }}" + + // build a desired infrastructure cluster with incompatible changes. + clusterInfrastructure1WithIncompatibleChanges := clusterInfrastructure1.DeepCopy() + clusterInfrastructure1WithIncompatibleChanges.SetName("infrastructure-cluster1-changed") + tests := []struct { - name string - current *unstructured.Unstructured - desired *unstructured.Unstructured - want *unstructured.Unstructured - wantErr bool + name string + original *unstructured.Unstructured + externalChanges string + desired *unstructured.Unstructured + want *unstructured.Unstructured + wantErr bool }{ { - name: "Should create desired InfrastructureCluster if the current does not exists yet", - current: nil, - desired: clusterInfrastructure1, - want: clusterInfrastructure1, - wantErr: false, + name: "Should create desired InfrastructureCluster if the current does not exists yet", + original: nil, + desired: clusterInfrastructure1, + want: clusterInfrastructure1, + wantErr: false, }, { - name: "No-op if current InfrastructureCluster is equal to desired", - current: clusterInfrastructure2, - desired: clusterInfrastructure2, - want: clusterInfrastructure2, - wantErr: false, + name: "No-op if current InfrastructureCluster is equal to desired", + original: clusterInfrastructure1, + desired: clusterInfrastructure1, + want: clusterInfrastructure1, + wantErr: false, }, { - name: "Should preserve instance specific changes", - current: clusterInfrastructure3WithInstanceSpecificChanges, - desired: clusterInfrastructure3, - want: clusterInfrastructure3WithInstanceSpecificChanges, - wantErr: false, + name: "Should preserve changes from external controllers", + original: clusterInfrastructure1, + externalChanges: clusterInfrastructure1ExternalChanges, + desired: clusterInfrastructure1, + want: clusterInfrastructure1WithExternalChanges, + wantErr: false, }, { - name: "Should restore template values if overridden", - current: clusterInfrastructure4WithTemplateOverridingChanges, - desired: clusterInfrastructure4, - want: clusterInfrastructure4, - wantErr: false, + name: "Should restore template values if overridden by external controllers", + original: clusterInfrastructure1, + externalChanges: clusterInfrastructure1TemplateOverridingChanges, + desired: clusterInfrastructure1, + want: clusterInfrastructure1, + wantErr: false, }, { - name: "Fails for incompatible changes", - current: clusterInfrastructure5, - desired: clusterInfrastructure1, - wantErr: true, + name: "Fails for incompatible changes", + original: clusterInfrastructure1, + desired: clusterInfrastructure1WithIncompatibleChanges, + wantErr: true, }, } for _, tt := range tests { @@ -420,25 +427,37 @@ func TestReconcileInfrastructureCluster(t *testing.T) { // Create namespace and modify input to have correct namespace set namespace, err := env.CreateNamespace(ctx, "reconcile-infrastructure-cluster") g.Expect(err).ToNot(HaveOccurred()) - if tt.current != nil { - tt.current.SetNamespace(namespace.GetName()) + if tt.original != nil { + tt.original.SetNamespace(namespace.GetName()) } if tt.desired != nil { tt.desired.SetNamespace(namespace.GetName()) } + if tt.want != nil { + tt.want.SetNamespace(namespace.GetName()) + } - if tt.current != nil { - g.Expect(env.CreateAndWait(ctx, tt.current)).To(Succeed()) + if tt.original != nil { + // NOTE: it is required to use server side apply to creat the object in order to ensure consistency with the topology controller behaviour. + g.Expect(env.PatchAndWait(ctx, tt.original.DeepCopy(), client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + // NOTE: it is required to apply instance specific changes with a "plain" Patch operation to simulate a different manger. + if tt.externalChanges != "" { + g.Expect(env.Patch(ctx, tt.original.DeepCopy(), client.RawPatch(types.MergePatchType, []byte(tt.externalChanges)))).To(Succeed()) + } } s := scope.New(&clusterv1.Cluster{}) - s.Current.InfrastructureCluster = tt.current - - s.Desired = &scope.ClusterState{InfrastructureCluster: tt.desired} + if tt.original != nil { + current := builder.TestInfrastructureCluster("", "").Build() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.original), current)).To(Succeed()) + s.Current.InfrastructureCluster = current + } + s.Desired = &scope.ClusterState{InfrastructureCluster: tt.desired.DeepCopy()} r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } err = r.reconcileInfrastructureCluster(ctx, s) if tt.wantErr { @@ -470,104 +489,165 @@ func TestReconcileInfrastructureCluster(t *testing.T) { } } -func TestReconcileControlPlaneObject(t *testing.T) { +func TestReconcileControlPlane(t *testing.T) { g := NewWithT(t) - // Create InfrastructureMachineTemplates for test cases - infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build() - infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra2").Build() - // Infrastructure object with a different Kind. - incompatibleInfrastructureMachineTemplate := infrastructureMachineTemplate2.DeepCopy() - incompatibleInfrastructureMachineTemplate.SetKind("incompatibleInfrastructureMachineTemplate") - updatedInfrastructureMachineTemplate := infrastructureMachineTemplate.DeepCopy() - err := unstructured.SetNestedField(updatedInfrastructureMachineTemplate.UnstructuredContent(), true, "spec", "differentSetting") - g.Expect(err).ToNot(HaveOccurred()) + // Objects for testing reconciliation of a control plane without machines. // Create cluster class which does not require controlPlaneInfrastructure. ccWithoutControlPlaneInfrastructure := &scope.ControlPlaneBlueprint{} - // Create clusterClasses requiring controlPlaneInfrastructure and one not. + + // Create ControlPlaneObject without machine templates. + controlPlaneWithoutInfrastructure := builder.TestControlPlane(metav1.NamespaceDefault, "cp1"). + WithSpecFields(map[string]interface{}{"spec.foo": "foo"}). + Build() + + // Create desired ControlPlaneObject without machine templates but introducing some change. + controlPlaneWithoutInfrastructureWithChanges := controlPlaneWithoutInfrastructure.DeepCopy() + g.Expect(unstructured.SetNestedField(controlPlaneWithoutInfrastructureWithChanges.UnstructuredContent(), "foo-changed", "spec", "foo")).To(Succeed()) + + // Build a patch used to simulate instance specific changes made by an external controller, and build the expected control plane object. + controlPlaneWithoutInfrastructureExternalChanges := "{ \"spec\": { \"bar\": \"bar\" }}" + controlPlaneWithoutInfrastructureWithExternalChanges := controlPlaneWithoutInfrastructure.DeepCopy() + g.Expect(unstructured.SetNestedField(controlPlaneWithoutInfrastructureWithExternalChanges.UnstructuredContent(), "bar", "spec", "bar")).To(Succeed()) + + // Build a patch used to simulate an external controller overriding a field managed by the topology controller. + controlPlaneWithoutInfrastructureWithExternalOverridingChanges := "{ \"spec\": { \"foo\": \"foo-override\" }}" + + // Create a desired ControlPlaneObject without machine templates but introducing incompatible changes. + controlPlaneWithoutInfrastructureWithIncompatibleChanges := controlPlaneWithoutInfrastructure.DeepCopy() + controlPlaneWithoutInfrastructureWithIncompatibleChanges.SetName("cp1-changed") + + // Objects for testing reconciliation of a control plane with machines. + + // Create cluster class which does not require controlPlaneInfrastructure. + infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1"). + WithSpecFields(map[string]interface{}{"spec.template.spec.foo": "foo"}). + Build() ccWithControlPlaneInfrastructure := &scope.ControlPlaneBlueprint{InfrastructureMachineTemplate: infrastructureMachineTemplate} - // Create ControlPlaneObjects for test cases. - controlPlane1 := builder.ControlPlane(metav1.NamespaceDefault, "cp1"). + + // Create ControlPlaneObject with machine templates. + controlPlaneWithInfrastructure := builder.TestControlPlane(metav1.NamespaceDefault, "cp1"). WithInfrastructureMachineTemplate(infrastructureMachineTemplate). + WithSpecFields(map[string]interface{}{"spec.foo": "foo"}). Build() - controlPlane2 := builder.ControlPlane(metav1.NamespaceDefault, "cp2"). - WithInfrastructureMachineTemplate(infrastructureMachineTemplate2). - Build() - // ControlPlane object with novel field in the spec. - controlPlane3 := controlPlane1.DeepCopy() - err = unstructured.SetNestedField(controlPlane3.UnstructuredContent(), true, "spec", "differentSetting") - g.Expect(err).ToNot(HaveOccurred()) - // ControlPlane object with a new label. - controlPlaneWithInstanceSpecificChanges := controlPlane1.DeepCopy() - controlPlaneWithInstanceSpecificChanges.SetLabels(map[string]string{"foo": "bar"}) - // ControlPlane object with instance specific machine template labels. - controlPlaneWithInstanceSpecificMachineTemplateLabels := controlPlane1.DeepCopy() - err = contract.ControlPlane().MachineTemplate().Metadata().Set(controlPlaneWithInstanceSpecificMachineTemplateLabels, &clusterv1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}) - g.Expect(err).ToNot(HaveOccurred()) + + // Create desired controlPlaneInfrastructure with some change. + infrastructureMachineTemplateWithChanges := infrastructureMachineTemplate.DeepCopy() + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplateWithChanges.UnstructuredContent(), "foo-changed", "spec", "template", "spec", "foo")).To(Succeed()) + + // Build a patch used to simulate instance specific changes made by an external controller, and build the expected machine infrastructure object. + infrastructureMachineTemplateExternalChanges := "{ \"spec\": { \"template\": { \"spec\": { \"bar\": \"bar\" } } }}" + infrastructureMachineTemplateWithExternalChanges := infrastructureMachineTemplate.DeepCopy() + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplateWithExternalChanges.UnstructuredContent(), "bar", "spec", "template", "spec", "bar")).To(Succeed()) + + // Build a patch used to simulate an external controller overriding a field managed by the topology controller. + infrastructureMachineTemplateExternalOverridingChanges := "{ \"spec\": { \"template\": { \"spec\": { \"foo\": \"foo-override\" } } }}" + + // Create a desired infrastructure machine template with incompatible changes. + infrastructureMachineTemplateWithIncompatibleChanges := infrastructureMachineTemplate.DeepCopy() + gvk := infrastructureMachineTemplateWithIncompatibleChanges.GroupVersionKind() + gvk.Kind = "KindChanged" + infrastructureMachineTemplateWithIncompatibleChanges.SetGroupVersionKind(gvk) tests := []struct { - name string - class *scope.ControlPlaneBlueprint - current *scope.ControlPlaneState - desired *scope.ControlPlaneState - want *scope.ControlPlaneState - wantErr bool + name string + class *scope.ControlPlaneBlueprint + original *scope.ControlPlaneState + controlPlaneExternalChanges string + machineInfrastructureExternalChanges string + desired *scope.ControlPlaneState + want *scope.ControlPlaneState + wantRotation bool + wantErr bool }{ + // Testing reconciliation of a control plane without machines. { - name: "Should create desired ControlPlane if the current does not exist", - class: ccWithoutControlPlaneInfrastructure, - current: nil, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should create desired ControlPlane without machine infrastructure if the current does not exist", + class: ccWithoutControlPlaneInfrastructure, + original: nil, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + wantErr: false, }, { - name: "Fail on updating ControlPlaneObject with incompatible changes, here a different Kind for the infrastructureMachineTemplate", - class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane2.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: true, + name: "Should update the ControlPlane without machine infrastructure", + class: ccWithoutControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructureWithChanges.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructureWithChanges.DeepCopy()}, + wantErr: false, }, { - name: "Update to ControlPlaneObject with no update to the underlying infrastructure", - class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should preserve external changes to ControlPlane without machine infrastructure", + class: ccWithoutControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + controlPlaneExternalChanges: controlPlaneWithoutInfrastructureExternalChanges, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructureWithExternalChanges.DeepCopy()}, + wantErr: false, }, { - name: "Update to ControlPlaneObject with underlying infrastructure.", - class: ccWithControlPlaneInfrastructure, - current: &scope.ControlPlaneState{InfrastructureMachineTemplate: nil}, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should restore template values if overridden by external controllers into a ControlPlane without machine infrastructure", + class: ccWithoutControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + controlPlaneExternalChanges: controlPlaneWithoutInfrastructureWithExternalOverridingChanges, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + wantErr: false, }, { - name: "Update to ControlPlaneObject with no underlying infrastructure", - class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()}, - wantErr: false, + name: "Fail on updating ControlPlane without machine infrastructure in case of incompatible changes", + class: ccWithoutControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructureWithIncompatibleChanges.DeepCopy()}, + wantErr: true, }, + + // Testing reconciliation of a control plane with machines. { - name: "Preserve specific changes to the ControlPlaneObject", - class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should create desired ControlPlane with machine infrastructure if the current does not exist", + class: ccWithControlPlaneInfrastructure, + original: nil, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + wantErr: false, }, { - name: "Enforce machineTemplate.metadata", - class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificMachineTemplateLabels.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should rotate machine infrastructure in case of changes to the desired template", + class: ccWithControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplateWithChanges.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplateWithChanges.DeepCopy()}, + wantRotation: true, + wantErr: false, + }, + { + name: "Should preserve external changes to ControlPlane's machine infrastructure", // NOTE: template are not expected to mutate, this is for extra safety. + class: ccWithControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + machineInfrastructureExternalChanges: infrastructureMachineTemplateExternalChanges, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplateWithExternalChanges.DeepCopy()}, + wantRotation: false, + wantErr: false, + }, + { + name: "Should restore template values if overridden by external controllers into the ControlPlane's machine infrastructure", // NOTE: template are not expected to mutate, this is for extra safety. + class: ccWithControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + machineInfrastructureExternalChanges: infrastructureMachineTemplateExternalOverridingChanges, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + wantRotation: true, + wantErr: false, + }, + { + name: "Fail on updating ControlPlane with a machine infrastructure in case of incompatible changes", + class: ccWithControlPlaneInfrastructure, + original: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplateWithIncompatibleChanges.DeepCopy()}, + wantErr: true, }, } for _, tt := range tests { @@ -580,8 +660,8 @@ func TestReconcileControlPlaneObject(t *testing.T) { if tt.class != nil { // *scope.ControlPlaneBlueprint tt.class = prepareControlPlaneBluePrint(tt.class, namespace.GetName()) } - if tt.current != nil { // *scope.ControlPlaneState - tt.current = prepareControlPlaneState(g, tt.current, namespace.GetName()) + if tt.original != nil { // *scope.ControlPlaneState + tt.original = prepareControlPlaneState(g, tt.original, namespace.GetName()) } if tt.desired != nil { // *scope.ControlPlaneState tt.desired = prepareControlPlaneState(g, tt.desired, namespace.GetName()) @@ -601,19 +681,37 @@ func TestReconcileControlPlaneObject(t *testing.T) { } s.Current.ControlPlane = &scope.ControlPlaneState{} - if tt.current != nil { - s.Current.ControlPlane = tt.current - if tt.current.Object != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.Object)).To(Succeed()) + if tt.original != nil { + if tt.original.InfrastructureMachineTemplate != nil { + // NOTE: it is required to use server side apply to creat the object in order to ensure consistency with the topology controller behaviour. + g.Expect(env.PatchAndWait(ctx, tt.original.InfrastructureMachineTemplate.DeepCopy(), client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + // NOTE: it is required to apply instance specific changes with a "plain" Patch operation to simulate a different manger. + if tt.machineInfrastructureExternalChanges != "" { + g.Expect(env.Patch(ctx, tt.original.InfrastructureMachineTemplate.DeepCopy(), client.RawPatch(types.MergePatchType, []byte(tt.machineInfrastructureExternalChanges)))).To(Succeed()) + } + + current := builder.TestInfrastructureMachineTemplate("", "").Build() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.original.InfrastructureMachineTemplate), current)).To(Succeed()) + s.Current.ControlPlane.InfrastructureMachineTemplate = current } - if tt.current.InfrastructureMachineTemplate != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.InfrastructureMachineTemplate)).To(Succeed()) + if tt.original.Object != nil { + // NOTE: it is required to use server side apply to creat the object in order to ensure consistency with the topology controller behaviour. + g.Expect(env.PatchAndWait(ctx, tt.original.Object.DeepCopy(), client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + // NOTE: it is required to apply instance specific changes with a "plain" Patch operation to simulate a different manger. + if tt.controlPlaneExternalChanges != "" { + g.Expect(env.Patch(ctx, tt.original.Object.DeepCopy(), client.RawPatch(types.MergePatchType, []byte(tt.controlPlaneExternalChanges)))).To(Succeed()) + } + + current := builder.TestControlPlane("", "").Build() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.original.Object), current)).To(Succeed()) + s.Current.ControlPlane.Object = current } } r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } s.Desired = &scope.ClusterState{ @@ -632,10 +730,26 @@ func TestReconcileControlPlaneObject(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) // Create ControlPlane object for fetching data into - gotControlPlaneObject := builder.ControlPlane("", "").Build() + gotControlPlaneObject := builder.TestControlPlane("", "").Build() err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.want.Object), gotControlPlaneObject) g.Expect(err).ToNot(HaveOccurred()) + // check for template rotation. + gotRotation := false + var gotInfrastructureMachineRef *corev1.ObjectReference + if tt.class.InfrastructureMachineTemplate != nil { + gotInfrastructureMachineRef, err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Get(gotControlPlaneObject) + g.Expect(err).ToNot(HaveOccurred()) + if tt.original != nil { + if tt.original.InfrastructureMachineTemplate != nil && tt.original.InfrastructureMachineTemplate.GetName() != gotInfrastructureMachineRef.Name { + gotRotation = true + // if template has been rotated, fixup infrastructureRef in the wantControlPlaneObjectSpec before comparison. + g.Expect(contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(tt.want.Object, refToUnstructured(gotInfrastructureMachineRef))).To(Succeed()) + } + } + } + g.Expect(gotRotation).To(Equal(tt.wantRotation)) + // Get the spec from the ControlPlaneObject we are expecting wantControlPlaneObjectSpec, ok, err := unstructured.NestedMap(tt.want.Object.UnstructuredContent(), "spec") g.Expect(err).NotTo(HaveOccurred()) @@ -645,184 +759,63 @@ func TestReconcileControlPlaneObject(t *testing.T) { gotControlPlaneObjectSpec, ok, err := unstructured.NestedMap(gotControlPlaneObject.UnstructuredContent(), "spec") g.Expect(err).NotTo(HaveOccurred()) g.Expect(ok).To(BeTrue()) + for k, v := range wantControlPlaneObjectSpec { g.Expect(gotControlPlaneObjectSpec).To(HaveKeyWithValue(k, v)) } for k, v := range tt.want.Object.GetLabels() { g.Expect(gotControlPlaneObject.GetLabels()).To(HaveKeyWithValue(k, v)) } - }) - } -} - -func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) { - g := NewWithT(t) - - // Create InfrastructureMachineTemplates for test cases - infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1"). - Build() - infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra2"). - Build() - - // Create the blueprint mandating controlPlaneInfrastructure. - blueprint := &scope.ClusterBlueprint{ - ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithControlPlaneInfrastructureMachineTemplate(infrastructureMachineTemplate). - Build(), - ControlPlane: &scope.ControlPlaneBlueprint{ - InfrastructureMachineTemplate: infrastructureMachineTemplate, - }, - } - // Infrastructure object with a different Kind. - incompatibleInfrastructureMachineTemplate := infrastructureMachineTemplate2.DeepCopy() - incompatibleInfrastructureMachineTemplate.SetKind("incompatibleInfrastructureMachineTemplate") - updatedInfrastructureMachineTemplate := infrastructureMachineTemplate.DeepCopy() - err := unstructured.SetNestedField(updatedInfrastructureMachineTemplate.UnstructuredContent(), true, "spec", "differentSetting") - g.Expect(err).ToNot(HaveOccurred()) - // Create ControlPlaneObjects for test cases. - controlPlane1 := builder.ControlPlane(metav1.NamespaceDefault, "cp1"). - WithInfrastructureMachineTemplate(infrastructureMachineTemplate). - Build() - // ControlPlane object with novel field in the spec. - controlPlane2 := controlPlane1.DeepCopy() - err = unstructured.SetNestedField(controlPlane2.UnstructuredContent(), true, "spec", "differentSetting") - g.Expect(err).ToNot(HaveOccurred()) - // ControlPlane object with a new label. - controlPlaneWithInstanceSpecificChanges := controlPlane1.DeepCopy() - controlPlaneWithInstanceSpecificChanges.SetLabels(map[string]string{"foo": "bar"}) - // ControlPlane object with the same name as controlPlane1 but a different InfrastructureMachineTemplate - controlPlane3 := builder.ControlPlane(metav1.NamespaceDefault, "cp1"). - WithInfrastructureMachineTemplate(updatedInfrastructureMachineTemplate). - Build() - - tests := []struct { - name string - current *scope.ControlPlaneState - desired *scope.ControlPlaneState - want *scope.ControlPlaneState - wantErr bool - }{ - { - name: "Create desired InfrastructureMachineTemplate where it doesn't exist", - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, - }, - { - name: "Update desired InfrastructureMachineTemplate connected to controlPlane", - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate}, - want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate}, - wantErr: false, - }, - { - name: "Fail on updating infrastructure with incompatible changes", - current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate}, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - // Create namespace and modify input to have correct namespace set - namespace, err := env.CreateNamespace(ctx, "reconcile-control-plane") - g.Expect(err).ToNot(HaveOccurred()) - if tt.current != nil { // *scope.ControlPlaneState - tt.current = prepareControlPlaneState(g, tt.current, namespace.GetName()) - } - if tt.desired != nil { // *scope.ControlPlaneState - tt.desired = prepareControlPlaneState(g, tt.desired, namespace.GetName()) - } - if tt.want != nil { // *scope.ControlPlaneState - tt.want = prepareControlPlaneState(g, tt.want, namespace.GetName()) - } - - // Create Cluster object for test cases. - cluster := builder.Cluster(namespace.GetName(), "cluster1").Build() - - s := scope.New(cluster) - s.Blueprint = blueprint - if tt.current != nil { - s.Current.ControlPlane = tt.current - if tt.current.Object != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.Object)).To(Succeed()) - } - if tt.current.InfrastructureMachineTemplate != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.InfrastructureMachineTemplate)).To(Succeed()) + // Check the infrastructure template + if tt.want.InfrastructureMachineTemplate != nil { + // Check to see if the controlPlaneObject has been updated with a new template. + // This check is just for the naming format uses by generated templates - here it's templateName-* + // This check is only performed when we had an initial template that has been changed + if gotRotation { + pattern := fmt.Sprintf("%s.*", controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name)) + ok, err := regexp.Match(pattern, []byte(gotInfrastructureMachineRef.Name)) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(ok).To(BeTrue()) } - } - - r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), - } - s.Desired = &scope.ClusterState{ControlPlane: &scope.ControlPlaneState{Object: tt.desired.Object, InfrastructureMachineTemplate: tt.desired.InfrastructureMachineTemplate}} - - // Run reconcileControlPlane with the states created in the initial section of the test. - err = r.reconcileControlPlane(ctx, s) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - return - } - g.Expect(err).ToNot(HaveOccurred()) - // Create ControlPlane object for fetching data into - gotControlPlaneObject := builder.ControlPlane("", "").Build() - err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.want.Object), gotControlPlaneObject) - g.Expect(err).ToNot(HaveOccurred()) + // Create object to hold the queried InfrastructureMachineTemplate + gotInfrastructureMachineTemplateKey := client.ObjectKey{Namespace: gotInfrastructureMachineRef.Namespace, Name: gotInfrastructureMachineRef.Name} + gotInfrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate("", "").Build() + err = env.GetAPIReader().Get(ctx, gotInfrastructureMachineTemplateKey, gotInfrastructureMachineTemplate) + g.Expect(err).ToNot(HaveOccurred()) - // Check to see if the controlPlaneObject has been updated with a new template. - // This check is just for the naming format uses by generated templates - here it's templateName-* - // This check is only performed when we had an initial template that has been changed - gotInfrastructureMachineRef, err := contract.ControlPlane().MachineTemplate().InfrastructureRef().Get(gotControlPlaneObject) - g.Expect(err).ToNot(HaveOccurred()) - if tt.current.InfrastructureMachineTemplate != nil { - pattern := fmt.Sprintf("%s.*", controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name)) - ok, err := regexp.Match(pattern, []byte(gotInfrastructureMachineRef.Name)) + // Get the spec from the InfrastructureMachineTemplate we are expecting + wantInfrastructureMachineTemplateSpec, ok, err := unstructured.NestedMap(tt.want.InfrastructureMachineTemplate.UnstructuredContent(), "spec") g.Expect(err).NotTo(HaveOccurred()) g.Expect(ok).To(BeTrue()) - } - // Create object to hold the queried InfrastructureMachineTemplate - gotInfrastructureMachineTemplateKey := client.ObjectKey{Namespace: gotInfrastructureMachineRef.Namespace, Name: gotInfrastructureMachineRef.Name} - gotInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate("", "").Build() - err = env.GetAPIReader().Get(ctx, gotInfrastructureMachineTemplateKey, gotInfrastructureMachineTemplate) - g.Expect(err).ToNot(HaveOccurred()) - - // Get the spec from the InfrastructureMachineTemplate we are expecting - wantInfrastructureMachineTemplateSpec, ok, err := unstructured.NestedMap(tt.want.InfrastructureMachineTemplate.UnstructuredContent(), "spec") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(BeTrue()) - - // Get the spec from the InfrastructureMachineTemplate we got from the client.Get - gotInfrastructureMachineTemplateSpec, ok, err := unstructured.NestedMap(gotInfrastructureMachineTemplate.UnstructuredContent(), "spec") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ok).To(BeTrue()) + // Get the spec from the InfrastructureMachineTemplate we got from the client.Get + gotInfrastructureMachineTemplateSpec, ok, err := unstructured.NestedMap(gotInfrastructureMachineTemplate.UnstructuredContent(), "spec") + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(ok).To(BeTrue()) - // Compare all keys and values in the InfrastructureMachineTemplate Spec - for k, v := range wantInfrastructureMachineTemplateSpec { - g.Expect(gotInfrastructureMachineTemplateSpec).To(HaveKeyWithValue(k, v)) - } + // Compare all keys and values in the InfrastructureMachineTemplate Spec + for k, v := range wantInfrastructureMachineTemplateSpec { + g.Expect(gotInfrastructureMachineTemplateSpec).To(HaveKeyWithValue(k, v)) + } - // Check to see that labels are as expected on the object - for k, v := range tt.want.InfrastructureMachineTemplate.GetLabels() { - g.Expect(gotInfrastructureMachineTemplate.GetLabels()).To(HaveKeyWithValue(k, v)) - } + // Check to see that labels are as expected on the object + for k, v := range tt.want.InfrastructureMachineTemplate.GetLabels() { + g.Expect(gotInfrastructureMachineTemplate.GetLabels()).To(HaveKeyWithValue(k, v)) + } - // If the template was rotated during the reconcile we want to make sure the old template was deleted. - if tt.current.InfrastructureMachineTemplate != nil && tt.current.InfrastructureMachineTemplate.GetName() != tt.desired.InfrastructureMachineTemplate.GetName() { - obj := &unstructured.Unstructured{} - obj.SetAPIVersion(builder.InfrastructureGroupVersion.String()) - obj.SetKind(builder.GenericInfrastructureMachineTemplateKind) - err := r.Client.Get(ctx, client.ObjectKey{ - Namespace: tt.current.InfrastructureMachineTemplate.GetNamespace(), - Name: tt.current.InfrastructureMachineTemplate.GetName(), - }, obj) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + // If the template was rotated during the reconcile we want to make sure the old template was deleted. + if gotRotation { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion(builder.InfrastructureGroupVersion.String()) + obj.SetKind(builder.GenericInfrastructureMachineTemplateKind) + err := r.Client.Get(ctx, client.ObjectKey{ + Namespace: tt.original.InfrastructureMachineTemplate.GetNamespace(), + Name: tt.original.InfrastructureMachineTemplate.GetName(), + }, obj) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + } } }) } @@ -830,7 +823,7 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) { func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { // Create InfrastructureMachineTemplates for test cases - infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build() + infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build() mhcClass := &clusterv1.MachineHealthCheckClass{ UnhealthyConditions: []clusterv1.UnhealthyCondition{ @@ -852,7 +845,7 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { } // Create ControlPlane Object. - controlPlane1 := builder.ControlPlane(metav1.NamespaceDefault, "cp1"). + controlPlane1 := builder.TestControlPlane(metav1.NamespaceDefault, "cp1"). WithInfrastructureMachineTemplate(infrastructureMachineTemplate). Build() @@ -891,7 +884,7 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { desired: &scope.ControlPlaneState{ Object: controlPlane1.DeepCopy(), // ControlPlane does not have defined MachineInfrastructure. - //InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(), + // InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(), }, want: nil, }, @@ -961,19 +954,26 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { if tt.current != nil { s.Current.ControlPlane = tt.current if tt.current.Object != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.Object)).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, tt.current.Object, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) } if tt.current.InfrastructureMachineTemplate != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.InfrastructureMachineTemplate)).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, tt.current.InfrastructureMachineTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) } if tt.current.MachineHealthCheck != nil { - g.Expect(env.CreateAndWait(ctx, tt.current.MachineHealthCheck)).To(Succeed()) + tt.current.MachineHealthCheck.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(tt.current.Object)}) + g.Expect(env.PatchAndWait(ctx, tt.current.MachineHealthCheck, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) } } + // copy over uid of created and desired ControlPlane + if tt.current != nil && tt.current.Object != nil && tt.desired != nil && tt.desired.Object != nil { + tt.desired.Object.SetUID(tt.current.Object.GetUID()) + } + r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } s.Desired = &scope.ClusterState{ @@ -999,10 +999,7 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) if tt.want != nil { - for i, ref := range tt.want.OwnerReferences { - ref.UID = gotCP.GetUID() - tt.want.OwnerReferences[i] = ref - } + tt.want.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(gotCP)}) } g.Expect(gotMHC).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{{"kind"}, {"apiVersion"}})) @@ -1013,38 +1010,43 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { func TestReconcileMachineDeployments(t *testing.T) { g := NewWithT(t) - infrastructureMachineTemplate1 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build() - bootstrapTemplate1 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-1").Build() + // 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()) + + infrastructureMachineTemplate1 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build() + bootstrapTemplate1 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-1").Build() md1 := newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate1, bootstrapTemplate1, nil) - infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-2").Build() - bootstrapTemplate2 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-2").Build() + infrastructureMachineTemplate2 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-2").Build() + bootstrapTemplate2 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-2").Build() md2 := newFakeMachineDeploymentTopologyState("md-2", infrastructureMachineTemplate2, bootstrapTemplate2, nil) infrastructureMachineTemplate2WithChanges := infrastructureMachineTemplate2.DeepCopy() - g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate2WithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate2WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) md2WithRotatedInfrastructureMachineTemplate := newFakeMachineDeploymentTopologyState("md-2", infrastructureMachineTemplate2WithChanges, bootstrapTemplate2, nil) - infrastructureMachineTemplate3 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-3").Build() - bootstrapTemplate3 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-3").Build() + infrastructureMachineTemplate3 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-3").Build() + bootstrapTemplate3 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-3").Build() md3 := newFakeMachineDeploymentTopologyState("md-3", infrastructureMachineTemplate3, bootstrapTemplate3, nil) bootstrapTemplate3WithChanges := bootstrapTemplate3.DeepCopy() - g.Expect(unstructured.SetNestedField(bootstrapTemplate3WithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(bootstrapTemplate3WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) md3WithRotatedBootstrapTemplate := newFakeMachineDeploymentTopologyState("md-3", infrastructureMachineTemplate3, bootstrapTemplate3WithChanges, nil) bootstrapTemplate3WithChangeKind := bootstrapTemplate3.DeepCopy() bootstrapTemplate3WithChangeKind.SetKind("AnotherGenericBootstrapTemplate") md3WithRotatedBootstrapTemplateChangedKind := newFakeMachineDeploymentTopologyState("md-3", infrastructureMachineTemplate3, bootstrapTemplate3WithChanges, nil) - infrastructureMachineTemplate4 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-4").Build() - bootstrapTemplate4 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-4").Build() + infrastructureMachineTemplate4 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-4").Build() + bootstrapTemplate4 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-4").Build() md4 := newFakeMachineDeploymentTopologyState("md-4", infrastructureMachineTemplate4, bootstrapTemplate4, nil) infrastructureMachineTemplate4WithChanges := infrastructureMachineTemplate4.DeepCopy() - g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate4WithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate4WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) bootstrapTemplate4WithChanges := bootstrapTemplate4.DeepCopy() - g.Expect(unstructured.SetNestedField(bootstrapTemplate4WithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(bootstrapTemplate4WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) md4WithRotatedTemplates := newFakeMachineDeploymentTopologyState("md-4", infrastructureMachineTemplate4WithChanges, bootstrapTemplate4WithChanges, nil) - infrastructureMachineTemplate4m := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-4m").Build() - bootstrapTemplate4m := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-4m").Build() + infrastructureMachineTemplate4m := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-4m").Build() + bootstrapTemplate4m := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-4m").Build() md4m := newFakeMachineDeploymentTopologyState("md-4m", infrastructureMachineTemplate4m, bootstrapTemplate4m, nil) infrastructureMachineTemplate4mWithChanges := infrastructureMachineTemplate4m.DeepCopy() infrastructureMachineTemplate4mWithChanges.SetLabels(map[string]string{"foo": "bar"}) @@ -1052,41 +1054,41 @@ func TestReconcileMachineDeployments(t *testing.T) { bootstrapTemplate4mWithChanges.SetLabels(map[string]string{"foo": "bar"}) md4mWithInPlaceUpdatedTemplates := newFakeMachineDeploymentTopologyState("md-4m", infrastructureMachineTemplate4mWithChanges, bootstrapTemplate4mWithChanges, nil) - infrastructureMachineTemplate5 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-5").Build() - bootstrapTemplate5 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-5").Build() + infrastructureMachineTemplate5 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-5").Build() + bootstrapTemplate5 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-5").Build() md5 := newFakeMachineDeploymentTopologyState("md-5", infrastructureMachineTemplate5, bootstrapTemplate5, nil) infrastructureMachineTemplate5WithChangedKind := infrastructureMachineTemplate5.DeepCopy() infrastructureMachineTemplate5WithChangedKind.SetKind("ChangedKind") md5WithChangedInfrastructureMachineTemplateKind := newFakeMachineDeploymentTopologyState("md-4", infrastructureMachineTemplate5WithChangedKind, bootstrapTemplate5, nil) - infrastructureMachineTemplate6 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-6").Build() - bootstrapTemplate6 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-6").Build() + infrastructureMachineTemplate6 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-6").Build() + bootstrapTemplate6 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-6").Build() md6 := newFakeMachineDeploymentTopologyState("md-6", infrastructureMachineTemplate6, bootstrapTemplate6, nil) bootstrapTemplate6WithChangedNamespace := bootstrapTemplate6.DeepCopy() bootstrapTemplate6WithChangedNamespace.SetNamespace("ChangedNamespace") md6WithChangedBootstrapTemplateNamespace := newFakeMachineDeploymentTopologyState("md-6", infrastructureMachineTemplate6, bootstrapTemplate6WithChangedNamespace, nil) - infrastructureMachineTemplate7 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-7").Build() - bootstrapTemplate7 := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-7").Build() + infrastructureMachineTemplate7 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-7").Build() + bootstrapTemplate7 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-7").Build() md7 := newFakeMachineDeploymentTopologyState("md-7", infrastructureMachineTemplate7, bootstrapTemplate7, nil) - infrastructureMachineTemplate8Create := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-create").Build() - bootstrapTemplate8Create := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-create").Build() + infrastructureMachineTemplate8Create := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-create").Build() + bootstrapTemplate8Create := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-create").Build() md8Create := newFakeMachineDeploymentTopologyState("md-8-create", infrastructureMachineTemplate8Create, bootstrapTemplate8Create, nil) - infrastructureMachineTemplate8Delete := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-delete").Build() - bootstrapTemplate8Delete := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-delete").Build() + infrastructureMachineTemplate8Delete := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-delete").Build() + bootstrapTemplate8Delete := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-delete").Build() md8Delete := newFakeMachineDeploymentTopologyState("md-8-delete", infrastructureMachineTemplate8Delete, bootstrapTemplate8Delete, nil) - infrastructureMachineTemplate8Update := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-update").Build() - bootstrapTemplate8Update := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-update").Build() + infrastructureMachineTemplate8Update := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-8-update").Build() + bootstrapTemplate8Update := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-8-update").Build() md8Update := newFakeMachineDeploymentTopologyState("md-8-update", infrastructureMachineTemplate8Update, bootstrapTemplate8Update, nil) infrastructureMachineTemplate8UpdateWithChanges := infrastructureMachineTemplate8Update.DeepCopy() - g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) bootstrapTemplate8UpdateWithChanges := bootstrapTemplate3.DeepCopy() - g.Expect(unstructured.SetNestedField(bootstrapTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec")).To(Succeed()) + g.Expect(unstructured.SetNestedField(bootstrapTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) md8UpdateWithRotatedTemplates := newFakeMachineDeploymentTopologyState("md-8-update", infrastructureMachineTemplate8UpdateWithChanges, bootstrapTemplate8UpdateWithChanges, nil) - infrastructureMachineTemplate9m := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-9m").Build() - bootstrapTemplate9m := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-9m").Build() + infrastructureMachineTemplate9m := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-9m").Build() + bootstrapTemplate9m := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-9m").Build() md9 := newFakeMachineDeploymentTopologyState("md-9m", infrastructureMachineTemplate9m, bootstrapTemplate9m, nil) md9.Object.Spec.Template.ObjectMeta.Labels = map[string]string{clusterv1.ClusterLabelName: "cluster-1", "foo": "bar"} md9.Object.Spec.Selector.MatchLabels = map[string]string{clusterv1.ClusterLabelName: "cluster-1", "foo": "bar"} @@ -1211,9 +1213,9 @@ func TestReconcileMachineDeployments(t *testing.T) { } for _, s := range tt.current { - g.Expect(env.CreateAndWait(ctx, s.InfrastructureMachineTemplate)).To(Succeed()) - g.Expect(env.CreateAndWait(ctx, s.BootstrapTemplate)).To(Succeed()) - g.Expect(env.CreateAndWait(ctx, s.Object)).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, s.InfrastructureMachineTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, s.BootstrapTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, s.Object, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) } currentMachineDeploymentStates := toMachineDeploymentTopologyStateMap(tt.current) @@ -1223,8 +1225,9 @@ func TestReconcileMachineDeployments(t *testing.T) { s.Desired = &scope.ClusterState{MachineDeployments: toMachineDeploymentTopologyStateMap(tt.desired)} r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } err = r.reconcileMachineDeployments(ctx, s) if tt.wantErr { @@ -1269,7 +1272,7 @@ func TestReconcileMachineDeployments(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) - g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata, IgnoreTopologyManagedFieldAnnotation, IgnoreNameGenerated)) + g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata, IgnoreNameGenerated)) // Check BootstrapTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.BootstrapTemplate != nil { @@ -1293,7 +1296,7 @@ func TestReconcileMachineDeployments(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) - g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata, IgnoreTopologyManagedFieldAnnotation, IgnoreNameGenerated)) + g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata, IgnoreNameGenerated)) // Check InfrastructureMachineTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.InfrastructureMachineTemplate != nil { @@ -1314,6 +1317,12 @@ 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) { + // g := NewWithT(t) + // Write the config file to access the test env for debugging. + // g.Expect(os.WriteFile("test.conf", kubeconfig.FromEnvTestConfig(env.Config, &clusterv1.Cluster{ + // ObjectMeta: metav1.ObjectMeta{Name: "test"}, + // }), 0777)).To(Succeed()) + type object struct { spec map[string]interface{} } @@ -1373,26 +1382,10 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { reconcileStep{ name: "Drop enable-hostpath-provisioner", desired: object{ - spec: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - // enable-hostpath-provisioner has been removed by e.g a change in ClusterClass (and extraArgs with it). - "controllerManager": map[string]interface{}{}, - }, - }, - }, + spec: nil, }, want: object{ - spec: map[string]interface{}{ - "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - "controllerManager": map[string]interface{}{ - // Reconcile to drop enable-hostpath-provisioner, extraArgs has been set to an empty object. - "extraArgs": map[string]interface{}{}, - }, - }, - }, - }, + spec: nil, }, }, }, @@ -1459,69 +1452,6 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { }, }, }, - { - name: "Should drop label in metadata (even if outside of .spec.machineTemplate.metadata)", - // Note: This test verifies that reconcileReferencedObject treats changes to fields existing in templates as authoritative - // and most specifically it verifies that when a spec.template label is deleted, it gets deleted - // from the generated object (and it is not being treated as instance specific value). - // E.g. AzureMachineTemplate has .spec.template.metadata.labels. - reconcileSteps: []interface{}{ - reconcileStep{ - name: "Initially reconcile", - desired: object{ - spec: map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "label.with.dots/owned": "true", - "anotherLabel": "true", - }, - }, - }, - }, - }, - want: object{ - spec: map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - "label.with.dots/owned": "true", - "anotherLabel": "true", - }, - }, - }, - }, - }, - }, - reconcileStep{ - name: "Drop the label with dots", - desired: object{ - spec: map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - // label.with.dots/owned has been removed e.g a change in ClusterClass. - "anotherLabel": "true", - }, - }, - }, - }, - }, - want: object{ - spec: map[string]interface{}{ - "template": map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": map[string]interface{}{ - // Reconcile to drop label.with.dots/owned label. - "anotherLabel": "true", - }, - }, - }, - }, - }, - }, - }, - }, { name: "Should enforce field", // Note: This test verifies that reconcileReferencedObject treats changes to fields existing in templates as authoritative @@ -1531,12 +1461,12 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "Initially reconcile", desired: object{ spec: map[string]interface{}{ - "stringField": "ccValue", + "foo": "ccValue", }, }, want: object{ spec: map[string]interface{}{ - "stringField": "ccValue", + "foo": "ccValue", }, }, }, @@ -1544,7 +1474,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "User changes value", object: object{ spec: map[string]interface{}{ - "stringField": "userValue", + "foo": "userValue", }, }, }, @@ -1553,13 +1483,13 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { desired: object{ spec: map[string]interface{}{ // ClusterClass still proposing the old value. - "stringField": "ccValue", + "foo": "ccValue", }, }, want: object{ spec: map[string]interface{}{ // Reconcile to restore the old value. - "stringField": "ccValue", + "foo": "ccValue", }, }, }, @@ -1607,7 +1537,6 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { "clusterConfiguration": map[string]interface{}{ "controllerManager": map[string]interface{}{ "extraArgs": map[string]interface{}{ - "enable-hostpath-provisioner": "true", // User adds enable-garbage-collector. "enable-garbage-collector": "true", }, @@ -1622,10 +1551,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { desired: object{ spec: map[string]interface{}{ "kubeadmConfigSpec": map[string]interface{}{ - "clusterConfiguration": map[string]interface{}{ - // enable-hostpath-provisioner has been removed by e.g a change in ClusterClass (and extraArgs with it). - "controllerManager": map[string]interface{}{}, - }, + "clusterConfiguration": map[string]interface{}{}, }, }, }, @@ -1656,16 +1582,12 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "Initially reconcile", desired: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{}, - }, + "machineTemplate": map[string]interface{}{}, }, }, want: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{}, - }, + "machineTemplate": map[string]interface{}{}, }, }, }, @@ -1673,12 +1595,10 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "User adds an additional object", object: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "userDefinedObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", - }, + "machineTemplate": map[string]interface{}{ + "infrastructureRef": map[string]interface{}{ + "apiVersion": "foo/v1alpha1", + "kind": "Foo", }, }, }, @@ -1688,33 +1608,31 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "ClusterClass starts having an opinion about some fields", desired: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "clusterClassObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", + "machineTemplate": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", }, - "clusterClassField": true, }, + "nodeDeletionTimeout": "10m", }, }, }, want: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - // User fields are preserved. - "userDefinedObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", - }, - // ClusterClass authoritative fields are added. - "clusterClassObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", + "machineTemplate": map[string]interface{}{ + // User fields are preserved. + "infrastructureRef": map[string]interface{}{ + "apiVersion": "foo/v1alpha1", + "kind": "Foo", + }, + // ClusterClass authoritative fields are added. + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", }, - "clusterClassField": true, }, + "nodeDeletionTimeout": "10m", }, }, }, @@ -1723,30 +1641,28 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "ClusterClass stops having an opinion on the field", desired: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "clusterClassObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", + "machineTemplate": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", }, - // clusterClassField has been removed by e.g a change in ClusterClass (and extraArgs with it). }, + // clusterClassField has been removed by e.g a change in ClusterClass (and extraArgs with it). }, }, }, want: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - // Reconcile to drop clusterClassField, - // while preserving user-defined field and clusterClassField. - "userDefinedObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", - }, - "clusterClassObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", + "machineTemplate": map[string]interface{}{ + // Reconcile to drop clusterClassField, + // while preserving user-defined field and clusterClassField. + "infrastructureRef": map[string]interface{}{ + "apiVersion": "foo/v1alpha1", + "kind": "Foo", + }, + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", }, }, }, @@ -1757,22 +1673,19 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { name: "ClusterClass stops having an opinion on the object", desired: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{}, // clusterClassObject has been removed by e.g a change in ClusterClass (and extraArgs with it). + "machineTemplate": map[string]interface{}{ + // clusterClassObject has been removed by e.g a change in ClusterClass (and extraArgs with it). }, }, }, want: object{ spec: map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - // Reconcile to drop clusterClassObject, - // while preserving user-defined field. - "userDefinedObject": map[string]interface{}{ - "boolField": true, - "stringField": "def", - }, - "clusterClassObject": map[string]interface{}{}, + "machineTemplate": map[string]interface{}{ + // Reconcile to drop clusterClassObject, + // while preserving user-defined field. + "infrastructureRef": map[string]interface{}{ + "apiVersion": "foo/v1alpha1", + "kind": "Foo", }, }, }, @@ -1791,8 +1704,9 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } s := scope.New(&clusterv1.Cluster{}) @@ -1807,20 +1721,28 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { if i > 0 { currentControlPlane = &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "GenericControlPlane", - "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": builder.TestControlPlaneKind, + "apiVersion": builder.ControlPlaneGroupVersion.String(), }, } g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKey{Namespace: namespace.GetName(), Name: "my-cluster"}, currentControlPlane)).To(Succeed()) } if step, ok := step.(externalStep); ok { - // This is a user step, so let's just update the object. - patchHelper, err := patch.NewHelper(currentControlPlane, env) + // This is a user step, so let's just update the object using SSA. + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": builder.TestControlPlaneKind, + "apiVersion": builder.ControlPlaneGroupVersion.String(), + "metadata": map[string]interface{}{ + "name": "my-cluster", + "namespace": namespace.GetName(), + }, + "spec": step.object.spec, + }, + } + err := env.PatchAndWait(ctx, obj, client.FieldOwner("other-controller"), client.ForceOwnership) g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(unstructured.SetNestedField(currentControlPlane.Object, step.object.spec, "spec")).To(Succeed()) - g.Expect(patchHelper.Patch(context.Background(), currentControlPlane)).To(Succeed()) continue } @@ -1836,23 +1758,18 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { ControlPlane: &scope.ControlPlaneState{ Object: &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "GenericControlPlane", - "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": builder.TestControlPlaneKind, + "apiVersion": builder.ControlPlaneGroupVersion.String(), "metadata": map[string]interface{}{ "name": "my-cluster", "namespace": namespace.GetName(), }, - "spec": step.desired.spec, }, }, }, } - if currentControlPlane != nil { - // Set the annotation of the current control plane. - annotations, found, err := unstructured.NestedFieldCopy(currentControlPlane.Object, "metadata", "annotations") - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(found).To(BeTrue()) - g.Expect(unstructured.SetNestedField(s.Desired.ControlPlane.Object.Object, annotations, "metadata", "annotations")).To(Succeed()) + if step.desired.spec != nil { + s.Desired.ControlPlane.Object.Object["spec"] = step.desired.spec } // Execute a reconcile.0 @@ -1860,25 +1777,22 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { cluster: s.Current.Cluster, current: s.Current.ControlPlane.Object, desired: s.Desired.ControlPlane.Object, - opts: []mergepatch.HelperOption{ - mergepatch.AuthoritativePaths{ - // Note: Just using .spec.machineTemplate.metadata here as an example. - contract.ControlPlane().MachineTemplate().Metadata().Path(), - }, - }})).To(Succeed()) + })).To(Succeed()) // Build the object for comparison. want := &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "GenericControlPlane", - "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": builder.TestControlPlaneKind, + "apiVersion": builder.ControlPlaneGroupVersion.String(), "metadata": map[string]interface{}{ "name": "my-cluster", "namespace": namespace.GetName(), }, - "spec": step.want.spec, }, } + if step.want.spec != nil { + want.Object["spec"] = step.want.spec + } // Get the reconciled object. got := want.DeepCopy() // this is required otherwise Get will modify want @@ -1923,8 +1837,8 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}). WithClusterName("cluster1") - infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build() - bootstrapTemplate := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-1").Build() + infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build() + bootstrapTemplate := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-1").Build() tests := []struct { name string @@ -2020,16 +1934,33 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { tt.want[i].SetNamespace(namespace.GetName()) } + uidsByName := map[string]types.UID{} + for _, mdts := range tt.current { - g.Expect(env.CreateAndWait(ctx, mdts.Object)).To(Succeed()) - g.Expect(env.CreateAndWait(ctx, mdts.InfrastructureMachineTemplate)).To(Succeed()) - g.Expect(env.CreateAndWait(ctx, mdts.BootstrapTemplate)).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, mdts.Object, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, mdts.InfrastructureMachineTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, mdts.BootstrapTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + + uidsByName[mdts.Object.Name] = mdts.Object.GetUID() + if mdts.MachineHealthCheck != nil { for i, ref := range mdts.MachineHealthCheck.OwnerReferences { ref.UID = mdts.Object.GetUID() mdts.MachineHealthCheck.OwnerReferences[i] = ref } - g.Expect(env.CreateAndWait(ctx, mdts.MachineHealthCheck)).To(Succeed()) + g.Expect(env.PatchAndWait(ctx, mdts.MachineHealthCheck, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed()) + } + } + + // copy over ownerReference for desired MachineHealthCheck + for _, mdts := range tt.desired { + if mdts.MachineHealthCheck != nil { + for i, ref := range mdts.MachineHealthCheck.OwnerReferences { + if uid, ok := uidsByName[ref.Name]; ok { + ref.UID = uid + mdts.MachineHealthCheck.OwnerReferences[i] = ref + } + } } } @@ -2040,8 +1971,9 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { s.Desired = &scope.ClusterState{MachineDeployments: toMachineDeploymentTopologyStateMap(tt.desired)} r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } err = r.reconcileMachineDeployments(ctx, s) @@ -2201,8 +2133,9 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) { } r := Reconciler{ - Client: env, - recorder: env.GetEventRecorderFor("test"), + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env), + recorder: env.GetEventRecorderFor("test"), } if tt.current != nil { g.Expect(env.CreateAndWait(ctx, tt.current)).To(Succeed()) diff --git a/internal/controllers/topology/cluster/mergepatch/doc.go b/internal/controllers/topology/cluster/structuredmerge/doc.go similarity index 76% rename from internal/controllers/topology/cluster/mergepatch/doc.go rename to internal/controllers/topology/cluster/structuredmerge/doc.go index b55b7508d168..7a3d159f9c18 100644 --- a/internal/controllers/topology/cluster/mergepatch/doc.go +++ b/internal/controllers/topology/cluster/structuredmerge/doc.go @@ -1,5 +1,5 @@ /* -Copyright 2021 The Kubernetes Authors. +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. @@ -14,5 +14,5 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package mergepatch implements merge patch support for managed topology. -package mergepatch +// Package structuredmerge implements server side apply support for managed topology controllers. +package structuredmerge diff --git a/internal/controllers/topology/cluster/structuredmerge/drop_diff.go b/internal/controllers/topology/cluster/structuredmerge/drop_diff.go new file mode 100644 index 000000000000..ae99020aaf65 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/drop_diff.go @@ -0,0 +1,73 @@ +/* +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 "sigs.k8s.io/cluster-api/internal/contract" + +// dropDiff allow to change the modified object so the generated patch will not contain changes +// that match the shouldDropDiff criteria. +// NOTE: This func is called recursively only for fields of type Map, but this is ok given the current use cases +// this func has to address. More specifically, we are using only for not allowed paths and for ignore paths; +// all of them are defined in reconcile_state.go and are targeting well-known fields inside nested maps. +// Allowed paths / ignore paths which point to an array are not supported by the current implementation. +func dropDiff(ctx *dropDiffInput) { + original, _ := ctx.original.(map[string]interface{}) + modified, _ := ctx.modified.(map[string]interface{}) + for field := range modified { + fieldCtx := &dropDiffInput{ + // Compose the path for the nested field. + path: ctx.path.Append(field), + // Gets the original and the modified value for the field. + original: original[field], + modified: modified[field], + // Carry over global values from the context. + shouldDropDiffFunc: ctx.shouldDropDiffFunc, + } + + // Note: for everything we should drop changes we are making modified equal to original, so the generated patch doesn't include this change + if fieldCtx.shouldDropDiffFunc(fieldCtx.path) { + // If original exists, make modified equal to original, otherwise if original does not exist, drop the change. + if o, ok := original[field]; ok { + modified[field] = o + } else { + delete(modified, field) + } + continue + } + + // Process nested fields. + dropDiff(fieldCtx) + + // Ensure we are not leaving empty maps around. + if v, ok := fieldCtx.modified.(map[string]interface{}); ok && len(v) == 0 { + delete(modified, field) + } + } +} + +// dropDiffInput holds info required while computing dropDiff. +type dropDiffInput struct { + // the path of the field being processed. + path contract.Path + + // the original and the modified value for the current path. + original interface{} + modified interface{} + + // shouldDropDiffFunc handle the func that determine if the current path should be dropped or not. + shouldDropDiffFunc func(path contract.Path) bool +} diff --git a/internal/controllers/topology/cluster/structuredmerge/drop_diff_test.go b/internal/controllers/topology/cluster/structuredmerge/drop_diff_test.go new file mode 100644 index 000000000000..2f74c5eac82e --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/drop_diff_test.go @@ -0,0 +1,273 @@ +/* +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_dropDiffForNotAllowedPaths(t *testing.T) { + tests := []struct { + name string + ctx *dropDiffInput + wantModified map[string]interface{} + }{ + { + name: "Sets not allowed paths to original value if defined", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "foo", + }, + "status": map[string]interface{}{ + "foo": "123", + }, + }, + modified: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "foo-changed", + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + "status": map[string]interface{}{ + "foo": "123-changed", + }, + }, + shouldDropDiffFunc: isNotAllowedPath( + []contract.Path{ // NOTE: we are dropping everything not in this list (IsNotAllowed) + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"spec"}, + }, + ), + }, + wantModified: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "foo", // metadata.name aligned to original + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + "status": map[string]interface{}{ // status aligned to original + "foo": "123", + }, + }, + }, + { + name: "Drops not allowed paths if they do not exist in original", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + // Original doesn't have values for not allowed paths. + }, + modified: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "foo", + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + "status": map[string]interface{}{ + "foo": "123", + }, + }, + shouldDropDiffFunc: isNotAllowedPath( + []contract.Path{ // NOTE: we are dropping everything not in this list (IsNotAllowed) + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"spec"}, + }, + ), + }, + wantModified: map[string]interface{}{ + "metadata": map[string]interface{}{ + // metadata.name dropped + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + // status dropped + }, + }, + { + name: "Cleanup empty maps", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + // Original doesn't have values for not allowed paths. + }, + modified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "123", + }, + }, + shouldDropDiffFunc: isNotAllowedPath( + []contract.Path{}, // NOTE: we are dropping everything not in this list (IsNotAllowed) + ), + }, + wantModified: map[string]interface{}{ + // we are dropping spec.foo and then spec given that it is an empty map + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + dropDiff(tt.ctx) + + g.Expect(tt.ctx.modified).To(Equal(tt.wantModified)) + }) + } +} + +func Test_dropDiffForIgnoredPaths(t *testing.T) { + tests := []struct { + name string + ctx *dropDiffInput + wantModified map[string]interface{} + }{ + { + name: "Sets ignored paths to original value if defined", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + "controlPlaneEndpoint": map[string]interface{}{ + "host": "foo", + "port": "123", + }, + }, + }, + modified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + "controlPlaneEndpoint": map[string]interface{}{ + "host": "foo-changed", + "port": "123-changed", + }, + }, + }, + shouldDropDiffFunc: isIgnorePath( + []contract.Path{ + {"spec", "controlPlaneEndpoint"}, + }, + ), + }, + wantModified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + "controlPlaneEndpoint": map[string]interface{}{ // spec.controlPlaneEndpoint aligned to original + "host": "foo", + "port": "123", + }, + }, + }, + }, + { + name: "Drops ignore paths if they do not exist in original", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + // Original doesn't have values for ignore paths. + }, + modified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + "controlPlaneEndpoint": map[string]interface{}{ + "host": "foo-changed", + "port": "123-changed", + }, + }, + }, + shouldDropDiffFunc: isIgnorePath( + []contract.Path{ + {"spec", "controlPlaneEndpoint"}, + }, + ), + }, + wantModified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + // spec.controlPlaneEndpoint dropped + }, + }, + }, + { + name: "Cleanup empty maps", + ctx: &dropDiffInput{ + path: contract.Path{}, + original: map[string]interface{}{ + // Original doesn't have values for not allowed paths. + }, + modified: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "123", + }, + }, + shouldDropDiffFunc: isIgnorePath( + []contract.Path{ + {"spec", "foo"}, + }, + ), + }, + wantModified: map[string]interface{}{ + // we are dropping spec.foo and then spec given that it is an empty map + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + dropDiff(tt.ctx) + + g.Expect(tt.ctx.modified).To(Equal(tt.wantModified)) + }) + } +} diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun.go b/internal/controllers/topology/cluster/structuredmerge/dryrun.go new file mode 100644 index 000000000000..37c2abff8b4c --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun.go @@ -0,0 +1,302 @@ +/* +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 new file mode 100644 index 000000000000..8b08c520335d --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go @@ -0,0 +1,888 @@ +/* +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/filterintent.go b/internal/controllers/topology/cluster/structuredmerge/filterintent.go new file mode 100644 index 000000000000..2b743db040d2 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/filterintent.go @@ -0,0 +1,134 @@ +/* +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 ( + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +// filterObject filter out changes not relevant for the topology controller. +func filterObject(obj *unstructured.Unstructured, helperOptions *HelperOptions) { + // filter out changes not in the allowed paths (fields to not consider, e.g. status); + if len(helperOptions.allowedPaths) > 0 { + filterIntent(&filterIntentInput{ + path: contract.Path{}, + value: obj.Object, + shouldFilter: isNotAllowedPath(helperOptions.allowedPaths), + }) + } + + // filter out changes for ignore paths (well known fields owned by other controllers, e.g. + // spec.controlPlaneEndpoint in the InfrastructureCluster object); + if len(helperOptions.ignorePaths) > 0 { + filterIntent(&filterIntentInput{ + path: contract.Path{}, + value: obj.Object, + shouldFilter: isIgnorePath(helperOptions.ignorePaths), + }) + } +} + +// filterIntent ensures that object only includes the fields and values for which the topology controller has an opinion, +// and filter out everything else by removing it from the unstructured object. +// NOTE: This func is called recursively only for fields of type Map, but this is ok given the current use cases +// this func has to address. More specifically, we are using this func for filtering out not allowed paths and for ignore paths; +// all of them are defined in reconcile_state.go and are targeting well-known fields inside nested maps. +// Allowed paths / ignore paths which point to an array are not supported by the current implementation. +func filterIntent(ctx *filterIntentInput) bool { + value, ok := ctx.value.(map[string]interface{}) + if !ok { + return false + } + + gotDeletions := false + for field := range value { + fieldCtx := &filterIntentInput{ + // Compose the path for the nested field. + path: ctx.path.Append(field), + // Gets the original and the modified value for the field. + value: value[field], + // Carry over global values from the context. + shouldFilter: ctx.shouldFilter, + } + + // If the field should be filtered out, delete it from the modified object. + if fieldCtx.shouldFilter(fieldCtx.path) { + delete(value, field) + gotDeletions = true + continue + } + + // Process nested fields and get in return if filterIntent removed fields. + if filterIntent(fieldCtx) { + // Ensure we are not leaving empty maps around. + if v, ok := fieldCtx.value.(map[string]interface{}); ok && len(v) == 0 { + delete(value, field) + } + } + } + return gotDeletions +} + +// filterIntentInput holds info required while filtering the intent for server side apply. +// NOTE: in server side apply an intent is a partial object that only includes the fields and values for which the user has an opinion. +type filterIntentInput struct { + // the path of the field being processed. + path contract.Path + + // the value for the current path. + value interface{} + + // shouldFilter handle the func that determine if the current path should be dropped or not. + shouldFilter func(path contract.Path) bool +} + +// isAllowedPath returns true when the path is one of the allowedPaths. +func isAllowedPath(allowedPaths []contract.Path) func(path contract.Path) bool { + return func(path contract.Path) bool { + for _, p := range allowedPaths { + // NOTE: we allow everything Equal or one IsParentOf one of the allowed paths. + // e.g. if allowed path is metadata.labels, we allow both metadata and metadata.labels; + // this is required because allowed path is called recursively. + if path.Overlaps(p) { + return true + } + } + return false + } +} + +// isNotAllowedPath returns true when the path is NOT one of the allowedPaths. +func isNotAllowedPath(allowedPaths []contract.Path) func(path contract.Path) bool { + return func(path contract.Path) bool { + isAllowed := isAllowedPath(allowedPaths) + return !isAllowed(path) + } +} + +// isIgnorePath returns true when the path is one of the ignorePaths. +func isIgnorePath(ignorePaths []contract.Path) func(path contract.Path) bool { + return func(path contract.Path) bool { + for _, p := range ignorePaths { + if path.Equal(p) { + return true + } + } + return false + } +} diff --git a/internal/controllers/topology/cluster/structuredmerge/filterintent_test.go b/internal/controllers/topology/cluster/structuredmerge/filterintent_test.go new file mode 100644 index 000000000000..30be02e21277 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/filterintent_test.go @@ -0,0 +1,180 @@ +/* +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_filterNotAllowedPaths(t *testing.T) { + tests := []struct { + name string + ctx *filterIntentInput + wantValue map[string]interface{} + }{ + { + name: "Filters out not allowed paths", + ctx: &filterIntentInput{ + path: contract.Path{}, + value: map[string]interface{}{ + "apiVersion": "foo.bar/v1", + "kind": "Foo", + "metadata": map[string]interface{}{ + "name": "foo", + "namespace": "bar", + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + "resourceVersion": "123", + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + "status": map[string]interface{}{ + "foo": "123", + }, + }, + shouldFilter: isNotAllowedPath( + []contract.Path{ // NOTE: we are dropping everything not in this list + {"apiVersion"}, + {"kind"}, + {"metadata", "name"}, + {"metadata", "namespace"}, + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"spec"}, + }, + ), + }, + wantValue: map[string]interface{}{ + "apiVersion": "foo.bar/v1", + "kind": "Foo", + "metadata": map[string]interface{}{ + "name": "foo", + "namespace": "bar", + "labels": map[string]interface{}{ + "foo": "123", + }, + "annotations": map[string]interface{}{ + "foo": "123", + }, + // metadata.resourceVersion filtered out + }, + "spec": map[string]interface{}{ + "foo": "123", + }, + // status filtered out + }, + }, + { + name: "Cleanup empty maps", + ctx: &filterIntentInput{ + path: contract.Path{}, + value: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "123", + }, + }, + shouldFilter: isNotAllowedPath( + []contract.Path{}, // NOTE: we are filtering out everything not in this list (everything) + ), + }, + wantValue: map[string]interface{}{ + // we are filtering out spec.foo and then spec given that it is an empty map + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + filterIntent(tt.ctx) + + g.Expect(tt.ctx.value).To(Equal(tt.wantValue)) + }) + } +} + +func Test_filterIgnoredPaths(t *testing.T) { + tests := []struct { + name string + ctx *filterIntentInput + wantValue map[string]interface{} + }{ + { + name: "Filters out ignore paths", + ctx: &filterIntentInput{ + path: contract.Path{}, + value: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + "controlPlaneEndpoint": map[string]interface{}{ + "host": "foo-changed", + "port": "123-changed", + }, + }, + }, + shouldFilter: isIgnorePath( + []contract.Path{ + {"spec", "controlPlaneEndpoint"}, + }, + ), + }, + wantValue: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "bar", + // spec.controlPlaneEndpoint filtered out + }, + }, + }, + { + name: "Cleanup empty maps", + ctx: &filterIntentInput{ + path: contract.Path{}, + value: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "123", + }, + }, + shouldFilter: isIgnorePath( + []contract.Path{ + {"spec", "foo"}, + }, + ), + }, + wantValue: map[string]interface{}{ + // we are filtering out spec.foo and then spec given that it is an empty map + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + filterIntent(tt.ctx) + + g.Expect(tt.ctx.value).To(Equal(tt.wantValue)) + }) + } +} diff --git a/internal/controllers/topology/cluster/structuredmerge/interfaces.go b/internal/controllers/topology/cluster/structuredmerge/interfaces.go new file mode 100644 index 000000000000..28e1dbc5dd2b --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/interfaces.go @@ -0,0 +1,42 @@ +/* +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 ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// PatchHelperFactoryFunc defines a func that returns a new PatchHelper. +type PatchHelperFactoryFunc func(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. +// NOTE: this interface is required to allow to plug in different PatchHelper implementations, because the +// default one is based on server side apply, and it cannot be used for topology dry run, so we have a +// minimal viable replacement based on two-ways merge. +type PatchHelper interface { + // HasChanges return true if the modified object is generating changes vs the original object. + HasChanges() bool + + // HasSpecChanges return true if the modified object is generating spec changes vs the original object. + HasSpecChanges() bool + + // Patch patches the given obj in the Kubernetes cluster. + Patch(ctx context.Context) error +} diff --git a/internal/controllers/topology/cluster/mergepatch/options.go b/internal/controllers/topology/cluster/structuredmerge/options.go similarity index 70% rename from internal/controllers/topology/cluster/mergepatch/options.go rename to internal/controllers/topology/cluster/structuredmerge/options.go index 86d74dbc21ea..26f0e9d13f1b 100644 --- a/internal/controllers/topology/cluster/mergepatch/options.go +++ b/internal/controllers/topology/cluster/structuredmerge/options.go @@ -1,5 +1,5 @@ /* -Copyright 2021 The Kubernetes Authors. +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. @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package mergepatch +package structuredmerge import ( "sigs.k8s.io/cluster-api/internal/contract" @@ -29,12 +29,17 @@ type HelperOption interface { // HelperOptions contains options for Helper. type HelperOptions struct { // internally managed options. + + // allowedPaths instruct the Helper to ignore everything except given paths when computing a patch. allowedPaths []contract.Path - managedPaths []contract.Path // user defined options. - ignorePaths []contract.Path - authoritativePaths []contract.Path + + // IgnorePaths instruct the Helper to ignore given paths when computing a patch. + // NOTE: ignorePaths are used to filter out fields nested inside allowedPaths, e.g. + // spec.ControlPlaneEndpoint. + // NOTE: ignore paths which point to an array are not supported by the current implementation. + ignorePaths []contract.Path } // ApplyOptions applies the given patch options on these options, @@ -47,19 +52,11 @@ func (o *HelperOptions) ApplyOptions(opts []HelperOption) *HelperOptions { } // IgnorePaths instruct the Helper to ignore given paths when computing a patch. +// NOTE: ignorePaths are used to filter out fields nested inside allowedPaths, e.g. +// spec.ControlPlaneEndpoint. type IgnorePaths []contract.Path // ApplyToHelper applies this configuration to the given helper options. func (i IgnorePaths) ApplyToHelper(opts *HelperOptions) { opts.ignorePaths = i } - -// AuthoritativePaths instruct the Helper to enforce changes for paths when computing a patch -// (instead of using two way merge that preserves values from existing objects). -// NOTE: AuthoritativePaths will be superseded by IgnorePaths in case a path exists in both. -type AuthoritativePaths []contract.Path - -// ApplyToHelper applies this configuration to the given helper options. -func (i AuthoritativePaths) ApplyToHelper(opts *HelperOptions) { - opts.authoritativePaths = i -} diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go new file mode 100644 index 000000000000..ed09aa500aa0 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -0,0 +1,197 @@ +/* +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" + + "github.com/pkg/errors" + "golang.org/x/net/context" + 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/internal/contract" +) + +// TopologyManagerName is the manager name in managed fields for the topology controller. +const TopologyManagerName = "capi-topology" + +type serverSidePatchHelper struct { + client client.Client + modified *unstructured.Unstructured + hasChanges bool + hasSpecChanges bool +} + +// NewServerSidePatchHelper returns a new PatchHelper using server side apply. +func NewServerSidePatchHelper(original, modified client.Object, c client.Client, opts ...HelperOption) (PatchHelper, error) { + helperOptions := &HelperOptions{} + helperOptions = helperOptions.ApplyOptions(opts) + helperOptions.allowedPaths = []contract.Path{ + // apiVersion, kind, name and namespace are required field for a server side apply intent. + {"apiVersion"}, + {"kind"}, + {"metadata", "name"}, + {"metadata", "namespace"}, + // the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only. + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"metadata", "ownerReferences"}, + {"spec"}, + } + + // 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 + if !isNil(original) { + originalUnstructured = &unstructured.Unstructured{} + switch original.(type) { + case *unstructured.Unstructured: + originalUnstructured = original.DeepCopyObject().(*unstructured.Unstructured) + default: + if err := c.Scheme().Convert(original, originalUnstructured, nil); err != nil { + 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(originalUnstructured, c); err != nil { + return nil, errors.Wrap(err, "failed to cleanup legacy managed fields from original object") + } + } + + filterObject(originalUnstructured, helperOptions) + } + + modifiedUnstructured := &unstructured.Unstructured{} + switch modified.(type) { + case *unstructured.Unstructured: + modifiedUnstructured = modified.DeepCopyObject().(*unstructured.Unstructured) + default: + if err := c.Scheme().Convert(modified, modifiedUnstructured, nil); err != nil { + return nil, errors.Wrap(err, "failed to convert modified object to Unstructured") + } + } + filterObject(modifiedUnstructured, helperOptions) + + // 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. + var hasChanges, hasSpecChanges bool + switch { + case isNil(original): + hasChanges, hasSpecChanges = true, true + default: + hasChanges, hasSpecChanges = dryRunPatch(&dryRunInput{ + path: contract.Path{}, + fieldsV1: getTopologyManagedFields(original), + original: originalUnstructured.Object, + modified: modifiedUnstructured.Object, + }) + } + + return &serverSidePatchHelper{ + client: c, + modified: modifiedUnstructured, + hasChanges: hasChanges, + hasSpecChanges: hasSpecChanges, + }, nil +} + +// HasSpecChanges return true if the patch has changes to the spec field. +func (h *serverSidePatchHelper) HasSpecChanges() bool { + return h.hasSpecChanges +} + +// HasChanges return true if the patch has changes. +func (h *serverSidePatchHelper) HasChanges() bool { + return h.hasChanges +} + +// Patch will server side apply the current intent (the modified object. +func (h *serverSidePatchHelper) Patch(ctx context.Context) error { + if !h.HasChanges() { + return nil + } + + log := ctrl.LoggerFrom(ctx) + log.V(5).Info("Patching object", "Intent", h.modified) + + options := []client.PatchOption{ + client.FieldOwner(TopologyManagerName), + // NOTE: we are using force ownership so in case of conflicts the topology controller + // overwrite values and become sole manager. + client.ForceOwnership, + } + 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(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(context.TODO(), obj, client.MergeFrom(base)) +} diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go new file mode 100644 index 000000000000..38754108af98 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -0,0 +1,446 @@ +/* +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" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/util/patch" +) + +// NOTE: This test ensures the ServerSideApply works as expected when the object is co-authored by other controllers. +func TestServerSideApply(t *testing.T) { + g := NewWithT(t) + + // Write the config file to access the test env for debugging. + // g.Expect(os.WriteFile("test.conf", kubeconfig.FromEnvTestConfig(env.Config, &clusterv1.Cluster{ + // ObjectMeta: metav1.ObjectMeta{Name: "test"}, + // }), 0777)).To(Succeed()) + + // Create a namespace for running the test + ns, err := env.CreateNamespace(ctx, "ssa") + g.Expect(err).ToNot(HaveOccurred()) + + // Build the test object to work with. + obj := builder.TestInfrastructureCluster(ns.Name, "obj1").WithSpecFields(map[string]interface{}{ + "spec.controlPlaneEndpoint.host": "1.2.3.4", + "spec.controlPlaneEndpoint.port": int64(1234), + "spec.foo": "", // this field is then explicitly ignored by the patch helper + }).Build() + g.Expect(unstructured.SetNestedField(obj.Object, "", "status", "foo")).To(Succeed()) // this field is then ignored by the patch helper (not allowed path). + + t.Run("Server side apply detect changes on object creation (unstructured)", func(t *testing.T) { + g := NewWithT(t) + + var original *unstructured.Unstructured + modified := obj.DeepCopy() + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + }) + t.Run("Server side apply detect changes on object creation (typed)", func(t *testing.T) { + g := NewWithT(t) + + var original *clusterv1.MachineDeployment + modified := obj.DeepCopy() + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + }) + + t.Run("When creating an object using server side apply, it should track managed fields for the topology controller", func(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"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + + // Create the object using server side apply + g.Expect(p0.Patch(ctx)).To(Succeed()) + + // Check the object and verify managed field are properly set. + got := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + fieldV1 := getTopologyManagedFields(got) + g.Expect(fieldV1).ToNot(BeEmpty()) + g.Expect(fieldV1).To(HaveKey("f:spec")) // topology controller should express opinions on spec. + g.Expect(fieldV1).ToNot(HaveKey("f:status")) // topology controller should not express opinions on status/not allowed paths. + + specFieldV1 := fieldV1["f:spec"].(map[string]interface{}) + g.Expect(specFieldV1).ToNot(BeEmpty()) + g.Expect(specFieldV1).To(HaveKey("f:controlPlaneEndpoint")) // topology controller should express opinions on spec.controlPlaneEndpoint. + g.Expect(specFieldV1).ToNot(HaveKey("f:foo")) // topology controller should not express opinions on ignore paths. + + controlPlaneEndpointFieldV1 := specFieldV1["f:controlPlaneEndpoint"].(map[string]interface{}) + 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. + }) + + t.Run("Server side apply patch helper detects no changes", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with no changes. + modified := obj.DeepCopy() + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Server side apply patch helper discard changes in not allowed fields, e.g. status", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes only in status. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "status", "foo")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Server side apply patch helper detect changes", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes in spec. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "bar")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + }) + + t.Run("Server side apply patch helper detect changes impacting only metadata.labels", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes only in metadata. + modified := obj.DeepCopy() + modified.SetLabels(map[string]string{"foo": "changed"}) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Server side apply patch helper detect changes impacting only metadata.annotations", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes only in metadata. + modified := obj.DeepCopy() + modified.SetAnnotations(map[string]string{"foo": "changed"}) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Server side apply patch helper detect changes impacting only metadata.ownerReferences", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes only in metadata. + modified := obj.DeepCopy() + modified.SetOwnerReferences([]metav1.OwnerReference{ + { + APIVersion: "foo/v1alpha1", + Kind: "foo", + Name: "foo", + }, + }) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Server side apply patch helper discard changes in ignore paths", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with changes only in an ignoredField. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "foo")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) + + t.Run("Another controller applies changes", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + obj := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + + // Create a patch helper like we do/recommend doing in the controllers and use it to apply some changes. + p, err := patch.NewHelper(obj, env.Client) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(unstructured.SetNestedField(obj.Object, "changed", "spec", "foo")).To(Succeed()) // Controller sets a well known field ignored in the topology controller + g.Expect(unstructured.SetNestedField(obj.Object, "changed", "spec", "bar")).To(Succeed()) // Controller sets an infra specific field the topology controller is not aware of + g.Expect(unstructured.SetNestedField(obj.Object, "changed", "status", "foo")).To(Succeed()) // Controller sets something in status + g.Expect(unstructured.SetNestedField(obj.Object, true, "status", "ready")).To(Succeed()) // Required field + + g.Expect(p.Patch(ctx, obj)).To(Succeed()) + }) + + t.Run("Topology controller reconcile again with no changes on topology managed fields", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with no changes to what previously applied by th topology manager. + modified := obj.DeepCopy() + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + + // Create the object using server side apply + 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()) + + v1, _, _ := unstructured.NestedString(got.Object, "spec", "foo") + g.Expect(v1).To(Equal("changed")) + v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") + g.Expect(v2).To(Equal("changed")) + v3, _, _ := unstructured.NestedString(got.Object, "status", "foo") + g.Expect(v3).To(Equal("changed")) + v4, _, _ := unstructured.NestedBool(got.Object, "status", "ready") + g.Expect(v4).To(Equal(true)) + + fieldV1 := getTopologyManagedFields(got) + g.Expect(fieldV1).ToNot(BeEmpty()) + g.Expect(fieldV1).To(HaveKey("f:spec")) // topology controller should express opinions on spec. + g.Expect(fieldV1).ToNot(HaveKey("f:status")) // topology controller should not express opinions on status/not allowed paths. + + specFieldV1 := fieldV1["f:spec"].(map[string]interface{}) + g.Expect(specFieldV1).ToNot(BeEmpty()) + g.Expect(specFieldV1).To(HaveKey("f:controlPlaneEndpoint")) // topology controller should express opinions on spec.controlPlaneEndpoint. + g.Expect(specFieldV1).ToNot(HaveKey("f:foo")) // topology controller should not express opinions on ignore paths. + g.Expect(specFieldV1).ToNot(HaveKey("f:bar")) // topology controller should not express opinions on fields managed by other controllers. + }) + + t.Run("Topology controller reconcile again with some changes on topology managed fields", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with some changes to what previously applied by th topology manager. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + + // Create the object using server side apply + g.Expect(p0.Patch(ctx)).To(Succeed()) + + // Check the object and verify the change is applied as well as 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()) + + v0, _, _ := unstructured.NestedString(got.Object, "spec", "controlPlaneEndpoint", "host") + g.Expect(v0).To(Equal("changed")) + v1, _, _ := unstructured.NestedString(got.Object, "spec", "foo") + g.Expect(v1).To(Equal("changed")) + v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") + g.Expect(v2).To(Equal("changed")) + v3, _, _ := unstructured.NestedString(got.Object, "status", "foo") + g.Expect(v3).To(Equal("changed")) + v4, _, _ := unstructured.NestedBool(got.Object, "status", "ready") + g.Expect(v4).To(Equal(true)) + }) + + t.Run("Topology controller reconcile again with an opinion on a field managed by another controller (force ownership)", func(t *testing.T) { + g := NewWithT(t) + + // Get the current object (assumes tests to be run in sequence). + original := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed()) + + // Create a patch helper for a modified object with some changes to what previously applied by th topology manager. + modified := obj.DeepCopy() + g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed()) + g.Expect(unstructured.SetNestedField(modified.Object, "changed-by-topology-controller", "spec", "bar")).To(Succeed()) + + p0, err := NewServerSidePatchHelper(original, modified, env.GetClient(), IgnorePaths{{"spec", "foo"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeTrue()) + g.Expect(p0.HasSpecChanges()).To(BeTrue()) + + // Create the object using server side apply + g.Expect(p0.Patch(ctx)).To(Succeed()) + + // Check the object and verify the change is applied as well as managed field updated accordingly. + got := obj.DeepCopy() + g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(got), got)).To(Succeed()) + + v2, _, _ := unstructured.NestedString(got.Object, "spec", "bar") + g.Expect(v2).To(Equal("changed-by-topology-controller")) + + fieldV1 := getTopologyManagedFields(got) + g.Expect(fieldV1).ToNot(BeEmpty()) + g.Expect(fieldV1).To(HaveKey("f:spec")) // topology controller should express opinions on spec. + + specFieldV1 := fieldV1["f:spec"].(map[string]interface{}) + g.Expect(specFieldV1).ToNot(BeEmpty()) + g.Expect(specFieldV1).To(HaveKey("f:controlPlaneEndpoint")) // topology controller should express opinions on spec.controlPlaneEndpoint. + g.Expect(specFieldV1).ToNot(HaveKey("f:foo")) // topology controller should not express opinions on ignore paths. + g.Expect(specFieldV1).To(HaveKey("f:bar")) // topology controller now has an opinion on a field previously managed by other controllers (force ownership). + }) + t.Run("No-op on unstructured object having empty map[string]interface in spec", func(t *testing.T) { + g := NewWithT(t) + + obj2 := builder.TestInfrastructureCluster(ns.Name, "obj2"). + WithSpecFields(map[string]interface{}{ + "spec.fooMap": map[string]interface{}{}, + "spec.fooList": []interface{}{}, + }). + Build() + + // create new object having an empty map[string]interface in spec and a copy of it for further testing + original := obj2.DeepCopy() + modified := obj2.DeepCopy() + + // Create the object using server side apply + g.Expect(env.PatchAndWait(ctx, original, client.FieldOwner(TopologyManagerName))).To(Succeed()) + // Get created object to have managed fields + 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()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) + }) +} + +func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) { + g := NewWithT(t) + + // Write the config file to access the test env for debugging. + // g.Expect(os.WriteFile("test.conf", kubeconfig.FromEnvTestConfig(env.Config, &clusterv1.Cluster{ + // ObjectMeta: metav1.ObjectMeta{Name: "test"}, + // }), 0777)).To(Succeed()) + + // Create a namespace for running the test + ns, err := env.CreateNamespace(ctx, "ssa") + g.Expect(err).ToNot(HaveOccurred()) + + // 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(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()) + }) +} diff --git a/internal/controllers/topology/cluster/structuredmerge/suite_test.go b/internal/controllers/topology/cluster/structuredmerge/suite_test.go new file mode 100644 index 000000000000..5991ecd5bf9d --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/suite_test.go @@ -0,0 +1,55 @@ +/* +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 ( + "os" + "testing" + "time" + + . "github.com/onsi/gomega" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/test/envtest" +) + +var ( + ctx = ctrl.SetupSignalHandler() + fakeScheme = runtime.NewScheme() + env *envtest.Environment +) + +func init() { + _ = clientgoscheme.AddToScheme(fakeScheme) + _ = clusterv1.AddToScheme(fakeScheme) + _ = apiextensionsv1.AddToScheme(fakeScheme) +} + +func TestMain(m *testing.M) { + SetDefaultEventuallyPollingInterval(100 * time.Millisecond) + SetDefaultEventuallyTimeout(30 * time.Second) + os.Exit(envtest.Run(ctx, envtest.RunInput{ + M: m, + SetupEnv: func(e *envtest.Environment) { env = e }, + // We are testing the patch helper against a real API Server, no need of additional indexes/reconcilers. + MinK8sVersion: "v1.22.0", // ClusterClass uses server side apply that went GA in 1.22; we do not support previous version because of bug/inconsistent behaviours in the older release. + })) +} diff --git a/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go b/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go new file mode 100644 index 000000000000..78771290bf87 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper.go @@ -0,0 +1,239 @@ +/* +Copyright 2021 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 ( + "bytes" + "context" + "encoding/json" + "reflect" + + jsonpatch "github.com/evanphx/json-patch/v5" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + "sigs.k8s.io/cluster-api/internal/contract" +) + +// TwoWaysPatchHelper helps with a patch that yields the modified document when applied to the original document. +type TwoWaysPatchHelper struct { + client client.Client + + // original holds the object to which the patch should apply to, to be used in the Patch method. + original client.Object + + // patch holds the merge patch in json format. + patch []byte + + // hasSpecChanges documents if the patch impacts the object spec + hasSpecChanges bool +} + +// NewTwoWaysPatchHelper will return a patch that yields the modified document when applied to the original document +// using the two-ways merge algorithm. +// NOTE: In the case of ClusterTopologyReconciler, original is the current object, modified is the desired object, and +// the patch returns all the changes required to align current to what is defined in desired; fields not managed +// by the topology controller are going to be preserved without changes. +// NOTE: TwoWaysPatch is considered a minimal viable replacement for server side apply during topology dry run, with +// the following limitations: +// - TwoWaysPatch doesn't consider OpenAPI schema extension like +ListMap this can lead to false positive when topology +// dry run is simulating a change to an existing slice +// (TwoWaysPatch always revert external changes, like server side apply when +ListMap=atomic). +// - TwoWaysPatch doesn't consider existing metadata.managedFields, and this can lead to false negative when topology dry run +// is simulating a change to an existing object where the topology controller is dropping an opinion for a field +// (TwoWaysPatch always preserve dropped fields, like server side apply when the field has more than one manager). +// - TwoWaysPatch doesn't generate metadata.managedFields as server side apply does. +// NOTE: NewTwoWaysPatchHelper consider changes only in metadata.labels, metadata.annotation and spec; it also respects +// the ignorePath option (same as the server side apply helper). +func NewTwoWaysPatchHelper(original, modified client.Object, c client.Client, opts ...HelperOption) (*TwoWaysPatchHelper, error) { + helperOptions := &HelperOptions{} + helperOptions = helperOptions.ApplyOptions(opts) + helperOptions.allowedPaths = []contract.Path{ + {"metadata", "labels"}, + {"metadata", "annotations"}, + {"spec"}, // NOTE: The handling of managed path requires/assumes spec to be within allowed path. + } + // In case we are creating an object, we extend the set of allowed fields adding apiVersion, Kind + // metadata.name, metadata.namespace (who are required by the API server) and metadata.ownerReferences + // that gets set to avoid orphaned objects. + if isNil(original) { + helperOptions.allowedPaths = append(helperOptions.allowedPaths, + contract.Path{"apiVersion"}, + contract.Path{"kind"}, + contract.Path{"metadata", "name"}, + contract.Path{"metadata", "namespace"}, + contract.Path{"metadata", "ownerReferences"}, + ) + } + + // Convert the input objects to json; if original is nil, use empty object so the + // following logic works without panicking. + originalJSON, err := json.Marshal(original) + if err != nil { + return nil, errors.Wrap(err, "failed to marshal original object to json") + } + if isNil(original) { + originalJSON = []byte("{}") + } + + modifiedJSON, err := json.Marshal(modified) + if err != nil { + return nil, errors.Wrap(err, "failed to marshal modified object to json") + } + + // Apply patch options including: + // - exclude paths (fields to not consider, e.g. status); + // - ignore paths (well known fields owned by something else, e.g. spec.controlPlaneEndpoint in the + // InfrastructureCluster object); + // NOTE: All the above options trigger changes in the modified object so the resulting two ways patch + // includes or not the specific change. + modifiedJSON, err = applyOptions(&applyOptionsInput{ + original: originalJSON, + modified: modifiedJSON, + options: helperOptions, + }) + if err != nil { + return nil, errors.Wrap(err, "failed to apply options to modified") + } + + // Apply the modified object to the original one, merging the values of both; + // in case of conflicts, values from the modified object are preserved. + originalWithModifiedJSON, err := jsonpatch.MergePatch(originalJSON, modifiedJSON) + if err != nil { + return nil, errors.Wrap(err, "failed to apply modified json to original json") + } + + // Compute the merge patch that will align the original object to the target + // state defined above. + twoWayPatch, err := jsonpatch.CreateMergePatch(originalJSON, originalWithModifiedJSON) + if err != nil { + return nil, errors.Wrap(err, "failed to create merge patch") + } + + twoWayPatchMap := make(map[string]interface{}) + if err := json.Unmarshal(twoWayPatch, &twoWayPatchMap); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal two way merge patch") + } + + // check if the changes impact the spec field. + hasSpecChanges := twoWayPatchMap["spec"] != nil + + return &TwoWaysPatchHelper{ + client: c, + patch: twoWayPatch, + hasSpecChanges: hasSpecChanges, + original: original, + }, nil +} + +type applyOptionsInput struct { + original []byte + modified []byte + options *HelperOptions +} + +// Apply patch options changing the modified object so the resulting two ways patch +// includes or not the specific change. +func applyOptions(in *applyOptionsInput) ([]byte, error) { + originalMap := make(map[string]interface{}) + if err := json.Unmarshal(in.original, &originalMap); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal original") + } + + modifiedMap := make(map[string]interface{}) + if err := json.Unmarshal(in.modified, &modifiedMap); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal modified") + } + + // drop changes for exclude paths (fields to not consider, e.g. status); + // Note: for everything not allowed it sets modified equal to original, so the generated patch doesn't include this change + if len(in.options.allowedPaths) > 0 { + dropDiff(&dropDiffInput{ + path: contract.Path{}, + original: originalMap, + modified: modifiedMap, + shouldDropDiffFunc: isNotAllowedPath(in.options.allowedPaths), + }) + } + + // drop changes for ignore paths (well known fields owned by something else, e.g. + // spec.controlPlaneEndpoint in the InfrastructureCluster object); + // Note: for everything ignored it sets modified equal to original, so the generated patch doesn't include this change + if len(in.options.ignorePaths) > 0 { + dropDiff(&dropDiffInput{ + path: contract.Path{}, + original: originalMap, + modified: modifiedMap, + shouldDropDiffFunc: isIgnorePath(in.options.ignorePaths), + }) + } + + modified, err := json.Marshal(&modifiedMap) + if err != nil { + return nil, errors.Wrap(err, "failed to marshal modified") + } + + return modified, nil +} + +// HasSpecChanges return true if the patch has changes to the spec field. +func (h *TwoWaysPatchHelper) HasSpecChanges() bool { + return h.hasSpecChanges +} + +// HasChanges return true if the patch has changes. +func (h *TwoWaysPatchHelper) HasChanges() bool { + return !bytes.Equal(h.patch, []byte("{}")) +} + +// Patch will attempt to apply the twoWaysPatch to the original object. +func (h *TwoWaysPatchHelper) Patch(ctx context.Context) error { + if !h.HasChanges() { + return nil + } + log := ctrl.LoggerFrom(ctx) + + if isNil(h.original) { + modifiedMap := make(map[string]interface{}) + if err := json.Unmarshal(h.patch, &modifiedMap); err != nil { + return errors.Wrap(err, "failed to unmarshal two way merge patch") + } + + obj := &unstructured.Unstructured{ + Object: modifiedMap, + } + return h.client.Create(ctx, obj) + } + + // Note: deepcopy before patching in order to avoid modifications to the original object. + log.V(5).Info("Patching object", "Patch", string(h.patch)) + return h.client.Patch(ctx, h.original.DeepCopyObject().(client.Object), client.RawPatch(types.MergePatchType, h.patch)) +} + +func isNil(i interface{}) bool { + if i == nil { + return true + } + switch reflect.TypeOf(i).Kind() { + case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice: + return reflect.ValueOf(i).IsValid() && reflect.ValueOf(i).IsNil() + } + return false +} diff --git a/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper_test.go new file mode 100644 index 000000000000..fd35f0e99059 --- /dev/null +++ b/internal/controllers/topology/cluster/structuredmerge/twowayspatchhelper_test.go @@ -0,0 +1,452 @@ +/* +Copyright 2021 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 ( + "fmt" + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "sigs.k8s.io/cluster-api/internal/contract" + "sigs.k8s.io/cluster-api/internal/test/builder" +) + +func TestNewHelper(t *testing.T) { + tests := []struct { + name string + original *unstructured.Unstructured // current + modified *unstructured.Unstructured // desired + options []HelperOption + wantHasChanges bool + wantHasSpecChanges bool + wantPatch []byte + }{ + // Create + + { + name: "Create if original does not exists", + original: nil, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "apiVersion": builder.BootstrapGroupVersion.String(), + "kind": builder.GenericBootstrapConfigKind, + "metadata": map[string]interface{}{ + "namespace": metav1.NamespaceDefault, + "name": "foo", + }, + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + options: []HelperOption{}, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte(fmt.Sprintf("{\"apiVersion\":%q,\"kind\":%q,\"metadata\":{\"name\":\"foo\",\"namespace\":%q},\"spec\":{\"foo\":\"foo\"}}", builder.BootstrapGroupVersion.String(), builder.GenericBootstrapConfigKind, metav1.NamespaceDefault)), + }, + + // Ignore fields + + { + name: "Ignore fields are removed from the patch", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "controlPlaneEndpoint": map[string]interface{}{ + "host": "", + "port": int64(0), + }, + }, + }, + }, + options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}}, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + + // Allowed Path fields + + { + name: "Not allowed fields are removed from the patch", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "status": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + + // Field both in original and in modified --> align to modified if different + + { + name: "Field (spec.foo) both in original and in modified, no-op when equal", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + { + name: "Field (metadata.label) both in original and in modified, align to modified when different", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo-modified", + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"foo-modified\"}}}"), + }, + { + name: "Field (spec.template.spec.foo) both in original and in modified, no-op when equal", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + + { + name: "Field (spec.foo) both in original and in modified, align to modified when different", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"foo\":\"foo-changed\"}}"), + }, + { + name: "Field (metadata.label) both in original and in modified, align to modified when different", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"foo-changed\"}}}"), + }, + { + name: "Field (spec.template.spec.foo) both in original and in modified, align to modified when different", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"foo\":\"foo-changed\"}}}}"), + }, + + { + name: "Value of type Array or Slice both in original and in modified,, align to modified when different", // Note: fake treats all the slice as atomic (false positive) + original: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "slice": []interface{}{ + "D", + "C", + "B", + }, + }, + }, + }, + modified: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "slice": []interface{}{ + "A", + "B", + "C", + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"), + }, + + // Field only in modified (not existing in original) --> align to modified + + { + name: "Field (spec.foo) in modified only, align to modified", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"foo\":\"foo-changed\"}}"), + }, + { + name: "Field (metadata.label) in modified only, align to modified", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: false, + wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"foo-changed\"}}}"), + }, + { + name: "Field (spec.template.spec.foo) in modified only, align to modified when different", + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo-changed", + }, + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"foo\":\"foo-changed\"}}}}"), + }, + + { + name: "Value of type Array or Slice in modified only, align to modified when different", + original: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + modified: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "slice": []interface{}{ + "A", + "B", + "C", + }, + }, + }, + }, + wantHasChanges: true, + wantHasSpecChanges: true, + wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"), + }, + + // Field only in original (not existing in modified) --> preserve original + + { + name: "Field (spec.foo) in original only, preserve", // Note: fake can't detect if has been originated from templates or from external controllers, so it assumes (false negative) + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{}, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + { + name: "Field (metadata.label) in original only, preserve", // Note: fake can't detect if has been originated from templates or from external controllers (false negative) + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{}, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + { + name: "Field (spec.template.spec.foo) in original only, preserve", // Note: fake can't detect if has been originated from templates or from external controllers (false negative) + original: &unstructured.Unstructured{ // current + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "foo": "foo", + }, + }, + }, + }, + }, + modified: &unstructured.Unstructured{ // desired + Object: map[string]interface{}{}, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + + { + name: "Value of type Array or Slice in original only, preserve", // Note: fake can't detect if has been originated from templates or from external controllers (false negative) + original: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "slice": []interface{}{ + "D", + "C", + "B", + }, + }, + }, + }, + modified: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + wantHasChanges: false, + wantHasSpecChanges: false, + wantPatch: []byte("{}"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + patch, err := NewTwoWaysPatchHelper(tt.original, tt.modified, env.GetClient(), tt.options...) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(patch.patch).To(Equal(tt.wantPatch)) + g.Expect(patch.HasChanges()).To(Equal(tt.wantHasChanges)) + g.Expect(patch.HasSpecChanges()).To(Equal(tt.wantHasSpecChanges)) + }) + } +} diff --git a/internal/controllers/topology/cluster/suite_test.go b/internal/controllers/topology/cluster/suite_test.go index 53cfbdfa35c1..fec64edd3b35 100644 --- a/internal/controllers/topology/cluster/suite_test.go +++ b/internal/controllers/topology/cluster/suite_test.go @@ -91,10 +91,10 @@ func TestMain(m *testing.M) { SetDefaultEventuallyPollingInterval(100 * time.Millisecond) SetDefaultEventuallyTimeout(30 * time.Second) os.Exit(envtest.Run(ctx, envtest.RunInput{ - M: m, - ManagerUncachedObjs: []client.Object{}, - SetupEnv: func(e *envtest.Environment) { env = e }, - SetupIndexes: setupIndexes, - SetupReconcilers: setupReconcilers, + M: m, + SetupEnv: func(e *envtest.Environment) { env = e }, + SetupIndexes: setupIndexes, + SetupReconcilers: setupReconcilers, + MinK8sVersion: "v1.22.0", // ClusterClass uses server side apply that went GA in 1.22; we do not support previous version because of bug/inconsistent behaviours in the older release. })) } diff --git a/internal/controllers/topology/cluster/util_test.go b/internal/controllers/topology/cluster/util_test.go index b6da59f7eb55..accd83f15f89 100644 --- a/internal/controllers/topology/cluster/util_test.go +++ b/internal/controllers/topology/cluster/util_test.go @@ -101,6 +101,7 @@ func TestGetReference(t *testing.T) { r := &Reconciler{ Client: fakeClient, UnstructuredCachingClient: fakeClient, + patchHelperFactory: dryRunPatchHelperFactory(fakeClient), } got, err := r.getReference(ctx, tt.ref) if tt.wantErr { diff --git a/internal/test/builder/infrastructure.go b/internal/test/builder/infrastructure.go index 3ac4258cc6db..1f2857d39b0b 100644 --- a/internal/test/builder/infrastructure.go +++ b/internal/test/builder/infrastructure.go @@ -80,6 +80,24 @@ func testInfrastructureClusterCRD(gvk schema.GroupVersionKind) *apiextensionsv1. // General purpose fields to be used in different test scenario. "foo": {Type: "string"}, "bar": {Type: "string"}, + "fooMap": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "foo": {Type: "string"}, + }, + }, + "fooList": { + Type: "array", + Items: &apiextensionsv1.JSONSchemaPropsOrArray{ + Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "foo": {Type: "string"}, + }, + }, + }, + }, + // Field for testing }, }, "status": { diff --git a/internal/test/envtest/environment.go b/internal/test/envtest/environment.go index ecab906b7b50..c2bd921b8543 100644 --- a/internal/test/envtest/environment.go +++ b/internal/test/envtest/environment.go @@ -58,6 +58,7 @@ import ( "sigs.k8s.io/cluster-api/internal/test/builder" runtimewebhooks "sigs.k8s.io/cluster-api/internal/webhooks/runtime" "sigs.k8s.io/cluster-api/util/kubeconfig" + "sigs.k8s.io/cluster-api/version" "sigs.k8s.io/cluster-api/webhooks" ) @@ -90,6 +91,7 @@ type RunInput struct { SetupIndexes func(ctx context.Context, mgr ctrl.Manager) SetupReconcilers func(ctx context.Context, mgr ctrl.Manager) SetupEnv func(e *Environment) + MinK8sVersion string } // Run executes the tests of the given testing.M in a test environment. @@ -116,6 +118,16 @@ func Run(ctx context.Context, input RunInput) int { // Start the environment. env.start(ctx) + if input.MinK8sVersion != "" { + if err := version.CheckKubernetesVersion(env.Config, input.MinK8sVersion); err != nil { + fmt.Printf("[IMPORTANT] skipping tests after failing version check: %v\n", err) + if err := env.stop(); err != nil { + fmt.Println("[WARNING] Failed to stop the test environment") + } + return 0 + } + } + // Expose the environment. input.SetupEnv(env) @@ -286,7 +298,7 @@ func (e *Environment) start(ctx context.Context) { } }() <-e.Manager.Elected() - e.WaitForWebhooks() + e.waitForWebhooks() } // stop stops the test environment. @@ -296,8 +308,8 @@ func (e *Environment) stop() error { return e.env.Stop() } -// WaitForWebhooks waits for the webhook server to be available. -func (e *Environment) WaitForWebhooks() { +// waitForWebhooks waits for the webhook server to be available. +func (e *Environment) waitForWebhooks() { port := e.env.WebhookInstallOptions.LocalServingPort klog.V(2).Infof("Waiting for webhook port %d to be open prior to running tests", port) diff --git a/main.go b/main.go index 25443153b4af..39046126556c 100644 --- a/main.go +++ b/main.go @@ -219,6 +219,17 @@ func main() { restConfig := ctrl.GetConfigOrDie() restConfig.UserAgent = remote.DefaultClusterAPIUserAgent("cluster-api-controller-manager") + + minVer := version.MinimumKubernetesVersion + if feature.Gates.Enabled(feature.ClusterTopology) { + minVer = version.MinimumKubernetesVersionClusterTopology + } + + if err := version.CheckKubernetesVersion(restConfig, minVer); err != nil { + setupLog.Error(err, "unable to start manager") + os.Exit(1) + } + mgr, err := ctrl.NewManager(restConfig, ctrl.Options{ Scheme: scheme, MetricsBindAddress: metricsBindAddr, diff --git a/version/version.go b/version/version.go index 98ef2d61ff6f..62798a78c715 100644 --- a/version/version.go +++ b/version/version.go @@ -20,8 +20,41 @@ package version import ( "fmt" "runtime" + + "github.com/pkg/errors" + utilversion "k8s.io/apimachinery/pkg/util/version" + "k8s.io/client-go/discovery" + "k8s.io/client-go/rest" +) + +const ( + // MinimumKubernetesVersion defines the minimum Kubernetes version that can be used in a Management Cluster. + MinimumKubernetesVersion = "v1.20.0" + + // MinimumKubernetesVersionClusterTopology defines the minimum Kubernetes version that can be used in a + // Management Cluster when enabling the ClusterTopology feature gate. + MinimumKubernetesVersionClusterTopology = "v1.22.0" ) +// CheckKubernetesVersion return an error if the Kubernetes version in a cluster is lower than the specified minK8sVersion. +func CheckKubernetesVersion(config *rest.Config, minK8sVersion string) error { + client := discovery.NewDiscoveryClientForConfigOrDie(config) + serverVersion, err := client.ServerVersion() + if err != nil { + return errors.Wrap(err, "failed to get the Kubernetes version") + } + + compareResult, err := utilversion.MustParseGeneric(serverVersion.String()).Compare(minK8sVersion) + if err != nil { + return errors.Wrap(err, "failed to check MinK8sVersion") + } + + if compareResult == -1 { + return errors.Errorf("unsupported management cluster server version: %s - minimum required version is %s", serverVersion.String(), minK8sVersion) + } + return nil +} + var ( gitMajor string // major version, always numeric gitMinor string // minor version, numeric possibly followed by "+"