Use Server Side Apply in topology controller

Co-authored-by: chrischdi <schlotterc@vmware.com>

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 it 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 it 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 is 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

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))
+		})
+	}
+}

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)

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"
@@ -51,6 +52,8 @@ import (
 // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;create;delete
 
+type patchHelperFactoryFunc func(original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error)
+
 // Reconciler reconciles a managed topology for a Cluster object.
 type Reconciler struct {
 	Client client.Client
@@ -70,6 +73,8 @@ type Reconciler struct {
 
 	// patchEngine is used to apply patches during computeDesiredState.
 	patchEngine patches.Engine
+
+	patchHelperFactory patchHelperFactoryFunc
 }
 
 func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
@@ -102,13 +107,19 @@ 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
 }
 
+// Add two factory functions.
+
 // SetupForDryRun prepares the Reconciler for a dry run execution.
 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 +296,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 controllers.
+func serverSideApplyPatchHelperFactory(c client.Client) 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 way patch and is used in situations where we cannot rely on managed fields.
+func dryRunPatchHelperFactory(c client.Client) patchHelperFactoryFunc {
+	return func(original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) {
+		return structuredmerge.NewTwoWaysPatchHelper(original, modified, c, opts...)
+	}
+}

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
 		}
 

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)
 

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
 }