Add aws sts library to support IRSA for logging

[msaffitz]

What / How

Adds libs.aws.java.sdk.sts to the classpath to expose WebIdentityTokenFileCredentialsProvider for use with IRSA (IAM Roles for Service Accounts) for logging to S3. This (hopefully!) will enable use of S3 without providing keys. The docs on this for v1 of the SDK are little ambiguous; the v2 docs are much clearer. See also the discussion here

This builds on the previous work done in #7766 and supersedes #177.

Can this PR be safely reverted / rolled back?

• YES 💚
• NO ❌

🚨 User Impact 🚨

No known user impact.

[msaffitz]

@bothra90 -- FYI. You mentioned here #177 (comment) that you had to make some changes and were able to get things working. Does this look right to you? Did I miss anything?

[msaffitz]

cc @pmossman -- Thanks for the help with the previous PR; FYI for this follow on.

[bothra90]

@msaffitz: Here's a git patch for what I was able to get working: https://gist.github.com/bothra90/62403b32449c90807ee4b8ec9c14f862. Your PR is in particular missing the necessary changes to log4j2.xml.

[msaffitz]

Thanks @bothra90 . With removing the values from log4j2.xml, did you confirm if logging still works when using AWS keys? I wasn't sure about that part.

(I'm curious about the error you saw with leaving the values in the XML file -- I'd think the null / empty string values here would both be ok? Unfortunately I don't have a good way to test this locally at the moment as our AWS stuff is only configured to pull from official sources).

[bothra90]

Yes, I tested that logging works with the values removed.

[msaffitz]

Awesome, thanks for confirming. I'll update this PR this evening with those additional changes.

[weichunnn]

Thanks @msaffitz @bothra90 - looking forward to this PR getting landed into the main codebase 👀

[pmossman]

@msaffitz can you explain why the removal of these lines are necessary? What happens if you introduce the sts library without removing these?

[msaffitz]

I'm not actually sure; I had the same question above (#263 (comment)) and @bothra90 indicated that removing them was necessary to get things functioning. @bothra90 -- do you have more context on this?

[bothra90]

Yes, I was unable to create a source from the console and seeing the following error log in airbyte-server process:

Cannot end publish with com.van.logging.aws.S3PublishHelper@53f4b9a9 due to error:
Cannot end publishing: Cannot publish to S3: The AWS Access Key Id you provided does not exist in our records. (Service: Amazon S3; Status Code: 403; Error Code:
InvalidAccessKeyId; Request ID: TY5JJZ76SMD9BA86; S3 Extended Request ID: WEK4bCkgavHUqbgoywsSNVMwBt14/ke89r9mC/usS879oBQ1MYNw+S/2urb3xedmfWxWBpfj90TMLqnYL+hCfw==; Proxy: null)

[pmossman]

I discussed this internally with some folks that have more familiarity around our Log4J2 setup. Simply removing these properties won't work -- we use an underlying log4j-appender implementation that will need to support IRSA.

We opened an issue for this in that project so we'll wait and see what the maintainer says.

[bothra90]

FWIW, it did the job for me. Happy to jump on a quick Slack huddle and demo it to you or @davinchia

[pmossman]

It appears that the underlying appender library that I linked above actually supports roles by default. The s3-aws-key and s3-aws-secret properties are optional, for situations where role-based credentials aren't available. That said, I'm concerned that if we simply delete these properties from the xml configuration, we could break Airbyte installations that are using those properties instead of roles.

An ideal solution would be backwards compatible for such installations, though I'm not sure what that would look like, since it seems like as soon as these tags are defined, that appender will try to use them, even if the value is blank

[bothra90]

I understand the concern. Ultimately though, the passed parameters get used here and if the s3AwsSecret and s3AwsKey are not provided, the library will use the default AWS providers chain which will correctly use AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY. This is how these variables in the configuration are populated in the first place, so I don't suspect this would be an issue.

Having said that, I can try to test this out on my end and make sure nothing breaks.

[weichunnn]

Seconded @bothra90's comment. Given that Airbyte is already following the standard naming conversion for storing AWS keys which is how it's currently being passed as environment variables, I also don't see any issue with letting the DefaultAWSCredentialsProviderChain handle it instead of being passed explicitly.

Using the ProviderChain would be backwards compatible with existing setup as any existing or new credentials will just be picked up according to their priority - in this case ENV first followed by roles / instance profile

Reference: https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html

[pmossman]

@bothra90 @weichunnn thanks for pointing this out, and apologies for the slow-moving back and forth here. I agree that it seems like these XML properties look unnecessary. @bothra90 if you're able to test and verify that this works as expected on your end, I'd love to see your results.

Otherwise, I'll open this PR against our internal repo where I can run our full suite of tests. Thanks for bearing with me here!

[weichunnn]

Thanks @pmossman! Checking in to see if there are any updates on the review internally? 👀

[pmossman]

/create-oss-pr

[pmossman]

@msaffitz @bothra90 @weichunnn I was able to test these changes with accessKey/secretKey configuration and verified it's all working as expected. The internal PR is merged, so I am going to close this PR. Thank you all for your engagement and contribution!