Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[EPIC] Support additional parameters input for JDBC sources & destinations #9501

Closed
2 tasks
tuliren opened this issue Jan 14, 2022 · 17 comments
Closed
2 tasks

[EPIC] Support additional parameters input for JDBC sources & destinations #9501

tuliren opened this issue Jan 14, 2022 · 17 comments
Labels
area/connectors Connector related issues area/databases area/warehouses Epic team/destinations Destinations team's backlog type/enhancement New feature or request

Comments

@tuliren
Copy link
Contributor

tuliren commented Jan 14, 2022 •
edited
Loading

Tell us about the problem you're trying to solve

  • JDBC support appending additional parameters in the URL. This enables users to customize their connection.
  • Some of our JDBC connectors support this feature, but some do not.

Describe the solution you’d like

  • Support this feature in all JDBC based connectors

Describe the alternative you’ve considered or used

N/A

TODOs

  • Support additional parameters for high priority connectors.
  • Support additional parameters for medium priority connectors.
Connector Source Priority Destination Priority Note
MySQL ✅ - ✅ #9434 High
SQL Server ❌ High ❌ High
Postgres ❌ High ❌ High
Redshift ❌ Medium ❌ High
Snowflake ✅ #9623 Medium ✅ #9623 High
Oracle ❌ Medium ❌ Medium
DB2 ❌ Low No such connector -
Clickhouse ❌ Low ❌ Low
CockroachDB ❌ Low No such connector -

Relevant Issues
@tuliren tuliren mentioned this issue Jan 14, 2022
Closed
@noahkawasakigoogle
Copy link
Contributor

🎉

@tuliren tuliren added the Epic label Jan 14, 2022
@noahkawasakigoogle
Copy link
Contributor

Another benefit to this I'm realizing, for a number of JDBC based connectors, there are "StrictEncrypted" versions of those connectors which basically re-use the entire connector but manipulate the SSL JDBC parameter. By allowing any params to be added in, theoretically the base connector could have any SSL behavior desired and some duplicate connectors could be deleted.

@noahkawasakigoogle noahkawasakigoogle mentioned this issue Jan 20, 2022
Merged
16 tasks
@sherifnada
Copy link
Contributor

@tuliren some UX questions about this approach:

  1. Are JDBC params the right interface to give to the user? (vs. exposing granular targeted options e.g: "Enable SSL" which manipulate the JDBC string under the hood)
  2. What is the QA story for exposing JDBC params? Since the user can configure any param, do we expect them to all work out of the box? Would they interfere with any assumptions the connector currently makes?
  3. what is the correct behavior if a user tries to setup a JDBC param which was already set by the connector (e.g: they disable encryption in the JDBC string but enable it in the checkbox field or use the strict-encrypt connector)?

@noahkawasakigoogle
Copy link
Contributor

noahkawasakigoogle commented Jan 24, 2022 •
edited
Loading

Hey @sherifnada I can reply to these with my thoughts because I've had to think about the same scenarios:

  1. It would be a lot of overhead for every connector to have targeted options for every possible parameter, as there would need to be potentially 20+ options for each JDBC based connector. Every time someone realizes they want another option exposed to be changeable, another PR would be needed, and another release, and another update. As drivers add new features, Airbyte must keep up with constant updates. It wouldn't be hard code-wise. But it's a lot of work for Airbyte to commit to over time, especially with an infinite amount of connectors some day. Allowing user set parameters solves this once and for all.
  2. Documenting certain required or "suggested" parameter values and known issues could solve this, but its definitely a real problem. Things like SSL, schema/database settings, are very common and mostly uniform across JDBC drivers, having Airbyte expose those as generic options feels right. While uncommon driver specific things like Snowflake's QUERY_TAG could be flexibly set with this.
  3. This could be as simple as wrapping each parameter Airbyte wants to set a value for with:
// If not already set, set this default
if (!config.get("jdbc_url_params").contains("some_param")) {
  jdbcUrl.append("&some_param=aribyte_default_value")
}

And then adding a test suite that asserts it's possible to override things. And then document that if nothing is set, a value will be set for some particular values.

I personally did not include that in the Snowflake PR I put up because those two parameters were things that, in my opinion and reading of the comments, should always have those values (but this should be documented now that I think about it).

A con about this approach I'll point out is the security vulnerabilities with allowing arbitrary parameters be set in this string. As JDBC URL's are a common target for attacks by specifying certain things like allowLocalInfile for MYSQL allowing reading host filesystem files. However this issue exists currently with the host/database or any other string field so Airbyte will eventually need to add some restrictions someday anyways.

@tuliren
Copy link
Contributor Author

tuliren commented Jan 25, 2022

Noah, thank you for the thorough reply.

The only thing to add is that if there are some parameters that should not be set by the user for security concerns, we can document it, ignore the user input just for those parameters, and writing a warning in the log.

@alafanechere
Copy link
Contributor

The only thing to add is that if there are some parameters that should not be set by the user for security concerns, we can document it, ignore the user input just for those parameters, and writing a warning in the log.

@tuliren do you think this is a blocker for the merge of Noah's PR's #9623? For Snowflake, I can't see any hardcoded JDBC params that could be overwritten by the user's input.

@ChristopheDuong ChristopheDuong added this to the ConnCore Feb 16, 2021 milestone Feb 9, 2022
@ChristopheDuong ChristopheDuong self-assigned this Feb 9, 2022
@luisazuluaga
Copy link

Hi guys it's me, any ETA to promote this change , we are looking for MSQL additional parameters to start a PoC with our customer, thanks for your help in advance.

@ChristopheDuong ChristopheDuong removed their assignment Feb 9, 2022
@sherifnada
Copy link
Contributor

Assigning to @tuliren for scoping a generalized solution

@sherifnada
Copy link
Contributor

@luisazuluaga we'll share an ETA once we know it!

@tuliren
Copy link
Contributor Author

tuliren commented Feb 13, 2022 •
edited
Loading

This can be the second project for Alexandre. Here is the project doc that adds support for additional parameters for all JDBC destinations. I am taking it out of the GL queue for now.

@Tolsto Tolsto mentioned this issue Feb 18, 2022
Closed
@tuliren tuliren assigned girarda and unassigned tuliren Feb 23, 2022
@girarda
Copy link
Contributor

girarda commented Feb 23, 2022

I'm working on this right now.
ETA for a generic solution is by end of week. PR is #10421
Once that's merged, we'll only need to update the connectors spec

@sherifnada sherifnada removed this from the ConnCore Feb 23, 2022 milestone Feb 23, 2022
This was referenced Feb 23, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@sherifnada sherifnada removed this from the ConnCore March 2, 2022 milestone Mar 2, 2022
@lfzuluagah
Copy link

@girarda @tuliren one of my team members finished the MSSQL source with additional URL params, let us know if it's appropriate to generate a PR request to review this contribution.

@girarda
Copy link
Contributor

girarda commented Mar 2, 2022

@lfzuluagah a PR would be great! Feel free to submit one at your convenience

Closed
14 tasks
This was referenced May 18, 2022
Closed
Merged
@philippeboyd
Copy link
Contributor

@lfzuluagah any news on the PR from your team mate?

@lfzuluagah
Copy link

lfzuluagah commented Jul 25, 2022 via email

@philippeboyd
Copy link
Contributor

philippeboyd commented Aug 2, 2022 •
edited
Loading

I've encountered a couple of commits in the code potentially related to this feature since mid-July: 5edf3fe and 063b01f; both made by @ryankfu

Wondering if this ever made it to the connectors?

Source MSSQL: when I try to set the JDBC param queryTimeout=0 to disable query timeout or even super high (queryTimeout=6000), I still get a MSSQL query timeout after 10 minutes which seems to be the default server side. See issue and comment on #14459 (comment)

@grishick grishick added the team/destinations Destinations team's backlog label Sep 27, 2022
@evantahler
Copy link
Contributor

Closing!
All the linked issues appear to be complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/connectors Connector related issues area/databases area/warehouses Epic team/destinations Destinations team's backlog type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests