Merge pull request #1823 from rexagod/regression-1575

fix: Set resource version for CRs

Makefile

@@ -26,5 +26,9 @@ verify-podnetworkconnectivitychecks:
 test-e2e-encryption: GO_TEST_PACKAGES :=./test/e2e-encryption/...
 .PHONY: test-e2e-encryption
 
-test-e2e-monitoring:
+test-e2e-monitoring: GO_TEST_PACKAGES :=./test/e2e-monitoring/...
+test-e2e-monitoring: GO_TEST_FLAGS += -v
+test-e2e-monitoring: GO_TEST_FLAGS += -timeout 5m
+test-e2e-monitoring: GO_TEST_FLAGS += -p 1
+test-e2e-monitoring: test-unit
 .PHONY: test-e2e-monitoring

pkg/operator/resource/resourceapply/monitoring.go

@@ -2,23 +2,17 @@ package resourceapply
 
 import (
 	"context"
-	errorsstdlib "errors"
-	"fmt"
 
+	"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"
 	"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,10 +82,10 @@ 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{})
-		resourcehelper.ReportCreateEvent(recorder, required, err)
+		want, errCreate := client.Resource(resourceGVR).Namespace(namespace).Create(ctx, required, metav1.CreateOptions{})
+		resourcehelper.ReportCreateEvent(recorder, required, errCreate)
 		cache.UpdateCachedResourceMetadata(required, want)
-		return want, true, err
+		return want, true, errCreate
 	}
 	if err != nil {
 		return nil, false, err
@@ -102,71 +96,42 @@ 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)
+	// Replace and/or merge certain metadata fields.
+	didMetadataModify := false
+	err = resourcemerge.EnsureObjectMetaForUnstructured(&didMetadataModify, existingCopy, required)
 	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.
-	didMetadataModify := ptr.To(false)
-	resourcemerge.EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped)
-
 	// Deep-check the spec objects for equality, and update the cache in either case.
 	if defaultingFunc == nil {
 		defaultingFunc = noDefaulting
 	}
 	if equalityChecker == nil {
 		equalityChecker = equality.Semantic
 	}
-	existingCopy, didSpecModify, err := ensureGenericSpec(required, existingCopy, defaultingFunc, equalityChecker)
+	didSpecModify := 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, required, metav1.UpdateOptions{})
-	resourcehelper.ReportUpdateEvent(recorder, required, err)
-	cache.UpdateCachedResourceMetadata(required, actual)
-	return actual, true, err
+	actual, errUpdate := client.Resource(resourceGVR).Namespace(namespace).Update(ctx, existingCopy, metav1.UpdateOptions{})
+	resourcehelper.ReportUpdateEvent(recorder, existingCopy, errUpdate)
+	cache.UpdateCachedResourceMetadata(existingCopy, actual)
+	return actual, true, errUpdate
 }
 
 // DeleteUnstructuredResource deletes the unstructured resource.
@@ -182,27 +147,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

@@ -2,14 +2,16 @@ package resourceapply
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"k8s.io/apimachinery/pkg/api/equality"
 	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/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/json"
 	dynamicfake "k8s.io/client-go/dynamic/fake"
 	clienttesting "k8s.io/client-go/testing"
@@ -19,31 +21,48 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 )
 
+var structuredServiceMonitor = pov1api.ServiceMonitor{
+	TypeMeta: metav1.TypeMeta{
+		APIVersion: "monitoring.coreos.com/v1",
+		Kind:       "ServiceMonitor",
+	},
+	ObjectMeta: metav1.ObjectMeta{
+		Name:      "test-sm",
+		Namespace: "test-ns",
+		Labels:    map[string]string{"app": "test-app"},
+	},
+	Spec: pov1api.ServiceMonitorSpec{
+		NamespaceSelector: pov1api.NamespaceSelector{
+			MatchNames: []string{"test-ns"},
+		},
+	},
+}
+
 func TestApplyServiceMonitor(t *testing.T) {
 	dynamicScheme := runtime.NewScheme()
 	dynamicScheme.AddKnownTypeWithName(schema.GroupVersionKind{Group: "monitoring.coreos.com", Version: "v1", Kind: "ServiceMonitor"}, &unstructured.Unstructured{})
 
-	structuredServiceMonitor := pov1api.ServiceMonitor{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: "monitoring.coreos.com/v1",
-			Kind:       "ServiceMonitor",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test-sm",
-			Namespace: "test-ns",
-			Labels:    map[string]string{"app": "test-app"},
-		},
-		Spec: pov1api.ServiceMonitorSpec{
-			NamespaceSelector: pov1api.NamespaceSelector{
-				MatchNames: []string{"test-ns"},
-			},
-		},
-	}
 	unstructuredServiceMonitor := structuredToUnstructuredServiceMonitor(&structuredServiceMonitor)
 
 	structuredServiceMonitorDifferentLabels := structuredServiceMonitor.DeepCopy()
-	structuredServiceMonitorDifferentLabels.Labels = map[string]string{"app": "different-test-app"}
+	structuredServiceMonitorDifferentLabels.Labels = map[string]string{"fake-app": "fake-test-app"}
 	unstructuredServiceMonitorDifferentLabels := structuredToUnstructuredServiceMonitor(structuredServiceMonitorDifferentLabels)
+	serviceMonitorDifferentLabelsValidationFunc := func(oI interface{}) error {
+		o, ok := oI.(*unstructured.Unstructured)
+		if !ok {
+			return errors.New("unexpected object type")
+		}
+		gotLabels := o.GetLabels()
+		wantLabels := structuredServiceMonitor.Labels
+		for k, v := range structuredServiceMonitorDifferentLabels.Labels {
+			wantLabels[k] = v
+		}
+		if !equality.Semantic.DeepEqual(gotLabels, wantLabels) {
+			return errors.New(fmt.Sprintf("service monitor labels were not merged correctly (+got, -want): %s", cmp.Diff(gotLabels, wantLabels)))
+		}
+
+		return nil
+	}
 
 	structuredServiceMonitorDifferentSpec := structuredServiceMonitor.DeepCopy()
 	structuredServiceMonitorDifferentSpec.Spec.NamespaceSelector.MatchNames = []string{"different-test-ns"}
@@ -58,6 +77,7 @@ func TestApplyServiceMonitor(t *testing.T) {
 		existing                           *unstructured.Unstructured
 		expectExistingResourceToBeModified bool
 		expectActionsDuringModification    []string
+		delegateValidation                 func(interface{}) error
 	}{
 		{
 			name:                            "same label, same spec",
@@ -69,6 +89,7 @@ func TestApplyServiceMonitor(t *testing.T) {
 			existing:                           unstructuredServiceMonitorDifferentLabels,
 			expectExistingResourceToBeModified: true,
 			expectActionsDuringModification:    []string{"get", "update"},
+			delegateValidation:                 serviceMonitorDifferentLabelsValidationFunc,
 		},
 		{
 			name:                               "same label, different spec",
@@ -85,7 +106,7 @@ func TestApplyServiceMonitor(t *testing.T) {
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			dynamicClient := dynamicfake.NewSimpleDynamicClient(dynamicScheme, tc.existing)
-			_, modified, err := ApplyServiceMonitor(context.TODO(), dynamicClient, events.NewInMemoryRecorder("monitor-test"), unstructuredServiceMonitor)
+			got, modified, err := ApplyServiceMonitor(context.TODO(), dynamicClient, events.NewInMemoryRecorder("monitor-test"), unstructuredServiceMonitor)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}
@@ -103,26 +124,30 @@ func TestApplyServiceMonitor(t *testing.T) {
 				}
 			}
 
-			if len(tc.expectActionsDuringModification) > 1 &&
+			if tc.delegateValidation != nil {
+				if err = tc.delegateValidation(got); err != nil {
+					t.Fatalf("delegated validation failed: %v", err)
+				}
+			} else if len(tc.expectActionsDuringModification) > 1 &&
 				tc.expectActionsDuringModification[1] == "update" {
 				updateAction, isUpdate := actions[1].(clienttesting.UpdateAction)
 				if !isUpdate {
 					t.Fatalf("expected second action to be update, got %+v", actions[1])
 				}
 				updatedMonitorObj := updateAction.GetObject().(*unstructured.Unstructured)
 
-				// Verify `metadata`.
+				// Verify `metadata`. Note that the `metadata` is merged in some cases.
 				requiredMonitorMetadata, _, _ := unstructured.NestedMap(unstructuredServiceMonitor.UnstructuredContent(), "metadata")
 				existingMonitorMetadata, _, _ := unstructured.NestedMap(updatedMonitorObj.UnstructuredContent(), "metadata")
 				if !equality.Semantic.DeepEqual(requiredMonitorMetadata, existingMonitorMetadata) {
-					t.Fatalf("expected resulting service monitor metadata to match required metadata: %s", diff.ObjectDiff(requiredMonitorMetadata, existingMonitorMetadata))
+					t.Fatalf("expected resulting service monitor metadata to match required metadata: %s", cmp.Diff(requiredMonitorMetadata, existingMonitorMetadata))
 				}
 
 				// Verify `spec`.
 				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", diff.ObjectDiff(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,48 @@ func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required meta
 	MergeOwnerRefs(modified, &existing.OwnerReferences, required.OwnerReferences)
 }
 
+func EnsureObjectMetaForUnstructured(modified *bool, existing *unstructured.Unstructured, required *unstructured.Unstructured) error {
+
+	// Ensure metadata field is present on the object.
+	existingObjectMeta, found, err := unstructured.NestedMap(existing.Object, "metadata")
+	if err != nil {
+		return err
+	}
+	if !found {
+		return errorsstdlib.New(fmt.Sprintf("metadata not found in the existing object: %s/%s", existing.GetNamespace(), existing.GetName()))
+	}
+	var requiredObjectMeta map[string]interface{}
+	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(modified, &existingObjectMetaTyped, requiredObjectMetaTyped)
+	if *modified {
+		existing.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 {

test/e2e-monitoring/OWNERS

@@ -0,0 +1,9 @@
+reviewers:
+  - dgrisonnet
+  - p0lyn0mial
+  - tkashem
+  - rexagod
+approvers:
+  - dgrisonnet
+  - p0lyn0mial
+  - tkashem