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

Destination S3 / S3 Data Lake: use micronaut injection instead of System.getenv for assume role #51618

Open
wants to merge 6 commits into
base: edgao/micronaut_initial_cleanup
Choose a base branch
from

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Jan 18, 2025

rebased on #51600.

recommend reviewing by commit:

  1. add wiring for connectors to enable additional micronaut environments at runtime, and update tests appropriately
    1. Feedback appreciated - if there's some way to do this automatically, that would be much nicer (i.e. as soon as you add the toolkit-load-aws dependency, can we enable the aws environment?)
  2. update s3 to use the aws environment; add application-aws to inject the assume role creds; wire everything together
  3. do the same for s3-data-lake
  4. update base tests + destination-s3 to support passing micronaut properties to the connector (probably start with DestinationProcess + AwsAssumeRoleCredentialsTestUtil, most of the rest of the diff is just passing properties through the callstack)
    1. adds S3V2CsvAssumeRole, which brings the assume role test into the new CDK (!!)
  5. do the same for s3-data-lake (in particular, kill the preexisting envVars thing, which only supported System.getenv)

future PRs:

  • delete all the env var stuff from integration tests
  • switch the USE_FILE_TRANSFER flag to use micronaut properties at test time, instead of mocking an env var
  • figure out dev-null's deployment: mode: thing in its application.yaml

Copy link

vercel bot commented Jan 18, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2025 0:16am

@edgao edgao force-pushed the edgao/connectors_opt_into_envs branch from 8bbbf2d to 81dd6cc Compare January 18, 2025 00:45
@edgao edgao changed the base branch from master to edgao/micronaut_initial_cleanup January 18, 2025 00:55
@edgao edgao force-pushed the edgao/connectors_opt_into_envs branch from 13b1d37 to a5f28ae Compare January 18, 2025 01:09
@edgao edgao force-pushed the edgao/connectors_opt_into_envs branch from a5f28ae to b726430 Compare January 21, 2025 17:25
@edgao edgao force-pushed the edgao/connectors_opt_into_envs branch from b726430 to 628c99b Compare January 21, 2025 17:49
@edgao edgao changed the title Destination S3 / S3 Data Lake: use micronaut injection instead of System.getenv Destination S3 / S3 Data Lake: use micronaut injection instead of System.getenv for assume role Jan 21, 2025
@edgao edgao force-pushed the edgao/micronaut_initial_cleanup branch from 7cfcceb to ebce44e Compare January 21, 2025 17:50
@edgao edgao force-pushed the edgao/connectors_opt_into_envs branch from 628c99b to 8a5a199 Compare January 21, 2025 17:50
@edgao edgao marked this pull request as ready for review January 21, 2025 17:52
@edgao edgao requested review from a team as code owners January 21, 2025 17:52
@frifriSF59
Copy link
Contributor

This is great, Thanks!

Anyway we could write some sort of documentation about what the process should be if want to add new properties to be used in our code?
Do you think that has value?

Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit: One thing to potentially consider is using a masking pattern like Micronaut supports here (https://docs.micronaut.io/latest/guide/#environmentEndpoint) instead of relying a comment to ensure that secrets are printed. Instead of passing a list/map of String around, we could potentially wrap each env var in an object whose toString implements the masking logic you can see in something like EnvironmentEndpoint or EnvironmentFilterSpecification. Definitely something that could be done separate from this if we are really concerned about it.

@edgao
Copy link
Contributor Author

edgao commented Jan 22, 2025

Anyway we could write some sort of documentation about what the process should be if want to add new properties to be used in our code?

probably yeah. lemme post in the team slack and see if we have a typical process for this, it doesn't seem like a good fit for random code comments

masking

the problem is that at the end, we still have to render it down to List<String> (to pass into ProcessBuilder) :/ probably worth doing for the rest of our code though

(for now I'll just override the toString on AwsAssumeRoleCredentials to nothing 🤷 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants