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

Added docker_config_json parameter #317

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

markround
Copy link

This means you can specify a full ~/.docker/config.json file to use for
authentication, which is useful to work around Docker Hub's rate-limiting.

You can pass this in (e.g. from a secrets source such as Credhub or k8s
secrets), and authenticate against multiple registries.

Also added GODEBUG="x509ignoreCN=0" env var to work around "legacy CN"
format SSL certificates warning, introduced with latest version of Go SSL
library.

This means you can specify a full ~/.docker/config.json file to use for
authentication, which is useful to work around Docker Hub's rate-limiting.

You can pass this in (e.g. from a secrets source such as Credhub or k8s
secrets), and authenticate against multiple registries.

Also added GODEBUG="x509ignoreCN=0" env var to work around "legacy CN"
format SSL certificates warning, introduced with latest version of Go SSL
library.
@markround markround requested a review from xtremerui as a code owner November 24, 2020 09:29
@markround
Copy link
Author

This is a work-around for #278 which has recently bitten me due to Docker Hub's rate-limiting. I needed to be able to drop a raw docker config.json in so I can authenticate against multiple registries e.g. log into Docker Hub so I don't get rate-limited when pulling base images, but also authenticate against my target private registry.

Hope this helps someone in any case!

@markround markround changed the title Added docker_config_json paraneter Added docker_config_json parameter Nov 24, 2020
@xtremerui
Copy link
Contributor

Hi, thx for the PR. Have you tried https://github.com/concourse/registry-image-resource? We encourage users to move to registry-image-resource rather than adding new features to this resource type (though this PR falls between new feature and bug fix as the change is introduced from docker itself)

If that works in your case I'd rather hold on this PR since all your credetials will be stored plain text in the config file, refer to #243

@markround
Copy link
Author

@xtremerui The registry-image-resource doesn't really work for me, as it doesn't deal with building containers using public base images. Which is where I was being badly bitten by Docker Hub's rate-limiting when not authenticating as I'm behind CGNAT and presumably sharing my public IP with 100s/1000s of other users.

I could I guess hook registry-image up to the oci-build resource but that's somewhat in flux at the moment and doesn't really offer anything at the moment over this resource as far as I can see (e.g. it still requires privileged: true, although I think that's planned to change in the future).

I understand the hesitation to merge given the config file being written to disk, but then as far as I understand it, that's still what happens with the "docker login" approach. Although I agree it's not best practice to leave credentials floating around on disk.

I'm fine with whatever you choose to do though :)

The fact that this PR exists and is linked in the associated issue is enough for me, hopefully it'll help someone else with a workaround in the worst-case scenario.

@@ -27,6 +27,8 @@ Note: docker registry must be [v2](https://docs.docker.com/registry/spec/api/).
* `aws_access_key_id`: *Optional.* AWS access key to use for acquiring ECR
credentials.

* `docker_config_json` : *Optional.* The raw `config.json` file used for authenticating with Docker registries. If specified, `username` and `password` parameters will be ignored. You may find this useful if you need to be authenticated against multiple registries (e.g. pushing to a private registry, but you also also need to pull authenticate to pull images from Docker Hub without being rate-limited).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `docker_config_json` : *Optional.* The raw `config.json` file used for authenticating with Docker registries. If specified, `username` and `password` parameters will be ignored. You may find this useful if you need to be authenticated against multiple registries (e.g. pushing to a private registry, but you also also need to pull authenticate to pull images from Docker Hub without being rate-limited).
* `docker_config_json` : *Optional.* The raw `config.json` file used for authenticating with Docker registries. If specified, `username` and `password` parameters will be ignored. You may find this useful if you need to be authenticated against multiple registries (e.g. pushing to a private registry, but you also need to pull authenticate to pull images from Docker Hub without being rate-limited).

log_in "$username" "$password" "$registry"
else
docker_config_json_to_file "$docker_config_json"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an out test to cover this feature?

@@ -2,6 +2,9 @@ LOG_FILE=${LOG_FILE:-/tmp/docker.log}
SKIP_PRIVILEGED=${SKIP_PRIVILEGED:-false}
STARTUP_TIMEOUT=${STARTUP_TIMEOUT:-120}

# Otherwise we get "certificate relies on legacy Common Name field"
export GODEBUG="x509ignoreCN=0"
Copy link
Contributor

Choose a reason for hiding this comment

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

In what user case that you need this env var? Are you seeing the error when check/in/out?

From what I understand in this resource, only the check cmd is using Go, so in/out that uses bash script should not be affected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants