fixup! fixup! regression: Set resource version for CRs

openshift · Oct 10, 2024 · 3773374 · 3773374
1 parent e4a33c9
commit 3773374
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 30 deletions.
diff --git a/pkg/operator/resource/resourceapply/monitoring.go b/pkg/operator/resource/resourceapply/monitoring.go
@@ -139,6 +139,10 @@ func ApplyUnstructuredResourceImproved(
 	// Check if the metadata objects differ. This only checks for selective fields (excluding the resource version, among others).
 	didMetadataModify := ptr.To(false)
 	resourcemerge.EnsureObjectMeta(didMetadataModify, &existingObjectMetaTyped, requiredObjectMetaTyped)
+	existingCopy.Object["metadata"], err = runtime.DefaultUnstructuredConverter.ToUnstructured(&existingObjectMetaTyped)
+	if err != nil {
+		return nil, false, err
+	}
 
 	// Deep-check the spec objects for equality, and update the cache in either case.
 	if defaultingFunc == nil {
@@ -162,14 +166,10 @@ func ApplyUnstructuredResourceImproved(
 		klog.Infof("%s %q changes: %v", resourceGVR.String(), namespace+"/"+name, JSONPatchNoError(existing, existingCopy))
 	}
 
-	// CRs, unlike certain core objects (e.g., deployments), need to specify the resource version for updates.
-	requiredCopy := required.DeepCopy()
-	requiredCopy.SetResourceVersion(existingCopy.GetResourceVersion())
-
 	// Perform update if resource exists but different from the required (desired) one.
-	actual, err := client.Resource(resourceGVR).Namespace(namespace).Update(ctx, requiredCopy, metav1.UpdateOptions{})
-	resourcehelper.ReportUpdateEvent(recorder, requiredCopy, err)
-	cache.UpdateCachedResourceMetadata(requiredCopy, actual)
+	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
 }
 

diff --git a/pkg/operator/resource/resourceapply/monitoring_test.go b/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(requiredMonitorMetadata, existingMonitorMetadata))
 				}
 			}
 		})