MD: Implement CanUpdateMachineSet

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

api/runtime/hooks/v1alpha1/inplaceupdate_types.go

@@ -67,14 +67,17 @@ type CanUpdateMachineResponse struct {
 	CommonResponse `json:",inline"`
 
 	// machinePatch when applied to the current Machine spec, indicates changes handled in-place.
+	// Only fields in spec have to be covered by the patch.
 	// +optional
 	MachinePatch Patch `json:"machinePatch,omitempty,omitzero"`
 
 	// infrastructureMachinePatch indicates infra Machine spec changes handled in-place.
+	// Only fields in spec have to be covered by the patch.
 	// +optional
 	InfrastructureMachinePatch Patch `json:"infrastructureMachinePatch,omitempty,omitzero"`
 
 	// bootstrapConfigPatch indicates bootstrap config spec changes handled in-place.
+	// Only fields in spec have to be covered by the patch.
 	// +optional
 	BootstrapConfigPatch Patch `json:"bootstrapConfigPatch,omitempty,omitzero"`
 }
@@ -119,14 +122,17 @@ type CanUpdateMachineSetRequest struct {
 // CanUpdateMachineSetRequestObjects groups objects for CanUpdateMachineSetRequest.
 type CanUpdateMachineSetRequestObjects struct {
 	// machineSet is the full MachineSet object.
+	// Only fields in spec.template.spec have to be covered by the patch.
 	// +required
 	MachineSet clusterv1.MachineSet `json:"machineSet,omitempty,omitzero"`
 
 	// infrastructureMachineTemplate is the provider-specific InfrastructureMachineTemplate object.
+	// Only fields in spec.template.spec have to be covered by the patch.
 	// +required
 	InfrastructureMachineTemplate runtime.RawExtension `json:"infrastructureMachineTemplate,omitempty,omitzero"`
 
 	// bootstrapConfigTemplate is the provider-specific BootstrapConfigTemplate object.
+	// Only fields in spec.template.spec have to be covered by the patch.
 	// +optional
 	BootstrapConfigTemplate runtime.RawExtension `json:"bootstrapConfigTemplate,omitempty,omitzero"`
 }
@@ -217,7 +223,8 @@ func init() {
 			"Notes:\n" +
 			"- This hook is called during the planning phase of updates\n" +
 			"- Only spec is provided, status fields are not included\n" +
-			"- If no extension can cover the required changes, CAPI will fallback to rolling updates\n",
+			"- If no extension can cover the required changes, CAPI will fallback to rolling updates\n" +
+			"- Only fields in Machine/InfraMachine/BootstrapConfig spec have to be covered by patches\n",
 	})
 
 	catalogBuilder.RegisterHook(CanUpdateMachineSet, &runtimecatalog.HookMeta{
@@ -230,7 +237,8 @@ func init() {
 			"Notes:\n" +
 			"- This hook is called during the planning phase of updates\n" +
 			"- Only spec is provided, status fields are not included\n" +
-			"- If no extension can cover the required changes, CAPI will fallback to rolling updates\n",
+			"- If no extension can cover the required changes, CAPI will fallback to rolling updates\n" +
+			"- Only fields in MachineSet/InfraMachineTemplate/BootstrapConfigTemplate spec.template.spec have to be covered by patches\n",
 	})
 
 	catalogBuilder.RegisterHook(UpdateMachine, &runtimecatalog.HookMeta{

api/runtime/hooks/v1alpha1/zz_generated.openapi.go

@@ -123,8 +123,9 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
 
 // MachineDeploymentReconciler reconciles a MachineDeployment object.
 type MachineDeploymentReconciler struct {
-	Client    client.Client
-	APIReader client.Reader
+	Client        client.Client
+	APIReader     client.Reader
+	RuntimeClient runtimeclient.Client
 
 	// WatchFilterValue is the label value used to filter events prior to reconciliation.
 	WatchFilterValue string
@@ -134,6 +135,7 @@ func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr
 	return (&machinedeploymentcontroller.Reconciler{
 		Client:           r.Client,
 		APIReader:        r.APIReader,
+		RuntimeClient:    r.RuntimeClient,
 		WatchFilterValue: r.WatchFilterValue,
 	}).SetupWithManager(ctx, mgr, options)
 }

controllers/alias.go

@@ -17,19 +17,14 @@ limitations under the License.
 package controllers
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
-	"runtime/debug"
 	"strings"
 
-	jsonpatch "github.com/evanphx/json-patch/v5"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -40,7 +35,7 @@ import (
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/util/compare"
-	patchutil "sigs.k8s.io/cluster-api/internal/util/patch"
+	"sigs.k8s.io/cluster-api/internal/util/patch"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 )
 
@@ -75,7 +70,7 @@ func (r *KubeadmControlPlaneReconciler) canUpdateMachine(ctx context.Context, ma
 		return false, nil
 	}
 	if len(extensionHandlers) > 1 {
-		return false, errors.Errorf("found multiple CanUpdateMachine hooks (%s) (more than one is not supported yet)", strings.Join(extensionHandlers, ","))
+		return false, errors.Errorf("found multiple CanUpdateMachine hooks (%s): only one hook is supported", strings.Join(extensionHandlers, ","))
 	}
 
 	canUpdateMachine, reasons, err := r.canExtensionsUpdateMachine(ctx, machine, machineUpToDateResult, extensionHandlers)
@@ -186,19 +181,19 @@ func createRequest(ctx context.Context, c client.Client, currentMachine *cluster
 		},
 	}
 	var err error
-	req.Current.BootstrapConfig, err = convertToRawExtension(cleanupKubeadmConfig(currentKubeadmConfigForDiff))
+	req.Current.BootstrapConfig, err = patch.ConvertToRawExtension(cleanupKubeadmConfig(currentKubeadmConfigForDiff))
 	if err != nil {
 		return nil, err
 	}
-	req.Desired.BootstrapConfig, err = convertToRawExtension(cleanupKubeadmConfig(desiredKubeadmConfigForDiff))
+	req.Desired.BootstrapConfig, err = patch.ConvertToRawExtension(cleanupKubeadmConfig(desiredKubeadmConfigForDiff))
 	if err != nil {
 		return nil, err
 	}
-	req.Current.InfrastructureMachine, err = convertToRawExtension(cleanupUnstructured(currentInfraMachineForDiff))
+	req.Current.InfrastructureMachine, err = patch.ConvertToRawExtension(cleanupUnstructured(currentInfraMachineForDiff))
 	if err != nil {
 		return nil, err
 	}
-	req.Desired.InfrastructureMachine, err = convertToRawExtension(cleanupUnstructured(desiredInfraMachineForDiff))
+	req.Desired.InfrastructureMachine, err = patch.ConvertToRawExtension(cleanupUnstructured(desiredInfraMachineForDiff))
 	if err != nil {
 		return nil, err
 	}
@@ -255,143 +250,28 @@ func cleanupUnstructured(u *unstructured.Unstructured) *unstructured.Unstructure
 	return cleanedUpU
 }
 
-func convertToRawExtension(object runtime.Object) (runtime.RawExtension, error) {
-	objectBytes, err := json.Marshal(object)
-	if err != nil {
-		return runtime.RawExtension{}, errors.Wrap(err, "failed to marshal object to JSON")
-	}
-
-	objectUnstructured, ok := object.(*unstructured.Unstructured)
-	if !ok {
-		objectUnstructured = &unstructured.Unstructured{}
-		// Note: This only succeeds if object has apiVersion & kind set (which is always the case).
-		if err := json.Unmarshal(objectBytes, objectUnstructured); err != nil {
-			return runtime.RawExtension{}, errors.Wrap(err, "failed to Unmarshal object into Unstructured")
-		}
-	}
-
-	// Note: Raw and Object are always both set and Object is always set as an Unstructured
-	//       to simplify subsequent code in matchesUnstructuredSpec.
-	return runtime.RawExtension{
-		Raw:    objectBytes,
-		Object: objectUnstructured,
-	}, nil
-}
-
 func applyPatchesToRequest(ctx context.Context, req *runtimehooksv1.CanUpdateMachineRequest, resp *runtimehooksv1.CanUpdateMachineResponse) error {
 	if resp.MachinePatch.IsDefined() {
-		if err := applyPatchToMachine(ctx, &req.Current.Machine, resp.MachinePatch); err != nil {
+		if err := patch.ApplyPatchToTypedObject(ctx, &req.Current.Machine, resp.MachinePatch, "spec"); err != nil {
 			return err
 		}
 	}
 
 	if resp.BootstrapConfigPatch.IsDefined() {
-		if _, err := applyPatchToObject(ctx, &req.Current.BootstrapConfig, resp.BootstrapConfigPatch); err != nil {
+		if _, err := patch.ApplyPatchToObject(ctx, &req.Current.BootstrapConfig, resp.BootstrapConfigPatch, "spec"); err != nil {
 			return err
 		}
 	}
 
 	if resp.InfrastructureMachinePatch.IsDefined() {
-		if _, err := applyPatchToObject(ctx, &req.Current.InfrastructureMachine, resp.InfrastructureMachinePatch); err != nil {
+		if _, err := patch.ApplyPatchToObject(ctx, &req.Current.InfrastructureMachine, resp.InfrastructureMachinePatch, "spec"); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
 
-func applyPatchToMachine(ctx context.Context, currentMachine *clusterv1.Machine, machinePath runtimehooksv1.Patch) error {
-	// Note: Machine needs special handling because it is not a runtime.RawExtension. Simply converting it here to
-	//       a runtime.RawExtension so we can avoid making the code in applyPatchToObject more complex.
-	currentMachineRaw, err := convertToRawExtension(currentMachine)
-	if err != nil {
-		return err
-	}
-
-	machineChanged, err := applyPatchToObject(ctx, &currentMachineRaw, machinePath)
-	if err != nil {
-		return err
-	}
-
-	if !machineChanged {
-		return nil
-	}
-
-	// Note: json.Unmarshal can't be used directly on *currentMachine as json.Unmarshal does not unset fields.
-	patchedCurrentMachine := &clusterv1.Machine{}
-	if err := json.Unmarshal(currentMachineRaw.Raw, patchedCurrentMachine); err != nil {
-		return err
-	}
-	*currentMachine = *patchedCurrentMachine
-	return nil
-}
-
-// applyPatchToObject applies the patch to the obj.
-// Note: This is following the same general structure that is used in the applyPatchToRequest func in
-// internal/controllers/topology/cluster/patches/engine.go.
-func applyPatchToObject(ctx context.Context, obj *runtime.RawExtension, patch runtimehooksv1.Patch) (objChanged bool, reterr error) {
-	log := ctrl.LoggerFrom(ctx)
-
-	if patch.PatchType == "" {
-		return false, errors.Errorf("failed to apply patch: patchType is not set")
-	}
-
-	defer func() {
-		if r := recover(); r != nil {
-			log.Info(fmt.Sprintf("Observed a panic when applying patch: %v\n%s", r, string(debug.Stack())))
-			reterr = kerrors.NewAggregate([]error{reterr, fmt.Errorf("observed a panic when applying patch: %v", r)})
-		}
-	}()
-
-	// Create a copy of obj.Raw.
-	// The patches will be applied to the copy and then only spec changes will be copied back to the obj.
-	patchedObject := bytes.Clone(obj.Raw)
-	var err error
-
-	switch patch.PatchType {
-	case runtimehooksv1.JSONPatchType:
-		log.V(5).Info("Accumulating JSON patch", "patch", string(patch.Patch))
-		jsonPatch, err := jsonpatch.DecodePatch(patch.Patch)
-		if err != nil {
-			log.Error(err, "Failed to apply patch: error decoding json patch (RFC6902)", "patch", string(patch.Patch))
-			return false, errors.Wrap(err, "failed to apply patch: error decoding json patch (RFC6902)")
-		}
-
-		if len(jsonPatch) == 0 {
-			// Return if there are no patches, nothing to do.
-			return false, nil
-		}
-
-		patchedObject, err = jsonPatch.Apply(patchedObject)
-		if err != nil {
-			log.Error(err, "Failed to apply patch: error applying json patch (RFC6902)", "patch", string(patch.Patch))
-			return false, errors.Wrap(err, "failed to apply patch: error applying json patch (RFC6902)")
-		}
-	case runtimehooksv1.JSONMergePatchType:
-		if len(patch.Patch) == 0 || bytes.Equal(patch.Patch, []byte("{}")) {
-			// Return if there are no patches, nothing to do.
-			return false, nil
-		}
-
-		log.V(5).Info("Accumulating JSON merge patch", "patch", string(patch.Patch))
-		patchedObject, err = jsonpatch.MergePatch(patchedObject, patch.Patch)
-		if err != nil {
-			log.Error(err, "Failed to apply patch: error applying json merge patch (RFC7386)", "patch", string(patch.Patch))
-			return false, errors.Wrap(err, "failed to apply patch: error applying json merge patch (RFC7386)")
-		}
-	default:
-		return false, errors.Errorf("failed to apply patch: unknown patchType %s", patch.PatchType)
-	}
-
-	// Overwrite the spec of obj with the spec of the patchedObject,
-	// to ensure that we only pick up changes to the spec.
-	if err := patchutil.PatchSpec(obj, patchedObject); err != nil {
-		return false, errors.Wrap(err, "failed to apply patch to object")
-	}
-
-	return true, nil
-}
-
 func matchesMachine(req *runtimehooksv1.CanUpdateMachineRequest) (bool, []string, error) {
 	var reasons []string
 	match, diff, err := matchesMachineSpec(&req.Current.Machine, &req.Desired.Machine)

controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go

@@ -42,6 +42,7 @@ import (
 	"sigs.k8s.io/cluster-api/feature"
 	fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake"
 	"sigs.k8s.io/cluster-api/internal/util/compare"
+	"sigs.k8s.io/cluster-api/internal/util/patch"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util/test/builder"
 )
@@ -104,7 +105,7 @@ func Test_canUpdateMachine(t *testing.T) {
 				canUpdateMachineGVH: {"test-update-extension-1", "test-update-extension-2"},
 			},
 			wantError:        true,
-			wantErrorMessage: "found multiple CanUpdateMachine hooks (test-update-extension-1,test-update-extension-2) (more than one is not supported yet)",
+			wantErrorMessage: "found multiple CanUpdateMachine hooks (test-update-extension-1,test-update-extension-2): only one hook is supported",
 		},
 		{
 			name:                            "Return false if canExtensionsUpdateMachine returns false",
@@ -1174,7 +1175,7 @@ func diff(a, b any) string {
 }
 
 func mustConvertToRawExtension(object runtime.Object) runtime.RawExtension {
-	raw, err := convertToRawExtension(object)
+	raw, err := patch.ConvertToRawExtension(object)
 	if err != nil {
 		panic(err)
 	}

controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go

@@ -19,6 +19,7 @@ package machine
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -149,7 +150,7 @@ func (r *Reconciler) callUpdateMachineHook(ctx context.Context, s *scope) (ctrl.
 	}
 
 	if len(extensions) > 1 {
-		return ctrl.Result{}, "", errors.Errorf("multiple extensions registered for UpdateMachine hook: only one extension is supported, found %d extensions: %v", len(extensions), extensions)
+		return ctrl.Result{}, "", errors.Errorf("found multiple UpdateMachine hooks (%s): only one hook is supported", strings.Join(extensions, ","))
 	}
 
 	// Note: When building request message, dropping status; Runtime extension should treat UpdateMachine

internal/controllers/machine/machine_controller_inplace_update.go

@@ -367,13 +367,8 @@ func TestCallUpdateMachineHook(t *testing.T) {
 					Build()
 				return &Reconciler{RuntimeClient: runtimeClient}, &scope{machine: newTestMachine(), infraMachine: newTestUnstructured("GenericInfrastructureMachine", "infrastructure.cluster.x-k8s.io/v1beta2", "infra")}
 			},
-			wantErr: true,
-			wantErrSubstrings: []string{
-				"multiple extensions registered for UpdateMachine hook",
-				"only one extension is supported",
-				"ext-a",
-				"ext-b",
-			},
+			wantErr:           true,
+			wantErrSubstrings: []string{"found multiple UpdateMachine hooks (ext-a,ext-b): only one hook is supported"},
 		},
 		{
 			name: "fails when hook invocation returns error",