From 8952b4558bb3f14845d080be6a93b6c43d4b0c16 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 20 Jul 2022 08:52:06 -0700 Subject: [PATCH 1/4] Remove warning for valid IMDS provider use-case (#1559) --- CHANGELOG.next.toml | 6 ++++++ aws/rust-runtime/aws-config/src/imds/credentials.rs | 3 --- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index bbeefaeed2..336888bc0b 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -116,3 +116,9 @@ a version bump in all of them, but this should not be relied upon. references = ["smithy-rs#1540"] meta = { "breaking" = false, "tada" = false, "bug" = false } author = "jdisanti" + +[[aws-sdk-rust]] +message = "Remove warning for valid IMDS provider use-case" +references = ["smithy-rs#1559", "aws-sdk-rust#582"] +meta = { "breaking" = false, "tada" = false, "bug" = true } +author = "jdisanti" diff --git a/aws/rust-runtime/aws-config/src/imds/credentials.rs b/aws/rust-runtime/aws-config/src/imds/credentials.rs index 9c0ac78aff..e0b574704e 100644 --- a/aws/rust-runtime/aws-config/src/imds/credentials.rs +++ b/aws/rust-runtime/aws-config/src/imds/credentials.rs @@ -63,9 +63,6 @@ impl Builder { /// /// For more information about IMDS client configuration loading see [`imds::Client`] pub fn imds_client(mut self, client: imds::Client) -> Self { - if self.provider_config.is_some() { - tracing::warn!("provider config override by a full client override"); - } self.imds_override = Some(client); self } From e8c20b29e6912bc7737f1480825aba09b12f1ff9 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 20 Jul 2022 08:53:25 -0700 Subject: [PATCH 2/4] Don't warn about home dir expansion for default profile paths (#1558) --- CHANGELOG.next.toml | 10 +++ .../aws-config/src/profile/parser/source.rs | 81 +++++++++---------- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 336888bc0b..a794e5d30a 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -122,3 +122,13 @@ message = "Remove warning for valid IMDS provider use-case" references = ["smithy-rs#1559", "aws-sdk-rust#582"] meta = { "breaking" = false, "tada" = false, "bug" = true } author = "jdisanti" + +[[aws-sdk-rust]] +message = """ +Only emit a warning about failing to expand a `~` to the home +directory in a profile file's path if that path was explicitly +set (don't emit it for the default paths) +""" +references = ["smithy-rs#1558", "aws-sdk-rust#583"] +meta = { "breaking" = false, "tada" = false, "bug" = true } +author = "jdisanti" \ No newline at end of file diff --git a/aws/rust-runtime/aws-config/src/profile/parser/source.rs b/aws/rust-runtime/aws-config/src/profile/parser/source.rs index 7ae849ecc7..fd99f40e7d 100644 --- a/aws/rust-runtime/aws-config/src/profile/parser/source.rs +++ b/aws/rust-runtime/aws-config/src/profile/parser/source.rs @@ -8,7 +8,11 @@ use aws_types::os_shim_internal; use std::borrow::Cow; use std::io::ErrorKind; use std::path::{Component, Path, PathBuf}; -use tracing::Instrument; +use tracing::{warn, Instrument}; + +const HOME_EXPANSION_FAILURE_WARNING: &str = + "home directory expansion was requested (via `~` character) for the profile \ + config file path, but no home directory could be determined"; /// In-memory source of profile data pub(super) struct Source { @@ -88,12 +92,12 @@ async fn load_config_file( fs: &os_shim_internal::Fs, environment: &os_shim_internal::Env, ) -> File { - let path = environment + let (path_is_default, path) = environment .get(kind.override_environment_variable()) - .map(Cow::Owned) + .map(|p| (false, Cow::Owned(p))) .ok() - .unwrap_or_else(|| kind.default_path().into()); - let expanded = expand_home(path.as_ref(), home_directory, environment); + .unwrap_or_else(|| (true, kind.default_path().into())); + let expanded = expand_home(path.as_ref(), path_is_default, home_directory); if path != expanded.to_string_lossy() { tracing::debug!(before = ?path, after = ?expanded, "home directory expanded"); } @@ -134,8 +138,8 @@ async fn load_config_file( fn expand_home( path: impl AsRef, + path_is_default: bool, home_dir: &Option, - environment: &os_shim_internal::Env, ) -> PathBuf { let path = path.as_ref(); let mut components = path.components(); @@ -150,15 +154,9 @@ fn expand_home( dir.clone() } None => { - // Lambdas don't have home directories and emitting this warning is not helpful - // to users running the SDK from within a Lambda. This warning will be silenced - // if we determine that that is the case. - let is_likely_running_on_a_lambda = - check_is_likely_running_on_a_lambda(environment); - if !is_likely_running_on_a_lambda { - tracing::warn!( - "could not determine home directory but home expansion was requested" - ); + // Only log a warning if the path was explicitly set by the customer. + if !path_is_default { + warn!(HOME_EXPANSION_FAILURE_WARNING); } // if we can't determine the home directory, just leave it as `~` "~".into() @@ -179,30 +177,25 @@ fn expand_home( } } -/// Returns true or false based on whether or not this code is likely running inside an AWS Lambda. -/// [Lambdas set many environment variables](https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars.html#configuration-envvars-runtime) -/// that we can check. -fn check_is_likely_running_on_a_lambda(environment: &aws_types::os_shim_internal::Env) -> bool { - // AWS_LAMBDA_FUNCTION_NAME – The name of the running Lambda function. Available both in Functions and Extensions - environment.get("AWS_LAMBDA_FUNCTION_NAME").is_ok() -} - #[cfg(test)] mod tests { - use crate::profile::parser::source::{expand_home, load, load_config_file, FileKind}; + use crate::profile::parser::source::{ + expand_home, load, load_config_file, FileKind, HOME_EXPANSION_FAILURE_WARNING, + }; use aws_types::os_shim_internal::{Env, Fs}; + use futures_util::FutureExt; use serde::Deserialize; use std::collections::HashMap; use std::error::Error; use std::fs; + use tracing_test::traced_test; #[test] fn only_expand_home_prefix() { // ~ is only expanded as a single component (currently) let path = "~aws/config"; - let environment = Env::from_slice(&[]); assert_eq!( - expand_home(&path, &None, &environment).to_str().unwrap(), + expand_home(&path, false, &None).to_str().unwrap(), "~aws/config" ); } @@ -238,9 +231,6 @@ mod tests { Ok(()) } - use futures_util::FutureExt; - use tracing_test::traced_test; - #[traced_test] #[test] fn logs_produced_default() { @@ -260,14 +250,22 @@ mod tests { #[traced_test] #[test] - fn load_config_file_should_not_emit_warning_on_lambda() { - let env = Env::from_slice(&[("AWS_LAMBDA_FUNCTION_NAME", "someName")]); + fn load_config_file_should_not_emit_warning_when_path_not_explicitly_set() { + let env = Env::from_slice(&[]); + let fs = Fs::from_slice(&[]); + + let _src = load_config_file(FileKind::Config, &None, &fs, &env).now_or_never(); + assert!(!logs_contain(HOME_EXPANSION_FAILURE_WARNING)); + } + + #[traced_test] + #[test] + fn load_config_file_should_emit_warning_when_path_explicitly_set() { + let env = Env::from_slice(&[("AWS_CONFIG_FILE", "~/some/path")]); let fs = Fs::from_slice(&[]); let _src = load_config_file(FileKind::Config, &None, &fs, &env).now_or_never(); - assert!(!logs_contain( - "could not determine home directory but home expansion was requested" - )); + assert!(logs_contain(HOME_EXPANSION_FAILURE_WARNING)); } async fn check(test_case: TestCase) { @@ -302,9 +300,8 @@ mod tests { #[cfg_attr(windows, ignore)] fn test_expand_home() { let path = "~/.aws/config"; - let environment = Env::from_slice(&[]); assert_eq!( - expand_home(&path, &Some("/user/foo".to_string()), &environment) + expand_home(&path, false, &Some("/user/foo".to_string())) .to_str() .unwrap(), "/user/foo/.aws/config" @@ -313,21 +310,16 @@ mod tests { #[test] fn expand_home_no_home() { - let environment = Env::from_slice(&[]); // there is an edge case around expansion when no home directory exists // if no home directory can be determined, leave the path as is if !cfg!(windows) { assert_eq!( - expand_home("~/config", &None, &environment) - .to_str() - .unwrap(), + expand_home("~/config", false, &None).to_str().unwrap(), "~/config" ) } else { assert_eq!( - expand_home("~/config", &None, &environment) - .to_str() - .unwrap(), + expand_home("~/config", false, &None).to_str().unwrap(), "~\\config" ) } @@ -338,9 +330,8 @@ mod tests { #[cfg_attr(not(windows), ignore)] fn test_expand_home_windows() { let path = "~/.aws/config"; - let environment = Env::from_slice(&[]); assert_eq!( - expand_home(&path, &Some("C:\\Users\\name".to_string()), &environment) + expand_home(&path, true, &Some("C:\\Users\\name".to_string()),) .to_str() .unwrap(), "C:\\Users\\name\\.aws\\config" From 9224a16cb6df0a677f4fb0b4a8d7f6129d67f2d6 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 20 Jul 2022 08:54:21 -0700 Subject: [PATCH 3/4] Make `imds::Client` implement `Clone` (#1557) --- CHANGELOG.next.toml | 8 ++- .../aws-config/src/imds/client.rs | 62 +++++++++++-------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index a794e5d30a..06a78e1348 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -131,4 +131,10 @@ set (don't emit it for the default paths) """ references = ["smithy-rs#1558", "aws-sdk-rust#583"] meta = { "breaking" = false, "tada" = false, "bug" = true } -author = "jdisanti" \ No newline at end of file +author = "jdisanti" + +[[aws-sdk-rust]] +message = "The `imds::Client` in `aws-config` now implements `Clone`" +references = ["smithy-rs#1557", "aws-sdk-rust#580"] +meta = { "breaking" = false, "tada" = true, "bug" = false } +author = "jdisanti" diff --git a/aws/rust-runtime/aws-config/src/imds/client.rs b/aws/rust-runtime/aws-config/src/imds/client.rs index cf83616349..67e81371b9 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -12,6 +12,7 @@ use std::convert::TryFrom; use std::error::Error; use std::fmt::{Display, Formatter}; use std::str::FromStr; +use std::sync::Arc; use std::time::Duration; use aws_http::user_agent::{ApiMetadata, AwsUserAgent, UserAgentStage}; @@ -124,10 +125,15 @@ fn user_agent() -> AwsUserAgent { /// /// 7. The default value of `http://169.254.169.254` will be used. /// -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct Client { + inner: Arc, +} + +#[derive(Debug)] +struct ClientInner { endpoint: Endpoint, - inner: aws_smithy_client::Client, + smithy_client: aws_smithy_client::Client, } /// Client where build is sync, but usage is async @@ -194,25 +200,29 @@ impl Client { /// ``` pub async fn get(&self, path: &str) -> Result { let operation = self.make_operation(path)?; - self.inner.call(operation).await.map_err(|err| match err { - SdkError::ConstructionFailure(err) => match err.downcast::() { - Ok(token_failure) => *token_failure, - Err(other) => ImdsError::Unexpected(other), - }, - SdkError::TimeoutError(err) => ImdsError::IoError(err), - SdkError::DispatchFailure(err) => ImdsError::IoError(err.into()), - SdkError::ResponseError { err, .. } => ImdsError::IoError(err), - SdkError::ServiceError { - err: InnerImdsError::BadStatus, - raw, - } => ImdsError::ErrorResponse { - response: raw.into_parts().0, - }, - SdkError::ServiceError { - err: InnerImdsError::InvalidUtf8, - .. - } => ImdsError::Unexpected("IMDS returned invalid UTF-8".into()), - }) + self.inner + .smithy_client + .call(operation) + .await + .map_err(|err| match err { + SdkError::ConstructionFailure(err) => match err.downcast::() { + Ok(token_failure) => *token_failure, + Err(other) => ImdsError::Unexpected(other), + }, + SdkError::TimeoutError(err) => ImdsError::IoError(err), + SdkError::DispatchFailure(err) => ImdsError::IoError(err.into()), + SdkError::ResponseError { err, .. } => ImdsError::IoError(err), + SdkError::ServiceError { + err: InnerImdsError::BadStatus, + raw, + } => ImdsError::ErrorResponse { + response: raw.into_parts().0, + }, + SdkError::ServiceError { + err: InnerImdsError::InvalidUtf8, + .. + } => ImdsError::Unexpected("IMDS returned invalid UTF-8".into()), + }) } /// Creates a aws_smithy_http Operation to for `path` @@ -224,7 +234,7 @@ impl Client { path: &str, ) -> Result, ImdsError> { let mut base_uri: Uri = path.parse().map_err(|_| ImdsError::InvalidPath)?; - self.endpoint.set_endpoint(&mut base_uri, None); + self.inner.endpoint.set_endpoint(&mut base_uri, None); let request = http::Request::builder() .uri(base_uri) .body(SdkBody::empty()) @@ -568,7 +578,7 @@ impl Builder { config.sleep(), ); let middleware = ImdsMiddleware { token_loader }; - let inner_client = aws_smithy_client::Builder::new() + let smithy_client = aws_smithy_client::Builder::new() .connector(connector.clone()) .middleware(middleware) .sleep_impl(config.sleep()) @@ -577,8 +587,10 @@ impl Builder { .with_timeout_config(timeout_config); let client = Client { - endpoint, - inner: inner_client, + inner: Arc::new(ClientInner { + endpoint, + smithy_client, + }), }; Ok(client) } From 05a9bafa00d0ab9271f493ebd93cdc5ee4b234c3 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 20 Jul 2022 08:55:16 -0700 Subject: [PATCH 4/4] Expose `sleep_impl` configuration in `SdkConfig` (#1556) --- CHANGELOG.next.toml | 6 ++++++ aws/rust-runtime/aws-types/src/sdk_config.rs | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 06a78e1348..3ad8eeda96 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -138,3 +138,9 @@ message = "The `imds::Client` in `aws-config` now implements `Clone`" references = ["smithy-rs#1557", "aws-sdk-rust#580"] meta = { "breaking" = false, "tada" = true, "bug" = false } author = "jdisanti" + +[[aws-sdk-rust]] +message = "The `sleep_impl` methods on the `SdkConfig` builder are now exposed and documented." +references = ["smithy-rs#1556"] +meta = { "breaking" = false, "tada" = false, "bug" = false } +author = "jdisanti" diff --git a/aws/rust-runtime/aws-types/src/sdk_config.rs b/aws/rust-runtime/aws-types/src/sdk_config.rs index 3c403d7250..6c0715c535 100644 --- a/aws/rust-runtime/aws-types/src/sdk_config.rs +++ b/aws/rust-runtime/aws-types/src/sdk_config.rs @@ -208,7 +208,6 @@ impl Builder { self } - #[doc(hidden)] /// Set the sleep implementation for the builder. The sleep implementation is used to create /// timeout futures. /// @@ -236,7 +235,6 @@ impl Builder { self } - #[doc(hidden)] /// Set the sleep implementation for the builder. The sleep implementation is used to create /// timeout futures. ///