Change ComputeRequestIdentifier parameters to clarify usage

Signed-off-by: Lennart Jern <[email protected]>

Author: lentzi90

internal/controllers/topology/cluster/structuredmerge/dryrun.go

@@ -54,7 +54,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
 	// The identifier consists of: gvk, namespace, name and resourceVersion of originalUnstructured
 	// and a hash of modifiedUnstructured.
 	// This ensures that we re-run the request as soon as either original or modified changes.
-	requestIdentifier, err := ssa.ComputeRequestIdentifier(dryRunCtx.client.Scheme(), dryRunCtx.originalUnstructured, dryRunCtx.modifiedUnstructured)
+	requestIdentifier, err := ssa.ComputeRequestIdentifier(dryRunCtx.client.Scheme(), dryRunCtx.originalUnstructured.GetResourceVersion(), dryRunCtx.modifiedUnstructured)
 	if err != nil {
 		return false, false, nil, err
 	}

internal/util/ssa/cache.go

@@ -104,18 +104,18 @@ func (r *ssaCache) Has(key, kind string) bool {
 // ComputeRequestIdentifier computes a request identifier for the cache.
 // The identifier is unique for a specific request to ensure we don't have to re-run the request
 // once we found out that it would not produce a diff.
-// The identifier consists of: gvk, namespace, name and resourceVersion of the original object and a hash of the modified
-// object. This ensures that we re-run the request as soon as either original or modified changes.
-func ComputeRequestIdentifier(scheme *runtime.Scheme, original, modified client.Object) (string, error) {
-	modifiedObjectHash, err := hash.Compute(modified)
+// The identifier consists of: gvk, namespace, name and resourceVersion of the object and a hash of the modified
+// object. This ensures that we re-run the request as soon as anything changes.
+func ComputeRequestIdentifier(scheme *runtime.Scheme, resourceVersion string, obj client.Object) (string, error) {
+	objHash, err := hash.Compute(obj)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to calculate request identifier: failed to compute hash for modified object")
+		return "", errors.Wrapf(err, "failed to calculate request identifier: failed to compute hash for object")
 	}
 
-	gvk, err := apiutil.GVKForObject(original, scheme)
+	gvk, err := apiutil.GVKForObject(obj, scheme)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to calculate request identifier: failed to get GroupVersionKind of original object %s", klog.KObj(original))
+		return "", errors.Wrapf(err, "failed to calculate request identifier: failed to get GroupVersionKind of object %s", klog.KObj(obj))
 	}
 
-	return fmt.Sprintf("%s.%s.%s.%d", gvk.String(), klog.KObj(original), original.GetResourceVersion(), modifiedObjectHash), nil
+	return fmt.Sprintf("%s.%s.%s.%d", gvk.String(), klog.KObj(obj), resourceVersion, objHash), nil
 }

internal/util/ssa/patch.go

@@ -94,7 +94,7 @@ func Patch(ctx context.Context, c client.Client, fieldManager string, modified c
 	var requestIdentifier string
 	if options.WithCachingProxy {
 		// Check if the request is cached.
-		requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), options.Original, modifiedUnstructured)
+		requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), options.Original.GetResourceVersion(), modifiedUnstructured)
 		if err != nil {
 			return errors.Wrapf(err, "failed to apply object")
 		}
@@ -135,9 +135,9 @@ func Patch(ctx context.Context, c client.Client, fieldManager string, modified c
 	if options.WithCachingProxy && !options.WithDryRun {
 		// If the object changed, we need to recompute the request identifier before caching.
 		if options.Original.GetResourceVersion() != modifiedUnstructured.GetResourceVersion() {
-			// NOTE: This takes the resourceVersion from modifiedUnstructured, and the hash from
-			// modifiedUnstructuredBeforeApply, which is what we want.
-			requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), modifiedUnstructured, modifiedUnstructuredBeforeApply)
+			// NOTE: This uses the resourceVersion from modifiedUnstructured (after apply), and the hash from
+			// modifiedUnstructuredBeforeApply (what we wanted to apply), which is what we want.
+			requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), modifiedUnstructured.GetResourceVersion(), modifiedUnstructuredBeforeApply)
 			if err != nil {
 				return errors.Wrapf(err, "failed to compute request identifier after apply")
 			}

internal/util/ssa/patch_test.go

@@ -87,7 +87,7 @@ func TestPatch(t *testing.T) {
 		// Compute request identifier before the update, so we can later verify that the update call was not cached with this identifier.
 		modifiedUnstructured, err := prepareModified(env.Scheme(), modifiedObject)
 		g.Expect(err).ToNot(HaveOccurred())
-		oldRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), originalObject, modifiedUnstructured)
+		oldRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), originalObject.GetResourceVersion(), modifiedUnstructured)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Save a copy of modifiedUnstructured before apply to compute the new identifier later
 		modifiedUnstructuredBeforeApply := modifiedUnstructured.DeepCopy()
@@ -101,7 +101,7 @@ func TestPatch(t *testing.T) {
 		objectAfterApply := initialObject.DeepCopy()
 		g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(objectAfterApply), objectAfterApply)).To(Succeed())
 		// Compute the new request identifier (after apply)
-		newRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), objectAfterApply, modifiedUnstructuredBeforeApply)
+		newRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), objectAfterApply.GetResourceVersion(), modifiedUnstructuredBeforeApply)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Verify that request was cached with the new identifier (after apply)
 		g.Expect(ssaCache.Has(newRequestIdentifier, initialObject.GetKind())).To(BeTrue())
@@ -116,7 +116,7 @@ func TestPatch(t *testing.T) {
 		// Compute request identifier, so we can later verify that the update call was cached.
 		modifiedUnstructured, err = prepareModified(env.Scheme(), modifiedObject)
 		g.Expect(err).ToNot(HaveOccurred())
-		requestIdentifierNoOp, err := ComputeRequestIdentifier(env.GetScheme(), originalObject, modifiedUnstructured)
+		requestIdentifierNoOp, err := ComputeRequestIdentifier(env.GetScheme(), originalObject.GetResourceVersion(), modifiedUnstructured)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Update the object
 		applyCallCount = 0
@@ -189,7 +189,7 @@ func TestPatch(t *testing.T) {
 		// Compute request identifier before the update, so we can later verify that the update call was not cached with this identifier.
 		modifiedUnstructured, err := prepareModified(env.Scheme(), modifiedObject)
 		g.Expect(err).ToNot(HaveOccurred())
-		oldRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), originalObject, modifiedUnstructured)
+		oldRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), originalObject.GetResourceVersion(), modifiedUnstructured)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Save a copy of modifiedUnstructured before apply to compute the new identifier later
 		modifiedUnstructuredBeforeApply := modifiedUnstructured.DeepCopy()
@@ -204,12 +204,8 @@ func TestPatch(t *testing.T) {
 		// Get the actual object from server after apply to compute the new request identifier
 		objectAfterApply := initialObject.DeepCopy()
 		g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(objectAfterApply), objectAfterApply)).To(Succeed())
-		// Convert to unstructured WITHOUT filtering to preserve server fields (like resourceVersion)
-		objectAfterApplyUnstructured := &unstructured.Unstructured{}
-		err = env.GetScheme().Convert(objectAfterApply, objectAfterApplyUnstructured, nil)
-		g.Expect(err).ToNot(HaveOccurred())
 		// Compute the new request identifier (after apply)
-		newRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), objectAfterApplyUnstructured, modifiedUnstructuredBeforeApply)
+		newRequestIdentifier, err := ComputeRequestIdentifier(env.GetScheme(), objectAfterApply.GetResourceVersion(), modifiedUnstructuredBeforeApply)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Verify that request was cached with the new identifier (after apply)
 		g.Expect(ssaCache.Has(newRequestIdentifier, initialObject.GetObjectKind().GroupVersionKind().Kind)).To(BeTrue())
@@ -232,7 +228,7 @@ func TestPatch(t *testing.T) {
 		// Compute request identifier, so we can later verify that the update call was cached.
 		modifiedUnstructured, err = prepareModified(env.Scheme(), modifiedObject)
 		g.Expect(err).ToNot(HaveOccurred())
-		requestIdentifierNoOp, err := ComputeRequestIdentifier(env.GetScheme(), originalObject, modifiedUnstructured)
+		requestIdentifierNoOp, err := ComputeRequestIdentifier(env.GetScheme(), originalObject.GetResourceVersion(), modifiedUnstructured)
 		g.Expect(err).ToNot(HaveOccurred())
 		// Update the object
 		applyCallCount = 0