From 50e4e1cf91b74d67d456ac140b328fcb070ee2fc Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Mon, 14 Nov 2022 16:09:23 -0800 Subject: [PATCH 1/4] Improve `Endpoint` panic-safety and ergonomics - `Endpoint::set_endpoint` no longer panics when called on an endpoint without a scheme - `Endpoint::mutable` and `Endpoint::immutable` now both return a result so that constructing an endpoint without a scheme is an error - `Endpoint::mutable` and `Endpoint::immutable` both now take a string instead of a `Uri` as a convenience - `Endpoint::mutable_uri` and `Endpoint::immutable_uri` were added to construct an endpoint directly from a `Uri` --- aws/rust-runtime/aws-config/src/ecs.rs | 7 +- .../aws-config/src/imds/client.rs | 12 +- .../aws-config/src/imds/client/error.rs | 14 +- .../aws-config/src/imds/client/token.rs | 4 +- aws/rust-runtime/aws-config/src/lib.rs | 5 +- .../aws-endpoint/src/partition/endpoint.rs | 2 +- .../aws-endpoint/src/partition/mod.rs | 2 +- aws/rust-runtime/aws-types/src/endpoint.rs | 17 ++- aws/rust-runtime/aws-types/src/sdk_config.rs | 6 +- .../smithy/rustsdk/AwsEndpointDecorator.kt | 8 +- .../dynamodb/tests/endpoints.rs | 8 +- .../s3/tests/streaming-response.rs | 6 +- .../integration-tests/s3/tests/timeouts.rs | 17 ++- rust-runtime/aws-smithy-http/src/endpoint.rs | 122 +++++++++++++----- .../aws-smithy-http/src/endpoint/error.rs | 54 +++++++- 15 files changed, 204 insertions(+), 80 deletions(-) diff --git a/aws/rust-runtime/aws-config/src/ecs.rs b/aws/rust-runtime/aws-config/src/ecs.rs index b31a0664c3..20764fa35f 100644 --- a/aws/rust-runtime/aws-config/src/ecs.rs +++ b/aws/rust-runtime/aws-config/src/ecs.rs @@ -190,8 +190,11 @@ impl Provider { }); } }; - let endpoint = Endpoint::immutable(Uri::from_static(BASE_HOST)); - endpoint.set_endpoint(&mut relative_uri, None); + let endpoint = + Endpoint::immutable_uri(Uri::from_static(BASE_HOST)).expect("BASE_HOST is valid"); + endpoint + .set_endpoint(&mut relative_uri, None) + .expect("appending relative URLs to the ECS endpoint should always succeed"); Ok(relative_uri) } } diff --git a/aws/rust-runtime/aws-config/src/imds/client.rs b/aws/rust-runtime/aws-config/src/imds/client.rs index f25998c531..7e44c7f747 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -237,7 +237,10 @@ impl Client { let mut base_uri: Uri = path.parse().map_err(|_| { ImdsError::unexpected("IMDS path was not a valid URI. Hint: does it begin with `/`?") })?; - self.inner.endpoint.set_endpoint(&mut base_uri, None); + self.inner + .endpoint + .set_endpoint(&mut base_uri, None) + .map_err(ImdsError::unexpected)?; let request = http::Request::builder() .uri(base_uri) .body(SdkBody::empty()) @@ -433,7 +436,7 @@ impl Builder { .endpoint .unwrap_or_else(|| EndpointSource::Env(config.env(), config.fs())); let endpoint = endpoint_source.endpoint(self.mode_override).await?; - let endpoint = Endpoint::immutable(endpoint); + let endpoint = Endpoint::immutable_uri(endpoint)?; let retry_config = retry::Config::default() .with_max_attempts(self.max_attempts.unwrap_or(DEFAULT_ATTEMPTS)); let token_loader = token::TokenMiddleware::new( @@ -503,9 +506,8 @@ impl EndpointSource { profile.get(profile_keys::ENDPOINT).map(Cow::Borrowed) }; if let Some(uri) = uri_override { - return Ok( - Uri::try_from(uri.as_ref()).map_err(BuildErrorKind::InvalidEndpointUri)? - ); + return Ok(Uri::try_from(uri.as_ref()) + .map_err(|err| BuildErrorKind::InvalidEndpointUri(err.into()))?); } // if not, load a endpoint mode from the environment diff --git a/aws/rust-runtime/aws-config/src/imds/client/error.rs b/aws/rust-runtime/aws-config/src/imds/client/error.rs index f1845feb71..3cbc7547ba 100644 --- a/aws/rust-runtime/aws-config/src/imds/client/error.rs +++ b/aws/rust-runtime/aws-config/src/imds/client/error.rs @@ -8,7 +8,7 @@ use crate::profile::credentials::ProfileFileError; use aws_smithy_client::SdkError; use aws_smithy_http::body::SdkBody; -use http::uri::InvalidUri; +use aws_smithy_http::endpoint::error::InvalidEndpointError; use std::error::Error; use std::fmt; @@ -182,7 +182,7 @@ pub(super) enum BuildErrorKind { InvalidProfile(ProfileFileError), /// The specified endpoint was not a valid URI - InvalidEndpointUri(InvalidUri), + InvalidEndpointUri(Box), } /// Error constructing IMDSv2 Client @@ -209,7 +209,7 @@ impl Error for BuildError { match &self.kind { InvalidEndpointMode(e) => Some(e), InvalidProfile(e) => Some(e), - InvalidEndpointUri(e) => Some(e), + InvalidEndpointUri(e) => Some(e.as_ref()), } } } @@ -220,6 +220,14 @@ impl From for BuildError { } } +impl From for BuildError { + fn from(err: InvalidEndpointError) -> Self { + Self { + kind: BuildErrorKind::InvalidEndpointUri(err.into()), + } + } +} + #[derive(Debug)] pub(super) enum TokenErrorKind { /// The token was invalid diff --git a/aws/rust-runtime/aws-config/src/imds/client/token.rs b/aws/rust-runtime/aws-config/src/imds/client/token.rs index e3f572e1e9..615a52289b 100644 --- a/aws/rust-runtime/aws-config/src/imds/client/token.rs +++ b/aws/rust-runtime/aws-config/src/imds/client/token.rs @@ -128,7 +128,9 @@ impl TokenMiddleware { async fn get_token(&self) -> Result<(Token, SystemTime), ImdsError> { let mut uri = Uri::from_static("/latest/api/token"); - self.endpoint.set_endpoint(&mut uri, None); + self.endpoint + .set_endpoint(&mut uri, None) + .map_err(ImdsError::unexpected)?; let request = http::Request::builder() .header( X_AWS_EC2_METADATA_TOKEN_TTL_SECONDS, diff --git a/aws/rust-runtime/aws-config/src/lib.rs b/aws/rust-runtime/aws-config/src/lib.rs index e767203c06..ae6bb84376 100644 --- a/aws/rust-runtime/aws-config/src/lib.rs +++ b/aws/rust-runtime/aws-config/src/lib.rs @@ -323,13 +323,14 @@ mod loader { /// /// Use a static endpoint for all services /// ```no_run - /// # async fn create_config() { + /// # async fn create_config() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> { /// use aws_config::endpoint::Endpoint; /// /// let sdk_config = aws_config::from_env() - /// .endpoint_resolver(Endpoint::immutable("http://localhost:1234".parse().expect("valid URI"))) + /// .endpoint_resolver(Endpoint::immutable("http://localhost:1234")?) /// .load() /// .await; + /// # Ok(()) /// # } pub fn endpoint_resolver( mut self, diff --git a/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs b/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs index 70233e0363..75fdfa777c 100644 --- a/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs +++ b/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs @@ -54,7 +54,7 @@ impl ResolveAwsEndpoint for Metadata { fn resolve_endpoint(&self, region: &Region) -> Result { let uri = self.uri_template.replace("{region}", region.as_ref()); let uri = format!("{}://{}", self.protocol.as_str(), uri); - let endpoint = Endpoint::mutable(uri.parse()?); + let endpoint = Endpoint::mutable(uri)?; let mut credential_scope = CredentialScope::builder().region( self.credential_scope .region() diff --git a/aws/rust-runtime/aws-endpoint/src/partition/mod.rs b/aws/rust-runtime/aws-endpoint/src/partition/mod.rs index 0f595e0d3e..f07c669673 100644 --- a/aws/rust-runtime/aws-endpoint/src/partition/mod.rs +++ b/aws/rust-runtime/aws-endpoint/src/partition/mod.rs @@ -373,7 +373,7 @@ mod test { .resolve_endpoint(&Region::new(test_case.region)) .expect("valid region"); let mut test_uri = Uri::from_static("/"); - endpoint.set_endpoint(&mut test_uri, None); + endpoint.set_endpoint(&mut test_uri, None).unwrap(); assert_eq!(test_uri, Uri::from_static(test_case.uri)); assert_eq!( endpoint.credential_scope().region(), diff --git a/aws/rust-runtime/aws-types/src/endpoint.rs b/aws/rust-runtime/aws-types/src/endpoint.rs index 2b0998e0e7..abac50ab77 100644 --- a/aws/rust-runtime/aws-types/src/endpoint.rs +++ b/aws/rust-runtime/aws-types/src/endpoint.rs @@ -7,6 +7,7 @@ use crate::region::{Region, SigningRegion}; use crate::SigningService; +use aws_smithy_http::endpoint::error::InvalidEndpointError; use aws_smithy_http::endpoint::{Endpoint, EndpointPrefix}; use std::error::Error; use std::fmt::Debug; @@ -43,8 +44,12 @@ impl AwsEndpoint { } /// Sets the endpoint on a given `uri` based on this endpoint - pub fn set_endpoint(&self, uri: &mut http::Uri, endpoint_prefix: Option<&EndpointPrefix>) { - self.endpoint.set_endpoint(uri, endpoint_prefix); + pub fn set_endpoint( + &self, + uri: &mut http::Uri, + endpoint_prefix: Option<&EndpointPrefix>, + ) -> Result<(), InvalidEndpointError> { + self.endpoint.set_endpoint(uri, endpoint_prefix) } } @@ -71,12 +76,12 @@ pub type BoxError = Box; /// # } /// # } /// # } +/// # fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> { /// use aws_smithy_http::endpoint::Endpoint; -/// use http::Uri; /// let config = dynamodb::Config::builder() -/// .endpoint( -/// Endpoint::immutable(Uri::from_static("http://localhost:8080")) -/// ); +/// .endpoint(Endpoint::immutable("http://localhost:8080")?); +/// # Ok(()) +/// # } /// ``` /// Each AWS service generates their own implementation of `ResolveAwsEndpoint`. pub trait ResolveAwsEndpoint: Send + Sync + Debug { diff --git a/aws/rust-runtime/aws-types/src/sdk_config.rs b/aws/rust-runtime/aws-types/src/sdk_config.rs index 6fb916421a..c714276c52 100644 --- a/aws/rust-runtime/aws-types/src/sdk_config.rs +++ b/aws/rust-runtime/aws-types/src/sdk_config.rs @@ -90,13 +90,15 @@ impl Builder { /// /// # Examples /// ``` + /// # fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> { /// use std::sync::Arc; /// use aws_types::SdkConfig; /// use aws_smithy_http::endpoint::Endpoint; - /// use http::Uri; /// let config = SdkConfig::builder().endpoint_resolver( - /// Endpoint::immutable(Uri::from_static("http://localhost:8080")) + /// Endpoint::immutable("http://localhost:8080")? /// ).build(); + /// # Ok(()) + /// # } /// ``` pub fn endpoint_resolver( mut self, diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt index 822dca09a1..d82cadd104 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt @@ -138,14 +138,16 @@ class EndpointConfigCustomization( /// /// ## Examples /// ```no_run + /// ## fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> { /// use #{aws_types}::region::Region; /// use $moduleUseName::config::{Builder, Config}; /// use $moduleUseName::Endpoint; /// /// let config = $moduleUseName::Config::builder() - /// .endpoint_resolver( - /// Endpoint::immutable("http://localhost:8080".parse().expect("valid URI")) - /// ).build(); + /// .endpoint_resolver(Endpoint::immutable("http://localhost:8080")?) + /// .build(); + /// ## Ok(()) + /// ## } /// ``` pub fn endpoint_resolver(mut self, endpoint_resolver: impl #{ResolveAwsEndpoint} + 'static) -> Self { self.endpoint_resolver = Some(std::sync::Arc::new(#{EndpointShim}::from_resolver(endpoint_resolver)) as _); diff --git a/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs b/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs index 1c934e6e20..82356e3738 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs @@ -14,9 +14,7 @@ async fn endpoints_can_be_overridden_globally() { let shared_config = aws_types::SdkConfig::builder() .region(Region::new("us-east-4")) .http_connector(conn) - .endpoint_resolver(Endpoint::immutable( - "http://localhost:8000".parse().unwrap(), - )) + .endpoint_resolver(Endpoint::immutable("http://localhost:8000").expect("valid endpoint")) .build(); let conf = aws_sdk_dynamodb::config::Builder::from(&shared_config) .credentials_provider(Credentials::new("asdf", "asdf", None, None, "test")) @@ -38,9 +36,7 @@ async fn endpoints_can_be_overridden_locally() { .build(); let conf = aws_sdk_dynamodb::config::Builder::from(&shared_config) .credentials_provider(Credentials::new("asdf", "asdf", None, None, "test")) - .endpoint_resolver(Endpoint::immutable( - "http://localhost:8000".parse().unwrap(), - )) + .endpoint_resolver(Endpoint::immutable("http://localhost:8000").expect("valid endpoint")) .build(); let svc = aws_sdk_dynamodb::Client::from_conf(conf); let _ = svc.list_tables().send().await; diff --git a/aws/sdk/integration-tests/s3/tests/streaming-response.rs b/aws/sdk/integration-tests/s3/tests/streaming-response.rs index fe6ea94603..3d8bbfc354 100644 --- a/aws/sdk/integration-tests/s3/tests/streaming-response.rs +++ b/aws/sdk/integration-tests/s3/tests/streaming-response.rs @@ -30,9 +30,9 @@ async fn test_streaming_response_fails_when_eof_comes_before_content_length_reac "test", ))) .region(Region::new("us-east-1")) - .endpoint_resolver(Endpoint::immutable( - format!("http://{server_addr}").parse().expect("valid URI"), - )) + .endpoint_resolver( + Endpoint::immutable(format!("http://{server_addr}")).expect("valid endpoint"), + ) .build(); let client = Client::new(&sdk_config); diff --git a/aws/sdk/integration-tests/s3/tests/timeouts.rs b/aws/sdk/integration-tests/s3/tests/timeouts.rs index f8c02e8858..a928ae895a 100644 --- a/aws/sdk/integration-tests/s3/tests/timeouts.rs +++ b/aws/sdk/integration-tests/s3/tests/timeouts.rs @@ -104,9 +104,9 @@ async fn test_read_timeout() { .read_timeout(Duration::from_millis(300)) .build(), ) - .endpoint_resolver(Endpoint::immutable( - format!("http://{server_addr}").parse().expect("valid URI"), - )) + .endpoint_resolver( + Endpoint::immutable(format!("http://{server_addr}")).expect("valid endpoint"), + ) .region(Some(Region::from_static("us-east-1"))) .credentials_provider(SharedCredentialsProvider::new(Credentials::new( "test", "test", None, None, "test", @@ -148,10 +148,13 @@ async fn test_connect_timeout() { .connect_timeout(Duration::from_millis(300)) .build(), ) - .endpoint_resolver(Endpoint::immutable( - // Emulate a connect timeout error by hitting an unroutable IP - "http://172.255.255.0:18104".parse().unwrap(), - )) + .endpoint_resolver( + Endpoint::immutable( + // Emulate a connect timeout error by hitting an unroutable IP + "http://172.255.255.0:18104", + ) + .expect("valid endpoint"), + ) .region(Some(Region::from_static("us-east-1"))) .credentials_provider(SharedCredentialsProvider::new(Credentials::new( "test", "test", None, None, "test", diff --git a/rust-runtime/aws-smithy-http/src/endpoint.rs b/rust-runtime/aws-smithy-http/src/endpoint.rs index 6aed737e1d..71a80d39ab 100644 --- a/rust-runtime/aws-smithy-http/src/endpoint.rs +++ b/rust-runtime/aws-smithy-http/src/endpoint.rs @@ -3,10 +3,11 @@ * SPDX-License-Identifier: Apache-2.0 */ -use crate::endpoint::error::{InvalidEndpointError, InvalidEndpointErrorKind}; +use crate::endpoint::error::InvalidEndpointError; use crate::operation::error::BuildError; use http::uri::{Authority, Uri}; use std::borrow::Cow; +use std::result::Result as StdResult; use std::str::FromStr; pub mod error; @@ -34,7 +35,7 @@ pub struct Endpoint { #[derive(Clone, Debug, Eq, PartialEq)] pub struct EndpointPrefix(String); impl EndpointPrefix { - pub fn new(prefix: impl Into) -> std::result::Result { + pub fn new(prefix: impl Into) -> StdResult { let prefix = prefix.into(); match Authority::from_str(&prefix) { Ok(_) => Ok(EndpointPrefix(prefix)), @@ -61,7 +62,7 @@ pub fn apply_endpoint( uri: &mut Uri, endpoint: &Uri, prefix: Option<&EndpointPrefix>, -) -> std::result::Result<(), InvalidEndpointError> { +) -> StdResult<(), InvalidEndpointError> { let prefix = prefix.map(|p| p.0.as_str()).unwrap_or(""); let authority = endpoint .authority() @@ -73,17 +74,17 @@ pub fn apply_endpoint( } else { Authority::from_str(authority) } - .map_err(|_| InvalidEndpointErrorKind::FailedToConstructAuthority)?; + .map_err(InvalidEndpointError::failed_to_construct_authority)?; let scheme = *endpoint .scheme() .as_ref() - .ok_or(InvalidEndpointErrorKind::EndpointMustHaveScheme)?; + .ok_or_else(InvalidEndpointError::endpoint_must_have_scheme)?; let new_uri = Uri::builder() .authority(authority) .scheme(scheme.clone()) .path_and_query(Endpoint::merge_paths(endpoint, uri).as_ref()) .build() - .map_err(|_| InvalidEndpointErrorKind::FailedToConstructUri)?; + .map_err(InvalidEndpointError::failed_to_construct_uri)?; *uri = new_uri; Ok(()) } @@ -94,11 +95,22 @@ impl Endpoint { /// Certain services will augment the endpoint with additional metadata. For example, /// S3 can prefix the host with the bucket name. If your endpoint does not support this, /// (for example, when communicating with localhost), use [`Endpoint::immutable`]. - pub fn mutable(uri: Uri) -> Self { - Endpoint { - uri, + pub fn mutable_uri(uri: Uri) -> StdResult { + Ok(Endpoint { + uri: Self::validate_endpoint(uri)?, immutable: false, - } + }) + } + + /// Create a new endpoint from a URI string + /// + /// Certain services will augment the endpoint with additional metadata. For example, + /// S3 can prefix the host with the bucket name. If your endpoint does not support this, + /// (for example, when communicating with localhost), use [`Endpoint::immutable`]. + pub fn mutable(uri: impl AsRef) -> StdResult { + Self::mutable_uri( + Uri::try_from(uri.as_ref()).map_err(InvalidEndpointError::failed_to_construct_uri)?, + ) } /// Returns the URI of this endpoint @@ -111,27 +123,57 @@ impl Endpoint { /// ```rust /// # use aws_smithy_http::endpoint::Endpoint; /// use http::Uri; - /// let endpoint = Endpoint::immutable(Uri::from_static("http://localhost:8000")); + /// let uri = Uri::from_static("http://localhost:8000"); + /// let endpoint = Endpoint::immutable_uri(uri); /// ``` /// /// Certain services will augment the endpoint with additional metadata. For example, /// S3 can prefix the host with the bucket name. This constructor creates an endpoint which will /// ignore those mutations. If you want an endpoint which will obey mutation requests, use /// [`Endpoint::mutable`] instead. - pub fn immutable(uri: Uri) -> Self { - Endpoint { - uri, + pub fn immutable_uri(uri: Uri) -> StdResult { + Ok(Endpoint { + uri: Self::validate_endpoint(uri)?, immutable: true, - } + }) + } + + /// Create a new immutable endpoint from a URI string + /// + /// ```rust + /// # use aws_smithy_http::endpoint::Endpoint; + /// let endpoint = Endpoint::immutable("http://localhost:8000"); + /// ``` + /// + /// Certain services will augment the endpoint with additional metadata. For example, + /// S3 can prefix the host with the bucket name. This constructor creates an endpoint which will + /// ignore those mutations. If you want an endpoint which will obey mutation requests, use + /// [`Endpoint::mutable`] instead. + pub fn immutable(uri: impl AsRef) -> StdResult { + Self::immutable_uri( + Uri::try_from(uri.as_ref()).map_err(InvalidEndpointError::failed_to_construct_uri)?, + ) } /// Sets the endpoint on `uri`, potentially applying the specified `prefix` in the process. - pub fn set_endpoint(&self, uri: &mut http::Uri, prefix: Option<&EndpointPrefix>) { + pub fn set_endpoint( + &self, + uri: &mut http::Uri, + prefix: Option<&EndpointPrefix>, + ) -> StdResult<(), InvalidEndpointError> { let prefix = match self.immutable { true => None, false => prefix, }; - apply_endpoint(uri, &self.uri, prefix).expect("failed to set endpoint"); + apply_endpoint(uri, &self.uri, prefix) + } + + fn validate_endpoint(endpoint: Uri) -> StdResult { + if endpoint.scheme().is_none() { + Err(InvalidEndpointError::endpoint_must_have_scheme()) + } else { + Ok(endpoint) + } } fn merge_paths<'a>(endpoint: &'a Uri, uri: &'a Uri) -> Cow<'a, str> { @@ -154,18 +196,19 @@ impl Endpoint { #[cfg(test)] mod test { - use http::Uri; - + use crate::endpoint::error::{InvalidEndpointError, InvalidEndpointErrorKind}; use crate::endpoint::{Endpoint, EndpointPrefix}; + use http::Uri; #[test] fn prefix_endpoint() { - let ep = Endpoint::mutable(Uri::from_static("https://us-east-1.dynamo.amazonaws.com")); + let ep = Endpoint::mutable("https://us-east-1.dynamo.amazonaws.com").unwrap(); let mut uri = Uri::from_static("/list_tables?k=v"); ep.set_endpoint( &mut uri, Some(&EndpointPrefix::new("subregion.").expect("valid prefix")), - ); + ) + .unwrap(); assert_eq!( uri, Uri::from_static("https://subregion.us-east-1.dynamo.amazonaws.com/list_tables?k=v") @@ -174,14 +217,13 @@ mod test { #[test] fn prefix_endpoint_custom_port() { - let ep = Endpoint::mutable(Uri::from_static( - "https://us-east-1.dynamo.amazonaws.com:6443", - )); + let ep = Endpoint::mutable("https://us-east-1.dynamo.amazonaws.com:6443").unwrap(); let mut uri = Uri::from_static("/list_tables?k=v"); ep.set_endpoint( &mut uri, Some(&EndpointPrefix::new("subregion.").expect("valid prefix")), - ); + ) + .unwrap(); assert_eq!( uri, Uri::from_static( @@ -192,12 +234,13 @@ mod test { #[test] fn prefix_immutable_endpoint() { - let ep = Endpoint::immutable(Uri::from_static("https://us-east-1.dynamo.amazonaws.com")); + let ep = Endpoint::immutable("https://us-east-1.dynamo.amazonaws.com").unwrap(); let mut uri = Uri::from_static("/list_tables?k=v"); ep.set_endpoint( &mut uri, Some(&EndpointPrefix::new("subregion.").expect("valid prefix")), - ); + ) + .unwrap(); assert_eq!( uri, Uri::from_static("https://us-east-1.dynamo.amazonaws.com/list_tables?k=v") @@ -211,12 +254,13 @@ mod test { "https://us-east-1.dynamo.amazonaws.com/private", "https://us-east-1.dynamo.amazonaws.com/private/", ] { - let ep = Endpoint::immutable(Uri::from_static(uri)); + let ep = Endpoint::immutable(uri).unwrap(); let mut uri = Uri::from_static("/list_tables?k=v"); ep.set_endpoint( &mut uri, Some(&EndpointPrefix::new("subregion.").expect("valid prefix")), - ); + ) + .unwrap(); assert_eq!( uri, Uri::from_static("https://us-east-1.dynamo.amazonaws.com/private/list_tables?k=v") @@ -226,9 +270,25 @@ mod test { #[test] fn set_endpoint_empty_path() { - let ep = Endpoint::immutable(Uri::from_static("http://localhost:8000")); + let ep = Endpoint::immutable("http://localhost:8000").unwrap(); let mut uri = Uri::from_static("/"); - ep.set_endpoint(&mut uri, None); + ep.set_endpoint(&mut uri, None).unwrap(); assert_eq!(uri, Uri::from_static("http://localhost:8000/")) } + + #[test] + fn endpoint_construction_missing_scheme() { + assert!(matches!( + Endpoint::mutable("localhost:8000"), + Err(InvalidEndpointError { + kind: InvalidEndpointErrorKind::EndpointMustHaveScheme + }) + )); + assert!(matches!( + Endpoint::immutable("localhost:8000"), + Err(InvalidEndpointError { + kind: InvalidEndpointErrorKind::EndpointMustHaveScheme + }) + )); + } } diff --git a/rust-runtime/aws-smithy-http/src/endpoint/error.rs b/rust-runtime/aws-smithy-http/src/endpoint/error.rs index fd850bfe30..72104ead4f 100644 --- a/rust-runtime/aws-smithy-http/src/endpoint/error.rs +++ b/rust-runtime/aws-smithy-http/src/endpoint/error.rs @@ -43,17 +43,48 @@ impl StdError for ResolveEndpointError { } } -#[non_exhaustive] #[derive(Debug)] pub(super) enum InvalidEndpointErrorKind { EndpointMustHaveScheme, - FailedToConstructAuthority, - FailedToConstructUri, + FailedToConstructAuthority { + source: Box, + }, + FailedToConstructUri { + source: Box, + }, } #[derive(Debug)] pub struct InvalidEndpointError { - kind: InvalidEndpointErrorKind, + pub(super) kind: InvalidEndpointErrorKind, +} + +impl InvalidEndpointError { + pub(super) fn endpoint_must_have_scheme() -> Self { + Self { + kind: InvalidEndpointErrorKind::EndpointMustHaveScheme, + } + } + + pub(super) fn failed_to_construct_authority( + source: impl Into>, + ) -> Self { + Self { + kind: InvalidEndpointErrorKind::FailedToConstructAuthority { + source: source.into(), + }, + } + } + + pub(super) fn failed_to_construct_uri( + source: impl Into>, + ) -> Self { + Self { + kind: InvalidEndpointErrorKind::FailedToConstructUri { + source: source.into(), + }, + } + } } impl From for InvalidEndpointError { @@ -67,13 +98,22 @@ impl fmt::Display for InvalidEndpointError { use InvalidEndpointErrorKind as ErrorKind; match self.kind { ErrorKind::EndpointMustHaveScheme => write!(f, "endpoint must contain a valid scheme"), - ErrorKind::FailedToConstructAuthority => write!( + ErrorKind::FailedToConstructAuthority { .. } => write!( f, "endpoint must contain a valid authority when combined with endpoint prefix" ), - ErrorKind::FailedToConstructUri => write!(f, "failed to construct URI"), + ErrorKind::FailedToConstructUri { .. } => write!(f, "failed to construct URI"), } } } -impl StdError for InvalidEndpointError {} +impl StdError for InvalidEndpointError { + fn source(&self) -> Option<&(dyn StdError + 'static)> { + use InvalidEndpointErrorKind as ErrorKind; + match &self.kind { + ErrorKind::FailedToConstructUri { source } + | ErrorKind::FailedToConstructAuthority { source } => Some(source.as_ref()), + ErrorKind::EndpointMustHaveScheme => None, + } + } +} From 7e0458187722711bb277b87e70083a7fc477fcab Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Tue, 15 Nov 2022 17:11:41 -0800 Subject: [PATCH 2/4] Update the changelog --- CHANGELOG.next.toml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index f97ce21668..b53b1bb2db 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -339,3 +339,21 @@ Server SDKs now correctly reject operation inputs that don't set values for `req references = ["smithy-rs#1714", "smithy-rs#1342", "smithy-rs#1860"] meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server" } author = "david-perez" + +[[aws-sdk-rust]] +message = "Functions on `aws_smithy_http::endpoint::Endpoint` now return a `Result` instead of panicking." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti" + +[[aws-sdk-rust]] +message = "`Endpoint::mutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::mutable_uri`." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti" + +[[aws-sdk-rust]] +message = "`Endpoint::immutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::immutable_uri`." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti" From 8725d88612e934e5fa9c70fa1ed5bc5c2b728020 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 16 Nov 2022 15:35:35 -0800 Subject: [PATCH 3/4] Incorporate feedback --- CHANGELOG.next.toml | 18 +++++++++++ .../aws-config/src/imds/client.rs | 13 ++++---- .../aws-config/src/imds/client/error.rs | 30 ++++++++++++++----- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index b53b1bb2db..e19d74371b 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -346,14 +346,32 @@ references = ["smithy-rs#1984", "smithy-rs#1496"] meta = { "breaking" = true, "tada" = false, "bug" = false } author = "jdisanti" +[[smithy-rs]] +message = "Functions on `aws_smithy_http::endpoint::Endpoint` now return a `Result` instead of panicking." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } +author = "jdisanti" + [[aws-sdk-rust]] message = "`Endpoint::mutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::mutable_uri`." references = ["smithy-rs#1984", "smithy-rs#1496"] meta = { "breaking" = true, "tada" = false, "bug" = false } author = "jdisanti" +[[smithy-rs]] +message = "`Endpoint::mutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::mutable_uri`." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } +author = "jdisanti" + [[aws-sdk-rust]] message = "`Endpoint::immutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::immutable_uri`." references = ["smithy-rs#1984", "smithy-rs#1496"] meta = { "breaking" = true, "tada" = false, "bug" = false } author = "jdisanti" + +[[smithy-rs]] +message = "`Endpoint::immutable` now takes `impl AsRef` instead of `Uri`. For the old functionality, use `Endpoint::immutable_uri`." +references = ["smithy-rs#1984", "smithy-rs#1496"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } +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 7e44c7f747..5f716c84ee 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -8,9 +8,7 @@ //! Client for direct access to IMDSv2. use crate::connector::expect_connector; -use crate::imds::client::error::{ - BuildError, BuildErrorKind, ImdsError, InnerImdsError, InvalidEndpointMode, -}; +use crate::imds::client::error::{BuildError, ImdsError, InnerImdsError, InvalidEndpointMode}; use crate::imds::client::token::TokenMiddleware; use crate::provider_config::ProviderConfig; use crate::{profile, PKG_VERSION}; @@ -499,15 +497,14 @@ impl EndpointSource { // load an endpoint override from the environment let profile = profile::load(fs, env, &Default::default()) .await - .map_err(BuildErrorKind::InvalidProfile)?; + .map_err(BuildError::invalid_profile)?; let uri_override = if let Ok(uri) = env.get(env::ENDPOINT) { Some(Cow::Owned(uri)) } else { profile.get(profile_keys::ENDPOINT).map(Cow::Borrowed) }; if let Some(uri) = uri_override { - return Ok(Uri::try_from(uri.as_ref()) - .map_err(|err| BuildErrorKind::InvalidEndpointUri(err.into()))?); + return Uri::try_from(uri.as_ref()).map_err(BuildError::invalid_endpoint_uri); } // if not, load a endpoint mode from the environment @@ -515,10 +512,10 @@ impl EndpointSource { mode } else if let Ok(mode) = env.get(env::ENDPOINT_MODE) { mode.parse::() - .map_err(BuildErrorKind::InvalidEndpointMode)? + .map_err(BuildError::invalid_endpoint_mode)? } else if let Some(mode) = profile.get(profile_keys::ENDPOINT_MODE) { mode.parse::() - .map_err(BuildErrorKind::InvalidEndpointMode)? + .map_err(BuildError::invalid_endpoint_mode)? } else { EndpointMode::IpV4 }; diff --git a/aws/rust-runtime/aws-config/src/imds/client/error.rs b/aws/rust-runtime/aws-config/src/imds/client/error.rs index 3cbc7547ba..72ddb2dab9 100644 --- a/aws/rust-runtime/aws-config/src/imds/client/error.rs +++ b/aws/rust-runtime/aws-config/src/imds/client/error.rs @@ -174,7 +174,7 @@ impl Error for InvalidEndpointMode {} #[derive(Debug)] #[allow(clippy::enum_variant_names)] -pub(super) enum BuildErrorKind { +enum BuildErrorKind { /// The endpoint mode was invalid InvalidEndpointMode(InvalidEndpointMode), @@ -191,6 +191,28 @@ pub struct BuildError { kind: BuildErrorKind, } +impl BuildError { + pub(super) fn invalid_endpoint_mode(source: InvalidEndpointMode) -> Self { + Self { + kind: BuildErrorKind::InvalidEndpointMode(source), + } + } + + pub(super) fn invalid_profile(source: ProfileFileError) -> Self { + Self { + kind: BuildErrorKind::InvalidProfile(source), + } + } + + pub(super) fn invalid_endpoint_uri( + source: impl Into>, + ) -> Self { + Self { + kind: BuildErrorKind::InvalidEndpointUri(source.into()), + } + } +} + impl fmt::Display for BuildError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result { use BuildErrorKind::*; @@ -214,12 +236,6 @@ impl Error for BuildError { } } -impl From for BuildError { - fn from(kind: BuildErrorKind) -> Self { - Self { kind } - } -} - impl From for BuildError { fn from(err: InvalidEndpointError) -> Self { Self { From a62726da24c94e25a11f7696b1f45407974887bd Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 16 Nov 2022 15:45:56 -0800 Subject: [PATCH 4/4] Fix CI failures --- aws/rust-runtime/aws-config/external-types.toml | 5 +++-- aws/rust-runtime/aws-types/external-types.toml | 1 + .../smithy/generators/protocol/ProtocolTestGenerator.kt | 5 ++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/aws/rust-runtime/aws-config/external-types.toml b/aws/rust-runtime/aws-config/external-types.toml index ade75eb332..b743d35957 100644 --- a/aws/rust-runtime/aws-config/external-types.toml +++ b/aws/rust-runtime/aws-config/external-types.toml @@ -8,11 +8,12 @@ allowed_external_types = [ "aws_smithy_client::bounds::SmithyConnector", "aws_smithy_client::erase::DynConnector", "aws_smithy_client::erase::boxclone::BoxCloneService", - "aws_smithy_client::http_connector::HttpConnector", "aws_smithy_client::http_connector::ConnectorSettings", + "aws_smithy_client::http_connector::HttpConnector", "aws_smithy_http::body::SdkBody", - "aws_smithy_http::result::SdkError", "aws_smithy_http::endpoint", + "aws_smithy_http::endpoint::error::InvalidEndpointError", + "aws_smithy_http::result::SdkError", "aws_smithy_types::retry", "aws_smithy_types::retry::*", "aws_smithy_types::timeout", diff --git a/aws/rust-runtime/aws-types/external-types.toml b/aws/rust-runtime/aws-types/external-types.toml index 051e0986ec..2c0fc9226c 100644 --- a/aws/rust-runtime/aws-types/external-types.toml +++ b/aws/rust-runtime/aws-types/external-types.toml @@ -4,6 +4,7 @@ allowed_external_types = [ "aws_smithy_client::http_connector::HttpConnector", "aws_smithy_http::endpoint::Endpoint", "aws_smithy_http::endpoint::EndpointPrefix", + "aws_smithy_http::endpoint::error::InvalidEndpointError", "aws_smithy_types::retry::RetryConfig", "aws_smithy_types::timeout::TimeoutConfig", "http::uri::Uri", diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt index f4e3044fa6..50bec8cd21 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt @@ -69,7 +69,6 @@ class ProtocolTestGenerator( private val codegenScope = arrayOf( "SmithyHttp" to CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType(), - "Http" to CargoDependency.Http.asType(), "AssertEq" to CargoDependency.PrettyAssertions.asType().member("assert_eq!"), ) @@ -187,8 +186,8 @@ class ProtocolTestGenerator( rustTemplate( """ let mut http_request = http_request; - let ep = #{SmithyHttp}::endpoint::Endpoint::mutable(#{Http}::Uri::from_static(${withScheme.dq()})); - ep.set_endpoint(http_request.uri_mut(), parts.acquire().get()); + let ep = #{SmithyHttp}::endpoint::Endpoint::mutable(${withScheme.dq()}).expect("valid endpoint"); + ep.set_endpoint(http_request.uri_mut(), parts.acquire().get()).expect("valid endpoint"); """, *codegenScope, )