Do not fall back to HTTP for connection timeout (#1949)

GoogleContainerTools · Sep 4, 2019 · 178f982 · 178f982
1 parent 68402d1
commit 178f982
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 20 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
@@ -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/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 (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;
+      }
+
       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.

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
@@ -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<String> 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 =