From e52bbee44572862c3275839b712755deda0f39ed Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Tue, 3 Sep 2019 18:46:31 -0400 Subject: [PATCH 1/2] Do not fall back to HTTP for connection timeout --- .../tools/jib/builder/steps/StepsRunner.java | 40 +++++++++---------- .../registry/RegistryBrokenPipeException.java | 12 ++++-- .../jib/registry/RegistryEndpointCaller.java | 18 ++++++++- .../registry/RegistryNoResponseException.java | 4 +- .../registry/RegistryEndpointCallerTest.java | 20 ++++++++++ 5 files changed, 66 insertions(+), 28 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java b/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java index 42666842e5..647f0cf512 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java @@ -165,26 +165,6 @@ public StepsRunner registryPushSteps() { return this; } - private void addRetrievalSteps(boolean layersRequiredLocally) { - ImageConfiguration baseImageConfiguration = buildConfiguration.getBaseImageConfiguration(); - - if (baseImageConfiguration.getTarPath().isPresent()) { - // If tarPath is present, a TarImage was used - results.tarPath = Futures.immediateFuture(baseImageConfiguration.getTarPath().get()); - stepsToRun.add(this::extractTar); - - } else if (baseImageConfiguration.getDockerClient().isPresent()) { - // If dockerClient is present, a DockerDaemonImage was used - stepsToRun.add(this::saveDocker); - stepsToRun.add(this::extractTar); - - } else { - // Otherwise default to RegistryImage - stepsToRun.add(this::pullBaseImage); - stepsToRun.add(() -> obtainBaseImageLayers(layersRequiredLocally)); - } - } - public BuildResult run() throws ExecutionException, InterruptedException { Preconditions.checkNotNull(rootProgressDescription); @@ -205,6 +185,26 @@ public BuildResult run() throws ExecutionException, InterruptedException { } } + private void addRetrievalSteps(boolean layersRequiredLocally) { + ImageConfiguration baseImageConfiguration = buildConfiguration.getBaseImageConfiguration(); + + if (baseImageConfiguration.getTarPath().isPresent()) { + // If tarPath is present, a TarImage was used + results.tarPath = Futures.immediateFuture(baseImageConfiguration.getTarPath().get()); + stepsToRun.add(this::extractTar); + + } else if (baseImageConfiguration.getDockerClient().isPresent()) { + // If dockerClient is present, a DockerDaemonImage was used + stepsToRun.add(this::saveDocker); + stepsToRun.add(this::extractTar); + + } else { + // Otherwise default to RegistryImage + stepsToRun.add(this::pullBaseImage); + stepsToRun.add(() -> obtainBaseImageLayers(layersRequiredLocally)); + } + } + private void retrieveTargetRegistryCredentials() { ProgressEventDispatcher.Factory childProgressDispatcherFactory = Verify.verifyNotNull(rootProgressDispatcher).newChildProducer(); diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java index 76508c4184..2a1cf83109 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java @@ -21,11 +21,15 @@ /** Thrown when the registry shut down the connection. */ class RegistryBrokenPipeException extends RegistryException { - RegistryBrokenPipeException(Throwable cause) { + RegistryBrokenPipeException(String registry, String repository, Throwable cause) { super( - "I/O error due to broken pipe: the server shut down the connection. " - + "Check the server log if possible. This could also be a proxy issue. For example," - + "a proxy may prevent sending packets that are too large.", + "I/O error due to broken pipe from " + + registry + + "/" + + repository + + ": the server shut down the connection. Check the server log if possible. This could " + + "also be a proxy issue. For example, a proxy may prevent sending packets that are " + + "too large.", cause); } } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java index 0300879379..d2b90891db 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java @@ -175,6 +175,14 @@ private T callWithAllowInsecureRegistryHandling(URL url) throws IOException, Reg return handleUnverifiableServerException(url); } catch (ConnectException ex) { + // It is observed that Open/Oracle JDKs sometimes throw SocketTimeoutException but other times + // ConnectException for connection timeout. (Could be a JDK bug.) Note SocketTimeoutException + // does not extend ConnectException, and we want to be consistent to error out on timeouts. + // See https://github.com/GoogleContainerTools/jib/issues/1895#issuecomment-527544094 + if (ex.getMessage() != null && ex.getMessage().contains("timed out")) { + throw ex; + } + if (allowInsecureRegistries && isHttpsProtocol(url) && url.getPort() == -1) { // Fall back to HTTP only if "url" had no port specified (i.e., we tried the default HTTPS // port 443) and we could not connect to 443. It's worth trying port 80. @@ -297,11 +305,17 @@ private T call(URL url, Function connectionFactory) } } } catch (NoHttpResponseException ex) { - throw new RegistryNoResponseException(ex); + throw new RegistryNoResponseException( + registryEndpointRequestProperties.getServerUrl(), + registryEndpointRequestProperties.getImageName(), + ex); } catch (IOException ex) { if (isBrokenPipe(ex)) { - throw new RegistryBrokenPipeException(ex); + throw new RegistryBrokenPipeException( + registryEndpointRequestProperties.getServerUrl(), + registryEndpointRequestProperties.getImageName(), + ex); } throw ex; } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java index 70513c21ee..440059d70b 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java @@ -21,7 +21,7 @@ /** Thrown when a registry did not respond. */ class RegistryNoResponseException extends RegistryException { - RegistryNoResponseException(Throwable cause) { - super(cause); + RegistryNoResponseException(String registry, String repository, Throwable cause) { + super("no response from " + registry + "/" + repository, cause); } } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java index 77b540b0e9..febd365a10 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/registry/RegistryEndpointCallerTest.java @@ -277,6 +277,26 @@ public void testCall_insecureCallerOnNonListeningServerAndPortSpecified() Mockito.verifyNoMoreInteractions(mockInsecureConnectionFactory); } + @Test + public void testCall_timeoutFromConnectException() throws IOException, RegistryException { + ConnectException mockConnectException = Mockito.mock(ConnectException.class); + Mockito.when(mockConnectException.getMessage()).thenReturn("Connection timed out"); + Mockito.when(mockConnection.send(Mockito.eq("httpMethod"), Mockito.any())) + .thenThrow(mockConnectException) // server times out on 443 + .thenReturn(mockResponse); // respond when connected through 80 + + try { + RegistryEndpointCaller insecureEndpointCaller = + createRegistryEndpointCaller(true, -1); + insecureEndpointCaller.call(); + Assert.fail("Should not fall back to HTTP if timed out even for ConnectionException"); + + } catch (ConnectException ex) { + Assert.assertSame(mockConnectException, ex); + Mockito.verify(mockConnection).send(Mockito.anyString(), Mockito.any()); + } + } + @Test public void testCall_noHttpResponse() throws IOException, RegistryException { NoHttpResponseException mockNoHttpResponseException = From 1030fcc15a092e0d846c319f9583e60eff179f1b Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Tue, 3 Sep 2019 19:01:33 -0400 Subject: [PATCH 2/2] Restore original code --- .../jib/registry/RegistryBrokenPipeException.java | 12 ++++-------- .../tools/jib/registry/RegistryEndpointCaller.java | 14 ++++---------- .../jib/registry/RegistryNoResponseException.java | 4 ++-- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java index 2a1cf83109..76508c4184 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryBrokenPipeException.java @@ -21,15 +21,11 @@ /** Thrown when the registry shut down the connection. */ class RegistryBrokenPipeException extends RegistryException { - RegistryBrokenPipeException(String registry, String repository, Throwable cause) { + RegistryBrokenPipeException(Throwable cause) { super( - "I/O error due to broken pipe from " - + registry - + "/" - + repository - + ": the server shut down the connection. Check the server log if possible. This could " - + "also be a proxy issue. For example, a proxy may prevent sending packets that are " - + "too large.", + "I/O error due to broken pipe: the server shut down the connection. " + + "Check the server log if possible. This could also be a proxy issue. For example," + + "a proxy may prevent sending packets that are too large.", cause); } } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java index d2b90891db..2cf3529efc 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java @@ -177,8 +177,8 @@ private T callWithAllowInsecureRegistryHandling(URL url) throws IOException, Reg } catch (ConnectException ex) { // It is observed that Open/Oracle JDKs sometimes throw SocketTimeoutException but other times // ConnectException for connection timeout. (Could be a JDK bug.) Note SocketTimeoutException - // does not extend ConnectException, and we want to be consistent to error out on timeouts. - // See https://github.com/GoogleContainerTools/jib/issues/1895#issuecomment-527544094 + // does not extend ConnectException (or vice versa), and we want to be consistent to error out + // on timeouts: https://github.com/GoogleContainerTools/jib/issues/1895#issuecomment-527544094 if (ex.getMessage() != null && ex.getMessage().contains("timed out")) { throw ex; } @@ -305,17 +305,11 @@ private T call(URL url, Function connectionFactory) } } } catch (NoHttpResponseException ex) { - throw new RegistryNoResponseException( - registryEndpointRequestProperties.getServerUrl(), - registryEndpointRequestProperties.getImageName(), - ex); + throw new RegistryNoResponseException(ex); } catch (IOException ex) { if (isBrokenPipe(ex)) { - throw new RegistryBrokenPipeException( - registryEndpointRequestProperties.getServerUrl(), - registryEndpointRequestProperties.getImageName(), - ex); + throw new RegistryBrokenPipeException(ex); } throw ex; } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java index 440059d70b..70513c21ee 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryNoResponseException.java @@ -21,7 +21,7 @@ /** Thrown when a registry did not respond. */ class RegistryNoResponseException extends RegistryException { - RegistryNoResponseException(String registry, String repository, Throwable cause) { - super("no response from " + registry + "/" + repository, cause); + RegistryNoResponseException(Throwable cause) { + super(cause); } }