🚨 Validate source JDBC url parameters 🚨 #14586

ryankfu · 2022-07-11T16:54:02Z

What

Adding in validating logic for the check functionality within Postgres Source connector #13851

EDIT: bumped version for postgres-strict-encrypt here: #14735

How

The solution follows similar to the Postgres Destination connector where the code verifies that JDBC parameters do not overlap with the custom set parameters by the user in jdbc_url_params

NOTE: The order of validation mirrors the AbstractJdbcDestination class where the validation check comes after a toJdbcConfig has been created. There is some questions regarding the bifurcation of why the naming of methods between Source and Destination are not the same for creating a database config but that has been punted in favor of getting this functionality matching the Destination class

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Should not have any breaking changes but it CAN due to the situation where the code now throws an IllegalArgumentException because in the past we did not notify the user their jdbc_url_params had conflicting parameters to the custom parameters set within Postgres

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-07-11T16:54:07Z

All committers have signed the CLA.

grishick

Overall - great job on refactoring! My main concern is the behavior immediate after we upgrade a JDBC source connector to a version that contains this change. We need to avoid breaking sync jobs that were working before.

grishick · 2022-07-11T20:32:34Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/JdbcUtils.java

@@ -62,4 +65,8 @@ public static Map<String, String> parseJdbcParameters(final String jdbcPropertie
    return parameters;
  }

+  public static boolean useSsl(final JsonNode config) {
+    return !config.has(SSL_KEY) || config.get(SSL_KEY).asBoolean();


If I am reading this correctly, useSsl returns true if there is no SSL_KEY in config? Is that the intended behavior?

To be quite frank, I'm not completely sure the reason for why useSsl would want to return true if there is no SSL_KEY. I mainly refactored this portion of code from other parts of the code base

However, it seem that all the places I saw this usage of useSsl if no SSL_KEY has been set in the config object that we want to set ssl in the config. My presumption would be because these connections require ssl to operate correctly. I can update the function with javadoc comment that the code checks for if no SSL_KEY exists we also return true so it's clear in case there's a misunderstanding on the functionality of this method. Would there be a group or individual that knows more about why this method operates the way it does?

This was changed here f308690#diff-f14bc8e9216a505150004f54d41ca7aeeba9a9460fc4a660b11225dd7abb274fR81 and my guess is that if that the default behaviour should be ssl true if the field is not present.

I was able to clarify the logic and will be adding some tests to codify the logic but the asBoolean is the key to understanding this method. The asBoolean function will map a value to a boolean if it can (e.g. <int>0 -> <boolean>false, <string>false -> <boolean>false, <int>3 -> <boolean> true). Basically the logic is:

If SSL_KEY is not set OR (if SSL_KEY is set AND the value of the SSL_KEY is mapped to true) THEN we set ssl fields

grishick · 2022-07-11T20:34:32Z

...ouse/src/main/java/io/airbyte/integrations/destination/clickhouse/ClickhouseDestination.java

@@ -66,9 +67,6 @@ public JsonNode toJdbcConfig(final JsonNode config) {
    return Jsons.jsonNode(configBuilder.build());
  }

-  private boolean useSsl(final JsonNode config) {
-    return !config.has("ssl") || config.get("ssl").asBoolean();


Good job factoring out this copy-pasta!

grishick · 2022-07-11T21:01:27Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+   * @return A mapping of the default connection properties
+   */
+  protected Map<String, String> getDefaultConnectionProperties(final JsonNode config) {
+    return JdbcUtils.parseJdbcParameters(config, "connection_properties", getJdbcParameterDelimiter());


we use "connection_properties" string in 10+ places in Java code, let's make it a public static String constant in JdbcUtils

Same comment as with the usage of ssl and sslmode for constants. I'm happy to make the necessary commit to update these as constants, however I left them out merely to focus on getting the main functionality addressed

grishick · 2022-07-11T21:03:12Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+      final Map<String, String> defaultParameters) {
+    for (final String key : defaultParameters.keySet()) {
+      if (customParameters.containsKey(key) && !Objects.equals(customParameters.get(key), defaultParameters.get(key))) {
+        throw new IllegalArgumentException("Cannot overwrite default JDBC parameter " + key);


Question: when Airbyte tries to run a sync job, will this exception be thrown if a user has created a connection with an older version of the connector that did not perform this validation and then upgraded to the version of connector that performs this validation?

Yes, there will be an exception thrown upon upgrading to this new version where validation would exist. Based on that, it seems like I should note this commit will potentially cause breaking changes.

I'm curious to know if when the validation check was added to the AbstractJdbcDestination did we notify customers ahead of time as well because the validation check did not originally exist to the best of my knowledge. @girarda it seems you were the person that last touched this assertCustomParametersDontOverwriteDefaultParameters method in AbstractJdbcDestination, can you confirm if we notified customers that their jdbc_url parameters could break or how did we approach this?

We didn't notify the customers because it wasn't yet possible to set jdbc_url parameters when we added that check

Given that we would like to have feature parity of validation between Sources and Destinations it seems that this is a concern that I would defer the decision to someone who can make that executive call.

My thoughts would be, while it may be unfortunate that users will have breaking changes, the scenario for a breaking change is mostly if Postgres users have conflicting ssl settings in their jdbc_url in which case they may even already have unexpected results since the jdbc_url parameters may be overwriting our own custom ssl configurations. However, I cannot say with 100% certainty that there might not be other scenarios that could break a customer's jdbc_url parameters so will want to proceed with caution

EDIT: The conclusion reached is to confirm if this validation check is in the critical path for syncs, meaning that this change would break users who already configured their jdbc_url parameters improperly but the code was not aware. If the validation check is in the critical path, opt to instead of throwing an IllegalArgumentException to have a AirbyteTraceMessage. Otherwise if the logic only affects the UI side then remain with throwing an Exception

I think its fine, we should improve the error message to include which property is causing the problem and ask user to remove that property from the setup form

agreed with Subodh

grishick · 2022-07-11T21:04:40Z

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


  static final String DRIVER_CLASS = DatabaseDriver.POSTGRESQL.getDriverClassName();
+  static final Map<String, String> SSL_JDBC_PARAMETERS = ImmutableMap.of(
+      "ssl", "true",


we should use shared constants for "ssl" and "sslmode"

I agree as well, I opted not to add this change into this commit since it's not quite in scope. However, I'm not married to the idea that it should be a separate ticket, if anything it should be a separate commit in case we want to roll back that change. What are your thoughts? Should I include the shared constants in this PR or create a separate ticket to address the ssl and sslmode constants?

grishick · 2022-07-11T21:06:39Z

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

-    final Builder<Object, Object> configBuilder = ImmutableMap.builder()
-        .put("username", config.get("username").asText())
-        .put("jdbc_url", jdbcUrl.toString());
+    final ImmutableMap.Builder<Object, Object> configBuilder = ImmutableMap.builder()


good job getting rid of strings

subodh1810

This makes sense! I would approve once you run all the tests for each of the connector that we are editing here.

subodh1810 · 2022-07-12T15:42:09Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+   */
+  protected Map<String, String> getConnectionProperties(final JsonNode config) {
+    final Map<String, String> customProperties = JdbcUtils.parseJdbcParameters(config, JdbcUtils.JDBC_URL_PARAMS_KEY);
+    final Map<String, String> defaultProperties = getDefaultConnectionProperties(config);


The getDefaultConnectionProperties in master uses the jdbcConfig (output of final JsonNode jdbcConfig = toDatabaseConfig(config); ) not the config but we are changing it to use the config, is that intentional?

Yes, the reason I chose to have config as opposed to jdbcConfig is because it will match the same arguments that AbstractJdbcDestination#getDefaultConnectionProperties gets passed from AbstractJdbcDestination#getDataSource. Also jdbcConfig is merely a subset of config so it will have the same information overall

subodh1810 · 2022-07-12T15:43:25Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+      final Map<String, String> defaultParameters) {
+    for (final String key : defaultParameters.keySet()) {
+      if (customParameters.containsKey(key) && !Objects.equals(customParameters.get(key), defaultParameters.get(key))) {
+        throw new IllegalArgumentException("Cannot overwrite default JDBC parameter " + key);


I think its fine, we should improve the error message to include which property is causing the problem and ask user to remove that property from the setup form

subodh1810 · 2022-07-12T15:44:32Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/JdbcUtils.java

@@ -62,4 +65,8 @@ public static Map<String, String> parseJdbcParameters(final String jdbcPropertie
    return parameters;
  }

+  public static boolean useSsl(final JsonNode config) {
+    return !config.has(SSL_KEY) || config.get(SSL_KEY).asBoolean();


This was changed here f308690#diff-f14bc8e9216a505150004f54d41ca7aeeba9a9460fc4a660b11225dd7abb274fR81 and my guess is that if that the default behaviour should be ssl true if the field is not present.

grishick · 2022-07-12T22:39:32Z

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/2659819852
❌ source-postgres https://github.com/airbytehq/airbyte/actions/runs/2659819852
🐛 https://gradle.com/s/yeuz4wutxwmqu

Build Failed

Test summary info:

Could not find result summary

grishick

Looks like there is a compilation error after the rebase, because @tuliren moved isCdC from PostgresSource to PostgresUtils

ryankfu · 2022-07-13T16:48:09Z

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/2664977213
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/2664977213
No Python unittests run

Build Passed

Test summary info:

All Passed

ryankfu · 2022-07-13T19:02:48Z

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/2665683874
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/2665683874
No Python unittests run

Build Passed

Test summary info:

All Passed

ryankfu · 2022-07-14T23:22:55Z

/publish connector=connectors/source-postgres

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/2673696734

Connector	Did it publish?	Were definitions generated?
connectors/source-postgres	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

grishick · 2022-07-15T01:34:42Z

@ryankfu you can merge this PR now

…ig function

…g imports

ryankfu requested a review from a team as a code owner July 11, 2022 16:54

octavia-squidington-iii added connectors/destination/mysql connectors/destination/clickhouse connectors/destination/postgres connectors/destination/jdbc connectors/source/postgres connectors/source/jdbc labels Jul 11, 2022

github-actions bot added the area/connectors Connector related issues label Jul 11, 2022

ryankfu requested a review from grishick July 11, 2022 16:58

grishick suggested changes Jul 11, 2022

View reviewed changes

ryankfu changed the title ~~Validate source JDBC url parameters~~ 🚨 Validate source JDBC url parameters 🚨 Jul 11, 2022

subodh1810 reviewed Jul 12, 2022

View reviewed changes

ryankfu temporarily deployed to more-secrets July 12, 2022 21:38 Inactive

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from 6d38f20 to b96e09e Compare July 12, 2022 23:29

ryankfu temporarily deployed to more-secrets July 12, 2022 23:32 Inactive

ryankfu temporarily deployed to more-secrets July 13, 2022 01:14 Inactive

grishick suggested changes Jul 13, 2022

View reviewed changes

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from d8af95a to 724e21b Compare July 13, 2022 04:39

ryankfu temporarily deployed to more-secrets July 13, 2022 04:42 Inactive

ryankfu requested a review from grishick July 13, 2022 17:55

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from 724e21b to de6750f Compare July 13, 2022 17:57

ryankfu temporarily deployed to more-secrets July 13, 2022 17:59 Inactive

ryankfu temporarily deployed to more-secrets July 13, 2022 21:07 Inactive

ryankfu temporarily deployed to more-secrets July 13, 2022 21:45 Inactive

grishick approved these changes Jul 13, 2022

View reviewed changes

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from de6750f to 43240fe Compare July 13, 2022 23:49

ryankfu temporarily deployed to more-secrets July 14, 2022 23:23 Inactive

ryankfu mentioned this pull request Jul 14, 2022

Bumping minor version number for postgres strict encrypt #14735

Closed

37 tasks

octavia-squidington-iii temporarily deployed to more-secrets July 15, 2022 00:46 Inactive

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from 6d6c4e2 to 68e14e9 Compare July 15, 2022 01:51

ryankfu temporarily deployed to more-secrets July 15, 2022 01:53 Inactive

ryankfu temporarily deployed to more-secrets July 15, 2022 02:10 Inactive

ryankfu temporarily deployed to more-secrets July 15, 2022 02:34 Inactive

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from 68e14e9 to a8dcaf6 Compare July 15, 2022 04:44

ryankfu temporarily deployed to more-secrets July 15, 2022 04:46 Inactive

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from a8dcaf6 to f1bff81 Compare July 15, 2022 15:33

ryankfu temporarily deployed to more-secrets July 15, 2022 15:36 Inactive

ryankfu temporarily deployed to more-secrets July 15, 2022 17:21 Inactive

ryankfu and others added 10 commits July 15, 2022 10:46

Moved toDatabaseConfigStatic subfunction into the main toDatabaseConf…

4a7c1eb

…ig function

Moved Postgres JDBC_URL_PARAMS_KEY into JdbcUtils to reduce redundancy

6ec6555

Migrated useSsl method into JdbcUtils to remove duplication

82f0a85

Created constants for config parameters to match PostgresDestination

7c1a7ac

Add validation check for overwritten connection properties

6bdb2e0

Added test coverage for useSsl and clarifying javadoc comments

966a274

Updated useSsl tests to match linter and javadoc comments for useSsl

110e9e2

Fixed isCDC method with proper PostgresUtil function and correspondin…

e4f0022

…g imports

Bumping Dockerfile and adding changelog description

13f5835

auto-bump connector version

2e5988f

ryankfu force-pushed the ryan/validate-source-jdbc-params branch from f1bff81 to 2e5988f Compare July 15, 2022 17:46

ryankfu temporarily deployed to more-secrets July 15, 2022 17:49 Inactive

ryankfu merged commit 5edf3fe into master Jul 15, 2022

ryankfu deleted the ryan/validate-source-jdbc-params branch July 15, 2022 18:25

ryankfu mentioned this pull request Jul 19, 2022

Validate JDBC params in check operation #13851

Closed

octavia-squidington-iii mentioned this pull request Jul 23, 2022

Bump Airbyte version from 0.39.37-alpha to 0.39.38-alpha #14976

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚨 Validate source JDBC url parameters 🚨 #14586

🚨 Validate source JDBC url parameters 🚨 #14586

ryankfu commented Jul 11, 2022 •

edited

Loading

CLAassistant commented Jul 11, 2022 •

edited

Loading

grishick left a comment

grishick Jul 11, 2022

ryankfu Jul 11, 2022

subodh1810 Jul 12, 2022

ryankfu Jul 12, 2022

grishick Jul 11, 2022

grishick Jul 11, 2022

ryankfu Jul 11, 2022

grishick Jul 11, 2022

ryankfu Jul 11, 2022

girarda Jul 11, 2022

ryankfu Jul 11, 2022 •

edited

Loading

subodh1810 Jul 12, 2022

grishick Jul 13, 2022

grishick Jul 11, 2022

ryankfu Jul 11, 2022

grishick Jul 11, 2022

subodh1810 left a comment

subodh1810 Jul 12, 2022

ryankfu Jul 12, 2022

subodh1810 Jul 12, 2022

subodh1810 Jul 12, 2022

grishick commented Jul 12, 2022 •

edited by github-actions bot

Loading

grishick left a comment

ryankfu commented Jul 13, 2022 •

edited by github-actions bot

Loading

ryankfu commented Jul 13, 2022 •

edited by github-actions bot

Loading

ryankfu commented Jul 14, 2022 •

edited by github-actions bot

Loading

grishick commented Jul 15, 2022

🚨 Validate source JDBC url parameters 🚨 #14586

🚨 Validate source JDBC url parameters 🚨 #14586

Conversation

ryankfu commented Jul 11, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Jul 11, 2022 • edited Loading

grishick 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

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

ryankfu Jul 11, 2022 • 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

subodh1810 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

grishick commented Jul 12, 2022 • edited by github-actions bot Loading

Build Failed

grishick left a comment

Choose a reason for hiding this comment

ryankfu commented Jul 13, 2022 • edited by github-actions bot Loading

Build Passed

ryankfu commented Jul 13, 2022 • edited by github-actions bot Loading

Build Passed

ryankfu commented Jul 14, 2022 • edited by github-actions bot Loading

grishick commented Jul 15, 2022

ryankfu commented Jul 11, 2022 •

edited

Loading

CLAassistant commented Jul 11, 2022 •

edited

Loading

ryankfu Jul 11, 2022 •

edited

Loading

grishick commented Jul 12, 2022 •

edited by github-actions bot

Loading

ryankfu commented Jul 13, 2022 •

edited by github-actions bot

Loading

ryankfu commented Jul 13, 2022 •

edited by github-actions bot

Loading

ryankfu commented Jul 14, 2022 •

edited by github-actions bot

Loading