CNTRLPLANE-3386: Required scc annotation checker updates - #31325
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@liouk: This pull request references CNTRLPLANE-3386 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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
Bot
added
the
do-not-merge/work-in-progress
|
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:
Walkthrough
ChangesRequired-SCC Monitor Filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liouk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
… required-scc requirement
…e system namespaces instead of flaking
b9ff22b to
51eaa69
Compare
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 `@pkg/monitortests/authentication/requiredsccmonitortests/monitortest.go`:
- Line 84: The pod filtering in CollectData only skips entire namespaces, so
CatalogSource-owned pods in allowed namespaces still reach the SCC validation
path. Update the pod-processing logic in CollectData to add an explicit owner
check for CatalogSource before calling the RequiredSCCAnnotation validation,
using the existing namespace skip helper and the pod iteration flow to keep the
exclusion at the pod level.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7d959271-f444-4097-bcc5-f7dc5f1c240e
📒 Files selected for processing (1)
pkg/monitortests/authentication/requiredsccmonitortests/monitortest.go
|
Scheduling required tests: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/monitortests/authentication/requiredsccmonitortests/monitortest.go`:
- Around line 102-105: Update the shouldSkipPod function to add an explicit
owner-kind check for CatalogSource, in addition to the existing ignoredWorkloads
lookup. The function should return true when a pod is owned by a CatalogSource,
ensuring that pods owned by CatalogSource are properly skipped during SCC
validation regardless of their namespace or name.
- Around line 50-55: Update shouldSkipPod to account for the actual controller
kind behind each ignored workload: when an ignored workload is
Deployment-managed, resolve the ReplicaSet owner to the Deployment name or use
an appropriate generated-name prefix match instead of exact matching owner.Name.
Verify all three ignoredWorkloads entries against their controller types,
preserving exact matching for non-Deployment controllers.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d310279-6d36-4912-bec0-cb6fea4e21dc
📒 Files selected for processing (1)
pkg/monitortests/authentication/requiredsccmonitortests/monitortest.go
|
Scheduling required tests: |
…pin an SCC to In case of proprietary workloads, or workloads for which we do not have a way of modifying their deployment to add the annotation to.
a0cc1e6 to
97ca722
Compare
liouk
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Holding until all dependency PRs listed in the description get merged. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling required tests: |
|
@liouk: The following tests 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
This PR does the following updates in the
required-scc-annotation-checkermonitor test:openshift.io/run-levelsince SCC admission does not run on them, instead of carrying them as pending exceptions.namespacesWithPendingSCCPinninglist and its flake-on-failure behavior are removed. System namespaces now cause a hard failure instead of a flake.nutanix-csi,nutanix-csi-controller,nutanix-csi-operator-controller-manager) that we cannot pin a required SCC to. Matching uses both exact and prefix comparison to handle Deployment-owned pods whose ReplicaSet names include a hash suffix.shouldSkipNamespacehelper for readability.Dependencies
This PR depends on the following fix-PRs:
Summary by CodeRabbit