🐛Destination-dynamodb: enforce ssl connection #18672

etsybaev · 2022-10-30T20:13:56Z

What

Users are able to use a non-secure connection

How

Updated check method to enforce SSL connection.

🚨 User Impact 🚨

No breaking changes expected

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.

github-actions · 2022-10-30T20:14:56Z

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

destination-dynamodb

etsybaev · 2022-10-30T20:20:02Z

/test connector=connectors/destination-dynamodb

🕑 connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3356926115
✅ connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3356926115
No Python unittests run

Build Passed

Test summary info:

All Passed

...dynamodb/src/main/java/io/airbyte/integrations/destination/dynamodb/DynamodbDestination.java

github-actions · 2022-11-01T14:42:45Z

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

destination-dynamodb

etsybaev · 2022-11-01T14:43:20Z

/test connector=connectors/destination-dynamodb

🕑 connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3370301467
✅ connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3370301467
No Python unittests run

Build Passed

Test summary info:

All Passed

suhomud

LGTM

…e-ssl-connection

github-actions · 2022-11-01T15:46:07Z

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

Run integration tests
Bump connector version
Add changelog
Publish the new version

✅ Sources (0)

Connector	Version	Changelog	Publish

See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (1)

Connector	Version	Changelog	Publish
`destination-dynamodb`	`0.1.7`	✅	✅

See "Actionable Items" below for how to resolve warnings and errors.

Actionable Items

(click to expand)

Category	Status	Actionable Item
Version	❌ mismatch	The version of the connector is different from its normal variant. Please bump the version of the connector.
	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
Changelog	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
	❌ changelog missing	There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish	⚠ not in seed	The connector is not in the seed file (e.g. `source_definitions.yaml`), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.
	❌ diff seed version	The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the `/publish` command to publish the latest version.

etsybaev · 2022-11-01T16:41:33Z

etsybaev · 2022-11-01T19:36:09Z

/publish connector=connectors/destination-dynamodb

🕑 Publishing the following connectors:
connectors/destination-dynamodb
https://github.com/airbytehq/airbyte/actions/runs/3372319361

Connector	Did it publish?	Were definitions generated?
connectors/destination-dynamodb	✅	✅

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

grishick

If I am reading the code correctly, this change will always enforce HTTPS for DynamoDB. However, we only need this to happen in Airbyte Cloud and we want to continue allowing non-SSL connections in Open Source. @ryankfu could you walk @etsybaev through how to introduce strict encrypt variant to an existing destination connector w/o creating a new image and how to add the check only in Airbyte Cloud scope (what you did for destination-S3)?

ryankfu · 2022-11-02T07:14:14Z

Hi @etsybaev, in case you weren't aware there's a PR that captures a very similar change here. Basically the main pieces that would help to separate this functionality so that it only affects cloud is to:

create a new DynamoDestinationRunner and DynamoDestinationStrictEncrypt in the same package as DynamoDestination
update dynamo destination's build.gradle to point to the DynamoDestinationRunner

If you have any questions regarding the work, let's set up a quick 1:1 to knowledge share. For most context the Runner class will act similar to a case scenario where it will run a different class based on the environment variable

Also note that you'd probably want to create a new StrictEncryptTest so that it only tests for this behavior in an isolated manner

...main/java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncrypt.java

.../java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncryptTest.java

ryankfu

minor code reusability changes but overall looks good

ryankfu · 2022-11-02T23:28:19Z

.../java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncryptTest.java

+  }
+
+  /**
+   * Test that check failes if user is using a non-secure (http) connection


nit: type for fails

Updated, thanks

ryankfu · 2022-11-02T23:29:34Z

build.gradle

@@ -156,7 +156,7 @@ spotless {
    format 'styling', {
        target createSpotlessTarget(['**/*.yaml', '**/*.json'])

-        prettier()
+//        prettier()


How come you've commented out the prettier() method? presume this is simply pretty printing but regardless doesn't seem relevant to this PR

Accidentally committed. Already rolled back.
How this could come? There was a version bumping change in check-style some time ago. Since then some linux users started seeing issues with importing and refreshing project in IntellijIdea but still works when executing build from console. I raised that issue but it seems like the author was busy with other tasks and wasn't going to switch back to his changes. I tried to check it in my free time and found that the issue comes from "prettier" and then stuck. Also found a few similar issues raised on the internet but without answers. So since then every time I have to comment it while working with code and then uncomment from time to time to run "./gradlew --no-daemon format --scan" command to format code and then comment it back.
Here is what the issue looks like for me in IntellijIdea when I refresh gradle project with uncommented Prettier

ryankfu

Overall looks good, there's a minor nit and a question regarding a prettier() method getting commented out in build.gradle

…e-ssl-connection

etsybaev · 2022-11-03T08:53:39Z

/test connector=connectors/destination-dynamodb

🕑 connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3384239320
✅ connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3384239320
No Python unittests run

Build Passed

Test summary info:

All Passed

grishick

@etsybaev now that you've made the changes to enforce SSL only in Cloud you also need to bump the version again and publish the newer version before merging

…e-ssl-connection

grishick

Approved assuming you'll bump the connector version and publish ;)

etsybaev · 2022-11-03T18:58:57Z

/test connector=connectors/destination-dynamodb

🕑 connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3388504702
✅ connectors/destination-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3388504702
No Python unittests run

Build Passed

Test summary info:

All Passed

etsybaev · 2022-11-03T19:41:28Z

etsybaev · 2022-11-03T19:48:47Z

/publish connector=connectors/destination-dynamodb

🕑 Publishing the following connectors:
connectors/destination-dynamodb
https://github.com/airbytehq/airbyte/actions/runs/3388820224

Connector	Did it publish?	Were definitions generated?
connectors/destination-dynamodb	✅	✅

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

[16283] Destination-dynamodb: enforce ssl connection

ea39075

github-actions bot added the area/connectors Connector related issues label Oct 30, 2022

octavia-squidington-iv added the connectors/destination/dynamodb label Oct 30, 2022

etsybaev linked an issue Oct 30, 2022 that may be closed by this pull request

Add ability to enforce SSL in DynamoDB Destination connector #16283

Closed

etsybaev changed the title ~~[16283] Destination-dynamodb: enforce ssl connection~~ 🐛Destination-dynamodb: enforce ssl connection Oct 30, 2022

suhomud self-assigned this Nov 1, 2022

suhomud reviewed Nov 1, 2022

View reviewed changes

...dynamodb/src/main/java/io/airbyte/integrations/destination/dynamodb/DynamodbDestination.java Outdated Show resolved Hide resolved

Minor refactor

cc52c13

suhomud approved these changes Nov 1, 2022

View reviewed changes

Merge branch 'master' into etsybaev/16283-destination-dynamodb-enforc…

c2073c4

…e-ssl-connection

bumped version

75f13db

github-actions bot added the area/documentation Improvements or additions to documentation label Nov 1, 2022

auto-bump connector version

24e8d3e

octavia-squidington-iii temporarily deployed to more-secrets November 1, 2022 20:00 Inactive

grishick suggested changes Nov 1, 2022

View reviewed changes

Added strict-encrypt runner

d687af8

etsybaev temporarily deployed to more-secrets November 2, 2022 10:26 Inactive

etsybaev requested a review from ryankfu November 2, 2022 10:43

ryankfu reviewed Nov 2, 2022

View reviewed changes

...main/java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncrypt.java Outdated Show resolved Hide resolved

ryankfu reviewed Nov 2, 2022

View reviewed changes

.../java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncryptTest.java Outdated Show resolved Hide resolved

ryankfu reviewed Nov 2, 2022

View reviewed changes

.../java/io/airbyte/integrations/destination/dynamodb/DynamodbDestinationStrictEncryptTest.java Outdated Show resolved Hide resolved

ryankfu suggested changes Nov 2, 2022

View reviewed changes

etsybaev temporarily deployed to more-secrets November 2, 2022 23:08 Inactive

etsybaev requested a review from ryankfu November 2, 2022 23:08

ryankfu reviewed Nov 2, 2022

View reviewed changes

ryankfu approved these changes Nov 2, 2022

View reviewed changes

etsybaev temporarily deployed to more-secrets November 3, 2022 06:42 Inactive

minor fix

6178874

etsybaev temporarily deployed to more-secrets November 3, 2022 08:14 Inactive

Merge branch 'master' into etsybaev/16283-destination-dynamodb-enforc…

528f3a8

…e-ssl-connection

etsybaev temporarily deployed to more-secrets November 3, 2022 08:55 Inactive

etsybaev requested review from grishick November 3, 2022 09:30

grishick suggested changes Nov 3, 2022

View reviewed changes

Merge branch 'master' into etsybaev/16283-destination-dynamodb-enforc…

6688844

…e-ssl-connection

etsybaev temporarily deployed to more-secrets November 3, 2022 17:07 Inactive

grishick approved these changes Nov 3, 2022

View reviewed changes

Bumped versino

5baf6be

etsybaev temporarily deployed to more-secrets November 3, 2022 19:46 Inactive

auto-bump connector version

127dcd4

octavia-squidington-iii temporarily deployed to more-secrets November 3, 2022 20:12 Inactive

etsybaev merged commit 5c870c1 into master Nov 3, 2022

etsybaev deleted the etsybaev/16283-destination-dynamodb-enforce-ssl-connection branch November 3, 2022 20:15

grishick added the team/destinations Destinations team's backlog label Nov 8, 2022

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

🐛Destination-dynamodb: enforce ssl connection #18672

🐛Destination-dynamodb: enforce ssl connection #18672

Uh oh!

Conversation

etsybaev commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

Uh oh!

github-actions bot commented Oct 30, 2022

Uh oh!

etsybaev commented Oct 30, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Passed

Uh oh!

Uh oh!

github-actions bot commented Nov 1, 2022

Uh oh!

etsybaev commented Nov 1, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Passed

Uh oh!

suhomud left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected Connector Report

✅ Sources (0)

✅ Destinations (1)

Actionable Items

Uh oh!

etsybaev commented Nov 1, 2022

Uh oh!

etsybaev commented Nov 1, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grishick left a comment

Choose a reason for hiding this comment

Uh oh!

ryankfu commented Nov 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryankfu left a comment

Choose a reason for hiding this comment

Uh oh!

ryankfu Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

etsybaev Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

ryankfu Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

etsybaev Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

ryankfu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etsybaev commented Nov 3, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Passed

Uh oh!

grishick left a comment

Choose a reason for hiding this comment

Uh oh!

grishick left a comment

Choose a reason for hiding this comment

etsybaev commented Oct 30, 2022 •

edited

Loading

etsybaev commented Oct 30, 2022 •

edited by github-actions bot

Loading

etsybaev commented Nov 1, 2022 •

edited by github-actions bot

Loading

github-actions bot commented Nov 1, 2022 •

edited

Loading

etsybaev commented Nov 1, 2022 •

edited by github-actions bot

Loading

ryankfu left a comment •

edited

Loading

etsybaev commented Nov 3, 2022 •

edited by github-actions bot

Loading

etsybaev commented Nov 3, 2022 •

edited by github-actions bot

Loading

etsybaev commented Nov 3, 2022 •

edited by github-actions bot

Loading