MG-265: Make serviceAccountName required for MustGather CR#363
MG-265: Make serviceAccountName required for MustGather CR#363neha037 wants to merge 10 commits into
Conversation
This change makes the serviceAccountName field mandatory for all MustGather Custom Resources, addressing MG-265 requirements. Changes: - API: Added +kubebuilder:validation:Required marker to serviceAccountName - API: Added +kubebuilder:validation:MinLength=1 validation - API: Removed +kubebuilder:default marker (no default value) - API: Removed 'omitempty' from JSON tag - API: Simplified CEL validation rules for audit mode - Controller: Removed fallback logic for empty serviceAccountName - CRDs: Regenerated with new validation rules - Tests: Added integration test suite for validation scenarios This is a breaking change: existing CRs without serviceAccountName will fail validation after upgrade. This is intentional to force explicit SA configuration and prevent silent failures. Jira: MG-265 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
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
ChangesServiceAccount required and audit validation constraints
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@api/v1alpha1/mustgather_types.go`:
- Around line 31-37: The issue is that serviceAccountName is marked as required
within spec, but spec itself is optional due to the json:"spec,omitempty" tag,
allowing CRs to bypass the requirement by omitting spec entirely. Remove the
omitempty tag from the spec field definition (at line 236) to make it required
at the root level, then regenerate the CRDs using the appropriate kubebuilder
code generation command to ensure the generated manifests reflect that spec is
now a required field.
In `@controllers/mustgather/mustgather_controller.go`:
- Line 157: Add an explicit empty validation check on the saName variable
immediately after it is assigned at line 157 from
instance.Spec.ServiceAccountName. If saName is empty, invoke
setValidationFailureStatus to properly mark this as a validation failure and
prevent the code from proceeding to the Get operation at line 204, which would
otherwise fail non-fatally and cause indefinite requeuing in lines 214-215. This
ensures empty serviceAccountName values are treated as validation failures
rather than transient errors.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 095c3d49-212b-4a3d-bbcf-c2a34501c6ae
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
api/v1alpha1/mustgather_types.goapi/v1alpha1/tests/mustgathers.operator.openshift.io/mustgather.testsuite.yamlbundle/manifests/tech-preview/operator.openshift.io_mustgathers.yamlcontrollers/mustgather/mustgather_controller.godeploy/crds/operator.openshift.io_mustgathers.yaml
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
The controller no longer falls back to "default" when serviceAccountName is empty, so tests must reflect the new validation behavior. Consolidate two SA defaulting tests into one validation-failure test, and add explicit ServiceAccountName to all SFTP test cases so they pass SA validation and actually exercise SFTP logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest-required |
neha037
changed the title
|
@neha037: This pull request references MG-265 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 story 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-robot
added
the
jira/valid-reference
|
/retest-required |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
- Coverage 82.32% 82.28% -0.04%
==========================================
Files 8 8
Lines 905 903 -2
==========================================
- Hits 745 743 -2
Misses 148 148
Partials 12 12
🚀 New features to boost your workflow:
|
|
/retest-required |
| @@ -25,13 +25,15 @@ import ( | |||
| // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. | |||
|
|
|||
| // MustGatherSpec defines the desired state of MustGather | |||
There was a problem hiding this comment.
CodeRabbit flagged that the original messages as it exposed internal field names (imageStreamRef) that end users shouldn't need to see.
|
/retest |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
| // pre-existing CRs created before this check was introduced are flagged. | ||
| saName := instance.Spec.ServiceAccountName | ||
|
|
||
| if saName == "" { |
There was a problem hiding this comment.
This will be a redundant check now. I think we can remove this .
| }, | ||
| { | ||
| name: "reconcile_empty_service_account_name_defaults_to_default", | ||
| name: "reconcile_empty_service_account_name_validation_fails", |
There was a problem hiding this comment.
Please remove this test-case as well as the original code will be removed.
CI Monitor Report: #363PR: MG-265: Make serviceAccountName required for MustGather CR CI Check Summary
Failure Analysis
Actionable Failures (1)lint-failure:
Log snippets for actionable failuresci/prow/validate-boilerplate: Prow Job Breakdown
Generated by oape-ci-monitor on 2026-06-23 05:41 UTC | classification: deterministic (regex-based) | release context: true |
shivprakashmuley
left a comment
shivprakashmuley
left a comment
There was a problem hiding this comment.
One small change @neha037 otherwise looks good .
| required: | ||
| - completed | ||
| type: object | ||
| required: |
There was a problem hiding this comment.
Please add this change in bundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml as well.
|
/lgtm |
|
@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. |
4 participants
Summary
This PR implements MG-265: Mandate ServiceAccount for MustGather CR for all users.
The
serviceAccountNamefield is now required for all MustGather Custom Resources, with no default value. This forces users to explicitly specify which ServiceAccount to use for must-gather jobs, eliminating confusing failures when the default SA lacks necessary permissions.Changes
API Types (
api/v1alpha1/mustgather_types.go)+kubebuilder:validation:Requiredmarker toserviceAccountName+kubebuilder:validation:MinLength=1validation+kubebuilder:default:="default"markeromitemptyfrom JSON tagController (
controllers/mustgather/mustgather_controller.go)serviceAccountNameto "default"Generated Files
api/v1alpha1/zz_generated.deepcopy.go- Regenerateddeploy/crds/operator.openshift.io_mustgathers.yaml- Updated with validationbundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml- UpdatedTests
api/v1alpha1/tests/mustgathers.operator.openshift.io/mustgather.testsuite.yamlAcceptance Criteria
All acceptance criteria from MG-265 are met:
serviceAccountNameis a required field — CRs submitted without it are rejected at admissionserviceAccountNamecannot be set to an empty string (minLength: 1)Testing
Validation Tests
serviceAccountName→ Admission blocked with "required field" errorserviceAccountName→ Admission blocked with "minLength" errorserviceAccountName→ Job created successfullyAudit Mode Tests
audit: truewith default imageaudit: truewith customimageStreamRef→ CEL validation erroraudit: truewith customgatherSpec.command→ CEL validation errorIntegration Test Suite
Generated test suite in
api/v1alpha1/tests/covers:E2E Tests
Generated comprehensive e2e tests (see
output/e2e_must-gather-operator/):Breaking Change⚠️
This is a breaking change. Existing MustGather CRs that omit
serviceAccountNamewill fail validation after the CRD is updated.Migration Path:
serviceAccountNameto all existing MustGather CRsserviceAccountNameCode Review
✅ Production-ready and approved (Principal Engineer review)
Review highlights:
Full review report: See commit message for details
Related Issues
Checklist
Next Steps
🤖 Generated with Claude Code
Summary by CodeRabbit
spec.serviceAccountNameis missing/empty, preventing job creation with an invalid configuration.specis now required.spec.serviceAccountNameis now required and must be non-empty.specchanges on update.