OCPBUGS-79069 - fix the since and sinceTime validation issue#345
OCPBUGS-79069 - fix the since and sinceTime validation issue#345praveencodes wants to merge 5 commits into
Conversation
WalkthroughAdds CRD-level validations for GatherSpec.since and GatherSpec.sinceTime (regex and non-negativity), removes generic format annotations, and enforces a controller-side rejection when SinceTime is in the future; a unit test covering the future-SinceTime path is included. ChangesMustGather validation and reconciliation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as MustGather Controller
participant Clock as System Clock
participant Status as CR Status Updater
Client->>Controller: Reconcile MustGather (with SinceTime)
Controller->>Clock: Read current time (now)
Controller->>Controller: Compare SinceTime > now?
alt SinceTime > now
Controller->>Status: Mark CR Status Failed (ValidationSinceTime)
Status-->>Client: Status updated (Failed)
else SinceTime <= now
Controller->>Controller: Proceed to job lookup/creation
Controller-->>Client: Continue reconcile flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: praveencodes 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml`:
- Around line 75-76: The CRD in bundle/manifests still declares
spec.gatherSpec.since with format: duration while the deploy CRD removed it,
causing inconsistent validation; update the bundle CRD by removing the format:
duration entry for the spec.gatherSpec.since property so the schema matches the
deploy CRD (ensure the property under spec.gatherSpec.since in the
operator.openshift.io_mustgathers CRD only has type: string and no format).
🪄 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: 03cc9209-84ea-4967-89b6-e85c7de30f40
📒 Files selected for processing (6)
api/v1alpha1/mustgather_types.gobundle/manifests/tech-preview/operator.openshift.io_mustgathers.yamlcontrollers/mustgather/constant.gocontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/mustgather_controller_test.godeploy/crds/operator.openshift.io_mustgathers.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #345 +/- ##
==========================================
- Coverage 62.89% 62.87% -0.02%
==========================================
Files 8 8
Lines 830 835 +5
==========================================
+ Hits 522 525 +3
- Misses 302 303 +1
- Partials 6 7 +1
🚀 New features to boost your workflow:
|
praveencodes
force-pushed
the
OCPBUGS-79069
branch
from
6ce4a4a to
d4fa624
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 `@controllers/mustgather/mustgather_controller.go`:
- Around line 401-406: Run the sinceTime validation (the block that calls
setValidationFailureStatus with ValidationSinceTime) before calling
getMustGatherImage / reading OPERATOR_IMAGE so it short-circuits early; after
setting the validation failure status return a handled reconcile result that
does not propagate an error to ManageError (i.e., return the zero/handled
ctrl.Result and nil error from Reconcile) so downstream ManageError is not
invoked. Ensure you update the control flow in Reconcile to check the validation
first and use the same setValidationFailureStatus call/site as before.
🪄 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: b749aef8-f8e6-48df-a55e-3ec7310a9979
📒 Files selected for processing (2)
controllers/mustgather/mustgather_controller.gocontrollers/mustgather/mustgather_controller_test.go
| if instance.Spec.GatherSpec != nil && instance.Spec.GatherSpec.SinceTime != nil && instance.Spec.GatherSpec.SinceTime.After(time.Now()) { | ||
| err := fmt.Errorf("gatherSpec.sinceTime must be at or before the current date and time") | ||
| if _, validationErr := r.setValidationFailureStatus(ctx, log, instance, ValidationSinceTime, err); validationErr != nil { | ||
| return nil, fmt.Errorf("failed to set validation failure status: %w, %w", err, validationErr) | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
Short-circuit handled sinceTime validation before generic error handling.
This branch updates status via setValidationFailureStatus, but then still returns a non-nil error. Downstream, Line 159 sends that error into ManageError, so the future-sinceTime case is not actually a clean validation short-circuit. Also, because this check runs after image/env resolution, unrelated prereq failures can mask the sinceTime validation entirely.
Please run this validation before getMustGatherImage / OPERATOR_IMAGE lookup, and return a handled signal that Reconcile can exit on without calling ManageError.
🤖 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/mustgather/mustgather_controller.go` around lines 401 - 406, Run
the sinceTime validation (the block that calls setValidationFailureStatus with
ValidationSinceTime) before calling getMustGatherImage / reading OPERATOR_IMAGE
so it short-circuits early; after setting the validation failure status return a
handled reconcile result that does not propagate an error to ManageError (i.e.,
return the zero/handled ctrl.Result and nil error from Reconcile) so downstream
ManageError is not invoked. Ensure you update the control flow in Reconcile to
check the validation first and use the same setValidationFailureStatus call/site
as before.
|
@praveencodes: 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
OCPBUGS-79069 - Fixed the since and sinceTime validation issue.
since: 1d / since: 1w / since: 1y - The invalid values like this give validation error now.
since: -10s / since: -10d - These negative since values give validation error now.
sinceTime: 2026-04-02T00:00:00Z - The future date won't work. The CR error status will be updated now.
sinceTime: 2026-02-02t00:00:00Z / sinceTime: 2026-02-02T00:00:00z - The invalid values like these gives the validation error now.
Validation error for invalid since value - The MustGather "mustgather-with-since" is invalid: spec.gatherSpec: Invalid value: "object": since may only use these duration suffixes: ns, us, µs, ms, s, m, h (e.g. 2h, 30m, 168h for one week).
Validation error for invalid sinceTime value - The MustGather "mustgather-with-sincetime" is invalid: spec.gatherSpec: Invalid value: "object": sinceTime must be in RFC3339 format.
NOTE: since: 0s is not handled as part of this PR.
Summary by CodeRabbit
Bug Fixes
sincemust be non-negative and use allowed duration units;sinceTimemust match RFC3339 and cannot be in the future. Reconciliation now marks instances as validation failures whensinceTimeis a future timestamp.Tests
sinceTimevalidation path.