GetAll -> Get to retrieve credentials from credential helpers #840

dekkagaijin · 2018-01-26T22:57:57Z

- What I did
Improved docker build times.

- How I did it
Only request credentials for the configured registry from the corresponding credential helper, rather than doing a GetAll for each one.

- How to verify it
Build some toy image with several credential helpers configured...

jsand@jsand:~/go/src/github.com/docker/cli$ time docker build ~/gcloud-cred-helper-test
Sending build context to Docker daemon   5.12kB
Step 1/7 : FROM node:8
 ---> b87c2ad8344d
Step 2/7 : WORKDIR /usr/src/app
 ---> Using cache
 ---> b27c87b196eb
Step 3/7 : COPY package*.json ./
COPY failed: no source files were specified

real	1m28.929s
user	0m54.868s
sys	0m10.988s
jsand@jsand:~/go/src/github.com/docker/cli$ time ./build/docker build ~/gcloud-cred-helper-test
Sending build context to Docker daemon   5.12kB
Step 1/7 : FROM node:8
 ---> b87c2ad8344d
Step 2/7 : WORKDIR /usr/src/app
 ---> Using cache
 ---> b27c87b196eb
Step 3/7 : COPY package*.json ./
COPY failed: no source files were specified

real	0m7.137s
user	0m4.408s
sys	0m0.884s

- Description for the changelog
docker build now runs faster when registry-specific credential helper(s) are configured.

codecov-io · 2018-01-26T23:04:05Z

Codecov Report

Merging #840 into master will increase coverage by 0.08%.
The diff coverage is 64.7%.

@@            Coverage Diff             @@
##           master     #840      +/-   ##
==========================================
+ Coverage   52.93%   53.02%   +0.08%     
==========================================
  Files         245      244       -1     
  Lines       15848    15829      -19     
==========================================
+ Hits         8389     8393       +4     
+ Misses       6905     6884      -21     
+ Partials      554      552       -2

dnephin

Thanks! In general it probably makes sense to only query for the specific credentials.

I have a couple questions about this change.

dnephin · 2018-01-29T23:27:03Z

cli/config/configfile/file.go

+			stripped = strings.TrimPrefix(serverAddress, "http://")
+		} else if strings.HasPrefix(serverAddress, "https://") {
+			stripped = strings.TrimPrefix(serverAddress, "https://")
+		}


This should probably use registry.ConvertToHostname(), although I'm not sure this is the correct place to do the convert.

How is this related to the rest of the change?

dnephin · 2018-01-29T23:41:08Z

cli/config/configfile/file.go

+
+	// Auth configs from a registry-specific helper should override those from the default store.
+	for registryHostname := range configFile.CredentialHelpers {
+		serverAddress := "https://" + registryHostname


Why is this necessary? From what I can tell other callers of GetAuthConfig() seem to be calling it with a url that doesn't have a scheme.

Keys in configFile.CredentialHelpers field are schemeless, whereas keys in configFile.AuthConfigs (and which back the default file store) contain the scheme. I see that the file store is now stripping the scheme from the auths entries with registry.ConvertToHostname() in file_store, so I'll remove this code here.

dekkagaijin · 2018-01-30T00:46:23Z

Done...

dnephin · 2018-01-30T00:55:03Z

cli/config/configfile/file_test.go

+	configFile.AuthConfigs["https://"+testFileStoreServerAddress] = expectedFileStoreAuth
+
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {
+		return NewMockNativeStore(map[string]types.AuthConfig{testCredHelperServerAddress: expectedCredHelperAuth})


I think this probably contain some extra key/values so that the test shows that not everything is being returned.

dnephin · 2018-01-30T00:55:34Z

cli/config/configfile/file_test.go

+}
+
+func (c *mockNativeStore) GetAll() (map[string]types.AuthConfig, error) {
+	c.GetAllCallCount = c.GetAllCallCount + 1


It looks like this field isn't being checked in the tests. I assume you want to check it is 0.

dnephin · 2018-01-30T00:56:15Z

cli/config/configfile/file_test.go

+	assert.Equal(t, expected, authConfigs)
+}
+
+func TestGetAllCredentialsCredStoreAndCredHelper(t *testing.T) {


I think this one test might cover all the behaviour, but I guess it doesn't hurt to have the other ones. They should be pretty fast.

dnephin · 2018-01-30T00:57:51Z

cli/config/configfile/file_test.go

+	GetAllCallCount int
+	EraseCalls      []string
+	GetCalls        []string
+	StoreCalls      []string


These Calls fields are also unused. They can be removed.

dnephin · 2018-01-30T01:04:19Z

@thaJeztah do you know who is familiar with the credential helpers and could help review this?

dekkagaijin · 2018-01-30T01:18:36Z

Done.
@dnephin I probably have some of the most context on the credHelpers in particular, otherwise one of the people from the original PR might be of help: moby/moby#25851

thaJeztah · 2018-01-30T06:13:06Z

ping @n4ss PTAL

dnephin · 2018-01-30T22:19:53Z

cli/config/configfile/file_test.go

+	assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount)
+}
+
+func TestGetAllCredentialsCredStoreAndCredHelper(t *testing.T) {


Is this testing anything that isn't already covered by TestGetAllCredentialsCredHelperOverridesDefaultStore ?

It tests that values from both are returned, if they don't conflict, rather than just one of the other. TestGetAllCredentialsCredHelperOverridesDefaultStore tests for the conflicting case.

dnephin · 2018-01-30T22:21:29Z

cli/config/configfile/file_test.go

+		Password: "cred_helper_pass",
+	}
+
+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testServerAddress: expectedCredHelperAuth})


I think my previous comment wasn't clear. This map[string]types.AuthConfig should contain more than a single item. It should have extra items that you don't expect to get returned. Otherwise the implementation could return everything (the behaviour you're trying to fix) and the test would still pass.

n4ss · 2018-01-31T22:18:47Z

cli/config/configfile/file.go

 	defaultStore := configFile.GetCredentialsStore("")
 	newAuths, err := defaultStore.GetAll()
 	if err != nil {
 		return nil, err
 	}
 	addAll(newAuths)
+
+	// Auth configs from a registry-specific helper should override those from the default store.
+	for serverAddress := range configFile.CredentialHelpers {


nit: serverAdress is a bit too vague as a var name. Can we keep registry or registryAddr (same in the tests)?

LGTM besides this

registryHostname? Done.

dnephin

A test table, or extracting some of the common parts into setup functions might help reduce the duplication in the tests, but I think it will do as-is.

dnephin · 2018-01-31T23:06:42Z

cli/config/configfile/file_test.go

+	testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testServerAddress: expectedCredHelperAuth})
+	testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testServerAddress: unexpectedCredStoreAuth})
+
+	newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store {


I guess there should be a defer to restore this to the original value when the test exits.

dnephin

LGTM

dekkagaijin · 2018-02-05T17:59:47Z

Gentle ping :)

thaJeztah · 2018-02-06T02:41:39Z

ping @n4ss could you have another look

To reviewers: don't merge yet, as this may need some squashing once review is complete 🤗

n4ss · 2018-02-06T03:35:09Z

@thaJeztah already reviewed & LGTM'd in a comment that got nested, sorry #840 (comment)

…lpers. Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin · 2018-02-06T04:36:13Z

squished

dekkagaijin · 2018-02-08T20:27:26Z

Bump <3

Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin · 2018-02-09T21:53:20Z

Added a fix for a small style nit, should let Jenkins and Codecov execute properly again as well.

dekkagaijin · 2018-02-15T18:29:15Z

ping :)

vdemeester

LGTM 🐯

GordonTheTurtle added the status/0-triage label Jan 26, 2018

dnephin reviewed Jan 29, 2018

View reviewed changes

dnephin reviewed Jan 30, 2018

View reviewed changes

n4ss reviewed Jan 31, 2018

View reviewed changes

dnephin reviewed Jan 31, 2018

View reviewed changes

dnephin reviewed Feb 1, 2018

View reviewed changes

dnephin approved these changes Feb 1, 2018

View reviewed changes

dnephin added status/2-code-review and removed status/0-triage labels Feb 5, 2018

Use Get rather than GetAll to retrieve credentials from credential he…

1d0a37c

…lpers. Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin force-pushed the master branch from 468d156 to 1d0a37c Compare February 6, 2018 04:32

defaultIndexserver -> defaultIndexServer

e1b607a

Signed-off-by: Jake Sanders <jsand@google.com>

vdemeester approved these changes Feb 16, 2018

View reviewed changes

vdemeester merged commit 69432c4 into docker:master Feb 16, 2018

GordonTheTurtle added this to the 18.03.0 milestone Feb 16, 2018

dekkagaijin mentioned this pull request Apr 26, 2018

docker builds are slow, hanging on docker-credential-gcloud get GoogleCloudPlatform/docker-credential-gcr#11

Closed

dekkagaijin mentioned this pull request Oct 16, 2019

remove generic credsStore support GoogleCloudPlatform/docker-credential-gcr#64

Merged

jonjohnsonjr mentioned this pull request Feb 20, 2020

Unable to use docker / docker-compose when auth fails GoogleCloudPlatform/docker-credential-gcr#74

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetAll -> Get to retrieve credentials from credential helpers #840

GetAll -> Get to retrieve credentials from credential helpers #840

dekkagaijin commented Jan 26, 2018 •

edited

Loading

codecov-io commented Jan 26, 2018 •

edited

Loading

dnephin left a comment

dnephin Jan 29, 2018

dnephin Jan 29, 2018

dekkagaijin Jan 30, 2018

dekkagaijin commented Jan 30, 2018

dnephin Jan 30, 2018

dnephin Jan 30, 2018

dnephin Jan 30, 2018

dnephin Jan 30, 2018

dnephin commented Jan 30, 2018

dekkagaijin commented Jan 30, 2018

thaJeztah commented Jan 30, 2018

dnephin Jan 30, 2018

dekkagaijin Jan 30, 2018

dnephin Jan 30, 2018

dekkagaijin Jan 30, 2018

dekkagaijin Jan 31, 2018

n4ss Jan 31, 2018

n4ss Jan 31, 2018

dekkagaijin Jan 31, 2018

dnephin left a comment

dnephin Jan 31, 2018

dekkagaijin Jan 31, 2018

dnephin left a comment

dekkagaijin commented Feb 5, 2018

thaJeztah commented Feb 6, 2018

n4ss commented Feb 6, 2018

dekkagaijin commented Feb 6, 2018

dekkagaijin commented Feb 8, 2018

dekkagaijin commented Feb 9, 2018

dekkagaijin commented Feb 15, 2018

vdemeester left a comment

GetAll -> Get to retrieve credentials from credential helpers #840

GetAll -> Get to retrieve credentials from credential helpers #840

Conversation

dekkagaijin commented Jan 26, 2018 • edited Loading

codecov-io commented Jan 26, 2018 • edited Loading

Codecov Report

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin commented Jan 30, 2018

dekkagaijin commented Jan 30, 2018

thaJeztah commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dekkagaijin commented Feb 5, 2018

thaJeztah commented Feb 6, 2018

n4ss commented Feb 6, 2018

dekkagaijin commented Feb 6, 2018

dekkagaijin commented Feb 8, 2018

dekkagaijin commented Feb 9, 2018

dekkagaijin commented Feb 15, 2018

vdemeester left a comment

Choose a reason for hiding this comment

dekkagaijin commented Jan 26, 2018 •

edited

Loading

codecov-io commented Jan 26, 2018 •

edited

Loading