Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object_store: builder configuration api #3436

Merged
merged 10 commits into from
Jan 4, 2023
Merged

object_store: builder configuration api #3436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0597445
feat: draf configuration api for azure
roeap Jan 3, 2023
360c9db
feat: add configuration for aws and gcp
roeap Jan 3, 2023
3ab6ac8
fix: clippy
roeap Jan 3, 2023
ac638a7
feat: allow passing typed config keys
roeap Jan 4, 2023
2f5e5cc
refactor: implement try_from for config keys
roeap Jan 4, 2023
f7ebee1
chore: PR feedback
roeap Jan 4, 2023
a9cf97f
refactor: make options api fallible
roeap Jan 4, 2023
786f469
fix: docs errors
roeap Jan 4, 2023
15668bd
chore: remove helpers
roeap Jan 4, 2023
7dea6c8
test: test sas key splitting and un-nit nits
roeap Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion object_store/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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"]
Expand Down
188 changes: 165 additions & 23 deletions object_store/src/azure/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Error> for super::Error {
Expand Down Expand Up @@ -367,13 +375,103 @@ pub struct MicrosoftAzureBuilder {
client_secret: Option<String>,
tenant_id: Option<String>,
sas_query_pairs: Option<Vec<(String, String)>>,
sas_key: Option<String>,
authority_host: Option<String>,
url: Option<String>,
use_emulator: bool,
retry_config: RetryConfig,
client_options: ClientOptions,
}

#[derive(PartialEq, Eq)]
enum AzureConfigKey {
/// The name of the azure storage account
///
/// Supported keys:
Copy link
Contributor

@winding-lines winding-lines Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I see now why a second layer is needed: you want to support multiple variants of the environment variables. This is ok with me, but supporting more than 2-3 variants for the same name will be confusing in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually one of my main concern. On one hand there simply is quite a few variants out there people migt be used to, on the other hand, having many options for the same thing is confusing.

/// - `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<HashMap<&'static str, AzureConfigKey>> = Lazy::new(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a HashMap noticeably faster than as simple match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not - main reason for a map is so we can iterate through it when reading from the env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, we got rid of the maps now :).

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!(
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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<String>,
value: impl Into<String>,
) -> 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<String, String>) -> 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
Expand Down Expand Up @@ -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 {})
}?;
Expand Down Expand Up @@ -673,6 +788,33 @@ fn url_from_env(env_name: &str, default_url: &str) -> Result<Url> {
Ok(url)
}

fn split_sas(sas: &str) -> Result<Vec<(String, String)>, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any tests of this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we do .. tested the refrerence key before that is was useful - albeit now exired :)

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::*;
Expand Down