fix: Correctly set and pick up environment variables

cmd/finch/main.go

@@ -111,13 +111,12 @@ func virtualMachineCommands(
 	fc *config.Finch,
 ) *cobra.Command {
 	optionalDepGroups := []*dependency.Group{vmnet.NewDependencyGroup(ecc, lcc, fs, fp, logger)}
-
 	return newVirtualMachineCommand(
 		lcc,
 		logger,
 		optionalDepGroups,
 		config.NewLimaApplier(fc, ecc, fs, fp.LimaOverrideConfigPath(), system.NewStdLib()),
-		config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath()),
+		config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath(), system.NewStdLib().Env("USER")),
 		fp,
 		fs,
 		disk.NewUserDataDiskManager(lcc, ecc, &afero.OsFs{}, fp, system.NewStdLib().Env("HOME"), fc),

cmd/finch/nerdctl.go

@@ -145,8 +145,9 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 		finalArgs = append(finalArgs, "-e", fmt.Sprintf("%s=%s", key, val))
 	}
 	finalArgs = append(finalArgs, nerdctlArgs...)
-
-	limaArgs := append([]string{"shell", limaInstanceName, "sudo", nerdctlCmdName, cmdName}, finalArgs...)
+	// 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([]string{"shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, cmdName}, finalArgs...)
 
 	if nc.shouldReplaceForHelp(cmdName, args) {
 		return nc.creator.RunWithReplacingStdout([]command.Replacement{{Source: "nerdctl", Target: "finch"}}, limaArgs...)

cmd/finch/nerdctl_test.go

@@ -54,7 +54,7 @@ func TestNerdctlCommand_runAdaptor(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "info").Return(c)
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "info").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -104,7 +104,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "build", "-t", "demo", ".").Return(c)
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "build", "-t", "demo", ".").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -205,7 +205,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				logger.EXPECT().SetLevel(flog.Debug)
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "pull", "test:tag").Return(c)
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "pull", "test:tag").Return(c)
 				c.EXPECT().Run()
 			},
 		},
@@ -229,7 +229,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				c := mocks.NewCommand(ctrl)
 				ncsd.EXPECT().LookupEnv("ARG2")
 				ncsd.EXPECT().LookupEnv("ARG3")
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"-e", "ARG1=val1", "--rm", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
@@ -254,7 +254,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				c := mocks.NewCommand(ctrl)
 				ncsd.EXPECT().LookupEnv("ARG2")
 				ncsd.EXPECT().LookupEnv("ARG3").Return("val3", true)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"-e", "ARG3=val3", "--rm", "alpine:latest", "env").Return(c)
 				c.EXPECT().Run()
 			},
@@ -283,7 +283,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("ARG2")
 				ncsd.EXPECT().LookupEnv("NOTSETARG")
 				lcc.EXPECT().
-					Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run", "-e", "ARG1=val1", "--rm", "alpine:latest", "env").
+					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "-e", "ARG1=val1", "--rm", "alpine:latest", "env").
 					Return(c)
 				c.EXPECT().Run()
 			},
@@ -312,7 +312,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				ncsd.EXPECT().LookupEnv("ARG2").Return("val2", true)
 				ncsd.EXPECT().LookupEnv("NOTSETARG")
 				lcc.EXPECT().
-					Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run", "-e", "ARG2=val2", "--rm", "alpine:latest", "env").
+					Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run", "-e", "ARG2=val2", "--rm", "alpine:latest", "env").
 					Return(c)
 				c.EXPECT().Run()
 			},
@@ -355,7 +355,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				logger.EXPECT().Debugf(`Resolving special IP "host-gateway" to %q for host %q`, "192.168.5.2", "name")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"--rm", "--add-host", "name:192.168.5.2", "alpine:latest").Return(c)
 				c.EXPECT().Run()
 			},
@@ -378,7 +378,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"--rm", "--add-host", "name:0.0.0.0", "alpine:latest").Return(c)
 				c.EXPECT().Run()
 			},
@@ -401,7 +401,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"--rm", "--add-host", "alpine:latest").Return(c)
 				c.EXPECT().Run().Return(errors.New("run cmd error"))
 			},
@@ -425,7 +425,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				logger.EXPECT().Debugf(`Resolving special IP "host-gateway" to %q for host %q`, "192.168.5.2", "name")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"--rm", "--add-host=name:192.168.5.2", "alpine:latest").Return(c)
 				c.EXPECT().Run()
 			},
@@ -448,7 +448,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				c := mocks.NewCommand(ctrl)
-				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", nerdctlCmdName, "run",
+				lcc.EXPECT().Create("shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "run",
 					"--rm", "--add-host=name:0.0.0.0", "alpine:latest").Return(c)
 				c.EXPECT().Run()
 			},
@@ -471,7 +471,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				lcc.EXPECT().RunWithReplacingStdout(
-					testStdoutRs, "shell", limaInstanceName, "sudo", nerdctlCmdName, "pull", "test:tag", "--help").Return(nil)
+					testStdoutRs, "shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "pull", "test:tag", "--help").Return(nil)
 			},
 		},
 		{
@@ -492,7 +492,7 @@ func TestNerdctlCommand_run(t *testing.T) {
 				getVMStatusC.EXPECT().Output().Return([]byte("Running"), nil)
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 				lcc.EXPECT().RunWithReplacingStdout(
-					testStdoutRs, "shell", limaInstanceName, "sudo", nerdctlCmdName, "pull", "test:tag", "--help").
+					testStdoutRs, "shell", limaInstanceName, "sudo", "-E", nerdctlCmdName, "pull", "test:tag", "--help").
 					Return(fmt.Errorf("failed to replace"))
 			},
 		},

cmd/finch/version.go

@@ -88,8 +88,9 @@ func (va *versionAction) printVersion() error {
 	if status != lima.Running {
 		return errors.New("detailed version info is unavailable because VM is not running")
 	}
-
-	limaArgs := []string{"shell", limaInstanceName, "sudo", "nerdctl", "version", "--format", "json"}
+	// 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 := []string{"shell", limaInstanceName, "sudo", "-E", "nerdctl", "version", "--format", "json"}
 	out, err := va.creator.CreateWithoutStdio(limaArgs...).Output()
 	if err != nil {
 		return fmt.Errorf("failed to create the nerdctl version command: %w", err)

cmd/finch/version_test.go

@@ -52,7 +52,8 @@ func TestVersionAction_runAdaptor(t *testing.T) {
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 
 				command := mocks.NewCommand(ctrl)
-				lcc.EXPECT().CreateWithoutStdio("shell", limaInstanceName, "sudo", "nerdctl", "version", "--format", "json").Return(command)
+				lcc.EXPECT().CreateWithoutStdio("shell", limaInstanceName, "sudo", "-E", "nerdctl", "version",
+					"--format", "json").Return(command)
 				//nolint: lll	// Version output format is larger than 500
 				command.EXPECT().Output().Return([]byte(`{"Client":{"Version":"v1.0.0","GitCommit":"c00780a1f5b905b09812722459c54936c9e070e6","GoVersion":"go1.19.2","Os":"linux","Arch":"arm64","Components":[{"Name":"buildctl","Version":"v0.10.5","Details":{"GitCommit":"bc26045116045516ff2427201abd299043eaf8f7"}}]},"Server":{"Components":[{"Name":"containerd","Version":"v1.6.8","Details":{"GitCommit":"9cd3357b7fd7218e4aec3eae239db1f68a5a6ec6"}},{"Name":"runc","Version":"1.1.4","Details":{"GitCommit":"v1.1.4-0-g5fd4c4d1"}}]}}`), nil)
 			},
@@ -98,7 +99,8 @@ func TestVersionAction_run(t *testing.T) {
 				logger.EXPECT().Debugf("Status of virtual machine: %s", "Running")
 
 				command := mocks.NewCommand(ctrl)
-				lcc.EXPECT().CreateWithoutStdio("shell", limaInstanceName, "sudo", "nerdctl", "version", "--format", "json").Return(command)
+				lcc.EXPECT().CreateWithoutStdio("shell", limaInstanceName, "sudo", "-E", "nerdctl", "version",
+					"--format", "json").Return(command)
 				command.EXPECT().Output().Return([]byte(`{
 					"Client":{
 						"Version":"v1.0.0",

e2e/vm/finch_config_file_test.go

@@ -0,0 +1,69 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package vm
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"time"
+
+	"github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+	"github.com/runfinch/common-tests/command"
+	"github.com/runfinch/common-tests/ffs"
+	"github.com/runfinch/common-tests/fnet"
+	"github.com/runfinch/common-tests/option"
+)
+
+// testFinchConfigFile makes sure that DOCKER_CONFIG is properly set to ~/.finch so that related information
+// is written to ~/.finch/config.json file.
+var testFinchConfigFile = func(o *option.Option) {
+	ginkgo.Describe("finch config file", func() {
+		ginkgo.It("should store login credentials", func() {
+			filename := "htpasswd"
+			registryImage := "public.ecr.aws/docker/library/registry:2"
+			registryContainer := "auth-registry"
+			// The htpasswd is generated by
+			// `finch run --entrypoint htpasswd public.ecr.aws/docker/library/httpd:2 -Bbn testUser testPassword`.
+			// We don't want to generate it on the fly because:
+			// 1. Pulling the httpd image can take a long time, sometimes even more 10 seconds.
+			// 2. It's unlikely that we will have to update this in the future.
+			// 3. It's not the thing we want to validate by the functional tests. We only want the output produced by it.
+			//nolint:gosec // This password is only used for testing purpose.
+			htpasswd := "testUser:$2y$05$wE0sj3r9O9K9q7R0MXcfPuIerl/06L1IsxXkCuUr3QZ8lHWwicIdS"
+			htpasswdDir := filepath.Dir(ffs.CreateTempFile(filename, htpasswd))
+			ginkgo.DeferCleanup(os.RemoveAll, htpasswdDir)
+			port := fnet.GetFreePort()
+			containerID := command.StdoutStr(o, "run",
+				"-dp", fmt.Sprintf("%d:5000", port),
+				"--name", registryContainer,
+				"-v", fmt.Sprintf("%s:/auth", htpasswdDir),
+				"-e", "REGISTRY_AUTH=htpasswd",
+				"-e", "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm",
+				"-e", fmt.Sprintf("REGISTRY_AUTH_HTPASSWD_PATH=/auth/%s", filename),
+				registryImage)
+			ginkgo.DeferCleanup(command.Run, o, "rmi", registryImage)
+			ginkgo.DeferCleanup(command.Run, o, "rm", "-f", registryContainer)
+			for command.StdoutStr(o, "inspect", "-f", "{{.State.Running}}", containerID) != "true" {
+				time.Sleep(1 * time.Second)
+			}
+			registry := fmt.Sprintf(`localhost:%d`, port)
+			command.Run(o, "login", registry, "-u", "testUser", "-p", "testPassword")
+
+			homeDir, err := os.UserHomeDir()
+			gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
+			configPath := fmt.Sprintf("%s/.finch/config.json", homeDir)
+			configContent, err := os.ReadFile(filepath.Clean(configPath))
+			gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
+
+			gomega.Expect(string(configContent)).Should(gomega.ContainSubstring(registry))
+			command.Run(o, "logout", registry)
+
+			configContent, err = os.ReadFile(filepath.Clean(configPath))
+			gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
+			gomega.Expect(string(configContent)).ShouldNot(gomega.ContainSubstring(registry))
+		})
+	})
+}

e2e/vm/vm_test.go

@@ -45,6 +45,7 @@ func TestVM(t *testing.T) {
 		testVMLifecycle(o)
 		testAdditionalDisk(o)
 		testConfig(o, *e2e.Installed)
+		testFinchConfigFile(o)
 		testVersion(o)
 		testVirtualizationFrameworkAndRosetta(o, *e2e.Installed)
 	})

pkg/config/nerdctl_config_applier.go

@@ -26,17 +26,19 @@ type nerdctlConfigApplier struct {
 	dialer         fssh.Dialer
 	fs             afero.Fs
 	privateKeyPath string
+	hostUser       string
 }
 
 var _ NerdctlConfigApplier = (*nerdctlConfigApplier)(nil)
 
 // NewNerdctlApplier creates a new NerdctlConfigApplier that
 // applies nerdctl configuration changes by SSHing to the lima VM to update the nerdctl configuration file in it.
-func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath string) NerdctlConfigApplier {
+func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath, hostUser string) NerdctlConfigApplier {
 	return &nerdctlConfigApplier{
 		dialer:         dialer,
 		fs:             fs,
 		privateKeyPath: privateKeyPath,
+		hostUser:       hostUser,
 	}
 }
 
@@ -46,13 +48,16 @@ func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath string) N
 // The [GNU docs for Bash] explain how these files work together in more details.
 // The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to
 // ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json).
+// The reason that we don't set environment variables inside /root/.bashrc is that the vars inside it are
+// not able to be picked up even if we specify `sudo -E`. We have to switch to root user in order to access them, while
+// normally we would access the VM as the regular user.
 // For more information on the variable, see the registry nerdctl docs.
 //
 // [GNU docs for Bash]: https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html
 //
 // [registry nerdctl docs]: https://github.com/containerd/nerdctl/blob/master/docs/registry.md
 func updateEnvironment(fs afero.Fs, user string) error {
-	profileFilePath := "/root/.bashrc"
+	profileFilePath := fmt.Sprintf("/home/%s.linux/.bashrc", user)
 	profBuf, err := afero.ReadFile(fs, profileFilePath)
 	if err != nil {
 		return fmt.Errorf("failed to read config file: %w", err)
@@ -139,7 +144,7 @@ func (nca *nerdctlConfigApplier) Apply(remoteAddr string) error {
 		return fmt.Errorf("failed to update the nerdctl config file: %w", err)
 	}
 
-	if err := updateEnvironment(sftpFs, user); err != nil {
+	if err := updateEnvironment(sftpFs, nca.hostUser); err != nil {
 		return fmt.Errorf("failed to update the user's .profile file: %w", err)
 	}
 	return nil

pkg/config/nerdctl_config_applier_test.go

@@ -42,10 +42,10 @@ func Test_updateEnvironment(t *testing.T) {
 			name: "happy path",
 			user: "mock_user",
 			mockSvc: func(t *testing.T, fs afero.Fs) {
-				require.NoError(t, afero.WriteFile(fs, "/root/.bashrc", []byte(""), 0o644))
+				require.NoError(t, afero.WriteFile(fs, "/home/mock_user.linux/.bashrc", []byte(""), 0o644))
 			},
 			postRunCheck: func(t *testing.T, fs afero.Fs) {
-				fileBytes, err := afero.ReadFile(fs, "/root/.bashrc")
+				fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
 				require.NoError(t, err)
 				assert.Equal(t, []byte("\n"+`export DOCKER_CONFIG="/Users/mock_user/.finch"`+"\n"), fileBytes)
 			},
@@ -59,14 +59,14 @@ func Test_updateEnvironment(t *testing.T) {
 					t,
 					afero.WriteFile(
 						fs,
-						"/root/.bashrc",
+						"/home/mock_user.linux/.bashrc",
 						[]byte(`export DOCKER_CONFIG="/Users/mock_user/.finch"`),
 						0o644,
 					),
 				)
 			},
 			postRunCheck: func(t *testing.T, fs afero.Fs) {
-				fileBytes, err := afero.ReadFile(fs, "/root/.bashrc")
+				fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
 				require.NoError(t, err)
 				assert.Equal(t, []byte(`export DOCKER_CONFIG="/Users/mock_user/.finch"`), fileBytes)
 			},
@@ -79,7 +79,7 @@ func Test_updateEnvironment(t *testing.T) {
 			postRunCheck: func(t *testing.T, fs afero.Fs) {},
 			want: fmt.Errorf(
 				"failed to read config file: %w",
-				&fs.PathError{Op: "open", Path: "/root/.bashrc", Err: errors.New("file does not exist")},
+				&fs.PathError{Op: "open", Path: "/home/mock_user.linux/.bashrc", Err: errors.New("file does not exist")},
 			),
 		},
 	}
@@ -190,6 +190,7 @@ func TestNerdctlConfigApplier_Apply(t *testing.T) {
 	testCases := []struct {
 		name       string
 		path       string
+		hostUser   string
 		remoteAddr string
 		mockSvc    func(t *testing.T, fs afero.Fs, d *mocks.Dialer)
 		want       error
@@ -198,6 +199,7 @@ func TestNerdctlConfigApplier_Apply(t *testing.T) {
 			name:       "private key path doesn't exist",
 			path:       "/private-key",
 			remoteAddr: "",
+			hostUser:   "mock-host-user",
 			mockSvc: func(t *testing.T, fs afero.Fs, d *mocks.Dialer) {
 			},
 			want: fmt.Errorf(
@@ -232,7 +234,7 @@ func TestNerdctlConfigApplier_Apply(t *testing.T) {
 			d := mocks.NewDialer(ctrl)
 
 			tc.mockSvc(t, fs, d)
-			got := NewNerdctlApplier(d, fs, tc.path).Apply(tc.remoteAddr)
+			got := NewNerdctlApplier(d, fs, tc.path, tc.hostUser).Apply(tc.remoteAddr)
 
 			assert.Equal(t, tc.want, got)
 		})