Implement strategy for resolving conflicts in updater by merging-in externally managed conditions

Author: Moritz Clasmeier

pkg/reconciler/internal/updater/updater.go

@@ -18,7 +18,10 @@ package updater
 
 import (
 	"context"
+	"fmt"
+	"reflect"
 
+	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/release"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -33,17 +36,30 @@ import (
 	"github.com/operator-framework/helm-operator-plugins/pkg/internal/status"
 )
 
-func New(client client.Client) Updater {
+func New(client client.Client, logger logr.Logger) Updater {
+	logger = logger.WithName("updater")
 	return Updater{
 		client: client,
+		logger: logger,
 	}
 }
 
 type Updater struct {
-	isCanceled        bool
-	client            client.Client
-	updateFuncs       []UpdateFunc
-	updateStatusFuncs []UpdateStatusFunc
+	isCanceled                        bool
+	client                            client.Client
+	logger                            logr.Logger
+	updateFuncs                       []UpdateFunc
+	updateStatusFuncs                 []UpdateStatusFunc
+	externallyManagedStatusConditions map[string]struct{}
+}
+
+func (u *Updater) RegisterExternallyManagedStatusConditions(conditions map[string]struct{}) {
+	if u.externallyManagedStatusConditions == nil {
+		u.externallyManagedStatusConditions = make(map[string]struct{}, len(conditions))
+	}
+	for conditionType := range conditions {
+		u.externallyManagedStatusConditions[conditionType] = struct{}{}
+	}
 }
 
 type UpdateFunc func(*unstructured.Unstructured) bool
@@ -113,7 +129,18 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err
 		st.updateStatusObject()
 		obj.Object["status"] = st.StatusObject
 		if err := retryOnRetryableUpdateError(backoff, func() error {
-			return u.client.Status().Update(ctx, obj)
+			updateErr := u.client.Status().Update(ctx, obj)
+			if errors.IsConflict(updateErr) {
+				resolved, resolveErr := u.tryRefreshObject(ctx, obj)
+				if resolveErr != nil {
+					return resolveErr
+				}
+				if !resolved {
+					return updateErr
+				}
+				return fmt.Errorf("status update conflict due to externally-managed status conditions") // retriable error.
+			}
+			return updateErr
 		}); err != nil {
 			return err
 		}
@@ -125,14 +152,154 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err
 	}
 	if needsUpdate {
 		if err := retryOnRetryableUpdateError(backoff, func() error {
-			return u.client.Update(ctx, obj)
+			updateErr := u.client.Update(ctx, obj)
+			if errors.IsConflict(updateErr) {
+				resolved, resolveErr := u.tryRefreshObject(ctx, obj)
+				if resolveErr != nil {
+					return resolveErr
+				}
+				if !resolved {
+					return updateErr
+				}
+				return fmt.Errorf("update conflict due to externally-managed status conditions") // retriable error.
+			}
+			return updateErr
 		}); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
+// This function tries to merge the status of obj with the current version of the status on the cluster.
+// The unstructured obj is expected to have been modified and to have caused a conflict error during an update attempt.
+// If the only differences between obj and the current version are in externally managed status conditions,
+// those conditions are merged from the current version into obj.
+// Returns true if updating shall be retried with the updated obj.
+// Returns false if the conflict could not be resolved.
+func (u *Updater) tryRefreshObject(ctx context.Context, obj *unstructured.Unstructured) (bool, error) {
+	// Retrieve current version from the cluster.
+	current := &unstructured.Unstructured{}
+	current.SetGroupVersionKind(obj.GroupVersionKind())
+	objectKey := client.ObjectKeyFromObject(obj)
+	if err := u.client.Get(ctx, objectKey, current); err != nil {
+		err = fmt.Errorf("refreshing object %s/%s: %w", objectKey.Namespace, objectKey.Name, err)
+		return false, err
+	}
+
+	if !reflect.DeepEqual(obj.Object["spec"], current.Object["spec"]) {
+		// Diff in object spec. Nothing we can do about it -> Fail.
+		u.logger.V(1).Info("cluster resource %s/%s cannot be updated due to spec mismatch")
+		return false, nil
+	}
+
+	// Merge externally managed conditions from current into object copy.
+	objCopy := obj.DeepCopy()
+	u.mergeExternallyManagedConditions(objCopy, current)
+
+	// Check if the merged status matches the current status.
+	// If they don't match, the conflict is not just about external conditions, but something
+	// else went off the rails as well. It's probably better to not resolve the conflict here
+	// and instead start over.
+	if !reflect.DeepEqual(objCopy.Object["status"], current.Object["status"]) {
+		u.logger.V(1).Info("cluster resource %s/%s cannot be updated due to status mismatch")
+		return false, nil
+	}
+
+	// Overwrite metadata with the most recent in-cluster version.
+	// This ensures we have the latest resourceVersion, annotations, labels, etc.
+	objCopy.Object["metadata"] = current.Object["metadata"]
+
+	// We were able to resolve the conflict by merging external conditions.
+	obj.Object = objCopy.Object
+
+	u.logger.V(1).Info("Resolved update conflict by merging externally-managed status conditions")
+	return true, nil
+}
+
+// mergeExternallyManagedConditions updates obj's status conditions by replacing
+// externally managed conditions with their values from current.
+// Uses current's ordering to avoid false positives in conflict detection.
+func (u *Updater) mergeExternallyManagedConditions(obj, current *unstructured.Unstructured) {
+	objConditions := statusConditionsFromObject(obj)
+	if objConditions == nil {
+		return
+	}
+
+	currentConditions := statusConditionsFromObject(current)
+	if currentConditions == nil {
+		return
+	}
+
+	// Build a map of all conditions from obj (by type).
+	objConditionsByType := make(map[string]map[string]interface{})
+	for _, cond := range objConditions {
+		if condType, ok := cond["type"].(string); ok {
+			objConditionsByType[condType] = cond
+		}
+	}
+
+	// Build merged conditions starting from current's ordering.
+	mergedConditions := make([]map[string]interface{}, 0, len(currentConditions))
+	for _, cond := range currentConditions {
+		condType, ok := cond["type"].(string)
+		if !ok {
+			// Shouldn't happen.
+			continue
+		}
+		if _, isExternal := u.externallyManagedStatusConditions[condType]; isExternal {
+			// Keep external condition from current.
+			mergedConditions = append(mergedConditions, cond)
+		} else if objCond, found := objConditionsByType[condType]; found {
+			// Replace with non-external condition from obj.
+			mergedConditions = append(mergedConditions, objCond)
+			delete(objConditionsByType, condType) // Mark as used.
+		}
+		// Note: If condition exists in current but not in obj (and is non-external),
+		// we skip it.
+	}
+
+	// Add any remaining non-externally managed conditions from obj that weren't in current.
+	for condType, cond := range objConditionsByType {
+		if _, isExternal := u.externallyManagedStatusConditions[condType]; isExternal {
+			continue
+		}
+		mergedConditions = append(mergedConditions, cond)
+	}
+
+	// Convert to []interface{} for SetNestedField
+	mergedConditionsInterface := make([]interface{}, len(mergedConditions))
+	for i, cond := range mergedConditions {
+		mergedConditionsInterface[i] = cond
+	}
+
+	// Write the modified conditions back.
+	_ = unstructured.SetNestedField(obj.Object, mergedConditionsInterface, "status", "conditions")
+}
+
+// statusConditionsFromObject extracts status conditions from an unstructured object.
+// Returns nil if the conditions field is not found or is not the expected type.
+func statusConditionsFromObject(obj *unstructured.Unstructured) []map[string]interface{} {
+	conditionsRaw, ok, _ := unstructured.NestedFieldNoCopy(obj.Object, "status", "conditions")
+	if !ok {
+		return nil
+	}
+
+	conditionsSlice, ok := conditionsRaw.([]interface{})
+	if !ok {
+		return nil
+	}
+
+	// Convert []interface{} to []map[string]interface{}
+	result := make([]map[string]interface{}, 0, len(conditionsSlice))
+	for _, cond := range conditionsSlice {
+		if condMap, ok := cond.(map[string]interface{}); ok {
+			result = append(result, condMap)
+		}
+	}
+	return result
+}
+
 func RemoveFinalizer(finalizer string) UpdateFunc {
 	return func(obj *unstructured.Unstructured) bool {
 		if !controllerutil.ContainsFinalizer(obj, finalizer) {

pkg/reconciler/internal/updater/updater_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 
+	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -51,7 +52,7 @@ var _ = Describe("Updater", func() {
 
 	JustBeforeEach(func() {
 		cl = fake.NewClientBuilder().WithInterceptorFuncs(interceptorFuncs).Build()
-		u = New(cl)
+		u = New(cl, logr.Discard())
 		obj = &unstructured.Unstructured{Object: map[string]interface{}{
 			"apiVersion": "apps/v1",
 			"kind":       "Deployment",
@@ -326,3 +327,153 @@ var _ = Describe("statusFor", func() {
 		Expect(statusFor(obj)).To(Equal(&helmAppStatus{}))
 	})
 })
+
+var _ = Describe("tryMergeUpdatedObjectStatus", func() {
+	var (
+		cl                     client.Client
+		u                      Updater
+		obj                    *unstructured.Unstructured
+		current                *unstructured.Unstructured
+		externalCondition      map[string]interface{}
+		externalConditionTypes map[string]struct{}
+		nonExternalCondition   map[string]interface{}
+		nonExternalConditionB  map[string]interface{}
+	)
+
+	BeforeEach(func() {
+		externalCondition = map[string]interface{}{
+			"type":   "ExternalCondition",
+			"status": string(corev1.ConditionTrue),
+			"reason": "ExternallyManaged",
+		}
+		externalConditionTypes = map[string]struct{}{
+			externalCondition["type"].(string): {},
+		}
+		nonExternalCondition = map[string]interface{}{
+			"type":   "Deployed",
+			"status": string(corev1.ConditionTrue),
+			"reason": "InitialDeployment",
+		}
+		nonExternalConditionB = map[string]interface{}{
+			"type":   "Foo",
+			"status": string(corev1.ConditionTrue),
+			"reason": "Success",
+		}
+
+		// Setup obj with initial state (version 100).
+		obj = &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "example.com/v1",
+				"kind":       "TestResource",
+				"metadata": map[string]interface{}{
+					"name":            "test-obj",
+					"namespace":       "default",
+					"resourceVersion": "100",
+				},
+				"status": map[string]interface{}{},
+			},
+		}
+
+		// Setup current with updated state (version 101).
+		current = &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "example.com/v1",
+				"kind":       "TestResource",
+				"metadata": map[string]interface{}{
+					"name":            "test-obj",
+					"namespace":       "default",
+					"resourceVersion": "101",
+				},
+				"status": map[string]interface{}{},
+			},
+		}
+	})
+
+	// When("status empty in both obj and current", func() {
+	// })
+
+	When("only external conditions differ", func() {
+		BeforeEach(func() {
+			obj.Object["status"] = map[string]interface{}{
+				"conditions": []interface{}{nonExternalCondition},
+			}
+			current.Object["status"] = map[string]interface{}{
+				"conditions": []interface{}{nonExternalCondition, externalCondition},
+			}
+
+			cl = fake.NewClientBuilder().WithObjects(current).Build()
+			u = New(cl, logr.Discard())
+			u.RegisterExternallyManagedStatusConditions(externalConditionTypes)
+		})
+
+		It("should merge and return resolved", func() {
+			resolved, err := u.tryRefreshObject(context.Background(), obj)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(resolved).To(BeTrue())
+			Expect(obj.GetResourceVersion()).To(Equal("101"))
+
+			// Verify external condition was merged
+			conditions, ok, _ := unstructured.NestedSlice(obj.Object, "status", "conditions")
+			Expect(ok).To(BeTrue())
+			Expect(conditions).To(HaveLen(2))
+
+			// Verify ordering (Deployed first, ExternalCondition second from current)
+			Expect(conditions[0].(map[string]interface{})["type"]).To(Equal("Deployed"))
+			Expect(conditions[1].(map[string]interface{})["type"]).To(Equal("ExternalCondition"))
+		})
+	})
+
+	When("non-external condition differs", func() {
+		BeforeEach(func() {
+			obj.Object["status"] = map[string]interface{}{
+				"conditions": []interface{}{nonExternalCondition},
+			}
+			current.Object["status"] = map[string]interface{}{
+				"conditions": []interface{}{nonExternalCondition, nonExternalConditionB},
+			}
+
+			cl = fake.NewClientBuilder().WithObjects(current).Build()
+			u = New(cl, logr.Discard())
+			u.RegisterExternallyManagedStatusConditions(externalConditionTypes)
+		})
+
+		It("should not resolve", func() {
+			resolved, err := u.tryRefreshObject(context.Background(), obj)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(resolved).To(BeFalse())
+			Expect(obj.GetResourceVersion()).To(Equal("100"), "resource version should not be updated")
+			Expect(obj.Object["status"].(map[string]interface{})["conditions"]).To(HaveLen(1))
+		})
+	})
+
+	When("no external conditions are configured", func() {
+		BeforeEach(func() {
+			cl = fake.NewClientBuilder().Build()
+			u = New(cl, logr.Discard())
+		})
+
+		It("should return early without calling Get", func() {
+			resolved, err := u.tryRefreshObject(context.Background(), obj)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(resolved).To(BeFalse())
+		})
+	})
+
+	When("Get returns an error", func() {
+		BeforeEach(func() {
+			// Empty client - Get will return NotFound
+			cl = fake.NewClientBuilder().Build()
+			u = New(cl, logr.Discard())
+			u.RegisterExternallyManagedStatusConditions(externalConditionTypes)
+		})
+
+		It("should return the error", func() {
+			resolved, err := u.tryRefreshObject(context.Background(), obj)
+			Expect(err).To(HaveOccurred())
+			Expect(apierrors.IsNotFound(err)).To(BeTrue())
+			Expect(resolved).To(BeFalse())
+		})
+	})
+})