From 059744527830c6b4b9870ab0c8b9d216f5ce535d Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Tue, 3 Jan 2023 14:10:00 +0100 Subject: [PATCH 01/10] feat: draf configuration api for azure --- object_store/Cargo.toml | 3 +- object_store/src/azure/mod.rs | 188 +++++++++++++++++++++++++++++----- 2 files changed, 167 insertions(+), 24 deletions(-) diff --git a/object_store/Cargo.toml b/object_store/Cargo.toml index a9cc151b985a..2f352729bfcd 100644 --- a/object_store/Cargo.toml +++ b/object_store/Cargo.toml @@ -44,6 +44,7 @@ walkdir = "2" # Cloud storage support base64 = { version = "0.20", default-features = false, features = ["std"], optional = true } +once_cell = { version = "1.12.0", optional = true } quick-xml = { version = "0.27.0", features = ["serialize"], optional = true } serde = { version = "1.0", default-features = false, features = ["derive"], optional = true } serde_json = { version = "1.0", default-features = false, optional = true } @@ -57,7 +58,7 @@ aws-types = { version = "0.52", optional = true } aws-config = { version = "0.52", optional = true } [features] -cloud = ["serde", "serde_json", "quick-xml", "reqwest", "reqwest/json", "reqwest/stream", "chrono/serde", "base64", "rand", "ring"] +cloud = ["serde", "serde_json", "quick-xml", "reqwest", "reqwest/json", "reqwest/stream", "chrono/serde", "base64", "rand", "ring", "once_cell"] azure = ["cloud"] gcp = ["cloud", "rustls-pemfile"] aws = ["cloud"] diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index 7cf369de3b3a..e0bd791808b4 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -37,8 +37,10 @@ use async_trait::async_trait; use bytes::Bytes; use chrono::{TimeZone, Utc}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; +use once_cell::sync::Lazy; +use percent_encoding::percent_decode_str; use snafu::{OptionExt, ResultExt, Snafu}; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::fmt::{Debug, Formatter}; use std::io; use std::ops::Range; @@ -124,6 +126,12 @@ enum Error { #[snafu(display("URL did not match any known pattern for scheme: {}", url))] UrlNotRecognised { url: String }, + + #[snafu(display("Failed parsing an SAS key"))] + DecodeSasKey { source: std::str::Utf8Error }, + + #[snafu(display("Missing component in SAS query pair"))] + MissingSasComponent {}, } impl From for super::Error { @@ -367,6 +375,7 @@ pub struct MicrosoftAzureBuilder { client_secret: Option, tenant_id: Option, sas_query_pairs: Option>, + sas_key: Option, authority_host: Option, url: Option, use_emulator: bool, @@ -374,6 +383,95 @@ pub struct MicrosoftAzureBuilder { client_options: ClientOptions, } +#[derive(PartialEq, Eq)] +enum AzureConfigKey { + /// The name of the azure storage account + /// + /// Supported keys: + /// - `azure_storage_account_name` + /// - `account_name` + AccountName, + + /// Master key for accessing storage account + /// + /// Supported keys: + /// - `azure_storage_account_key` + /// - `azure_storage_access_key` + /// - `azure_storage_master_key` + /// - `access_key` + /// - `account_key` + /// - `master_key` + AccessKey, + + /// Service principal client id for authorizing requests + /// + /// Supported keys: + /// - `azure_storage_client_id` + /// - `azure_client_id` + /// - `client_id` + ClientId, + + /// Service principal client secret for authorizing requests + /// + /// Supported keys: + /// - `azure_storage_client_secret` + /// - `azure_client_secret` + /// - `client_secret` + ClientSecret, + + /// Tenant id used in oauth flows + /// + /// Supported keys: + /// - `azure_storage_tenant_id` + /// - `azure_storage_authority_id` + /// - `azure_tenant_id` + /// - `azure_authority_id` + /// - `tenant_id` + /// - `authority_id` + AuthorityId, + SasKey, + UseEmulator, +} + +static ALIAS_MAP: Lazy> = Lazy::new(|| { + HashMap::from([ + // access key + ("azure_storage_account_key", AzureConfigKey::AccessKey), + ("azure_storage_access_key", AzureConfigKey::AccessKey), + ("azure_storage_master_key", AzureConfigKey::AccessKey), + ("master_key", AzureConfigKey::AccessKey), + ("account_key", AzureConfigKey::AccessKey), + ("access_key", AzureConfigKey::AccessKey), + // sas key + ("azure_storage_sas_token", AzureConfigKey::SasKey), + ("azure_storage_sas_key", AzureConfigKey::SasKey), + ("sas_token", AzureConfigKey::SasKey), + ("sas_key", AzureConfigKey::SasKey), + // account name + ("azure_storage_account_name", AzureConfigKey::AccountName), + ("account_name", AzureConfigKey::AccountName), + // client id + ("azure_storage_client_id", AzureConfigKey::ClientId), + ("azure_client_id", AzureConfigKey::ClientId), + ("client_id", AzureConfigKey::ClientId), + // client secret + ("azure_storage_client_secret", AzureConfigKey::ClientSecret), + ("azure_client_secret", AzureConfigKey::ClientSecret), + ("client_secret", AzureConfigKey::ClientSecret), + // authority id + ("azure_storage_tenant_id", AzureConfigKey::AuthorityId), + ("azure_storage_authority_id", AzureConfigKey::AuthorityId), + ("azure_tenant_id", AzureConfigKey::AuthorityId), + ("azure_authority_id", AzureConfigKey::AuthorityId), + ("tenant_id", AzureConfigKey::AuthorityId), + ("authority_id", AzureConfigKey::AuthorityId), + // use emulator + ("azure_storage_use_emulator", AzureConfigKey::UseEmulator), + ("object_store_use_emulator", AzureConfigKey::UseEmulator), + ("use_emulator", AzureConfigKey::UseEmulator), + ]) +}); + impl Debug for MicrosoftAzureBuilder { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( @@ -409,29 +507,13 @@ impl MicrosoftAzureBuilder { /// ``` pub fn from_env() -> Self { let mut builder = Self::default(); - - if let Ok(account_name) = std::env::var("AZURE_STORAGE_ACCOUNT_NAME") { - builder.account_name = Some(account_name); - } - - if let Ok(access_key) = std::env::var("AZURE_STORAGE_ACCOUNT_KEY") { - builder.access_key = Some(access_key); - } else if let Ok(access_key) = std::env::var("AZURE_STORAGE_ACCESS_KEY") { - builder.access_key = Some(access_key); - } - - if let Ok(client_id) = std::env::var("AZURE_STORAGE_CLIENT_ID") { - builder.client_id = Some(client_id); - } - - if let Ok(client_secret) = std::env::var("AZURE_STORAGE_CLIENT_SECRET") { - builder.client_secret = Some(client_secret); - } - - if let Ok(tenant_id) = std::env::var("AZURE_STORAGE_TENANT_ID") { - builder.tenant_id = Some(tenant_id); + for (key, _) in ALIAS_MAP.iter() { + if key.starts_with("azure_") { + if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { + builder = builder.with_option(*key, value) + } + } } - builder } @@ -462,6 +544,37 @@ impl MicrosoftAzureBuilder { self } + /// Set an option on the builder via a key - value pair. + pub fn with_option( + mut self, + key: impl Into, + value: impl Into, + ) -> Self { + let raw = key.into(); + if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { + match key { + AzureConfigKey::AccessKey => self.access_key = Some(value.into()), + AzureConfigKey::AccountName => self.account_name = Some(value.into()), + AzureConfigKey::ClientId => self.client_id = Some(value.into()), + AzureConfigKey::ClientSecret => self.client_secret = Some(value.into()), + AzureConfigKey::AuthorityId => self.tenant_id = Some(value.into()), + AzureConfigKey::SasKey => self.sas_key = Some(value.into()), + AzureConfigKey::UseEmulator => { + self.use_emulator = str_is_truthy(&value.into()) + } + }; + } + self + } + + /// Hydrate builder from key value pairs + pub fn with_options(mut self, options: &HashMap) -> Self { + for (key, value) in options { + self = self.with_option(key, value); + } + self + } + /// Sets properties on this builder based on a URL /// /// This is a separate member function to allow fallible computation to @@ -636,6 +749,8 @@ impl MicrosoftAzureBuilder { )) } else if let Some(query_pairs) = self.sas_query_pairs { Ok(credential::CredentialProvider::SASToken(query_pairs)) + } else if let Some(sas) = self.sas_key { + Ok(credential::CredentialProvider::SASToken(split_sas(&sas)?)) } else { Err(Error::MissingCredentials {}) }?; @@ -673,6 +788,33 @@ fn url_from_env(env_name: &str, default_url: &str) -> Result { Ok(url) } +fn split_sas(sas: &str) -> Result, Error> { + let sas = percent_decode_str(sas) + .decode_utf8() + .context(DecodeSasKeySnafu {})?; + let kv_str_pairs = sas + .trim_start_matches('?') + .split('&') + .filter(|s| !s.chars().all(char::is_whitespace)); + let mut pairs = Vec::new(); + for kv_pair_str in kv_str_pairs { + let (k, v) = kv_pair_str + .trim() + .split_once('=') + .ok_or(Error::MissingSasComponent {})?; + pairs.push((k.into(), v.into())) + } + Ok(pairs) +} + +pub(crate) fn str_is_truthy(val: &str) -> bool { + val == "1" + || val.to_lowercase() == "true" + || val.to_lowercase() == "on" + || val.to_lowercase() == "yes" + || val.to_lowercase() == "y" +} + #[cfg(test)] mod tests { use super::*; From 360c9db04c577b87f48e32abf9bbec316bdcda75 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Tue, 3 Jan 2023 21:33:38 +0100 Subject: [PATCH 02/10] feat: add configuration for aws and gcp --- object_store/src/aws/mod.rs | 259 +++++++++++++++++++++++++++++++--- object_store/src/azure/mod.rs | 71 ++++++++-- object_store/src/gcp/mod.rs | 91 +++++++++++- object_store/src/util.rs | 9 ++ 4 files changed, 395 insertions(+), 35 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index 786ccd20f18a..5c626c786563 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -37,8 +37,9 @@ use chrono::{DateTime, Utc}; use futures::stream::BoxStream; use futures::TryStreamExt; use itertools::Itertools; +use once_cell::sync::Lazy; use snafu::{OptionExt, ResultExt, Snafu}; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::ops::Range; use std::sync::Arc; use tokio::io::AsyncWrite; @@ -51,6 +52,7 @@ use crate::aws::credential::{ StaticCredentialProvider, WebIdentityProvider, }; use crate::multipart::{CloudMultiPartUpload, CloudMultiPartUploadImpl, UploadPart}; +use crate::util::str_is_truthy; use crate::{ ClientOptions, GetResult, ListResult, MultipartId, ObjectMeta, ObjectStore, Path, Result, RetryConfig, StreamExt, @@ -379,6 +381,162 @@ pub struct AmazonS3Builder { client_options: ClientOptions, } +#[derive(PartialEq, Eq)] +enum AmazonS3ConfigKey { + /// AWS Access Key + /// + /// See [`AmazonS3Builder::with_access_key_id`] for details. + /// + /// Supported keys: + /// - `aws_access_key_id` + /// - `access_key_id` + AccessKeyId, + + /// Secret Access Key + /// + /// See [`AmazonS3Builder::with_secret_access_key`] for details. + /// + /// Supported keys: + /// - `aws_secret_access_key` + /// - `secret_access_key` + SecretAccessKey, + + /// Region + /// + /// See [`AmazonS3Builder::with_region`] for details. + /// + /// Supported keys: + /// - `aws_region` + /// - `region` + Region, + + /// Default region + /// + /// See [`AmazonS3Builder::with_region`] for details. + /// + /// Supported keys: + /// - `aws_default_region` + /// - `default_region` + DefaultRegion, + + /// Bucket name + /// + /// See [`AmazonS3Builder::with_bucket_name`] for details. + /// + /// Supported keys: + /// - `aws_bucket` + /// - `aws_bucket_name` + /// - `bucket` + /// - `bucket_name` + Bucket, + + /// Sets custom endpoint for communicating with AWS S3. + /// + /// See [`AmazonS3Builder::with_endpoint`] for details. + /// + /// Supported keys: + /// - `aws_endpoint` + /// - `aws_endpoint_url` + /// - `endpoint` + /// - `endpoint_url` + Endpoint, + + /// Token to use for requests (passed to underlying provider) + /// + /// See [`AmazonS3Builder::with_token`] for details. + /// + /// Supported keys: + /// - `aws_session_token` + /// - `aws_token` + /// - `session_token` + /// - `token` + Token, + + /// Fall back to ImdsV1 + /// + /// See [`AmazonS3Builder::with_imdsv1_fallback`] for details. + /// + /// Supported keys: + /// - `aws_imdsv1_fallback` + /// - `imdsv1_fallback` + ImdsV1Fallback, + + /// If virtual hosted style request has to be used + /// + /// See [`AmazonS3Builder::with_virtual_hosted_style_request`] for details. + /// + /// Supported keys: + /// - `aws_virtual_hosted_style_request` + /// - `virtual_hosted_style_request` + VirtualHostedStyleRequest, + + /// Set the instance metadata endpoint + /// + /// See [`AmazonS3Builder::with_metadata_endpoint`] for details. + /// + /// Supported keys: + /// - `aws_metadata_endpoint` + /// - `metadata_endpoint` + MetadataEndpoint, + + /// AWS profile name + /// + /// Supported keys: + /// - `aws_profile` + /// - `profile` + Profile, +} + +static ALIAS_MAP: Lazy> = Lazy::new(|| { + BTreeMap::from([ + // access key id + ("aws_access_key_id", AmazonS3ConfigKey::AccessKeyId), + ("access_key_id", AmazonS3ConfigKey::AccessKeyId), + // secret access key + ("aws_secret_access_key", AmazonS3ConfigKey::SecretAccessKey), + ("secret_access_key", AmazonS3ConfigKey::SecretAccessKey), + // default region + ("aws_default_region", AmazonS3ConfigKey::DefaultRegion), + ("default_region", AmazonS3ConfigKey::DefaultRegion), + // region + ("aws_region", AmazonS3ConfigKey::Region), + ("region", AmazonS3ConfigKey::Region), + // bucket + ("aws_bucket", AmazonS3ConfigKey::Bucket), + ("aws_bucket_name", AmazonS3ConfigKey::Bucket), + ("bucket_name", AmazonS3ConfigKey::Bucket), + ("bucket", AmazonS3ConfigKey::Bucket), + // custom S3 endpoint + ("aws_endpoint_url", AmazonS3ConfigKey::Endpoint), + ("aws_endpoint", AmazonS3ConfigKey::Endpoint), + ("endpoint_url", AmazonS3ConfigKey::Endpoint), + ("endpoint", AmazonS3ConfigKey::Endpoint), + // session token + ("aws_session_token", AmazonS3ConfigKey::Token), + ("aws_token", AmazonS3ConfigKey::Token), + ("session_token", AmazonS3ConfigKey::Token), + ("token", AmazonS3ConfigKey::Token), + // virtual hosted style request + ( + "aws_virtual_hosted_style_request", + AmazonS3ConfigKey::VirtualHostedStyleRequest, + ), + ( + "virtual_hosted_style_request", + AmazonS3ConfigKey::VirtualHostedStyleRequest, + ), + // profile + ("aws_profile", AmazonS3ConfigKey::Profile), + ("profile", AmazonS3ConfigKey::Profile), + // imds v1 fallback + ("aws_imdsv1_fallback", AmazonS3ConfigKey::ImdsV1Fallback), + ("imdsv1_fallback", AmazonS3ConfigKey::ImdsV1Fallback), + // metadata endpoint + ("aws_metadata_endpoint", AmazonS3ConfigKey::MetadataEndpoint), + ("metadata_endpoint", AmazonS3ConfigKey::MetadataEndpoint), + ]) +}); + impl AmazonS3Builder { /// Create a new [`AmazonS3Builder`] with default values. pub fn new() -> Self { @@ -407,28 +565,12 @@ impl AmazonS3Builder { pub fn from_env() -> Self { let mut builder: Self = Default::default(); - if let Ok(access_key_id) = std::env::var("AWS_ACCESS_KEY_ID") { - builder.access_key_id = Some(access_key_id); - } - - if let Ok(secret_access_key) = std::env::var("AWS_SECRET_ACCESS_KEY") { - builder.secret_access_key = Some(secret_access_key); - } - - if let Ok(secret) = std::env::var("AWS_DEFAULT_REGION") { - builder.region = Some(secret); - } - - if let Ok(endpoint) = std::env::var("AWS_ENDPOINT") { - builder.endpoint = Some(endpoint); - } - - if let Ok(token) = std::env::var("AWS_SESSION_TOKEN") { - builder.token = Some(token); - } - - if let Ok(profile) = std::env::var("AWS_PROFILE") { - builder.profile = Some(profile); + for (key, _) in ALIAS_MAP.iter() { + if key.starts_with("aws_") { + if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { + builder = builder.with_option(*key, value) + } + } } // This env var is set in ECS @@ -472,6 +614,49 @@ impl AmazonS3Builder { self } + /// Set an option on the builder via a key - value pair. + pub fn with_option( + mut self, + key: impl Into, + value: impl Into, + ) -> Self { + let raw = key.into(); + if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { + match key { + AmazonS3ConfigKey::AccessKeyId => self.access_key_id = Some(value.into()), + AmazonS3ConfigKey::SecretAccessKey => { + self.secret_access_key = Some(value.into()) + } + AmazonS3ConfigKey::Region => self.region = Some(value.into()), + AmazonS3ConfigKey::Bucket => self.bucket_name = Some(value.into()), + AmazonS3ConfigKey::Endpoint => self.endpoint = Some(value.into()), + AmazonS3ConfigKey::Token => self.token = Some(value.into()), + AmazonS3ConfigKey::ImdsV1Fallback => { + self.imdsv1_fallback = str_is_truthy(&value.into()) + } + AmazonS3ConfigKey::VirtualHostedStyleRequest => { + self.virtual_hosted_style_request = str_is_truthy(&value.into()) + } + AmazonS3ConfigKey::DefaultRegion => { + self.region = self.region.or_else(|| Some(value.into())) + } + AmazonS3ConfigKey::MetadataEndpoint => { + self.metadata_endpoint = Some(value.into()) + } + AmazonS3ConfigKey::Profile => self.profile = Some(value.into()), + }; + } + self + } + + /// Hydrate builder from key value pairs + pub fn with_options(mut self, options: &HashMap) -> Self { + for (key, value) in options { + self = self.with_option(key, value); + } + self + } + /// Sets properties on this builder based on a URL /// /// This is a separate member function to allow fallible computation to @@ -915,6 +1100,34 @@ mod tests { assert_eq!(builder.metadata_endpoint.unwrap(), metadata_uri); } + #[test] + fn s3_test_config_from_map() { + let aws_access_key_id = "object_store:fake_access_key_id".to_string(); + let aws_secret_access_key = "object_store:fake_secret_key".to_string(); + let aws_default_region = "object_store:fake_default_region".to_string(); + let aws_endpoint = "object_store:fake_endpoint".to_string(); + let aws_session_token = "object_store:fake_session_token".to_string(); + let options = HashMap::from([ + ("aws_access_key_id".to_string(), aws_access_key_id.clone()), + ( + "aws_secret_access_key".to_string(), + aws_secret_access_key.clone(), + ), + ("aws_default_region".to_string(), aws_default_region.clone()), + ("aws_endpoint".to_string(), aws_endpoint.clone()), + ("aws_session_token".to_string(), aws_session_token.clone()), + ]); + + let builder = AmazonS3Builder::new() + .with_options(&options) + .with_option("aws_secret_access_key", "new-secret-key"); + assert_eq!(builder.access_key_id.unwrap(), aws_access_key_id.as_str()); + assert_eq!(builder.secret_access_key.unwrap(), "new-secret-key"); + assert_eq!(builder.region.unwrap(), aws_default_region); + assert_eq!(builder.endpoint.unwrap(), aws_endpoint); + assert_eq!(builder.token.unwrap(), aws_session_token); + } + #[tokio::test] async fn s3_test() { let config = maybe_skip_integration!(); diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index e0bd791808b4..6428e432dc0f 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -48,7 +48,7 @@ use std::sync::Arc; use tokio::io::AsyncWrite; use url::Url; -use crate::util::RFC1123_FMT; +use crate::util::{str_is_truthy, RFC1123_FMT}; pub use credential::authority_hosts; mod client; @@ -429,7 +429,33 @@ enum AzureConfigKey { /// - `tenant_id` /// - `authority_id` AuthorityId, + + /// Shared access signature. + /// + /// The signature is expected to be percent-encoded, much like they are provided + /// in the azure storage explorer or azure protal. + /// + /// Supported keys: + /// - `azure_storage_sas_key` + /// - `azure_storage_sas_token` + /// - `sas_key` + /// - `sas_token` SasKey, + + /// Bearer token + /// + /// Supported keys: + /// - `azure_storage_token` + /// - `bearer_token` + /// - `token` + Token, + + /// Use object store with azurite storage emulator + /// + /// Supported keys: + /// - `azure_storage_use_emulator` + /// - `object_store_use_emulator` + /// - `use_emulator` UseEmulator, } @@ -465,6 +491,15 @@ static ALIAS_MAP: Lazy> = Lazy::new(|| { ("azure_authority_id", AzureConfigKey::AuthorityId), ("tenant_id", AzureConfigKey::AuthorityId), ("authority_id", AzureConfigKey::AuthorityId), + // account name + ("azure_storage_sas_key", AzureConfigKey::SasKey), + ("azure_storage_sas_token", AzureConfigKey::SasKey), + ("sas_key", AzureConfigKey::SasKey), + ("sas_token", AzureConfigKey::SasKey), + // bearer token + ("azure_storage_token", AzureConfigKey::Token), + ("bearer_token", AzureConfigKey::Token), + ("token", AzureConfigKey::Token), // use emulator ("azure_storage_use_emulator", AzureConfigKey::UseEmulator), ("object_store_use_emulator", AzureConfigKey::UseEmulator), @@ -559,6 +594,7 @@ impl MicrosoftAzureBuilder { AzureConfigKey::ClientSecret => self.client_secret = Some(value.into()), AzureConfigKey::AuthorityId => self.tenant_id = Some(value.into()), AzureConfigKey::SasKey => self.sas_key = Some(value.into()), + AzureConfigKey::Token => self.bearer_token = Some(value.into()), AzureConfigKey::UseEmulator => { self.use_emulator = str_is_truthy(&value.into()) } @@ -807,14 +843,6 @@ fn split_sas(sas: &str) -> Result, Error> { Ok(pairs) } -pub(crate) fn str_is_truthy(val: &str) -> bool { - val == "1" - || val.to_lowercase() == "true" - || val.to_lowercase() == "on" - || val.to_lowercase() == "yes" - || val.to_lowercase() == "y" -} - #[cfg(test)] mod tests { use super::*; @@ -974,4 +1002,29 @@ mod tests { builder.parse_url(case).unwrap_err(); } } + + #[test] + fn azure_test_config_from_map() { + let azure_client_id = "object_store:fake_access_key_id".to_string(); + let azure_storage_account_name = "object_store:fake_secret_key".to_string(); + let azure_storage_token = "object_store:fake_default_region".to_string(); + let options = HashMap::from([ + ("azure_client_id".to_string(), azure_client_id.clone()), + ( + "azure_storage_account_name".to_string(), + azure_storage_account_name.clone(), + ), + ( + "azure_storage_token".to_string(), + azure_storage_token.clone(), + ), + ]); + + let builder = MicrosoftAzureBuilder::new() + .with_options(&options) + .with_option("unknown-key", "unknown-value"); + assert_eq!(builder.client_id.unwrap(), azure_client_id); + assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); + assert_eq!(builder.bearer_token.unwrap(), azure_storage_token); + } } diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index f2638748f6ca..5ae5ab4cc754 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -29,7 +29,7 @@ //! to abort the upload and drop those unneeded parts. In addition, you may wish to //! consider implementing automatic clean up of unused parts that are older than one //! week. -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::fs::File; use std::io::{self, BufReader}; use std::ops::Range; @@ -39,6 +39,7 @@ use async_trait::async_trait; use bytes::{Buf, Bytes}; use chrono::{DateTime, Utc}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; +use once_cell::sync::Lazy; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use reqwest::header::RANGE; use reqwest::{header, Client, Method, Response, StatusCode}; @@ -796,6 +797,40 @@ pub struct GoogleCloudStorageBuilder { client_options: ClientOptions, } +#[derive(PartialEq, Eq)] +enum GoogleConfigKey { + /// Path to the service account file + /// + /// Supported keys: + /// - `google_service_account` + /// - `service_account` + ServiceAccount, + + /// Bucket name + /// + /// See [`AmazonS3Builder::with_bucket_name`] for details. + /// + /// Supported keys: + /// - `google_bucket` + /// - `google_bucket_name` + /// - `bucket` + /// - `bucket_name` + Bucket, +} + +static ALIAS_MAP: Lazy> = Lazy::new(|| { + BTreeMap::from([ + // service account + ("google_service_account", GoogleConfigKey::ServiceAccount), + ("service_account", GoogleConfigKey::ServiceAccount), + // bucket + ("google_bucket", GoogleConfigKey::Bucket), + ("google_bucket_name", GoogleConfigKey::Bucket), + ("bucket_name", GoogleConfigKey::Bucket), + ("bucket", GoogleConfigKey::Bucket), + ]) +}); + impl Default for GoogleCloudStorageBuilder { fn default() -> Self { Self { @@ -835,8 +870,12 @@ impl GoogleCloudStorageBuilder { builder.service_account_path = Some(service_account_path); } - if let Ok(service_account_path) = std::env::var("GOOGLE_SERVICE_ACCOUNT") { - builder.service_account_path = Some(service_account_path); + for (key, _) in ALIAS_MAP.iter() { + if key.starts_with("google_") { + if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { + builder = builder.with_option(*key, value) + } + } } builder @@ -863,6 +902,32 @@ impl GoogleCloudStorageBuilder { self } + /// Set an option on the builder via a key - value pair. + pub fn with_option( + mut self, + key: impl Into, + value: impl Into, + ) -> Self { + let raw = key.into(); + if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { + match key { + GoogleConfigKey::ServiceAccount => { + self.service_account_path = Some(value.into()) + } + GoogleConfigKey::Bucket => self.bucket_name = Some(value.into()), + }; + } + self + } + + /// Hydrate builder from key value pairs + pub fn with_options(mut self, options: &HashMap) -> Self { + for (key, value) in options { + self = self.with_option(key, value); + } + self + } + /// Sets properties on this builder based on a URL /// /// This is a separate member function to allow fallible computation to @@ -1205,4 +1270,24 @@ mod test { builder.parse_url(case).unwrap_err(); } } + + #[test] + fn gcs_test_config_from_map() { + let google_service_account = "object_store:fake_service_account".to_string(); + let google_bucket_name = "object_store:fake_bucket".to_string(); + let options = HashMap::from([ + ( + "google_service_account".to_string(), + google_service_account.clone(), + ), + ("google_bucket_name".to_string(), google_bucket_name.clone()), + ]); + + let builder = GoogleCloudStorageBuilder::new().with_options(&options); + assert_eq!( + builder.service_account_path.unwrap(), + google_service_account.as_str() + ); + assert_eq!(builder.bucket_name.unwrap(), google_bucket_name.as_str()); + } } diff --git a/object_store/src/util.rs b/object_store/src/util.rs index e592e7b64f2d..0e4e740e4314 100644 --- a/object_store/src/util.rs +++ b/object_store/src/util.rs @@ -185,6 +185,15 @@ fn merge_ranges( ret } +#[allow(dead_code)] +pub(crate) fn str_is_truthy(val: &str) -> bool { + val == "1" + || val.to_lowercase() == "true" + || val.to_lowercase() == "on" + || val.to_lowercase() == "yes" + || val.to_lowercase() == "y" +} + #[cfg(test)] mod tests { use super::*; From 3ab6ac8ab66ca5387c8265eacb00279253004bb9 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Tue, 3 Jan 2023 21:43:31 +0100 Subject: [PATCH 03/10] fix: clippy --- object_store/src/aws/mod.rs | 5 +---- object_store/src/gcp/mod.rs | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index 5c626c786563..808be4a4e671 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -1109,10 +1109,7 @@ mod tests { let aws_session_token = "object_store:fake_session_token".to_string(); let options = HashMap::from([ ("aws_access_key_id".to_string(), aws_access_key_id.clone()), - ( - "aws_secret_access_key".to_string(), - aws_secret_access_key.clone(), - ), + ("aws_secret_access_key".to_string(), aws_secret_access_key), ("aws_default_region".to_string(), aws_default_region.clone()), ("aws_endpoint".to_string(), aws_endpoint.clone()), ("aws_session_token".to_string(), aws_session_token.clone()), diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 5ae5ab4cc754..56d2a7062c82 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -808,7 +808,7 @@ enum GoogleConfigKey { /// Bucket name /// - /// See [`AmazonS3Builder::with_bucket_name`] for details. + /// See [`GoogleCloudStorageBuilder::with_bucket_name`] for details. /// /// Supported keys: /// - `google_bucket` From ac638a7b197157bf97e6b7d1ba2ecc7f74c56090 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 10:29:27 +0100 Subject: [PATCH 04/10] feat: allow passing typed config keys --- object_store/src/aws/mod.rs | 57 ++++++++++++++++++++++++++++++++--- object_store/src/azure/mod.rs | 54 +++++++++++++++++++++++++++------ object_store/src/gcp/mod.rs | 41 ++++++++++++++++++++++--- 3 files changed, 135 insertions(+), 17 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index 808be4a4e671..d33e8b67923f 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -381,8 +381,9 @@ pub struct AmazonS3Builder { client_options: ClientOptions, } -#[derive(PartialEq, Eq)] -enum AmazonS3ConfigKey { +/// Configuration keys for [`AmazonS3Builder`] +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +pub enum AmazonS3ConfigKey { /// AWS Access Key /// /// See [`AmazonS3Builder::with_access_key_id`] for details. @@ -487,6 +488,26 @@ enum AmazonS3ConfigKey { Profile, } +impl From for String { + fn from(value: AmazonS3ConfigKey) -> Self { + match value { + AmazonS3ConfigKey::AccessKeyId => Self::from("aws_access_key_id"), + AmazonS3ConfigKey::SecretAccessKey => Self::from("aws_secret_access_key"), + AmazonS3ConfigKey::Region => Self::from("aws_region"), + AmazonS3ConfigKey::Bucket => Self::from("aws_bucket"), + AmazonS3ConfigKey::Endpoint => Self::from("aws_endpoint"), + AmazonS3ConfigKey::Token => Self::from("aws_session_token"), + AmazonS3ConfigKey::ImdsV1Fallback => Self::from("aws_imdsv1_fallback"), + AmazonS3ConfigKey::VirtualHostedStyleRequest => { + Self::from("aws_virtual_hosted_style_request") + } + AmazonS3ConfigKey::DefaultRegion => Self::from("aws_default_region"), + AmazonS3ConfigKey::MetadataEndpoint => Self::from("aws_metadata_endpoint"), + AmazonS3ConfigKey::Profile => Self::from("aws_profile"), + } + } +} + static ALIAS_MAP: Lazy> = Lazy::new(|| { BTreeMap::from([ // access key id @@ -650,9 +671,12 @@ impl AmazonS3Builder { } /// Hydrate builder from key value pairs - pub fn with_options(mut self, options: &HashMap) -> Self { + pub fn with_options( + mut self, + options: &HashMap + Clone, String>, + ) -> Self { for (key, value) in options { - self = self.with_option(key, value); + self = self.with_option(key.clone(), value); } self } @@ -1125,6 +1149,31 @@ mod tests { assert_eq!(builder.token.unwrap(), aws_session_token); } + #[test] + fn s3_test_config_from_typed_map() { + let aws_access_key_id = "object_store:fake_access_key_id".to_string(); + let aws_secret_access_key = "object_store:fake_secret_key".to_string(); + let aws_default_region = "object_store:fake_default_region".to_string(); + let aws_endpoint = "object_store:fake_endpoint".to_string(); + let aws_session_token = "object_store:fake_session_token".to_string(); + let options = HashMap::from([ + (AmazonS3ConfigKey::AccessKeyId, aws_access_key_id.clone()), + (AmazonS3ConfigKey::SecretAccessKey, aws_secret_access_key), + (AmazonS3ConfigKey::DefaultRegion, aws_default_region.clone()), + (AmazonS3ConfigKey::Endpoint, aws_endpoint.clone()), + (AmazonS3ConfigKey::Token, aws_session_token.clone()), + ]); + + let builder = AmazonS3Builder::new() + .with_options(&options) + .with_option(AmazonS3ConfigKey::SecretAccessKey, "new-secret-key"); + assert_eq!(builder.access_key_id.unwrap(), aws_access_key_id.as_str()); + assert_eq!(builder.secret_access_key.unwrap(), "new-secret-key"); + assert_eq!(builder.region.unwrap(), aws_default_region); + assert_eq!(builder.endpoint.unwrap(), aws_endpoint); + assert_eq!(builder.token.unwrap(), aws_session_token); + } + #[tokio::test] async fn s3_test() { let config = maybe_skip_integration!(); diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index 6428e432dc0f..368de6c2cf30 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -383,8 +383,9 @@ pub struct MicrosoftAzureBuilder { client_options: ClientOptions, } -#[derive(PartialEq, Eq)] -enum AzureConfigKey { +/// Configuration keys for [`MicrosoftAzureBuilder`] +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +pub enum AzureConfigKey { /// The name of the azure storage account /// /// Supported keys: @@ -459,6 +460,21 @@ enum AzureConfigKey { UseEmulator, } +impl From for String { + fn from(value: AzureConfigKey) -> Self { + match value { + AzureConfigKey::AccountName => Self::from("azure_storage_account_name"), + AzureConfigKey::AccessKey => Self::from("azure_storage_account_key"), + AzureConfigKey::ClientId => Self::from("azure_storage_client_id"), + AzureConfigKey::ClientSecret => Self::from("azure_storage_client_secret"), + AzureConfigKey::AuthorityId => Self::from("azure_storage_tenant_id"), + AzureConfigKey::SasKey => Self::from("azure_storage_sas_key"), + AzureConfigKey::Token => Self::from("azure_storage_token"), + AzureConfigKey::UseEmulator => Self::from("azure_storage_use_emulator"), + } + } +} + static ALIAS_MAP: Lazy> = Lazy::new(|| { HashMap::from([ // access key @@ -468,11 +484,6 @@ static ALIAS_MAP: Lazy> = Lazy::new(|| { ("master_key", AzureConfigKey::AccessKey), ("account_key", AzureConfigKey::AccessKey), ("access_key", AzureConfigKey::AccessKey), - // sas key - ("azure_storage_sas_token", AzureConfigKey::SasKey), - ("azure_storage_sas_key", AzureConfigKey::SasKey), - ("sas_token", AzureConfigKey::SasKey), - ("sas_key", AzureConfigKey::SasKey), // account name ("azure_storage_account_name", AzureConfigKey::AccountName), ("account_name", AzureConfigKey::AccountName), @@ -604,9 +615,12 @@ impl MicrosoftAzureBuilder { } /// Hydrate builder from key value pairs - pub fn with_options(mut self, options: &HashMap) -> Self { + pub fn with_options( + mut self, + options: &HashMap + Clone, String>, + ) -> Self { for (key, value) in options { - self = self.with_option(key, value); + self = self.with_option(key.clone(), value); } self } @@ -1027,4 +1041,26 @@ mod tests { assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); assert_eq!(builder.bearer_token.unwrap(), azure_storage_token); } + + #[test] + fn azure_test_config_from_typed_map() { + let azure_client_id = "object_store:fake_access_key_id".to_string(); + let azure_storage_account_name = "object_store:fake_secret_key".to_string(); + let azure_storage_token = "object_store:fake_default_region".to_string(); + let options = HashMap::from([ + (AzureConfigKey::ClientId, azure_client_id.clone()), + ( + AzureConfigKey::AccountName, + azure_storage_account_name.clone(), + ), + (AzureConfigKey::Token, azure_storage_token.clone()), + ]); + + let builder = MicrosoftAzureBuilder::new() + .with_options(&options) + .with_option("unknown-key", "unknown-value"); + assert_eq!(builder.client_id.unwrap(), azure_client_id); + assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); + assert_eq!(builder.bearer_token.unwrap(), azure_storage_token); + } } diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 56d2a7062c82..e905f4523a5f 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -797,8 +797,9 @@ pub struct GoogleCloudStorageBuilder { client_options: ClientOptions, } -#[derive(PartialEq, Eq)] -enum GoogleConfigKey { +/// Configuration keys for [`GoogleCloudStorageBuilder`] +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +pub enum GoogleConfigKey { /// Path to the service account file /// /// Supported keys: @@ -818,6 +819,15 @@ enum GoogleConfigKey { Bucket, } +impl From for String { + fn from(value: GoogleConfigKey) -> Self { + match value { + GoogleConfigKey::ServiceAccount => Self::from("google_service_account"), + GoogleConfigKey::Bucket => Self::from("google_bucket"), + } + } +} + static ALIAS_MAP: Lazy> = Lazy::new(|| { BTreeMap::from([ // service account @@ -921,9 +931,12 @@ impl GoogleCloudStorageBuilder { } /// Hydrate builder from key value pairs - pub fn with_options(mut self, options: &HashMap) -> Self { + pub fn with_options( + mut self, + options: &HashMap + Clone, String>, + ) -> Self { for (key, value) in options { - self = self.with_option(key, value); + self = self.with_option(key.clone(), value); } self } @@ -1290,4 +1303,24 @@ mod test { ); assert_eq!(builder.bucket_name.unwrap(), google_bucket_name.as_str()); } + + #[test] + fn gcs_test_config_from_typed_map() { + let google_service_account = "object_store:fake_service_account".to_string(); + let google_bucket_name = "object_store:fake_bucket".to_string(); + let options = HashMap::from([ + ( + GoogleConfigKey::ServiceAccount, + google_service_account.clone(), + ), + (GoogleConfigKey::Bucket, google_bucket_name.clone()), + ]); + + let builder = GoogleCloudStorageBuilder::new().with_options(&options); + assert_eq!( + builder.service_account_path.unwrap(), + google_service_account.as_str() + ); + assert_eq!(builder.bucket_name.unwrap(), google_bucket_name.as_str()); + } } From 2f5e5ccd4a012f1741489071f9e254f084b868b0 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 11:27:49 +0100 Subject: [PATCH 05/10] refactor: implement try_from for config keys --- object_store/Cargo.toml | 3 +- object_store/src/aws/mod.rs | 154 ++++++++++++++++------------------ object_store/src/azure/mod.rs | 127 ++++++++++++++-------------- object_store/src/gcp/mod.rs | 66 ++++++++------- 4 files changed, 171 insertions(+), 179 deletions(-) diff --git a/object_store/Cargo.toml b/object_store/Cargo.toml index 2f352729bfcd..a9cc151b985a 100644 --- a/object_store/Cargo.toml +++ b/object_store/Cargo.toml @@ -44,7 +44,6 @@ walkdir = "2" # Cloud storage support base64 = { version = "0.20", default-features = false, features = ["std"], optional = true } -once_cell = { version = "1.12.0", optional = true } quick-xml = { version = "0.27.0", features = ["serialize"], optional = true } serde = { version = "1.0", default-features = false, features = ["derive"], optional = true } serde_json = { version = "1.0", default-features = false, optional = true } @@ -58,7 +57,7 @@ aws-types = { version = "0.52", optional = true } aws-config = { version = "0.52", optional = true } [features] -cloud = ["serde", "serde_json", "quick-xml", "reqwest", "reqwest/json", "reqwest/stream", "chrono/serde", "base64", "rand", "ring", "once_cell"] +cloud = ["serde", "serde_json", "quick-xml", "reqwest", "reqwest/json", "reqwest/stream", "chrono/serde", "base64", "rand", "ring"] azure = ["cloud"] gcp = ["cloud", "rustls-pemfile"] aws = ["cloud"] diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index d33e8b67923f..e67e5d91fef2 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -37,9 +37,8 @@ use chrono::{DateTime, Utc}; use futures::stream::BoxStream; use futures::TryStreamExt; use itertools::Itertools; -use once_cell::sync::Lazy; use snafu::{OptionExt, ResultExt, Snafu}; -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeSet, HashMap}; use std::ops::Range; use std::sync::Arc; use tokio::io::AsyncWrite; @@ -135,6 +134,9 @@ enum Error { #[snafu(display("URL did not match any known pattern for scheme: {}", url))] UrlNotRecognised { url: String }, + + #[snafu(display("Configuration key: '{}' is not known.", key))] + UnknownConfigurationKey { key: String }, } impl From for super::Error { @@ -508,55 +510,42 @@ impl From for String { } } -static ALIAS_MAP: Lazy> = Lazy::new(|| { - BTreeMap::from([ - // access key id - ("aws_access_key_id", AmazonS3ConfigKey::AccessKeyId), - ("access_key_id", AmazonS3ConfigKey::AccessKeyId), - // secret access key - ("aws_secret_access_key", AmazonS3ConfigKey::SecretAccessKey), - ("secret_access_key", AmazonS3ConfigKey::SecretAccessKey), - // default region - ("aws_default_region", AmazonS3ConfigKey::DefaultRegion), - ("default_region", AmazonS3ConfigKey::DefaultRegion), - // region - ("aws_region", AmazonS3ConfigKey::Region), - ("region", AmazonS3ConfigKey::Region), - // bucket - ("aws_bucket", AmazonS3ConfigKey::Bucket), - ("aws_bucket_name", AmazonS3ConfigKey::Bucket), - ("bucket_name", AmazonS3ConfigKey::Bucket), - ("bucket", AmazonS3ConfigKey::Bucket), - // custom S3 endpoint - ("aws_endpoint_url", AmazonS3ConfigKey::Endpoint), - ("aws_endpoint", AmazonS3ConfigKey::Endpoint), - ("endpoint_url", AmazonS3ConfigKey::Endpoint), - ("endpoint", AmazonS3ConfigKey::Endpoint), - // session token - ("aws_session_token", AmazonS3ConfigKey::Token), - ("aws_token", AmazonS3ConfigKey::Token), - ("session_token", AmazonS3ConfigKey::Token), - ("token", AmazonS3ConfigKey::Token), - // virtual hosted style request - ( - "aws_virtual_hosted_style_request", - AmazonS3ConfigKey::VirtualHostedStyleRequest, - ), - ( - "virtual_hosted_style_request", - AmazonS3ConfigKey::VirtualHostedStyleRequest, - ), - // profile - ("aws_profile", AmazonS3ConfigKey::Profile), - ("profile", AmazonS3ConfigKey::Profile), - // imds v1 fallback - ("aws_imdsv1_fallback", AmazonS3ConfigKey::ImdsV1Fallback), - ("imdsv1_fallback", AmazonS3ConfigKey::ImdsV1Fallback), - // metadata endpoint - ("aws_metadata_endpoint", AmazonS3ConfigKey::MetadataEndpoint), - ("metadata_endpoint", AmazonS3ConfigKey::MetadataEndpoint), - ]) -}); +impl TryFrom<&str> for AmazonS3ConfigKey { + type Error = super::Error; + + fn try_from(value: &str) -> Result { + match value.to_ascii_lowercase().as_str() { + "aws_access_key_id" | "access_key_id" => Ok(Self::AccessKeyId), + "aws_secret_access_key" | "secret_access_key" => Ok(Self::SecretAccessKey), + "aws_default_region" | "default_region" => Ok(Self::DefaultRegion), + "aws_region" | "region" => Ok(Self::Region), + "aws_bucket" | "aws_bucket_name" | "bucket_name" | "bucket" => { + Ok(Self::Bucket) + } + "aws_endpoint_url" | "aws_endpoint" | "endpoint_url" | "endpoint" => { + Ok(Self::Endpoint) + } + "aws_session_token" | "aws_token" | "session_token" | "token" => { + Ok(Self::Token) + } + "aws_virtual_hosted_style_request" | "virtual_hosted_style_request" => { + Ok(Self::VirtualHostedStyleRequest) + } + "aws_profile" | "profile" => Ok(Self::Profile), + "aws_imdsv1_fallback" | "imdsv1_fallback" => Ok(Self::ImdsV1Fallback), + "aws_metadata_endpoint" | "metadata_endpoint" => Ok(Self::MetadataEndpoint), + _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + } + } +} + +impl TryFrom for AmazonS3ConfigKey { + type Error = super::Error; + + fn try_from(value: String) -> Result { + Self::try_from(value.as_str()) + } +} impl AmazonS3Builder { /// Create a new [`AmazonS3Builder`] with default values. @@ -586,12 +575,11 @@ impl AmazonS3Builder { pub fn from_env() -> Self { let mut builder: Self = Default::default(); - for (key, _) in ALIAS_MAP.iter() { - if key.starts_with("aws_") { - if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { - builder = builder.with_option(*key, value) - } - } + for (key, value) in std::env::vars() + .into_iter() + .filter(|(key, _)| key.starts_with("AWS_")) + { + builder = builder.with_option(key, value); } // This env var is set in ECS @@ -605,7 +593,7 @@ impl AmazonS3Builder { if let Ok(text) = std::env::var("AWS_ALLOW_HTTP") { builder.client_options = - builder.client_options.with_allow_http(text == "true"); + builder.client_options.with_allow_http(str_is_truthy(&text)); } builder @@ -641,32 +629,30 @@ impl AmazonS3Builder { key: impl Into, value: impl Into, ) -> Self { - let raw = key.into(); - if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { - match key { - AmazonS3ConfigKey::AccessKeyId => self.access_key_id = Some(value.into()), - AmazonS3ConfigKey::SecretAccessKey => { - self.secret_access_key = Some(value.into()) - } - AmazonS3ConfigKey::Region => self.region = Some(value.into()), - AmazonS3ConfigKey::Bucket => self.bucket_name = Some(value.into()), - AmazonS3ConfigKey::Endpoint => self.endpoint = Some(value.into()), - AmazonS3ConfigKey::Token => self.token = Some(value.into()), - AmazonS3ConfigKey::ImdsV1Fallback => { - self.imdsv1_fallback = str_is_truthy(&value.into()) - } - AmazonS3ConfigKey::VirtualHostedStyleRequest => { - self.virtual_hosted_style_request = str_is_truthy(&value.into()) - } - AmazonS3ConfigKey::DefaultRegion => { - self.region = self.region.or_else(|| Some(value.into())) - } - AmazonS3ConfigKey::MetadataEndpoint => { - self.metadata_endpoint = Some(value.into()) - } - AmazonS3ConfigKey::Profile => self.profile = Some(value.into()), - }; - } + match AmazonS3ConfigKey::try_from(key.into()) { + Ok(AmazonS3ConfigKey::AccessKeyId) => self.access_key_id = Some(value.into()), + Ok(AmazonS3ConfigKey::SecretAccessKey) => { + self.secret_access_key = Some(value.into()) + } + Ok(AmazonS3ConfigKey::Region) => self.region = Some(value.into()), + Ok(AmazonS3ConfigKey::Bucket) => self.bucket_name = Some(value.into()), + Ok(AmazonS3ConfigKey::Endpoint) => self.endpoint = Some(value.into()), + Ok(AmazonS3ConfigKey::Token) => self.token = Some(value.into()), + Ok(AmazonS3ConfigKey::ImdsV1Fallback) => { + self.imdsv1_fallback = str_is_truthy(&value.into()) + } + Ok(AmazonS3ConfigKey::VirtualHostedStyleRequest) => { + self.virtual_hosted_style_request = str_is_truthy(&value.into()) + } + Ok(AmazonS3ConfigKey::DefaultRegion) => { + self.region = self.region.or_else(|| Some(value.into())) + } + Ok(AmazonS3ConfigKey::MetadataEndpoint) => { + self.metadata_endpoint = Some(value.into()) + } + Ok(AmazonS3ConfigKey::Profile) => self.profile = Some(value.into()), + Err(_) => (), + }; self } diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index 368de6c2cf30..db9f93174d24 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -37,7 +37,6 @@ use async_trait::async_trait; use bytes::Bytes; use chrono::{TimeZone, Utc}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; -use once_cell::sync::Lazy; use percent_encoding::percent_decode_str; use snafu::{OptionExt, ResultExt, Snafu}; use std::collections::{BTreeSet, HashMap}; @@ -132,6 +131,9 @@ enum Error { #[snafu(display("Missing component in SAS query pair"))] MissingSasComponent {}, + + #[snafu(display("Configuration key: '{}' is not known.", key))] + UnknownConfigurationKey { key: String }, } impl From for super::Error { @@ -475,48 +477,48 @@ impl From for String { } } -static ALIAS_MAP: Lazy> = Lazy::new(|| { - HashMap::from([ - // access key - ("azure_storage_account_key", AzureConfigKey::AccessKey), - ("azure_storage_access_key", AzureConfigKey::AccessKey), - ("azure_storage_master_key", AzureConfigKey::AccessKey), - ("master_key", AzureConfigKey::AccessKey), - ("account_key", AzureConfigKey::AccessKey), - ("access_key", AzureConfigKey::AccessKey), - // account name - ("azure_storage_account_name", AzureConfigKey::AccountName), - ("account_name", AzureConfigKey::AccountName), - // client id - ("azure_storage_client_id", AzureConfigKey::ClientId), - ("azure_client_id", AzureConfigKey::ClientId), - ("client_id", AzureConfigKey::ClientId), - // client secret - ("azure_storage_client_secret", AzureConfigKey::ClientSecret), - ("azure_client_secret", AzureConfigKey::ClientSecret), - ("client_secret", AzureConfigKey::ClientSecret), - // authority id - ("azure_storage_tenant_id", AzureConfigKey::AuthorityId), - ("azure_storage_authority_id", AzureConfigKey::AuthorityId), - ("azure_tenant_id", AzureConfigKey::AuthorityId), - ("azure_authority_id", AzureConfigKey::AuthorityId), - ("tenant_id", AzureConfigKey::AuthorityId), - ("authority_id", AzureConfigKey::AuthorityId), - // account name - ("azure_storage_sas_key", AzureConfigKey::SasKey), - ("azure_storage_sas_token", AzureConfigKey::SasKey), - ("sas_key", AzureConfigKey::SasKey), - ("sas_token", AzureConfigKey::SasKey), - // bearer token - ("azure_storage_token", AzureConfigKey::Token), - ("bearer_token", AzureConfigKey::Token), - ("token", AzureConfigKey::Token), - // use emulator - ("azure_storage_use_emulator", AzureConfigKey::UseEmulator), - ("object_store_use_emulator", AzureConfigKey::UseEmulator), - ("use_emulator", AzureConfigKey::UseEmulator), - ]) -}); +impl TryFrom<&str> for AzureConfigKey { + type Error = super::Error; + + fn try_from(value: &str) -> Result { + match value.to_ascii_lowercase().as_str() { + "azure_storage_account_key" + | "azure_storage_access_key" + | "azure_storage_master_key" + | "master_key" + | "account_key" + | "access_key" => Ok(Self::AccessKey), + "azure_storage_account_name" | "account_name" => Ok(Self::AccountName), + "azure_storage_client_id" | "azure_client_id" | "client_id" => { + Ok(Self::ClientId) + } + "azure_storage_client_secret" | "azure_client_secret" | "client_secret" => { + Ok(Self::ClientSecret) + } + "azure_storage_tenant_id" + | "azure_storage_authority_id" + | "azure_tenant_id" + | "azure_authority_id" + | "tenant_id" + | "authority_id" => Ok(Self::AuthorityId), + "azure_storage_sas_key" + | "azure_storage_sas_token" + | "sas_key" + | "sas_token" => Ok(Self::SasKey), + "azure_storage_token" | "bearer_token" | "token" => Ok(Self::Token), + "azure_storage_use_emulator" | "use_emulator" => Ok(Self::UseEmulator), + _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + } + } +} + +impl TryFrom for AzureConfigKey { + type Error = super::Error; + + fn try_from(value: String) -> Result { + Self::try_from(value.as_str()) + } +} impl Debug for MicrosoftAzureBuilder { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { @@ -553,12 +555,11 @@ impl MicrosoftAzureBuilder { /// ``` pub fn from_env() -> Self { let mut builder = Self::default(); - for (key, _) in ALIAS_MAP.iter() { - if key.starts_with("azure_") { - if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { - builder = builder.with_option(*key, value) - } - } + for (key, value) in std::env::vars() + .into_iter() + .filter(|(key, _)| key.starts_with("AZURE_")) + { + builder = builder.with_option(key, value); } builder } @@ -596,21 +597,19 @@ impl MicrosoftAzureBuilder { key: impl Into, value: impl Into, ) -> Self { - let raw = key.into(); - if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { - match key { - AzureConfigKey::AccessKey => self.access_key = Some(value.into()), - AzureConfigKey::AccountName => self.account_name = Some(value.into()), - AzureConfigKey::ClientId => self.client_id = Some(value.into()), - AzureConfigKey::ClientSecret => self.client_secret = Some(value.into()), - AzureConfigKey::AuthorityId => self.tenant_id = Some(value.into()), - AzureConfigKey::SasKey => self.sas_key = Some(value.into()), - AzureConfigKey::Token => self.bearer_token = Some(value.into()), - AzureConfigKey::UseEmulator => { - self.use_emulator = str_is_truthy(&value.into()) - } - }; - } + match AzureConfigKey::try_from(key.into()) { + Ok(AzureConfigKey::AccessKey) => self.access_key = Some(value.into()), + Ok(AzureConfigKey::AccountName) => self.account_name = Some(value.into()), + Ok(AzureConfigKey::ClientId) => self.client_id = Some(value.into()), + Ok(AzureConfigKey::ClientSecret) => self.client_secret = Some(value.into()), + Ok(AzureConfigKey::AuthorityId) => self.tenant_id = Some(value.into()), + Ok(AzureConfigKey::SasKey) => self.sas_key = Some(value.into()), + Ok(AzureConfigKey::Token) => self.bearer_token = Some(value.into()), + Ok(AzureConfigKey::UseEmulator) => { + self.use_emulator = str_is_truthy(&value.into()) + } + Err(_) => (), + }; self } diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index e905f4523a5f..06b5ce4c10bb 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -29,7 +29,7 @@ //! to abort the upload and drop those unneeded parts. In addition, you may wish to //! consider implementing automatic clean up of unused parts that are older than one //! week. -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeSet, HashMap}; use std::fs::File; use std::io::{self, BufReader}; use std::ops::Range; @@ -39,7 +39,6 @@ use async_trait::async_trait; use bytes::{Buf, Bytes}; use chrono::{DateTime, Utc}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; -use once_cell::sync::Lazy; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use reqwest::header::RANGE; use reqwest::{header, Client, Method, Response, StatusCode}; @@ -146,6 +145,9 @@ enum Error { #[snafu(display("URL did not match any known pattern for scheme: {}", url))] UrlNotRecognised { url: String }, + + #[snafu(display("Configuration key: '{}' is not known.", key))] + UnknownConfigurationKey { key: String }, } impl From for super::Error { @@ -828,18 +830,27 @@ impl From for String { } } -static ALIAS_MAP: Lazy> = Lazy::new(|| { - BTreeMap::from([ - // service account - ("google_service_account", GoogleConfigKey::ServiceAccount), - ("service_account", GoogleConfigKey::ServiceAccount), - // bucket - ("google_bucket", GoogleConfigKey::Bucket), - ("google_bucket_name", GoogleConfigKey::Bucket), - ("bucket_name", GoogleConfigKey::Bucket), - ("bucket", GoogleConfigKey::Bucket), - ]) -}); +impl TryFrom<&str> for GoogleConfigKey { + type Error = super::Error; + + fn try_from(value: &str) -> Result { + match value.to_ascii_lowercase().as_str() { + "google_service_account" | "service_account" => Ok(Self::ServiceAccount), + "google_bucket" | "google_bucket_name" | "bucket" | "bucket_name" => { + Ok(Self::Bucket) + } + _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + } + } +} + +impl TryFrom for GoogleConfigKey { + type Error = super::Error; + + fn try_from(value: String) -> Result { + Self::try_from(value.as_str()) + } +} impl Default for GoogleCloudStorageBuilder { fn default() -> Self { @@ -880,12 +891,11 @@ impl GoogleCloudStorageBuilder { builder.service_account_path = Some(service_account_path); } - for (key, _) in ALIAS_MAP.iter() { - if key.starts_with("google_") { - if let Ok(value) = std::env::var(key.to_ascii_uppercase()) { - builder = builder.with_option(*key, value) - } - } + for (key, value) in std::env::vars() + .into_iter() + .filter(|(key, _)| key.starts_with("GOOGLE_")) + { + builder = builder.with_option(key, value); } builder @@ -918,15 +928,13 @@ impl GoogleCloudStorageBuilder { key: impl Into, value: impl Into, ) -> Self { - let raw = key.into(); - if let Some(key) = ALIAS_MAP.get(&*raw.to_ascii_lowercase()) { - match key { - GoogleConfigKey::ServiceAccount => { - self.service_account_path = Some(value.into()) - } - GoogleConfigKey::Bucket => self.bucket_name = Some(value.into()), - }; - } + match GoogleConfigKey::try_from(key.into()) { + Ok(GoogleConfigKey::ServiceAccount) => { + self.service_account_path = Some(value.into()) + } + Ok(GoogleConfigKey::Bucket) => self.bucket_name = Some(value.into()), + Err(_) => (), + }; self } From f7ebee1d46ef773d3cca42468df5dd6c63aea2e2 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 15:18:31 +0100 Subject: [PATCH 06/10] chore: PR feedback --- object_store/src/aws/mod.rs | 109 +++++++++++++++++++--------------- object_store/src/azure/mod.rs | 109 +++++++++++++++++++--------------- object_store/src/gcp/mod.rs | 90 +++++++++++++++------------- object_store/src/util.rs | 10 ++-- 4 files changed, 177 insertions(+), 141 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index e67e5d91fef2..fac57028b0f1 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -37,9 +37,11 @@ use chrono::{DateTime, Utc}; use futures::stream::BoxStream; use futures::TryStreamExt; use itertools::Itertools; +use serde::{Deserialize, Serialize}; use snafu::{OptionExt, ResultExt, Snafu}; -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; use std::ops::Range; +use std::str::FromStr; use std::sync::Arc; use tokio::io::AsyncWrite; use tracing::info; @@ -384,7 +386,28 @@ pub struct AmazonS3Builder { } /// Configuration keys for [`AmazonS3Builder`] -#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +/// +/// Configuration via keys can be dome via the [`with_option`](AmazonS3Builder::with_option) +/// or [`with_options`](AmazonS3Builder::with_options) methods on the builder. +/// +/// # Example +/// ``` +/// use std::collections::HashMap; +/// use object_store::aws::{AmazonS3Builder, AmazonS3ConfigKey}; +/// +/// let options = HashMap::from([ +/// ("aws_access_key_id", "my-access-key-id"), +/// ("aws_secret_access_key", "my-secret-access-key"), +/// ]); +/// let typed_options = vec![ +/// (AmazonS3ConfigKey::DefaultRegion, "my-default-region"), +/// ]; +/// let azure = AmazonS3Builder::new() +/// .with_options(options) +/// .with_options(typed_options) +/// .with_option(AmazonS3ConfigKey::Region, "my-region"); +/// ``` +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Serialize, Deserialize)] pub enum AmazonS3ConfigKey { /// AWS Access Key /// @@ -490,31 +513,31 @@ pub enum AmazonS3ConfigKey { Profile, } -impl From for String { - fn from(value: AmazonS3ConfigKey) -> Self { - match value { - AmazonS3ConfigKey::AccessKeyId => Self::from("aws_access_key_id"), - AmazonS3ConfigKey::SecretAccessKey => Self::from("aws_secret_access_key"), - AmazonS3ConfigKey::Region => Self::from("aws_region"), - AmazonS3ConfigKey::Bucket => Self::from("aws_bucket"), - AmazonS3ConfigKey::Endpoint => Self::from("aws_endpoint"), - AmazonS3ConfigKey::Token => Self::from("aws_session_token"), - AmazonS3ConfigKey::ImdsV1Fallback => Self::from("aws_imdsv1_fallback"), +impl AsRef for AmazonS3ConfigKey { + fn as_ref(&self) -> &str { + match self { + AmazonS3ConfigKey::AccessKeyId => "aws_access_key_id", + AmazonS3ConfigKey::SecretAccessKey => "aws_secret_access_key", + AmazonS3ConfigKey::Region => "aws_region", + AmazonS3ConfigKey::Bucket => "aws_bucket", + AmazonS3ConfigKey::Endpoint => "aws_endpoint", + AmazonS3ConfigKey::Token => "aws_session_token", + AmazonS3ConfigKey::ImdsV1Fallback => "aws_imdsv1_fallback", AmazonS3ConfigKey::VirtualHostedStyleRequest => { - Self::from("aws_virtual_hosted_style_request") + "aws_virtual_hosted_style_request" } - AmazonS3ConfigKey::DefaultRegion => Self::from("aws_default_region"), - AmazonS3ConfigKey::MetadataEndpoint => Self::from("aws_metadata_endpoint"), - AmazonS3ConfigKey::Profile => Self::from("aws_profile"), + AmazonS3ConfigKey::DefaultRegion => "aws_default_region", + AmazonS3ConfigKey::MetadataEndpoint => "aws_metadata_endpoint", + AmazonS3ConfigKey::Profile => "aws_profile", } } } -impl TryFrom<&str> for AmazonS3ConfigKey { - type Error = super::Error; +impl FromStr for AmazonS3ConfigKey { + type Err = super::Error; - fn try_from(value: &str) -> Result { - match value.to_ascii_lowercase().as_str() { + fn from_str(s: &str) -> Result { + match s { "aws_access_key_id" | "access_key_id" => Ok(Self::AccessKeyId), "aws_secret_access_key" | "secret_access_key" => Ok(Self::SecretAccessKey), "aws_default_region" | "default_region" => Ok(Self::DefaultRegion), @@ -534,19 +557,11 @@ impl TryFrom<&str> for AmazonS3ConfigKey { "aws_profile" | "profile" => Ok(Self::Profile), "aws_imdsv1_fallback" | "imdsv1_fallback" => Ok(Self::ImdsV1Fallback), "aws_metadata_endpoint" | "metadata_endpoint" => Ok(Self::MetadataEndpoint), - _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + _ => Err(Error::UnknownConfigurationKey { key: s.into() }.into()), } } } -impl TryFrom for AmazonS3ConfigKey { - type Error = super::Error; - - fn try_from(value: String) -> Result { - Self::try_from(value.as_str()) - } -} - impl AmazonS3Builder { /// Create a new [`AmazonS3Builder`] with default values. pub fn new() -> Self { @@ -575,11 +590,12 @@ impl AmazonS3Builder { pub fn from_env() -> Self { let mut builder: Self = Default::default(); - for (key, value) in std::env::vars() - .into_iter() - .filter(|(key, _)| key.starts_with("AWS_")) - { - builder = builder.with_option(key, value); + for (os_key, os_value) in std::env::vars_os().into_iter() { + if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { + if key.starts_with("AWS_") { + builder = builder.with_option(key.to_ascii_lowercase(), value); + } + } } // This env var is set in ECS @@ -624,12 +640,8 @@ impl AmazonS3Builder { } /// Set an option on the builder via a key - value pair. - pub fn with_option( - mut self, - key: impl Into, - value: impl Into, - ) -> Self { - match AmazonS3ConfigKey::try_from(key.into()) { + pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { + match AmazonS3ConfigKey::from_str(key.as_ref()) { Ok(AmazonS3ConfigKey::AccessKeyId) => self.access_key_id = Some(value.into()), Ok(AmazonS3ConfigKey::SecretAccessKey) => { self.secret_access_key = Some(value.into()) @@ -657,12 +669,12 @@ impl AmazonS3Builder { } /// Hydrate builder from key value pairs - pub fn with_options( + pub fn with_options, impl Into)>>( mut self, - options: &HashMap + Clone, String>, + options: I, ) -> Self { for (key, value) in options { - self = self.with_option(key.clone(), value); + self = self.with_option(key, value); } self } @@ -968,6 +980,7 @@ mod tests { put_get_delete_list_opts, rename_and_copy, stream_get, }; use bytes::Bytes; + use std::collections::HashMap; use std::env; const NON_EXISTENT_NAME: &str = "nonexistentname"; @@ -1118,11 +1131,11 @@ mod tests { let aws_endpoint = "object_store:fake_endpoint".to_string(); let aws_session_token = "object_store:fake_session_token".to_string(); let options = HashMap::from([ - ("aws_access_key_id".to_string(), aws_access_key_id.clone()), - ("aws_secret_access_key".to_string(), aws_secret_access_key), - ("aws_default_region".to_string(), aws_default_region.clone()), - ("aws_endpoint".to_string(), aws_endpoint.clone()), - ("aws_session_token".to_string(), aws_session_token.clone()), + ("aws_access_key_id", aws_access_key_id.clone()), + ("aws_secret_access_key", aws_secret_access_key), + ("aws_default_region", aws_default_region.clone()), + ("aws_endpoint", aws_endpoint.clone()), + ("aws_session_token", aws_session_token.clone()), ]); let builder = AmazonS3Builder::new() diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index db9f93174d24..ad49829d1e16 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -38,12 +38,13 @@ use bytes::Bytes; use chrono::{TimeZone, Utc}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; use percent_encoding::percent_decode_str; +use serde::{Deserialize, Serialize}; use snafu::{OptionExt, ResultExt, Snafu}; -use std::collections::{BTreeSet, HashMap}; use std::fmt::{Debug, Formatter}; use std::io; use std::ops::Range; use std::sync::Arc; +use std::{collections::BTreeSet, str::FromStr}; use tokio::io::AsyncWrite; use url::Url; @@ -386,7 +387,28 @@ pub struct MicrosoftAzureBuilder { } /// Configuration keys for [`MicrosoftAzureBuilder`] -#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +/// +/// Configuration via keys can be dome via the [`with_option`](MicrosoftAzureBuilder::with_option) +/// or [`with_options`](MicrosoftAzureBuilder::with_options) methods on the builder. +/// +/// # Example +/// ``` +/// use std::collections::HashMap; +/// use object_store::azure::{MicrosoftAzureBuilder, AzureConfigKey}; +/// +/// let options = HashMap::from([ +/// ("azure_client_id", "my-client-id"), +/// ("azure_client_secret", "my-account-name"), +/// ]); +/// let typed_options = vec![ +/// (AzureConfigKey::AccountName, "my-account-name"), +/// ]; +/// let azure = MicrosoftAzureBuilder::new() +/// .with_options(options) +/// .with_options(typed_options) +/// .with_option(AzureConfigKey::AuthorityId, "my-tenant-id"); +/// ``` +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Deserialize, Serialize)] pub enum AzureConfigKey { /// The name of the azure storage account /// @@ -436,7 +458,7 @@ pub enum AzureConfigKey { /// Shared access signature. /// /// The signature is expected to be percent-encoded, much like they are provided - /// in the azure storage explorer or azure protal. + /// in the azure storage explorer or azure portal. /// /// Supported keys: /// - `azure_storage_sas_key` @@ -462,26 +484,26 @@ pub enum AzureConfigKey { UseEmulator, } -impl From for String { - fn from(value: AzureConfigKey) -> Self { - match value { - AzureConfigKey::AccountName => Self::from("azure_storage_account_name"), - AzureConfigKey::AccessKey => Self::from("azure_storage_account_key"), - AzureConfigKey::ClientId => Self::from("azure_storage_client_id"), - AzureConfigKey::ClientSecret => Self::from("azure_storage_client_secret"), - AzureConfigKey::AuthorityId => Self::from("azure_storage_tenant_id"), - AzureConfigKey::SasKey => Self::from("azure_storage_sas_key"), - AzureConfigKey::Token => Self::from("azure_storage_token"), - AzureConfigKey::UseEmulator => Self::from("azure_storage_use_emulator"), +impl AsRef for AzureConfigKey { + fn as_ref(&self) -> &str { + match self { + AzureConfigKey::AccountName => "azure_storage_account_name", + AzureConfigKey::AccessKey => "azure_storage_account_key", + AzureConfigKey::ClientId => "azure_storage_client_id", + AzureConfigKey::ClientSecret => "azure_storage_client_secret", + AzureConfigKey::AuthorityId => "azure_storage_tenant_id", + AzureConfigKey::SasKey => "azure_storage_sas_key", + AzureConfigKey::Token => "azure_storage_token", + AzureConfigKey::UseEmulator => "azure_storage_use_emulator", } } } -impl TryFrom<&str> for AzureConfigKey { - type Error = super::Error; +impl FromStr for AzureConfigKey { + type Err = super::Error; - fn try_from(value: &str) -> Result { - match value.to_ascii_lowercase().as_str() { + fn from_str(s: &str) -> Result { + match s { "azure_storage_account_key" | "azure_storage_access_key" | "azure_storage_master_key" @@ -507,19 +529,11 @@ impl TryFrom<&str> for AzureConfigKey { | "sas_token" => Ok(Self::SasKey), "azure_storage_token" | "bearer_token" | "token" => Ok(Self::Token), "azure_storage_use_emulator" | "use_emulator" => Ok(Self::UseEmulator), - _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + _ => Err(Error::UnknownConfigurationKey { key: s.into() }.into()), } } } -impl TryFrom for AzureConfigKey { - type Error = super::Error; - - fn try_from(value: String) -> Result { - Self::try_from(value.as_str()) - } -} - impl Debug for MicrosoftAzureBuilder { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( @@ -555,12 +569,19 @@ impl MicrosoftAzureBuilder { /// ``` pub fn from_env() -> Self { let mut builder = Self::default(); - for (key, value) in std::env::vars() - .into_iter() - .filter(|(key, _)| key.starts_with("AZURE_")) - { - builder = builder.with_option(key, value); + for (os_key, os_value) in std::env::vars_os().into_iter() { + if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { + if key.starts_with("AZURE_") { + builder = builder.with_option(key.to_ascii_lowercase(), value); + } + } + } + + if let Ok(text) = std::env::var("AZURE_ALLOW_HTTP") { + builder.client_options = + builder.client_options.with_allow_http(str_is_truthy(&text)); } + builder } @@ -592,12 +613,8 @@ impl MicrosoftAzureBuilder { } /// Set an option on the builder via a key - value pair. - pub fn with_option( - mut self, - key: impl Into, - value: impl Into, - ) -> Self { - match AzureConfigKey::try_from(key.into()) { + pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { + match AzureConfigKey::from_str(key.as_ref()) { Ok(AzureConfigKey::AccessKey) => self.access_key = Some(value.into()), Ok(AzureConfigKey::AccountName) => self.account_name = Some(value.into()), Ok(AzureConfigKey::ClientId) => self.client_id = Some(value.into()), @@ -614,12 +631,12 @@ impl MicrosoftAzureBuilder { } /// Hydrate builder from key value pairs - pub fn with_options( + pub fn with_options, impl Into)>>( mut self, - options: &HashMap + Clone, String>, + options: I, ) -> Self { for (key, value) in options { - self = self.with_option(key.clone(), value); + self = self.with_option(key, value); } self } @@ -863,6 +880,7 @@ mod tests { copy_if_not_exists, list_uses_directories_correctly, list_with_delimiter, put_get_delete_list, put_get_delete_list_opts, rename_and_copy, stream_get, }; + use std::collections::HashMap; use std::env; // Helper macro to skip tests if TEST_INTEGRATION and the Azure environment @@ -1022,15 +1040,12 @@ mod tests { let azure_storage_account_name = "object_store:fake_secret_key".to_string(); let azure_storage_token = "object_store:fake_default_region".to_string(); let options = HashMap::from([ - ("azure_client_id".to_string(), azure_client_id.clone()), + ("azure_client_id", azure_client_id.clone()), ( - "azure_storage_account_name".to_string(), + "azure_storage_account_name", azure_storage_account_name.clone(), ), - ( - "azure_storage_token".to_string(), - azure_storage_token.clone(), - ), + ("azure_storage_token", azure_storage_token.clone()), ]); let builder = MicrosoftAzureBuilder::new() diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 06b5ce4c10bb..799889ba15a3 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -29,10 +29,11 @@ //! to abort the upload and drop those unneeded parts. In addition, you may wish to //! consider implementing automatic clean up of unused parts that are older than one //! week. -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; use std::fs::File; use std::io::{self, BufReader}; use std::ops::Range; +use std::str::FromStr; use std::sync::Arc; use async_trait::async_trait; @@ -42,6 +43,7 @@ use futures::{stream::BoxStream, StreamExt, TryStreamExt}; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use reqwest::header::RANGE; use reqwest::{header, Client, Method, Response, StatusCode}; +use serde::{Deserialize, Serialize}; use snafu::{OptionExt, ResultExt, Snafu}; use tokio::io::AsyncWrite; use url::Url; @@ -800,7 +802,27 @@ pub struct GoogleCloudStorageBuilder { } /// Configuration keys for [`GoogleCloudStorageBuilder`] -#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +/// +/// Configuration via keys can be dome via the [`with_option`](GoogleCloudStorageBuilder::with_option) +/// or [`with_options`](GoogleCloudStorageBuilder::with_options) methods on the builder. +/// +/// # Example +/// ``` +/// use std::collections::HashMap; +/// use object_store::gcp::{GoogleCloudStorageBuilder, GoogleConfigKey}; +/// +/// let options = HashMap::from([ +/// ("google_service_account", "my-service-account"), +/// ]); +/// let typed_options = vec![ +/// (GoogleConfigKey::Bucket, "my-bucket"), +/// ]; +/// let azure = GoogleCloudStorageBuilder::new() +/// .with_options(options) +/// .with_options(typed_options) +/// .with_option(GoogleConfigKey::Bucket, "my-new-bucket"); +/// ``` +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Serialize, Deserialize)] pub enum GoogleConfigKey { /// Path to the service account file /// @@ -821,37 +843,29 @@ pub enum GoogleConfigKey { Bucket, } -impl From for String { - fn from(value: GoogleConfigKey) -> Self { - match value { - GoogleConfigKey::ServiceAccount => Self::from("google_service_account"), - GoogleConfigKey::Bucket => Self::from("google_bucket"), +impl AsRef for GoogleConfigKey { + fn as_ref(&self) -> &str { + match self { + GoogleConfigKey::ServiceAccount => "google_service_account", + GoogleConfigKey::Bucket => "google_bucket", } } } -impl TryFrom<&str> for GoogleConfigKey { - type Error = super::Error; +impl FromStr for GoogleConfigKey { + type Err = super::Error; - fn try_from(value: &str) -> Result { - match value.to_ascii_lowercase().as_str() { + fn from_str(s: &str) -> Result { + match s { "google_service_account" | "service_account" => Ok(Self::ServiceAccount), "google_bucket" | "google_bucket_name" | "bucket" | "bucket_name" => { Ok(Self::Bucket) } - _ => Err(Error::UnknownConfigurationKey { key: value.into() }.into()), + _ => Err(Error::UnknownConfigurationKey { key: s.into() }.into()), } } } -impl TryFrom for GoogleConfigKey { - type Error = super::Error; - - fn try_from(value: String) -> Result { - Self::try_from(value.as_str()) - } -} - impl Default for GoogleCloudStorageBuilder { fn default() -> Self { Self { @@ -891,11 +905,12 @@ impl GoogleCloudStorageBuilder { builder.service_account_path = Some(service_account_path); } - for (key, value) in std::env::vars() - .into_iter() - .filter(|(key, _)| key.starts_with("GOOGLE_")) - { - builder = builder.with_option(key, value); + for (os_key, os_value) in std::env::vars_os().into_iter() { + if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { + if key.starts_with("GOOGLE_") { + builder = builder.with_option(key.to_ascii_lowercase(), value); + } + } } builder @@ -923,12 +938,8 @@ impl GoogleCloudStorageBuilder { } /// Set an option on the builder via a key - value pair. - pub fn with_option( - mut self, - key: impl Into, - value: impl Into, - ) -> Self { - match GoogleConfigKey::try_from(key.into()) { + pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { + match GoogleConfigKey::from_str(key.as_ref()) { Ok(GoogleConfigKey::ServiceAccount) => { self.service_account_path = Some(value.into()) } @@ -939,12 +950,12 @@ impl GoogleCloudStorageBuilder { } /// Hydrate builder from key value pairs - pub fn with_options( + pub fn with_options, impl Into)>>( mut self, - options: &HashMap + Clone, String>, + options: I, ) -> Self { for (key, value) in options { - self = self.with_option(key.clone(), value); + self = self.with_option(key, value); } self } @@ -1081,9 +1092,9 @@ fn convert_object_meta(object: &Object) -> Result { #[cfg(test)] mod test { - use std::env; - use bytes::Bytes; + use std::collections::HashMap; + use std::env; use crate::{ tests::{ @@ -1297,11 +1308,8 @@ mod test { let google_service_account = "object_store:fake_service_account".to_string(); let google_bucket_name = "object_store:fake_bucket".to_string(); let options = HashMap::from([ - ( - "google_service_account".to_string(), - google_service_account.clone(), - ), - ("google_bucket_name".to_string(), google_bucket_name.clone()), + ("google_service_account", google_service_account.clone()), + ("google_bucket_name", google_bucket_name.clone()), ]); let builder = GoogleCloudStorageBuilder::new().with_options(&options); diff --git a/object_store/src/util.rs b/object_store/src/util.rs index 0e4e740e4314..08bfd86d9f67 100644 --- a/object_store/src/util.rs +++ b/object_store/src/util.rs @@ -187,11 +187,11 @@ fn merge_ranges( #[allow(dead_code)] pub(crate) fn str_is_truthy(val: &str) -> bool { - val == "1" - || val.to_lowercase() == "true" - || val.to_lowercase() == "on" - || val.to_lowercase() == "yes" - || val.to_lowercase() == "y" + val.eq_ignore_ascii_case("1") + | val.eq_ignore_ascii_case("true") + | val.eq_ignore_ascii_case("on") + | val.eq_ignore_ascii_case("yes") + | val.eq_ignore_ascii_case("y") } #[cfg(test)] From a9cf97f1292fb74d99c539946faa9edc9dfcae9a Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 16:10:51 +0100 Subject: [PATCH 07/10] refactor: make options api fallible --- object_store/src/aws/mod.rs | 133 +++++++++++++++++++++++----------- object_store/src/azure/mod.rs | 110 +++++++++++++++++++--------- object_store/src/gcp/mod.rs | 77 ++++++++++++++++---- object_store/src/lib.rs | 7 ++ 4 files changed, 236 insertions(+), 91 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index fac57028b0f1..53ebccb647e9 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -142,10 +142,15 @@ enum Error { } impl From for super::Error { - fn from(err: Error) -> Self { - Self::Generic { - store: "S3", - source: Box::new(err), + fn from(source: Error) -> Self { + match source { + Error::UnknownConfigurationKey { key } => { + Self::UnknownConfigurationKey { store: "S3", key } + } + _ => Self::Generic { + store: "S3", + source: Box::new(source), + }, } } } @@ -513,22 +518,33 @@ pub enum AmazonS3ConfigKey { Profile, } +impl AmazonS3ConfigKey { + /// Helper function to filter an iterator to only contain valid configuration keys. + pub fn filter_options< + I: IntoIterator, impl Into)>, + >( + options: I, + ) -> impl IntoIterator, impl Into)> { + options + .into_iter() + .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) + } +} + impl AsRef for AmazonS3ConfigKey { fn as_ref(&self) -> &str { match self { - AmazonS3ConfigKey::AccessKeyId => "aws_access_key_id", - AmazonS3ConfigKey::SecretAccessKey => "aws_secret_access_key", - AmazonS3ConfigKey::Region => "aws_region", - AmazonS3ConfigKey::Bucket => "aws_bucket", - AmazonS3ConfigKey::Endpoint => "aws_endpoint", - AmazonS3ConfigKey::Token => "aws_session_token", - AmazonS3ConfigKey::ImdsV1Fallback => "aws_imdsv1_fallback", - AmazonS3ConfigKey::VirtualHostedStyleRequest => { - "aws_virtual_hosted_style_request" - } - AmazonS3ConfigKey::DefaultRegion => "aws_default_region", - AmazonS3ConfigKey::MetadataEndpoint => "aws_metadata_endpoint", - AmazonS3ConfigKey::Profile => "aws_profile", + Self::AccessKeyId => "aws_access_key_id", + Self::SecretAccessKey => "aws_secret_access_key", + Self::Region => "aws_region", + Self::Bucket => "aws_bucket", + Self::Endpoint => "aws_endpoint", + Self::Token => "aws_session_token", + Self::ImdsV1Fallback => "aws_imdsv1_fallback", + Self::VirtualHostedStyleRequest => "aws_virtual_hosted_style_request", + Self::DefaultRegion => "aws_default_region", + Self::MetadataEndpoint => "aws_metadata_endpoint", + Self::Profile => "aws_profile", } } } @@ -590,10 +606,14 @@ impl AmazonS3Builder { pub fn from_env() -> Self { let mut builder: Self = Default::default(); - for (os_key, os_value) in std::env::vars_os().into_iter() { + for (os_key, os_value) in std::env::vars_os() { if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { if key.starts_with("AWS_") { - builder = builder.with_option(key.to_ascii_lowercase(), value); + if let Ok(config_key) = + AmazonS3ConfigKey::from_str(&key.to_ascii_lowercase()) + { + builder = builder.try_with_option(config_key, value).unwrap(); + } } } } @@ -640,43 +660,52 @@ impl AmazonS3Builder { } /// Set an option on the builder via a key - value pair. - pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { - match AmazonS3ConfigKey::from_str(key.as_ref()) { - Ok(AmazonS3ConfigKey::AccessKeyId) => self.access_key_id = Some(value.into()), - Ok(AmazonS3ConfigKey::SecretAccessKey) => { + /// + /// This method will return an `UnknownConfigKey` error if key cannot be parsed into [`AmazonS3ConfigKey`]. + pub fn try_with_option( + mut self, + key: impl AsRef, + value: impl Into, + ) -> Result { + match AmazonS3ConfigKey::from_str(key.as_ref())? { + AmazonS3ConfigKey::AccessKeyId => self.access_key_id = Some(value.into()), + AmazonS3ConfigKey::SecretAccessKey => { self.secret_access_key = Some(value.into()) } - Ok(AmazonS3ConfigKey::Region) => self.region = Some(value.into()), - Ok(AmazonS3ConfigKey::Bucket) => self.bucket_name = Some(value.into()), - Ok(AmazonS3ConfigKey::Endpoint) => self.endpoint = Some(value.into()), - Ok(AmazonS3ConfigKey::Token) => self.token = Some(value.into()), - Ok(AmazonS3ConfigKey::ImdsV1Fallback) => { + AmazonS3ConfigKey::Region => self.region = Some(value.into()), + AmazonS3ConfigKey::Bucket => self.bucket_name = Some(value.into()), + AmazonS3ConfigKey::Endpoint => self.endpoint = Some(value.into()), + AmazonS3ConfigKey::Token => self.token = Some(value.into()), + AmazonS3ConfigKey::ImdsV1Fallback => { self.imdsv1_fallback = str_is_truthy(&value.into()) } - Ok(AmazonS3ConfigKey::VirtualHostedStyleRequest) => { + AmazonS3ConfigKey::VirtualHostedStyleRequest => { self.virtual_hosted_style_request = str_is_truthy(&value.into()) } - Ok(AmazonS3ConfigKey::DefaultRegion) => { + AmazonS3ConfigKey::DefaultRegion => { self.region = self.region.or_else(|| Some(value.into())) } - Ok(AmazonS3ConfigKey::MetadataEndpoint) => { + AmazonS3ConfigKey::MetadataEndpoint => { self.metadata_endpoint = Some(value.into()) } - Ok(AmazonS3ConfigKey::Profile) => self.profile = Some(value.into()), - Err(_) => (), + AmazonS3ConfigKey::Profile => self.profile = Some(value.into()), }; - self + Ok(self) } /// Hydrate builder from key value pairs - pub fn with_options, impl Into)>>( + /// + /// This method will return an `UnknownConfigKey` error if any key cannot be parsed into [`AmazonS3ConfigKey`]. + pub fn try_with_options< + I: IntoIterator, impl Into)>, + >( mut self, options: I, - ) -> Self { + ) -> Result { for (key, value) in options { - self = self.with_option(key, value); + self = self.try_with_option(key, value)?; } - self + Ok(self) } /// Sets properties on this builder based on a URL @@ -1139,8 +1168,10 @@ mod tests { ]); let builder = AmazonS3Builder::new() - .with_options(&options) - .with_option("aws_secret_access_key", "new-secret-key"); + .try_with_options(&options) + .unwrap() + .try_with_option("aws_secret_access_key", "new-secret-key") + .unwrap(); assert_eq!(builder.access_key_id.unwrap(), aws_access_key_id.as_str()); assert_eq!(builder.secret_access_key.unwrap(), "new-secret-key"); assert_eq!(builder.region.unwrap(), aws_default_region); @@ -1164,8 +1195,10 @@ mod tests { ]); let builder = AmazonS3Builder::new() - .with_options(&options) - .with_option(AmazonS3ConfigKey::SecretAccessKey, "new-secret-key"); + .try_with_options(&options) + .unwrap() + .try_with_option(AmazonS3ConfigKey::SecretAccessKey, "new-secret-key") + .unwrap(); assert_eq!(builder.access_key_id.unwrap(), aws_access_key_id.as_str()); assert_eq!(builder.secret_access_key.unwrap(), "new-secret-key"); assert_eq!(builder.region.unwrap(), aws_default_region); @@ -1173,6 +1206,22 @@ mod tests { assert_eq!(builder.token.unwrap(), aws_session_token); } + #[test] + fn s3_test_config_fallible_options() { + let aws_access_key_id = "object_store:fake_access_key_id".to_string(); + let aws_secret_access_key = "object_store:fake_secret_key".to_string(); + let options = HashMap::from([ + ("aws_access_key_id", aws_access_key_id), + ("invalid-key", aws_secret_access_key), + ]); + + let builder = AmazonS3Builder::new().try_with_options(&options); + assert!(builder.is_err()); + let builder = AmazonS3Builder::new() + .try_with_options(AmazonS3ConfigKey::filter_options(&options)); + assert!(builder.is_ok()); + } + #[tokio::test] async fn s3_test() { let config = maybe_skip_integration!(); diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index ad49829d1e16..092b75ca0827 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -139,9 +139,15 @@ enum Error { impl From for super::Error { fn from(source: Error) -> Self { - Self::Generic { - store: "MicrosoftAzure", - source: Box::new(source), + match source { + Error::UnknownConfigurationKey { key } => Self::UnknownConfigurationKey { + store: "MicrosoftAzure", + key, + }, + _ => Self::Generic { + store: "MicrosoftAzure", + source: Box::new(source), + }, } } } @@ -484,17 +490,30 @@ pub enum AzureConfigKey { UseEmulator, } +impl AzureConfigKey { + /// Helper function to filter an iterator to only contain valid configuration keys. + pub fn filter_options< + I: IntoIterator, impl Into)>, + >( + options: I, + ) -> impl IntoIterator, impl Into)> { + options + .into_iter() + .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) + } +} + impl AsRef for AzureConfigKey { fn as_ref(&self) -> &str { match self { - AzureConfigKey::AccountName => "azure_storage_account_name", - AzureConfigKey::AccessKey => "azure_storage_account_key", - AzureConfigKey::ClientId => "azure_storage_client_id", - AzureConfigKey::ClientSecret => "azure_storage_client_secret", - AzureConfigKey::AuthorityId => "azure_storage_tenant_id", - AzureConfigKey::SasKey => "azure_storage_sas_key", - AzureConfigKey::Token => "azure_storage_token", - AzureConfigKey::UseEmulator => "azure_storage_use_emulator", + Self::AccountName => "azure_storage_account_name", + Self::AccessKey => "azure_storage_account_key", + Self::ClientId => "azure_storage_client_id", + Self::ClientSecret => "azure_storage_client_secret", + Self::AuthorityId => "azure_storage_tenant_id", + Self::SasKey => "azure_storage_sas_key", + Self::Token => "azure_storage_token", + Self::UseEmulator => "azure_storage_use_emulator", } } } @@ -569,10 +588,14 @@ impl MicrosoftAzureBuilder { /// ``` pub fn from_env() -> Self { let mut builder = Self::default(); - for (os_key, os_value) in std::env::vars_os().into_iter() { + for (os_key, os_value) in std::env::vars_os() { if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { if key.starts_with("AZURE_") { - builder = builder.with_option(key.to_ascii_lowercase(), value); + if let Ok(config_key) = + AzureConfigKey::from_str(&key.to_ascii_lowercase()) + { + builder = builder.try_with_option(config_key, value).unwrap(); + } } } } @@ -613,32 +636,37 @@ impl MicrosoftAzureBuilder { } /// Set an option on the builder via a key - value pair. - pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { - match AzureConfigKey::from_str(key.as_ref()) { - Ok(AzureConfigKey::AccessKey) => self.access_key = Some(value.into()), - Ok(AzureConfigKey::AccountName) => self.account_name = Some(value.into()), - Ok(AzureConfigKey::ClientId) => self.client_id = Some(value.into()), - Ok(AzureConfigKey::ClientSecret) => self.client_secret = Some(value.into()), - Ok(AzureConfigKey::AuthorityId) => self.tenant_id = Some(value.into()), - Ok(AzureConfigKey::SasKey) => self.sas_key = Some(value.into()), - Ok(AzureConfigKey::Token) => self.bearer_token = Some(value.into()), - Ok(AzureConfigKey::UseEmulator) => { + pub fn try_with_option( + mut self, + key: impl AsRef, + value: impl Into, + ) -> Result { + match AzureConfigKey::from_str(key.as_ref())? { + AzureConfigKey::AccessKey => self.access_key = Some(value.into()), + AzureConfigKey::AccountName => self.account_name = Some(value.into()), + AzureConfigKey::ClientId => self.client_id = Some(value.into()), + AzureConfigKey::ClientSecret => self.client_secret = Some(value.into()), + AzureConfigKey::AuthorityId => self.tenant_id = Some(value.into()), + AzureConfigKey::SasKey => self.sas_key = Some(value.into()), + AzureConfigKey::Token => self.bearer_token = Some(value.into()), + AzureConfigKey::UseEmulator => { self.use_emulator = str_is_truthy(&value.into()) } - Err(_) => (), }; - self + Ok(self) } /// Hydrate builder from key value pairs - pub fn with_options, impl Into)>>( + pub fn try_with_options< + I: IntoIterator, impl Into)>, + >( mut self, options: I, - ) -> Self { + ) -> Result { for (key, value) in options { - self = self.with_option(key, value); + self = self.try_with_option(key, value)?; } - self + Ok(self) } /// Sets properties on this builder based on a URL @@ -1049,8 +1077,8 @@ mod tests { ]); let builder = MicrosoftAzureBuilder::new() - .with_options(&options) - .with_option("unknown-key", "unknown-value"); + .try_with_options(&options) + .unwrap(); assert_eq!(builder.client_id.unwrap(), azure_client_id); assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); assert_eq!(builder.bearer_token.unwrap(), azure_storage_token); @@ -1071,10 +1099,26 @@ mod tests { ]); let builder = MicrosoftAzureBuilder::new() - .with_options(&options) - .with_option("unknown-key", "unknown-value"); + .try_with_options(&options) + .unwrap(); assert_eq!(builder.client_id.unwrap(), azure_client_id); assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); assert_eq!(builder.bearer_token.unwrap(), azure_storage_token); } + + #[test] + fn azure_test_config_fallible_options() { + let azure_client_id = "object_store:fake_access_key_id".to_string(); + let azure_storage_token = "object_store:fake_default_region".to_string(); + let options = HashMap::from([ + ("azure_client_id", azure_client_id), + ("invalid-key", azure_storage_token), + ]); + + let builder = MicrosoftAzureBuilder::new().try_with_options(&options); + assert!(builder.is_err()); + let builder = MicrosoftAzureBuilder::new() + .try_with_options(AzureConfigKey::filter_options(&options)); + assert!(builder.is_ok()); + } } diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 799889ba15a3..4ab8ab597d5b 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -169,6 +169,9 @@ impl From for super::Error { source: Box::new(source), path, }, + Error::UnknownConfigurationKey { key } => { + Self::UnknownConfigurationKey { store: "GCS", key } + } _ => Self::Generic { store: "GCS", source: Box::new(err), @@ -843,11 +846,24 @@ pub enum GoogleConfigKey { Bucket, } +impl GoogleConfigKey { + /// Helper function to filter an iterator to only contain valid configuration keys. + pub fn filter_options< + I: IntoIterator, impl Into)>, + >( + options: I, + ) -> impl IntoIterator, impl Into)> { + options + .into_iter() + .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) + } +} + impl AsRef for GoogleConfigKey { fn as_ref(&self) -> &str { match self { - GoogleConfigKey::ServiceAccount => "google_service_account", - GoogleConfigKey::Bucket => "google_bucket", + Self::ServiceAccount => "google_service_account", + Self::Bucket => "google_bucket", } } } @@ -905,10 +921,14 @@ impl GoogleCloudStorageBuilder { builder.service_account_path = Some(service_account_path); } - for (os_key, os_value) in std::env::vars_os().into_iter() { + for (os_key, os_value) in std::env::vars_os() { if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) { if key.starts_with("GOOGLE_") { - builder = builder.with_option(key.to_ascii_lowercase(), value); + if let Ok(config_key) = + GoogleConfigKey::from_str(&key.to_ascii_lowercase()) + { + builder = builder.try_with_option(config_key, value).unwrap(); + } } } } @@ -938,26 +958,31 @@ impl GoogleCloudStorageBuilder { } /// Set an option on the builder via a key - value pair. - pub fn with_option(mut self, key: impl AsRef, value: impl Into) -> Self { - match GoogleConfigKey::from_str(key.as_ref()) { - Ok(GoogleConfigKey::ServiceAccount) => { + pub fn try_with_option( + mut self, + key: impl AsRef, + value: impl Into, + ) -> Result { + match GoogleConfigKey::from_str(key.as_ref())? { + GoogleConfigKey::ServiceAccount => { self.service_account_path = Some(value.into()) } - Ok(GoogleConfigKey::Bucket) => self.bucket_name = Some(value.into()), - Err(_) => (), + GoogleConfigKey::Bucket => self.bucket_name = Some(value.into()), }; - self + Ok(self) } /// Hydrate builder from key value pairs - pub fn with_options, impl Into)>>( + pub fn try_with_options< + I: IntoIterator, impl Into)>, + >( mut self, options: I, - ) -> Self { + ) -> Result { for (key, value) in options { - self = self.with_option(key, value); + self = self.try_with_option(key, value)?; } - self + Ok(self) } /// Sets properties on this builder based on a URL @@ -1312,7 +1337,9 @@ mod test { ("google_bucket_name", google_bucket_name.clone()), ]); - let builder = GoogleCloudStorageBuilder::new().with_options(&options); + let builder = GoogleCloudStorageBuilder::new() + .try_with_options(&options) + .unwrap(); assert_eq!( builder.service_account_path.unwrap(), google_service_account.as_str() @@ -1332,11 +1359,29 @@ mod test { (GoogleConfigKey::Bucket, google_bucket_name.clone()), ]); - let builder = GoogleCloudStorageBuilder::new().with_options(&options); + let builder = GoogleCloudStorageBuilder::new() + .try_with_options(&options) + .unwrap(); assert_eq!( builder.service_account_path.unwrap(), google_service_account.as_str() ); assert_eq!(builder.bucket_name.unwrap(), google_bucket_name.as_str()); } + + #[test] + fn gcs_test_config_fallible_options() { + let google_service_account = "object_store:fake_service_account".to_string(); + let google_bucket_name = "object_store:fake_bucket".to_string(); + let options = HashMap::from([ + ("google_service_account", google_service_account), + ("invalid-key", google_bucket_name), + ]); + + let builder = GoogleCloudStorageBuilder::new().try_with_options(&options); + assert!(builder.is_err()); + let builder = GoogleCloudStorageBuilder::new() + .try_with_options(GoogleConfigKey::filter_options(&options)); + assert!(builder.is_ok()); + } } diff --git a/object_store/src/lib.rs b/object_store/src/lib.rs index 425c5cdba1d1..4ec58c387e49 100644 --- a/object_store/src/lib.rs +++ b/object_store/src/lib.rs @@ -555,6 +555,13 @@ pub enum Error { #[snafu(display("Operation not yet implemented."))] NotImplemented, + + #[snafu(display( + "Configuration key: '{}' is not valid for store '{}'.", + key, + store + ))] + UnknownConfigurationKey { store: &'static str, key: String }, } impl From for std::io::Error { From 786f4698be3946b694c89bc2c604080e5efd9493 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 16:25:22 +0100 Subject: [PATCH 08/10] fix: docs errors --- object_store/src/aws/mod.rs | 13 ++++++++----- object_store/src/azure/mod.rs | 13 ++++++++----- object_store/src/gcp/mod.rs | 13 ++++++++----- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index 53ebccb647e9..fa9cfd8f2e1b 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -392,8 +392,8 @@ pub struct AmazonS3Builder { /// Configuration keys for [`AmazonS3Builder`] /// -/// Configuration via keys can be dome via the [`with_option`](AmazonS3Builder::with_option) -/// or [`with_options`](AmazonS3Builder::with_options) methods on the builder. +/// Configuration via keys can be dome via the [`try_with_option`](AmazonS3Builder::try_with_option) +/// or [`with_options`](AmazonS3Builder::try_with_options) methods on the builder. /// /// # Example /// ``` @@ -408,9 +408,12 @@ pub struct AmazonS3Builder { /// (AmazonS3ConfigKey::DefaultRegion, "my-default-region"), /// ]; /// let azure = AmazonS3Builder::new() -/// .with_options(options) -/// .with_options(typed_options) -/// .with_option(AmazonS3ConfigKey::Region, "my-region"); +/// .try_with_options(options) +/// .unwrap() +/// .try_with_options(typed_options) +/// .unwrap() +/// .try_with_option(AmazonS3ConfigKey::Region, "my-region") +/// .unwrap(); /// ``` #[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Serialize, Deserialize)] pub enum AmazonS3ConfigKey { diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index 092b75ca0827..b777bacfecf3 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -394,8 +394,8 @@ pub struct MicrosoftAzureBuilder { /// Configuration keys for [`MicrosoftAzureBuilder`] /// -/// Configuration via keys can be dome via the [`with_option`](MicrosoftAzureBuilder::with_option) -/// or [`with_options`](MicrosoftAzureBuilder::with_options) methods on the builder. +/// Configuration via keys can be dome via the [`try_with_option`](MicrosoftAzureBuilder::try_with_option) +/// or [`with_options`](MicrosoftAzureBuilder::try_with_options) methods on the builder. /// /// # Example /// ``` @@ -410,9 +410,12 @@ pub struct MicrosoftAzureBuilder { /// (AzureConfigKey::AccountName, "my-account-name"), /// ]; /// let azure = MicrosoftAzureBuilder::new() -/// .with_options(options) -/// .with_options(typed_options) -/// .with_option(AzureConfigKey::AuthorityId, "my-tenant-id"); +/// .try_with_options(options) +/// .unwrap() +/// .try_with_options(typed_options) +/// .unwrap() +/// .try_with_option(AzureConfigKey::AuthorityId, "my-tenant-id") +/// .unwrap(); /// ``` #[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Deserialize, Serialize)] pub enum AzureConfigKey { diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 4ab8ab597d5b..28754b7e01d1 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -806,8 +806,8 @@ pub struct GoogleCloudStorageBuilder { /// Configuration keys for [`GoogleCloudStorageBuilder`] /// -/// Configuration via keys can be dome via the [`with_option`](GoogleCloudStorageBuilder::with_option) -/// or [`with_options`](GoogleCloudStorageBuilder::with_options) methods on the builder. +/// Configuration via keys can be dome via the [`try_with_option`](GoogleCloudStorageBuilder::try_with_option) +/// or [`with_options`](GoogleCloudStorageBuilder::try_with_options) methods on the builder. /// /// # Example /// ``` @@ -821,9 +821,12 @@ pub struct GoogleCloudStorageBuilder { /// (GoogleConfigKey::Bucket, "my-bucket"), /// ]; /// let azure = GoogleCloudStorageBuilder::new() -/// .with_options(options) -/// .with_options(typed_options) -/// .with_option(GoogleConfigKey::Bucket, "my-new-bucket"); +/// .try_with_options(options) +/// .unwrap() +/// .try_with_options(typed_options) +/// .unwrap() +/// .try_with_option(GoogleConfigKey::Bucket, "my-new-bucket") +/// .unwrap(); /// ``` #[derive(PartialEq, Eq, Hash, Clone, Debug, Copy, Serialize, Deserialize)] pub enum GoogleConfigKey { From 15668bdb91910ab41ab0ad6876c70ca74d37db70 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 17:13:58 +0100 Subject: [PATCH 09/10] chore: remove helpers --- object_store/src/aws/mod.rs | 16 ---------------- object_store/src/azure/mod.rs | 16 ---------------- object_store/src/gcp/mod.rs | 16 ---------------- 3 files changed, 48 deletions(-) diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index fa9cfd8f2e1b..4b633d9f5d24 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -521,19 +521,6 @@ pub enum AmazonS3ConfigKey { Profile, } -impl AmazonS3ConfigKey { - /// Helper function to filter an iterator to only contain valid configuration keys. - pub fn filter_options< - I: IntoIterator, impl Into)>, - >( - options: I, - ) -> impl IntoIterator, impl Into)> { - options - .into_iter() - .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) - } -} - impl AsRef for AmazonS3ConfigKey { fn as_ref(&self) -> &str { match self { @@ -1220,9 +1207,6 @@ mod tests { let builder = AmazonS3Builder::new().try_with_options(&options); assert!(builder.is_err()); - let builder = AmazonS3Builder::new() - .try_with_options(AmazonS3ConfigKey::filter_options(&options)); - assert!(builder.is_ok()); } #[tokio::test] diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index b777bacfecf3..3e519dec304a 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -493,19 +493,6 @@ pub enum AzureConfigKey { UseEmulator, } -impl AzureConfigKey { - /// Helper function to filter an iterator to only contain valid configuration keys. - pub fn filter_options< - I: IntoIterator, impl Into)>, - >( - options: I, - ) -> impl IntoIterator, impl Into)> { - options - .into_iter() - .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) - } -} - impl AsRef for AzureConfigKey { fn as_ref(&self) -> &str { match self { @@ -1120,8 +1107,5 @@ mod tests { let builder = MicrosoftAzureBuilder::new().try_with_options(&options); assert!(builder.is_err()); - let builder = MicrosoftAzureBuilder::new() - .try_with_options(AzureConfigKey::filter_options(&options)); - assert!(builder.is_ok()); } } diff --git a/object_store/src/gcp/mod.rs b/object_store/src/gcp/mod.rs index 28754b7e01d1..177812fa8930 100644 --- a/object_store/src/gcp/mod.rs +++ b/object_store/src/gcp/mod.rs @@ -849,19 +849,6 @@ pub enum GoogleConfigKey { Bucket, } -impl GoogleConfigKey { - /// Helper function to filter an iterator to only contain valid configuration keys. - pub fn filter_options< - I: IntoIterator, impl Into)>, - >( - options: I, - ) -> impl IntoIterator, impl Into)> { - options - .into_iter() - .filter_map(|(key, value)| Some((Self::from_str(key.as_ref()).ok()?, value))) - } -} - impl AsRef for GoogleConfigKey { fn as_ref(&self) -> &str { match self { @@ -1383,8 +1370,5 @@ mod test { let builder = GoogleCloudStorageBuilder::new().try_with_options(&options); assert!(builder.is_err()); - let builder = GoogleCloudStorageBuilder::new() - .try_with_options(GoogleConfigKey::filter_options(&options)); - assert!(builder.is_ok()); } } From 7dea6c8ee68dbda781ee613e537f2a65891ed322 Mon Sep 17 00:00:00 2001 From: Robert Pack Date: Wed, 4 Jan 2023 19:22:14 +0100 Subject: [PATCH 10/10] test: test sas key splitting and un-nit nits --- object_store/src/azure/mod.rs | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/object_store/src/azure/mod.rs b/object_store/src/azure/mod.rs index 3e519dec304a..416883ac95a2 100644 --- a/object_store/src/azure/mod.rs +++ b/object_store/src/azure/mod.rs @@ -1054,20 +1054,17 @@ mod tests { #[test] fn azure_test_config_from_map() { - let azure_client_id = "object_store:fake_access_key_id".to_string(); - let azure_storage_account_name = "object_store:fake_secret_key".to_string(); - let azure_storage_token = "object_store:fake_default_region".to_string(); + let azure_client_id = "object_store:fake_access_key_id"; + let azure_storage_account_name = "object_store:fake_secret_key"; + let azure_storage_token = "object_store:fake_default_region"; let options = HashMap::from([ - ("azure_client_id", azure_client_id.clone()), - ( - "azure_storage_account_name", - azure_storage_account_name.clone(), - ), - ("azure_storage_token", azure_storage_token.clone()), + ("azure_client_id", azure_client_id), + ("azure_storage_account_name", azure_storage_account_name), + ("azure_storage_token", azure_storage_token), ]); let builder = MicrosoftAzureBuilder::new() - .try_with_options(&options) + .try_with_options(options) .unwrap(); assert_eq!(builder.client_id.unwrap(), azure_client_id); assert_eq!(builder.account_name.unwrap(), azure_storage_account_name); @@ -1108,4 +1105,22 @@ mod tests { let builder = MicrosoftAzureBuilder::new().try_with_options(&options); assert!(builder.is_err()); } + + #[test] + fn azure_test_split_sas() { + let raw_sas = "?sv=2021-10-04&st=2023-01-04T17%3A48%3A57Z&se=2023-01-04T18%3A15%3A00Z&sr=c&sp=rcwl&sig=C7%2BZeEOWbrxPA3R0Cw%2Fw1EZz0%2B4KBvQexeKZKe%2BB6h0%3D"; + let expected = vec![ + ("sv".to_string(), "2021-10-04".to_string()), + ("st".to_string(), "2023-01-04T17:48:57Z".to_string()), + ("se".to_string(), "2023-01-04T18:15:00Z".to_string()), + ("sr".to_string(), "c".to_string()), + ("sp".to_string(), "rcwl".to_string()), + ( + "sig".to_string(), + "C7+ZeEOWbrxPA3R0Cw/w1EZz0+4KBvQexeKZKe+B6h0=".to_string(), + ), + ]; + let pairs = split_sas(raw_sas).unwrap(); + assert_eq!(expected, pairs); + } }