[ACM-16805] Fixed regression for template resource patching

controllers/common.go

@@ -56,7 +56,7 @@ type CacheSpec struct {
 }
 
 func (r *MultiClusterHubReconciler) ensureNoNamespace(m *operatorv1.MultiClusterHub, u *unstructured.Unstructured) (ctrl.Result, error) {
-	subLog := r.Log.WithValues("Name", u.GetName(), "Kind", u.GetKind())
+	subLog := r.Log.WithValues("Kind", u.GetKind(), "Name", u.GetName())
 	gone, err := r.uninstall(m, u)
 	if err != nil {
 		subLog.Error(err, "Failed to uninstall namespace")
@@ -70,7 +70,7 @@ func (r *MultiClusterHubReconciler) ensureNoNamespace(m *operatorv1.MultiCluster
 }
 
 func (r *MultiClusterHubReconciler) ensureUnstructuredResource(m *operatorv1.MultiClusterHub, u *unstructured.Unstructured) (ctrl.Result, error) {
-	obLog := r.Log.WithValues("Namespace", u.GetNamespace(), "Name", u.GetName(), "Kind", u.GetKind())
+	obLog := r.Log.WithValues("Namespace", u.GetNamespace(), "Kind", u.GetKind(), "Name", u.GetName())
 
 	found := &unstructured.Unstructured{}
 	found.SetGroupVersionKind(u.GroupVersionKind())
@@ -524,7 +524,7 @@ func (r *MultiClusterHubReconciler) waitForMCEReady(ctx context.Context) (ctrl.R
 		err = version.ValidMCEVersion(existingMCE.Status.CurrentVersion)
 	}
 	if err != nil {
-		return ctrl.Result{RequeueAfter: resyncPeriod}, fmt.Errorf("MCE version requirement not met: %w", err)
+		return ctrl.Result{}, fmt.Errorf("MCE version requirement not met: %w", err)
 	}
 	return ctrl.Result{}, nil
 }

controllers/multiclusterhub_controller.go

@@ -445,7 +445,7 @@ func (r *MultiClusterHubReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 
 	result, err = r.waitForMCEReady(ctx)
-	if result != (ctrl.Result{}) {
+	if result != (ctrl.Result{}) || err != nil {
 		return result, err
 	}
 
@@ -764,31 +764,44 @@ func (r *MultiClusterHubReconciler) applyTemplate(ctx context.Context, m *operat
 				}
 			}
 		}
-		err := r.Client.Get(ctx, types.NamespacedName{Name: template.GetName(), Namespace: template.GetNamespace()}, template)
-		template.SetManagedFields(nil)
-		// resource not found
-		if err != nil {
-			err := r.Client.Create(ctx, template, &client.CreateOptions{})
-			if err != nil {
-				log.Info(err.Error())
-				wrappedError := pkgerrors.Wrapf(err, "error applying object Name: %s Kind: %s", template.GetName(), template.GetKind())
-				SetHubCondition(&m.Status, *NewHubCondition(operatorv1.ComponentFailure+": "+operatorv1.HubConditionType(template.GetName())+"(Kind:)"+operatorv1.HubConditionType(template.GetKind()), metav1.ConditionTrue, FailedApplyingComponent, wrappedError.Error()))
-				return ctrl.Result{}, wrappedError
+
+		existing := template.DeepCopy()
+		if err := r.Client.Get(ctx, types.NamespacedName{Name: existing.GetName(),
+			Namespace: existing.GetNamespace()}, existing); err != nil {
+			// Template resource does not exist
+			if errors.IsNotFound(err) {
+				if err := r.Client.Create(ctx, template, &client.CreateOptions{}); err != nil {
+					return r.logAndSetCondition(err, "failed to create resource", template, m)
+				}
+				log.Info("Creating resource", "Kind", template.GetKind(), "Name", template.GetName())
 			} else {
-				r.Log.Info("Creating resource", "Name", template.GetName(), "Kind", template.GetKind())
+				return r.logAndSetCondition(err, "failed to get resource", existing, m)
 			}
 		} else {
-			// resource found
+			desiredVersion := os.Getenv("OPERATOR_VERSION")
+			if desiredVersion == "" {
+				log.Info("Warning: OPERATOR_VERSION environment variable is not set")
+			}
+
+			if !r.ensureResourceVersionAlignment(existing, desiredVersion) {
+				condition := NewHubCondition(
+					operatorv1.Progressing, metav1.ConditionTrue, ComponentsUpdatingReason,
+					fmt.Sprintf("Updating %s/%s to target version: %s.", template.GetKind(),
+						template.GetName(), desiredVersion),
+				)
+				SetHubCondition(&m.Status, *condition)
+			}
+
+			// Resource exists; use the original template for patching to avoid issues with managedFields
+			// Apply the object data.
 			force := true
-			err := r.Client.Patch(ctx, template, client.Apply, &client.PatchOptions{Force: &force, FieldManager: "multiclusterhub-operator"})
-			if err != nil {
-				log.Info(err.Error())
-				wrappedError := pkgerrors.Wrapf(err, "error applying object Name: %s Kind: %s", template.GetName(), template.GetKind())
-				SetHubCondition(&m.Status, *NewHubCondition(operatorv1.ComponentFailure+": "+operatorv1.HubConditionType(template.GetName())+"(Kind:)"+operatorv1.HubConditionType(template.GetKind()), metav1.ConditionTrue, FailedApplyingComponent, wrappedError.Error()))
-				return ctrl.Result{}, wrappedError
+			if err := r.Client.Patch(ctx, template, client.Apply, &client.PatchOptions{
+				Force: &force, FieldManager: "multiclusterhub-operator"}); err != nil {
+				return r.logAndSetCondition(err, "failed to update resource", template, m)
 			}
 		}
 	}
+
 	return ctrl.Result{}, nil
 }
 
@@ -1243,7 +1256,7 @@ func (r *MultiClusterHubReconciler) deleteTemplate(ctx context.Context, m *opera
 		log.Error(err, "Failed to delete template")
 		return ctrl.Result{}, err
 	} else {
-		r.Log.Info("Finalizing template... Deleting resource", "Name", template.GetName(), "Kind", template.GetKind())
+		r.Log.Info("Finalizing template... Deleting resource", "Kind", template.GetKind(), "Name", template.GetName())
 	}
 	return ctrl.Result{}, nil
 }
@@ -1522,8 +1535,7 @@ func (r *MultiClusterHubReconciler) installCRDs(reqLogger logr.Logger, m *operat
 		utils.AddInstallerLabel(crd, m.GetName(), m.GetNamespace())
 		err, ok := deploying.Deploy(r.Client, crd)
 		if err != nil {
-			err := fmt.Errorf("failed to deploy %s %s", crd.GetKind(), crd.GetName())
-			reqLogger.Error(err, err.Error())
+			reqLogger.Error(err, "failed to deploy", "Kind", crd.GetKind(), "Name", crd.GetName())
 			return DeployFailedReason, err
 		}
 		if ok {
@@ -1597,10 +1609,10 @@ func (r *MultiClusterHubReconciler) deployResources(reqLogger logr.Logger, m *op
 		}
 		err, ok := deploying.Deploy(r.Client, res)
 		if err != nil {
-			err := fmt.Errorf("failed to deploy %s %s", res.GetKind(), res.GetName())
-			reqLogger.Error(err, err.Error())
+			reqLogger.Error(err, "failed to deploy resource", "Kind", res.GetKind(), "Name", res.GetName())
 			return DeployFailedReason, err
 		}
+
 		if ok {
 			message := fmt.Sprintf("created new resource: %s %s", res.GetKind(), res.GetName())
 			condition := NewHubCondition(operatorv1.Progressing, metav1.ConditionTrue, NewComponentReason, message)
@@ -1755,3 +1767,44 @@ func (r *MultiClusterHubReconciler) CheckDeprecatedFieldUsage(m *operatorv1.Mult
 		}
 	}
 }
+
+func (r *MultiClusterHubReconciler) ensureResourceVersionAlignment(template *unstructured.Unstructured,
+	desiredVersion string) bool {
+	if desiredVersion == "" {
+		return false
+	}
+
+	// Check the release version annotation on the existing resource
+	annotations := template.GetAnnotations()
+	currentVersion, ok := annotations[utils.AnnotationReleaseVersion]
+	if !ok {
+		log.Info(fmt.Sprintf("Annotation '%v' not found on resource", utils.AnnotationReleaseVersion),
+			"Kind", template.GetKind(), "Name", template.GetName())
+		return false
+	}
+
+	if currentVersion != desiredVersion {
+		log.Info("Resource version mismatch detected; attempting to update resource",
+			"Kind", template.GetName(), "Name", template.GetKind(),
+			"CurrentVersion", currentVersion, "DesiredVersion", desiredVersion)
+
+		return false
+	}
+
+	return true // Resource is aligned with the desired version
+}
+
+func (r *MultiClusterHubReconciler) logAndSetCondition(err error, message string,
+	template *unstructured.Unstructured, m *operatorv1.MultiClusterHub) (ctrl.Result, error) {
+
+	log.Error(err, message, "Kind", template.GetKind(), "Name", template.GetName())
+	wrappedError := pkgerrors.Wrapf(err, "%s Kind: %s Name: %s", message, template.GetName(), template.GetKind())
+
+	condType := fmt.Sprintf("%v: %v (Kind:%v)", operatorv1.ComponentFailure, template.GetName(),
+		template.GetKind())
+
+	SetHubCondition(&m.Status, *NewHubCondition(operatorv1.HubConditionType(condType), metav1.ConditionTrue,
+		FailedApplyingComponent, wrappedError.Error()))
+
+	return ctrl.Result{}, wrappedError
+}

controllers/multiclusterhub_controller_test.go

@@ -1821,3 +1821,113 @@ func Test_applyEnvConfig(t *testing.T) {
 		})
 	}
 }
+
+func Test_logAndSetCondition(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      error
+		mch      *operatorv1.MultiClusterHub
+		message  string
+		template *unstructured.Unstructured
+	}{
+		{
+			name: "should log and set condition of MCH",
+			err:  fmt.Errorf("Test error"),
+			mch: &operatorv1.MultiClusterHub{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "multiclusterhub",
+					Namespace: "test-ns",
+				},
+			},
+			message: "This is a test error",
+			template: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "Deployment",
+					"metadata": map[string]interface{}{
+						"name":      "test-deployment",
+						"namespace": "test-ns",
+					},
+					"spec": map[string]interface{}{},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			if _, err := recon.logAndSetCondition(tt.err, tt.message, tt.template, tt.mch); err == nil {
+				t.Errorf("logAndSetCondition() = %v, expected an error", err)
+			}
+		})
+	}
+}
+
+func Test_ensureResourceVersionAlignment(t *testing.T) {
+	tests := []struct {
+		name     string
+		mch      *operatorv1.MultiClusterHub
+		template *unstructured.Unstructured
+		want     bool
+	}{
+		{
+			name: "should ensure resource version alignment is false",
+			mch: &operatorv1.MultiClusterHub{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "multiclusterhub",
+					Namespace: "test-ns",
+				},
+			},
+			template: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "Deployment",
+					"metadata": map[string]interface{}{
+						"annotations": map[string]interface{}{
+							utils.AnnotationReleaseVersion: "2.12.0",
+						},
+						"name":      "test-deployment",
+						"namespace": "test-ns",
+					},
+					"spec": map[string]interface{}{},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "should ensure resource version alignment is true",
+			mch: &operatorv1.MultiClusterHub{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "multiclusterhub",
+					Namespace: "test-ns",
+				},
+			},
+			template: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "apps/v1",
+					"kind":       "Deployment",
+					"metadata": map[string]interface{}{
+						"annotations": map[string]interface{}{
+							utils.AnnotationReleaseVersion: "2.13.0",
+						},
+						"name":      "test-deployment",
+						"namespace": "test-ns",
+					},
+					"spec": map[string]interface{}{},
+				},
+			},
+			want: true,
+		},
+	}
+
+	os.Setenv("OPERATOR_VERSION", "2.13.0")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := recon.ensureResourceVersionAlignment(tt.template, os.Getenv("OPERATOR_VERSION"))
+			if got != tt.want {
+				t.Errorf("ensureResourceVersionAlignment() = %v, want: %v", got, tt.want)
+			}
+		})
+	}
+}

controllers/status.go

@@ -34,6 +34,8 @@ const (
 	// ComponentsUnavailableReason is added in a hub when one or more components are
 	// in an unready state
 	ComponentsUnavailableReason = "ComponentsUnavailable"
+	// ComponentUpdatingReason is added when the hub is actively updating a component resource
+	ComponentsUpdatingReason = "UpdatingComponentResource"
 	// NewComponentReason is added when the hub creates a new install resource successfully
 	NewComponentReason = "NewResourceCreated"
 	// DeployFailedReason is added when the hub fails to deploy a resource
@@ -189,6 +191,7 @@ func calculateStatus(hub *operatorsv1.MultiClusterHub, allDeps []*appsv1.Deploym
 			progressing := NewHubCondition(operatorsv1.Progressing, metav1.ConditionTrue, ReconcileReason, "Hub is reconciling.")
 			SetHubCondition(&status, *progressing)
 		}
+
 		// only add unavailable status if complete status already present
 		if HubConditionPresent(status, operatorsv1.Complete) {
 			unavailable := NewHubCondition(operatorsv1.Complete, metav1.ConditionFalse, ComponentsUnavailableReason, "Not all hub components ready.")
@@ -533,14 +536,15 @@ func aggregatePhase(status operatorsv1.MultiClusterHubStatus) operatorsv1.HubPha
 	case cv == "":
 		// Hub has not reached success for first time
 		return operatorsv1.HubInstalling
-	case cv != version.Version:
 
+	case cv != version.Version:
 		if HubConditionPresent(status, operatorsv1.Blocked) {
 			return operatorsv1.HubUpdatingBlocked
 		} else {
 			// Hub has not completed upgrade to newest version
 			return operatorsv1.HubUpdating
 		}
+
 	default:
 		// Hub has reached desired version, but degraded
 		return operatorsv1.HubPending
@@ -578,7 +582,7 @@ func RemoveHubCondition(status *operatorsv1.MultiClusterHubStatus, condType oper
 	status.HubConditions = filterOutCondition(status.HubConditions, condType)
 }
 
-// FindHubCondition returns the condition you're looking for or nil.
+// GetHubCondition returns the condition you're looking for or nil.
 func GetHubCondition(status operatorsv1.MultiClusterHubStatus, condType operatorsv1.HubConditionType) *operatorsv1.HubCondition {
 	for i := range status.HubConditions {
 		c := status.HubConditions[i]
@@ -623,7 +627,7 @@ func filterOutConditionWithSubstring(conditions []operatorsv1.HubCondition, subs
 	return newConditions
 }
 
-// IsHubConditionPresentAndEqual indicates if the condition is present and equal to the given status.
+// HubConditionPresent indicates if the condition is present and equal to the given status.
 func HubConditionPresent(status operatorsv1.MultiClusterHubStatus, conditionType operatorsv1.HubConditionType) bool {
 	for _, condition := range status.HubConditions {
 		if condition.Type == conditionType {

controllers/uninstall.go

@@ -238,7 +238,7 @@ func (r *MultiClusterHubReconciler) ensureRemovalsGone(m *operatorsv1.MultiClust
 // uninstall return true if resource does not exist and returns an error if a GET or DELETE errors unexpectedly. A false response without error
 // means the resource is in the process of deleting.
 func (r *MultiClusterHubReconciler) uninstall(m *operatorsv1.MultiClusterHub, u *unstructured.Unstructured) (bool, error) {
-	obLog := r.Log.WithValues("Namespace", u.GetNamespace(), "Name", u.GetName(), "Kind", u.GetKind())
+	obLog := r.Log.WithValues("Namespace", u.GetNamespace(), "Kind", u.GetKind(), "Name", u.GetName())
 
 	err := r.Client.Get(context.TODO(), types.NamespacedName{
 		Name:      u.GetName(),