RPK transmutes redpanda CLI args without equals sign

[travisdowns]

Version & Environment

Redpanda version: latest (rev 54f2f84)

What went wrong?

Failed to start redpanda when specifying --abort-on-seastar-bad-alloc on the command line.

What should have happened instead?

Redpanda should start with that CLI argument passed by rpk.

How to reproduce the issue?

1. Build redpanda and cd into the build dir (dir that contains bin directory)
2. rpk redpanda start --config ../../../rp-localhost.yaml --install-dir . --abort-on-seastar-bad-alloc
3. Error message is as follows:

libc++abi: terminating with uncaught exception of type boost::wrapexcept<boost::program_options::invalid_command_line_syntax>: option '--abort-on-seastar-bad-alloc' does not take any arguments
Aborted (core dumped)

Additional information

Evidently rpk transforms arguments of the form --foo into -foo=true, however this is not a valid transformation for all arguments, e.g., not for the one specified above. -foo= is also transmuted the same way. However, -foo= specified in the the config section rpk.additional_start_flags does work per @r-vasquez.

[twmb]

We also have a competing idea of completely removing rpk redpanda {start,stop} and potentially other things. Pinging @jcsp if we have an issue tracking that yet, but I don't think so.

[piyushredpanda]

@twmb: #4794 is the epic to remove rpk redpanda start from systemd flow, not completely removing the command, fyi

[travisdowns]

@twmb - ping the check if this is on deck to be fixed soonish. We have applied a workaround inside redpanda itself to strip the =true from certain args in a custom image delivered to affected parties, but we want retire this image and this would need to be fixed to do that.

Is the problem that adding =true is the correct behavior for some arguments and so we'd need an exhaustive list of arguments which need each behavior?

[twmb]

The problem is this logic:

redpanda/src/go/rpk/pkg/cli/cmd/redpanda/start.go

Lines 807 to 852 in b5a9328

	func parseFlags(flags []string) map[string]string {
	parsed := map[string]string{}
	for i := 0; i < len(flags); i++ {
	f := flags[i]
	isFlag := strings.HasPrefix(f, "-")
	trimmed := strings.Trim(f, " -")
	// Filter out elements that aren't flags or are empty.
	if !isFlag || trimmed == "" {
	continue
	}
	// Check if it's in name=value format
	// Split only into 2 tokens, since some flags can have multiple '='
	// in them, like --logger-log-level=archival=debug:cloud_storage=debug
	parts := strings.SplitN(trimmed, "=", 2)
	if len(parts) >= 2 {
	name := strings.Trim(parts[0], " ")
	value := strings.Trim(parts[1], " ")
	parsed[name] = value
	continue
	}
	// Otherwise, it can be a boolean flag (i.e. -v) or in
	// name<space>value format
	if i == len(flags)-1 {
	// We've reached the last element, so it's a single flag
	parsed[trimmed] = "true"
	continue
	}
	// Check if the next element starts with a hyphen
	// If it does, it's another flag, and the current element is a
	// boolean flag
	next := flags[i+1]
	if strings.HasPrefix(next, "-") {
	parsed[trimmed] = "true"
	continue
	}
	// Otherwise, the current element is the name of the flag and
	// the next one is its value
	parsed[trimmed] = next
	i += 1
	}
	return parsed

(particularly line 842 and 843)
assumes boolean flags must be passed to redpanda with the value of true. It looks as if every flag (key) must have a value.

The logic to fix this would probably be a bit deeper. The logic here:

redpanda/src/go/rpk/pkg/redpanda/launcher.go

Lines 92 to 107 in b5a9328

	for flag, value := range args.SeastarFlags {
	single := isSingle(flag)
	if single && value != "true" {
	// If it's a 'single'-type flag and it's set to false,
	// then there's no need to include it.
	continue
	}
	if single || value == "" {
	redpandaArgs = append(redpandaArgs, "--"+flag)
	continue
	}
	redpandaArgs = append(
	redpandaArgs,
	fmt.Sprintf("--%s=%s", flag, value),
	)
	}

adds key, value flags when invoking redpanda.

The change could probably be:

• instead of setting true, set to a blank string on line 843 in the first file
• in this second file, if the value is a blank string, just include the flag with no value

This entire bit could probably be a bit simpler, but the proposal in this comment would be a quick fix

cc @r-vasquez

[emaxerrno]

Note that is start is removed we need something to do sys checks and error out if not a good working environment

[r-vasquez]

@twmb

in this second file, if the value is a blank string, just include the flag with no value

Line 99 already does this so it's easier, pushing a fix in a min.