SSH for Postgres Source #5742

cgardens · 2021-08-30T20:19:43Z

What

Borrows most of the actual SSH logic from @airbyte-jenny 's original PR: https://github.com/airbytehq/airbyte/pull/5438/files.
The goal of this PR is to add SSH only for the Postgres Source. It does not try to deal with how it can be reused for other sources or destinations.

How

Handles injecting the SSH tunnel in the check, discover, read methods of PostgresSource directly. This make it relatively straightforward to inject without worrying about touching other jdbc dbs.

Pre-merge Checklist

add secrets to GH
publish version

cgardens · 2021-08-30T20:23:28Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183623144

cgardens · 2021-08-30T20:41:01Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183674111
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183674111

cgardens · 2021-08-30T20:57:39Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183716158
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183716158

cgardens · 2021-08-30T21:32:16Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183820042
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1183820042

jrhizor · 2021-08-30T21:49:05Z

.../airbyte/integrations/io/airbyte/integration_tests/sources/PostgresSourceAcceptanceTest.java

@@ -105,7 +106,10 @@ protected String getImageName() {

  @Override
  protected ConnectorSpecification getSpec() throws Exception {
-    return Jsons.deserialize(MoreResources.readResource("spec.json"), ConnectorSpecification.class);
+    final ConnectorSpecification originalSpec = Jsons.deserialize(MoreResources.readResource("spec.json"), ConnectorSpecification.class);


Should this use a function to match the actual getspec?

added it in ssh helpers

.../src/test/java/io/airbyte/integrations/source/postgres/PostgresJdbcSourceAcceptanceTest.java

...rbyte/integrations/io/airbyte/integration_tests/sources/CdcPostgresSourceAcceptanceTest.java

jrhizor · 2021-08-30T21:50:42Z

airbyte-integrations/bases/base-java/build.gradle

@@ -5,6 +5,12 @@ plugins {

 dependencies {
    implementation 'commons-cli:commons-cli:1.4'
+    implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'


nit: formatting doesn't match the 'commons-cli:commons-cli:1.4' style we're using elsewhere.

...rs/source-postgres/src/main/java/io/airbyte/integrations/source/postgres/PostgresSource.java

sherifnada · 2021-08-30T22:23:37Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SshTunnel.java

+      Preconditions.checkNotNull(tunnelSshPort);
+      Preconditions.checkNotNull(user);
+      Preconditions.checkArgument(sshKey != null || password != null,
+          "SSH Tunnel was requested to be opened while it was already open.  This is a coding error.");


is this error message correct? i'm not sure how it relates to the condition?

nope. fixed.

sherifnada · 2021-08-30T22:26:12Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SshTunnel.java

+      this.tunnelDatabasePort = tunnelDatabasePort;
+
+      this.sshclient = createClient();
+      this.tunnelSession = openTunnel(sshclient);


why are we opening a tunnel in the constructor of this object? shouldn't this either be opened explicitly or be re-entrant/a singleton?

why? the original version was a mix between a factory and this. so i just picked one of the patterns. what does singleton or reentrant get us here?

sorry, I mixed two concerns in my initial comment.

The main unexpected behavior I'm seeing here is that we instantiate a tunnel upon creating an object. It just seemed unintuitive. I guess this is fine since it's meant to be used in a try-with-resources?

I remember there was a case where we got into trouble for trying to open the tunnel twice (because read calls check or something) hence the re-entrant suggestion, so we don't create two tunnels.

sherifnada · 2021-08-30T22:35:05Z

airbyte-integrations/bases/base-java/src/main/resources/ssh-tunnel-spec.json

+          "multiline": true,
+          "order": 4
+        },
+        "tunnel_db_remote_host": {


IMO this should be the current hostname field, no need to add an extra host field.

Transparently we should replace the host field with localhost and use this in the SSH tunnel config instead.

ah. good point. let me see what i need to do to make that work.

@sherifnada - i followed the path here. it complicates the code a fair amount, but it does provide a better use experience. i think it is the right thing to do, but would love to get your eyes on it before I go forward. You can either look at this PR as it is now or if you want to see exactly what had to change see this commit: d55049d.

The thing that is a bit complicated is that you need to add logic that:

knows what fields in the config are the host and port

replaces those with localhost and the port that we use for the tunnel

make sure the source actually uses the new config

Because this is tricky, I went ahead and did the same SshSource decorator that I did for the destination. I think that at least contains all of the complexity in one spot.

Nice job! I think you placed it in a good place

airbyte-jenny

Marked as approve so I'm non-blocking. The only important change is to modify that toString() method so creds don't end up in a log file. The rest is just style.

airbyte-jenny · 2021-08-30T22:29:55Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SshTunnel.java

+        ", tunnelSshPort='" + tunnelSshPort + '\'' +
+        ", user='" + user + '\'' +
+        ", sshkey='" + sshkey + '\'' +
+        ", password='" + password + '\'' +


The secrets (key, pass) should not be included in the toString method, as it can leak secrets to logs.

airbyte-jenny · 2021-08-30T22:31:28Z

...rs/source-postgres/src/main/java/io/airbyte/integrations/source/postgres/PostgresSource.java

-
-    if (check.getStatus().equals(AirbyteConnectionStatus.Status.FAILED)) {
-      throw new RuntimeException("Unable establish a connection: " + check.getMessage());
+    final SshTunnel tunnel = SshTunnel.getInstance(config);


You're getting a tunnel instance here but I don't see a call to open the tunnel. Is that a coding mistake, or is the call happening farther in?

i changed the structure of the class so that when the tunnel class is instantiated it immediately creates the actual ssh tunnel.

cgardens · 2021-08-31T06:31:10Z

@sherifnada do you have any opinions on the spec. are these the clearest names? I did a rename within the constructor of SshTunnel to names that were easier for me to keep track of. LMK if you like those better and I'll swap them into the spec.

One additional thing, I'm considering is removing the name "database" from any of the fields. After all, all of the SSH machinery can be used for any resource that we want to access via an ssh tunnel. The only remenents of it being specific to a db at this point is that some fields have the word database in them. so something like:

tunnel_db_remote_host => tunnel_resource_remote_host
tunnel_db_remote_port => tunnel_resource_remote_port

wdyt?

cgardens · 2021-08-31T06:35:49Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1185049046
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1185049046

cgardens · 2021-08-31T17:09:55Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1187063893
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1187063893

sherifnada · 2021-08-31T21:05:33Z

tunnel_db_remote_host => tunnel_resource_remote_host
tunnel_db_remote_port => tunnel_resource_remote_port

My only gripe here is that for the foreseeable future, we're only gonna be doing this for DBs. I think the actual variable name is better with your change, but the title should say database not resource. At least until we modify the injectSshConfig method to allow setting the title.

cgardens · 2021-08-31T22:43:28Z

My only gripe here is that for the foreseeable future, we're only gonna be doing this for DBs.

@sherifnada agreed. it is one of those things that is pretty painful to change that's why i was thinking about being forward thinking. i'll do as you suggest, i will change the names in the spec to say resource, but the titles will say db. thanks!

once that's done, i'll publish and merge.

cgardens · 2021-09-01T16:58:36Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1191076063
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1191076063

cgardens · 2021-09-02T12:40:56Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194204683
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194204683

cgardens · 2021-09-02T13:51:06Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194443376
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194443376

cgardens · 2021-09-02T15:36:48Z

/publish connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194813214
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/1194813214

github-actions bot added the area/connectors Connector related issues label Aug 30, 2021

cgardens requested review from jrhizor, sherifnada and airbyte-jenny August 30, 2021 20:20

jrhizor temporarily deployed to more-secrets August 30, 2021 20:25 Inactive

jrhizor temporarily deployed to more-secrets August 30, 2021 20:43 Inactive

jrhizor temporarily deployed to more-secrets August 30, 2021 21:00 Inactive

jrhizor temporarily deployed to more-secrets August 30, 2021 21:34 Inactive

jrhizor approved these changes Aug 30, 2021

View reviewed changes

cgardens mentioned this pull request Aug 30, 2021

SSH for Postgres Destination #5743

Merged

4 tasks

sherifnada reviewed Aug 30, 2021

View reviewed changes

airbyte-jenny approved these changes Aug 30, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets August 31, 2021 06:38 Inactive

jrhizor temporarily deployed to more-secrets August 31, 2021 17:12 Inactive

sherifnada approved these changes Aug 31, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets September 1, 2021 17:01 Inactive

jrhizor temporarily deployed to more-secrets September 2, 2021 12:43 Inactive

github-actions bot added the area/documentation Improvements or additions to documentation label Sep 2, 2021

autogenerated finals

ef6d819

cgardens added 12 commits September 2, 2021 06:50

add secrets to ci

42f7880

fix those tests

5fe371d

clean

210ae3f

get spec helper

1ae7d73

fix deps style

19052b4

rename

729b406

change config in test

0daa13b

remove extra information from spec

b30f87f

renames

861ec7a

fix get spect test

6502529

renames for consistencey

e089a0b

add readme

437fb14

cgardens force-pushed the cgardens/pg_ssh branch from 37f685d to 437fb14 Compare September 2, 2021 13:50

cgardens changed the base branch from cgardens/pg_ssh_final to master September 2, 2021 13:50

jrhizor temporarily deployed to more-secrets September 2, 2021 13:53 Inactive

cgardens added 3 commits September 2, 2021 07:30

format

6f6fb44

add docs

79b3563

bump version

62cfc14

jrhizor temporarily deployed to more-secrets September 2, 2021 15:39 Inactive

cgardens merged commit 7bf531a into master Sep 2, 2021

cgardens deleted the cgardens/pg_ssh branch September 2, 2021 18:32

jrhizor mentioned this pull request Sep 3, 2021

Bump Airbyte version from 0.29.13-alpha to 0.29.14-alpha #5831

Merged

sherifnada mentioned this pull request Sep 8, 2021

[EPIC] SSH tunnel support for OLTP DB sources & destinations #5901

Closed

7 tasks

This was referenced Sep 10, 2021

Add support & test cases for MSSQL Source via SSH tunnels #5966

Closed

Add support & test cases for MySQL Source via SSH tunnels #5967

Closed

Add support & test cases for Oracle Source via SSH tunnels #5968

Closed

alexandr-shegeda mentioned this pull request Oct 20, 2021

🎉 Source Clickhouse: add SSH tunnel support #7213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH for Postgres Source #5742

SSH for Postgres Source #5742

cgardens commented Aug 30, 2021 •

edited

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

jrhizor Aug 30, 2021

cgardens Aug 30, 2021

cgardens Aug 30, 2021

jrhizor Aug 30, 2021

sherifnada Aug 30, 2021

cgardens Aug 31, 2021

sherifnada Aug 30, 2021

cgardens Aug 31, 2021

sherifnada Aug 31, 2021

sherifnada Aug 30, 2021

cgardens Aug 31, 2021

cgardens Aug 31, 2021

sherifnada Aug 31, 2021

airbyte-jenny left a comment

airbyte-jenny Aug 30, 2021

airbyte-jenny Aug 30, 2021

cgardens Aug 30, 2021 •

edited

Loading

cgardens commented Aug 31, 2021 •

edited

Loading

cgardens commented Aug 31, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 31, 2021 •

edited by github-actions bot

Loading

sherifnada commented Aug 31, 2021

cgardens commented Aug 31, 2021 •

edited

Loading

cgardens commented Sep 1, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading

SSH for Postgres Source #5742

SSH for Postgres Source #5742

Conversation

cgardens commented Aug 30, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

cgardens commented Aug 30, 2021 • edited by github-actions bot Loading

cgardens commented Aug 30, 2021 • edited by github-actions bot Loading

cgardens commented Aug 30, 2021 • edited by github-actions bot Loading

cgardens commented Aug 30, 2021 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airbyte-jenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens commented Aug 31, 2021 • edited Loading

cgardens commented Aug 31, 2021 • edited by github-actions bot Loading

cgardens commented Aug 31, 2021 • edited by github-actions bot Loading

sherifnada commented Aug 31, 2021

cgardens commented Aug 31, 2021 • edited Loading

cgardens commented Sep 1, 2021 • edited by github-actions bot Loading

cgardens commented Sep 2, 2021 • edited by github-actions bot Loading

cgardens commented Sep 2, 2021 • edited by github-actions bot Loading

cgardens commented Sep 2, 2021 • edited by github-actions bot Loading

cgardens commented Aug 30, 2021 •

edited

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 30, 2021 •

edited by github-actions bot

Loading

cgardens Aug 30, 2021 •

edited

Loading

cgardens commented Aug 31, 2021 •

edited

Loading

cgardens commented Aug 31, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 31, 2021 •

edited by github-actions bot

Loading

cgardens commented Aug 31, 2021 •

edited

Loading

cgardens commented Sep 1, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading

cgardens commented Sep 2, 2021 •

edited by github-actions bot

Loading