Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

add RegistryAuthSupplier interface #759

Merged
merged 8 commits into from
May 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,43 @@
# Changelog

## 8.6.0 (not yet released)

### Revamped support for authentication
This version introduces a new way to configure DefaultDockerClient to use
authentication - the RegistryAuthSupplier interface.

Historically, a single RegistryAuth instance was configured in
DefaultDockerClient at construction-time and the instance would be used
throughout the lifetime of the DefaultDockerClient instance. Many of the static
factory methods in the RegistryAuth class would use the first auth element
found in the docker client config file, and a DefaultDockerClient configured
with `dockerAuth(true)` would have this behavior enabled by default.

Inspired by a desire to be able to integrate with pushing and pulling images to
Google Container Registry (where the docker client config file contains
short-lived access tokens), the previous behavior has been removed and is
replaced by RegistryAuthSupplier. DefaultDockerClient will now invoke the
appropriate method on the configured RegistryAuthSupplier instance before each
API operation that requires authentication. This allows for use of
authentication info that is dynamic and changes during the lifetime of the
DefaultDockerClient instance.

The docker-client library contains an implementation of this interface that
returns static RegistryAuth instances (NoOpRegistryAuthSupplier, which is
configured for you if you use the old method `registryAuth(RegistryAuth)` in
DefaultDockerClient.Builder) and an implementation for refreshing GCR access
tokens with `gcloud docker -a`. We suggest that users implement this interface
themselves if there is a need to customize the behavior.

The new class DockerConfigReader replaces the static factory methods from
RegistryAuth.

The following methods are deprecated and will be removed in a future release:
- DefaultDockerClient.Builder.registryAuth(RegistryAuth)
- all overloads of RegistryAuth.fromDockerConfig(...)

[740](https://github.com/spotify/docker-client/issues/740), [759](https://github.com/spotify/docker-client/pull/759)

## 8.5.0 (released May 18 2017)

### Removal of deprecated methods
Expand Down
7 changes: 6 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
</parent>

<artifactId>docker-client</artifactId>
<version>8.5.1-SNAPSHOT</version>
<version>8.6.0-SNAPSHOT</version>
<packaging>jar</packaging>
<name>docker-client</name>
<description>A docker client</description>
Expand Down Expand Up @@ -121,6 +121,11 @@
<artifactId>commons-compress</artifactId>
<version>1.9</version>
</dependency>
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.5</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
Expand Down
75 changes: 50 additions & 25 deletions src/main/java/com/spotify/docker/client/DefaultDockerClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import com.google.common.base.MoreObjects;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
Expand Down Expand Up @@ -97,6 +98,7 @@
import com.spotify.docker.client.messages.NetworkCreation;
import com.spotify.docker.client.messages.ProgressMessage;
import com.spotify.docker.client.messages.RegistryAuth;
import com.spotify.docker.client.messages.RegistryAuthSupplier;
import com.spotify.docker.client.messages.RegistryConfigs;
import com.spotify.docker.client.messages.RemovedImage;
import com.spotify.docker.client.messages.ServiceCreateResponse;
Expand Down Expand Up @@ -324,7 +326,7 @@ public void progress(ProgressMessage message) throws DockerException {

private final URI uri;
private final String apiVersion;
private final RegistryAuth registryAuth;
private final RegistryAuthSupplier registryAuthSupplier;

private final Map<String, Object> headers;

Expand Down Expand Up @@ -398,7 +400,12 @@ protected DefaultDockerClient(final Builder builder) {
.property(ApacheClientProperties.CONNECTION_MANAGER, cm)
.property(ApacheClientProperties.REQUEST_CONFIG, requestConfig);

this.registryAuth = builder.registryAuth;

if (builder.registryAuthSupplier == null) {
this.registryAuthSupplier = new NoOpRegistryAuthSupplier();
} else {
this.registryAuthSupplier = builder.registryAuthSupplier;
}

this.client = ClientBuilder.newBuilder()
.withConfig(config)
Expand Down Expand Up @@ -1165,17 +1172,16 @@ public InputStream saveMultiple(final String... images)
throws DockerException, IOException, InterruptedException {

final WebTarget resource = resource().path("images").path("get");
if (images != null) {
for (final String image : images) {
resource.queryParam("names", urlEncode(image));
}
for (final String image : images) {
resource.queryParam("names", urlEncode(image));
}

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])))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 am not sure, but the existing code has it.

);
}

Expand All @@ -1187,7 +1193,7 @@ public void pull(final String image) throws DockerException, InterruptedExceptio
@Override
public void pull(final String image, final ProgressHandler handler)
throws DockerException, InterruptedException {
pull(image, registryAuth, handler);
pull(image, registryAuthSupplier.authFor(image), handler);
}

@Override
Expand Down Expand Up @@ -1241,7 +1247,7 @@ public void push(final String image, final RegistryAuth registryAuth)
@Override
public void push(final String image, final ProgressHandler handler)
throws DockerException, InterruptedException {
push(image, handler, registryAuth);
push(image, handler, registryAuthSupplier.authFor(image));
}

@Override
Expand Down Expand Up @@ -1360,18 +1366,7 @@ public String build(final Path directory, final String name, final String docker
}

// Convert auth to X-Registry-Config format
final RegistryConfigs registryConfigs;
if (registryAuth == null) {
registryConfigs = RegistryConfigs.empty();
} else {
registryConfigs = RegistryConfigs.create(singletonMap(
registryAuth.serverAddress(),
RegistryConfigs.RegistryConfig.create(
registryAuth.serverAddress(),
registryAuth.username(),
registryAuth.password(),
registryAuth.email())));
}
final RegistryConfigs registryConfigs = registryAuthSupplier.authForBuild();

try (final CompressedDirectory compressedDirectory = CompressedDirectory.create(directory);
final InputStream fileStream = Files.newInputStream(compressedDirectory.file());
Expand Down Expand Up @@ -1784,8 +1779,7 @@ public void unlock(final UnlockKey unlockKey) throws DockerException, Interrupte
@Override
public ServiceCreateResponse createService(ServiceSpec spec)
throws DockerException, InterruptedException {

return createService(spec, registryAuth);
return createService(spec, registryAuthSupplier.authForSwarm());
}

@Override
Expand Down Expand Up @@ -2514,6 +2508,10 @@ public static Builder fromEnv() throws DockerCertificateException {

public static class Builder {

public static final String ERROR_MESSAGE =
"LOGIC ERROR: DefaultDockerClient does not support being built "
+ "with both `registryAuth` and `registryAuthSupplier`. "
+ "Please build with at most one of these options.";
private URI uri;
private String apiVersion;
private long connectTimeoutMillis = DEFAULT_CONNECT_TIMEOUT_MILLIS;
Expand All @@ -2522,6 +2520,7 @@ public static class Builder {
private DockerCertificatesStore dockerCertificatesStore;
private boolean dockerAuth;
private RegistryAuth registryAuth;
private RegistryAuthSupplier registryAuthSupplier;
private Map<String, Object> headers = new HashMap<>();

public URI uri() {
Expand Down Expand Up @@ -2632,7 +2631,9 @@ public boolean dockerAuth() {
*
* @param dockerAuth tells if Docker auth info should be used
* @return Builder
* @deprecated in favor of {@link #registryAuthSupplier(RegistryAuthSupplier)}
*/
@Deprecated
public Builder dockerAuth(final boolean dockerAuth) {
this.dockerAuth = dockerAuth;
return this;
Expand All @@ -2647,16 +2648,40 @@ public RegistryAuth registryAuth() {
*
* @param registryAuth RegistryAuth object
* @return Builder
*
* @deprecated in favor of {@link #registryAuthSupplier(RegistryAuthSupplier)}
*/
@Deprecated
public Builder registryAuth(final RegistryAuth registryAuth) {
if (this.registryAuthSupplier != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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'd like to remove all the deprecated methods in a future release - see what I added to Changelog

throw new IllegalStateException(ERROR_MESSAGE);
}
this.registryAuth = registryAuth;

// stuff the static RegistryAuth into a RegistryConfigs instance to maintain what
// DefaultDockerClient used to do with the RegistryAuth before we introduced the
// RegistryAuthSupplier
final RegistryConfigs configs = RegistryConfigs.create(singletonMap(
MoreObjects.firstNonNull(registryAuth.serverAddress(), ""),
registryAuth
));

this.registryAuthSupplier = new NoOpRegistryAuthSupplier(registryAuth, configs);
return this;
}

public Builder registryAuthSupplier(final RegistryAuthSupplier registryAuthSupplier) {
if (this.registryAuthSupplier != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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 would remove the registryAuth field from the Builder altogether but there is a registryAuth() getter for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

throw new IllegalStateException(ERROR_MESSAGE);
}
this.registryAuthSupplier = registryAuthSupplier;
return this;
}

public DefaultDockerClient build() {
if (dockerAuth) {
if (dockerAuth && registryAuthSupplier == null && registryAuth == null) {
try {
this.registryAuth = RegistryAuth.fromDockerConfig().build();
registryAuth(RegistryAuth.fromDockerConfig().build());
} catch (IOException e) {
log.warn("Unable to use Docker auth info", e);
}
Expand Down
119 changes: 119 additions & 0 deletions src/main/java/com/spotify/docker/client/DockerConfigReader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*-
* -\-\-
* docker-client
* --
* Copyright (C) 2016 Spotify AB
* --
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
* -/-/-
*/

package com.spotify.docker.client;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Strings.isNullOrEmpty;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.base.Preconditions;
import com.spotify.docker.client.messages.RegistryAuth;
import com.spotify.docker.client.messages.RegistryConfigs;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Map;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class DockerConfigReader {
private static final Logger LOG = LoggerFactory.getLogger(DockerConfigReader.class);

private static final ObjectMapper MAPPER = ObjectMapperProvider.objectMapper();

/** Returns all RegistryConfig instances from the configuration file. */
public RegistryConfigs fromConfig(Path configPath) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 this was done to make it easy to mock it

return parseDockerConfig(configPath);
}

public RegistryAuth fromConfig(Path configPath, String serverAddress) throws IOException {
return parseDockerConfig(configPath, serverAddress);
}

/**
* @deprecated do not use - only exists for backwards compatibility. Use {@link #fromConfig(Path)}
* instead.
*/
@Deprecated
public RegistryAuth fromFirstConfig(Path configPath) throws IOException {
return parseDockerConfig(configPath, null);
}

private RegistryAuth parseDockerConfig(final Path configPath, String serverAddress)
throws IOException {
checkNotNull(configPath);

final Map<String, RegistryAuth> configs = parseDockerConfig(configPath).configs();
if (serverAddress != null && configs.containsKey(serverAddress) ) {
return configs.get(serverAddress);
}

if (isNullOrEmpty(serverAddress)) {
if (configs.isEmpty()) {
return RegistryAuth.builder().build();
}
LOG.warn("Returning first entry from docker config file - use fromConfig(Path) instead, "
+ "this behavior is deprecated and will soon be removed");
return configs.values().iterator().next();
}

throw new IllegalArgumentException(
"serverAddress=" + serverAddress + " does not appear in config file at " + configPath);
}

private RegistryConfigs parseDockerConfig(final Path configPath) throws IOException {
checkNotNull(configPath);
return MAPPER.treeToValue(extractAuthJson(configPath), RegistryConfigs.class);
}

public Path defaultConfigPath() {
final String home = System.getProperty("user.home");
final Path dockerConfig = Paths.get(home, ".docker", "config.json");
final Path dockerCfg = Paths.get(home, ".dockercfg");

if (Files.exists(dockerConfig)) {
LOG.debug("Using configfile: {}", dockerConfig);
return dockerConfig;
} else {
LOG.debug("Using configfile: {} ", dockerCfg);
return dockerCfg;
}
}

private ObjectNode extractAuthJson(final Path configPath) throws IOException {
final JsonNode config = MAPPER.readTree(configPath.toFile());

Preconditions.checkState(config.isObject(),
"config file contents are not a JSON Object, instead it is a %s", config.getNodeType());

if (config.has("auths")) {
final JsonNode auths = config.get("auths");
Preconditions.checkState(auths.isObject(),
"config file contents are not a JSON Object, instead it is a %s", auths.getNodeType());
return (ObjectNode) auths;
}

return (ObjectNode) config;
}
}
Loading