Add support for tunneling JDBC traffic over an HTTP proxy #541

kunalkandekar · 2015-12-17T23:07:47Z

Frequently data stores to be accessed by Gobblin reside outside data centers. In these cases, outbound access from a data center typically needs to go through a gateway proxy for security purposes. In some cases this is an HTTP proxy. However, some protocols like JDBC don't support the concept of "proxies", let alone HTTP proxies, and hence a solution is needed to enable this.

This Pull Request provides a method of tunneling JDBC connections over an HTTP proxy. Note that it is currently only implemented for JDBC, but can be extended to work with any other TCP-based protocol.

The way this works for JDBC is:

When a JdbcProvider is invoked, it checks if the WorkUnit has a proxy host and port defined.
If so, it extracts the remote host and port from the connectionUrl.
It then creates a Tunnel instance configured with the remote host and port. The Tunnel starts a thread that listens on an arbitrary port on localhost.
The JdbcProvider then changes the connectionUrl to replace the remote host and port with the localhost and port and passes it on to the driver.
Hence when the driver creates a JDBC connection, it connects to the Tunnel socket. The Tunnel then connects to the remote host through the proxy via a HTTP CONNECT request.
If established successfully, the Tunnel then just relays bytes back and forth.
When the JDBC data source is closed down, the Tunnel is shut down as well.

The Tunnel can accept as many connections as the JdbcExtractor opens. The Tunnel uses NIO to minimize resource usage.

chavdar · 2015-12-18T05:47:34Z

It looks like the patch uses Java 8 features. Currently, Gobblin is Java 7-based.

kunalkandekar · 2015-12-18T14:54:01Z

Ah, right. Will fix.

kunalkandekar · 2015-12-22T08:01:59Z

The test that fails has a 137 return code from its Gradle Test Executor. It could be that a specific test is failing because it's using up too much memory, in which case TravisCI SIGKILLS it (see: https://discuss.gradle.org/t/gradle-travis-ci/11928/3). I'll try to fix the test to use less memory.

kunalkandekar · 2015-12-22T19:31:37Z

@chavdar It seems TravisCI is killing certain test processes (possibly because they are consuming too many resources.) I just significantly reduced the memory usage of some tests, but they still continue to get killed. Can you think of anything we can do to debug or sidestep this issue?

kunalkandekar · 2015-12-23T20:46:41Z

While I'm trying to debug the TravisCI/gradle issue, here are instructions for performing end-to-end testing for these changes using gobblin standalone and a publicly accessible MySQL database: https://gist.github.com/kunalkandekar/b4f8c5fff6df62084e93

chavdar · 2016-01-07T17:42:32Z

@sahilTakiar can you please help reviewing this?

sahilTakiar · 2016-01-07T18:00:02Z

Will start reviewing today.

sahilTakiar · 2016-01-07T20:27:42Z

Few general comments:

1: Can you squash all your commits into a single commit (http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git)
2: Can you remove all mentions of "GaaP" so that this code is not tied to a specific proxy - you should just be able to make it a configuration option (like user.agent.name or something along those lines)
3: Is there a more detailed design doc on how this all works?
4: Adding more javadocs to the Tunnel.java class would be very helpful, and consider breaking the Tunnel.java class down into multiple classes as it is pretty long.
5: If you could follow the Gobblin coding style guide that would be great (https://github.com/linkedin/gobblin/wiki/CodingStyle)

kunalkandekar · 2016-01-07T21:03:55Z

Thanks for the comments, will incorporate the feedback.

liyinan926 · 2016-01-07T23:03:32Z

@kunalkandekar just one more comment: Gobblin is not using the standard LinkedIn coding style so none of our classes use variable names starting with _. Check it out here https://github.com/linkedin/gobblin/wiki/CodingStyle.

kunalkandekar · 2016-01-11T13:34:40Z

Thanks @sahilTakiar, am incorporating your feedback. Out of curiosity, could you explain the rationale between squashing commits into a single one? There is no detailed design doc, only the comment on the original commit, will include details in the JavaDocs.

sahilTakiar · 2016-01-11T17:43:38Z

We generally try to have 1 commit per new feature in Gobblin. This way we can have a single, descriptive commit message describing the change.

kunalkandekar · 2016-01-11T18:57:38Z

@sahilTakiar I see. Would it make sense to squash the commits once I push my current changes or should I do so beforehand?

chavdar · 2016-01-11T19:35:18Z

@pcadabam Can you also help @sahilTakiar with the review?

pcadabam-zz · 2016-01-11T21:59:09Z

Looking into it.

pcadabam-zz · 2016-01-11T22:10:23Z

gobblin-tunnel/src/main/java/gobblin/tunnel/Tunnel.java

+ *
+ * Implements a tunnel through a proxy to resource on the internet
+ */
+public class Tunnel {


This class seems too long. Can you pull out the private classes into a different file? Also Can you add more javadoc for the public methods and the classes?

Sure, this also what @sahilTakiar suggested. I'm refactoring accordingly.

sahilTakiar · 2016-01-11T22:12:00Z

@kunalkandekar you can go ahead to push all your local changes, and squash the commits once the PR is ready to be merged

pcadabam-zz · 2016-01-11T22:12:04Z

gobblin-tunnel/src/test/java/gobblin/tunnel/TestTunnelWithArbitraryTCPTraffic.java

+    _delayedDoubleEchoServer = startDoubleEchoServer(1000);
+    System.out.println("Delayed DoubleEchoServer on " + _delayedDoubleEchoServer.getServerSocketPort());
+    _talkFirstEchoServer = startTalkFirstEchoServer();
+    System.out.println("TalkFirstEchoServer on " + _delayedDoubleEchoServer.getServerSocketPort());


same as above

pcadabam-zz · 2016-01-11T22:31:54Z

Overall looks good.
Main comments around documentation and some refactor. Like @sahilTakiar said please squash the commits and update the pr.
Here are the steps

git rebase -i HEAD~num_commits_to_squash
An editor will show up where you can squash/pick commits
git push upstream master --force

sahilTakiar · 2016-01-21T20:08:54Z

gobblin-tunnel/src/main/java/gobblin/tunnel/ProxySetupHandler.java

+
+  private static final ByteBuffer OK_REPLY = ByteBuffer.wrap("HTTP/1.1 200".getBytes());
+  private static final Set<ByteBuffer> OK_REPLIES = new HashSet<ByteBuffer>(
+      Arrays.asList(OK_REPLY, ByteBuffer.wrap("HTTP/1.0 200".getBytes())));


Should this be HTTP 1.1 instead of HTTP 1.0 - might be good to make the HTTP version a string constant so that version changes are easy.

The HTTP/1.0 string is actually in addition to the HTTP/1.1 response. This ProxySetupHandler compares the response from the proxy against both strings. This is to handle the case where the intermediate proxy is based on HTTP 1.0 rather than 1.1. May help to move them to string constants, however.

…s could be because the Tunnel thread was not dying. This is an experimental change to test this hypothesis.

sahilTakiar · 2016-01-25T20:54:53Z

gobblin-tunnel/src/test/java/gobblin/tunnel/EasyThread.java

+ * @author kkandekar@linkedin.com
+ */
+abstract class EasyThread extends Thread {
+  static Set<EasyThread> ALL_THREADS = Collections.synchronizedSet(new HashSet<EasyThread>());


make this final so it can't be mutated by any other class

sahilTakiar · 2016-02-02T23:06:38Z

@kunalkandekar LGTM!

Are there any other changes you want to commit before I merge this?

kunalkandekar · 2016-02-03T21:19:44Z

Hey @sahilTakiar, I'm just running some end-to-end tests once more on the latest changes with some adverse situations (connections being cut unexpectedly, the proxy dying halfway through, etc.) to ensure these are handled cleanly. Will ping as soon as I'm done.

…thread cleanup

sahilTakiar · 2016-02-10T03:26:35Z

Can you resolve the merge conflicts?

kunalkandekar · 2016-02-10T16:46:16Z

Whoops, hadn't noticed the conflicts, sorry. Merged and pushed.

sahilTakiar · 2016-02-10T18:24:40Z

Do you know what's going on with the Java 8 build on Travis? Let's just disable any tests that currently don't run on Travis using the disabledOnTravis tag.

kunalkandekar · 2016-02-18T19:44:08Z

@sahilTakiar I've tried disabling all Tunnel tests by adding them to the "disabledOnTravis" group, but they are apparently still running and the build is still failing. Could you take a look and let me know if I'm doing it wrong?

Add support for tunneling JDBC traffic over an HTTP proxy

sahilTakiar · 2016-02-23T23:37:11Z

@kunalkandekar thanks for doing all of this and for addressing all the comments! This feature has now been merged!

kunalkandekar · 2016-02-24T00:44:33Z

@sahilTakiar nice! Thanks!

Fixing FindBugs warnings for gobblin-tunnel from #541

pcadabam-zz reviewed Jan 11, 2016
View reviewed changes

sahilTakiar reviewed Jan 21, 2016
View reviewed changes

kkandeka added 3 commits January 21, 2016 21:18

Changes to include further feedback

5f67dee

Noticed that setting as daemon caused TravisCI builds to succeed. Thi…

9164faf

…s could be because the Tunnel thread was not dying. This is an experimental change to test this hypothesis.

Another experimental change to see if Travis build succeed now

a67828f

sahilTakiar reviewed Jan 25, 2016
View reviewed changes

kkandeka added 2 commits January 25, 2016 20:26

Log exceptions in tests just in case

89ae41a

Close client and proxy sockets for any exception

e30ee6b

Added test for abrupt exceptions while tunneling and checking tunnel …

1bc147b

…thread cleanup

Merge with upstream

6e6b431

Kunal Kandekar added 4 commits February 11, 2016 13:03

Try disabling new test on Travis to see if builds work

6a28793

Disabling all Tunnel tests to see if TravisCI builds work

cd6a87b

Try disabling different tests on Travis to isolate problematic tests

8314fdf

Try disabling all Tunnel tests on Travis to see if Travis build works

a391077

sahilTakiar mentioned this pull request Feb 23, 2016

Debugging Travis CI Tests for Gobblin Tunnel #742

Closed

sahilTakiar added a commit that referenced this pull request Feb 23, 2016

Merge pull request #541 from navteniev/master

07be6b5

Add support for tunneling JDBC traffic over an HTTP proxy

sahilTakiar merged commit 07be6b5 into apache:master Feb 23, 2016

sahilTakiar pushed a commit to sahilTakiar/gobblin that referenced this pull request Mar 3, 2016

Fixing FindBugs warnings for gobblin-tunnel from apache#541

bf0475a

sahilTakiar mentioned this pull request Mar 3, 2016

Fixing FindBugs warnings for gobblin-tunnel from #541 #791

Merged

sahilTakiar added a commit that referenced this pull request Mar 3, 2016

Merge pull request #791 from sahilTakiar/findBugsTunnel

c847c85

Fixing FindBugs warnings for gobblin-tunnel from #541

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tunneling JDBC traffic over an HTTP proxy #541

Add support for tunneling JDBC traffic over an HTTP proxy #541

kunalkandekar commented Dec 17, 2015

chavdar commented Dec 18, 2015

kunalkandekar commented Dec 18, 2015

kunalkandekar commented Dec 22, 2015

kunalkandekar commented Dec 22, 2015

kunalkandekar commented Dec 23, 2015

chavdar commented Jan 7, 2016

sahilTakiar commented Jan 7, 2016

sahilTakiar commented Jan 7, 2016

kunalkandekar commented Jan 7, 2016

liyinan926 commented Jan 7, 2016

kunalkandekar commented Jan 11, 2016

sahilTakiar commented Jan 11, 2016

kunalkandekar commented Jan 11, 2016

chavdar commented Jan 11, 2016

pcadabam-zz commented Jan 11, 2016

pcadabam-zz Jan 11, 2016

kunalkandekar Jan 12, 2016

sahilTakiar commented Jan 11, 2016

pcadabam-zz Jan 11, 2016

pcadabam-zz commented Jan 11, 2016

sahilTakiar Jan 21, 2016

kunalkandekar Jan 22, 2016

sahilTakiar Jan 25, 2016

sahilTakiar commented Feb 2, 2016

kunalkandekar commented Feb 3, 2016

sahilTakiar commented Feb 10, 2016

kunalkandekar commented Feb 10, 2016

sahilTakiar commented Feb 10, 2016

kunalkandekar commented Feb 18, 2016

sahilTakiar commented Feb 23, 2016

kunalkandekar commented Feb 24, 2016

Add support for tunneling JDBC traffic over an HTTP proxy #541

Add support for tunneling JDBC traffic over an HTTP proxy #541

Conversation

kunalkandekar commented Dec 17, 2015

chavdar commented Dec 18, 2015

kunalkandekar commented Dec 18, 2015

kunalkandekar commented Dec 22, 2015

kunalkandekar commented Dec 22, 2015

kunalkandekar commented Dec 23, 2015

chavdar commented Jan 7, 2016

sahilTakiar commented Jan 7, 2016

sahilTakiar commented Jan 7, 2016

kunalkandekar commented Jan 7, 2016

liyinan926 commented Jan 7, 2016

kunalkandekar commented Jan 11, 2016

sahilTakiar commented Jan 11, 2016

kunalkandekar commented Jan 11, 2016

chavdar commented Jan 11, 2016

pcadabam-zz commented Jan 11, 2016

pcadabam-zz Jan 11, 2016

Choose a reason for hiding this comment

kunalkandekar Jan 12, 2016

Choose a reason for hiding this comment

sahilTakiar commented Jan 11, 2016

pcadabam-zz Jan 11, 2016

Choose a reason for hiding this comment

pcadabam-zz commented Jan 11, 2016

sahilTakiar Jan 21, 2016

Choose a reason for hiding this comment

kunalkandekar Jan 22, 2016

Choose a reason for hiding this comment

sahilTakiar Jan 25, 2016

Choose a reason for hiding this comment

sahilTakiar commented Feb 2, 2016

kunalkandekar commented Feb 3, 2016

sahilTakiar commented Feb 10, 2016

kunalkandekar commented Feb 10, 2016

sahilTakiar commented Feb 10, 2016

kunalkandekar commented Feb 18, 2016

sahilTakiar commented Feb 23, 2016

kunalkandekar commented Feb 24, 2016