From f9b7788aeeedb2bfc29240885d6eefb91a5bea3b Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 13:41:38 -0400 Subject: [PATCH 1/7] fix: also get aws creds on run since it may pull Signed-off-by: Justin Alvarez --- cmd/finch/nerdctl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/finch/nerdctl.go b/cmd/finch/nerdctl.go index bcf5ccde1..2a54d0bca 100644 --- a/cmd/finch/nerdctl.go +++ b/cmd/finch/nerdctl.go @@ -173,7 +173,7 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error { var additionalEnv []string switch cmdName { - case "build", "pull", "push": + case "build", "pull", "push", "run": ensureRemoteCredentials(nc.fc, nc.ecc, &additionalEnv, nc.logger) } From 586a7cf37fedcc00f1e0cee5394e075216dfd84b Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 14:33:23 -0400 Subject: [PATCH 2/7] fix: better UX when config.json exists but isn't set to use credential helper Signed-off-by: Justin Alvarez --- .../credhelper/cred_helper_binary.go | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/pkg/dependency/credhelper/cred_helper_binary.go b/pkg/dependency/credhelper/cred_helper_binary.go index 8a3ed2b50..d88989837 100644 --- a/pkg/dependency/credhelper/cred_helper_binary.go +++ b/pkg/dependency/credhelper/cred_helper_binary.go @@ -82,7 +82,7 @@ func updateConfigFile(bin *credhelperbin) error { } 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 +103,34 @@ func updateConfigFile(bin *credhelperbin) error { return nil } +func (bin *credhelperbin) configFileInstalled() (bool, error) { + cfgPath := fmt.Sprintf("%s%s", 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 { @@ -144,7 +172,7 @@ func (bin *credhelperbin) Installed() bool { bin.l.Error(err) return false } - if strings.Compare(hash.String(), bin.hcfg.hash) != 0 { + if hash.String() != bin.hcfg.hash { bin.l.Info("Hash of the installed credential helper binary does not match") err := bin.fs.Remove(bin.fullInstallPath()) if err != nil { @@ -152,6 +180,14 @@ func (bin *credhelperbin) Installed() bool { } return false } + + if cfgInstalled, err := bin.configFileInstalled(); err != nil { + bin.l.Error(err) + return false + } else if !cfgInstalled { + return false + } + return true } @@ -160,7 +196,7 @@ func (bin *credhelperbin) Install() error { if bin.helper == "" { return nil } - if strings.Compare(bin.helper, bin.credHelperConfigName()) != 0 { + if bin.helper != bin.credHelperConfigName() { return nil } credHelperName := strings.ReplaceAll(bin.credHelperConfigName(), "-login", "") From 939ba77601a9d579bbf406264b9119e44982be83 Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 16:12:35 -0400 Subject: [PATCH 3/7] update tests Signed-off-by: Justin Alvarez --- .../credhelper/cred_helper_binary.go | 140 +++++++++++------- .../credhelper/cred_helper_binary_test.go | 57 ++++++- 2 files changed, 138 insertions(+), 59 deletions(-) diff --git a/pkg/dependency/credhelper/cred_helper_binary.go b/pkg/dependency/credhelper/cred_helper_binary.go index d88989837..d3698089f 100644 --- a/pkg/dependency/credhelper/cred_helper_binary.go +++ b/pkg/dependency/credhelper/cred_helper_binary.go @@ -103,6 +103,10 @@ func updateConfigFile(bin *credhelperbin) error { return nil } +func (bin *credhelperbin) binaryInstalled() (bool, error) { + return binaryInstalled(bin) +} + func (bin *credhelperbin) configFileInstalled() (bool, error) { cfgPath := fmt.Sprintf("%s%s", bin.hcfg.finchPath, "config.json") binCfgName := bin.credHelperConfigName() @@ -145,39 +149,10 @@ func (bin *credhelperbin) fullInstallPath() string { // 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 - } - defer file.Close() //nolint:errcheck // closing the file - hash, err := digest.FromReader(file) - if err != nil { - bin.l.Error(err) - return false - } - if hash.String() != bin.hcfg.hash { - 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 !binInstalled { return false } @@ -193,35 +168,56 @@ func (bin *credhelperbin) Installed() bool { // Install installs and configures the specified credential helper. func (bin *credhelperbin) Install() error { - 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) - 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) + mkdirCmd := bin.cmdCreator.Create("mkdir", "-p", bin.hcfg.installFolder) + if _, err := mkdirCmd.Output(); 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(), 0o755); 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 } @@ -229,3 +225,45 @@ func (bin *credhelperbin) Install() error { 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 index 000cdea42..eac4326af 100644 --- 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,21 +223,25 @@ 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) @@ -234,6 +253,25 @@ func TestBinaries_Install(t *testing.T) { "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"}`) + }, }, } @@ -247,6 +285,7 @@ func TestBinaries_Install(t *testing.T) { 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" + @@ -254,13 +293,15 @@ func TestBinaries_Install(t *testing.T) { 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) }) } } From 1b934540c3ac6ad75ea89018a73cb05c9ac4f303 Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 18:16:11 -0400 Subject: [PATCH 4/7] move defer up Signed-off-by: Justin Alvarez --- pkg/dependency/credhelper/cred_helper_binary.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/dependency/credhelper/cred_helper_binary.go b/pkg/dependency/credhelper/cred_helper_binary.go index d3698089f..65e4dabd5 100644 --- a/pkg/dependency/credhelper/cred_helper_binary.go +++ b/pkg/dependency/credhelper/cred_helper_binary.go @@ -71,6 +71,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,7 +82,6 @@ func updateConfigFile(bin *credhelperbin) error { return err } credsStore := cfg.CredentialsStore - defer fileRead.Close() //nolint:errcheck // closing the file if credsStore != binCfgName { file, err := bin.fs.OpenFile(cfgPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o755) if err != nil { From 6a7e7c18f9ffd7517a910cb230ae0132dce25704 Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 18:31:13 -0400 Subject: [PATCH 5/7] remove parallel testing to reduce flakiness Signed-off-by: Justin Alvarez --- pkg/dependency/credhelper/cred_helper_binary_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/dependency/credhelper/cred_helper_binary_test.go b/pkg/dependency/credhelper/cred_helper_binary_test.go index eac4326af..f09d7715e 100644 --- a/pkg/dependency/credhelper/cred_helper_binary_test.go +++ b/pkg/dependency/credhelper/cred_helper_binary_test.go @@ -278,8 +278,6 @@ func TestBinaries_Install(t *testing.T) { 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() From b160c41631103c32258ce6b7b5aa8274f27b91dc Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 18:53:20 -0400 Subject: [PATCH 6/7] address comments Signed-off-by: Justin Alvarez --- pkg/dependency/credhelper/cred_helper_binary.go | 12 ++++++------ pkg/dependency/credhelper/cred_helper_binary_test.go | 3 +-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pkg/dependency/credhelper/cred_helper_binary.go b/pkg/dependency/credhelper/cred_helper_binary.go index 65e4dabd5..f3f2f241e 100644 --- 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 { @@ -108,7 +109,7 @@ func (bin *credhelperbin) binaryInstalled() (bool, error) { } func (bin *credhelperbin) configFileInstalled() (bool, error) { - cfgPath := fmt.Sprintf("%s%s", bin.hcfg.finchPath, "config.json") + cfgPath := filepath.Join(bin.hcfg.finchPath, "config.json") binCfgName := bin.credHelperConfigName() if fileExists, err := afero.Exists(bin.fs, cfgPath); err != nil { @@ -143,7 +144,7 @@ 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 @@ -182,8 +183,7 @@ func (bin *credhelperbin) Install() error { } credHelperName := strings.ReplaceAll(bin.credHelperConfigName(), "-login", "") bin.l.Infof("Installing %s credential helper", credHelperName) - mkdirCmd := bin.cmdCreator.Create("mkdir", "-p", bin.hcfg.installFolder) - if _, err := mkdirCmd.Output(); err != nil { + 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) } @@ -202,7 +202,7 @@ func (bin *credhelperbin) Install() error { 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(), 0o755); err != nil { + if err := bin.fs.Chmod(bin.fullInstallPath(), 0o700); err != nil { return err } } diff --git a/pkg/dependency/credhelper/cred_helper_binary_test.go b/pkg/dependency/credhelper/cred_helper_binary_test.go index f09d7715e..c132a44a7 100644 --- a/pkg/dependency/credhelper/cred_helper_binary_test.go +++ b/pkg/dependency/credhelper/cred_helper_binary_test.go @@ -244,9 +244,8 @@ func TestBinaries_Install(t *testing.T) { } _, 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", From 6aa7326b0beee3cc0ede6aedee8fa4fe036bedbd Mon Sep 17 00:00:00 2001 From: Justin Alvarez Date: Tue, 31 Oct 2023 19:02:40 -0400 Subject: [PATCH 7/7] add additional commands/subcommands that might need creds Signed-off-by: Justin Alvarez --- cmd/finch/nerdctl.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/finch/nerdctl.go b/cmd/finch/nerdctl.go index 2a54d0bca..468af0bed 100644 --- a/cmd/finch/nerdctl.go +++ b/cmd/finch/nerdctl.go @@ -173,6 +173,14 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error { var additionalEnv []string switch cmdName { + 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) }