WIP: Docker authentiation using credential store/helpers #647
Conversation
CHANGELOG.md
Outdated
| @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. | |||
| - Fixed missing `commons-codec` dependency ([\#642](https://github.com/testcontainers/testcontainers-java/issues/642)) | |||
| - Fixed `HostPortWaitStrategy` throws `NumberFormatException` when port is exposed but not mapped ([\#640](https://github.com/testcontainers/testcontainers-java/issues/640)) | |||
| - Fixed log processing: multibyte unicode, linebreaks and ASCII color codes. Color codes can be turned on with `withRemoveAnsiCodes(false)` ([PR \#643](https://github.com/testcontainers/testcontainers-java/pull/643)) | |||
| - Add support for private repositories using docker credential stores/helpers (fixes [\#567](https://github.com/testcontainers/testcontainers-java/issues/567)) | |||
There was a problem hiding this comment.
aren't we link PRs here? :) Maybe next to "fixes" part, useful to check what was changed
| * @param defaultAuthConfig an AuthConfig object that should be returned if there is no overriding authentication | ||
| * available for images that are looked up | ||
| */ | ||
| public RegistryAuthLocator(AuthConfig defaultAuthConfig) { |
There was a problem hiding this comment.
IMO this ctor should call
this(defaultAuthConfig, new File(System.getProperty("user.home") + "/.docker/config.json"), "")
| */ | ||
| public RegistryAuthLocator(AuthConfig defaultAuthConfig) { | ||
| this.defaultAuthConfig = defaultAuthConfig; | ||
| this.configFile = new File(System.getProperty("user.home") + "/.docker/config.json"); |
There was a problem hiding this comment.
it seems that the location is configurable. Not sure we can detect it tho
There was a problem hiding this comment.
Well... I guess we can do a best-efforts detection of the location using the DOCKER_CONFIG env var. I'll add that.
There was a problem hiding this comment.
See also DefaultDockerClientConfig::dockerConfigPath field
|
|
||
| private AuthConfig authConfigUsingCredentialsStoreOrHelper(String hostName, JsonNode config) throws Exception { | ||
|
|
||
| final String credsStoreName = config.at("/credsStore").asText(); |
There was a problem hiding this comment.
FYI asText() returns "null" instead of null when it's null :)
Also, config.path("/credsStore") should be faster (doesn't parse the json path, but keeps the null safety by returning a missing node), but not critical here :)
rnorth
force-pushed
the
credential-helpers
branch
2 times, most recently
from
eb5558b
to
c8dfeee
Compare
rnorth
force-pushed
the
credential-helpers
branch
2 times, most recently
from
5c8f9e6
to
56217da
Compare
|
I'll try and test this on Windows tonight, then I think we should be good to go. |
rnorth
force-pushed
the
credential-helpers
branch
from
56217da
to
b9a64ea
Compare
rnorth
force-pushed
the
credential-helpers
branch
from
f9372d2
to
fcd5053
Compare
|
I've added the
|
|
@rnorth Remember our discussion about containerised Compose? Maybe this is a great moment to finally make a decision and remove it? :) |
|
I'd be fine with removing dockerized compose, it promises to be easy for user, but it adds a lot of additional hidden complexity, which can lead to strange edge cases. I can test this on Windows this weekend though :) |
| final String reposName = dockerImageName.getRegistry(); | ||
| final JsonNode auths = config.at("/auths/" + reposName); | ||
|
|
||
| if (!auths.isMissingNode() && auths.size() == 0) { |
There was a problem hiding this comment.
I've tested this with Docker-for-Mac, works perfectly fine if changed to || here.
There was a problem hiding this comment.
I wonder if !auths.isMissingNode() is needed here, maybe just auths.size() == 0 is enough?
There was a problem hiding this comment.
Checked Jackson source code, should be safe to leave size() == 0 only, but I do not think it matters that much, leaving both of them should be fine too.
|
Small comment on how to make it work with Docker-for-Mac, other than that works like charm! So, gentle ping to continue the progress :) |
|
I tried to test this on Windows myself, but seems like Docker-for-Windows does not work on official |
|
Closed in favour of #729 |
See also #647 for previous discussions.
Not quite ready for review yet, but nearly.
This change adds support for docker's credential store/helper mechanism.
The main gap at the moment is Windows support; it may work, but I've not tried it. The tests so far definitely don't work on Windows.