Skip to content

Fix issue where one bad credential helper causes no credentials to be returned #3996

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Jan 31, 2023
Merged

Fix issue where one bad credential helper causes no credentials to be returned #3996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion cli/config/configfile/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,8 @@ func (configFile *ConfigFile) GetAllCredentials() (map[string]types.AuthConfig,
for registryHostname := range configFile.CredentialHelpers {
newAuth, err := configFile.GetAuthConfig(registryHostname)
if err != nil {
return nil, err
logrus.WithError(err).Warnf("Failed to get credentials for registry: %s", registryHostname)
Copy link
Member

@thaJeztah thaJeztah Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking at this again, and while we should look at using the logger more, we should probably look at the output for it. Currently, the output looks like;

WARN[0000] Failed to get credentials for registry: foo   error="something went wrong"

Still readable, but perhaps not as "friendly". I see elsewhere we just print our own, e.g.;

cli/cli/config/config.go

Line 144 in 3ae101f

fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err)

Which may look like;

WARNING: Error loading config file: /root/.docker/config.json: json: cannot unmarshal string into Go struct field ConfigFile.auths of type types.AuthConfig

Only issue is that this location of the code currently doesn't have a reference to the CLI's stdout or stderr, so if we change it, we probably need to hardcode it to os.Stderr (not ideal either).

Copy link
Member

@thaJeztah thaJeztah Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideas / suggestions (perhaps someone else has thoughts) welcome.

Don't think it's necessarily a blocker, if there's no better solution (for now)

Copy link
Collaborator Author

@laurazard laurazard Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! In general I think having the more standardized logging is good (vs just calling fmt.Fprintf), but the friendliness is a good point. Maybe we can look into seeing if we can configure the output for it. I don't have better ideas right now though 😅

Copy link
Member

@thaJeztah thaJeztah Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a lot to look into in this area;

  • There's fmt.PrintXX in many places
  • While it's "generally" ok to use the defaults (I suspect logrus is not configured anywhere to use something other than os.StdOut / os.StdErr), ideally we'd have better controls for that
  • ^^ sometimes we need to use a different approach depending on (e.g.) if an actual TTY is attached (see cli: additionalHelp() don't decorate output if it's piped, and add extra newline #3973)
  • ^^ or (e.g.) if we want the output to be "quiet" (could be in the CLI itself, or other projects using parts of the CLI as a "library")
  • I also went looking for some of that, e.g. WIP: make printing output easier (approach 1) #3797
  • (as mentioned) logrus (or other loggers) are great for logs, but the default output format often isn't great for printing "one-off", user-facing messages

So yes, this needs work (perhaps I should create a tracking ticket somewhere); e.g.

  • Consider using a context-logger to pass a logger around
  • That logger could (default) be a logger with a format that's "friendly enough"
  • Look into best approach to pass around the streams (STDIN/STDOUT/STDERR), but when doing so, try to prevent coupling things too tightly (i.e., we may not want to pass cobra.Command everywhere).

continue
}
auths[registryHostname] = newAuth
}
Expand Down
68 changes: 56 additions & 12 deletions cli/config/configfile/file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package configfile
import (
"bytes"
"encoding/json"
"errors"
"os"
"testing"

Expand Down Expand Up @@ -166,8 +167,9 @@ func TestConfigFile(t *testing.T) {
}

type mockNativeStore struct {
GetAllCallCount int
authConfigs map[string]types.AuthConfig
GetAllCallCount int
authConfigs map[string]types.AuthConfig
authConfigErrors map[string]error
}

func (c *mockNativeStore) Erase(registryHostname string) error {
Expand All @@ -176,7 +178,7 @@ func (c *mockNativeStore) Erase(registryHostname string) error {
}

func (c *mockNativeStore) Get(registryHostname string) (types.AuthConfig, error) {
return c.authConfigs[registryHostname], nil
return c.authConfigs[registryHostname], c.authConfigErrors[registryHostname]
}

func (c *mockNativeStore) GetAll() (map[string]types.AuthConfig, error) {
Expand All @@ -191,8 +193,8 @@ func (c *mockNativeStore) Store(authConfig types.AuthConfig) error {
// make sure it satisfies the interface
var _ credentials.Store = (*mockNativeStore)(nil)

func NewMockNativeStore(authConfigs map[string]types.AuthConfig) credentials.Store {
return &mockNativeStore{authConfigs: authConfigs}
func NewMockNativeStore(authConfigs map[string]types.AuthConfig, authConfigErrors map[string]error) credentials.Store {
return &mockNativeStore{authConfigs: authConfigs, authConfigErrors: authConfigErrors}
}

func TestGetAllCredentialsFileStoreOnly(t *testing.T) {
Expand Down Expand Up @@ -220,7 +222,7 @@ func TestGetAllCredentialsCredsStore(t *testing.T) {
Password: "pass",
}

testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedAuth})
testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedAuth}, nil)

tmpNewNativeStore := newNativeStore
defer func() { newNativeStore = tmpNewNativeStore }()
Expand All @@ -237,6 +239,48 @@ func TestGetAllCredentialsCredsStore(t *testing.T) {
assert.Check(t, is.Equal(1, testCredsStore.(*mockNativeStore).GetAllCallCount))
}

func TestGetAllCredentialsCredStoreErrorHandling(t *testing.T) {
const (
workingHelperRegistryHostname = "working-helper.example.com"
brokenHelperRegistryHostname = "broken-helper.example.com"
)
configFile := New("filename")
configFile.CredentialHelpers = map[string]string{
workingHelperRegistryHostname: "cred_helper",
brokenHelperRegistryHostname: "broken_cred_helper",
}
expectedAuth := types.AuthConfig{
Username: "username",
Password: "pass",
}
// configure the mock store to throw an error
// when calling the helper for this registry
authErrors := map[string]error{
brokenHelperRegistryHostname: errors.New("an error"),
}

testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{
workingHelperRegistryHostname: expectedAuth,
// configure an auth entry for the "broken" credential
// helper that will throw an error
brokenHelperRegistryHostname: {},
}, authErrors)

tmpNewNativeStore := newNativeStore
defer func() { newNativeStore = tmpNewNativeStore }()
newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
return testCredsStore
}

authConfigs, err := configFile.GetAllCredentials()

// make sure we're still returning the expected credentials
// and skipping the ones throwing an error
assert.NilError(t, err)
assert.Check(t, is.Equal(1, len(authConfigs)))
assert.Check(t, is.DeepEqual(expectedAuth, authConfigs[workingHelperRegistryHostname]))
}

func TestGetAllCredentialsCredHelper(t *testing.T) {
const (
testCredHelperSuffix = "test_cred_helper"
Expand All @@ -261,7 +305,7 @@ func TestGetAllCredentialsCredHelper(t *testing.T) {
// Add in an extra auth entry which doesn't appear in CredentialHelpers section of the configFile.
// This verifies that only explicitly configured registries are being requested from the cred helpers.
testExtraCredHelperRegistryHostname: unexpectedCredHelperAuth,
})
}, nil)

tmpNewNativeStore := newNativeStore
defer func() { newNativeStore = tmpNewNativeStore }()
Expand Down Expand Up @@ -298,7 +342,7 @@ func TestGetAllCredentialsFileStoreAndCredHelper(t *testing.T) {
configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix}
configFile.AuthConfigs[testFileStoreRegistryHostname] = expectedFileStoreAuth

testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth})
testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth}, nil)

newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
return testCredHelper
Expand Down Expand Up @@ -337,8 +381,8 @@ func TestGetAllCredentialsCredStoreAndCredHelper(t *testing.T) {
Password: "cred_helper_pass",
}

testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth})
testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testCredStoreRegistryHostname: expectedCredStoreAuth})
testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth}, nil)
testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testCredStoreRegistryHostname: expectedCredStoreAuth}, nil)

tmpNewNativeStore := newNativeStore
defer func() { newNativeStore = tmpNewNativeStore }()
Expand Down Expand Up @@ -380,8 +424,8 @@ func TestGetAllCredentialsCredHelperOverridesDefaultStore(t *testing.T) {
Password: "cred_helper_pass",
}

testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedCredHelperAuth})
testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: unexpectedCredStoreAuth})
testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedCredHelperAuth}, nil)
testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: unexpectedCredStoreAuth}, nil)

tmpNewNativeStore := newNativeStore
defer func() { newNativeStore = tmpNewNativeStore }()
Expand Down