[fastlane] Fix S3ClientHelper side effects #16687
Conversation
atreat
force-pushed
the
fix/s3-client-helper-side-effects
branch
from
10a47de
to
329497a
Compare
atreat
changed the title
janpio
changed the title
|
Could this be a breaking change to anyone or is this deep enough in the code that we do not think anyone uses this directly? |
|
I couldn't find any other callers of the S3ClientHelper. It is possible that people call this directly though. I can add back in the original initializer and print out a deprecation notice if that is best. |
atreat
force-pushed
the
fix/s3-client-helper-side-effects
branch
3 times, most recently
from
2700e46
to
8b528c6
Compare
|
Realized I could pass the access_key/secret to the underlying S3 client if they exist. This allows for the existing interface to be used and removes the need to call |
|
@janpio let me know if you have any feedback on the updates |
The initializer was calling Aws.config.update which was overwriting existing configuration. This was preventing access when using STS/Aws::AssumeRoleCredentials to assume a temporary IAM role. Removing the need to initialize with access_key/secret allows the caller to update Aws.config before calling fastlane and having their expected credentials used. Credentials will now be pulled from Amazon's documented sources. Maintains `region` option by initializing the S3 client with this option.
This is mainly to be used to inject this dependency in for specs, but if callers feel they would rather configure an s3 client this allows them to do so.
Reverts changes to keep initializer interface non-breaking. Updates creation of underlying S3 client to support passed in credentials (access_key/secret). Will use AWS config otherwise
atreat
force-pushed
the
fix/s3-client-helper-side-effects
branch
from
8b528c6
to
ea46158
Compare
|
Hey @atreat 👋 Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉 Please let us know if this change requires an immediate release by adding a comment here 👍 |
There was a problem hiding this comment.
Congratulations! 🎉 This was released as part of fastlane 2.151.0 🚀
* Updates S3ClientHelper to prevent side effects The initializer was calling Aws.config.update which was overwriting existing configuration. This was preventing access when using STS/Aws::AssumeRoleCredentials to assume a temporary IAM role. Removing the need to initialize with access_key/secret allows the caller to update Aws.config before calling fastlane and having their expected credentials used. Credentials will now be pulled from Amazon's documented sources. Maintains `region` option by initializing the S3 client with this option. * Updates match's S3Storage object to initialize S3ClientHelper with new interface. * Updates S3ClientHelper with option s3_client initializer option This is mainly to be used to inject this dependency in for specs, but if callers feel they would rather configure an s3 client this allows them to do so. * Updates S3ClientHelper with much more versatile initializer Reverts changes to keep initializer interface non-breaking. Updates creation of underlying S3 client to support passed in credentials (access_key/secret). Will use AWS config otherwise * Reverts change in S3Storage that prevented access_key/secret from being used if passed in
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validMotivation and Context
When testing the changes in #16682 I found that my authentication was not working because the S3ClientHelper was overwriting the Aws config I expected. This change will remove that side-effect and allow the caller to dictate what AWS credentials should be used.
Description
Updates the S3ClientHelper to remove the access_key/secret initializer parameters. Removes the call to
Aws.config.update. Instead, passes the remainingregionparameter to the S3 client when initializing that.Also cleans up some confusing code in the file. The
clientmethod was defined as private outside of the S3ClientHelper class. The S3ClientHelper then had a publicattr_reader :clientthat allowed the underlying client to leak out. This has been updated such that there is a privateclientmethod that is not accessible outside the helper class as I believe the intention is to always use the interface defined by the helper.Testing Steps
Updated the necessary call sites in match's S3Storage and specs. Specs for the S3ClientHelper continue to pass.