-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[tiered-storage] Add support for AWS instance and role creds #4433
Conversation
run integration tests |
the integration test passes locally, the failure was in the offloader test though, so it is possible there is some race condition this slightly introduces? Running them again to see if that is the case... |
This commit makes changes to the tiered storage support for S3 to allow for support of ec2 metadata instance credentials as well as additional config options for assuming a role to get credentials. This works by changing the way we provide credentials to use the funtional `Supplier` interface and for using the AWS specific `SessionCredentials` object for when we detect that the `CredentialProvider` is providing credentials that have a session token.
This changes the s3 handling slightly, instead of falling back to static credentials, we instead now fail if no s3 credentials can be found and change the unit tests to start a broker with s3 credentials. With the new Supplier API, we now fetch credentials on every request. Because of this, the failure and subsequent try/catch is costly and the integration tests were using this, which caused them to be significantly slower. Instead, we just check to see if we can fetch creds, and if we can't consider it an error condition to exit the app as it is unlikely in a production scenario to not have some credentials.
I wasn't fully understanding how the integration tests work by needing to bake a new container first, so I wasn't actually testing this changeset! I found the issue, which was that the try/catch that the unit tests relied on were causing the tests to be massively slower. Added a new commit, which just changes this to ensure we have credentials on boot and then changes the tests to include some static credentials. This could technically be a breaking change if someone in production is using s3/s3-compatible storage without credentials, but that seems somewhat unlikely and is easy worked around by provided some fake static credentials, and this seems like the cleaner choice rather than a warning and not having offloading work as expected. |
okay, was also able to get this deployed to a dev cluster and was able to validate it working with ec2 instance credentials, so assuming I got all the tests fixed, I think this should be good to go |
run java8 tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (apache#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (apache#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (apache#4433)
1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (apache#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (apache#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (apache#4433)
# Motivation The PR #6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433)
# Motivation The PR #6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff)
…ache#8630) # Motivation The PR apache#6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff) (cherry picked from commit 8793963)
…ache#8630) # Motivation The PR apache#6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff) (cherry picked from commit 8793963) (cherry picked from commit 0388a1a)
Motivation
This commit makes changes to the tiered storage support for S3
to allow for support of ec2 metadata instance credentials as well as
additional config options for assuming a role to get credentials.
Currently, because ec2 instance credentials require a session token,
the existing implementation does not let you use credentials provided
by the ec2 metadata API.
Also, the usage of roles can be very helpful, this commit also adds support
for roles.
Modifications
This works by changing the way we provide credentials to use the
funtional
Supplier
interface and for using the AWS specificSessionCredentials
object for when we detect that theCredentialProvider
is providing credentials that have a session token.The creation of the AWSCredentialProvider is moved into the
TieredStorageConfigurationData
objectand two new configuration options are exposed, which are used to provide details of the role name and roleSessionName.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
Documentation