-
Notifications
You must be signed in to change notification settings - Fork 835
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
Object Store (AWS): Support region configured via named profile #4161
Conversation
* moved ProfileProvider to aws::profile module * added aws::region::RegionProvider * lazy-init profile credential provider * support overriding profile region * 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.
Thank you, left some comments
object_store/src/aws/mod.rs
Outdated
use std::env; | ||
|
||
impl RegionProvider for ProfileProvider { | ||
fn get_region(&self) -> Option<String> { |
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.
I can't help feeling this formulation doesn't really test the region inference logic? Is there some way to create a fake profile for testing purposes?
Perhaps https://docs.rs/aws-config/latest/aws_config/profile/profile_file/struct.ProfileFiles.html
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.
Agreed, this only tests the logic in AmazonS3Builder
that falls back on the profile region when a profile is provided.
I'll add a test in the aws::profile
module that uses ProfileFiles to verify the configured profile region is actually what comes out of ProfileProvider::get_region
.
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.
I added a fake profile for testing in ProfileProvider::get_region_provider
. Let me know if you want to see more tests or think this should be organized differently.
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.
Looking good, I think this mostly just needs an integration test of some description
object_store/src/aws/mod.rs
Outdated
|
||
let provider = profile::ProfileProvider::new(profile, None); | ||
|
||
block_on(provider.get_region()) |
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.
block_on(provider.get_region()) | |
Handle::current().block_on(provider.get_region()) |
Using tokio::Handle
might be safer, as I believe the upstream assumes tokio
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.
Good call. Unfortunately, using tokio, calling block_on
on the current runtime results in the following error:
ERROR: Cannot start a runtime from within a runtime. This happens because a function (like block_on) attempted to block the current thread while the thread is being used to drive asynchronous tasks.
According to the tokio::Handle::block_on
docs, we need to spawn a new thread and block there.
use std::{panic, thread};
use tokio::runtime::Handle;
let handle = Handle::current();
let provider = profile::ProfileProvider::new(profile, None);
let result = thread::spawn(move || handle.block_on(provider.get_region()));
match result.join() {
Ok(region) => region,
Err(e) => panic::resume_unwind(e),
}
aws::profile::region -> aws::profile
@tustvold This is ready for your review. |
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.
This looks good to me, the block_on is perhaps a little unfortunate, but we can hopefully clean this up following #4163
object_store/src/aws/mod.rs
Outdated
let result = thread::spawn(move || handle.block_on(provider.get_region())); | ||
|
||
match result.join() { | ||
Ok(region) => region, | ||
Err(e) => panic::resume_unwind(e), | ||
} |
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.
let result = thread::spawn(move || handle.block_on(provider.get_region())); | |
match result.join() { | |
Ok(region) => region, | |
Err(e) => panic::resume_unwind(e), | |
} | |
handle.block_on(provider.get_region()) |
Both are equally "problematic" in that they pin a tokio worker, but this at least avoids spawning an additional thread
Is this still necessary following #4188 ? |
This is definitely far less helpful following #4188. Personally, it doesn't seem right that Object Store supports AWS profiles at all... It seems like that support should happen "further up the stack" (e.g., in DataFusion CLI). User-facing apps built on top of Object Store will know their users best and can decide how to build the necessary integrations to support named profiles, SSO, etc. I think #4163 moves in the right direction and I'd be happy to help make DataFusion CLI the reference implementation for whatever approach the community decides on. But wherever named profile support is implemented, I think it should be complete (i.e., supporting both config and credentials). It's an unusual experience for an AWS user to find that an app supports named profiles, only to receive an error that the region set in their profile config was not used. 🥲 |
Which issue does this PR close?
Closes #4158.
Rationale for this change
Rationale in #4158.
What changes are included in this PR?
ProfileProvider
toaws::profile
moduleaws::region::RegionProvider
AmazonS3Builder::with_region
.Are there any user-facing changes?
When using a named profile, users no longer need to explicitly set a region when using
AmazonS3Builder
.