From ee1c692ed91eb0d26c9e654077e661cb7380231a Mon Sep 17 00:00:00 2001 From: shreyash bhosale Date: Mon, 1 Jun 2026 22:29:09 +0530 Subject: [PATCH 1/2] add additive extraArgs for Velero server and node-agent (#2209) --- .../dataprotectionapplication_types.go | 10 ++ api/v1alpha1/zz_generated.deepcopy.go | 14 +++ ...enshift.io_dataprotectionapplications.yaml | 16 +++ ...enshift.io_dataprotectionapplications.yaml | 16 +++ internal/controller/nodeagent.go | 4 + internal/controller/nodeagent_test.go | 87 ++++++++++++++ internal/controller/velero.go | 3 + internal/controller/velero_test.go | 113 ++++++++++++++++++ pkg/common/common.go | 70 +++++++++++ pkg/common/common_test.go | 79 ++++++++++++ 10 files changed, 412 insertions(+) diff --git a/api/v1alpha1/dataprotectionapplication_types.go b/api/v1alpha1/dataprotectionapplication_types.go index f26f5d5192..43fc3564e8 100644 --- a/api/v1alpha1/dataprotectionapplication_types.go +++ b/api/v1alpha1/dataprotectionapplication_types.go @@ -370,6 +370,11 @@ type VeleroConfig struct { // Velero args are settings to customize velero server arguments. Overrides values in other fields. // +optional Args *VeleroServerArgs `json:"args,omitempty"` + // ExtraArgs are additional arguments to append to the Velero server. + // Keys are flag names (without leading --), values are flag values. + // These are applied additively on top of operator defaults and Args. + // +optional + ExtraArgs map[string]string `json:"extraArgs,omitempty"` // LoadAffinityConfig is the config for data path load affinity. // +optional LoadAffinityConfig []*LoadAffinity `json:"loadAffinity,omitempty"` @@ -598,6 +603,11 @@ type NodeAgentConfig struct { // Embedding KopiaRepoOptions // +optional KopiaRepoOptions `json:",inline"` + // ExtraArgs are additional arguments to append to the node-agent server. + // Keys are flag names (without leading --), values are flag values. + // These are applied additively on top of operator defaults. + // +optional + ExtraArgs map[string]string `json:"extraArgs,omitempty"` } type KopiaRepoOptions struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index aa1db83c2a..b6d3fadb2f 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -901,6 +901,13 @@ func (in *NodeAgentConfig) DeepCopyInto(out *NodeAgentConfig) { } in.NodeAgentConfigMapSettings.DeepCopyInto(&out.NodeAgentConfigMapSettings) in.KopiaRepoOptions.DeepCopyInto(&out.KopiaRepoOptions) + if in.ExtraArgs != nil { + in, out := &in.ExtraArgs, &out.ExtraArgs + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodeAgentConfig. @@ -1405,6 +1412,13 @@ func (in *VeleroConfig) DeepCopyInto(out *VeleroConfig) { *out = new(VeleroServerArgs) (*in).DeepCopyInto(*out) } + if in.ExtraArgs != nil { + in, out := &in.ExtraArgs, &out.ExtraArgs + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.LoadAffinityConfig != nil { in, out := &in.LoadAffinityConfig, &out.LoadAffinityConfig *out = make([]*LoadAffinity, len(*in)) diff --git a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml index a88ff5ad91..2a1613775f 100644 --- a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml +++ b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml @@ -294,6 +294,14 @@ spec: enable defines a boolean pointer whether we want the daemonset to exist or not type: boolean + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs are additional arguments to append to the node-agent server. + Keys are flag names (without leading --), values are flag values. + These are applied additively on top of operator defaults. + type: object fullMaintenanceInterval: description: |- fullMaintenanceInterval determines the time between kopia full maintenance operations. @@ -1274,6 +1282,14 @@ spec: the 1-second polling interval for the first 10 seconds while waiting for the snaphandle type: boolean + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs are additional arguments to append to the Velero server. + Keys are flag names (without leading --), values are flag values. + These are applied additively on top of operator defaults and Args. + type: object featureFlags: description: featureFlags defines the list of features to enable for Velero instance items: diff --git a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml index b820f03362..c59d99c701 100644 --- a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml +++ b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml @@ -294,6 +294,14 @@ spec: enable defines a boolean pointer whether we want the daemonset to exist or not type: boolean + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs are additional arguments to append to the node-agent server. + Keys are flag names (without leading --), values are flag values. + These are applied additively on top of operator defaults. + type: object fullMaintenanceInterval: description: |- fullMaintenanceInterval determines the time between kopia full maintenance operations. @@ -1274,6 +1282,14 @@ spec: the 1-second polling interval for the first 10 seconds while waiting for the snaphandle type: boolean + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs are additional arguments to append to the Velero server. + Keys are flag names (without leading --), values are flag values. + These are applied additively on top of operator defaults and Args. + type: object featureFlags: description: featureFlags defines the list of features to enable for Velero instance items: diff --git a/internal/controller/nodeagent.go b/internal/controller/nodeagent.go index e78e830309..a8bc91f740 100644 --- a/internal/controller/nodeagent.go +++ b/internal/controller/nodeagent.go @@ -702,6 +702,10 @@ func (r *DataProtectionApplicationReconciler) customizeNodeAgentDaemonset(ds *ap nodeAgentContainer.Args = append(nodeAgentContainer.Args, fmt.Sprintf("--log-format=%s", dpa.Spec.LogFormat)) } + if len(dpa.Spec.Configuration.NodeAgent.ExtraArgs) > 0 { + nodeAgentContainer.Args = common.MergeExtraArgs(nodeAgentContainer.Args, dpa.Spec.Configuration.NodeAgent.ExtraArgs) + } + // Apply unsupported server args from the specified ConfigMap. // This will completely override any previously set args for the node-agent server. // If the ConfigMap exists and is not empty, its key-value pairs will be used as the new CLI arguments. diff --git a/internal/controller/nodeagent_test.go b/internal/controller/nodeagent_test.go index b71470bfa6..baa58cb4c7 100644 --- a/internal/controller/nodeagent_test.go +++ b/internal/controller/nodeagent_test.go @@ -927,6 +927,93 @@ func TestDPAReconciler_buildNodeAgentDaemonset(t *testing.T) { nodeAgentDaemonSet: testNodeAgentDaemonSet.DeepCopy(), errorMessage: "configmaps \"missing-unsupported-node-agent-server-args-cm\" not found", }, + { + name: "valid DPA CR with NodeAgent ExtraArgs, DaemonSet is built with extra args appended", + dpa: createTestDpaWith( + nil, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{}, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{}, + UploaderType: "kopia", + ExtraArgs: map[string]string{ + "custom-flag": "value1", + }, + }, + }, + }, + ), + clientObjects: []client.Object{testGenericInfrastructure}, + nodeAgentDaemonSet: testNodeAgentDaemonSet.DeepCopy(), + wantNodeAgentDaemonSet: createTestBuiltNodeAgentDaemonSet(TestBuiltNodeAgentDaemonSetOptions{ + args: []string{ + "--custom-flag=value1", + }, + }), + }, + { + name: "valid DPA CR with NodeAgent ExtraArgs overriding an existing arg, DaemonSet is built with overridden value", + dpa: createTestDpaWith( + nil, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{}, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{}, + DataMoverPrepareTimeout: &metav1.Duration{Duration: 10 * time.Second}, + UploaderType: "kopia", + ExtraArgs: map[string]string{ + "data-mover-prepare-timeout": "45m", + }, + }, + }, + }, + ), + clientObjects: []client.Object{testGenericInfrastructure}, + nodeAgentDaemonSet: testNodeAgentDaemonSet.DeepCopy(), + wantNodeAgentDaemonSet: createTestBuiltNodeAgentDaemonSet(TestBuiltNodeAgentDaemonSetOptions{ + args: []string{ + "--data-mover-prepare-timeout=45m", + }, + }), + }, + { + name: "valid DPA CR with NodeAgent ExtraArgs and Unsupported Server Args, annotation takes priority", + dpa: createTestDpaWith( + map[string]string{common.UnsupportedNodeAgentServerArgsAnnotation: "unsupported-node-agent-server-args-cm"}, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{}, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{}, + UploaderType: "kopia", + ExtraArgs: map[string]string{ + "custom-flag": "value1", + }, + }, + }, + }, + ), + clientObjects: []client.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unsupported-node-agent-server-args-cm", + Namespace: testNamespaceName, + }, + Data: map[string]string{ + "unsupported-arg": "value2", + }, + }, + testGenericInfrastructure, + }, + nodeAgentDaemonSet: testNodeAgentDaemonSet.DeepCopy(), + wantNodeAgentDaemonSet: createTestBuiltNodeAgentDaemonSet(TestBuiltNodeAgentDaemonSetOptions{ + args: []string{ + "--unsupported-arg=value2", + }, + }), + }, { name: "valid DPA CR with NodeAgent resource allocations, NodeAgent DaemonSet is built with resource allocations", dpa: createTestDpaWith( diff --git a/internal/controller/velero.go b/internal/controller/velero.go index 5dffa21d74..9a1d55277e 100644 --- a/internal/controller/velero.go +++ b/internal/controller/velero.go @@ -480,6 +480,9 @@ func (r *DataProtectionApplicationReconciler) customizeVeleroDeployment(veleroDe } r.appendPluginSpecificSpecs(veleroDeployment, veleroContainer, providerNeedsDefaultCreds) setPodTemplateSpecDefaults(&veleroDeployment.Spec.Template) + if dpa.Spec.Configuration.Velero != nil && len(dpa.Spec.Configuration.Velero.ExtraArgs) > 0 { + veleroContainer.Args = common.MergeExtraArgs(veleroContainer.Args, dpa.Spec.Configuration.Velero.ExtraArgs) + } if configMapName, ok := dpa.Annotations[common.UnsupportedVeleroServerArgsAnnotation]; ok { if configMapName != "" { unsupportedServerArgsCM := corev1.ConfigMap{} diff --git a/internal/controller/velero_test.go b/internal/controller/velero_test.go index 5cfc3dced0..2855fc9aea 100644 --- a/internal/controller/velero_test.go +++ b/internal/controller/velero_test.go @@ -1226,6 +1226,119 @@ func TestDPAReconciler_buildVeleroDeployment(t *testing.T) { veleroDeployment: testVeleroDeployment.DeepCopy(), errorMessage: "configmaps \"missing-unsupported-server-args-cm\" not found", }, + { + name: "valid DPA CR with ExtraArgs, Velero Deployment is built with extra args appended", + dpa: createTestDpaWith( + nil, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + ExtraArgs: map[string]string{ + "custom-timeout": "30m", + "another-flag": "value1", + }, + }, + }, + }, + ), + veleroDeployment: testVeleroDeployment.DeepCopy(), + wantVeleroDeployment: createTestBuiltVeleroDeployment(TestBuiltVeleroDeploymentOptions{ + args: []string{ + defaultFileSystemBackupTimeout, + defaultRestoreResourcePriorities, + defaultDisableInformerCache, + "--another-flag=value1", + "--custom-timeout=30m", + }, + }), + }, + { + name: "valid DPA CR with ExtraArgs overriding a default, Velero Deployment is built with overridden value", + dpa: createTestDpaWith( + nil, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + ExtraArgs: map[string]string{ + "fs-backup-timeout": "8h", + }, + }, + }, + }, + ), + veleroDeployment: testVeleroDeployment.DeepCopy(), + wantVeleroDeployment: createTestBuiltVeleroDeployment(TestBuiltVeleroDeploymentOptions{ + args: []string{ + "--fs-backup-timeout=8h", + defaultRestoreResourcePriorities, + defaultDisableInformerCache, + }, + }), + }, + { + name: "valid DPA CR with ExtraArgs and Unsupported Server Args, annotation takes priority", + dpa: createTestDpaWith( + map[string]string{common.UnsupportedVeleroServerArgsAnnotation: "unsupported-server-args-cm"}, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + ExtraArgs: map[string]string{ + "custom-flag": "value1", + }, + }, + }, + }, + ), + clientObjects: []client.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unsupported-server-args-cm", + Namespace: testNamespaceName, + }, + Data: map[string]string{ + "unsupported-arg": "value2", + }, + }, + }, + veleroDeployment: testVeleroDeployment.DeepCopy(), + wantVeleroDeployment: createTestBuiltVeleroDeployment(TestBuiltVeleroDeploymentOptions{ + args: []string{ + "--unsupported-arg=value2", + }, + }), + }, + { + name: "valid DPA CR with Args and ExtraArgs, ExtraArgs adds on top of Args", + dpa: createTestDpaWith( + nil, + oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + Args: &oadpv1alpha1.VeleroServerArgs{ + ServerFlags: oadpv1alpha1.ServerFlags{ + MetricsAddress: fmt.Sprintf(":%v", argsMetricsPortTest), + }, + }, + ExtraArgs: map[string]string{ + "custom-flag": "custom-value", + "disable-informer-cache": "true", + }, + }, + }, + }, + ), + veleroDeployment: testVeleroDeployment.DeepCopy(), + wantVeleroDeployment: createTestBuiltVeleroDeployment(TestBuiltVeleroDeploymentOptions{ + metricsPort: argsMetricsPortTest, + args: []string{ + fmt.Sprintf("--metrics-address=:%v", argsMetricsPortTest), + "--fs-backup-timeout=4h0m0s", + defaultRestoreResourcePriorities, + "--disable-informer-cache=true", + "--custom-flag=custom-value", + }, + }), + }, { name: "valid DPA CR with ItemOperationSyncFrequency, Velero Deployment is built with ItemOperationSyncFrequency arg", dpa: createTestDpaWith( diff --git a/pkg/common/common.go b/pkg/common/common.go index 32eea51c15..0851bfb9e9 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -301,6 +301,76 @@ func ApplyUnsupportedServerArgsOverride(container *corev1.Container, unsupported } } +// MergeExtraArgs merges extraArgs into a copy of the existing container args. +// Handles both --flag=value and --flag value formats. +// If a flag already exists, its value is replaced on the first occurrence and +// subsequent duplicates are removed. New flags are appended in sorted order. +func MergeExtraArgs(args []string, extraArgs map[string]string) []string { + if len(extraArgs) == 0 { + return args + } + + replaced := make(map[string]bool, len(extraArgs)) + var result []string + skip := false + + for i, arg := range args { + if skip { + skip = false + continue + } + + flagName := extractFlagName(arg) + value, isExtra := extraArgs[flagName] + if !isExtra { + result = append(result, arg) + continue + } + + if replaced[flagName] { + if !strings.Contains(arg, "=") && i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") { + skip = true + } + continue + } + + replaced[flagName] = true + if strings.Contains(arg, "=") { + result = append(result, fmt.Sprintf("--%s=%s", flagName, value)) + } else { + result = append(result, arg) + if i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") { + result = append(result, value) + skip = true + } + } + } + + var extra []string + for key, value := range extraArgs { + if !replaced[key] { + extra = append(extra, fmt.Sprintf("--%s=%s", key, value)) + } + } + sort.Strings(extra) + result = append(result, extra...) + + return result +} + +// extractFlagName returns the flag name from an arg like --flag=value or --flag. +// Returns empty string for non-flag args (e.g. "server"). +func extractFlagName(arg string) string { + if !strings.HasPrefix(arg, "-") { + return "" + } + arg = strings.TrimLeft(arg, "-") + if idx := strings.Index(arg, "="); idx >= 0 { + return arg[:idx] + } + return arg +} + // UpdateBackupStorageLocation updates the BackupStorageLocation spec and config. // //nolint:unparam // Keeping error return type for making the public function flexible for future usage diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 711c8eddf5..644cfb4e0b 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -310,6 +310,85 @@ func TestGenerateCliArgsFromConfigMap(t *testing.T) { } } +func TestMergeExtraArgs(t *testing.T) { + tests := []struct { + name string + args []string + extraArgs map[string]string + want []string + }{ + { + name: "nil extraArgs is a no-op", + args: []string{"server", "--log-level=info"}, + extraArgs: nil, + want: []string{"server", "--log-level=info"}, + }, + { + name: "empty extraArgs is a no-op", + args: []string{"server", "--log-level=info"}, + extraArgs: map[string]string{}, + want: []string{"server", "--log-level=info"}, + }, + { + name: "new flags are appended in sorted order", + args: []string{"server", "--log-level=info"}, + extraArgs: map[string]string{"resource-timeout": "20m", "custom-flag": "val"}, + want: []string{"server", "--log-level=info", "--custom-flag=val", "--resource-timeout=20m"}, + }, + { + name: "existing equals-format flag is replaced in place", + args: []string{"server", "--log-level=info", "--fs-backup-timeout=4h"}, + extraArgs: map[string]string{"log-level": "debug"}, + want: []string{"server", "--log-level=debug", "--fs-backup-timeout=4h"}, + }, + { + name: "existing space-separated flag is replaced in place", + args: []string{"server", "--log-level", "info", "--fs-backup-timeout=4h"}, + extraArgs: map[string]string{"log-level": "debug"}, + want: []string{"server", "--log-level", "debug", "--fs-backup-timeout=4h"}, + }, + { + name: "mix of replace and append", + args: []string{"server", "--log-level=info", "--fs-backup-timeout=4h"}, + extraArgs: map[string]string{"log-level": "debug", "new-flag": "value"}, + want: []string{"server", "--log-level=debug", "--fs-backup-timeout=4h", "--new-flag=value"}, + }, + { + name: "does not mutate input slice", + args: []string{"server", "--log-level=info"}, + extraArgs: map[string]string{"log-level": "debug"}, + want: []string{"server", "--log-level=debug"}, + }, + { + name: "duplicate flags are deduplicated", + args: []string{"server", "--log-level=info", "--log-level=warning"}, + extraArgs: map[string]string{"log-level": "debug"}, + want: []string{"server", "--log-level=debug"}, + }, + { + name: "duplicate space-separated flags are deduplicated", + args: []string{"server", "--log-level", "info", "--log-level", "warning"}, + extraArgs: map[string]string{"log-level": "debug"}, + want: []string{"server", "--log-level", "debug"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := make([]string, len(tt.args)) + copy(original, tt.args) + + got := MergeExtraArgs(tt.args, tt.extraArgs) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("MergeExtraArgs() = %v, want %v", got, tt.want) + } + if !reflect.DeepEqual(tt.args, original) { + t.Errorf("MergeExtraArgs() mutated input: got %v, original was %v", tt.args, original) + } + }) + } +} + func TestUpdateBackupStorageLocation(t *testing.T) { tests := []struct { name string From edc84340826bf4fc7a1bbc276f54d2bd786b1c8b Mon Sep 17 00:00:00 2001 From: shreyash bhosale Date: Tue, 2 Jun 2026 09:06:22 +0530 Subject: [PATCH 2/2] address PR review: fix sensitive data logging, add key validation, document 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 --- .../dataprotectionapplication_types.go | 10 +++++-- ...enshift.io_dataprotectionapplications.yaml | 10 +++++-- ...enshift.io_dataprotectionapplications.yaml | 10 +++++-- internal/controller/velero.go | 9 ++---- pkg/common/common.go | 28 +++++++++++++++++-- pkg/common/common_test.go | 14 ++++++++++ 6 files changed, 65 insertions(+), 16 deletions(-) diff --git a/api/v1alpha1/dataprotectionapplication_types.go b/api/v1alpha1/dataprotectionapplication_types.go index 43fc3564e8..77df93f86e 100644 --- a/api/v1alpha1/dataprotectionapplication_types.go +++ b/api/v1alpha1/dataprotectionapplication_types.go @@ -372,7 +372,10 @@ type VeleroConfig struct { Args *VeleroServerArgs `json:"args,omitempty"` // ExtraArgs are additional arguments to append to the Velero server. // Keys are flag names (without leading --), values are flag values. - // These are applied additively on top of operator defaults and Args. + // These are merged additively on top of operator defaults and Args. + // If the unsupported-args annotation is set, it takes highest precedence + // and completely overrides all other args including ExtraArgs. + // Precedence: operator defaults / Args < ExtraArgs < unsupported-args annotation. // +optional ExtraArgs map[string]string `json:"extraArgs,omitempty"` // LoadAffinityConfig is the config for data path load affinity. @@ -605,7 +608,10 @@ type NodeAgentConfig struct { KopiaRepoOptions `json:",inline"` // ExtraArgs are additional arguments to append to the node-agent server. // Keys are flag names (without leading --), values are flag values. - // These are applied additively on top of operator defaults. + // These are merged additively on top of operator defaults. + // If the unsupported-args annotation is set, it takes highest precedence + // and completely overrides all other args including ExtraArgs. + // Precedence: operator defaults < ExtraArgs < unsupported-args annotation. // +optional ExtraArgs map[string]string `json:"extraArgs,omitempty"` } diff --git a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml index 2a1613775f..49bc46b40e 100644 --- a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml +++ b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml @@ -300,7 +300,10 @@ spec: description: |- ExtraArgs are additional arguments to append to the node-agent server. Keys are flag names (without leading --), values are flag values. - These are applied additively on top of operator defaults. + These are merged additively on top of operator defaults. + If the unsupported-args annotation is set, it takes highest precedence + and completely overrides all other args including ExtraArgs. + Precedence: operator defaults < ExtraArgs < unsupported-args annotation. type: object fullMaintenanceInterval: description: |- @@ -1288,7 +1291,10 @@ spec: description: |- ExtraArgs are additional arguments to append to the Velero server. Keys are flag names (without leading --), values are flag values. - These are applied additively on top of operator defaults and Args. + These are merged additively on top of operator defaults and Args. + If the unsupported-args annotation is set, it takes highest precedence + and completely overrides all other args including ExtraArgs. + Precedence: operator defaults / Args < ExtraArgs < unsupported-args annotation. type: object featureFlags: description: featureFlags defines the list of features to enable for Velero instance diff --git a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml index c59d99c701..63b6fb1a52 100644 --- a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml +++ b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml @@ -300,7 +300,10 @@ spec: description: |- ExtraArgs are additional arguments to append to the node-agent server. Keys are flag names (without leading --), values are flag values. - These are applied additively on top of operator defaults. + These are merged additively on top of operator defaults. + If the unsupported-args annotation is set, it takes highest precedence + and completely overrides all other args including ExtraArgs. + Precedence: operator defaults < ExtraArgs < unsupported-args annotation. type: object fullMaintenanceInterval: description: |- @@ -1288,7 +1291,10 @@ spec: description: |- ExtraArgs are additional arguments to append to the Velero server. Keys are flag names (without leading --), values are flag values. - These are applied additively on top of operator defaults and Args. + These are merged additively on top of operator defaults and Args. + If the unsupported-args annotation is set, it takes highest precedence + and completely overrides all other args including ExtraArgs. + Precedence: operator defaults / Args < ExtraArgs < unsupported-args annotation. type: object featureFlags: description: featureFlags defines the list of features to enable for Velero instance diff --git a/internal/controller/velero.go b/internal/controller/velero.go index 9a1d55277e..b676bc50fa 100644 --- a/internal/controller/velero.go +++ b/internal/controller/velero.go @@ -9,7 +9,6 @@ import ( "strings" "github.com/go-logr/logr" - "github.com/google/go-cmp/cmp" "github.com/operator-framework/operator-lib/proxy" "github.com/sirupsen/logrus" "github.com/vmware-tanzu/velero/pkg/install" @@ -89,11 +88,7 @@ func (r *DataProtectionApplicationReconciler) ReconcileVeleroDeployment(log logr Namespace: dpa.Namespace, }, } - var orig *appsv1.Deployment // for debugging purposes op, err := controllerutil.CreateOrPatch(r.Context, r.Client, veleroDeployment, func() error { - if debugMode { - orig = veleroDeployment.DeepCopy() // for debugging purposes - } // Setting Deployment selector if a new object is created as it is immutable if veleroDeployment.ObjectMeta.CreationTimestamp.IsZero() { veleroDeployment.Spec.Selector = &metav1.LabelSelector{ @@ -110,8 +105,8 @@ func (r *DataProtectionApplicationReconciler) ReconcileVeleroDeployment(log logr // Setting controller owner reference on the velero deployment return controllerutil.SetControllerReference(dpa, veleroDeployment, r.Scheme) }) - if debugMode && op != controllerutil.OperationResultNone { // for debugging purposes - fmt.Printf("DEBUG: There was a diff which resulted in an operation on Velero Deployment: %s\n", cmp.Diff(orig, veleroDeployment)) + if debugMode && op != controllerutil.OperationResultNone { + log.Info("Velero Deployment reconciled with changes", "operation", op) } if err != nil { diff --git a/pkg/common/common.go b/pkg/common/common.go index 0851bfb9e9..b4dfe3d3ac 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -305,12 +305,18 @@ func ApplyUnsupportedServerArgsOverride(container *corev1.Container, unsupported // Handles both --flag=value and --flag value formats. // If a flag already exists, its value is replaced on the first occurrence and // subsequent duplicates are removed. New flags are appended in sorted order. +// Keys are normalized by stripping leading dashes; empty or whitespace-only keys are skipped. func MergeExtraArgs(args []string, extraArgs map[string]string) []string { if len(extraArgs) == 0 { return args } - replaced := make(map[string]bool, len(extraArgs)) + normalized := normalizeExtraArgKeys(extraArgs) + if len(normalized) == 0 { + return args + } + + replaced := make(map[string]bool, len(normalized)) var result []string skip := false @@ -321,7 +327,7 @@ func MergeExtraArgs(args []string, extraArgs map[string]string) []string { } flagName := extractFlagName(arg) - value, isExtra := extraArgs[flagName] + value, isExtra := normalized[flagName] if !isExtra { result = append(result, arg) continue @@ -347,7 +353,7 @@ func MergeExtraArgs(args []string, extraArgs map[string]string) []string { } var extra []string - for key, value := range extraArgs { + for key, value := range normalized { if !replaced[key] { extra = append(extra, fmt.Sprintf("--%s=%s", key, value)) } @@ -358,6 +364,22 @@ func MergeExtraArgs(args []string, extraArgs map[string]string) []string { return result } +// normalizeExtraArgKeys strips leading dashes from map keys and skips +// empty or whitespace-only keys. This ensures keys like "--log-level" +// are treated the same as "log-level". +func normalizeExtraArgKeys(extraArgs map[string]string) map[string]string { + normalized := make(map[string]string, len(extraArgs)) + for key, value := range extraArgs { + key = strings.TrimLeft(key, "-") + key = strings.TrimSpace(key) + if key == "" { + continue + } + normalized[key] = value + } + return normalized +} + // extractFlagName returns the flag name from an arg like --flag=value or --flag. // Returns empty string for non-flag args (e.g. "server"). func extractFlagName(arg string) string { diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 644cfb4e0b..499453882c 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -310,6 +310,8 @@ func TestGenerateCliArgsFromConfigMap(t *testing.T) { } } +// TestMergeExtraArgs validates the merge behavior of extra CLI args including +// replacement, deduplication, key normalization, and sorted appending. func TestMergeExtraArgs(t *testing.T) { tests := []struct { name string @@ -371,6 +373,18 @@ func TestMergeExtraArgs(t *testing.T) { extraArgs: map[string]string{"log-level": "debug"}, want: []string{"server", "--log-level", "debug"}, }, + { + name: "keys with leading dashes are normalized", + args: []string{"server", "--log-level=info"}, + extraArgs: map[string]string{"--log-level": "debug"}, + want: []string{"server", "--log-level=debug"}, + }, + { + name: "empty and whitespace-only keys are skipped", + args: []string{"server", "--log-level=info"}, + extraArgs: map[string]string{"": "val1", " ": "val2", "custom": "val3"}, + want: []string{"server", "--log-level=info", "--custom=val3"}, + }, } for _, tt := range tests {