-
Notifications
You must be signed in to change notification settings - Fork 560
⚠️ Enforce PSA Restricted Level for CatalogSources (Drop Support for Catalogs built with opm < v1.21.0) #3526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
d4a546c
to
0907b88
Compare
Hi @ @kevinrizza @perdasilva The follow up requested :-) |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we confirm that we're okay with moving to restricted
?
I'm concerned we'll take these changes downstream, and then hit the same issue we were facing 2 years ago and be forced to revert (and potentially back port)
Looks like we might be facing an issue upstream too:
Could be a flake too. |
I updated this test: #3526 (comment) probably we need to remove or I did something wrong. But I agree we need to check this one properly.. we cannot only update. Thank you for call it out :-) |
Right, it looks like the test is possibly actually telling us we're not ready to move to |
0907b88
to
fc3aaa5
Compare
Hi @anik120 See the error:
This failure is only in a test that checks if OLM works with CatalogSources built using older opm versions. If we decide to move forward with this change motivated by the comment this comment , we should remove that test. The key question is: As mentioned in this comment by @kevinrizza, we may now be ready to do that. ✅ Yes — remove the test and move on. Why this matters:
Would love to hear from folks who can help make the call. |
I think that's fine with me. It makes sense to move (and to only support) the more secure opm. It shouldn't be easily enough to move catalogs to a newer version of opm if this breaks some old installations somewhere, and to the benefit of the user... imo, I'm good with this. |
fc3aaa5
to
45bcf4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, ack. Thanks for the context @camilamacedo86
/lgtm
…alogs Built with opm < v1.21.0) - enforce PSA restricted level - remove tests for Catalogs built with opm < v1.21.0 to ensure compatibility with those that needs baseline permissions
45bcf4c
to
153d05d
Compare
New changes are detected. LGTM label has been removed. |
This definitely needs an RFC since it is a breaking change. We need to carefully think through all of the implications. And we would ideally make a data driven decision about this. For example, Red Hat OCP collects telemetry. Can we tell how many CatalogSources are in baseline namespaces or how many explicitly set Also this PR doesn't seem to actually do anything different with CatalogSources that use spec.grpcPodConfig.securityContextConfig: legacy`, so it doesn't feel like we're actually dropping support for those older catalogs. It's hard to tell what the intention is here based on a comparison of the PR changes and the PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my above comment. I think we need a bit more due diligence and discussion to decide to make a breaking change.
Hi @joelanford Thank you for your input. Following the answers inline.
All Catalogues from OCP 4.11/4/12 should no longer be built using the OPM version < v1.21.0.
This PR only changes the permissions; instead of enforcing the baseline, we are enforcing restricted.
Given that, if you think that an RFC would be required, here is fine. I am addressing this need in JIRA and asking for an RFC. |
In general my perspective on OLMv0 is that we change nothing, except:
IMO, this change does not fall into that category. And I'm totally happy to discuss the merits of those change categories if folks disagree. |
What this PR does
This PR updates the OLM behavior to enforce the Pod Security Admission (PSA) level
restricted
instead ofbaseline
. As a result, OLM will no longer support CatalogSources built withopm
versions older thanv1.21.0
.Why this matters
restricted
PSA level aligns with modern security best practices, significantly reducing pod permissions and enhancing container isolation.opm
versions do not generate catalogs compatible with therestricted
profile.restricted
profile is already the default in OpenShift 4.11+ (which aligns with our vendor versions), and support for older setups is increasingly unnecessary.Impact
opm
versions< v1.21.0
.Motivation
For the last two years, we've defaulted to baseline enforcement. At this point, I expect everyone to use catalog binaries that can handle restricted enforcement.
Motivated by: #3524 (comment)