From 3a9f04d4d81dd2cfacf8f5bd139f007964d4bac6 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 31 May 2017 17:44:28 -0400 Subject: [PATCH 1/2] GCR: ignore exceptions when building images Ignore exceptions refreshing the accessToken in ContainerRegistryAuthSupplier when building the image or getting the RegistryAuth to be used for swarm. Since `authForBuild()` has no arguments, we currently try to return an AccessToken to use _in case_ the image being built needs to pull from gcr.io, but we should ignore failures in the case that the image being built does not need to pull anything from gcr.io. This way someone using ContainerRegistryAuthSupplier with credentials that do not actually have access to GCR do not get exceptions when building non-GCR images. If the accessToken can't be fetched in `authForBuild` for a gcr.io image, then the build will still fail but at a later point when the docker daemon tries to pull the FROM image and throws an error about how the image is "missing or access is denied". --- .../gcr/ContainerRegistryAuthSupplier.java | 41 ++++++++++++---- .../ContainerRegistryAuthSupplierTest.java | 48 +++++++++++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplier.java b/src/main/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplier.java index ef010bffc..1a8878812 100644 --- a/src/main/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplier.java +++ b/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 configs = new HashMap<>(GCR_REGISTRIES.size()); for (String serverName : GCR_REGISTRIES) { diff --git a/src/test/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplierTest.java b/src/test/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplierTest.java index d4513e707..c8efd0ab6 100644 --- a/src/test/java/com/spotify/docker/client/gcr/ContainerRegistryAuthSupplierTest.java +++ b/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())); + } } From 023246d4f25d83c4cc03f76856deb002b1a05059 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 31 May 2017 20:30:38 -0400 Subject: [PATCH 2/2] add 773 to changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 486fade35..a30071592 100644 --- a/CHANGELOG.md +++ b/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.