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

chore: fix many warnings across the codebase #2955

Merged
merged 4 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions crates/aws/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::time::Duration;
/// Custom S3 endpoint.
pub const AWS_ENDPOINT_URL: &str = "AWS_ENDPOINT_URL";
/// Custom DynamoDB endpoint.
///
/// If DynamoDB endpoint is not supplied, will use S3 endpoint (AWS_ENDPOINT_URL)
/// If it is supplied, this endpoint takes precedence over the global endpoint set in AWS_ENDPOINT_URL for DynamoDB
pub const AWS_ENDPOINT_URL_DYNAMODB: &str = "AWS_ENDPOINT_URL_DYNAMODB";
Expand Down Expand Up @@ -41,7 +42,9 @@ pub const AWS_IAM_ROLE_SESSION_NAME: &str = "AWS_IAM_ROLE_SESSION_NAME";
note = "Please use AWS_IAM_ROLE_SESSION_NAME instead"
)]
pub const AWS_S3_ROLE_SESSION_NAME: &str = "AWS_S3_ROLE_SESSION_NAME";
/// The `pool_idle_timeout` option of aws http client. Has to be lower than 20 seconds, which is
/// The `pool_idle_timeout` option of aws http client.
///
/// Has to be lower than 20 seconds, which is
/// default S3 server timeout <https://aws.amazon.com/premiumsupport/knowledge-center/s3-socket-connection-timeout-error/>.
/// However, since rusoto uses hyper as a client, its default timeout is 90 seconds
/// <https://docs.rs/hyper/0.13.2/hyper/client/struct.Builder.html#method.keep_alive_timeout>.
Expand All @@ -55,16 +58,19 @@ pub const AWS_STS_POOL_IDLE_TIMEOUT_SECONDS: &str = "AWS_STS_POOL_IDLE_TIMEOUT_S
pub const AWS_S3_GET_INTERNAL_SERVER_ERROR_RETRIES: &str =
"AWS_S3_GET_INTERNAL_SERVER_ERROR_RETRIES";
/// The web identity token file to use when using a web identity provider.
///
/// NOTE: web identity related options are set in the environment when
/// creating an instance of [crate::storage::s3::S3StorageOptions].
/// See also <https://docs.rs/rusoto_sts/0.47.0/rusoto_sts/struct.WebIdentityProvider.html#method.from_k8s_env>.
pub const AWS_WEB_IDENTITY_TOKEN_FILE: &str = "AWS_WEB_IDENTITY_TOKEN_FILE";
/// The role name to use for web identity.
///
/// NOTE: web identity related options are set in the environment when
/// creating an instance of [crate::storage::s3::S3StorageOptions].
/// See also <https://docs.rs/rusoto_sts/0.47.0/rusoto_sts/struct.WebIdentityProvider.html#method.from_k8s_env>.
pub const AWS_ROLE_ARN: &str = "AWS_ROLE_ARN";
/// The role session name to use for web identity.
///
/// NOTE: web identity related options are set in the environment when
/// creating an instance of [crate::storage::s3::S3StorageOptions].
/// See also <https://docs.rs/rusoto_sts/0.47.0/rusoto_sts/struct.WebIdentityProvider.html#method.from_k8s_env>.
Expand Down Expand Up @@ -99,8 +105,8 @@ pub const S3_OPTS: &[&str] = &[
AWS_SECRET_ACCESS_KEY,
AWS_SESSION_TOKEN,
AWS_S3_LOCKING_PROVIDER,
AWS_S3_ASSUME_ROLE_ARN,
AWS_S3_ROLE_SESSION_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned to @hntd187 in Slack that I would like to avoid removing these deprecation warnings until we have a few more releases which is print deprecation warnings for people

AWS_IAM_ROLE_ARN,
AWS_IAM_ROLE_SESSION_NAME,
AWS_WEB_IDENTITY_TOKEN_FILE,
AWS_ROLE_ARN,
AWS_ROLE_SESSION_NAME,
Expand Down
33 changes: 20 additions & 13 deletions crates/aws/src/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use deltalake_core::storage::StorageOptions;
use deltalake_core::DeltaResult;
use tracing::log::*;

use crate::constants::{self, AWS_ENDPOINT_URL};
use crate::constants;

/// An [object_store::CredentialProvider] which handles converting a populated [SdkConfig]
/// into a necessary [AwsCredential] type for configuring [object_store::aws::AmazonS3]
Expand Down Expand Up @@ -183,19 +183,26 @@ fn assume_role_arn(options: &StorageOptions) -> Option<String> {
options
.0
.get(constants::AWS_IAM_ROLE_ARN)
.or(options.0.get(constants::AWS_S3_ASSUME_ROLE_ARN))
.or(
#[allow(deprecated)]
options.0.get(constants::AWS_S3_ASSUME_ROLE_ARN),
)
.or(std::env::var_os(constants::AWS_IAM_ROLE_ARN)
.map(|o| {
o.into_string()
.expect("Failed to unwrap AWS_IAM_ROLE_ARN which may have invalid data")
})
.as_ref())
.or(std::env::var_os(constants::AWS_S3_ASSUME_ROLE_ARN)
.map(|o| {
o.into_string()
.expect("Failed to unwrap AWS_S3_ASSUME_ROLE_ARN which may have invalid data")
})
.as_ref())
.or(
#[allow(deprecated)]
std::env::var_os(constants::AWS_S3_ASSUME_ROLE_ARN)
.map(|o| {
o.into_string().expect(
"Failed to unwrap AWS_S3_ASSUME_ROLE_ARN which may have invalid data",
)
})
.as_ref(),
)
.cloned()
}

Expand All @@ -204,13 +211,13 @@ fn assume_session_name(options: &StorageOptions) -> String {
let assume_session = options
.0
.get(constants::AWS_IAM_ROLE_SESSION_NAME)
.or(options.0.get(constants::AWS_S3_ROLE_SESSION_NAME))
.or(
#[allow(deprecated)]
options.0.get(constants::AWS_S3_ROLE_SESSION_NAME),
)
.cloned();

match assume_session {
Some(s) => s,
None => assume_role_sessio_name(),
}
assume_session.unwrap_or_else(assume_role_sessio_name)
}

/// Take a set of [StorageOptions] and produce an appropriate AWS SDK [SdkConfig]
Expand Down
44 changes: 27 additions & 17 deletions crates/aws/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl LogStoreFactory for S3LogStoreFactory {
// With conditional put in S3-like API we can use the deltalake default logstore which use PutIfAbsent
if options.0.keys().any(|key| {
let key = key.to_ascii_lowercase();
vec![
[
AmazonS3ConfigKey::ConditionalPut.as_ref(),
"conditional_put",
]
Expand All @@ -69,7 +69,7 @@ impl LogStoreFactory for S3LogStoreFactory {

if options.0.keys().any(|key| {
let key = key.to_ascii_lowercase();
vec![
[
AmazonS3ConfigKey::CopyIfNotExists.as_ref(),
"copy_if_not_exists",
]
Expand Down Expand Up @@ -306,9 +306,11 @@ impl DynamoDbLockClient {
.send()
.await
},
|err| match err.as_service_error() {
Some(GetItemError::ProvisionedThroughputExceededException(_)) => true,
_ => false,
|err| {
matches!(
err.as_service_error(),
Some(GetItemError::ProvisionedThroughputExceededException(_))
)
},
)
.await
Expand Down Expand Up @@ -340,9 +342,11 @@ impl DynamoDbLockClient {
.await?;
Ok(())
},
|err: &SdkError<_, _>| match err.as_service_error() {
Some(PutItemError::ProvisionedThroughputExceededException(_)) => true,
_ => false,
|err: &SdkError<_, _>| {
matches!(
err.as_service_error(),
Some(PutItemError::ProvisionedThroughputExceededException(_))
)
},
)
.await
Expand Down Expand Up @@ -395,9 +399,11 @@ impl DynamoDbLockClient {
.send()
.await
},
|err: &SdkError<_, _>| match err.as_service_error() {
Some(QueryError::ProvisionedThroughputExceededException(_)) => true,
_ => false,
|err: &SdkError<_, _>| {
matches!(
err.as_service_error(),
Some(QueryError::ProvisionedThroughputExceededException(_))
)
},
)
.await
Expand Down Expand Up @@ -446,9 +452,11 @@ impl DynamoDbLockClient {
.await?;
Ok(())
},
|err: &SdkError<_, _>| match err.as_service_error() {
Some(UpdateItemError::ProvisionedThroughputExceededException(_)) => true,
_ => false,
|err: &SdkError<_, _>| {
matches!(
err.as_service_error(),
Some(UpdateItemError::ProvisionedThroughputExceededException(_))
)
},
)
.await;
Expand Down Expand Up @@ -488,9 +496,11 @@ impl DynamoDbLockClient {
.await?;
Ok(())
},
|err: &SdkError<_, _>| match err.as_service_error() {
Some(DeleteItemError::ProvisionedThroughputExceededException(_)) => true,
_ => false,
|err: &SdkError<_, _>| {
matches!(
err.as_service_error(),
Some(DeleteItemError::ProvisionedThroughputExceededException(_))
)
},
)
.await
Expand Down
Loading
Loading