fix: improve creds helper UX (#673)

Issue #, if available: *Description of changes:* - Make it so aws configure is also run on the run command, since that may invoke a pull - Clean up code and consider `config.json`'s state when checking if the cred helper is "installed" or not - TODO: remove need for var overriding to facilitate testing *Testing done:* - local testing, e2e tests - [x] 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: Justin Alvarez <[email protected]>
runfinch · Oct 31, 2023 · bc238cf · bc238cf
1 parent 2c97323
commit bc238cf
Show file tree

Hide file tree

Showing 3 changed files with 185 additions and 65 deletions.
diff --git a/cmd/finch/nerdctl.go b/cmd/finch/nerdctl.go
@@ -173,7 +173,15 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
 
 	var additionalEnv []string
 	switch cmdName {
-	case "build", "pull", "push":
+	case "image":
+		if slices.Contains(args, "build") || slices.Contains(args, "pull") || slices.Contains(args, "push") {
+			ensureRemoteCredentials(nc.fc, nc.ecc, &additionalEnv, nc.logger)
+		}
+	case "container":
+		if slices.Contains(args, "run") {
+			ensureRemoteCredentials(nc.fc, nc.ecc, &additionalEnv, nc.logger)
+		}
+	case "build", "pull", "push", "run":
 		ensureRemoteCredentials(nc.fc, nc.ecc, &additionalEnv, nc.logger)
 	}
 

diff --git a/pkg/dependency/credhelper/cred_helper_binary.go b/pkg/dependency/credhelper/cred_helper_binary.go
@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/opencontainers/go-digest"
@@ -49,7 +50,7 @@ func newCredHelperBinary(fp path.Finch, fs afero.Fs, cmdCreator command.Creator,
 
 // updateConfigFile updates the config.json file to configure the credential helper.
 func updateConfigFile(bin *credhelperbin) error {
-	cfgPath := fmt.Sprintf("%s%s", bin.hcfg.finchPath, "config.json")
+	cfgPath := filepath.Join(bin.hcfg.finchPath, "config.json")
 	binCfgName := bin.credHelperConfigName()
 	fileExists, err := afero.Exists(bin.fs, cfgPath)
 	if err != nil {
@@ -71,6 +72,7 @@ func updateConfigFile(bin *credhelperbin) error {
 		if err != nil {
 			return err
 		}
+		defer fileRead.Close() //nolint:errcheck // closing the file
 		bytes, err := afero.ReadAll(fileRead)
 		if err != nil {
 			return err
@@ -81,8 +83,7 @@ func updateConfigFile(bin *credhelperbin) error {
 			return err
 		}
 		credsStore := cfg.CredentialsStore
-		defer fileRead.Close() //nolint:errcheck // closing the file
-		if strings.Compare(credsStore, binCfgName) != 0 {
+		if credsStore != binCfgName {
 			file, err := bin.fs.OpenFile(cfgPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o755)
 			if err != nil {
 				return err
@@ -103,6 +104,38 @@ func updateConfigFile(bin *credhelperbin) error {
 	return nil
 }
 
+func (bin *credhelperbin) binaryInstalled() (bool, error) {
+	return binaryInstalled(bin)
+}
+
+func (bin *credhelperbin) configFileInstalled() (bool, error) {
+	cfgPath := filepath.Join(bin.hcfg.finchPath, "config.json")
+	binCfgName := bin.credHelperConfigName()
+
+	if fileExists, err := afero.Exists(bin.fs, cfgPath); err != nil {
+		return false, err
+	} else if !fileExists {
+		return false, nil
+	}
+
+	fileRead, err := bin.fs.Open(cfgPath)
+	if err != nil {
+		return false, err
+	}
+	bytes, err := afero.ReadAll(fileRead)
+	if err != nil {
+		return false, err
+	}
+	var cfg configfile.ConfigFile
+	err = json.Unmarshal(bytes, &cfg)
+	if err != nil {
+		return false, err
+	}
+	credsStore := cfg.CredentialsStore
+	defer fileRead.Close() //nolint:errcheck // closing the file
+	return credsStore == binCfgName, nil
+}
+
 // credHelperConfigName returns the name of the credential helper binary that will be used
 // inside the config.json.
 func (bin *credhelperbin) credHelperConfigName() string {
@@ -111,85 +144,126 @@ func (bin *credhelperbin) credHelperConfigName() string {
 
 // fullInstallPath returns the full installation path of the credential helper binary.
 func (bin *credhelperbin) fullInstallPath() string {
-	return fmt.Sprintf("%s%s", bin.hcfg.installFolder, bin.hcfg.binaryName)
+	return filepath.Join(bin.hcfg.installFolder, bin.hcfg.binaryName)
 }
 
 // Installed checks if the credential helper already exists in the specified
 // folder and checks if the hash of the installed binary is correct.
 func (bin *credhelperbin) Installed() bool {
-	dirExists, err := afero.DirExists(bin.fs, bin.hcfg.installFolder)
-	if err != nil {
-		bin.l.Errorf("failed to get status of credential helper directory: %v", err)
-		return false
-	}
-	if !dirExists {
-		return false
-	}
-	fileExists, err := afero.Exists(bin.fs, bin.fullInstallPath())
-	if err != nil {
-		bin.l.Errorf("failed to get status of credential helper binary: %v", err)
-		return false
-	}
-	if !fileExists {
-		return false
-	}
-	file, err := bin.fs.Open(bin.fullInstallPath())
-	if err != nil {
+	if binInstalled, err := bin.binaryInstalled(); err != nil {
 		bin.l.Error(err)
 		return false
+	} else if !binInstalled {
+		return false
 	}
-	defer file.Close() //nolint:errcheck // closing the file
-	hash, err := digest.FromReader(file)
-	if err != nil {
+
+	if cfgInstalled, err := bin.configFileInstalled(); err != nil {
 		bin.l.Error(err)
 		return false
-	}
-	if strings.Compare(hash.String(), bin.hcfg.hash) != 0 {
-		bin.l.Info("Hash of the installed credential helper binary does not match")
-		err := bin.fs.Remove(bin.fullInstallPath())
-		if err != nil {
-			bin.l.Error(err)
-		}
+	} else if !cfgInstalled {
 		return false
 	}
+
 	return true
 }
 
 // Install installs and configures the specified credential helper.
 func (bin *credhelperbin) Install() error {
-	if bin.helper == "" {
-		return nil
-	}
-	if strings.Compare(bin.helper, bin.credHelperConfigName()) != 0 {
-		return nil
-	}
-	credHelperName := strings.ReplaceAll(bin.credHelperConfigName(), "-login", "")
-	bin.l.Infof("Installing %s credential helper", credHelperName)
-	mkdirCmd := bin.cmdCreator.Create("mkdir", "-p", bin.hcfg.installFolder)
-	_, err := mkdirCmd.Output()
+	binInstalled, err := bin.binaryInstalled()
 	if err != nil {
-		return fmt.Errorf("error creating installation directory %s, err: %w", bin.hcfg.installFolder, err)
+		return err
 	}
 
-	curlCmd := bin.cmdCreator.Create("curl", "--retry", "5", "--retry-max-time", "30", "--url",
-		bin.hcfg.credHelperURL, "--output", bin.fullInstallPath())
+	if !binInstalled {
+		if bin.helper == "" {
+			return nil
+		}
+		if bin.helper != bin.credHelperConfigName() {
+			return nil
+		}
+		credHelperName := strings.ReplaceAll(bin.credHelperConfigName(), "-login", "")
+		bin.l.Infof("Installing %s credential helper", credHelperName)
+		if err := bin.fs.MkdirAll(bin.hcfg.installFolder, 0o700); err != nil {
+			return fmt.Errorf("error creating installation directory %s, err: %w", bin.hcfg.installFolder, err)
+		}
 
-	_, err = curlCmd.Output()
-	if err != nil {
-		return fmt.Errorf("error installing binary %s, err: %w", bin.hcfg.binaryName, err)
+		curlCmd := bin.cmdCreator.Create(
+			"curl",
+			"--retry",
+			"5",
+			"--retry-max-time",
+			"30",
+			"--url",
+			bin.hcfg.credHelperURL,
+			"--output",
+			bin.fullInstallPath(),
+		)
+
+		if _, err = curlCmd.Output(); err != nil {
+			return fmt.Errorf("error installing binary %s, err: %w", bin.hcfg.binaryName, err)
+		}
+		if err := bin.fs.Chmod(bin.fullInstallPath(), 0o700); err != nil {
+			return err
+		}
 	}
-	err = bin.fs.Chmod(bin.fullInstallPath(), 0o755)
+
+	cfgInstalled, err := bin.configFileInstalled()
 	if err != nil {
 		return err
 	}
-	err = updateConfigFile(bin)
-	if err != nil {
-		return err
+
+	if !cfgInstalled {
+		if err := updateConfigFile(bin); err != nil {
+			return err
+		}
 	}
+
 	return nil
 }
 
 // RequiresRoot returns whether the installation of the binary needs root permissions.
 func (bin *credhelperbin) RequiresRoot() bool {
 	return false
 }
+
+// Using a var function allows overriding during testing.
+// This is needed because the curl command directly outputs to a file, but binaryInstalled deletes
+// any incorrect file that might exist at the fullInstallPath.
+// This means that the mocking method that is typically used to mock filesystem will get the file it
+// creates deleted, and then, since cmd.Output() is what is writing the new/correct file, and there's
+// no opportunity to mock it or insert the file after it runs, the code that expects the file to exist
+// then errors out because it was deleted by binaryInstalled.
+var binaryInstalled = func(bin *credhelperbin) (bool, error) {
+	dirExists, err := afero.DirExists(bin.fs, bin.hcfg.installFolder)
+	if err != nil {
+		return false, fmt.Errorf("failed to get status of credential helper directory: %w", err)
+	}
+	if !dirExists {
+		return false, nil
+	}
+	fileExists, err := afero.Exists(bin.fs, bin.fullInstallPath())
+	if err != nil {
+		return false, fmt.Errorf("failed to get status of credential helper binary: %w", err)
+	}
+	if !fileExists {
+		return false, nil
+	}
+	file, err := bin.fs.Open(bin.fullInstallPath())
+	if err != nil {
+		return false, fmt.Errorf("failed to open cred helper binary: %w", err)
+	}
+	defer file.Close() //nolint:errcheck // closing the file
+	hash, err := digest.FromReader(file)
+	if err != nil {
+		return false, fmt.Errorf("failed to get cred helper binary hash: %w", err)
+	}
+	if hash.String() != bin.hcfg.hash {
+		bin.l.Info("Hash of the installed credential helper binary does not match")
+		if err := bin.fs.Remove(bin.fullInstallPath()); err != nil {
+			return false, fmt.Errorf("failed to remove mismatched cred helper binary: %w", err)
+		}
+		return false, nil
+	}
+
+	return true, nil
+}
diff --git a/pkg/dependency/credhelper/cred_helper_binary_test.go b/pkg/dependency/credhelper/cred_helper_binary_test.go
@@ -149,9 +149,23 @@ func TestBinaries_Installed(t *testing.T) {
 				fileData := []byte("")
 				_, err := mFs.Create("mock_prefix/cred-helpers/docker-credential-binary")
 				require.NoError(t, err)
-				err = afero.WriteFile(mFs, "mock_prefix/cred-helpers/docker-credential-binary",
-					fileData, 0o666)
+				err = afero.WriteFile(
+					mFs,
+					"mock_prefix/cred-helpers/docker-credential-binary",
+					fileData,
+					0o666,
+				)
+				require.NoError(t, err)
 
+				cfgFileData := []byte(`{"credsStore":"binary"}`)
+				_, err = mFs.Create("mock_prefix/.finch/config.json")
+				require.NoError(t, err)
+				err = afero.WriteFile(
+					mFs,
+					"mock_prefix/.finch/config.json",
+					cfgFileData,
+					0o666,
+				)
 				require.NoError(t, err)
 			},
 			want: true,
@@ -197,7 +211,8 @@ func TestBinaries_Installed(t *testing.T) {
 			tc.mockSvc(t, mFs, l)
 			hc := helperConfig{
 				"docker-credential-binary", "",
-				"sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "mock_prefix/cred-helpers/",
+				"sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+				"mock_prefix/cred-helpers/",
 				"mock_prefix/.finch/",
 			}
 			// hash of an empty file
@@ -208,59 +223,82 @@ func TestBinaries_Installed(t *testing.T) {
 	}
 }
 
+//nolint:paralleltest // This function manipulates a global variable to facilitate mocking.
 func TestBinaries_Install(t *testing.T) {
-	t.Parallel()
-
 	testCases := []struct {
 		name    string
 		mockSvc func(
 			l *mocks.Logger,
 			cmd *mocks.Command,
 			creator *mocks.CommandCreator,
-			mFs afero.Fs)
-		want error
+			mFs afero.Fs,
+		)
+		want         error
+		postRunCheck func(t *testing.T, mFs afero.Fs)
 	}{
 		{
 			name: "happy path",
 			mockSvc: func(l *mocks.Logger, cmd *mocks.Command, creator *mocks.CommandCreator, mFs afero.Fs) {
+				binaryInstalled = func(*credhelperbin) (bool, error) {
+					return false, nil
+				}
 				_, err := mFs.Create("mock_prefix/cred-helpers/docker-credential-ecr-login")
 				require.NoError(t, err)
-				cmd.EXPECT().Output().Times(2)
+				cmd.EXPECT().Output()
 				l.EXPECT().Infof("Installing %s credential helper", "ecr")
-				creator.EXPECT().Create("mkdir", "-p", "mock_prefix/cred-helpers/").Return(cmd)
 				creator.EXPECT().Create("curl", "--retry", "5", "--retry-max-time", "30", "--url",
 					"https://amazon-ecr-credential-helper-releases.s3.us-east-2.amazonaws.com"+
 						"/0.7.0/linux-arm64/docker-credential-ecr-login", "--output",
 					"mock_prefix/cred-helpers/docker-credential-ecr-login").Return(cmd)
 			},
 			want: nil,
+			postRunCheck: func(t *testing.T, mFs afero.Fs) {
+				f, err := afero.ReadFile(mFs, "mock_prefix/.finch/config.json")
+				require.NoError(t, err)
+				require.Equal(t, string(f), `{"credsStore":"ecr-login"}`)
+			},
+		},
+		{
+			name: "credential helper already installed, but config file not configured",
+			mockSvc: func(l *mocks.Logger, cmd *mocks.Command, creator *mocks.CommandCreator, mFs afero.Fs) {
+				binaryInstalled = func(*credhelperbin) (bool, error) {
+					return true, nil
+				}
+			},
+			want: nil,
+			postRunCheck: func(t *testing.T, mFs afero.Fs) {
+				f, err := afero.ReadFile(mFs, "mock_prefix/.finch/config.json")
+				require.NoError(t, err)
+				require.Equal(t, string(f), `{"credsStore":"ecr-login"}`)
+			},
 		},
 	}
 
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
 			ctrl := gomock.NewController(t)
 			cmd := mocks.NewCommand(ctrl)
 			mFs := afero.NewMemMapFs()
 			l := mocks.NewLogger(ctrl)
 			creator := mocks.NewCommandCreator(ctrl)
+			origBinaryInstalled := binaryInstalled
 			tc.mockSvc(l, cmd, creator, mFs)
 
 			credHelperURL := "https://amazon-ecr-credential-helper-releases.s3.us-east-2.amazonaws.com" +
 				"/0.7.0/linux-arm64/docker-credential-ecr-login"
 
 			hc := helperConfig{
 				"docker-credential-ecr-login", credHelperURL,
-				"sha256:ff14a4da40d28a2d2d81a12a7c9c36294ddf8e6439780c4ccbc96622991f3714",
+				"sha256:ec5c04babea79b08dffb0c8acb67b9e28dc05be0fe9bd4df2e234d75516061d7",
 				"mock_prefix/cred-helpers/",
 				"mock_prefix/.finch/",
 			}
 			fc := "ecr-login"
 			got := newCredHelperBinary(mockFinchPath, mFs, creator, l, fc, "", hc).Install()
+			binaryInstalled = origBinaryInstalled
 			assert.Equal(t, tc.want, got)
+			tc.postRunCheck(t, mFs)
 		})
 	}
 }