Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve creds helper UX #673

Merged
merged 7 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/finch/nerdctl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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":
vsiravar marked this conversation as resolved.
Show resolved Hide resolved
ensureRemoteCredentials(nc.fc, nc.ecc, &additionalEnv, nc.logger)
}

Expand Down
172 changes: 123 additions & 49 deletions pkg/dependency/credhelper/cred_helper_binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -103,6 +103,38 @@ 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")
austinvazquez marked this conversation as resolved.
Show resolved Hide resolved
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
austinvazquez marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand All @@ -117,79 +149,121 @@ 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
} 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)
mkdirCmd := bin.cmdCreator.Create("mkdir", "-p", bin.hcfg.installFolder)
vsiravar marked this conversation as resolved.
Show resolved Hide resolved
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 {
vsiravar marked this conversation as resolved.
Show resolved Hide resolved
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
}
57 changes: 49 additions & 8 deletions pkg/dependency/credhelper/cred_helper_binary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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"}`)
},
},
}

Expand All @@ -247,20 +285,23 @@ 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" +
"/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)
})
}
}
Expand Down
Loading