OCPBUGS-78538: fix default service account#342
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@neha037: This pull request references Jira Issue OCPBUGS-78538, 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. |
|
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:
WalkthroughDefault for MustGather.spec.serviceAccountName changed from "default" to "must-gather-admin"; a MinLength=1 validation was added to the CRD; controller defaults and tests updated; new ValidatingAdmissionPolicy and Binding plus e2e testdata/fixtures enforce and exercise ServiceAccount "use" authorization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User
participant APIServer as API Server
participant Admission as ValidatingAdmissionPolicy
participant Authorizer as Authorizer
participant Controller as MustGather Controller
participant Kube as Kubernetes API
Client->>APIServer: Create MustGather (spec.serviceAccountName set/omitted)
APIServer->>Admission: Evaluate policy for CREATE mustgathers
Admission->>Authorizer: Check "use" permission for ServiceAccount in namespace
Authorizer-->>Admission: allow / deny
alt allowed
APIServer-->>Client: Admit object (defaulting applies if omitted)
Controller->>Kube: Get ServiceAccount (uses default if omitted)
Kube-->>Controller: ServiceAccount exists / not
Controller->>Kube: Create Job with ServiceAccountName
else denied
Admission-->>APIServer: Deny (Forbidden)
APIServer-->>Client: Reject request with authorization message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #342 +/- ##
=======================================
Coverage 62.89% 62.89%
=======================================
Files 8 8
Lines 830 830
=======================================
Hits 522 522
Misses 302 302
Partials 6 6
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/mustgather/mustgather_controller_test.go (1)
775-818:⚠️ Potential issue | 🟡 MinorMinor: Update stale comment referencing 'default' service account.
The test name and comments correctly reference
must-gather-admin, but the assertion message at line 816 still mentions'default':t.Fatalf("expected job to not be created when 'default' service account is missing")This should reference
'must-gather-admin'for consistency.📝 Suggested fix
- t.Fatalf("expected job to not be created when 'default' service account is missing") + t.Fatalf("expected job to not be created when 'must-gather-admin' service account is missing")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/mustgather/mustgather_controller_test.go` around lines 775 - 818, Update the failing assertion message in the test case "reconcile_empty_service_account_name_must_gather_admin_not_found_calls_manage_error" (inside the postTestChecks closure) to reference the correct service account name: replace the t.Fatalf message that currently says "'default' service account is missing" with one that mentions "'must-gather-admin' service account is missing" so the comment and assertion message are consistent with the rest of the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/crds/operator.openshift.io_mustgathers.yaml`:
- Line 6: Regenerate both CRD manifests so their controller-gen annotations
match: run controller-gen (prefer project standard e.g., v0.19.0) to re-generate
deploy/crds/operator.openshift.io_mustgathers.yaml and
bundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml, replacing
the mismatched controller-gen.kubebuilder.io/version entry (currently v0.16.4 vs
v0.19.0) so both files contain the same version and identical generated output;
verify the file headers now match and commit the updated manifests.
---
Outside diff comments:
In `@controllers/mustgather/mustgather_controller_test.go`:
- Around line 775-818: Update the failing assertion message in the test case
"reconcile_empty_service_account_name_must_gather_admin_not_found_calls_manage_error"
(inside the postTestChecks closure) to reference the correct service account
name: replace the t.Fatalf message that currently says "'default' service
account is missing" with one that mentions "'must-gather-admin' service account
is missing" so the comment and assertion message are consistent with the rest of
the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf6160b7-8b0e-4fbf-b798-a80877467137
📒 Files selected for processing (6)
api/v1alpha1/mustgather_types.gobundle/manifests/tech-preview/operator.openshift.io_mustgathers.yamlcontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/mustgather_controller_test.gocontrollers/mustgather/template_test.godeploy/crds/operator.openshift.io_mustgathers.yaml
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@neha037: This pull request references Jira Issue OCPBUGS-78538, 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. |
openshift-ci-robot
added
jira/invalid-bug
|
@neha037: This pull request references Jira Issue OCPBUGS-78538, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@neha037: This pull request references Jira Issue OCPBUGS-78538, 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. |
These files were referenced by e2e tests but never committed, causing CI to fail with "file does not exist" in the BeforeAll hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ValidatingAdmissionPolicy blocks CR creation with 403 when the non-admin user lacks 'use' permission on the SA. Update the test to assert the admission denial instead of expecting controller-level status conditions on a CR that never gets created. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neha037 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 |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test all |
|
@neha037: 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. |
|
Changes will be done here - #347 |
|
@neha037: This pull request references Jira Issue OCPBUGS-78538. The bug has been updated to no longer 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. |
4 participants
Summary by CodeRabbit
Chores
Tests