fix: Improve CLI parsing for Image Args

Resolves #827

Evaluate CLI args according to positional group.
<args_before_options><option_args><image_name><image_args>

Signed-off-by: Sam Chew <[email protected]>

cmd/finch/nerdctl.go

@@ -16,6 +16,7 @@ import (
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
+	orderedmap "github.com/wk8/go-ordered-map"
 
 	"github.com/runfinch/finch/pkg/command"
 	"github.com/runfinch/finch/pkg/config"
@@ -108,12 +109,11 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 		return err
 	}
 	var (
-		nerdctlArgs, envs, fileEnvs       []string
-		skip, hasCmdHander, hasArgHandler bool
-		cmdHandler                        commandHandler
-		aMap                              map[string]argHandler
-		envArgPos                         int
-		isDebug                           int
+		nerdctlArgs, envs, fileEnvs, imgArgs []string
+		skip, hasCmdHander, hasArgHandler    bool
+		cmdHandler                           commandHandler
+		aMap                                 map[string]argHandler
+		optArgPos, imgPos                    int
 	)
 
 	alias, hasAlias := aliasMap[cmdName]
@@ -142,10 +142,24 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 		}
 	}
 
-	// envArgPos is used to preserve the position of first environment parameter
-	envArgPos = -1
-	// if a debug flag is passed before env arg pos we reduce the env arg pos by 1 to account for skipping debug flag
-	isDebug = 0
+	// optArgPos is used to track when the "Option Set" has been found
+	optArgPos = -1
+	// imgPos is used to track when the "Image Name" has been found
+	imgPos = -1
+
+	shortFlagBoolSet := sets.NewString("-d", "-i", "-t", "-di", "-dt", "-it", "-dit")
+	longFlagBoolSet := sets.NewString("--detach", "--init", "--interactive", "--oom-kill-disable", "--privileged", "--read-only", "--rm", "--rootfs", "--tty")
+
+	shortFlagArgsSet := sets.NewString("-h", "-m", "-u", "-w", "-p", "-l", "-v")
+
+	for i, arg := range args {
+		if strings.HasPrefix(arg, "-") && optArgPos < 0 {
+			// First finch Option found
+			optArgPos = i
+			break
+		}
+	}
+
 	for i, arg := range args {
 		// Check if command requires arg handling
 		if hasArgHandler {
@@ -161,41 +175,32 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 				arg = args[i]
 			}
 		}
-		// parsing environment values from the command line may pre-fetch and
-		// consume the next argument; this loop variable will skip these pre-consumed
-		// entries from the command line
+		// parsing arguments from the command line
+		// may pre-fetch and consume the next argument;
+		// the loop variable will skip any pre-consumed args
 		if skip {
 			skip = false
 			continue
 		}
+
+		// after image position is found, pass through each arg as an image argument (short-circuit)
+		if imgPos > 0 {
+			imgArgs = append(imgArgs, arg)
+			continue
+		}
+
 		switch {
 		case arg == "--debug":
-			// explicitly setting log level to avoid `--debug` flag being interpreted as nerdctl command
-			if envArgPos == -1 {
-				isDebug = 1
-			}
 			nc.logger.SetLevel(flog.Debug)
-		case argIsEnv(arg):
-			if envArgPos == -1 {
-				envArgPos = i - isDebug
-			}
-			shouldSkip, addEnv := handleEnv(nc.systemDeps, arg, args[i+1])
-			skip = shouldSkip
-			if addEnv != "" {
-				envs = append(envs, addEnv)
-			}
-		case strings.HasPrefix(arg, "--env-file"):
-			if envArgPos == -1 {
-				envArgPos = i - isDebug
-			}
-
-			shouldSkip, addEnvs, err := handleEnvFile(nc.fs, nc.systemDeps, arg, args[i+1])
-			if err != nil {
-				return err
-			}
-			skip = shouldSkip
-			fileEnvs = append(fileEnvs, addEnvs...)
+		case arg == "--help":
+			nerdctlArgs = append(nerdctlArgs, arg)
+			break
+		case shortFlagBoolSet.Has(arg) || longFlagBoolSet.Has(arg):
+			// -d, -i, -t, -di, -dt, -dit
+			// --detach,--init,--interactive,--oom-kill-disable,--privileged,--read-only,--rm,--rootfs,--tty
+			nerdctlArgs = append(nerdctlArgs, arg)
 		case strings.HasPrefix(arg, "--add-host"):
+			// finch Option: Add Host
 			switch arg {
 			case "--add-host":
 				args[i+1], err = resolveIP(args[i+1], nc.logger, nc.ecc)
@@ -213,24 +218,71 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 			if err != nil {
 				return err
 			}
+		case strings.HasPrefix(arg, "--env-file"):
+			// finch Option: Env File
+			shouldSkip, addEnvs, err := handleEnvFile(nc.fs, nc.systemDeps, arg, args[i+1])
+			if err != nil {
+				return err
+			}
+			skip = shouldSkip
+			fileEnvs = append(fileEnvs, addEnvs...)
+		case strings.HasPrefix(arg, "-e") || strings.HasPrefix(arg, "--env"):
+			// -e, -e="<value>", -e"<value>"
+			// --env="<key>=<value>", --env "<key>=<value>"
+			shouldSkip, addEnv := handleEnv(nc.systemDeps, arg, args[i+1])
+			skip = shouldSkip
+			if addEnv != "" {
+				envs = append(envs, addEnv)
+			}
+		case shortFlagArgsSet.Has(arg) || shortFlagArgsSet.Has(arg[:2]):
+			// -h,-m,-u,-w,-p,-l,-v
+			shouldSkip, addKey, addVal := handleFlagArg(nc.systemDeps, arg, args[i+1])
+			skip = shouldSkip
+			if addKey != "" {
+				nerdctlArgs = append(nerdctlArgs, addKey)
+				nerdctlArgs = append(nerdctlArgs, addVal)
+			}
+		case strings.HasPrefix(arg, "--"):
+			// --<long_flag>="<value>", --<long_flag> "<value>"
+			shouldSkip, addKey, addVal := handleFlagArg(nc.systemDeps, arg, args[i+1])
+			skip = shouldSkip
+			if addKey != "" {
+				nerdctlArgs = append(nerdctlArgs, addKey)
+				nerdctlArgs = append(nerdctlArgs, addVal)
+			}
 		default:
-			nerdctlArgs = append(nerdctlArgs, arg)
+			// arg other than a flag ("-?","--<long_flag>") or a skipped <flag_value>
+			switch {
+			case (i < optArgPos):
+				// arg is a value prior to the first found flag
+				// pass the arg as a nerdctl command argument
+				nerdctlArgs = append(nerdctlArgs, arg)
+			case (i >= optArgPos) && (imgPos < 0):
+				// The first arg after procecssing the flags should be the Image Name
+				imgPos = i
+				imgArgs = append(imgArgs, arg)
+			default:
+				// Unexpected case
+				// pass the arg as a nerdctl arg by default
+				nc.logger.Debugln("Unexpected Arg Value: ", arg)
+				nerdctlArgs = append(nerdctlArgs, arg)
+			}
 		}
 	}
 
 	// to handle environment variables properly, we add all entries found via
 	// env-file includes to the map first and then all command line environment
 	// flags, making sure that command line overrides environment file options,
 	// and that later command line flags override earlier ones
-	envVars := make(map[string]string)
+	envVars := orderedmap.New()
 
 	for _, e := range fileEnvs {
 		evar, eval, _ := strings.Cut(e, "=")
-		envVars[evar] = eval
+		envVars.Set(evar, eval)
 	}
 	for _, e := range envs {
 		evar, eval, _ := strings.Cut(e, "=")
-		envVars[evar] = eval
+		envVars.Set(evar, eval)
 	}
 
 	passedEnvs := []string{
@@ -275,21 +327,21 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 	limaArgs = append(limaArgs, append([]string{nerdctlCmdName}, strings.Fields(cmdName)...)...)
 
 	var envArgs []string
-	for key, val := range envVars {
-		envArgs = append(envArgs, "-e", fmt.Sprintf("%s=%s", key, val))
-	}
-	if envArgPos > -1 {
-		nerdctlArgs = append(nerdctlArgs[:envArgPos], append(envArgs, nerdctlArgs[envArgPos:]...)...)
+	for pair := envVars.Oldest(); pair != nil; pair = pair.Next() {
+		envArgs = append(envArgs, "-e", fmt.Sprintf(`"%s=%s"`, pair.Key, pair.Value))
 	}
+
 	// Add -E to sudo command in order to preserve existing environment variables, more info:
 	// https://stackoverflow.com/questions/8633461/how-to-keep-environment-variables-when-using-sudo/8633575#8633575
-	limaArgs = append(limaArgs, nerdctlArgs...)
+	runArgs := append(limaArgs, nerdctlArgs...)
+	runArgs = append(runArgs, envArgs...)
+	runArgs = append(runArgs, imgArgs...)
 
 	if nc.shouldReplaceForHelp(cmdName, args) {
-		return nc.lcc.RunWithReplacingStdout([]command.Replacement{{Source: "nerdctl", Target: "finch"}}, limaArgs...)
+		return nc.lcc.RunWithReplacingStdout([]command.Replacement{{Source: "nerdctl", Target: "finch"}}, runArgs...)
 	}
 
-	return nc.lcc.Create(limaArgs...).Run()
+	return nc.lcc.Create(runArgs...).Run()
 }
 
 func (nc *nerdctlCommand) assertVMIsRunning(creator command.LimaCmdCreator, logger flog.Logger) error {
@@ -342,6 +394,47 @@ func argIsEnv(arg string) bool {
 	return false
 }
 
+func handleFlagArg(systemDeps NerdctlCommandSystemDeps, arg string, nextArg string) (bool, string, string) {
+	// handling Flag arguements other than environment variables
+	var (
+		flagKey, flagVal string
+		skip             bool
+	)
+	switch {
+	case strings.HasPrefix(arg, "--") && strings.Contains(arg, "="):
+		// long flag concatenated to value by '=': --<long_flag>="<value>"
+		eqPos := strings.Index(arg, "=")
+		skip = false
+		flagKey = arg[:eqPos]
+		flagVal = arg[eqPos+1:]
+	case strings.HasPrefix(arg, "--") && !strings.HasPrefix(nextArg, "-"):
+		// long flag followed by a value: --<long_flag> "<value>"
+		skip = true
+		flagKey = arg
+		flagVal = nextArg
+	case strings.HasPrefix(arg, "-") && strings.Contains(arg, "="):
+		// short flag concatenated to value by '=': -?="<value>"
+		eqPos := strings.Index(arg, "=")
+		skip = false
+		flagKey = arg[:eqPos]
+		flagVal = arg[eqPos+1:]
+	case strings.HasPrefix(arg, "-") && len(arg) > 2:
+		// short flag adjacent to value: -?"<value>"
+		skip = false
+		flagKey = arg[:2]
+		flagVal = arg[2:]
+	case strings.HasPrefix(arg, "-") && len(arg) == 2:
+		// short flag followed by a value: -? "<value>"
+		skip = true
+		flagKey = arg
+		flagVal = nextArg
+	default:
+		return false, "", ""
+	}
+
+	return skip, flagKey, fmt.Sprintf("\"%s\"", flagVal)
+}
+
 func handleEnv(systemDeps NerdctlCommandSystemDeps, arg, arg2 string) (bool, string) {
 	var (
 		envVar string
@@ -357,7 +450,8 @@ func handleEnv(systemDeps NerdctlCommandSystemDeps, arg, arg2 string) (bool, str
 			envVar = arg[2:]
 		} else {
 			// only other case is "--env="; skip that prefix
-			envVar = arg[6:]
+			eqPos := strings.Index(arg, "=")
+			envVar = arg[eqPos+1:]
 		}
 	}