OCPBUGS-36246: Improve resource merge diffs

[2uasimojo]

Resource merges -- where CVO asserts the shape of "owned" resources based on manifests from the release payload -- were trying to display useful information when an update was triggered. However:

• In some places, we were comparing objects after syncing, so the output would never show the discrepancy that actually triggered the update.
• We were using cmp.Diff() without any filters, so the diff would always show mismatches in Metadata like UID, CreationTimestamp, etc.

Here we fix the instances of the former; and address the latter via a helper that filters (ignores):

• All TypeMeta, since by the time we get to this code, we're already sure the GVKs are correct (but sometimes the values from the manifest side can be blank?).
• Status
• ObjectMeta fields not explicitly synced by EnsureObjectMeta()

Summary by CodeRabbit

• Refactor

  • Standardized reconciliation “diff” computation across multiple Kubernetes resource types using a shared ManifestDiff helper, with filtering to ignore non-impactful fields.
  • Updated reconciling logs to use consistent, filtered diff output, including capturing snapshots of pre-reconcile resources for accurate comparisons.

• Bug Fixes

  • Reduces misleading “empty diff / hotloop” messaging by basing diff reporting (and related update-log decisions) on the filtered manifest differences, while preserving existing update behavior.

[openshift-ci-robot]

@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Resource merges -- where CVO asserts the shape of "owned" resources based on manifests from the release payload -- were trying to display useful information when an update was triggered. However:

• In some places, we were comparing objects after syncing, so the output would never show the discrepancy that actually triggered the update.
• We were using cmp.Diff() without any filters, so the diff would always show mismatches in Metadata like UID, CreationTimestamp, etc.

Here we fix the instances of the former; and address the latter via a helper that filters (ignores):

• All TypeMeta, since by the time we get to this code, we're already sure the GVKs are correct (but sometimes the values from the manifest side can be blank?).
• Status
• ObjectMeta fields not explicitly synced by EnsureObjectMeta()

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 7a6928a6-e918-4ca3-8246-465065e8a5e5

📥 Commits

Reviewing files that changed from the base of the PR and between 7334e8b and 1382cc5.

📒 Files selected for processing (13)

• lib/resourceapply/admissionregistration.go
• lib/resourceapply/apiext.go
• lib/resourceapply/apps.go
• lib/resourceapply/batch.go
• lib/resourceapply/core.go
• lib/resourceapply/cv.go
• lib/resourceapply/imagestream.go
• lib/resourceapply/interface.go
• lib/resourceapply/operators.go
• lib/resourceapply/rbac.go
• lib/resourceapply/security.go
• pkg/cvo/internal/generic.go
• pkg/cvo/internal/operatorstatus.go

✅ Files skipped from review due to trivial changes (4)

• lib/resourceapply/admissionregistration.go
• lib/resourceapply/cv.go
• pkg/cvo/internal/generic.go
• lib/resourceapply/batch.go

🚧 Files skipped from review as they are similar to previous changes (9)

• lib/resourceapply/security.go
• lib/resourceapply/operators.go
• lib/resourceapply/imagestream.go
• lib/resourceapply/apiext.go
• lib/resourceapply/interface.go
• lib/resourceapply/core.go
• lib/resourceapply/rbac.go
• lib/resourceapply/apps.go
• pkg/cvo/internal/operatorstatus.go

---

Walkthrough

A new exported ManifestDiff function is added to lib/resourceapply/interface.go. It wraps cmp.Diff with filters that exclude TypeMeta, Status, and most ObjectMeta fields (keeping only Name, Namespace, Labels, Annotations, and OwnerReferences). All Apply* resource helpers in lib/resourceapply/ and pkg/cvo/internal/operatorstatus.go then replace their direct cmp.Diff calls with ManifestDiff, removing per-file go-cmp imports. Several helpers also add a pre-merge deep-copy (original) to ensure the diff reflects state before Ensure* mutations.

Changes

ManifestDiff adoption across resource apply functions

Layer / File(s)	Summary
ManifestDiff helper definition lib/resourceapply/interface.go	Adds go-cmp/cmp import and exports ManifestDiff(fromKAS, fromManifest any) string, which computes a filtered cmp.Diff excluding TypeMeta, Status, and non-identity ObjectMeta fields.
Adoption in admission registration and API extension resources lib/resourceapply/admissionregistration.go, lib/resourceapply/apiext.go	ValidatingWebhookConfiguration and CRD Apply functions remove go-cmp/cmp imports and switch to ManifestDiff for reconciliation diff logging in conditional log message selection.
Adoption in app and batch resources lib/resourceapply/apps.go, lib/resourceapply/batch.go	Deployment, DaemonSet, Job, and CronJob Apply functions remove go-cmp/cmp imports and switch to ManifestDiff. DaemonSet adds a pre-mutation deep-copy snapshot (original) to ensure diff reflects state before EnsureDaemonSet modifications.
Adoption in core and cluster version resources lib/resourceapply/core.go, lib/resourceapply/cv.go	Namespace, Service, ServiceAccount, ConfigMap, and ClusterVersion Apply functions remove go-cmp/cmp imports and switch to ManifestDiff. Namespace, Service, ServiceAccount, and ConfigMap add pre-mutation deep-copy snapshots to ensure diffs reflect state before Ensure* mutations.
Adoption in imagestream, RBAC, operators, and security resources lib/resourceapply/imagestream.go, lib/resourceapply/rbac.go, lib/resourceapply/operators.go, lib/resourceapply/security.go	ImageStream, RBAC (ClusterRoleBinding, ClusterRole, RoleBinding, Role), OperatorGroup, and SecurityContextConstraints Apply functions remove go-cmp/cmp imports and switch to ManifestDiff. RBAC and SecurityContextConstraints add pre-mutation deep-copy snapshots to ensure diffs reflect state before Ensure* mutations.
CVO internal package integration and refinements pkg/cvo/internal/operatorstatus.go, pkg/cvo/internal/generic.go	ClusterOperator metadata reconciliation in operatorstatus.go switches to resourceapply.ManifestDiff with corresponding import updates. Generic unstructured apply logic refines skipKeys initialization type inference and clarifies empty-diff early-return comment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	ManifestDiff() logs ConfigMap.Data and .BinaryData fields without filtering, exposing potential passwords, API keys, tokens, and credentials in debug logs.	Extend ManifestDiff's filter to ignore .Data, .BinaryData, .Env fields in Containers, and other sensitive configuration fields.
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: improving resource merge diffs by replacing unfiltered comparisons with a filtered diff helper function across multiple resource types.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	No Ginkgo tests with dynamic information found. PR only adds test/cvo/cvo_suite_test.go as a Ginkgo suite bootstrapper; all other new tests use Go's table-driven testing pattern without dynamic names.
Test Structure And Quality	✅ Passed	This PR contains no Ginkgo tests. All added test files use standard Go testing (func Test*), making the Ginkgo test quality check not applicable.
Microshift Test Compatibility	✅ Passed	PR contains no new Ginkgo e2e tests. All changes are to production library code in lib/resourceapply/ and pkg/cvo/internal/ that refactors diff computation; check not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests are added in this PR; all changes are to implementation files in lib/resourceapply/ and pkg/cvo/internal/. The check for SNO-incompatible tests is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	This PR only refactors diff logging utility code and does not introduce any deployment manifests, operator code, or scheduling constraints. It is not applicable to topology-aware scheduling checks.
Ote Binary Stdout Contract	✅ Passed	No stdout writes in process-level code; all logging via klog.V() configured to stderr via alsologtostderr=true; ManifestDiff() is pure computation function with no side effects.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added in this PR. Only a standard Go unit test file (security_test.go) and refactoring of existing resourceapply code were introduced, so the IPv6/disconnected network...
No-Weak-Crypto	✅ Passed	PR contains no weak cryptography patterns (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or non-constant-time secret comparisons. Changes are purely about improving reso...
Container-Privileges	✅ Passed	This PR contains only Go source code refactoring for resource diff reporting. No container privilege escalation configurations (privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, allowPrivilegeE...

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[2uasimojo]

/jira refresh

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 2uasimojo
Once this PR has been reviewed and has the lgtm label, please assign pratikmahajan for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @dis016

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @dis016

Details

In response to this:

Resource merges -- where CVO asserts the shape of "owned" resources based on manifests from the release payload -- were trying to display useful information when an update was triggered. However:

• In some places, we were comparing objects after syncing, so the output would never show the discrepancy that actually triggered the update.
• We were using cmp.Diff() without any filters, so the diff would always show mismatches in Metadata like UID, CreationTimestamp, etc.

Here we fix the instances of the former; and address the latter via a helper that filters (ignores):

• All TypeMeta, since by the time we get to this code, we're already sure the GVKs are correct (but sometimes the values from the manifest side can be blank?).
• Status
• ObjectMeta fields not explicitly synced by EnsureObjectMeta()

Summary by CodeRabbit

• Refactor
• Enhanced resource update detection during reconciliation by refining which object fields are compared for changes, filtering out status and irrelevant metadata while preserving comparisons of names, namespaces, labels, annotations, and owner references across multiple resource types.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/resourceapply/interface.go`:
- Around line 33-36: The allow-list filter in the switch statement (checking
parts[1] against "Name", "Namespace", "Labels", "Annotations",
"OwnerReferences") fails when paths contain bracket notation or array indices
like `Labels["key"]` or `OwnerReferences[0].Name`. Extract the base field name
from parts[1] by removing any content after the first bracket character (such as
"[") before performing the switch case comparison, so that paths with bracket
notation or array indices still match the allow-listed field names and are not
incorrectly filtered out.

In `@lib/resourceapply/security.go`:
- Around line 39-41: The ManifestDiff comparison on line 40 is comparing
`original` vs `existing`, but the actual Update call on line 47 is updating with
`reconcile`. This mismatch can hide real changes in logs. Change the
ManifestDiff call to compare `&required` vs `existing` or the appropriate
objects that reflect what is actually being updated with the reconcile object,
ensuring the diff accurately shows what changes will be applied.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c20f1549-1bc8-44a1-ae3f-7b75c3047348

📥 Commits

Reviewing files that changed from the base of the PR and between 810bfc1 and c916190.

📒 Files selected for processing (13)

• lib/resourceapply/admissionregistration.go
• lib/resourceapply/apiext.go
• lib/resourceapply/apps.go
• lib/resourceapply/batch.go
• lib/resourceapply/core.go
• lib/resourceapply/cv.go
• lib/resourceapply/imagestream.go
• lib/resourceapply/interface.go
• lib/resourceapply/operators.go
• lib/resourceapply/rbac.go
• lib/resourceapply/security.go
• pkg/cvo/internal/generic.go
• pkg/cvo/internal/operatorstatus.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/resourceapply/interface.go`:
- Line 35: In the case statement in the interface.go file, correct the two field
name typos to match the actual metav1.ObjectMeta field names. Change
"GeneratedName" to "GenerateName" and change "DeletionGracePeriodSecords" to
"DeletionGracePeriodSeconds". These typos prevent the filter from matching the
actual Kubernetes ObjectMeta fields during comparison, which causes
false-positive diffs to appear in the ManifestDiff function output.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: fbd71455-bb14-4add-a3b1-3e0c73964236

📥 Commits

Reviewing files that changed from the base of the PR and between 4dbfdec and f2d8447.

📒 Files selected for processing (13)

• lib/resourceapply/admissionregistration.go
• lib/resourceapply/apiext.go
• lib/resourceapply/apps.go
• lib/resourceapply/batch.go
• lib/resourceapply/core.go
• lib/resourceapply/cv.go
• lib/resourceapply/imagestream.go
• lib/resourceapply/interface.go
• lib/resourceapply/operators.go
• lib/resourceapply/rbac.go
• lib/resourceapply/security.go
• pkg/cvo/internal/generic.go
• pkg/cvo/internal/operatorstatus.go

✅ Files skipped from review due to trivial changes (3)

• pkg/cvo/internal/generic.go
• lib/resourceapply/cv.go
• lib/resourceapply/core.go

🚧 Files skipped from review as they are similar to previous changes (9)

• lib/resourceapply/security.go
• lib/resourceapply/apiext.go
• pkg/cvo/internal/operatorstatus.go
• lib/resourceapply/admissionregistration.go
• lib/resourceapply/operators.go
• lib/resourceapply/batch.go
• lib/resourceapply/apps.go
• lib/resourceapply/rbac.go
• lib/resourceapply/imagestream.go