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 Data Lake: avoid System.setProperty in assume role mode #50971

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Jan 7, 2025

DefaultS3FileIOAwsClientFactory calls s3FileIOProperties.applyCredentialConfigurations, which calls awsClientProperties.credentialsProvider, which prioritizes the accessKey property over AwsClientProperties.CLIENT_CREDENTIALS_PROVIDER.

so stop setting that property entirely in assume role mode. Also make our creds provider a bit better.

(also, kill the region system property, which means we're now doing everything via the iceberg props 🎉 )

Copy link

vercel bot commented Jan 7, 2025

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

Name Status Preview Comments Updated (UTC)
airbyte-docs 🛑 Canceled (Inspect) Jan 9, 2025 9:47pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/destination/iceberg-v2 labels Jan 7, 2025
@edgao edgao requested a review from frifriSF59 January 7, 2025 23:26
Base automatically changed from frifri/rbac-iceberg-v2 to master January 9, 2025 17:09
@edgao edgao changed the title derp Destination S3 Data Lake: avoid System.setProperty in assume role mode Jan 9, 2025
@edgao
Copy link
Contributor Author

edgao commented Jan 9, 2025

fyi I pushed 3cd2c47, which seems to work? It kills the System.setProperty(region) thing, which was actually causing transient test failures (b/c our tests are hitting buckets in different regions, so this was transiently causing tests to try and use the wrong region)

it works locally on the tests I spot-checked 🤷

(cc @subodh1810 since you ran into the problem originally - is there some other problem I'm missing?)

@edgao edgao marked this pull request as ready for review January 9, 2025 21:26
@edgao edgao requested a review from a team as a code owner January 9, 2025 21:26
AwsProperties.CLIENT_ASSUME_ROLE_EXTERNAL_ID to externalId,
AwsClientProperties.CLIENT_CREDENTIALS_PROVIDER to
GlueCredentialsProvider::class.java.name,
"${AwsClientProperties.CLIENT_CREDENTIALS_PROVIDER}.$AWS_CREDENTIALS_MODE" to
Copy link
Contributor

@frifriSF59 frifriSF59 Jan 9, 2025

Choose a reason for hiding this comment

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

The fact that this is the expected way of handling those scenarios is blowing my mind.

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jan 9, 2025
AssumeRoleRequest.builder()
.externalId(properties[ASSUME_ROLE_EXTERNAL_ID])
.roleArn(properties[ASSUME_ROLE_ARN])
.roleSessionName("airbyte-sts-session")
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this end up in CloudTrail and is intended to be used as an audit tool. I wonder if we should have some sort of client/workspace identifier in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this from https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/bulk/toolkits/load-s3/src/main/kotlin/io/airbyte/cdk/load/file/s3/S3Client.kt#L202 :P (probably should actually point at a shared constant)

we don't actually have the workspace/connection ID at runtime unfortunately (... we really should get platform to pass that through to us though)

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.

3 participants