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.

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

client/driver: use correct repo address when using docker-credential helper #4266

Merged
merged 5 commits into from
May 15, 2018
Merged

client/driver: use correct repo address when using docker-credential helper #4266

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bbb3d74
client/driver: use correct repo address when using docker-credential …
nickethier May 8, 2018
e99304e
changelog: ref PR instead of individual issues
nickethier May 10, 2018
46c557c
client/driver: add test for docker auth helper
nickethier May 10, 2018
d3c138e
client/driver: parse repo instead of attempting to pull repo info
nickethier May 10, 2018
6e86c4e
client/driver: gaurd authHelper test from running on windows
nickethier May 14, 2018
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: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ IMPROVEMENTS:
* env: Default interpolation of optional meta fields of parameterized jobs to
an empty string rather than the field key. [[GH-3720](https://github.com/hashicorp/nomad/issues/3720)]

BUG FIXES:
* driver/docker: Fix docker credential helper support [[GH-3818](https://github.com/hashicorp/nomad/issues/3818)] [[GH-4221](https://github.com/hashicorp/nomad/issues/4221)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say what case is fixed? I am assuming it worked for some subset of image specification formats

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a fluke that it worked for gcr as the implementation didn’t follow the current spec. Maybe it changed along the way? I don’t think it’s used much outside of GCE.

Copy link
Contributor

@dadgar dadgar May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Okay the two I know of are GCR and AWS:
https://github.com/awslabs/amazon-ecr-credential-helper
https://github.com/GoogleCloudPlatform/docker-credential-gcr

Also we should like to only GH-4266. This PR should mark those two issues as closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it looks like the aws helper's regex doesn't care whats after the hostname.

GCR is using URL.Parse and verifying the Host is a part of GCR.

Copy link
Member Author

Choose a reason for hiding this comment

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

the gcloud helper which i just found out is actually an alias into the google cloud sdk expects the url to be one of these


## 0.8.3 (April 27, 2018)

BUG FIXES:
Expand Down
10 changes: 6 additions & 4 deletions client/driver/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2130,13 +2130,15 @@ func authFromHelper(helperName string) authBackend {
helper := dockerAuthHelperPrefix + helperName
cmd := exec.Command(helper, "get")

// Ensure that the HTTPs prefix exists
if !strings.HasPrefix(repo, "https://") {
repo = fmt.Sprintf("https://%s", repo)
repoInfo, err := parseRepositoryInfo(repo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get a test

if err != nil {
return nil, err
}

cmd.Stdin = strings.NewReader(repo)
// Ensure that the HTTPs prefix exists
repoAddr := fmt.Sprintf("https://%s", repoInfo.Hostname())
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to assume everyone uses https for internal hubs

Copy link
Member Author

Choose a reason for hiding this comment

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

This is whats passed to the helper. The docs only ever show examples with https, but even some of the major cloud registry helpers like AWS ECR and Google GCR strip the https if it exists comming in and do the look up based solely on the domain name.

https://github.com/awslabs/amazon-ecr-credential-helper/blob/master/ecr-login/api/client.go#L41-L43

https://github.com/GoogleCloudPlatform/docker-credential-gcr/blob/master/credhelper/helper.go#L252:16


cmd.Stdin = strings.NewReader(repoAddr)
output, err := cmd.Output()
if err != nil {
switch err.(type) {
Expand Down