fix(backup): apply configured podSecurityContext to backup CronJob#1655
Conversation
The backup CronJob controller does not set podSecurityContext on the backup Job pod spec, even though DevWorkspaceOperatorConfig documents that podSecurityContext applies to all workspace-related pods. This can cause permission or SELinux failures when the backup container reads workspace PVC data on clusters with a custom podSecurityContext. Apply the configured podSecurityContext from DevWorkspaceOperatorConfig to the backup Job pod template, consistent with workspace deployments and PVC cleanup jobs. Fixes: devfile#1636 Assisted-by: Claude Code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Chris Brown <chribrow@redhat.com>
snecklifter
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe backup CronJob’s generated Job pod template now applies ChangesBackup CronJob security context
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Hi @snecklifter. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/che-ai-assistant ok-pr-readiness Review is complete. Please check the review comments below. |
PR Readiness Assessment: PR #1655 — fix(backup): apply configured podSecurityContext to backup CronJobRepository: devfile/devworkspace-operator
Overall: NEEDS WORK Missing Information
What's Good
Generated by Claude Code using ok-prerelease-verification:ok-pr-readiness |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/backupcronjob/backupcronjob_controller_test.go (1)
429-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
By(...)steps to the new Ginkgo specs.These tests add multiple setup/act/assert phases but don't document them with
By(...), which is the repo convention for Ginkgo tests. As per coding guidelines,**/*_test.go: UseBy("...")to document test steps in Ginkgo tests.Also applies to: 469-501
🤖 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 `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 429 - 467, Add By(...) steps to the new Ginkgo spec in backupcronjob_controller_test.go so the setup, execution, and assertions are documented in the repo’s standard style. Update the affected It(...) blocks around executeBackupSync and the job security context checks to use clear By("...") markers for each phase, matching the existing Ginkgo conventions in this test file.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 429-467: Add By(...) steps to the new Ginkgo spec in
backupcronjob_controller_test.go so the setup, execution, and assertions are
documented in the repo’s standard style. Update the affected It(...) blocks
around executeBackupSync and the job security context checks to use clear
By("...") markers for each phase, matching the existing Ginkgo conventions in
this test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1457482d-ebab-4591-92d1-594938b4bca9
📒 Files selected for processing (2)
controllers/backupcronjob/backupcronjob_controller.gocontrollers/backupcronjob/backupcronjob_controller_test.go
|
Thank you for the PR @snecklifter , could you please fix the merge conflict? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17, rohanKanojia, snecklifter, tolusha The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Both branches added independent tests at the same location in backupcronjob_controller_test.go: upstream added imagePullPolicy tests, this branch added podSecurityContext tests. Keep all four tests. Assisted-by: Claude Code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
@dkwon17 done! |
|
Thank you! |
4 participants
Summary
podSecurityContextfromDevWorkspaceOperatorConfigto backup Job pod templatepodSecurityContextis not configured (nil), behavior is unchanged from currentFixes #1636
Verification
Configure
podSecurityContextin the DevWorkspaceOperatorConfig:kubectl patch devworkspaceoperatorconfig devworkspace-operator-config \ -n devworkspace-controller --type merge \ -p '{"config":{"workspace":{"podSecurityContext":{"fsGroupChangePolicy":"OnRootMismatch"}}}}'After a backup Job is created, confirm the security context is applied:
kubectl get job -l controller.devfile.io/devworkspace-backup=true \ -o jsonpath='{.items[0].spec.template.spec.securityContext}'Expected output should include the configured
fsGroupChangePolicy.Test plan
podSecurityContext(customFSGroupChangePolicyandSELinuxOptions)podSecurityContextis nil when not configuredgo test ./controllers/backupcronjob/...)go vetcleanAssisted-by: Claude Code
Summary by CodeRabbit