MS SQL Server Destination implementation #3195

masonwheeler · 2021-05-03T23:16:21Z

No description provided.

sherifnada

LGTM but requesting changes to confirm we're not forgetting a required param

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

sherifnada · 2021-05-04T00:48:56Z

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

+        config.get("username").asText(),
+        config.get("password").asText()));
+
+    if (config.has("ssl") && config.get("ssl").asBoolean()) {


ssl on v1, nice!

the SSL stuff is great. I think we need at least one test to check it. There are 2 options here:

just right a one off unit test that runs check connection with SSL turned on and sanity check that it works.

add a test in TestDestination that only runs if the destination is configurable with SSL. it would similarly probably just run check connection.

i don't have a really strong preference either way. 1 is probably easier 🤷‍♀️

...ation-mssql/src/main/java/io/airbyte/integrations/destination/mssql/SqlServerOperations.java

airbyte-integrations/connectors/destination-mssql/src/main/resources/spec.json

marcosmarxm

I was very curious about the implementation of a target, so I was super 🕵️ leave some comments in general LGTM

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

airbyte-integrations/connectors/destination-mssql/src/main/resources/spec.json

airbyte-db/src/main/java/io/airbyte/db/Databases.java

airbyte-integrations/connectors/destination-mssql/Dockerfile

cgardens

one question about why database is being removed from the implementation. after that's resolved i think this is good to go! nice work.

airbyte-integrations/connectors/destination-mssql/Dockerfile

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

...ation-mssql/src/main/java/io/airbyte/integrations/destination/mssql/SqlServerOperations.java

...rc/test-integration/java/io/airbyte/integrations/destination/mssql/MSSQLIntegrationTest.java

sherifnada

I believe we're using params not declared in spec.json so double checking on this.

sherifnada · 2021-05-04T20:53:13Z

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

-      additionalParameters.add("ssl=true");
-      additionalParameters.add("sslmode=require");
+      additionalParameters.add("encrypt=true");
+      if (config.has("trustServerCertificate") && config.get("trustServerCertificate").asBoolean()) {


where would these params come from? I don't see this in spec.json

This has been added to spec.json

airbyte-integrations/connectors/destination-mssql/src/main/resources/spec.json

sherifnada · 2021-05-04T20:55:14Z

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

+        additionalParameters.add("trustServerCertificate=true");
+      } else {
+        additionalParameters.add("trustServerCertificate=false");
+        additionalParameters.add("trustStore=" + config.get("trustStoreName").asText());


this is not declared in spec.json

This has now been added to spec.json.

...ion-test/src/main/java/io/airbyte/integrations/standardtest/destination/TestDestination.java

sherifnada · 2021-05-04T20:58:57Z

/test connector=destination-mssql

🕑 destination-mssql https://github.com/airbytehq/airbyte/actions/runs/811386089
✅ destination-mssql https://github.com/airbytehq/airbyte/actions/runs/811386089

cgardens

nice! a few more comments (each is described in more detail in line):

SSL needs to be in the spec.
SSL needs a test
we shouldn't need to implement getDatabase

airbyte-integrations/connectors/destination-mssql/src/main/resources/spec.json

cgardens · 2021-05-04T20:50:09Z

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

+      if (config.has("trustServerCertificate") && config.get("trustServerCertificate").asBoolean()) {
+        additionalParameters.add("trustServerCertificate=true");
+      } else {
+        additionalParameters.add("trustServerCertificate=false");


how is this getting set? it seems like you want to add some fields for users to optionally configure. if so you need to add them in the spec.json. ping me if the relationship between the 2 isn't clear and i can walk through it!

This has now been added to spec.json.

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

cgardens · 2021-05-04T21:05:17Z

...tination-mssql/src/main/java/io/airbyte/integrations/destination/mssql/MSSQLDestination.java

+        config.get("username").asText(),
+        config.get("password").asText()));
+
+    if (config.has("ssl") && config.get("ssl").asBoolean()) {


the SSL stuff is great. I think we need at least one test to check it. There are 2 options here:

just right a one off unit test that runs check connection with SSL turned on and sanity check that it works.

add a test in TestDestination that only runs if the destination is configurable with SSL. it would similarly probably just run check connection.

i don't have a really strong preference either way. 1 is probably easier 🤷‍♀️

cgardens

Great! 3 other things you need to do in order to get this ready (other than what is mentioned in the comments below:

Please make sure you add a documentation page. You can use the postgres one as an example.
Add this destination to https://github.com/airbytehq/airbyte/blob/master/airbyte-config/init/src/main/resources/seed/destination_definitions.yaml so that this new destination will appear in the UI by default.
Actually run a sync in the UI manually with this destination to sanity check that the E2E experience is good!

...rc/test-integration/java/io/airbyte/integrations/destination/mssql/MSSQLIntegrationTest.java

cgardens · 2021-05-12T21:30:37Z

...test-integration/java/io/airbyte/integrations/destination/mssql/MSSQLIntegrationTestSSL.java

+import org.testcontainers.containers.MSSQLServerContainer;
+import org.testcontainers.utility.DockerImageName;
+
+public class MSSQLIntegrationTestSSL extends TestDestination {


can you create an issue that we should figure out a way to consolidate SSL tests for destinations, so that we don't have top copy and paste test classes like this. definitely don't think we need to worry about doing this right now, but i want us to keep track of it.

cgardens · 2021-05-12T22:14:48Z

airbyte-integrations/connectors/destination-mssql/src/main/resources/spec.json

+      "ssl": {
+        "title": "SSL Connection",
+        "description": "Encrypt data using SSL.",
+        "type": "boolean",
+        "default": false,
+        "order": 6
+      },
+      "trustServerCertificate": {
+        "title": "Trust Server Certificate",
+        "type": "boolean",
+        "default": false,
+        "description": "Whether or not to implicitly trust the server certificate without verification.  (Useful for local test databases.)",
+        "examples": [true, false],
+        "order": 7
+      },
+      "trustStoreName": {
+        "title": "Trust Store Name",
+        "type": "string",
+        "description": "The full path (including filename) to the certificate trustStore file, which contains the list of certificates that the client trusts.",
+        "order": 8
+      },
+      "trustStorePassword": {
+        "title": "Trust Store Password",
+        "type": "string",
+        "description": "The password used to check the integrity of the trustStore data.",
+        "order": 9
+      },
+      "hostNameInCertificate": {
+        "title": "Host Name In Certificate",
+        "type": "string",
+        "description": "Specifies the host name of the server. The value of this property must match the subject property of the certificate.",
+        "order": 10
+      }
+    }
+  }


This seems like it should be a oneOf for clarity. I would suggest replacing this configuration with something that looks like this:

"ssl_method": { "title": "SSL Method", "type": "object", "description": "Encryption method to use when communicating with the database", "order": 6, "oneOf": [ { "title": "Unencrypted", "additionalProperties": false, "description": "Data transfer will not be encrypted.", "required": ["ssl_method"], "properties": { "ssl_method": { "type": "string", "enum": ["unencrypted"], "default": "unencrypted" } } }, { "title": "Encrypted (trust server certificate)", "additionalProperties": false, "description": "Use the cert provided by the server.", "required": ["ssl_method"], "properties": { "ssl_method": { "type": "string", "enum": ["encrypted_trust_server_certificate"], "default": "encrypted_trust_server_certificate" } } }, { "title": "Encrypted (verify certificate)", "additionalProperties": false, "description": "Data transfer will not be encrypted.", "required": ["ssl_method", "trustStoreName", "trustStorePassword"], "properties": { "ssl_method": { "type": "string", "enum": ["encrypted_verify_certificate"], "default": "encrypted_verify_certificate" }, "trustStoreName": { "title": "Trust Store Name", "type": "string", "description": "The full path (including filename) to the certificate trustStore file, which contains the list of certificates that the client trusts.", "order": 8 }, "trustStorePassword": { "title": "Trust Store Password", "type": "string", "description": "The password used to check the integrity of the trustStore data.", "order": 9 }, "hostNameInCertificate": { "title": "Host Name In Certificate", "type": "string", "description": "Specifies the host name of the server. The value of this property must match the subject property of the certificate.", "order": 10 } } } ] }

This displays in the UI as follows:

What do you think? If you adopt this configuration, you'll have to adjust how the config is parsed in the MSSQLDestination and the config that gets passed in the test suites.

sherifnada

LGTM with requested changes from charles

Co-authored-by: Charles <giardina.charles@gmail.com>

Adding the destination to the list Reworking SSL configuration to move internal information out of config

cgardens

Looks good. I think you're ready to ship it.

To deploy this you need to add this command as a comment in this PR:

/publish connector=connectors/source-mssql

You can see an example of this here. That will publish your docker container to docker hub. Once that succeeds, re-run the build in the PR. That should get you past the current build failure (which is just checking that the connector is published).

masonwheeler · 2021-05-17T15:20:49Z

/publish connector=connectors/destination-mssql

🕑 connectors/destination-mssql https://github.com/airbytehq/airbyte/actions/runs/850263597
✅ connectors/destination-mssql https://github.com/airbytehq/airbyte/actions/runs/850263597

After addressing the requested changes, GitHub is glitching, saying there are changes to address. Dismissing the review to get it to go away.

MS SQL Server Destination implementation

c636900

masonwheeler requested review from michel-tricot, ChristopheDuong, cgardens and davinchia May 3, 2021 23:16

auto-assign bot requested review from jrhizor and sherifnada May 3, 2021 23:16

This was linked to issues May 4, 2021

MSSQL Destination Connector #2434

Closed

SQL Server on premise as destination #613

Closed

This was unlinked from issues May 4, 2021

SQL Server on premise as destination #613

Closed

MSSQL Destination Connector #2434

Closed

sherifnada suggested changes May 4, 2021

View reviewed changes

marcosmarxm previously requested changes May 4, 2021

View reviewed changes

ChristopheDuong reviewed May 4, 2021

View reviewed changes

airbyte-integrations/connectors/destination-mssql/Dockerfile Outdated Show resolved Hide resolved

cgardens requested changes May 4, 2021

View reviewed changes

Updating SQL Server destination per points raised in code review.

ea55680

sherifnada suggested changes May 4, 2021

View reviewed changes

cgardens requested changes May 4, 2021

View reviewed changes

masonwheeler and others added 3 commits May 6, 2021 17:52

Fixed configuration to connect to the database the expected Airbyte way.

11365bf

Progressive checkin. SSL is building but failing to run.

fc1045f

fix

e8340b1

masonwheeler requested review from ChristopheDuong, sherifnada, marcosmarxm and cgardens May 12, 2021 19:34

cgardens requested changes May 12, 2021

View reviewed changes

sherifnada approved these changes May 13, 2021

View reviewed changes

masonwheeler and others added 2 commits May 13, 2021 11:10

Remove commented-out code

0ef52c8

Co-authored-by: Charles <giardina.charles@gmail.com>

Changing SSL configuration

7109604

sherifnada linked an issue May 13, 2021 that may be closed by this pull request

SQL Server on premise as destination #613

Closed

Adding documentation

b7a267d

Adding the destination to the list Reworking SSL configuration to move internal information out of config

cgardens approved these changes May 14, 2021

View reviewed changes

Merge branch 'master' into issue_613

c87176c

masonwheeler merged commit a67c769 into master May 17, 2021

masonwheeler deleted the issue_613 branch May 17, 2021 15:54

cgardens mentioned this pull request May 17, 2021

MSSQL Destination Connector #2434

Closed

alexandr-shegeda mentioned this pull request Sep 24, 2021

Add support for MSSQL source/destination via TLS/SSL #6007

Closed

5 tasks

karinakuz added the connectors/destination/e2e-test label Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MS SQL Server Destination implementation #3195

MS SQL Server Destination implementation #3195

masonwheeler commented May 3, 2021

sherifnada left a comment

sherifnada May 4, 2021

cgardens May 4, 2021

marcosmarxm left a comment

cgardens left a comment

sherifnada left a comment

sherifnada May 4, 2021

masonwheeler May 13, 2021

sherifnada May 4, 2021

masonwheeler May 13, 2021

sherifnada commented May 4, 2021 •

edited by github-actions bot

Loading

cgardens left a comment

cgardens May 4, 2021

masonwheeler May 13, 2021

cgardens May 4, 2021

cgardens left a comment

cgardens May 12, 2021

cgardens May 12, 2021

sherifnada left a comment

cgardens left a comment

masonwheeler commented May 17, 2021 •

edited by github-actions bot

Loading

MS SQL Server Destination implementation #3195

MS SQL Server Destination implementation #3195

Conversation

masonwheeler commented May 3, 2021

sherifnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm left a comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

sherifnada left a 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

sherifnada commented May 4, 2021 • edited by github-actions bot Loading

cgardens left a 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

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

masonwheeler commented May 17, 2021 • edited by github-actions bot Loading

sherifnada commented May 4, 2021 •

edited by github-actions bot

Loading

masonwheeler commented May 17, 2021 •

edited by github-actions bot

Loading