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

Create Profile File Provider for Region #682

Merged
merged 9 commits into from
Sep 2, 2021
Merged

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Sep 1, 2021

Motivation and Context

Although credentials can be loaded from the AWS profile, the profile was ignored when resolving regions.

Description

  • Add a profile provider for region
  • unify handling of provider configuration into ProviderConfig. This simplifies and removes boilerplate from the credential provider builders.

Testing

  • Unit tests added

Checklist

  • I have updated the CHANGELOG

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

As part of this work, I unified handling of provider configuration into `ProviderConfig`. This simplifies and removes boilerplate from the credential provider builders.
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Looks good! Mostly ready to approve, but want some clarification on aws-config having its own sleep implementation provider.

aws/rust-runtime/aws-config/src/default_provider.rs Outdated Show resolved Hide resolved
#[derive(Debug)]
pub struct DefaultRegionChain(RegionProviderChain);

impl DefaultRegionChain {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this have a builder() method like DefaultCredentialsChain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍🏻 done

let region_chain = self.region_chain;
let region = self
.region_override
.unwrap_or_else(move || Box::new(region_chain.build()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: seems like there should be a way to do this fallback without boxing.

Comment on lines 208 to 220
mod sleep {
use smithy_async::rt::sleep::{AsyncSleep, TokioSleep};
use std::sync::Arc;

#[cfg(feature = "rt-tokio")]
pub fn default_sleep() -> Option<Arc<dyn AsyncSleep>> {
Some(Arc::new(TokioSleep::new()))
}

#[cfg(not(feature = "rt-tokio"))]
pub fn default_sleep() -> Option<Arc<dyn AsyncSleep>> {
None
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this done here so that it can be an Arc<dyn rather than the Box<dyn that smithy-async returns? Should we add an alternate set of arc functions to smithy-async so that the feature logic doesn't need to be repeated? Or maybe we just decide to always make these runtime implementations Arc to simplify things? Maybe we even new-type Arc<dyn AsyncSleep> to DynAsyncSleep to hide the detail as well.

Edit: Looks like you did some of this in smithy-async in this PR.

/// use aws_config::provider_config::ProviderConfig;
/// use aws_sdk_sts::Region;
/// use aws_config::web_identity_token::WebIdentityTokenCredentialsProvider;
/// let conf = ProviderConfig::without_region().with_region(Some(Region::new("us-east-1")));
Copy link
Collaborator

Choose a reason for hiding this comment

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

without_region followed by with_region is a bit of a head scratcher, but I'm not sure I have a better suggestion. Maybe without_default_region, but then you can follow that with load_default_region and have the same problem.

@rcoh rcoh merged commit f649d55 into main Sep 2, 2021
@rcoh rcoh deleted the region-profile-providers branch September 2, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants