shift to an interface based implementation (instead of using reflection)

util/conditions/v1beta2/patch.go

@@ -23,7 +23,6 @@ import (
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	"sigs.k8s.io/cluster-api/util"
 )
@@ -53,24 +52,18 @@ const (
 )
 
 // NewPatch returns the Patch required to align source conditions to after conditions.
-func NewPatch(before, after runtime.Object) (Patch, error) {
+func NewPatch(before, after Getter) (Patch, error) {
 	var patch Patch
 
 	if util.IsNil(before) {
 		return nil, errors.New("error creating patch: before object is nil")
 	}
-	beforeConditions, err := GetAll(before)
-	if err != nil {
-		return nil, errors.Wrap(err, "error creating patch: failed to get conditions from before object")
-	}
+	beforeConditions := before.GetV1Beta2Conditions()
 
 	if util.IsNil(after) {
 		return nil, errors.New("error creating patch: after object is nil")
 	}
-	afterConditions, err := GetAll(after)
-	if err != nil {
-		return nil, errors.Wrap(err, "error creating patch: failed to get conditions from after object")
-	}
+	afterConditions := after.GetV1Beta2Conditions()
 
 	// Identify AddCondition and ModifyCondition changes.
 	for i := range afterConditions {
@@ -117,18 +110,15 @@ type ApplyOption func(*applyOptions)
 
 // Apply executes a three-way merge of a list of Patch.
 // When merge conflicts are detected (latest deviated from before in an incompatible way), an error is returned.
-func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
+func (p Patch) Apply(latest Setter, options ...ApplyOption) error {
 	if p.IsZero() {
 		return nil
 	}
 
 	if util.IsNil(latest) {
 		return errors.New("error patching conditions: latest object is nil")
 	}
-	latestConditions, err := GetAll(latest)
-	if err != nil {
-		return errors.Wrap(err, "error creating patch: failed to get conditions from latest object")
-	}
+	latestConditions := latest.GetV1Beta2Conditions()
 
 	applyOpt := &applyOptions{}
 	for _, o := range options {
@@ -211,7 +201,8 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
 		}
 	}
 
-	return SetAll(latest, latestConditions)
+	latest.SetV1Beta2Conditions(latestConditions)
+	return nil
 }
 
 // IsZero returns true if the patch is nil or has no changes.

util/conditions/v1beta2/patch_test.go

@@ -21,7 +21,6 @@ import (
 
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 )
@@ -32,8 +31,8 @@ func TestNewPatch(t *testing.T) {
 
 	tests := []struct {
 		name    string
-		before  runtime.Object
-		after   runtime.Object
+		before  Setter
+		after   Setter
 		want    Patch
 		wantErr bool
 	}{
@@ -136,9 +135,9 @@ func TestApply(t *testing.T) {
 
 	tests := []struct {
 		name    string
-		before  runtime.Object
-		after   runtime.Object
-		latest  runtime.Object
+		before  Setter
+		after   Setter
+		latest  Setter
 		options []ApplyOption
 		want    []metav1.Condition
 		wantErr bool
@@ -343,21 +342,19 @@ func TestApply(t *testing.T) {
 			}
 			g.Expect(err).ToNot(HaveOccurred())
 
-			gotConditions, err := GetAll(tt.latest)
-			g.Expect(err).ToNot(HaveOccurred())
-
+			gotConditions := tt.latest.GetV1Beta2Conditions()
 			g.Expect(gotConditions).To(MatchConditions(tt.want))
 		})
 	}
 }
 
-func objectWithConditions(conditions ...metav1.Condition) runtime.Object {
+func objectWithConditions(conditions ...metav1.Condition) Setter {
 	obj := &builder.Phase3Obj{}
 	obj.Status.Conditions = conditions
 	return obj
 }
 
-func nilObject() runtime.Object {
-	var obj runtime.Object
+func nilObject() Setter {
+	var obj *builder.Phase3Obj
 	return obj
 }

util/patch/options.go

@@ -41,6 +41,16 @@ type HelperOptions struct {
 	// OwnedV1Beta2Conditions defines condition types owned by the controller.
 	// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
 	OwnedV1Beta2Conditions []string
+
+	// Metav1ConditionsFields allows to override the path for the field hosting []metav1.Condition.
+	// Please note that the default value for this option is inferred from the object struct.
+	// The override for this option is considered only if the object implements the v1beta2conditions.Setter interface.
+	Metav1ConditionsFieldPath []string
+
+	// Clusterv1ConditionsFieldPath allows to override the path for the field hosting clusterv1.Conditions.
+	// Please note that the default value for this option is inferred from the object struct.
+	// The override for this option is considered only if the object implements the conditions.Setter interface.
+	Clusterv1ConditionsFieldPath []string
 }
 
 // WithForceOverwriteConditions allows the patch helper to overwrite conditions in case of conflicts.
@@ -82,3 +92,23 @@ type WithOwnedV1Beta2Conditions struct {
 func (w WithOwnedV1Beta2Conditions) ApplyToHelper(in *HelperOptions) {
 	in.OwnedV1Beta2Conditions = w.Conditions
 }
+
+// Metav1ConditionsFieldPath allows to override the path for the field hosting []metav1.Condition.
+// Please note that the default value for this option is inferred from the object struct.
+// The override for this option is considered only if the object implements the v1beta2conditions.Setter interface.
+type Metav1ConditionsFieldPath []string
+
+// ApplyToHelper applies this configuration to the given HelperOptions.
+func (w Metav1ConditionsFieldPath) ApplyToHelper(in *HelperOptions) {
+	in.Metav1ConditionsFieldPath = w
+}
+
+// Clusterv1ConditionsFieldPath allows to override the path for the field hosting clusterv1.Conditions.
+// Please note that the default value for this option is inferred from the object struct.
+// The override for this option is considered only if the object implements the conditions.Setter interface.
+type Clusterv1ConditionsFieldPath []string
+
+// ApplyToHelper applies this configuration to the given HelperOptions.
+func (w Clusterv1ConditionsFieldPath) ApplyToHelper(in *HelperOptions) {
+	in.Clusterv1ConditionsFieldPath = w
+}

util/patch/patch.go

@@ -48,12 +48,22 @@ type Helper struct {
 	after        *unstructured.Unstructured
 	changes      sets.Set[string]
 
-	metav1ConditionsFields    []string
-	clusterv1ConditionsFields []string
+	metav1ConditionsFieldPath    []string
+	clusterv1ConditionsFieldPath []string
 }
 
 // NewHelper returns an initialized Helper. Use NewHelper before changing
 // obj. After changing obj use Helper.Patch to persist your changes.
+//
+// Please note that patch helper implements a custom handling for objects implementing
+// the condition.Setter interface or the v1beta2conditions.Setter interface.
+//
+// It is also possible to implement wrappers for object not implementing those interfaces;
+// in case those objects have custom conditions types the wrapper should take care of conversions.
+// Additionally, if the conditions are not in the canonical place defined by the proposal for
+// improving status in Cluster API conditions, locations of the condition field must be
+// provided explicitly by using Metav1ConditionsFieldPath and Clusterv1ConditionsFieldPath options
+// during the Patch call.
 func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) {
 	// Return early if the object is nil.
 	if util.IsNil(obj) {
@@ -67,17 +77,19 @@ func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) {
 		return nil, errors.Wrapf(err, "failed to create patch helper for object %s", klog.KObj(obj))
 	}
 
-	metav1ConditionsFields, clusterv1ConditionsFields, err := identifySupportedConditions(obj)
+	// Identify location of the condition fields according to the canonical place defined by the proposal for
+	// // improving status in Cluster API conditions.
+	metav1ConditionsFieldPath, clusterv1ConditionsFieldPath, err := identifyConditionsFieldsPath(obj)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to identify condition fields for object %s", klog.KObj(obj))
 	}
 
 	// Check if the object satisfies the Cluster API conditions contract; if not, ignore condition fields for clusterv1.Conditions.
 	if _, canInterfaceConditions := obj.(conditions.Setter); !canInterfaceConditions {
-		clusterv1ConditionsFields = nil
+		clusterv1ConditionsFieldPath = nil
 	}
 	if _, canInterfaceConditions := obj.(conditions.Getter); !canInterfaceConditions {
-		clusterv1ConditionsFields = nil
+		clusterv1ConditionsFieldPath = nil
 	}
 
 	// Convert the object to unstructured to compare against our before copy.
@@ -87,12 +99,12 @@ func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) {
 	}
 
 	return &Helper{
-		client:                    crClient,
-		gvk:                       gvk,
-		before:                    unstructuredObj,
-		beforeObject:              obj.DeepCopyObject().(client.Object),
-		metav1ConditionsFields:    metav1ConditionsFields,
-		clusterv1ConditionsFields: clusterv1ConditionsFields,
+		client:                       crClient,
+		gvk:                          gvk,
+		before:                       unstructuredObj,
+		beforeObject:                 obj.DeepCopyObject().(client.Object),
+		metav1ConditionsFieldPath:    metav1ConditionsFieldPath,
+		clusterv1ConditionsFieldPath: clusterv1ConditionsFieldPath,
 	}, nil
 }
 
@@ -118,6 +130,22 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e
 		opt.ApplyToHelper(options)
 	}
 
+	// If condition field path override have been provided, propagate them to the helper for usage in various places of this func.
+	if len(options.Clusterv1ConditionsFieldPath) > 0 {
+		h.clusterv1ConditionsFieldPath = nil
+	}
+	if len(options.Metav1ConditionsFieldPath) > 0 {
+		h.metav1ConditionsFieldPath = nil
+	}
+
+	// Check if the object satisfies the Cluster API contract setter interfaces; if not, ignore condition field path entirely.
+	if _, canInterfaceConditions := obj.(conditions.Setter); !canInterfaceConditions {
+		h.clusterv1ConditionsFieldPath = nil
+	}
+	if _, canInterfaceV1Beta2Conditions := obj.(v1beta2conditions.Setter); !canInterfaceV1Beta2Conditions {
+		h.metav1ConditionsFieldPath = nil
+	}
+
 	// Convert the object to unstructured to compare against our before copy.
 	h.after, err = toUnstructured(obj, gvk)
 	if err != nil {
@@ -207,24 +235,24 @@ func (h *Helper) patchStatus(ctx context.Context, obj client.Object) error {
 // no unresolvable conflicts, the patch is sent again.
 func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, forceOverwrite bool, ownedConditions []clusterv1.ConditionType, ownedV1beta2Conditions []string) error {
 	// Nothing to do if the object doesn't have conditions (doesn't have conditions identified as needing a special treatment).
-	if len(h.clusterv1ConditionsFields) == 0 && len(h.metav1ConditionsFields) == 0 {
+	if len(h.clusterv1ConditionsFieldPath) == 0 && len(h.metav1ConditionsFieldPath) == 0 {
 		return nil
 	}
 
 	// If the object has clusterv1 conditions, create a function applying corresponding changes if any.
 	var clusterv1ApplyPatch func(client.Object) error
-	if len(h.clusterv1ConditionsFields) > 0 {
+	if len(h.clusterv1ConditionsFieldPath) > 0 {
 		// Make sure our before/after objects satisfy the proper interface before continuing.
 		//
 		// NOTE: The checks and error below are done so that we don't panic if any of the objects don't satisfy the
 		// interface any longer, although this shouldn't happen because we already check when creating the patcher.
-		before, ok := h.beforeObject.(conditions.Getter)
+		before, ok := h.beforeObject.(conditions.Setter)
 		if !ok {
-			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot patch", h.gvk.Kind, klog.KObj(before))
+			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot patch", h.gvk.Kind, klog.KObj(h.beforeObject))
 		}
-		after, ok := obj.(conditions.Getter)
+		after, ok := obj.(conditions.Setter)
 		if !ok {
-			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot compute patch", h.gvk.Kind, after.GetObjectKind())
+			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot compute patch", h.gvk.Kind, klog.KObj(obj))
 		}
 
 		diff, err := conditions.NewPatch(
@@ -238,7 +266,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 			clusterv1ApplyPatch = func(latest client.Object) error {
 				latestSetter, ok := latest.(conditions.Setter)
 				if !ok {
-					return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, before.GetObjectKind())
+					return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, klog.KObj(latest))
 				}
 
 				return diff.Apply(latestSetter, conditions.WithForceOverwrite(forceOverwrite), conditions.WithOwnedConditions(ownedConditions...))
@@ -248,18 +276,36 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 
 	// If the object has metav1 conditions, create a function applying corresponding changes if any.
 	var metav1ApplyPatch func(client.Object) error
-	if len(h.metav1ConditionsFields) > 0 {
+	if len(h.metav1ConditionsFieldPath) > 0 {
+		// Make sure our before/after objects satisfy the proper interface before continuing.
+		//
+		// NOTE: The checks and error below are done so that we don't panic if any of the objects don't satisfy the
+		// interface any longer, although this shouldn't happen because we already check when creating the patcher.
+		before, ok := h.beforeObject.(v1beta2conditions.Getter)
+		if !ok {
+			return errors.Errorf("%s %s doesn't satisfy v1beta2conditions.Getter, cannot patch", h.gvk.Kind, klog.KObj(h.beforeObject))
+		}
+		after, ok := obj.(v1beta2conditions.Getter)
+		if !ok {
+			return errors.Errorf("%s %s doesn't satisfy v1beta2conditions.Getter, cannot compute patch", h.gvk.Kind, klog.KObj(obj))
+		}
+
 		diff, err := v1beta2conditions.NewPatch(
-			h.beforeObject,
-			obj,
+			before,
+			after,
 		)
 		if err != nil {
 			return errors.Wrapf(err, "%s can not be patched", h.gvk.Kind)
 		}
 
 		if !diff.IsZero() {
 			metav1ApplyPatch = func(latest client.Object) error {
-				return diff.Apply(latest, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...))
+				latestSetter, ok := latest.(v1beta2conditions.Setter)
+				if !ok {
+					return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, klog.KObj(latest))
+				}
+
+				return diff.Apply(latestSetter, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...))
 			}
 		}
 	}
@@ -326,8 +372,8 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 // calculatePatch returns the before/after objects to be given in a controller-runtime patch, scoped down to the absolute necessary.
 func (h *Helper) calculatePatch(afterObj client.Object, focus patchType) (client.Object, client.Object, error) {
 	// Get a shallow unsafe copy of the before/after object in unstructured form.
-	before := unsafeUnstructuredCopy(h.before, focus, h.metav1ConditionsFields, h.clusterv1ConditionsFields)
-	after := unsafeUnstructuredCopy(h.after, focus, h.metav1ConditionsFields, h.clusterv1ConditionsFields)
+	before := unsafeUnstructuredCopy(h.before, focus, h.clusterv1ConditionsFieldPath, h.metav1ConditionsFieldPath)
+	after := unsafeUnstructuredCopy(h.after, focus, h.clusterv1ConditionsFieldPath, h.metav1ConditionsFieldPath)
 
 	// We've now applied all modifications to local unstructured objects,
 	// make copies of the original objects and convert them back.