🌱 Revision manifest sanitization by dtfranz · Pull Request #2319 · operator-framework/operator-controller

dtfranz · 2025-11-11T01:37:49Z

Description

Removes metadata fields used by the kube api to avoid situations like #2295

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-11-11T01:37:55Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`4e49490`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69146175746ffc00086c4211
😎 Deploy Preview	https://deploy-preview-2319--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2025-11-11T01:49:48Z

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.27%. Comparing base (6beca87) to head (4e49490).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/operator-controller/applier/boxcutter.go	86.20%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2319 +/- ##
==========================================
+ Coverage 74.23% 74.27% +0.03% 
==========================================
Files 91 91 Lines 7057 7083 +26 ==========================================
+ Hits 5239 5261 +22 - Misses 1403 1406 +3 - Partials 415 416 +1

Flag	Coverage Δ
e2e	`45.64% <0.00%> (-0.19%)`	⬇️
experimental-e2e	`48.36% <72.41%> (+0.11%)`	⬆️
unit	`58.60% <86.20%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

perdasilva · 2025-11-11T09:36:42Z

 	return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
 }

+var restrictedMetadata = []string{


There was a suggestion by Predrag on the ticket to maybe do it as an allow list instead, i.e. just allow:
.spec, .metadata.name, .metadata.namespace, .metadata.annotations, and .metadata.labels. Wdyt?

Ooooh I didn't understand that message at first, I didn't realize he meant a "white" list 😆 But yes, I can do that and that would be simpler.

On second thought this might make it more difficult to give high-granularity warnings when we find a field to remove, but I'll think about it some more.

Ooooh I didn't understand that message at first, I didn't realize he meant a "white" list 😆 But yes, I can do that and that would be simpler.
The terms "white" and "black" lists are problematic; the preferred terms are "allow" and "block" lists.

Thanks Todd - I avoid them myself but only mentioned it here because the original suggestion used that terminology.

camilamacedo86

Thanks for working on this! I really appreciate the effort to make things cleaner.

Just a small thought — I don’t think we should get in the middle of how the Kubernetes API manages its own fields.
We should let the API do its job and handle those values as it’s designed to.
If we start stripping or modifying them ourselves, we might cause unexpected behavior, ownership issues, or race conditions later on.

🆗 It’s totally fine to clean up our testdata for readability — that’s helpful and safe.
🚫 But in the controller or reconciler, it’s best to let k8s manage those fields naturally.

Letting k8s handle its own fields keeps our controller simple, predictable, and safe. 🌿

dtfranz · 2025-11-11T12:42:23Z

Thanks for working on this! I really appreciate the effort to make things cleaner.
Just a small thought — I don’t think we should get in the middle of how the Kubernetes API manages its own fields. We should let the API do its job and handle those values as it’s designed to. If we start stripping or modifying them ourselves, we might cause unexpected behavior, ownership issues, or race conditions later on.
* 🆗 It’s totally fine to clean up our **testdata** for readability — that’s helpful and safe.
* 🚫 But in the **controller or reconciler**, it’s best to let k8s manage those fields naturally.
Letting k8s handle its own fields keeps our controller simple, predictable, and safe. 🌿

Thanks Camila! But TBH, I'm not sure what you're referring to here WRT "getting in the middle of things". What we're doing here is removing fields controlled/modified/etc. by the kube API server from the bundle manifests before we add them to the CER phases. Bundle creators should not be setting these fields. If they do, and we don't clear them, the reconciler will see that a field was modified from the original revision and may infinitely create new revisions - see here.

…applied by revisions. Assisted-by: Gemini+Claude Signed-off-by: Daniel Franz <dfranz@redhat.com>

tmshort · 2025-11-11T13:45:40Z

Thanks Camila! But TBH, I'm not sure what you're referring to here WRT "getting in the middle of things". What we're doing here is removing fields controlled/modified/etc. by the kube API server from the bundle manifests before we add them to the CER phases. Bundle creators should not be setting these fields. If they do, and we don't clear them, the reconciler will see that a field was modified from the original revision and may infinitely create new revisions - see here.

Which is something (infinitely create new revisions) I'm seeing in another bug.

So, this is likely a case where we do want to intervene in the processing of these fields.

camilamacedo86 · 2025-11-11T16:35:41Z

Hi @tmshort @dtfranz

So, I think now I understand it, the goal here is to make sure that when we create new revisions (e.g., v2, v3, v4), they’re based on the latest revision object — which makes total sense. I agree that we shouldn’t carry over all the existing fields as-is into the new revision.

However, instead of stripping those fields out, what do you think about defining a new struct (or helper) that explicitly copies only the fields that make sense for the revision? This way, if Kubernetes adds new fields in the future, our code will still behave correctly without needing manual updates.

Maybe something like a NewRevisionFrom() function that builds a new revision object and only fills in the fields we need — that might make the intent clearer and the logic more robust.

I will not block this one ;-)

dtfranz · 2025-11-12T10:24:00Z

Branch updated; as suggested, we now filter the metadata through an allow-list rather than a block-list. However, for the root-level fields, I've set the filtering to remove only status, as there are too many root-level "speccy" fields aside from just spec on various k8s resources to reasonably create a comprehensive allow-list - see k8s NetworkPolicy API for instance.

Signed-off-by: Daniel Franz <dfranz@redhat.com>

tmshort · 2025-11-12T19:18:19Z

/approve

rashmigottipati

/lgtm

openshift-ci · 2025-11-12T20:04:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rashmigottipati, tmshort

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~internal/operator-controller/OWNERS~~ [tmshort]

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

dtfranz requested a review from a team as a code owner November 11, 2025 01:37

openshift-ciBot requested review from joelanford and pedjak November 11, 2025 01:37

rashmigottipati reviewed Nov 11, 2025
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter.go

perdasilva reviewed Nov 11, 2025
View reviewed changes

camilamacedo86 previously requested changes Nov 11, 2025
View reviewed changes

openshift-ciBot assigned camilamacedo86 Nov 11, 2025

Removes read-only or otherwise ill-advised fields from the manifests …
dff77b7
…applied by revisions. Assisted-by: Gemini+Claude Signed-off-by: Daniel Franz <dfranz@redhat.com>

dtfranz force-pushed the revision-manifest-sanitization branch from e237a5d to 64dfcf7Compare November 11, 2025 12:53

dtfranz force-pushed the revision-manifest-sanitization branch from 64dfcf7 to 0dc0a28Compare November 12, 2025 09:48

Cleanup, add logging
4e49490
Signed-off-by: Daniel Franz <dfranz@redhat.com>

dtfranz force-pushed the revision-manifest-sanitization branch from 0dc0a28 to 4e49490Compare November 12, 2025 10:29

camilamacedo86 removed their assignment Nov 12, 2025

openshift-ciBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2025

rashmigottipati approved these changes Nov 12, 2025
View reviewed changes

openshift-ciBot assigned rashmigottipati Nov 12, 2025

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2025

openshift-merge-botBot merged commit c95fc24 into operator-framework:mainNov 12, 2025
25 checks passed

Uh oh!

Conversation

dtfranz commented Nov 11, 2025

Description

Reviewer Checklist

Uh oh!

netlifyBot commented Nov 11, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecovBot commented Nov 11, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

perdasilvaNov 11, 2025

Choose a reason for hiding this comment

Uh oh!

dtfranzNov 11, 2025

Choose a reason for hiding this comment

Uh oh!

dtfranzNov 11, 2025

Choose a reason for hiding this comment

Uh oh!

tmshortNov 11, 2025

Choose a reason for hiding this comment

Uh oh!

dtfranzNov 12, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtfranz commented Nov 11, 2025

Uh oh!

tmshort commented Nov 11, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 commented Nov 11, 2025

Uh oh!

dtfranz commented Nov 12, 2025

Uh oh!

tmshort commented Nov 12, 2025

Uh oh!

rashmigottipati left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ciBot commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlifyBot commented Nov 11, 2025•
edited
Loading

codecovBot commented Nov 11, 2025•
edited
Loading

camilamacedo86 left a comment •
edited
Loading

tmshort commented Nov 11, 2025•
edited
Loading