K8SPSMDB-1595: set container security context for pbm-init container#2283
K8SPSMDB-1595: set container security context for pbm-init container#2283shajia-deshaw wants to merge 6 commits intopercona:mainfrom
pbm-init container#2283Conversation
shajia-deshaw
requested review from
egegunes,
gkech,
hors,
mayankshah1607,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
|
|
There was a problem hiding this comment.
Pull request overview
Fixes pod admission failures under Kubernetes Pod Security Admission (PSA) restricted by ensuring the pbm-init init container created during physical/PITR restore receives the configured container SecurityContext.
Changes:
- Set
pbm-init.SecurityContextfromcluster.Spec.InitContainerSecurityContextduring physical restore. - Add a unit test asserting
pbm-initgets the expectedSecurityContext.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controller/perconaservermongodbrestore/physical.go | Applies InitContainerSecurityContext to the pbm-init init container during updateStatefulSetForPhysicalRestore(). |
| pkg/controller/perconaservermongodbrestore/physical_test.go | Adds a unit test validating the pbm-init container’s SecurityContext is set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses pod admission failures under Kubernetes Pod Security Admission (PSA) restricted by ensuring the pbm-init init container added during physical/PITR restore receives the configured container SecurityContext, matching existing init-container behavior elsewhere in the operator.
Changes:
- Set
pbm-initcontainerSecurityContextfromcluster.Spec.InitContainerSecurityContextduring physical restore (version-gated). - Add a unit test asserting
pbm-initreceives the configuredSecurityContext.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/controller/perconaservermongodbrestore/physical.go | Apply init container security context to pbm-init during physical restore. |
| pkg/controller/perconaservermongodbrestore/physical_test.go | Add unit test coverage for pbm-init security context propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
@egegunes - anything I can do from my side to move this forward? |
|
Hi @shajia-deshaw , sorry, we are at KubeCon and partially available. We will review your PR tomorrow. Thanks for the contribution. |
Thanks for the quick reply and no worries! |
| } | ||
|
|
||
| func TestUpdateStatefulSetForPhysicalRestoreSecurityContext(t *testing.T) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Since go 1.24, we can use t.Context() instead of context.Background()
| assert.Equal(t, expectedURI, lastEnvVar.Value) | ||
| } | ||
|
|
||
| func TestUpdateStatefulSetForPhysicalRestoreSecurityContext(t *testing.T) { |
There was a problem hiding this comment.
Instead of having a whole new test for only asserting the security context, mostly the same as the existing, more generic test, why don't we incorporate the SC assertion in the existing?
There was a problem hiding this comment.
I have removed the new test that I added and added the additional CSP assertions into the existing one so there's minimal changes now.
There was a problem hiding this comment.
Pull request overview
This PR addresses Pod Security Admission (PSA) failures during physical/PITR restores by ensuring the pbm-init init container receives a SecurityContext, aligning it with how other init containers are configured elsewhere in the operator.
Changes:
- Apply
cluster.Spec.InitContainerSecurityContextto thepbm-initinit container during physical restore StatefulSet mutation (with a version gate). - Add a unit test asserting
pbm-initreceives the configuredSecurityContext.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controller/perconaservermongodbrestore/physical.go | Sets pbm-init.SecurityContext from cluster.Spec.InitContainerSecurityContext during physical restore StatefulSet updates. |
| pkg/controller/perconaservermongodbrestore/physical_test.go | Adds a unit test validating pbm-init inherits the init container security context from the CR spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cluster.Spec.ImagePullPolicy, | ||
| cmd, | ||
| ) | ||
| if cluster.CompareVersion("1.23.0") >= 0 && cluster.Spec.InitContainerSecurityContext != nil { |
There was a problem hiding this comment.
The version gate for applying InitContainerSecurityContext to pbm-init is >= 1.23.0, but the same field is already applied to other init containers for >= 1.14.0 (see pkg/psmdb/init.go where init.SecurityContext = cr.Spec.InitContainerSecurityContext is guarded by CompareVersion("1.14.0")). With the current 1.23.0 threshold, clusters running physical restore with spec.crVersion between 1.14.0 and 1.22.x can still miss the init container security context and continue to fail PSA restricted admission. Consider lowering/removing this version guard to match the existing init-container behavior so the setting is applied consistently across supported CR versions.
| if cluster.CompareVersion("1.23.0") >= 0 && cluster.Spec.InitContainerSecurityContext != nil { | |
| if cluster.CompareVersion("1.14.0") >= 0 && cluster.Spec.InitContainerSecurityContext != nil { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: "1_22_with_InitContainerSecurityContext_ignored", | ||
| crVersion: "1.22.0", | ||
| clusterInitSC: initSC, | ||
| wantPbmInitSC: nil, | ||
| }, | ||
| { | ||
| name: "1_22_without_InitContainerSecurityContext", | ||
| crVersion: "1.22.0", | ||
| clusterInitSC: nil, | ||
| wantPbmInitSC: nil, |
There was a problem hiding this comment.
These test cases encode that InitContainerSecurityContext should be ignored for CRVersion 1.22.0. That expectation looks inconsistent with existing behavior where init containers already apply spec.initContainerSecurityContext for versions >= 1.14.0 (see pkg/psmdb/init.go). If the restore path should mirror that behavior (and avoid PSA restricted failures for 1.14–1.22 clusters), update the test matrix/expected values accordingly.
There was a problem hiding this comment.
No, because:
updateStatefulSetForPhysicalRestore() calls EntrypointInitContainer() to create the pbm-init container but never assigns SecurityContext after creation - unlike the other caller, InitContainers() in init.go, which does apply it.
currently but we're going to be enabling that from v1.23.0
commit: e32dd8b |
CHANGE DESCRIPTION
Problem:
The
pbm-initinit container added during physical/PITR restore is missing aSecurityContext, causing pod admission failures in namespaces which has PSA inrestrictedmode. More details in #2250Cause:
updateStatefulSetForPhysicalRestore() calls EntrypointInitContainer() to create the pbm-init container but never assigns SecurityContext after creation - unlike the other caller, InitContainers() in init.go, which does apply it.
Solution:
Assign
cluster.Spec.InitContainerSecurityContextto thepbm-initcontainer after creation (for CR version ≥1.23.0- idk if the version guard that I have here is the right version but per the discussion in the issue, the idea was to allow this for >= 1.23.0 versions? Please feel free to correct me here.), mirroring the existing pattern.CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability