Fix coordinator/overlord redirects when TLS is enabled (#5037)

* Fix coordinator/overlord redirects when TLS is enabled

* address review comment

* fix UTs

* workaround to not ignore URL instance to fix the teamcity build

* update tls doc

docs/content/operations/tls-support.md

@@ -54,3 +54,8 @@ which can provide an instance of SSLContext. Druid comes with a simple extension
 which should be useful enough for most simple cases, see [this](./including-extensions.html) for how to include extensions.
 If this extension does not satisfy the requirements then please follow the extension [implementation](https://github.com/druid-io/druid/tree/master/extensions-core/simple-client-sslcontext)
 to create your own extension.
+
+# Upgrading Clients that interact with Overlord or Coordinator
+When Druid Coordinator/Overlord have both HTTP and HTTPS enabled and Client sends request to non-leader node, then Client is always redirected to the HTTPS endpoint on leader node.
+So, Clients should be first upgraded to be able to handle redirect to HTTPS. Then Druid Overlord/Coordinator should be upgraded and configured to run both HTTP and HTTPS ports. Then Client configuration should be changed to refer to Druid Coordinator/Overlord via the HTTPS endpoint and then HTTP port on Druid Coordinator/Overlord should be disabled.
+

indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java

@@ -53,7 +53,7 @@ public class OverlordRedirectInfo implements RedirectInfo
   }
 
   @Override
-  public URL getRedirectURL(String scheme, String queryString, String requestURI)
+  public URL getRedirectURL(String queryString, String requestURI)
   {
     try {
       final String leader = taskMaster.getCurrentLeader();
@@ -61,7 +61,7 @@ public class OverlordRedirectInfo implements RedirectInfo
         return null;
       }
 
-      String location = StringUtils.format("%s://%s%s", scheme, leader, requestURI);
+      String location = StringUtils.format("%s%s", leader, requestURI);
 
       if (queryString != null) {
         location = StringUtils.format("%s?%s", location, queryString);

indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java

@@ -70,7 +70,7 @@ public class OverlordRedirectInfoTest
   {
     EasyMock.expect(taskMaster.getCurrentLeader()).andReturn(null).anyTimes();
     EasyMock.replay(taskMaster);
-    URL url = redirectInfo.getRedirectURL("http", "query", "/request");
+    URL url = redirectInfo.getRedirectURL("query", "/request");
     Assert.assertNull(url);
     EasyMock.verify(taskMaster);
   }
@@ -80,7 +80,7 @@ public class OverlordRedirectInfoTest
   {
     EasyMock.expect(taskMaster.getCurrentLeader()).andReturn("").anyTimes();
     EasyMock.replay(taskMaster);
-    URL url = redirectInfo.getRedirectURL("http", "query", "/request");
+    URL url = redirectInfo.getRedirectURL("query", "/request");
     Assert.assertNull(url);
     EasyMock.verify(taskMaster);
   }
@@ -88,12 +88,12 @@ public class OverlordRedirectInfoTest
   @Test
   public void testGetRedirectURL()
   {
-    String host = "localhost";
+    String host = "http://localhost";
     String query = "foo=bar&x=y";
     String request = "/request";
     EasyMock.expect(taskMaster.getCurrentLeader()).andReturn(host).anyTimes();
     EasyMock.replay(taskMaster);
-    URL url = redirectInfo.getRedirectURL("http", query, request);
+    URL url = redirectInfo.getRedirectURL(query, request);
     Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString());
     EasyMock.verify(taskMaster);
   }
@@ -101,7 +101,7 @@ public class OverlordRedirectInfoTest
   @Test
   public void testGetRedirectURLWithEncodedCharacter() throws UnsupportedEncodingException
   {
-    String host = "localhost";
+    String host = "http://localhost";
     String request = "/druid/indexer/v1/task/" + URLEncoder.encode(
         "index_hadoop_datasource_2017-07-12T07:43:01.495Z",
         "UTF-8"
@@ -109,7 +109,7 @@ public class OverlordRedirectInfoTest
 
     EasyMock.expect(taskMaster.getCurrentLeader()).andReturn(host).anyTimes();
     EasyMock.replay(taskMaster);
-    URL url = redirectInfo.getRedirectURL("http", null, request);
+    URL url = redirectInfo.getRedirectURL(null, request);
     Assert.assertEquals(
         "http://localhost/druid/indexer/v1/task/index_hadoop_datasource_2017-07-12T07%3A43%3A01.495Z/status",
         url.toString()

server/src/main/java/io/druid/curator/discovery/CuratorDruidLeaderSelector.java

@@ -22,12 +22,12 @@ package io.druid.curator.discovery;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
 import com.metamx.emitter.EmittingLogger;
-import io.druid.java.util.common.concurrent.Execs;
 import io.druid.concurrent.LifecycleLock;
 import io.druid.discovery.DruidLeaderSelector;
 import io.druid.guice.annotations.Self;
 import io.druid.java.util.common.ISE;
 import io.druid.java.util.common.StringUtils;
+import io.druid.java.util.common.concurrent.Execs;
 import io.druid.java.util.common.guava.CloseQuietly;
 import io.druid.server.DruidNode;
 import org.apache.curator.framework.CuratorFramework;
@@ -71,7 +71,7 @@ public class CuratorDruidLeaderSelector implements DruidLeaderSelector
   private LeaderLatch createNewLeaderLatch()
   {
     final LeaderLatch newLeaderLatch = new LeaderLatch(
-        curator, latchPath, self.getHostAndPortToUse()
+        curator, latchPath, self.getServiceScheme() + "://" + self.getHostAndPortToUse()
     );
 
     newLeaderLatch.addListener(

server/src/main/java/io/druid/discovery/DruidLeaderClient.java

@@ -233,14 +233,27 @@ public class DruidLeaderClient
     }
 
     if (responseHolder.getStatus().getCode() == 200) {
-      return responseHolder.getContent();
+      String leaderUrl = responseHolder.getContent();
-    } else {
+
-      throw new ISE(
+      //verify this is valid url
-          "Couldn't find leader, failed response status is [%s] and content [%s].",
+      try {
-          responseHolder.getStatus().getCode(),
+        URL validatedUrl = new URL(leaderUrl);
-          responseHolder.getContent()
+        currentKnownLeader.set(leaderUrl);
-      );
+
+        // validatedUrl.toString() is returned instead of leaderUrl or else teamcity build fails because of breaking
+        // the rule of ignoring new URL(leaderUrl) object.
+        return validatedUrl.toString();
+      }
+      catch (MalformedURLException ex) {
+        log.error(ex, "Received malformed leader url[%s].", leaderUrl);
+      }
     }
+
+    throw new ISE(
+        "Couldn't find leader, failed response status is [%s] and content [%s].",
+        responseHolder.getStatus().getCode(),
+        responseHolder.getContent()
+    );
   }
 
   private String getCurrentKnownLeader(final boolean cached) throws IOException

server/src/main/java/io/druid/server/http/CoordinatorRedirectInfo.java

@@ -52,7 +52,7 @@ public class CoordinatorRedirectInfo implements RedirectInfo
   }
 
   @Override
-  public URL getRedirectURL(String scheme, String queryString, String requestURI)
+  public URL getRedirectURL(String queryString, String requestURI)
   {
     try {
       final String leader = coordinator.getCurrentLeader();
@@ -60,7 +60,7 @@ public class CoordinatorRedirectInfo implements RedirectInfo
         return null;
       }
 
-      String location = StringUtils.format("%s://%s%s", scheme, leader, requestURI);
+      String location = StringUtils.format("%s%s", leader, requestURI);
 
       if (queryString != null) {
         location = StringUtils.format("%s?%s", location, queryString);

server/src/main/java/io/druid/server/http/OverlordProxyServlet.java

@@ -24,6 +24,7 @@ import com.google.inject.Inject;
 import io.druid.client.indexing.IndexingService;
 import io.druid.discovery.DruidLeaderClient;
 import io.druid.java.util.common.ISE;
+import io.druid.java.util.common.StringUtils;
 import io.druid.server.security.AuthConfig;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.proxy.ProxyServlet;
@@ -57,13 +58,13 @@ public class OverlordProxyServlet extends ProxyServlet
         throw new ISE("Can't find Overlord leader.");
       }
 
-      return new URI(
+      String location = StringUtils.format("%s%s", overlordLeader, request.getRequestURI());
-          request.getScheme(),
+
-          overlordLeader,
+      if (request.getQueryString() != null) {
-          request.getRequestURI(),
+        location = StringUtils.format("%s?%s", location, request.getQueryString());
-          request.getQueryString(),
+      }
-          null
+
-      ).toString();
+      return new URI(location).toString();
     }
     catch (URISyntaxException e) {
       throw Throwables.propagate(e);

server/src/main/java/io/druid/server/http/RedirectFilter.java

@@ -71,7 +71,7 @@ public class RedirectFilter implements Filter
     if (redirectInfo.doLocal(request.getRequestURI())) {
       chain.doFilter(request, response);
     } else {
-      URL url = redirectInfo.getRedirectURL(request.getScheme(), request.getQueryString(), request.getRequestURI());
+      URL url = redirectInfo.getRedirectURL(request.getQueryString(), request.getRequestURI());
       log.debug("Forwarding request to [%s]", url);
 
       if (url == null) {

server/src/main/java/io/druid/server/http/RedirectInfo.java

@@ -27,5 +27,5 @@ public interface RedirectInfo
 {
   boolean doLocal(String requestURI);
 
-  URL getRedirectURL(String scheme, String queryString, String requestURI);
+  URL getRedirectURL(String queryString, String requestURI);
 }

server/src/test/java/io/druid/curator/discovery/CuratorDruidLeaderSelectorTest.java

@@ -126,7 +126,7 @@ public class CuratorDruidLeaderSelectorTest extends CuratorTestBase
     }
 
     Assert.assertTrue(leaderSelector2.isLeader());
-    Assert.assertEquals("h2:8080", leaderSelector1.getCurrentLeader());
+    Assert.assertEquals("http://h2:8080", leaderSelector1.getCurrentLeader());
     Assert.assertEquals(2, leaderSelector2.localTerm());
 
     CuratorDruidLeaderSelector leaderSelector3 = new CuratorDruidLeaderSelector(
@@ -159,7 +159,7 @@ public class CuratorDruidLeaderSelectorTest extends CuratorTestBase
     }
 
     Assert.assertTrue(leaderSelector3.isLeader());
-    Assert.assertEquals("h3:8080", leaderSelector1.getCurrentLeader());
+    Assert.assertEquals("http://h3:8080", leaderSelector1.getCurrentLeader());
     Assert.assertEquals(1, leaderSelector3.localTerm());
   }
 

server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java

@@ -68,7 +68,7 @@ public class CoordinatorRedirectInfoTest
   {
     EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn(null).anyTimes();
     EasyMock.replay(druidCoordinator);
-    URL url = coordinatorRedirectInfo.getRedirectURL("http", "query", "/request");
+    URL url = coordinatorRedirectInfo.getRedirectURL("query", "/request");
     Assert.assertNull(url);
     EasyMock.verify(druidCoordinator);
   }
@@ -76,12 +76,11 @@ public class CoordinatorRedirectInfoTest
   @Test
   public void testGetRedirectURL()
   {
-    String host = "localhost";
     String query = "foo=bar&x=y";
     String request = "/request";
-    EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn(host).anyTimes();
+    EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn("http://localhost").anyTimes();
     EasyMock.replay(druidCoordinator);
-    URL url = coordinatorRedirectInfo.getRedirectURL("http", query, request);
+    URL url = coordinatorRedirectInfo.getRedirectURL(query, request);
     Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString());
     EasyMock.verify(druidCoordinator);
   }

server/src/test/java/io/druid/server/http/OverlordProxyServletTest.java

@@ -33,10 +33,9 @@ public class OverlordProxyServletTest
   public void testRewriteURI()
   {
     DruidLeaderClient druidLeaderClient = EasyMock.createMock(DruidLeaderClient.class);
-    EasyMock.expect(druidLeaderClient.findCurrentLeader()).andReturn("overlord:port");
+    EasyMock.expect(druidLeaderClient.findCurrentLeader()).andReturn("https://overlord:port");
 
     HttpServletRequest request = EasyMock.createMock(HttpServletRequest.class);
-    EasyMock.expect(request.getScheme()).andReturn("https").anyTimes();
     EasyMock.expect(request.getQueryString()).andReturn("param1=test&param2=test2").anyTimes();
     EasyMock.expect(request.getRequestURI()).andReturn("/druid/overlord/worker").anyTimes();
 

services/src/main/java/io/druid/cli/CoordinatorOverlordRedirectInfo.java

@@ -51,11 +51,11 @@ public class CoordinatorOverlordRedirectInfo implements RedirectInfo
   }
 
   @Override
-  public URL getRedirectURL(String scheme, String queryString, String requestURI)
+  public URL getRedirectURL(String queryString, String requestURI)
   {
     return isOverlordRequest(requestURI) ?
-           overlordRedirectInfo.getRedirectURL(scheme, queryString, requestURI) :
+           overlordRedirectInfo.getRedirectURL(queryString, requestURI) :
-           coordinatorRedirectInfo.getRedirectURL(scheme, queryString, requestURI);
+           coordinatorRedirectInfo.getRedirectURL(queryString, requestURI);
   }
 
   private boolean isOverlordRequest(String requestURI)