OADP-7829: Add additive extraArgs support for Velero server and node-agent - #2230
OADP-7829: Add additive extraArgs support for Velero server and node-agent#2230Shreyashxredhat wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds validated ChangesAdditive extraArgs configuration
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant DPA
participant NodeAgentController
participant VeleroController
participant MergeExtraArgs
participant Workloads
DPA->>NodeAgentController: provide NodeAgent ExtraArgs
DPA->>VeleroController: provide Velero ExtraArgs
NodeAgentController->>MergeExtraArgs: merge Node-Agent arguments
VeleroController->>MergeExtraArgs: merge Velero arguments
MergeExtraArgs->>Workloads: update container arguments
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @Shreyashxredhat. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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/dataprotectionapplication_types.go`:
- Around line 373-377: Update the comment for the ExtraArgs field(s) to
explicitly document the full precedence order including the unsupported-args
annotation as the final override; specifically, in
dataprotectionapplication_types.go update the comment above the ExtraArgs
map[string]string `json:"extraArgs,omitempty"` (and the similar comment at the
other occurrence) to state that ExtraArgs are applied additively on top of
operator defaults and Args, and then any unsupported-args annotation on the
resource is applied last and overrides conflicting keys.
In `@config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml`:
- Around line 297-304: The extraArgs map currently allows arbitrary keys; add a
propertyNames schema to constrain map keys to valid flag names (no leading "--",
no whitespace or empty strings). Update the extraArgs schema (identifier:
extraArgs in the YAML) to include a propertyNames block with a regex pattern
(e.g. ^[A-Za-z0-9][A-Za-z0-9_.-]*$ or other agreed flag-name pattern) and
optionally minLength: 1 so invalid keys like "--log-level", empty strings or
strings with spaces are rejected at the CRD boundary.
🪄 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: 891c2791-4260-4690-bb85-edc1fc97c193
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (9)
api/v1alpha1/dataprotectionapplication_types.gobundle/manifests/oadp.openshift.io_dataprotectionapplications.yamlconfig/crd/bases/oadp.openshift.io_dataprotectionapplications.yamlinternal/controller/nodeagent.gointernal/controller/nodeagent_test.gointernal/controller/velero.gointernal/controller/velero_test.gopkg/common/common.gopkg/common/common_test.go
…cument precedence - Remove fmt.Printf/cmp.Diff debug logging that could expose sensitive ExtraArgs values; replace with structured log.Info - Add normalizeExtraArgKeys() to strip leading dashes and skip empty/whitespace keys at runtime - Document full precedence order in API docs and CRD descriptions: operator defaults / Args < ExtraArgs < unsupported-args annotation - Add test cases for key normalization behavior Co-authored-by: Cursor <cursoragent@cursor.com>
weshayutin
added
ok-to-test
|
@shubham-pampattiwar @sseago reviews? |
|
/test 4.22-images |
kaovilai
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@Shreyashxredhat: This pull request references OADP-7829 which is a valid jira issue. 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. |
|
/cherry-pick oadp-1.4 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
approved
CodeRabbit flagged that extraArgs map keys accepted arbitrary strings (e.g. "--log-level", whitespace, empty) with no rejection at the CRD boundary. The prior fix only normalized keys at runtime in common.MergeExtraArgs, which silently massages bad input instead of rejecting it. Add a kubebuilder XValidation CEL rule to VeleroConfig.ExtraArgs and NodeAgentConfig.ExtraArgs so malformed keys are rejected at admission time. Runtime normalization stays in place as a defensive fallback for CRs persisted before this validation existed. Regenerated config/crd/bases and bundle/manifests via make manifests/generate/bundle. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
openshift-ci
Bot
removed
the
approved
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
/retest |
|
@Shreyashxredhat: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, Shreyashxredhat, sseago 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 |
7 participants
Why the changes were made
Closes #2209
Currently, configuring additional Velero server or node-agent arguments requires using full argument overrides, which forces users to replicate the operator-generated defaults. This approach is difficult to maintain and can lead to configuration drift when defaults change in future releases.
This change introduces a new
extraArgsfield for bothVeleroConfigandNodeAgentConfig. The field allows users to add or override specific command-line arguments while preserving the operator-managed defaults.Key behaviors:
extraArgsare merged with the operator-generated arguments.extraArgs.--flag=valueand--flag valueargument formats.Precedence order:
How to test the changes made
Deploy the operator with this change.
Create a DPA with
extraArgsconfigured:Verify that specifying an existing flag in
extraArgsoverrides the operator-generated value.Run the test suite:
make testExpected results:
extraArgs.extraArgs.Summary by CodeRabbit
New Features
Testing