Remove SCC-injected security contexts for cross-cluster migration#24
Merged
Merged
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExports ChangesSecurity Context Stripping and Type Exports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift.go`:
- Around line 428-430: The allow-list that skips security-context stripping is
missing "DeploymentConfig", so update the conditional that checks kind (the if
in openshift.go comparing kind to "Pod","Deployment", etc.) to also include
"DeploymentConfig" so its spec.template.spec pod template has SCC
UID/GID/SELinux cleared; additionally, apply the corresponding patch from the
existing DeploymentConfig branch in cmd.go so PVC rename logic for
DeploymentConfig is preserved (ensure the cmd.go changes that handle PVC
renaming for DeploymentConfig are appended/merged into the current cmd.go
implementation).
- Around line 437-485: The current code deletes entire securityContext objects
(via unstructured.RemoveNestedField and delete(container, "securityContext")),
which loses user-authored settings; instead, modify logic in RemoveNestedField
calls and in stripContainerSecurityContext to only remove cluster-injected
fields: runAsUser, fsGroup, supplementalGroups, seLinuxOptions (and any
cluster-specific UID/GID fields), leaving other keys (privileged, capabilities,
allowPrivilegeEscalation, readOnlyRootFilesystem, seccompProfile) intact;
implement this by reading the securityContext map (use unstructured.NestedMap or
container["securityContext"]), deleting those specific keys, writing the map
back (and only removing the whole securityContext if the map becomes empty), and
optionally gate the existing destructive behavior behind an explicit option/flag
if full removal is desired; update references: modified.Object,
unstructured.RemoveNestedField usage, and the stripContainerSecurityContext
closure to perform targeted-key removal rather than delete(container,
"securityContext").
- Line 498: The call to jsonpatch.CreatePatch does not exist in the
evanphx/json-patch API; replace it by using
jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) if an RFC7396 merge patch
is acceptable (rename patch variable to mergePatch and use jsonpatch.MergePatch
APIs accordingly), or else implement RFC6902 operations yourself by constructing
the JSON Patch operation array (add/replace/remove objects) from originalJSON
and modifiedJSON and use that array as the patch payload instead of calling
CreatePatch; update references to the variable created from CreatePatch and any
apply/HTTP patch code to match the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
|
So there can be some security context values set by the user intentionally. Here we would only like to remove the values that are injected by SCC, basically these 3:
Other values we can always keep. |
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
strip-security-context
branch
from
189c2e7 to
e59928b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
openshift/plugin_test.go (1)
939-957: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ephemeralContainersverification to match runtime behavior.
StripSecurityContexthandlesephemeralContainers, but this test only validatescontainersandinitContainers. Add at least one case and assertion loop forephemeralContainersto close the coverage gap.🤖 Prompt for 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. In `@openshift/plugin_test.go` around lines 939 - 957, The test currently verifies security contexts for containers and initContainers by calling verifySecurityContext in similar loops, but does not include verification for ephemeralContainers even though StripSecurityContext handles them. Add a new verification block for ephemeralContainers following the same pattern as the existing containers and initContainers loops, where you check if ephemeralContainers exists in the spec, iterate through each ephemeral container, extract its securityContext, and call verifySecurityContext with an appropriately formatted path string like "spec.ephemeralContainers[%d].securityContext".
🤖 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 `@openshift/openshift.go`:
- Around line 512-526: The code currently strips runAsUser when it is >=
1000000000, but it does not handle fsGroup with the same threshold. Add a
similar check after the runAsUser deletion logic to also strip fsGroup from the
security context when its value is >= 1000000000. Cast fsGroup to int64, verify
it exists and meets the threshold condition, and delete it from the security
context map (sc) using the same pattern as the runAsUser check. Apply this same
fix to any other locations mentioned where similar security context stripping
occurs.
- Around line 482-487: The calls to unstructured.SetNestedMap and
unstructured.RemoveNestedField are ignoring their error return values, which
golangci-lint is flagging. Update the closure containing the security context
removal logic to return an error type, then check the error returned by both the
unstructured.RemoveNestedField call (when removing empty securityContext) and
the unstructured.SetNestedMap call (when setting the modified securityContext),
propagating any errors back through the closure's return. Apply the same error
handling pattern to the unstructured.SetNestedSlice calls also mentioned in the
StripSecurityContext function to ensure all nested-set operations properly
propagate errors.
- Around line 615-616: The variable ok1 is being assigned on line 615 during the
type assertion for origSC but is never used in the code that follows, only ok2
is read. Remove the unused ok1 assignment by replacing it with the blank
identifier underscore on line 615 where origSC is being assigned from the
securityContext type assertion to eliminate the ineffectual-assignment lint
error.
In `@openshift/plugin_test.go`:
- Around line 917-933: The test code currently allows nil spec values to pass
silently without validation, which can hide patch-related failures. After the
multi-level map resolution that sets the spec variable (based on the resource
kind check using tt.resource.GetKind()), add a validation check that calls
t.Fatalf with descriptive error details when spec is nil and tt.expectPatch is
true, including the resolved kind and the expected path structure (e.g., "spec",
"spec.jobTemplate.spec.template.spec", etc.) so that accidental removal or
movement of the pod spec is properly caught instead of producing a
false-positive pass.
- Around line 586-593: The TestStripSecurityContext test struct defines an
expectedSecurityCtx field to specify which security context fields should be
preserved after patching, but the test implementation does not verify these
preserved fields are actually present in the output. Locate the test loop where
each test case is executed and add positive assertions that check the
expectedSecurityCtx values are preserved in the patched resource. This will
ensure regressions that remove fields like seLinuxOptions.type/user/role,
readOnlyRootFilesystem, or capabilities would be caught by the tests, not just
verify that SCC fields are removed.
---
Nitpick comments:
In `@openshift/plugin_test.go`:
- Around line 939-957: The test currently verifies security contexts for
containers and initContainers by calling verifySecurityContext in similar loops,
but does not include verification for ephemeralContainers even though
StripSecurityContext handles them. Add a new verification block for
ephemeralContainers following the same pattern as the existing containers and
initContainers loops, where you check if ephemeralContainers exists in the spec,
iterate through each ephemeral container, extract its securityContext, and call
verifySecurityContext with an appropriately formatted path string like
"spec.ephemeralContainers[%d].securityContext".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c679f71e-5ce0-48f8-9195-b4749ff7dd39
📒 Files selected for processing (3)
openshift/openshift.goopenshift/plugin.goopenshift/plugin_test.go
nachandr
force-pushed
the
strip-security-context
branch
from
731af1a to
3660508
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift/plugin_test.go (1)
890-903: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecouple this test from
UpdateDefaultPullSecretsside effects.This suite is validating security-context stripping, but Pod
Runalso executes pull-secret mutation. Setting extras to disable pull-secret stripping in this test will keepexpectPatchassertions focused and future-proof.Suggested tweak
request := transform.PluginRequest{ Unstructured: *tt.resource, - Extras: map[string]string{}, + Extras: map[string]string{ + StripDefaultPullSecretsFlag: "false", + }, }🤖 Prompt for 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. In `@openshift/plugin_test.go` around lines 890 - 903, The test's PluginRequest currently has an empty Extras map, which means the Pod Run method executes both security-context stripping and pull-secret mutation, causing the expectPatch assertions to be affected by unwanted side effects. Populate the Extras field in the PluginRequest initialization (where it currently shows Extras: map[string]string{}) with the appropriate configuration values needed to disable pull-secret stripping, so the test assertions focus exclusively on validating security-context stripping behavior.
🤖 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 `@openshift/plugin_test.go`:
- Around line 1020-1108: The checkPreserved helper function has a logic bug for
seLinuxOptions validation where it attempts to cast modVal (which is nil when
the field was removed) to a map, causing the check to silently pass instead of
detecting removed fields. Additionally, in the container, initContainer, and
ephemeralContainer security context checks (starting around lines 1058, 1070,
and 1083), if modSC is missing entirely, the validation is skipped instead of
treating it as an empty map to properly catch removed fields. Fix this by
ensuring that when modVal is nil in the seLinuxOptions section, the modOpts is
initialized to an empty map, and apply the same fallback pattern to all
container type checks so that missing modSC is treated as an empty map rather
than skipping validation entirely.
---
Nitpick comments:
In `@openshift/plugin_test.go`:
- Around line 890-903: The test's PluginRequest currently has an empty Extras
map, which means the Pod Run method executes both security-context stripping and
pull-secret mutation, causing the expectPatch assertions to be affected by
unwanted side effects. Populate the Extras field in the PluginRequest
initialization (where it currently shows Extras: map[string]string{}) with the
appropriate configuration values needed to disable pull-secret stripping, so the
test assertions focus exclusively on validating security-context stripping
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3789875c-ae95-4245-a66c-e5e7155b1300
📒 Files selected for processing (3)
openshift/openshift.goopenshift/plugin.goopenshift/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openshift/plugin.go
nachandr
force-pushed
the
strip-security-context
branch
from
3660508 to
24c0152
Compare
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
strip-security-context
branch
from
24c0152 to
171a9d7
Compare
Contributor
Author
|
/coderabbit review |
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
There was a problem hiding this comment.
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 `@openshift/openshift.go`:
- Around line 482-490: The code currently only handles int64 type assertions for
runAsUser and fsGroup, but JSON unmarshaling of unstructured.Unstructured
produces float64 for numeric values, causing the type checks to fail silently
and leave SCC-injected UIDs in place. Create a helper function called sccUID
that accepts an interface and safely handles both int64 and float64 types by
returning the value as int64 and a boolean indicating success. Then replace all
occurrences of the manual type assertions for runAsUser and fsGroup (both in the
current block and in the container-level block mentioned at lines 539 and 544)
with calls to the sccUID helper function, using the returned int64 value to
compare against SCCNamespaceUIDMin before deleting the field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef027c36-af67-40de-a55a-ea4f9d8cd26a
📒 Files selected for processing (3)
openshift/openshift.goopenshift/plugin.goopenshift/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openshift/plugin.go
- openshift/plugin_test.go
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Contributor
Author
|
TestStripSecurityContext results: 
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes [#22 )
The changes add a stripSecurityContext() function that removes pod-level and container-level security contexts from workload resources (Pods, Deployments, StatefulSets, etc.) to prevent SCC validation failures when migrating between OpenShift clusters with different namespace UID ranges.
The stripSecurityContext() fn only strips the following and preserves all other security context values (capabilities, readOnlyRootFilesystem, etc.)
Summary by CodeRabbit
New Features
runAsUser,fsGroup, andseLinuxOptions.levelat both pod and container levels while preserving user-configured securityContext settings.Tests