OCPBUGS-86895: Ensure packageserver pod seccompProfile is always set - #1341
OCPBUGS-86895: Ensure packageserver pod seccompProfile is always set#1341tmshort wants to merge 1 commit into
Conversation
|
@tmshort: Jira Issue OCPBUGS-86895 is in a security level that is not in the allowed security levels for this repo.
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe package server manager now enforces a pod-level ChangesSeccomp profile enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/severity-critical
|
@tmshort: This pull request references Jira Issue OCPBUGS-86895, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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 |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tmshort: This pull request references Jira Issue OCPBUGS-86895, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@tmshort: This pull request references Jira Issue OCPBUGS-86895, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
273-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an existing non-
RuntimeDefaultprofile.The new cases cover a nil
SeccompProfileand a nilPodSecurityContext. They do not exercise the existing-profile branch at Line 160. Add one table case with a non-RuntimeDefaultprofile and assert that reconciliation reportsmodifiedand restoresRuntimeDefault.🤖 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 `@pkg/package-server-manager/controller_test.go` around lines 273 - 293, Add a table-driven test case in the reconciliation test cases for an existing non- RuntimeDefault SeccompProfile, using the relevant helper to construct that profile. Set the expected result to modified with no error, and define expectedCSV with the same configuration but a RuntimeDefault profile, covering the existing-profile branch alongside the nil-profile cases.
🤖 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 `@pkg/package-server-manager/controller_test.go`:
- Around line 273-293: Add a table-driven test case in the reconciliation test
cases for an existing non- RuntimeDefault SeccompProfile, using the relevant
helper to construct that profile. Set the expected result to modified with no
error, and define expectedCSV with the same configuration but a RuntimeDefault
profile, covering the existing-profile branch alongside the nil-profile cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 46bb4f81-beb5-403f-89ed-b3aebeca5015
📒 Files selected for processing (2)
pkg/package-server-manager/config.gopkg/package-server-manager/controller_test.go
The openshift-operator-lifecycle-manager namespace enforces pod-security.kubernetes.io/enforce: restricted:latest, which requires every pod to have securityContext.seccompProfile.type set to RuntimeDefault or Localhost. The packageserver CSV template (pkg/manifests/csv.yaml) includes the correct pod-level seccompProfile, but there is a window where the cluster-stored CSV can diverge from the template: - The cluster CSV may have been created before seccompProfile was added to the template (commit 26b2061), leaving it absent in the stored object. - During OLM upgrades, a race between the PSM and OLM can cause OLM to process a cached, pre-update version of the CSV and generate a Deployment without the field. In both cases OLM faithfully reproduces whatever is in the cluster CSV, so the generated Deployment pod template is missing seccompProfile. On most clusters this is masked: OpenShift's restricted-v2 SCC mutating admission adds seccompProfile to pods before PSA validates them. On OSD/ROSA Classic the managed admission chain processes PSA before (or independently of) SCC mutation, so pods are rejected, the Deployment stays unhealthy, and OLM enters a perpetual cert-rotation reinstall loop (observed at generation=716, revision=566). Fix: explicitly enforce seccompProfile in ensureCSVHighAvailability, which runs on every PSM reconcile. This makes the field mandatory regardless of what the stored CSV contains, closes the upgrade race window, and does not depend on the reflect.DeepEqual full-spec comparison in ensureCSV catching a single missing field among many. Add three test cases covering: missing seccompProfile with HA topology, missing seccompProfile with single-replica topology, and a nil SecurityContext (defensive nil-safety). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
07524d1 to
dacdc2c
Compare
|
@tmshort: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary
openshift-operator-lifecycle-managernamespace enforcesrestricted:latestPodSecurity, requiringseccompProfile: RuntimeDefaulton all pods.seccompProfile. On most clusters therestricted-v2SCC mutating admission adds it before PSA validates — masking the bug. On OSD/ROSA Classic, PSA runs before SCC mutation, so pods are rejected and OLM enters a perpetual reinstall loop (observed at generation=716, revision=566).seccompProfile: RuntimeDefaultinensureCSVHighAvailability, which runs on every PSM reconcile, making the field mandatory regardless of the stored CSV state.Test plan
TestEnsureCSV: missingseccompProfile(HA topology), missingseccompProfile(single-replica topology), nilSecurityContextgo test ./pkg/package-server-manager/...passesFixes: https://issues.redhat.com/browse/OCPBUGS-86895
🤖 Generated with Claude Code
Summary by CodeRabbit
RuntimeDefaultseccomp profile.