Skip to content

Fix coordinator/overlord redirects when TLS is enabled #5037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jon-wei merged 6 commits into from
Nov 9, 2017
Merged

Fix coordinator/overlord redirects when TLS is enabled #5037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6437f4c
Fix coordinator/overlord redirects when TLS is enabled
himanshug Nov 2, 2017
ec492f5
address review comment
himanshug Nov 2, 2017
0d91620
Merge remote-tracking branch 'druidio/master' into fix_tls_redirect
himanshug Nov 6, 2017
9638909
fix UTs
himanshug Nov 6, 2017
382291d
workaround to not ignore URL instance to fix the teamcity build
himanshug Nov 6, 2017
27da2d5
update tls doc
himanshug Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/content/operations/tls-support.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

4 changes: 2 additions & 2 deletions indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ public boolean doLocal(String requestURI)
}

@Override
public URL getRedirectURL(String scheme, String queryString, String requestURI)
public URL getRedirectURL(String queryString, String requestURI)
{
try {
final String leader = taskMaster.getCurrentLeader();
if (leader == null || leader.isEmpty()) {
return null;
}

String location = StringUtils.format("%s://%s%s", scheme, leader, requestURI);
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document somewhere that requests sent using "http" scheme would be redirected to "https" if both TLS/non-TLS ports are enabled?

This might be surprising to a user if the client being used can't handle HTTPS and fails because of the scheme change.

Ideally I think the redirect should follow the same scheme as the original request, but that doesn't seem straightforward to implement since the leaderID always prefers TLS if it's there. Any ideas on whether we could accomplish that in a simple way?

Copy link
Copy Markdown
Contributor Author

@himanshug himanshug Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, ideally http should get redirected to http but that is not simple to implement. however fixing it, so that it atleast redirects to a valid url, is necessary. so the upgrade path is to enable clients so that they can handle both http and https... then upgrade druid and enable both http and https ports... then change client configs to refer to druid with https and then disable http port on druid servers.

I'll update the tls specific doc page.

Copy link
Copy Markdown
Contributor Author

@himanshug himanshug Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the doc

String location = StringUtils.format("%s%s", leader, requestURI);

if (queryString != null) {
location = StringUtils.format("%s?%s", location, queryString);
Expand Down
12 changes: 6 additions & 6 deletions indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void testGetRedirectURLNull()
{
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);
}
Expand All @@ -80,36 +80,36 @@ public void testGetRedirectURLEmpty()
{
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);
}

@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);
}

@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"
) + "/status";

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()
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/io/druid/curator/discovery/CuratorDruidLeaderSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@
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;
Expand Down Expand Up @@ -71,7 +71,7 @@ public CuratorDruidLeaderSelector(CuratorFramework curator, @Self DruidNode self
private LeaderLatch createNewLeaderLatch()
{
final LeaderLatch newLeaderLatch = new LeaderLatch(
curator, latchPath, self.getHostAndPortToUse()
curator, latchPath, self.getServiceScheme() + "://" + self.getHostAndPortToUse()
);

newLeaderLatch.addListener(
Expand Down
27 changes: 20 additions & 7 deletions server/src/main/java/io/druid/discovery/DruidLeaderClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,14 +233,27 @@ public String findCurrentLeader()
}

if (responseHolder.getStatus().getCode() == 200) {
return responseHolder.getContent();
} else {
throw new ISE(
"Couldn't find leader, failed response status is [%s] and content [%s].",
responseHolder.getStatus().getCode(),
responseHolder.getContent()
);
String leaderUrl = responseHolder.getContent();

//verify this is valid url
try {
URL validatedUrl = new URL(leaderUrl);
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
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/io/druid/server/http/CoordinatorRedirectInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ public boolean doLocal(String requestURI)
}

@Override
public URL getRedirectURL(String scheme, String queryString, String requestURI)
public URL getRedirectURL(String queryString, String requestURI)
{
try {
final String leader = coordinator.getCurrentLeader();
if (leader == null) {
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);
Expand Down
15 changes: 8 additions & 7 deletions server/src/main/java/io/druid/server/http/OverlordProxyServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
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;
Expand Down Expand Up @@ -57,13 +58,13 @@ protected String rewriteTarget(HttpServletRequest request)
throw new ISE("Can't find Overlord leader.");
}

return new URI(
request.getScheme(),
overlordLeader,
request.getRequestURI(),
request.getQueryString(),
null
).toString();
String location = StringUtils.format("%s%s", overlordLeader, request.getRequestURI());

if (request.getQueryString() != null) {
location = StringUtils.format("%s?%s", location, request.getQueryString());
}

return new URI(location).toString();
}
catch (URISyntaxException e) {
throw Throwables.propagate(e);
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/io/druid/server/http/RedirectFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
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) {
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/io/druid/server/http/RedirectInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
4 changes: 2 additions & 2 deletions server/src/test/java/io/druid/curator/discovery/CuratorDruidLeaderSelectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public void stopBeingLeader()
}

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(
Expand Down Expand Up @@ -159,7 +159,7 @@ public void stopBeingLeader()
}

Assert.assertTrue(leaderSelector3.isLeader());
Assert.assertEquals("h3:8080", leaderSelector1.getCurrentLeader());
Assert.assertEquals("http://h3:8080", leaderSelector1.getCurrentLeader());
Assert.assertEquals(1, leaderSelector3.localTerm());
}

Expand Down
7 changes: 3 additions & 4 deletions server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,19 @@ public void testGetRedirectURLNull()
{
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);
}

@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);
}
Expand Down
3 changes: 1 addition & 2 deletions server/src/test/java/io/druid/server/http/OverlordProxyServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
6 changes: 3 additions & 3 deletions services/src/main/java/io/druid/cli/CoordinatorOverlordRedirectInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ public boolean doLocal(String requestURI)
}

@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) :
coordinatorRedirectInfo.getRedirectURL(scheme, queryString, requestURI);
overlordRedirectInfo.getRedirectURL(queryString, requestURI) :
coordinatorRedirectInfo.getRedirectURL(queryString, requestURI);
}

private boolean isOverlordRequest(String requestURI)
Expand Down