add RegistryAuthSupplier interface #759

mattnworb · 2017-05-22T19:30:17Z

This completes #749 - David and I worked on this together in-person.

The new RegistryAuthSupplier interface replaces the existing RegistryAuth support in DefaultDockerClient so that users can customize the authentication behavior much more closely. On each operation that requires authentication, DefaultDockerClient will invoke the appropriate method on the RegistryAuthSupplier instance, allowing the implementation to supply up-to-date authentication information/tokens.

This change is inspired by our attempts to integrate with Google Container Registry, where ~/.docker/config.json file contains short-lived access tokens that expire shortly after creation. This means that a long-lived DefaultDockerClient instance cannot store static RegistryAuth information from construction-time.

Also included is an implementation of RegistryAuthSupplier that refreshes the access tokens supplied by gcloud docker -a prior to each authFor(..) operation, as well as an implementation of RegistryAuthSupplier that returns static data.

Closes #740.

mattnworb · 2017-05-22T19:35:59Z

@davidxia @mavenraven

one thing we decided to punt on for the moment is customizing the GCR RegistryAuthSupplier to be able to do something like gcloud --account <accountname> docker ... to allow the user to choose which account/credentials to use. What is here now uses the "default" credentialed account in gcloud, which seems to static to rely on long-term. We'll come back to this soon, in a new PR.

mavenraven · 2017-05-22T21:08:09Z

src/main/java/com/spotify/docker/client/NoOpRegistryAuthSupplier.java

+import com.spotify.docker.client.messages.RegistryConfigs;
+
+/**
+ * Wraps a RegistryAuth with the RegisrtyAuthSupplier interface.


fixed, thanks

mavenraven · 2017-05-22T21:11:51Z

👍

codecov-io · 2017-05-23T15:39:22Z

Codecov Report

Merging #759 into master will increase coverage by 0.32%.
The diff coverage is 69.42%.

@@             Coverage Diff              @@
##             master     #759      +/-   ##
============================================
+ Coverage     65.56%   65.89%   +0.32%     
- Complexity      656      672      +16     
============================================
  Files           153      158       +5     
  Lines          2878     2926      +48     
  Branches        335      335              
============================================
+ Hits           1887     1928      +41     
- Misses          844      850       +6     
- Partials        147      148       +1

davidxia

Didn't take that close of a look at tests though.

davidxia · 2017-05-23T18:07:00Z

src/main/java/com/spotify/docker/client/DefaultDockerClient.java

    }

    return request(
        GET,
        InputStream.class,
        resource,
-        resource.request(APPLICATION_JSON_TYPE).header("X-Registry-Auth", authHeader(registryAuth))
+        resource.request(APPLICATION_JSON_TYPE).header("X-Registry-Auth", authHeader(
+            registryAuthSupplier.authFor(images[0])))


Does this endpoint need auth at all? Docs don't mention it.

I am not sure, but the existing code has it.

davidxia · 2017-05-23T18:25:17Z

src/main/java/com/spotify/docker/client/DefaultDockerClient.java

    public Builder registryAuth(final RegistryAuth registryAuth) {
+      if (this.registryAuthSupplier != null) {


Will we keep this logic or remove registryAuth() at some point?

I'd like to remove all the deprecated methods in a future release - see what I added to Changelog

davidxia · 2017-05-23T18:40:02Z

src/main/java/com/spotify/docker/client/gcr/GoogleContainerRegistryAuthSupplier.java

+        new DockerConfigReader().defaultConfigPath());
+  }
+
+  public GoogleContainerRegistryAuthSupplier(DockerConfigReader dockerCfgReader,


nit: can we put each param on a new line?

davidxia · 2017-05-23T18:46:45Z

src/main/java/com/spotify/docker/client/DockerConfigReader.java

+  private static final ObjectMapper MAPPER = ObjectMapperProvider.objectMapper();
+
+  /** Returns all RegistryConfig instances from the configuration file. */
+  public RegistryConfigs fromConfig(Path configPath) throws IOException {


Why not make these all static methods? Then you remove a call to the constructor at call sites.

I think this was done to make it easy to mock it

davidxia · 2017-05-23T18:49:53Z

src/main/java/com/spotify/docker/client/messages/RegistryAuth.java

-
-  private static final String DEFAULT_REGISTRY = "https://index.docker.io/v1/";
-  private static final String DUMMY_EMAIL = "1234@5678.com";
-
  @SuppressWarnings("FieldCanBeLocal")
  private static final ObjectMapper MAPPER =


davidxia · 2017-05-23T18:50:37Z

src/main/java/com/spotify/docker/client/messages/RegistryConfigs.java

-  public abstract ImmutableMap<String, RegistryConfig> configs();
-
-  @AutoValue
-  public abstract static class RegistryConfig {


Nice. Why did I add these in the first place?!

Adds support for RegistryAuthSupplier. Client code can consume this using DefaultDockerClient builder. Provides support for authenticating against GCR to allow pushes/pulls from GCR.

the Build method in the Docker Remote API allows for passing *all* of the RegistryAuth structs that the client knows about, this updates the RegistryAuthSupplier interface to match that.

use the same method for parsing the config file in both code paths

and bump library version to 8.6.0 as significant new functionality is added/deprecated here

testAuth and testBadAuth is failing on Docker 1.9 and 1.10 because RegistryAuth.email is not set when we send it to the daemon

johnflavin · 2017-05-23T19:40:38Z

src/main/java/com/spotify/docker/client/DefaultDockerClient.java

+    }
+
+    public Builder registryAuthSupplier(final RegistryAuthSupplier registryAuthSupplier) {
+      if (this.registryAuthSupplier != null) {


Shouldn't this check if(this.registryAuth != null)? Right now this would only get triggered if the user called builder().registryAuthSupplier(registryAuthSupplier).registryAuthSupplier(registryAuthSupplier).

Builder.registryAuth(RegistryAuth) is setting this.registryAuthSupplier to a non-null value

I would remove the registryAuth field from the Builder altogether but there is a registryAuth() getter for some reason

davidxia · 2017-05-23T22:35:08Z

src/main/java/com/spotify/docker/client/gcr/GoogleContainerRegistryAuthSupplier.java

+    try {
+      String registryName = "https://" + imageName.split("/")[0];
+      refresher.refresh();
+      return reader.fromConfig(configPath, registryName);


I wanted to pull busybox:latest and push to gcr.io/myproject/busybox:latest and realized I have to use two DockerClients to do this or implement my own RegistryAuthSupplier that checks if the image name starts with gcr.io/.

final DefaultDockerClient docker = DefaultDockerClient.fromEnv() .registryAuthSupplier(new GoogleContainerRegistryAuthSupplier()) .build(); docker.pull("busybox:latest"); docker.tag("busybox:latest", "gcr.io/myproject/busybox:latest"); docker.push("gcr.io/myproject/busybox:latest");

will throw

java.lang.IllegalArgumentException: serverAddress=https://busybox:latest does not appear in config file at /Users/dxia/.docker/config.json at com.spotify.docker.client.DockerConfigReader.parseDockerConfig(DockerConfigReader.java:81) at com.spotify.docker.client.DockerConfigReader.fromConfig(DockerConfigReader.java:51) at com.spotify.docker.client.gcr.GoogleContainerRegistryAuthSupplier.authFor(GoogleContainerRegistryAuthSupplier.java:58) at com.spotify.docker.client.DefaultDockerClient.pull(DefaultDockerClient.java:1196) at com.spotify.docker.client.DefaultDockerClient.pull(DefaultDockerClient.java:1190) at com.spotify.gcpprojectcreator.gcloud.IamPolicyHandlerTest.foo(IamPolicyHandlerTest.java:160) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

Is this intended?

Good catch, this is not intended.

I think this refactoring requires a few more changes before it is really complete and can be released. I wanted to merge what we have so far to avoid the PR growing gigantic. I'll fix this and I'm also close to being able to run this code locally in helios with GCR.

forgot to do this in #759 and #762

mattnworb mentioned this pull request May 22, 2017

Adds support for RegistryAuthSupplier #749

Closed

mattnworb force-pushed the mattbrown/registry-auth-supplier branch 3 times, most recently from c5c2236 to 8fea305 Compare May 22, 2017 20:59

mavenraven approved these changes May 22, 2017

View reviewed changes

mattnworb force-pushed the mattbrown/registry-auth-supplier branch 2 times, most recently from adf8d21 to b6d531d Compare May 23, 2017 13:40

mattnworb force-pushed the mattbrown/registry-auth-supplier branch from 2f67e16 to b43336d Compare May 23, 2017 16:53

davidxia approved these changes May 23, 2017

View reviewed changes

David Morhovich and others added 7 commits May 23, 2017 15:16

Adds support for RegistryAuthSupplier.

675cf08

Adds support for RegistryAuthSupplier. Client code can consume this using DefaultDockerClient builder. Provides support for authenticating against GCR to allow pushes/pulls from GCR.

add authForBuild() method to RegistryAuthSupplier

c47cb5c

the Build method in the Docker Remote API allows for passing *all* of the RegistryAuth structs that the client knows about, this updates the RegistryAuthSupplier interface to match that.

consolidate config file parsing in DockerConfigReader

e0c978c

use the same method for parsing the config file in both code paths

update CHANGELOG about RegistryAuthSupplier

1621c36

and bump library version to 8.6.0 as significant new functionality is added/deprecated here

ensure build() works as expected when dockerAuth is true

1b513bf

fix Findbugs warning in RegistryConfigs

9cd8837

ensure RegistryAuth email is set in tests

246f6a1

testAuth and testBadAuth is failing on Docker 1.9 and 1.10 because RegistryAuth.email is not set when we send it to the daemon

mattnworb force-pushed the mattbrown/registry-auth-supplier branch from 652ba48 to 9d65790 Compare May 23, 2017 19:18

johnflavin reviewed May 23, 2017

View reviewed changes

RegistryAuth: add method for building from auth token

073ad23

mattnworb force-pushed the mattbrown/registry-auth-supplier branch from 9d65790 to 073ad23 Compare May 23, 2017 19:42

mattnworb merged commit f3233d1 into master May 23, 2017

mattnworb deleted the mattbrown/registry-auth-supplier branch May 23, 2017 20:22

davidxia reviewed May 23, 2017

View reviewed changes

mattnworb mentioned this pull request May 25, 2017

RegistryAuth: handle case where password contains colon #765

Merged

mattnworb added a commit that referenced this pull request Jun 5, 2017

document RegistryAuthSupplier in the user manual

dba55b1

forgot to do this in #759 and #762

mattnworb mentioned this pull request Jun 5, 2017

document RegistryAuthSupplier in the user manual #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add RegistryAuthSupplier interface #759

add RegistryAuthSupplier interface #759

mattnworb commented May 22, 2017

mattnworb commented May 22, 2017

mavenraven May 22, 2017

mattnworb May 23, 2017

mavenraven commented May 22, 2017

codecov-io commented May 23, 2017 •

edited

Loading

davidxia left a comment

davidxia May 23, 2017

mattnworb May 23, 2017

davidxia May 23, 2017

mattnworb May 23, 2017

davidxia May 23, 2017

davidxia May 23, 2017

mattnworb May 23, 2017

davidxia May 23, 2017

davidxia May 23, 2017

johnflavin May 23, 2017

mattnworb May 23, 2017

mattnworb May 23, 2017

johnflavin May 23, 2017

davidxia May 23, 2017 •

edited

Loading

mattnworb May 23, 2017

		public Builder registryAuth(final RegistryAuth registryAuth) {
		if (this.registryAuthSupplier != null) {

add RegistryAuthSupplier interface #759

add RegistryAuthSupplier interface #759

Conversation

mattnworb commented May 22, 2017

mattnworb commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenraven commented May 22, 2017

codecov-io commented May 23, 2017 • edited Loading

Codecov Report

davidxia 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidxia May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 23, 2017 •

edited

Loading

davidxia May 23, 2017 •

edited

Loading