fixup! fixup! fixup! regression: Set resource version for CRs

pkg/operator/resource/resourceapply/monitoring.go

@@ -2,23 +2,19 @@ package resourceapply
 
 import (
 	"context"
-	errorsstdlib "errors"
-	"fmt"
 
+	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
 
 	"github.com/openshift/library-go/pkg/operator/events"
 	"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
-
-	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 )
 
 var alertmanagerGVR = schema.GroupVersionResource{Group: "monitoring.coreos.com", Version: "v1", Resource: "alertmanagers"}
@@ -88,7 +84,8 @@ func ApplyUnstructuredResourceImproved(
 	}
 	existing, err := client.Resource(resourceGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
-		want, err := client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{})
+		var want *unstructured.Unstructured
+		want, err = client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{})
 		resourcehelper.ReportCreateEvent(recorder, required, err)
 		cache.UpdateCachedResourceMetadata(required, want)
 		return want, true, err
@@ -102,44 +99,11 @@ func ApplyUnstructuredResourceImproved(
 		return existing, false, nil
 	}
 
-	// Ensure metadata field is present on the object.
 	existingCopy := existing.DeepCopy()
-	existingObjectMeta, found, err := unstructured.NestedMap(existingCopy.Object, "metadata")
-	if err != nil {
-		return nil, false, err
-	}
-	if !found {
-		return nil, false, errorsstdlib.New(fmt.Sprintf("metadata not found in the existing object: %s/%s", existing.GetNamespace(), existingCopy.GetName()))
-	}
-	requiredObjectMeta, found, err := unstructured.NestedMap(required.Object, "metadata")
-	if err != nil {
-		return nil, false, err
-	}
-	if !found {
-		return nil, false, errorsstdlib.New(fmt.Sprintf("metadata not found in the required object: %s/%s", required.GetNamespace(), required.GetName()))
-	}
-
-	// Cast the metadata to the correct type.
-	var existingObjectMetaTyped, requiredObjectMetaTyped metav1.ObjectMeta
-	err = runtime.DefaultUnstructuredConverter.FromUnstructured(existingObjectMeta, &existingObjectMetaTyped)
-	if err != nil {
-		return nil, false, err
-	}
-	err = runtime.DefaultUnstructuredConverter.FromUnstructured(requiredObjectMeta, &requiredObjectMetaTyped)
-	if err != nil {
-		return nil, false, err
-	}
-
-	// Fail-fast if the resource versions differ.
-	if requiredObjectMetaTyped.ResourceVersion != "" && existingObjectMetaTyped.ResourceVersion != requiredObjectMetaTyped.ResourceVersion {
-		err = errors.NewConflict(resourceGVR.GroupResource(), name, fmt.Errorf("rejected to update %s %s because the object has been modified: desired/actual ResourceVersion: %v/%v", existing.GetKind(), existing.GetName(), requiredObjectMetaTyped.ResourceVersion, existingObjectMetaTyped.ResourceVersion))
-		return nil, false, err
-	}
 
-	// Check if the metadata objects differ. This only checks for selective fields (excluding the resource version, among others).
+	// Replace and/or merge certain metadata fields.
 	didMetadataModify := ptr.To(false)
-	resourcemerge.EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped)
-	existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped)
+	err = resourcemerge.EnsureObjectMetaForUnstructured(didMetadataModify, existingCopy, required)
 	if err != nil {
 		return nil, false, err
 	}
@@ -151,23 +115,24 @@ func ApplyUnstructuredResourceImproved(
 	if equalityChecker == nil {
 		equalityChecker = equality.Semantic
 	}
-	existingCopy, didSpecModify, err := ensureGenericSpec(required, existingCopy, defaultingFunc, equalityChecker)
+	didSpecModify := ptr.To(false)
+	err = ensureGenericSpec(didSpecModify, required, existingCopy, defaultingFunc, equalityChecker)
 	if err != nil {
 		return nil, false, err
 	}
-	if !didSpecModify && !*didMetadataModify {
+	if !*didSpecModify && !*didMetadataModify {
 		// Update cache even if certain fields are not modified, in order to maintain a consistent cache based on the
 		// resource hash. The resource hash depends on the entire metadata, not just the fields that were checked above,
 		cache.UpdateCachedResourceMetadata(required, existingCopy)
 		return existingCopy, false, nil
 	}
 
+	// Perform update if resource exists but different from the required (desired) one.
 	if klog.V(4).Enabled() {
 		klog.Infof("%s %q changes: %v", resourceGVR.String(), namespace+"/"+name, JSONPatchNoError(existing, existingCopy))
 	}
-
-	// Perform update if resource exists but different from the required (desired) one.
-	actual, err := client.Resource(resourceGVR).Namespace(namespace).Update(ctx, existingCopy, metav1.UpdateOptions{})
+	var actual *unstructured.Unstructured
+	actual, err = client.Resource(resourceGVR).Namespace(namespace).Update(ctx, existingCopy, metav1.UpdateOptions{})
 	resourcehelper.ReportUpdateEvent(recorder, existingCopy, err)
 	cache.UpdateCachedResourceMetadata(existingCopy, actual)
 	return actual, true, err
@@ -186,27 +151,27 @@ func DeleteUnstructuredResource(ctx context.Context, client dynamic.Interface, r
 	return nil, true, nil
 }
 
-func ensureGenericSpec(required, existing *unstructured.Unstructured, mimicDefaultingFn mimicDefaultingFunc, equalityChecker equalityChecker) (*unstructured.Unstructured, bool, error) {
+func ensureGenericSpec(didSpecModify *bool, required, existing *unstructured.Unstructured, mimicDefaultingFn mimicDefaultingFunc, equalityChecker equalityChecker) error {
 	mimicDefaultingFn(required)
 	requiredSpec, _, err := unstructured.NestedMap(required.UnstructuredContent(), "spec")
 	if err != nil {
-		return nil, false, err
+		return err
 	}
 	existingSpec, _, err := unstructured.NestedMap(existing.UnstructuredContent(), "spec")
 	if err != nil {
-		return nil, false, err
+		return err
 	}
 
 	if equalityChecker.DeepEqual(existingSpec, requiredSpec) {
-		return existing, false, nil
+		return nil
 	}
 
-	existingCopy := existing.DeepCopy()
-	if err := unstructured.SetNestedMap(existingCopy.UnstructuredContent(), requiredSpec, "spec"); err != nil {
-		return nil, true, err
+	if err = unstructured.SetNestedMap(existing.UnstructuredContent(), requiredSpec, "spec"); err != nil {
+		return err
 	}
+	*didSpecModify = true
 
-	return existingCopy, true, nil
+	return nil
 }
 
 // mimicDefaultingFunc is used to set fields that are defaulted.  This allows for sparse manifests to apply correctly.

pkg/operator/resource/resourceapply/monitoring_test.go

@@ -147,7 +147,7 @@ func TestApplyServiceMonitor(t *testing.T) {
 				requiredMonitorSpec, _, _ := unstructured.NestedMap(unstructuredServiceMonitor.UnstructuredContent(), "spec")
 				existingMonitorSpec, _, _ := unstructured.NestedMap(updatedMonitorObj.UnstructuredContent(), "spec")
 				if !equality.Semantic.DeepEqual(requiredMonitorSpec, existingMonitorSpec) {
-					t.Fatalf("expected resulting service monitor spec to match required spec: %s", cmp.Diff(requiredMonitorMetadata, existingMonitorMetadata))
+					t.Fatalf("expected resulting service monitor spec to match required spec: %s", cmp.Diff(requiredMonitorSpec, existingMonitorSpec))
 				}
 			}
 		})

pkg/operator/resource/resourcemerge/object_merger.go

@@ -1,10 +1,14 @@
 package resourcemerge
 
 import (
+	errorsstdlib "errors"
+	"fmt"
 	"reflect"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
@@ -18,6 +22,47 @@ func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required meta
 	MergeOwnerRefs(modified, &existing.OwnerReferences, required.OwnerReferences)
 }
 
+func EnsureObjectMetaForUnstructured(didMetadataModify *bool, existingCopy *unstructured.Unstructured, required *unstructured.Unstructured) error {
+
+	// Ensure metadata field is present on the object.
+	existingObjectMeta, found, err := unstructured.NestedMap(existingCopy.Object, "metadata")
+	if err != nil {
+		return err
+	}
+	if !found {
+		return errorsstdlib.New(fmt.Sprintf("metadata not found in the existing object: %s/%s", existingCopy.GetNamespace(), existingCopy.GetName()))
+	}
+	requiredObjectMeta, found, err := unstructured.NestedMap(required.Object, "metadata")
+	if err != nil {
+		return err
+	}
+	if !found {
+		return errorsstdlib.New(fmt.Sprintf("metadata not found in the required object: %s/%s", required.GetNamespace(), required.GetName()))
+	}
+
+	// Cast the metadata to the correct type.
+	var existingObjectMetaTyped, requiredObjectMetaTyped metav1.ObjectMeta
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(existingObjectMeta, &existingObjectMetaTyped)
+	if err != nil {
+		return err
+	}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(requiredObjectMeta, &requiredObjectMetaTyped)
+	if err != nil {
+		return err
+	}
+
+	// Check if the metadata objects differ. This only checks for selective fields (excluding the resource version, among others).
+	EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped)
+	if *didMetadataModify {
+		existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // WithCleanLabelsAndAnnotations cleans the metadata off the removal annotations/labels/ownerrefs
 // (those that end with trailing "-")
 func WithCleanLabelsAndAnnotations(obj metav1.Object) metav1.Object {