[source-mysql-v2] cdk and spec change for ssl #45351

xiaohansong · 2024-09-09T22:24:32Z

What

https://github.com/airbytehq/airbyte-internal-issues/issues/9669

mysql-v2 supports SSL

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2024-09-09T22:24:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Sep 17, 2024 7:31pm

xiaohansong · 2024-09-10T17:30:53Z

...te-cdk/bulk/toolkits/extract-jdbc/src/main/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtils.kt

@@ -0,0 +1,282 @@
+/*


this file is copied from cdk1.0

https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/core/src/test/kotlin/io/airbyte/cdk/db/util/SSLCertificateUtilsTest.kt

OK, but since this was auto-generate code, it's worth doing a pass to bring this code up to a better standard of quality.

Furthermore, please update source-firetruck to use this code.

xiaohansong · 2024-09-10T17:33:45Z

...2/src/main/kotlin/io/airbyte/integrations/source/mysql/MysqlSourceConfigurationJsonObject.kt

+    "To always require encryption and verify that the source has a valid SSL certificate."
+)
+@SuppressFBWarnings(value = ["NP_NONNULL_RETURN_VIOLATION"], justification = "Micronaut DI")
+class SslVerifyIdentity : Encryption {


@postamar I'm actually not sure what's the best practice there. SslVerifyIdentity shares the same members as SslVerifyCertificate, but they just match to different JsonSchemaTitle and description.

Here I simply copied them. Should I have class SslVerifyIdentity extending from SslVerifyCertificate instead?

Copying is OK here. This code is not going to change much, ever. If it's really unreadable, then using sealed class to abstract shared components is OK. I don't know how well that works with the annotations, though.

The question is, should these objects also be in the CDK, like SSH tunneling? Are they common patterns? Let's look into this once we've migrated all the existing certified sources.

They may share some fields but most connectors have pretty different settings regarding to SSL

Thanks! Good to know 👍

xiaohansong · 2024-09-10T17:34:53Z

...dk/bulk/toolkits/extract-jdbc/src/test/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtilsTest.kt

this file is copied from cdk 1.0

Like the other, this file needs to be rewritten.

postamar

My biggest blocker is that the copied-over code needs to be improved as part of this PR. Everything else is minor.

airbyte-integrations/connectors/source-mysql-v2/metadata.yaml

airbyte-integrations/connectors/source-mysql-v2/build.gradle

postamar · 2024-09-10T17:36:08Z

airbyte-cdk/bulk/toolkits/extract-jdbc/build.gradle

@@ -1,6 +1,7 @@
 dependencies {
    implementation project(':airbyte-cdk:bulk:core:bulk-cdk-core-base')
    implementation project(':airbyte-cdk:bulk:core:bulk-cdk-core-extract')
+    implementation 'org.apache.httpcomponents:httpcore:4.4'


Why do we need this dependency? I don't see any imports from it

for SSLContext: org.apache.http.ssl.SSLContexts.custom().loadTrustMaterial(trustStore, null)

postamar · 2024-09-10T18:22:16Z

...dk/bulk/toolkits/extract-jdbc/src/test/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtilsTest.kt

+                "LUNeYd9wXefwMrEWwDn0DZSsShZmgJoppA15qOnq+FVW/bhZwRv5L4l3AJv0SGoA\n" +
+                "o7DXxD0VGHDA6aC4tJssZbrnoDCBPzYmt9s9GwVupuEroJHZ0Wks4pt4Wx50DUgA\n" +
+                "KC3v0Mo/gg==\n" +
+                "-----END CERTIFICATE-----\n")


These should be """ ... """ blocks

postamar · 2024-09-10T18:23:00Z

...te-cdk/bulk/toolkits/extract-jdbc/src/main/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtils.kt

@@ -0,0 +1,282 @@
+/*


OK, but since this was auto-generate code, it's worth doing a pass to bring this code up to a better standard of quality.

.../source-mysql-v2/src/main/kotlin/io/airbyte/integrations/source/mysql/MysqlJdbcEncryption.kt

postamar

This is close.

...te-cdk/bulk/toolkits/extract-jdbc/src/main/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtils.kt

postamar · 2024-09-13T00:00:11Z

...te-cdk/bulk/toolkits/extract-jdbc/src/main/kotlin/io/airbyte/cdk/jdbc/SSLCertificateUtils.kt

+                } catch (ex2: InvalidKeySpecException) {
+                    KeyFactory.getInstance("EC").generatePrivate(spec)
+                }
+            }


.../source-mysql-v2/src/main/kotlin/io/airbyte/integrations/source/mysql/MysqlJdbcEncryption.kt

postamar · 2024-09-13T00:07:05Z

...2/src/main/kotlin/io/airbyte/integrations/source/mysql/MysqlSourceConfigurationJsonObject.kt

+    "To always require encryption and verify that the source has a valid SSL certificate."
+)
+@SuppressFBWarnings(value = ["NP_NONNULL_RETURN_VIOLATION"], justification = "Micronaut DI")
+class SslVerifyIdentity : Encryption {


Thanks! Good to know 👍

postamar

Thanks! Very nice.

postamar · 2024-09-17T16:09:10Z

Sorry for the slow response. I should have approved this yesterday.

cdk and spec change for ssl

9534905

octavia-squidington-iii added connectors/source/mysql-v2 area/connectors Connector related issues CDK Connector Development Kit labels Sep 9, 2024

xiaohansong added 2 commits September 10, 2024 10:14

test and refactor

ca1cdcb

Merge remote-tracking branch 'origin/master' into xiaohan/ssl

451d5e0

vercel bot deployed to Preview September 10, 2024 17:21 View deployment

mark it back to archived

efb3188

xiaohansong marked this pull request as ready for review September 10, 2024 17:30

xiaohansong requested a review from a team as a code owner September 10, 2024 17:30

xiaohansong requested review from rodireich, postamar and akashkulk September 10, 2024 17:30

xiaohansong commented Sep 10, 2024

View reviewed changes

postamar requested changes Sep 11, 2024

View reviewed changes

xiaohansong added 3 commits September 12, 2024 12:00

bouncycastle

e86480f

bump up version

477118e

clean up

f256704

xiaohansong requested a review from postamar September 12, 2024 23:39

postamar reviewed Sep 13, 2024

View reviewed changes

xiaohansong added 3 commits September 13, 2024 10:54

clean up sslcertificateutils

16ad1b0

cert and key are not null either

e62b180

minor fix

51ca40e

xiaohansong requested a review from postamar September 13, 2024 18:23

postamar approved these changes Sep 17, 2024

View reviewed changes

Merge branch 'master' into xiaohan/ssl

4402912

xiaohansong enabled auto-merge (squash) September 17, 2024 17:05

vercel bot deployed to Preview September 17, 2024 17:10 View deployment

xiaohansong added 5 commits September 17, 2024 10:12

spotbug error

d66e961

spotbugs

a43b4a3

format

e23278d

fix spec test

11339f7

more format

c737167

xiaohansong merged commit 6469111 into master Sep 17, 2024
34 checks passed

xiaohansong deleted the xiaohan/ssl branch September 17, 2024 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[source-mysql-v2] cdk and spec change for ssl #45351

[source-mysql-v2] cdk and spec change for ssl #45351

xiaohansong commented Sep 9, 2024 •

edited

Loading

vercel bot commented Sep 9, 2024 •

edited

Loading

xiaohansong Sep 10, 2024

postamar Sep 10, 2024

postamar Sep 11, 2024

xiaohansong Sep 10, 2024

postamar Sep 11, 2024

postamar Sep 11, 2024

xiaohansong Sep 11, 2024

postamar Sep 13, 2024

xiaohansong Sep 10, 2024

postamar Sep 11, 2024

postamar left a comment

postamar Sep 10, 2024

xiaohansong Sep 11, 2024

postamar Sep 10, 2024

postamar Sep 10, 2024

postamar left a comment

postamar Sep 13, 2024

postamar Sep 13, 2024

postamar left a comment

postamar commented Sep 17, 2024

[source-mysql-v2] cdk and spec change for ssl #45351

[source-mysql-v2] cdk and spec change for ssl #45351

Conversation

xiaohansong commented Sep 9, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Sep 9, 2024 • edited 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

postamar 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

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

postamar commented Sep 17, 2024

xiaohansong commented Sep 9, 2024 •

edited

Loading

vercel bot commented Sep 9, 2024 •

edited

Loading