🎉 New implementation for AWS Secret Manager for issue #19690

mauricioalarcon · 2022-11-22T01:31:29Z

What

Initial implementation for AWS Secret Manager for issue
#10518

Added new implementation AWSSecretManagerPersistence and integration tests AWSSecretManagerPersistenceIntegrationTest

How

A new implementation of SecretPersistence to support AWS Secret Manager AWSSecretManagerPersistence
New Integration tests as suggested on the open Issue, similar to GCP secret manager

🚨 User Impact 🚨

No breaking changes, you need a new set of AWS credentials and AWS cli installed in order to run integration test.

Also note that the credentials need the following

"secretsmanager:GetResourcePolicy",
"secretsmanager:GetSecretValue",
"secretsmanager:DescribeSecret",
"secretsmanager:ListSecretVersionIds"
"secretsmanager:DeleteSecret"

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Feature Implemenation

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
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
PR name follows PR naming conventions

Tests

Unit

*Building Airbyte Sub Build: PLATFORM
Type-safe dependency accessors is an incubating feature.

Configure project :
configuring docker task for airbyte-bootloader
configuring docker task for airbyte-connector-builder-server
configuring docker task for airbyte-container-orchestrator
configuring docker task for airbyte-cron
configuring docker task for airbyte-proxy
configuring docker task for airbyte-server
configuring docker task for airbyte-temporal
configuring docker task for airbyte-webapp
configuring docker task for airbyte-workers
configuring docker task for init
configuring docker task for db-lib
configuring docker task for reporter

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1s
13 actionable tasks: 13 up-to-date
*

Integration

Gist with results here

Acceptance

Gist with results here

marcosmarxm · 2022-11-22T20:26:16Z

Hello 👋, first thank you for this amazing contribution.

We really appreciate the effort you've made to improve the project.
We ask you patience for the code review. Last month our team was focused on Hacktoberfest event and that probably left some PR without the proper feedback. And this week, due to the Thanksgiving US Holiday, most our team is out of office with their families. Another important piece of information why code won't be merge this week is: as a safety measure the core team has decided to freeze merging code to main branch to keep the release stable. Next week we'll return to you with the proper code review and update the status of your contribution.

If you have any questions feel free to send me a message in Slack!
Thanks!

mauricioalarcon · 2022-11-23T14:22:54Z

Hi @marcosmarxm, thanks for your reply; I understand Thanksgiving is a slow week for all of us. I'm looking forward to any feedback you guys have on this.

In the meantime, I've been merging from master any new changes to my branch.

Happy Thanksgiving

Mauricio

davinchia · 2022-11-29T17:03:11Z

Thanks for the contribution @mauricioalarcon !

@pmossman will take a look over the next few days!

mauricioalarcon · 2022-11-30T23:43:02Z

Thanks for the contribution @mauricioalarcon !

@pmossman will take a look over the next few days!

Thank you @davinchia - I'm looking forward to hear from @pmossman

pmossman · 2022-12-01T00:24:52Z

Hey @mauricioalarcon , thanks for the contribution! This is on my todo-list for this week. At first glance, it looks good to me, but I'll want to spend some time trying it out before I approve it. I'll keep you updated!

pmossman · 2022-12-01T00:37:15Z

Actually @mauricioalarcon it looks like our Snyk security scan found a vulnerability introduced by that aws-secretsmanager package that your PR introduces:

Perhaps there's a newer version of the package with that vulnerability fixed? Or perhaps there's a way to force one of the fixed versions of jackson-databind as indicated in the screenshot?

mauricioalarcon · 2022-12-01T18:14:43Z

Actually @mauricioalarcon it looks like our Snyk security scan found a vulnerability introduced by that aws-secretsmanager package that your PR introduces:

Perhaps there's a newer version of the package with that vulnerability fixed? Or perhaps there's a way to force one of the fixed versions of jackson-databind as indicated in the screenshot?

Interesting, yes, I see there's an updated version. I've just bumped it; I also added some new argument checks.

I think this should be fixed by now, I see it now brings com.fasterxml.jackson.core:jackson-databind:2.13.3 (*)

pmossman · 2022-12-01T22:56:14Z

Hmm that's odd, Snyk is now reporting a different security issue, this time for jackson-databind:2.12.3 even though it looks like gradle is ultimately using 2.13.3

I might need to pull in some extra eyes on this one for debugging gradle dependencies, I'll try to check back in tomorrow, sorry for the trouble @mauricioalarcon !

pmossman · 2022-12-02T23:11:39Z

@mauricioalarcon I've determined that Snyk's complaint about the dependency is a false alarm, since we declare an updated version of jackson-databind globally, but Snyk isn't able to detect it. I've manually marked your PR as resolved in Snyk so we shouldn't see that check fail any more.

I'm currently looking into how our CI system will run the integration test for the AWS secret manager. We have some specialized setup for testing our GCP secret manager in our CI, so I'm going to see what we need to do to make the AWS secret manager work in a similar fashion.

It might be non-trivial, since I think we have a dedicated GCP project stood up for integration tests, and we might need to create something similar for AWS.

I'll keep working to move this forward, just wanted to give you a quick end-of-week update. Have a good weekend!

mauricioalarcon · 2022-12-05T18:05:36Z

Thanks @pmossman, for the update and work on this. The only thing needed for the integration test will be just an account where we can run these.
Also, if you feel, I think I can put together a unit test that makes uses of mockito and assert the same functionality go along or avoid the integration test if that gets to complex.

lmk if I can help in any way, shape or form.

pmossman · 2022-12-05T18:17:45Z

...e/src/main/java/io/airbyte/config/persistence/split_secrets/AWSSecretManagerPersistence.java

+   * @see SecretCache
+   */
+  public AWSSecretManagerPersistence() {
+    this.client = AWSSecretsManagerClientBuilder.defaultClient();


@mauricioalarcon before we get this PR merged, can you incorporate the AWS Secret Manager into https://github.com/airbytehq/airbyte/blob/92f71accea6674f04cabf1bcdc8a48f7f4866905/airbyte-config/config-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretPersistence.java similar to GOOGLE_SECRET_MANAGER and VAULT?

We have two "modes" for secret persistence layers, "ephemeral" and "long-lived". For Google Secret Manager, we define separate TTLs for each, but for Vault, both use the same underlying implementation. I think for now, we can use the same implementation of the AWS Secret Manager for both, unless you want to take a look at the Google internals and replicate something similar for AWS, by all means go for it!

One final important note: these other secret persistence classes use credentials and configurations from our Configs.java, and I think it'd make sense to add some new configs for AWS Secrets Manager so that we're not relying on any defaults.

For example, these configs power the Vault-based secret persistence: https://github.com/airbytehq/airbyte/blob/master/airbyte-config/config-models/src/main/java/io/airbyte/config/Configs.java#L134-L150

And the implementation for these methods are here: https://github.com/airbytehq/airbyte/blob/master/airbyte-config/config-models/src/main/java/io/airbyte/config/EnvConfigs.java#L396-L409

This will make it easier for various environments to configure the AWS Secret manager, without needing to rely on default profiles, which can get particularly cumbersome in some environments where multiple profiles exist.

Let me know if you have any questions around any of that! Really appreciate your contribution here :)

Thanks @pmossman all of these changes make sense. I'll work on these and send an update to the PR.

@pmossman I adjusted the PR with the requested changes; I added the new config keys as part of the instructions at docs/operator-guides/configuring-airbyte.md, I'm not sure where else I need to update, if any.

As for the "ephemeral", that's not an option for AWS; that will require a bit more logic to mimic the TTL. I'll research a bit more on this, I'll keep you updated.

Thank you

mauricioalarcon · 2022-12-09T19:11:05Z

@pmossman - I think I know why the deployments were failing; due to formatting ? sorry, - I've just ran formatter and updated the PR.

pmossman · 2022-12-12T17:47:04Z

@mauricioalarcon I think this is very close, I'm just looking into a failing acceptance test that I believe is unrelated to your change. I think the s3 dependency version bump that I included in this PR is causing an S3-related connector to fail in our test suite, so I'm going to revert it in a separate branch and see what happens to our tests. Your changes in this PR look great so once I figure out this final dependency issue we should be good to merge

mauricioalarcon · 2022-12-12T23:03:57Z

@pmossman Thanks for the update - let me know if I can help.

airbytehq#10518 Added new implementation AWSSecretManagerPersistence and integration tests AWSSecretManagerPersistenceIntegrationTest A new implementation of SecretPersistence to support AWS Secret Manager AWSSecretManagerPersistence New Integration tests as suggested on the open Issue, similar to GCP secret manager

- Added new argument check for AWS Secret Manager.

…ind from being pulled in

- Externalized configurations for AWS Secret Manager into Configs and EnvConfigs.java as requested. - Adjusted test to use new constructor for `AWSSecretManagerPersistence` - Augmented instructions configuring-airbyte.md with the new config keys.

pmossman

@mauricioalarcon I verified that your PR passes our full suite of acceptance tests on another branch of mine, and I verified that the security warning from Snyk can be disregarded as it isn't smart enough to see that we're pulling in a fixed version of transitive dependencies.

Thanks so much for your contribution and for making the AWS Secret Manager compatible with our EnvConfigs setup! Sorry this took so much back and forth, we're still working on making our tooling around community PRs more streamlined, and your particular PR gave us some great data points for improving the process.

Thanks again!

mauricioalarcon · 2022-12-12T23:34:57Z

@pmossman That's amazing - thanks a lot; I'm happy to help with such a fantastic product that's Airbyte.

Gauravshah · 2023-02-16T04:29:42Z

...persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretPersistence.java

+      case AWS_SECRET_MANAGER -> {
+        return Optional.of(new AWSSecretManagerPersistence(configs.getAwsAccessKey(), configs.getAwsSecretAccessKey()));
+      }


any reason to not support IAM role based authentication ?

The Airbyte project at large doesn't seem to support role based auth at all, which is pretty rough.

madianas21 · 2023-04-25T09:15:42Z

Are there any docs on how to fetch secrets from AWS Secret Manager ?

octavia-squidington-iv added the community label Nov 22, 2022

mauricioalarcon mentioned this pull request Nov 22, 2022

🎉 New Implementation of SecretPersistence to support AWS secret manager. #19688

Closed

4 tasks

mauricioalarcon requested a review from a team as a code owner November 28, 2022 16:55

davinchia requested a review from pmossman November 29, 2022 17:02

pmossman temporarily deployed to more-secrets December 1, 2022 00:13 Inactive

marcosmarxm assigned pmossman Dec 2, 2022

pmossman force-pushed the aws-secret-support branch from daa5324 to e3ac0ad Compare December 5, 2022 17:56

pmossman requested changes Dec 5, 2022

View reviewed changes

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Dec 9, 2022

mauricioalarcon requested a review from pmossman December 9, 2022 04:48

pmossman temporarily deployed to more-secrets December 9, 2022 16:45 — with GitHub Actions Inactive

pmossman temporarily deployed to more-secrets December 9, 2022 21:38 — with GitHub Actions Inactive

pmossman force-pushed the aws-secret-support branch from 07dda51 to 56f990c Compare December 12, 2022 16:24

pmossman temporarily deployed to more-secrets December 12, 2022 16:30 — with GitHub Actions Inactive

pmossman temporarily deployed to more-secrets December 12, 2022 18:44 — with GitHub Actions Inactive

mauricioalarcon and others added 7 commits December 12, 2022 15:18

Updated version to aws-secretsmanager-caching-java:1.0.2

7496aca

- Added new argument check for AWS Secret Manager.

exclude jackson-databind from aws dependency and run gradle format

0516cdf

bump aws-related dependencies to prevent old version of jackson-datab…

f0510af

…ind from being pulled in

Formatting code.

1665a36

revert dependency version bump

a24ca65

pmossman force-pushed the aws-secret-support branch from 78f4bf1 to a24ca65 Compare December 12, 2022 23:18

pmossman approved these changes Dec 12, 2022

View reviewed changes

pmossman merged commit f76833e into airbytehq:master Dec 12, 2022

mauricioalarcon deleted the aws-secret-support branch December 12, 2022 23:41

github-actions bot mentioned this pull request Dec 13, 2022

Bump helm chart version reference to 0.43.3 #20408

Merged

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

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

mohitreddy1996 mentioned this pull request Jan 3, 2023

airbyte worker should be configured with the correct secrets manager based on env vars configured #20997

Closed

11 tasks

Gauravshah reviewed Feb 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 New implementation for AWS Secret Manager for issue #19690

🎉 New implementation for AWS Secret Manager for issue #19690

mauricioalarcon commented Nov 22, 2022 •

edited

marcosmarxm commented Nov 22, 2022

mauricioalarcon commented Nov 23, 2022

davinchia commented Nov 29, 2022

mauricioalarcon commented Nov 30, 2022

pmossman commented Dec 1, 2022

pmossman commented Dec 1, 2022

mauricioalarcon commented Dec 1, 2022 •

edited

pmossman commented Dec 1, 2022 •

edited

pmossman commented Dec 2, 2022

mauricioalarcon commented Dec 5, 2022

pmossman Dec 5, 2022

mauricioalarcon Dec 7, 2022

mauricioalarcon Dec 9, 2022 •

edited

mauricioalarcon commented Dec 9, 2022

pmossman commented Dec 12, 2022

mauricioalarcon commented Dec 12, 2022

pmossman left a comment

mauricioalarcon commented Dec 12, 2022

Gauravshah Feb 16, 2023

kjoconnor Jun 22, 2023

madianas21 commented Apr 25, 2023

🎉 New implementation for AWS Secret Manager for issue #19690

🎉 New implementation for AWS Secret Manager for issue #19690

Conversation

mauricioalarcon commented Nov 22, 2022 • edited

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Tests

marcosmarxm commented Nov 22, 2022

mauricioalarcon commented Nov 23, 2022

davinchia commented Nov 29, 2022

mauricioalarcon commented Nov 30, 2022

pmossman commented Dec 1, 2022

pmossman commented Dec 1, 2022

mauricioalarcon commented Dec 1, 2022 • edited

pmossman commented Dec 1, 2022 • edited

pmossman commented Dec 2, 2022

mauricioalarcon commented Dec 5, 2022

pmossman Dec 5, 2022

Choose a reason for hiding this comment

mauricioalarcon Dec 7, 2022

Choose a reason for hiding this comment

mauricioalarcon Dec 9, 2022 • edited

Choose a reason for hiding this comment

mauricioalarcon commented Dec 9, 2022

pmossman commented Dec 12, 2022

mauricioalarcon commented Dec 12, 2022

pmossman left a comment

Choose a reason for hiding this comment

mauricioalarcon commented Dec 12, 2022

Gauravshah Feb 16, 2023

Choose a reason for hiding this comment

kjoconnor Jun 22, 2023

Choose a reason for hiding this comment

madianas21 commented Apr 25, 2023

mauricioalarcon commented Nov 22, 2022 •

edited

mauricioalarcon commented Dec 1, 2022 •

edited

pmossman commented Dec 1, 2022 •

edited

mauricioalarcon Dec 9, 2022 •

edited