OCPBUGS-29729: Updates default security context behavior for catalog source pods #3206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change updates the logic for setting security contexts within the OLM pod reconciler. Now, it differentiates between 'Restricted' and 'Legacy' security contexts more explicitly. The 'Restricted' security context applies default security settings unless overridden, while the 'Legacy' context clears all security settings. When no security context is configured, it defaults to restricted. Additionally, the related tests have been updated to reflect these changes and ensure correct behavior. Signed-off-by: btofel <[email protected]>
Signed-off-by: btofel <[email protected]>
Signed-off-by: btofel <[email protected]> Signed-off-by: Brett Tofel <[email protected]>
perdasilva
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
perdasilva
force-pushed
the
OCPBUGS-29729-perdasilva
branch
7 times, most recently
from
50952ce to
9dc398d
Compare
perdasilva
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
11 tasks
joelanford
reviewed
Apr 24, 2024
joelanford
reviewed
Apr 24, 2024
perdasilva
force-pushed
the
OCPBUGS-29729-perdasilva
branch
8 times, most recently
from
2cadc5e to
73c129a
Compare
bentito
changed the title
m1kola
previously approved these changes
Apr 29, 2024
joelanford
reviewed
Apr 29, 2024
Comment on lines
43
to
47
| func WithSecurityContextConfig(securityContextConfig v1alpha1.SecurityConfig) PodOptionFunc { | ||
| return func(option *PodOption) { | ||
| option.SecurityContextConfig = securityContextConfig | ||
| } | ||
| } |
There was a problem hiding this comment.
This naming is still something that I think could lead to confusion. This is the default security context config, right?
This name implies that whatever value is here is what will be used. But it isn't until we evaluate the catalog source that we decide what the actual value to use is.
I'm also having trouble understanding why in some cases we use the options ...PodOptionFunc parameter and in others we use the podSecurityConfig v1alpha1.SecurityConfig parameter.
In this case, my suggestions would be:
- drop the option func (my reasoning is: calling a function that takes this variadic parameter means that passing that parameter is optional, which means we need a default for the default, which just complicates things even more).
- use
defaultPodSecurityConfigas the name of the variables and parameters everywhere, except where we finally decide thesecurityContextwe're going to put into the pod.
There was a problem hiding this comment.
see if that's better. I just really wanted to make as few changes as possible XD but you make good points.
|
This PR is a positive step towards better handling of security contexts in the OLM pod reconciler, making it easier to manage and test security configurations comprehensively. LGTM |
perdasilva
force-pushed
the
OCPBUGS-29729-perdasilva
branch
from
73c129a to
87232d0
Compare
joelanford
previously approved these changes
Apr 29, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
perdasilva
force-pushed
the
OCPBUGS-29729-perdasilva
branch
4 times, most recently
from
ee56764 to
66856c2
Compare
Signed-off-by: Per Goncalves da Silva <[email protected]>
perdasilva
force-pushed
the
OCPBUGS-29729-perdasilva
branch
from
66856c2 to
27f9246
Compare
|
I think this could merge. @joelanford did @perdasilva address the naming problem sufficiently? |
Merged
via the queue into
operator-framework:master
with commit May 2, 2024
9b28021
14 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
Before: by default CatalogSource would stamp out pods with the "legacy" security profile
Now: by default CatalogSource stamps out "restricted" security profile pods when in appropriately labeled namespaces and "legacy" otherwise
Motivation for the change:
Helps users be secure-by-default on PSA clusters when deploying in "restricted" namespaces (no change for older clusters)
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue