fix: parse management commands with proper arguments (#876)

Issue #, if available: #827 Description of changes: Addresses #827 by adding all environment variables at the location of first occurrence of the environment flag Testing done: Changes tested locally to ensure that the issue is fixed. $ ./_output/bin/finch container run --debug -it --rm --env="E=v" busybox env DEBU[0000] Creating limactl command: ARGUMENTS: [shell finch sudo -E nerdctl container run -e E=v -it --rm busybox env], LIMA_HOME: /Users/mharwani/work/runfinch/finch/_output/lima/data PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin E=v TERM=xterm HOME=/root *Testing done:* - [ ] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Shubharanshu Mahapatra <shubhum@amazon.com>
runfinch · Mar 28, 2024 · e2f42fe · e2f42fe
1 parent 9223219
commit e2f42fe
Show file tree

Hide file tree

Showing 3 changed files with 176 additions and 12 deletions.
diff --git a/cmd/finch/nerdctl.go b/cmd/finch/nerdctl.go
@@ -111,6 +111,8 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 		skip, hasCmdHander, hasArgHandler bool
 		cmdHandler                        commandHandler
 		aMap                              map[string]argHandler
+		envArgPos                         int
+		isDebug                           int
 	)
 
 	alias, hasAlias := aliasMap[cmdName]
@@ -139,6 +141,10 @@ 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
 	for i, arg := range args {
 		// Check if command requires arg handling
 		if hasArgHandler {
@@ -164,14 +170,24 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 		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
@@ -249,14 +265,16 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 
 	limaArgs = append(limaArgs, append([]string{nerdctlCmdName}, strings.Fields(cmdName)...)...)
 
-	var finalArgs []string
+	var envArgs []string
 	for key, val := range envVars {
-		finalArgs = append(finalArgs, "-e", fmt.Sprintf("%s=%s", key, val))
+		envArgs = append(envArgs, "-e", fmt.Sprintf("%s=%s", key, val))
+	}
+	if envArgPos > -1 {
+		nerdctlArgs = append(nerdctlArgs[:envArgPos], append(envArgs, nerdctlArgs[envArgPos:]...)...)
 	}
-	finalArgs = append(finalArgs, nerdctlArgs...)
 	// 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, finalArgs...)
+	limaArgs = append(limaArgs, nerdctlArgs...)
 
 	if nc.shouldReplaceForHelp(cmdName, args) {
 		return nc.lcc.RunWithReplacingStdout([]command.Replacement{{Source: "nerdctl", Target: "finch"}}, limaArgs...)

diff --git a/cmd/finch/nerdctl_darwin_test.go b/cmd/finch/nerdctl_darwin_test.go
@@ -176,7 +176,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
 
 				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
-					"-e", "ARG1=val1", "--rm", "alpine:latest", "env").Return(c)
+					"--rm", "-e", "ARG1=val1", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -207,7 +207,39 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("ARG3").Return("val3", true)
 				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
 				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
-					"-e", "ARG3=val3", "--rm", "alpine:latest", "env").Return(c)
+					"--rm", "-e", "ARG3=val3", "alpine:latest", "env").Return(c)
+				c.EXPECT().Run()
+			},
+		},
+		{
+			name:    "with environment flags parsing and env value exists and with --debug flag",
+			cmdName: "run",
+			fc:      &config.Finch{},
+			args:    []string{"--debug", "--rm", "--env=ARG2", "-eARG3", "alpine:latest", "env"},
+			wantErr: nil,
+			mockSvc: func(
+				_ *testing.T,
+				lcc *mocks.LimaCmdCreator,
+				_ *mocks.CommandCreator,
+				ncsd *mocks.NerdctlCommandSystemDeps,
+				logger *mocks.Logger,
+				ctrl *gomock.Controller,
+				_ afero.Fs,
+			) {
+				getVMStatusC := mocks.NewCommand(ctrl)
+				lcc.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
+				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
+				logger.EXPECT().SetLevel(flog.Debug)
+				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
+				ncsd.EXPECT().LookupEnv("AWS_ACCESS_KEY_ID").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SECRET_ACCESS_KEY").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SESSION_TOKEN").Return("", false)
+				c := mocks.NewCommand(ctrl)
+				ncsd.EXPECT().LookupEnv("ARG2")
+				ncsd.EXPECT().LookupEnv("ARG3").Return("val3", true)
+				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
+					"--rm", "-e", "ARG3=val3", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -241,7 +273,43 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("NOTSETARG")
 				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
 				lcc.EXPECT().
-					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "-e", "ARG1=val1", "--rm", "alpine:latest", "env").
+					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "--rm", "-e", "ARG1=val1", "alpine:latest", "env").
+					Return(c)
+				c.EXPECT().Run()
+			},
+		},
+		{
+			name:    "with --env-file flag replacement and with --debug flag",
+			cmdName: "run",
+			fc:      &config.Finch{},
+			args:    []string{"--debug", "--rm", "--env-file=/env-file", "alpine:latest", "env"},
+			wantErr: nil,
+			mockSvc: func(
+				t *testing.T,
+				lcc *mocks.LimaCmdCreator,
+				_ *mocks.CommandCreator,
+				ncsd *mocks.NerdctlCommandSystemDeps,
+				logger *mocks.Logger,
+				ctrl *gomock.Controller,
+				fs afero.Fs,
+			) {
+				envFileStr := "# a comment\nARG1=val1\n  ARG2\n\n  # a 2nd comment\nNOTSETARG\n  "
+				require.NoError(t, afero.WriteFile(fs, envFilePath, []byte(envFileStr), 0o600))
+
+				getVMStatusC := mocks.NewCommand(ctrl)
+				lcc.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
+				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
+				logger.EXPECT().SetLevel(flog.Debug)
+				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
+				ncsd.EXPECT().LookupEnv("AWS_ACCESS_KEY_ID").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SECRET_ACCESS_KEY").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SESSION_TOKEN").Return("", false)
+				c := mocks.NewCommand(ctrl)
+				ncsd.EXPECT().LookupEnv("ARG2")
+				ncsd.EXPECT().LookupEnv("NOTSETARG")
+				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
+				lcc.EXPECT().
+					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "--rm", "-e", "ARG1=val1", "alpine:latest", "env").
 					Return(c)
 				c.EXPECT().Run()
 			},
@@ -276,7 +344,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("NOTSETARG")
 				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
 				lcc.EXPECT().
-					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "-e", "ARG2=val2", "--rm", "alpine:latest", "env").
+					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "--rm", "-e", "ARG2=val2", "alpine:latest", "env").
 					Return(c)
 				c.EXPECT().Run()
 			},

diff --git a/cmd/finch/nerdctl_windows_test.go b/cmd/finch/nerdctl_windows_test.go
@@ -202,7 +202,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
 
 				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName, "sudo", "-E", nerdctlCmdName, "container", "run",
-					"-e", "ARG1=val1", "--rm", "alpine:latest", "env").Return(c)
+					"--rm", "-e", "ARG1=val1", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -239,7 +239,45 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
 				// alias substitution run=>container run
 				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName, "sudo", "-E", nerdctlCmdName, "container", "run",
-					"-e", "ARG3=val3", "--rm", "alpine:latest", "env").Return(c)
+					"--rm", "-e", "ARG3=val3", "alpine:latest", "env").Return(c)
+				c.EXPECT().Run()
+			},
+		},
+		{
+			name:    "with environment flags parsing and env value exists and with --debug flag",
+			cmdName: "run",
+			fc:      &config.Finch{},
+			args:    []string{"--debug", "--rm", "--env=ARG2", "-eARG3", "alpine:latest", "env"},
+			wantErr: nil,
+			mockSvc: func(
+				_ *testing.T,
+				_ *mocks.CommandCreator,
+				lcc *mocks.LimaCmdCreator,
+				_ *mocks.Command,
+				ncsd *mocks.NerdctlCommandSystemDeps,
+				logger *mocks.Logger,
+				ctrl *gomock.Controller,
+				_ afero.Fs,
+			) {
+				getVMStatusC := mocks.NewCommand(ctrl)
+				lcc.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
+				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
+				logger.EXPECT().SetLevel(flog.Debug)
+				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
+				ncsd.EXPECT().LookupEnv("AWS_ACCESS_KEY_ID").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SECRET_ACCESS_KEY").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SESSION_TOKEN").Return("", false)
+				c := mocks.NewCommand(ctrl)
+				ncsd.EXPECT().LookupEnv("ARG2")
+				ncsd.EXPECT().LookupEnv("ARG3").Return("val3", true)
+				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
+				ncsd.EXPECT().GetWd().Return("C:\\workdir", nil)
+				ncsd.EXPECT().FilePathAbs("C:\\workdir").Return("C:\\workdir", nil)
+				ncsd.EXPECT().FilePathJoin(string(filepath.Separator), "mnt", "c", "workdir").Return(augmentedPath)
+				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
+				// alias substitution run=>container run
+				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName, "sudo", "-E", nerdctlCmdName, "container", "run",
+					"--rm", "-e", "ARG3=val3", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -278,7 +316,47 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().FilePathJoin(string(filepath.Separator), "mnt", "c", "workdir").Return(augmentedPath)
 				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
 				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName,
-					"sudo", "-E", nerdctlCmdName, "container", "run", "-e", "ARG1=val1", "--rm", "alpine:latest", "env").Return(c)
+					"sudo", "-E", nerdctlCmdName, "container", "run", "--rm", "-e", "ARG1=val1", "alpine:latest", "env").Return(c)
+				c.EXPECT().Run()
+			},
+		},
+		{
+			name:    "with --env-file flag replacement and with --debug flag",
+			cmdName: "run",
+			fc:      &config.Finch{},
+			args:    []string{"--debug", "--rm", "--env-file=" + envFilePath, "alpine:latest", "env"},
+			wantErr: nil,
+			mockSvc: func(
+				t *testing.T,
+				_ *mocks.CommandCreator,
+				lcc *mocks.LimaCmdCreator,
+				_ *mocks.Command,
+				ncsd *mocks.NerdctlCommandSystemDeps,
+				logger *mocks.Logger,
+				ctrl *gomock.Controller,
+				fs afero.Fs,
+			) {
+				envFileStr := "# a comment\nARG1=val1\n  ARG2\n\n  # a 2nd comment\nNOTSETARG\n  "
+				require.NoError(t, afero.WriteFile(fs, envFilePath, []byte(envFileStr), 0o600))
+
+				getVMStatusC := mocks.NewCommand(ctrl)
+				lcc.EXPECT().CreateWithoutStdio("ls", "-f", "{{.Status}}", limaInstanceName).Return(getVMStatusC)
+				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
+				logger.EXPECT().SetLevel(flog.Debug)
+				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
+				ncsd.EXPECT().LookupEnv("AWS_ACCESS_KEY_ID").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SECRET_ACCESS_KEY").Return("", false)
+				ncsd.EXPECT().LookupEnv("AWS_SESSION_TOKEN").Return("", false)
+				c := mocks.NewCommand(ctrl)
+				ncsd.EXPECT().LookupEnv("ARG2")
+				ncsd.EXPECT().LookupEnv("NOTSETARG")
+				ncsd.EXPECT().LookupEnv("COSIGN_PASSWORD").Return("", false)
+				ncsd.EXPECT().GetWd().Return("C:\\workdir", nil)
+				ncsd.EXPECT().FilePathAbs("C:\\workdir").Return("C:\\workdir", nil)
+				ncsd.EXPECT().FilePathJoin(string(filepath.Separator), "mnt", "c", "workdir").Return(augmentedPath)
+				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
+				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName,
+					"sudo", "-E", nerdctlCmdName, "container", "run", "--rm", "-e", "ARG1=val1", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -317,7 +395,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().FilePathJoin(string(filepath.Separator), "mnt", "c", "workdir").Return(augmentedPath)
 				ncsd.EXPECT().FilePathToSlash(augmentedPath).Return(wslPath)
 				lcc.EXPECT().Create("shell", "--workdir", wslPath, limaInstanceName,
-					"sudo", "-E", nerdctlCmdName, "container", "run", "-e", "ARG2=val2", "--rm", "alpine:latest", "env").Return(c)
+					"sudo", "-E", nerdctlCmdName, "container", "run", "--rm", "-e", "ARG2=val2", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
 		},