Repository: scylladb/scylla-operator (go)

Issue:
`scyllaArgs` not accepting flags without values
`scyllaArgs` expects every `--flag` to have a value and mangles flags without values by adding an empty value to them.

This does not work with `--poll-mode` for which ScyllaDB expects no value, but Operator appends an empty string `""` value, causing ScyllaDB to fail to start up.

https://github.com/scylladb/scylla-operator/blob/master/pkg/sidecar/config/config.go#L191-L199

## Acceptance Criteria

- [ ] `--poll-mode` works if passed in `scyllaArgs`

Pull request description:
Omit empty value for switch parameters
**Description of your changes:**

When scyllaArgs contains switch parameter having no value, flag should only contain it's name without empty string as a value. 
This fixes issue with setting for example `--poll-mode` parameter in ScyllaDB.

Before change:
```
I0623 19:54:01.943747       1 config/config.go:265] "Scylla entrypoint" Command="/docker-entrypoint.py --listen-address=0.0.0.0 --seeds=10.107.87.173 --smp=1 --broadcast-address=10.107.87.173 --cpuset=0-3 --developer-mode=1 --poll-mode= --overprovisioned=1 --prometheus-address=0.0.0.0 --broadcast-rpc-address=10.107.87.173"
[...]
terminate called after throwing an instance of 'boost::wrapexcept<boost::program_options::invalid_command_line_syntax>'
what():  the argument for option '--poll-mode' should follow immediately after the equal sign
2025-06-23 19:54:13,113 WARN exited: scylla (terminated by SIGABRT (core dumped); not expected)
```
After change:
```
I0623 20:37:02.952290       1 config/config.go:269] "Scylla entrypoint" Command="/docker-entrypoint.py --smp=1 --prometheus-address=0.0.0.0 --cpuset=0-3 --broadcast-rpc-address=10.107.87.173 --poll-mode --developer-mode=1 --listen-address=0.0.0.0 --seeds=10.107.87.173 --overprovisioned=1 --broadcast-address=10.107.87.173"
```
And ScyllaDB starts just fine.

**Which issue is resolved by this Pull Request:**
Resolves #2706


Patch:
diff --git a/pkg/sidecar/config/config.go b/pkg/sidecar/config/config.go
index a5f8b0275..07c6de9c7 100644
--- a/pkg/sidecar/config/config.go
+++ b/pkg/sidecar/config/config.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"maps"
 	"os"
 	"os/exec"
 	"path"
@@ -174,28 +175,30 @@ func mergeSnitchConfigProperties(userConfig, operatorConfig *properties.Properti
 
 var scyllaArgumentsRegexp = regexp.MustCompile(`--([^= ]+)(="[^"]+"|=\S+|\s+"[^"]+"|\s+[^\s-]+|\s+-?\d*\.?\d+[^\s-]+|)`)
 
-func convertScyllaArguments(scyllaArguments string) map[string]string {
-	output := make(map[string]string)
+func parseScyllaArguments(scyllaArguments string) map[string]*string {
+	output := make(map[string]*string)
 	for _, value := range scyllaArgumentsRegexp.FindAllStringSubmatch(scyllaArguments, -1) {
 		if value[2] == "" {
-			output[value[1]] = ""
-		} else if value[2][0] == '=' {
-			output[value[1]] = strings.TrimSpace(value[2][1:])
+			output[value[1]] = nil
 		} else {
-			output[value[1]] = strings.TrimSpace(value[2])
+			output[value[1]] = pointer.Ptr(strings.TrimSpace(strings.TrimPrefix(value[2], "=")))
 		}
 	}
 	return output
 }
 
-func appendScyllaArguments(ctx context.Context, s *ScyllaConfig, scyllaArgs string, scyllaFinalArgs map[string]*string) {
-	for argName, argValue := range convertScyllaArguments(scyllaArgs) {
-		if existing := scyllaFinalArgs[argName]; existing == nil {
-			scyllaFinalArgs[argName] = pointer.Ptr(strings.TrimSpace(argValue))
+// mergeArguments merges two sets of arguments.
+// Arguments managed by the operator take precedence and cannot be overridden by user-provided arguments.
+func mergeArguments(operatorManagedArguments, userProvidedArguments map[string]*string) map[string]*string {
+	finalArguments := maps.Clone(operatorManagedArguments)
+	for argName, argValue := range userProvidedArguments {
+		if _, ok := operatorManagedArguments[argName]; !ok {
+			finalArguments[argName] = argValue
 		} else {
-			klog.Infof("ScyllaArgs: argument '%s' is ignored, it is already in the list", argName)
+			klog.Infof("ScyllaArgs: argument '%s' is ignored, it's managed by Operator", argName)
 		}
 	}
+	return finalArguments
 }
 
 func (s *ScyllaConfig) setupEntrypoint(ctx context.Context) (*exec.Cmd, error) {
@@ -241,7 +244,7 @@ func (s *ScyllaConfig) setupEntrypoint(ctx context.Context) (*exec.Cmd, error) {
 	}
 
 	if len(m.AdditionalScyllaDBArguments) > 0 {
-		appendScyllaArguments(ctx, s, strings.Join(m.AdditionalScyllaDBArguments, " "), args)
+		args = mergeArguments(parseScyllaArguments(strings.Join(m.AdditionalScyllaDBArguments, " ")), args)
 	}
 
 	if _, err := os.Stat(scyllaIOPropertiesPath); err == nil {


Test patch:
diff --git a/pkg/sidecar/config/config_test.go b/pkg/sidecar/config/config_test.go
index 5fd112b27..b2486c33e 100644
--- a/pkg/sidecar/config/config_test.go
+++ b/pkg/sidecar/config/config_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/magiconair/properties"
+	"github.com/scylladb/scylla-operator/pkg/pointer"
 	"k8s.io/apimachinery/pkg/api/equality"
 )
 
@@ -273,96 +274,101 @@ func TestAllowedCPUs(t *testing.T) {
 	t.Log(cpusAllowed)
 }
 
-func TestScyllaArguments(t *testing.T) {
+func TestParseScyllaArguments(t *testing.T) {
 	ts := []struct {
 		Name         string
 		Args         string
-		ExpectedArgs map[string]string
+		ExpectedArgs map[string]*string
 	}{
 		{
 			Name: "all combinations in one",
-			Args: `--arg1 --arg2=val2 --arg3 "val3" --arg4="val4" --arg5=-1.23 --arg6 -123 --arg7 123`,
-			ExpectedArgs: map[string]string{
-				"arg1": "",
-				"arg2": "val2",
-				"arg3": `"val3"`,
-				"arg4": `"val4"`,
-				"arg5": "-1.23",
-				"arg6": "-123",
-				"arg7": "123",
+			Args: `--arg1 --arg2=val2 --arg3 "val3" --arg4="val4" --arg5=-1.23 --arg6 -123 --arg7 123 --arg8="" --arg9 ""`,
+			ExpectedArgs: map[string]*string{
+				"arg1": nil,
+				"arg2": pointer.Ptr("val2"),
+				"arg3": pointer.Ptr(`"val3"`),
+				"arg4": pointer.Ptr(`"val4"`),
+				"arg5": pointer.Ptr("-1.23"),
+				"arg6": pointer.Ptr("-123"),
+				"arg7": pointer.Ptr("123"),
+				"arg8": pointer.Ptr(`""`),
+				"arg9": pointer.Ptr(`""`),
 			},
 		},
 		{
 			Name:         "single empty flag",
 			Args:         "--arg",
-			ExpectedArgs: map[string]string{"arg": ""},
+			ExpectedArgs: map[string]*string{"arg": nil},
 		},
 		{
 			Name:         "single flag",
 			Args:         "--arg val",
-			ExpectedArgs: map[string]string{"arg": "val"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("val")},
 		},
 		{
 			Name:         "integer value",
 			Args:         "--arg 123",
-			ExpectedArgs: map[string]string{"arg": "123"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("123")},
 		},
 		{
 			Name:         "negative integer value",
 			Args:         "--arg -123",
-			ExpectedArgs: map[string]string{"arg": "-123"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("-123")},
 		},
 		{
 			Name:         "float value",
 			Args:         "--arg 1.23",
-			ExpectedArgs: map[string]string{"arg": "1.23"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("1.23")},
 		},
 		{
 			Name:         "negative float value",
 			Args:         "--arg -1.23",
-			ExpectedArgs: map[string]string{"arg": "-1.23"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("-1.23")},
 		},
 		{
-			Name:         "negative float value and string parameter",
-			Args:         "--arg1 -1.23 --arg2 val",
-			ExpectedArgs: map[string]string{"arg1": "-1.23", "arg2": "val"},
+			Name: "negative float value and string parameter",
+			Args: "--arg1 -1.23 --arg2 val",
+			ExpectedArgs: map[string]*string{
+				"arg1": pointer.Ptr("-1.23"),
+				"arg2": pointer.Ptr("val"),
+			},
 		},
 		{
 			Name:         "bool value",
 			Args:         "--arg true",
-			ExpectedArgs: map[string]string{"arg": "true"},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr("true")},
 		},
 		{
 			Name:         "flag quoted",
 			Args:         `--arg "val"`,
-			ExpectedArgs: map[string]string{"arg": `"val"`},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr(`"val"`)},
 		},
 		{
 			Name:         "flag quoted with equal sign",
 			Args:         `--arg="val"`,
-			ExpectedArgs: map[string]string{"arg": `"val"`},
+			ExpectedArgs: map[string]*string{"arg": pointer.Ptr(`"val"`)},
 		},
 		{
 			Name:         "empty",
 			Args:         "",
-			ExpectedArgs: map[string]string{},
+			ExpectedArgs: map[string]*string{},
 		},
 		{
 			Name: "garbage inside",
 			Args: "--arg1 val asdasdasdas --arg2=val",
-			ExpectedArgs: map[string]string{
-				"arg1": "val",
-				"arg2": "val",
+			ExpectedArgs: map[string]*string{
+				"arg1": pointer.Ptr("val"),
+				"arg2": pointer.Ptr("val"),
 			},
 		},
 		{
 			Name: "mixed types",
 			Args: "--skip-wait-for-gossip-to-settle 0 --ring-delay-ms 5000 --compaction-enforce-min-threshold true --shadow-round-ms 1",
-			ExpectedArgs: map[string]string{
-				"skip-wait-for-gossip-to-settle":   "0",
-				"ring-delay-ms":                    "5000",
-				"compaction-enforce-min-threshold": "true",
-				"shadow-round-ms":                  "1",
+			ExpectedArgs: map[string]*string{
+				"skip-wait-for-gossip-to-settle":   pointer.Ptr("0"),
+				"ring-delay-ms":                    pointer.Ptr("5000"),
+				"compaction-enforce-min-threshold": pointer.Ptr("true"),
+				"shadow-round-ms":                  pointer.Ptr("1"),
 			},
 		},
 	}
@@ -371,10 +377,57 @@ func TestScyllaArguments(t *testing.T) {
 		t.Run(test.Name, func(t *testing.T) {
 			t.Parallel()
 
-			argumentsMap := convertScyllaArguments(test.Args)
+			argumentsMap := parseScyllaArguments(test.Args)
 			if !reflect.DeepEqual(test.ExpectedArgs, argumentsMap) {
 				t.Errorf("expected %+v, got %+v", test.ExpectedArgs, argumentsMap)
 			}
 		})
 	}
 }
+
+func TestMergeArguments(t *testing.T) {
+	t.Parallel()
+
+	ts := []struct {
+		name                     string
+		operatorManagedArguments map[string]*string
+		userProvidedArguments    map[string]*string
+		expectedArguments        map[string]*string
+	}{
+		{
+			name: "no conflicts",
+			operatorManagedArguments: map[string]*string{
+				"foo": pointer.Ptr("bar"),
+			},
+			userProvidedArguments: map[string]*string{
+				"bar": pointer.Ptr("foo"),
+			},
+			expectedArguments: map[string]*string{
+				"foo": pointer.Ptr("bar"),
+				"bar": pointer.Ptr("foo"),
+			},
+		},
+		{
+			name: "operator wins on conflicts",
+			operatorManagedArguments: map[string]*string{
+				"foo": pointer.Ptr("bar"),
+			},
+			userProvidedArguments: map[string]*string{
+				"foo": pointer.Ptr("buzz"),
+			},
+			expectedArguments: map[string]*string{
+				"foo": pointer.Ptr("bar"),
+			},
+		},
+	}
+	for _, test := range ts {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			gotArguments := mergeArguments(test.operatorManagedArguments, test.userProvidedArguments)
+			if !reflect.DeepEqual(test.expectedArguments, gotArguments) {
+				t.Errorf("expected %+v, got %+v", test.expectedArguments, gotArguments)
+			}
+		})
+	}
+}