client/driver: use correct repo address when using docker-credential helper #4266
Conversation
nickethier
force-pushed
the
b-3818-gce-docker-cred-helper
branch
from
505f8a1
to
0ca49c0
Compare
nickethier
force-pushed
the
b-3818-gce-docker-cred-helper
branch
from
0ca49c0
to
bbb3d74
Compare
CHANGELOG.md
Outdated
| @@ -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)] | |||
There was a problem hiding this comment.
Can we say what case is fixed? I am assuming it worked for some subset of image specification formats
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
client/driver/docker.go
Outdated
| // Ensure that the HTTPs prefix exists | ||
| if !strings.HasPrefix(repo, "https://") { | ||
| repo = fmt.Sprintf("https://%s", repo) | ||
| repoInfo, err := parseRepositoryInfo(repo) |
client/driver/docker.go
Outdated
| } | ||
|
|
||
| cmd.Stdin = strings.NewReader(repo) | ||
| // Ensure that the HTTPs prefix exists | ||
| repoAddr := fmt.Sprintf("https://%s", repoInfo.Hostname()) |
There was a problem hiding this comment.
is it safe to assume everyone uses https for internal hubs
There was a problem hiding this comment.
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.
client/driver/docker_test.go
Outdated
| require.NoError(t, err) | ||
| defer os.RemoveAll(dir) | ||
| helperPayload := "{\"Username\":\"hashi\",\"Secret\":\"nomad\"}" | ||
| helperContent := []byte(fmt.Sprintf("#!/bin/sh\ncat > %s/helper-$1.out;echo '%s'", dir, helperPayload)) |
There was a problem hiding this comment.
Guard this test from running on windows?
There was a problem hiding this comment.
Do build tags work on tests? Or should I skip if runtime.GOOS == "windows"?
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Uses the server address instead of the image repo reference as the input to the docker-credentials helper.
fixes #3818
fixes #4221