Merge pull request #773 from spotify/mattbrown/ignore-some-gcr-auth-e…

…xceptions

GCR: ignore exceptions when building images

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## 8.6.2 (not yet released)
+
+### Bugfixes
+- ContainerRegistryAuthSupplier should ignore exceptions in refreshing the
+  Access Token unless RegistryAuth info is needed for a GCR image ([773][])
+
+[773]: https://github.com/spotify/docker-client/pull/773
+
 ## 8.6.1
 
 Added NetworkConfig.Attachable.

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

@@ -218,16 +218,11 @@ public void refresh(final GoogleCredentials credentials) throws IOException {
    * Get an accessToken to use, possibly refreshing the token if it expires within the
    * minimumExpiryMillis.
    */
-  private AccessToken getAccessToken() throws DockerException {
+  private AccessToken getAccessToken() throws IOException {
     // synchronize attempts to refresh the accessToken
     synchronized (credentials) {
       if (needsRefresh(credentials.getAccessToken())) {
-        try {
-          credentialRefresher.refresh(credentials);
-        } catch (IOException e) {
-          // rethrow as DockerException to match the signature of the RegistryAuthSupplier methods
-          throw new DockerException("Could not refresh access token", e);
-        }
+        credentialRefresher.refresh(credentials);
       }
     }
     return credentials.getAccessToken();
@@ -254,7 +249,13 @@ public RegistryAuth authFor(final String imageName) throws DockerException {
       return null;
     }
 
-    return authForAccessToken(getAccessToken());
+    final AccessToken accessToken;
+    try {
+      accessToken = getAccessToken();
+    } catch (IOException e) {
+      throw new DockerException(e);
+    }
+    return authForAccessToken(accessToken);
   }
 
   // see https://cloud.google.com/container-registry/docs/advanced-authentication
@@ -267,12 +268,32 @@ private RegistryAuth authForAccessToken(final AccessToken accessToken) {
 
   @Override
   public RegistryAuth authForSwarm() throws DockerException {
-    return authForAccessToken(getAccessToken());
+    final AccessToken accessToken;
+    try {
+      accessToken = getAccessToken();
+    } catch (IOException e) {
+      // ignore the exception, as the user may not care if swarm is authenticated to use GCR
+      log.warn("unable to get access token for Google Container Registry due to exception, "
+               + "configuration for Swarm will not contain RegistryAuth for GCR",
+          e);
+      return null;
+    }
+    return authForAccessToken(accessToken);
   }
 
   @Override
   public RegistryConfigs authForBuild() throws DockerException {
-    final AccessToken accessToken = getAccessToken();
+    final AccessToken accessToken;
+    try {
+      accessToken = getAccessToken();
+    } catch (IOException e) {
+      // do not fail as the GCR access token may not be necessary for building the image currently
+      // being built
+      log.warn("unable to get access token for Google Container Registry, "
+               + "configuration for building image will not contain RegistryAuth for GCR",
+          e);
+      return RegistryConfigs.empty();
+    }
 
     final Map<String, RegistryAuth> configs = new HashMap<>(GCR_REGISTRIES.size());
     for (String serverName : GCR_REGISTRIES) {

src/test/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplierTest.java

@@ -21,10 +21,13 @@
 package com.spotify.docker.client.gcr;
 
 import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -33,16 +36,23 @@
 import com.google.api.client.util.Clock;
 import com.google.auth.oauth2.AccessToken;
 import com.google.auth.oauth2.GoogleCredentials;
+import com.spotify.docker.client.exceptions.DockerException;
 import com.spotify.docker.client.messages.RegistryAuth;
 import com.spotify.docker.client.messages.RegistryConfigs;
+import java.io.IOException;
 import java.util.concurrent.TimeUnit;
 import org.hamcrest.FeatureMatcher;
 import org.hamcrest.Matcher;
 import org.joda.time.DateTime;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 public class ContainerRegistryAuthSupplierTest {
 
+  @Rule
+  public final ExpectedException exception = ExpectedException.none();
+
   private final DateTime expiration = new DateTime(2017, 5, 23, 16, 25);
   private final String tokenValue = "abc123.foobar";
 
@@ -130,6 +140,21 @@ public void testAuthForImage_NonGcrImage() throws Exception {
     assertThat(supplier.authFor("foobar"), is(nullValue()));
   }
 
+  @Test
+  public void testAuthForImage_ExceptionOnRefresh() throws Exception {
+    when(clock.currentTimeMillis())
+        .thenReturn(expiration.minusSeconds(minimumExpirationSecs - 1).getMillis());
+
+    final IOException ex = new IOException("failure!!");
+    doThrow(ex).when(refresher).refresh(credentials);
+
+    // the exception should propagate up
+    exception.expect(DockerException.class);
+    exception.expectCause(is(ex));
+
+    supplier.authFor("gcr.io/example/foobar:1.2.3");
+  }
+
   @Test
   public void testAuthForSwarm_NoRefresh() throws Exception {
     when(clock.currentTimeMillis())
@@ -150,12 +175,23 @@ public void testAuthForSwarm_RefreshNeeded() throws Exception {
     verify(refresher).refresh(credentials);
   }
 
+  @Test
+  public void testAuthForSwarm_ExceptionOnRefresh() throws Exception {
+    when(clock.currentTimeMillis())
+        .thenReturn(expiration.minusSeconds(minimumExpirationSecs - 1).getMillis());
+
+    doThrow(new IOException("failure!!")).when(refresher).refresh(credentials);
+
+    assertThat(supplier.authForSwarm(), is(nullValue()));
+  }
+
   @Test
   public void testAuthForBuild_NoRefresh() throws Exception {
     when(clock.currentTimeMillis())
         .thenReturn(expiration.minusSeconds(minimumExpirationSecs + 1).getMillis());
 
     final RegistryConfigs configs = supplier.authForBuild();
+    assertThat(configs.configs().values(), is(not(empty())));
     assertThat(configs.configs().values(), everyItem(matchesAccessToken(accessToken)));
 
     verify(refresher, never()).refresh(credentials);
@@ -167,8 +203,20 @@ public void testAuthForBuild_RefreshNeeded() throws Exception {
         .thenReturn(expiration.minusSeconds(minimumExpirationSecs - 1).getMillis());
 
     final RegistryConfigs configs = supplier.authForBuild();
+    assertThat(configs.configs().values(), is(not(empty())));
     assertThat(configs.configs().values(), everyItem(matchesAccessToken(accessToken)));
 
     verify(refresher).refresh(credentials);
   }
+
+  @Test
+  public void testAuthForBuild_ExceptionOnRefresh() throws Exception {
+    when(clock.currentTimeMillis())
+        .thenReturn(expiration.minusSeconds(minimumExpirationSecs - 1).getMillis());
+
+    doThrow(new IOException("failure!!")).when(refresher).refresh(credentials);
+
+    final RegistryConfigs configs = supplier.authForBuild();
+    assertThat(configs.configs().values(), is(empty()));
+  }
 }