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.

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

🎉 Redshift Source: create secure-only version #7040

Closed

Conversation

VitaliiMaltsev
Copy link
Contributor

@VitaliiMaltsev VitaliiMaltsev commented Oct 14, 2021

What

We want to create secure-only versions of connectors that can be used in the Airbyte cloud. The idea is that these connectors inherently prevent certain insecure connections such as connecting to a database over the public internet without encryption.

How

Modified the connector's spec to hide any options which allow the user to disable TLS. Changed the connector to enable TLS by default if the TLS option is not specified

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? 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
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions
  • Connector added to connector index like described here

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
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command 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
  • Connector version bumped like described here

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
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub 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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


vmaltsev seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Oct 14, 2021
# Conflicts:
#	airbyte-integrations/connectors/destination-redshift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyS3Destination.java
#	airbyte-integrations/connectors/destination-redshift/src/main/resources/spec.json
#	airbyte-integrations/connectors/source-redshift/src/main/java/io/airbyte/integrations/source/redshift/RedshiftSource.java
#	airbyte-integrations/connectors/source-redshift/src/main/resources/spec.json
#	docs/integrations/destinations/redshift.md
@github-actions github-actions bot removed the area/documentation Improvements or additions to documentation label Oct 14, 2021
@VitaliiMaltsev VitaliiMaltsev temporarily deployed to more-secrets October 14, 2021 09:42 Inactive
@VitaliiMaltsev VitaliiMaltsev temporarily deployed to more-secrets October 14, 2021 09:47 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 07:04 Inactive
@VitaliiMaltsev VitaliiMaltsev temporarily deployed to more-secrets October 15, 2021 07:20 Inactive
@VitaliiMaltsev
Copy link
Contributor Author

VitaliiMaltsev commented Oct 15, 2021

/test connector=connectors/source-redshift-strict-encrypt

🕑 connectors/source-redshift-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1345035522
✅ connectors/source-redshift-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1345035522
No Python unittests run

@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 07:40 Inactive
@VitaliiMaltsev VitaliiMaltsev changed the title Redshift Source: create secure-only version 🎉 Redshift Source: create secure-only version Oct 15, 2021
@VitaliiMaltsev VitaliiMaltsev linked an issue Oct 15, 2021 that may be closed by this pull request
6 tasks
Copy link
Contributor

@irynakruk irynakruk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@VitaliiMaltsev VitaliiMaltsev marked this pull request as ready for review October 15, 2021 14:32
Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VitaliiMaltsev, please follow and check off the Pre-merge Checklist to properly register this new connector. Not everything is needed to add a strict encrypt connector, but currently some files are missing (e.g. registering the connector in the source_definitions.yaml file.

Also don't forget to register this new connector in NormalizationRunnerFactory.

vmaltsev added 2 commits October 19, 2021 10:27
# Conflicts:
#	airbyte-integrations/connectors/destination-redshift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyS3Destination.java
#	airbyte-integrations/connectors/source-redshift/src/main/java/io/airbyte/integrations/source/redshift/RedshiftSource.java
@VitaliiMaltsev VitaliiMaltsev temporarily deployed to more-secrets October 19, 2021 07:52 Inactive
@VitaliiMaltsev
Copy link
Contributor Author

Also don't forget to register this new connector in NormalizationRunnerFactory.
@tuliren I don't think we need to register this connector in NormalizationRunnerFactory, cause it's source connector. I added Redshift Destination secure-only to NormalizationRunnerFactory in other PR https://github.com/airbytehq/airbyte/pull/7121/files

@alexandr-shegeda
Copy link
Contributor

@VitaliiMaltsev, please follow and check off the Pre-merge Checklist to properly register this new connector. Not everything is needed to add a strict encrypt connector, but currently some files are missing (e.g. registering the connector in the source_definitions.yaml file.

hi @tuliren
thank you for your comment, just a quick note about adding new connectors to _definitions.yaml, as it was discussed previously with @sherifnada - we will add -strict-encrypt connectors later to the different config files because all of them should be available only for the cloud version

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all redshift clusters support encrypted connections? (I don't remember off the top of my head)

In other words, can we safely assume that we can always use ssl=true in the JDBC URL?

If so, then why don't we always do SSL by default? It eliminates the need for a connector fork and even having this as an option in redshift's config

@VitaliiMaltsev
Copy link
Contributor Author

VitaliiMaltsev commented Oct 20, 2021

Do all redshift clusters support encrypted connections? (I don't remember off the top of my head)

In other words, can we safely assume that we can always use ssl=true in the JDBC URL?

If so, then why don't we always do SSL by default? It eliminates the need for a connector fork and even having this as an option in redshift's config

@sherifnada Yes, the traffic will be encrypted for all redfshift clusters with the included ssl = true option in jdbc url. Do you mean not to create new strict-encrypt connectors, but to enable this option by default in the source-redshift and destination-redshift connectors?

@sherifnada
Copy link
Contributor

@VitaliiMaltsev correct. And also removing the option from the redshift connector and always setting it to true

@VitaliiMaltsev
Copy link
Contributor Author

@VitaliiMaltsev correct. And also removing the option from the redshift connector and always setting it to true

@sherifnada done in #7234. Please review

@sherifnada
Copy link
Contributor

LGTM we can close this one

@swyxio swyxio deleted the vmaltsev/6973-redshift-source-secure-only branch October 12, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants