Optimize insecure registry failover by caching failover history

jib-core/CHANGELOG.md

@@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file.
 ### Fixed
 
 - `Containerizer#setAllowInsecureRegistries(boolean)` and the `sendCredentialsOverHttp` system property are now effective for authentication service server connections. ([#2074](https://github.com/GoogleContainerTools/jib/pull/2074)
+- Fixed inefficient communications when interacting with insecure registries and servers (when `Containerizer#setAllowInsecureRegistries(boolean)` is set). ([#946](https://github.com/GoogleContainerTools/jib/issues/946))
 
 ## 0.12.0
 

jib-core/src/main/java/com/google/cloud/tools/jib/configuration/BuildContext.java

@@ -417,10 +417,6 @@ public EventHandlers getEventHandlers() {
     return eventHandlers;
   }
 
-  public FailoverHttpClient getHttpClient() {
-    return httpClient;
-  }
-
   public ExecutorService getExecutorService() {
     return executorService;
   }

jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java

@@ -26,12 +26,15 @@
 import com.google.api.client.util.SslUtils;
 import com.google.cloud.tools.jib.api.LogEvent;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import java.io.IOException;
 import java.net.ConnectException;
 import java.net.URL;
 import java.security.GeneralSecurityException;
 import java.util.ArrayDeque;
 import java.util.Deque;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import javax.net.ssl.SSLException;
@@ -71,6 +74,13 @@
  */
 public class FailoverHttpClient {
 
+  /** Represents failover actions taken. To be recorded in the failover history. */
+  private static enum Failover {
+    NONE, // no failover (secure HTTPS)
+    INSECURE_HTTPS, // HTTPS with certificate validation disabled
+    HTTP // plain HTTP
+  }
+
   private static boolean isHttpsProtocol(URL url) {
     return "https".equals(url.getProtocol());
   }
@@ -110,6 +120,8 @@ private static HttpTransport getInsecureHttpTransport() {
   private final Supplier<HttpTransport> secureHttpTransportFactory;
   private final Supplier<HttpTransport> insecureHttpTransportFactory;
 
+  private final ConcurrentHashMap<String, Failover> failoverHistory = new ConcurrentHashMap<>();
+
   private final Deque<HttpTransport> transportsCreated = new ArrayDeque<>();
   private final Deque<Response> responsesCreated = new ArrayDeque<>();
 
@@ -209,10 +221,18 @@ public Response put(URL url, Request request) throws IOException {
    * @throws IOException if building the HTTP request fails.
    */
   public Response call(String httpMethod, URL url, Request request) throws IOException {
-    if (!isHttpsProtocol(url) && !enableHttpAndInsecureFailover) {
+    if (!isHttpsProtocol(url)) {
+      if (enableHttpAndInsecureFailover) { // HTTP requested. We only care if HTTP is enabled.
+        return call(httpMethod, url, request, getHttpTransport(true));
+      }
       throw new SSLException("insecure HTTP connection not allowed: " + url);
     }
 
+    Optional<Response> fastPathResponse = followFailoverHistory(httpMethod, url, request);
+    if (fastPathResponse.isPresent()) {
+      return fastPathResponse.get();
+    }
+
     try {
       return call(httpMethod, url, request, getHttpTransport(true));
 
@@ -223,32 +243,49 @@ public Response call(String httpMethod, URL url, Request request) throws IOExcep
 
       try {
         logInsecureHttpsFailover(url);
-        return call(httpMethod, url, request, getHttpTransport(false));
+        Response response = call(httpMethod, url, request, getHttpTransport(false));
+        failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.INSECURE_HTTPS);
+        return response;
 
       } catch (SSLException ignored) { // This is usually when the server is plain-HTTP.
         logHttpFailover(url);
-        return call(httpMethod, toHttp(url), request, getHttpTransport(true));
+        Response response = call(httpMethod, toHttp(url), request, getHttpTransport(true));
+        failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.HTTP);
+        return response;
       }
 
     } 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;
-      }
-
-      // 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.
-      if (enableHttpAndInsecureFailover && isHttpsProtocol(url) && url.getPort() == -1) {
-        logHttpFailover(url);
-        return call(httpMethod, toHttp(url), request, getHttpTransport(true));
+      if (ex.getMessage() == null || !ex.getMessage().contains("timed out")) {
+        // 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.
+        if (enableHttpAndInsecureFailover && isHttpsProtocol(url) && url.getPort() == -1) {
+          logHttpFailover(url);
+          Response response = call(httpMethod, toHttp(url), request, getHttpTransport(true));
+          failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.HTTP);
+          return response;
+        }
       }
       throw ex;
     }
   }
 
+  private Optional<Response> followFailoverHistory(String httpMethod, URL url, Request request)
+      throws IOException {
+    Preconditions.checkArgument(isHttpsProtocol(url));
+    switch (failoverHistory.getOrDefault(url.getHost() + ":" + url.getPort(), Failover.NONE)) {
+      case HTTP:
+        return Optional.of(call(httpMethod, toHttp(url), request, getHttpTransport(true)));
+      case INSECURE_HTTPS:
+        return Optional.of(call(httpMethod, url, request, getHttpTransport(false)));
+      default:
+        return Optional.empty(); // No history found. Should go for normal execution path.
+    }
+  }
+
   private Response call(String httpMethod, URL url, Request request, HttpTransport httpTransport)
       throws IOException {
     boolean clearAuthorization = !isHttpsProtocol(url) && !sendAuthorizationOverHttp;
@@ -271,7 +308,7 @@ private Response call(String httpMethod, URL url, Request request, HttpTransport
     try {
       Response response = new Response(httpRequest.execute());
       synchronized (responsesCreated) {
-        responsesCreated.addLast(response);
+        responsesCreated.add(response);
       }
       return response;
     } catch (HttpResponseException ex) {
@@ -283,18 +320,18 @@ private HttpTransport getHttpTransport(boolean secureTransport) {
     HttpTransport transport =
         secureTransport ? secureHttpTransportFactory.get() : insecureHttpTransportFactory.get();
     synchronized (transportsCreated) {
-      transportsCreated.addLast(transport);
+      transportsCreated.add(transport);
     }
     return transport;
   }
 
   private void logHttpFailover(URL url) {
     String log = "Failed to connect to " + url + " over HTTPS. Attempting again with HTTP.";
-    logger.accept(LogEvent.info(log));
+    logger.accept(LogEvent.warn(log));
   }
 
   private void logInsecureHttpsFailover(URL url) {
     String log = "Cannot verify server at " + url + ". Attempting again with no TLS verification.";
-    logger.accept(LogEvent.info(log));
+    logger.accept(LogEvent.warn(log));
   }
 }

jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientTest.java

@@ -33,8 +33,10 @@
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.List;
 import java.util.concurrent.atomic.LongAdder;
 import java.util.function.Consumer;
+import java.util.stream.Collectors;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLPeerUnverifiedException;
 import org.junit.Assert;
@@ -150,14 +152,9 @@ public void testGet_insecureClientOnUnverifiableServer() throws IOException {
       Assert.assertEquals("body", new String(bytes, StandardCharsets.UTF_8));
     }
 
-    Assert.assertEquals(2, urlCaptor.getAllValues().size());
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0));
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(1));
-
-    String log =
-        "Cannot verify server at https://insecure. Attempting again with no TLS verification.";
-    Mockito.verify(logger).accept(LogEvent.info(log));
-    Mockito.verifyNoMoreInteractions(logger);
+    verifyCapturedUrls("https://insecure", "https://insecure");
+    verifyWarnings(
+        "Cannot verify server at https://insecure. Attempting again with no TLS verification.");
   }
 
   @Test
@@ -180,17 +177,10 @@ public void testGet_insecureClientOnHttpServer() throws IOException {
     Mockito.verify(mockHttpRequest, Mockito.times(2)).execute();
     Mockito.verify(mockInsecureHttpRequest, Mockito.times(1)).execute();
 
-    Assert.assertEquals(3, urlCaptor.getAllValues().size());
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0));
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(1));
-    Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(2));
-
-    String log1 =
-        "Cannot verify server at https://insecure. Attempting again with no TLS verification.";
-    String log2 = "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP.";
-    Mockito.verify(logger).accept(LogEvent.info(log1));
-    Mockito.verify(logger).accept(LogEvent.info(log2));
-    Mockito.verifyNoMoreInteractions(logger);
+    verifyCapturedUrls("https://insecure", "https://insecure", "http://insecure");
+    verifyWarnings(
+        "Cannot verify server at https://insecure. Attempting again with no TLS verification.",
+        "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP.");
   }
 
   @Test
@@ -211,13 +201,8 @@ public void testGet_insecureClientOnHttpServerAndNoPortSpecified() throws IOExce
     Mockito.verify(mockHttpRequest, Mockito.times(2)).execute();
     Mockito.verifyNoInteractions(mockInsecureHttpRequest);
 
-    Assert.assertEquals(2, urlCaptor.getAllValues().size());
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0));
-    Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(1));
-
-    String log = "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP.";
-    Mockito.verify(logger).accept(LogEvent.info(log));
-    Mockito.verifyNoMoreInteractions(logger);
+    verifyCapturedUrls("https://insecure", "http://insecure");
+    verifyWarnings("Failed to connect to https://insecure over HTTPS. Attempting again with HTTP.");
   }
 
   @Test
@@ -232,8 +217,7 @@ public void testGet_secureClientOnNonListeningServerAndNoPortSpecified() throws
     } catch (ConnectException ex) {
       Assert.assertEquals("my exception", ex.getMessage());
 
-      Assert.assertEquals(1, urlCaptor.getAllValues().size());
-      Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getValue());
+      verifyCapturedUrls("https://insecure");
 
       Mockito.verify(mockHttpRequest, Mockito.times(1)).execute();
       Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger);
@@ -253,8 +237,7 @@ public void testGet_insecureClientOnNonListeningServerAndPortSpecified() throws
     } catch (ConnectException ex) {
       Assert.assertEquals("my exception", ex.getMessage());
 
-      Assert.assertEquals(1, urlCaptor.getAllValues().size());
-      Assert.assertEquals(new GenericUrl("https://insecure:5000"), urlCaptor.getValue());
+      verifyCapturedUrls("https://insecure:5000");
 
       Mockito.verify(mockHttpRequest, Mockito.times(1)).execute();
       Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger);
@@ -273,8 +256,7 @@ public void testGet_timeoutFromConnectException() throws IOException {
     } catch (ConnectException ex) {
       Assert.assertEquals("Connection timed out", ex.getMessage());
 
-      Assert.assertEquals(1, urlCaptor.getAllValues().size());
-      Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getValue());
+      verifyCapturedUrls("https://insecure");
 
       Mockito.verify(mockHttpRequest, Mockito.times(1)).execute();
       Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger);
@@ -293,9 +275,7 @@ public void testGet_doNotSendCredentialsOverHttp() throws IOException {
     try (Response response =
         insecureHttpClient.get(new URL("https://insecure"), fakeRequest(null))) {}
 
-    Assert.assertEquals(2, urlCaptor.getAllValues().size());
-    Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0));
-    Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(1));
+    verifyCapturedUrls("https://insecure", "http://insecure");
 
     Assert.assertEquals(2, httpHeadersCaptor.getAllValues().size());
     Assert.assertEquals(
@@ -345,6 +325,79 @@ public void testShutDown() throws IOException {
     }
   }
 
+  @Test
+  public void testFollowFailoverHistory_insecureHttps() throws IOException {
+    FailoverHttpClient httpClient = newHttpClient(true, false);
+
+    Mockito.when(mockHttpRequest.execute())
+        .thenThrow(new SSLException(""))
+        .thenReturn(mockHttpResponse);
+    try (Response response1 = httpClient.get(new URL("https://url"), fakeRequest(null));
+        Response response2 = httpClient.post(new URL("https://url"), fakeRequest(null))) {}
+
+    Mockito.verify(mockHttpRequest, Mockito.times(1)).execute();
+    Mockito.verify(mockInsecureHttpRequest, Mockito.times(2)).execute();
+    verifyCapturedUrls("https://url", "https://url", "https://url");
+    verifyWarnings(
+        "Cannot verify server at https://url. Attempting again with no TLS verification.");
+  }
+
+  @Test
+  public void testFollowFailoverHistory_httpFailoverByConnectionError() throws IOException {
+    FailoverHttpClient httpClient = newHttpClient(true, false);
+
+    Mockito.when(mockHttpRequest.execute())
+        .thenThrow(new ConnectException())
+        .thenReturn(mockHttpResponse);
+
+    try (Response response1 = httpClient.get(new URL("https://url"), fakeRequest(null));
+        Response response2 = httpClient.post(new URL("https://url"), fakeRequest(null))) {}
+
+    Mockito.verify(mockHttpRequest, Mockito.times(3)).execute();
+    verifyCapturedUrls("https://url", "http://url", "http://url");
+    verifyWarnings("Failed to connect to https://url over HTTPS. Attempting again with HTTP.");
+  }
+
+  @Test
+  public void testFollowFailoverHistory_httpFailover() throws IOException {
+    FailoverHttpClient httpClient = newHttpClient(true, false);
+
+    Mockito.when(mockHttpRequest.execute())
+        .thenThrow(new SSLException(""))
+        .thenReturn(mockHttpResponse);
+    Mockito.when(mockInsecureHttpRequest.execute()).thenThrow(new SSLException(""));
+
+    try (Response response1 = httpClient.get(new URL("https://url:123"), fakeRequest(null));
+        Response response2 = httpClient.post(new URL("https://url:123"), fakeRequest(null))) {}
+
+    Mockito.verify(mockHttpRequest, Mockito.times(3)).execute();
+    Mockito.verify(mockInsecureHttpRequest, Mockito.times(1)).execute();
+    verifyCapturedUrls("https://url:123", "https://url:123", "http://url:123", "http://url:123");
+    verifyWarnings(
+        "Cannot verify server at https://url:123. Attempting again with no TLS verification.",
+        "Failed to connect to https://url:123 over HTTPS. Attempting again with HTTP.");
+  }
+
+  @Test
+  public void testFollowFailoverHistory_portsDifferent() throws IOException {
+    FailoverHttpClient httpClient = newHttpClient(true, false);
+
+    Mockito.when(mockHttpRequest.execute())
+        .thenThrow(new SSLException(""))
+        .thenThrow(new SSLException(""))
+        .thenReturn(mockHttpResponse);
+
+    try (Response response1 = httpClient.get(new URL("https://url:1"), fakeRequest(null));
+        Response response2 = httpClient.post(new URL("https://url:2"), fakeRequest(null))) {}
+
+    Mockito.verify(mockHttpRequest, Mockito.times(2)).execute();
+    Mockito.verify(mockInsecureHttpRequest, Mockito.times(2)).execute();
+    verifyCapturedUrls("https://url:1", "https://url:1", "https://url:2", "https://url:2");
+    verifyWarnings(
+        "Cannot verify server at https://url:1. Attempting again with no TLS verification.",
+        "Cannot verify server at https://url:2. Attempting again with no TLS verification.");
+  }
+
   private void setUpMocks(
       HttpTransport mockHttpTransport,
       HttpRequestFactory mockHttpRequestFactory,
@@ -406,4 +459,17 @@ private void verifyCall(String httpMethod, CallFunction callFunction) throws IOE
     Assert.assertEquals("crepecake", byteArrayOutputStream.toString(StandardCharsets.UTF_8.name()));
     Assert.assertEquals("crepecake".length(), totalByteCount.longValue());
   }
+
+  private void verifyWarnings(String... logs) {
+    for (String log : logs) {
+      Mockito.verify(logger, Mockito.times(1)).accept(LogEvent.warn(log));
+    }
+    Mockito.verifyNoMoreInteractions(logger);
+  }
+
+  private void verifyCapturedUrls(String... urls) {
+    List<String> captured =
+        urlCaptor.getAllValues().stream().map(GenericUrl::toString).collect(Collectors.toList());
+    Assert.assertEquals(Arrays.asList(urls), captured);
+  }
 }

jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java

@@ -92,7 +92,7 @@ public void testInsecureConnection_insecureHttpsFailover()
       String endpoint = server.getEndpoint();
       String expectedLog =
           "Cannot verify server at " + endpoint + ". Attempting again with no TLS verification.";
-      Mockito.verify(logger).accept(LogEvent.info(expectedLog));
+      Mockito.verify(logger).accept(LogEvent.warn(expectedLog));
     }
   }
 
@@ -114,8 +114,8 @@ public void testInsecureConnection_plainHttpFailover()
             "Cannot verify server at " + httpsUrl + ". Attempting again with no TLS verification.";
         String expectedLog2 =
             "Failed to connect to " + httpsUrl + " over HTTPS. Attempting again with HTTP.";
-        Mockito.verify(logger).accept(LogEvent.info(expectedLog1));
-        Mockito.verify(logger).accept(LogEvent.info(expectedLog2));
+        Mockito.verify(logger).accept(LogEvent.warn(expectedLog1));
+        Mockito.verify(logger).accept(LogEvent.warn(expectedLog2));
       }
     }
   }