Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

object_store: builder configuration api #3436

Merged
merged 10 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
293 changes: 269 additions & 24 deletions object_store/src/aws/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use futures::stream::BoxStream;
use futures::TryStreamExt;
use itertools::Itertools;
use snafu::{OptionExt, ResultExt, Snafu};
use std::collections::BTreeSet;
use std::collections::{BTreeSet, HashMap};
use std::ops::Range;
use std::sync::Arc;
use tokio::io::AsyncWrite;
Expand All @@ -51,6 +51,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,
Expand Down Expand Up @@ -133,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<Error> for super::Error {
Expand Down Expand Up @@ -379,6 +383,170 @@ pub struct AmazonS3Builder {
client_options: ClientOptions,
}

/// Configuration keys for [`AmazonS3Builder`]
#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)]
Copy link
Contributor

Choose a reason for hiding this comment

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

@roeap this is very close, I think the last issue from my side is to derive serde::Serialize and Deserialize for the *ConfigKey enums.

Pretty please 🤗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, serialize away 😆.

pub 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,
}

impl From<AmazonS3ConfigKey> 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"),
}
}
}
tustvold marked this conversation as resolved.
Show resolved Hide resolved

impl TryFrom<&str> for AmazonS3ConfigKey {
roeap marked this conversation as resolved.
Show resolved Hide resolved
type Error = super::Error;

fn try_from(value: &str) -> Result<Self> {
match value.to_ascii_lowercase().as_str() {
tustvold marked this conversation as resolved.
Show resolved Hide resolved
"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<String> for AmazonS3ConfigKey {
type Error = super::Error;

fn try_from(value: String) -> Result<Self> {
Self::try_from(value.as_str())
}
}

impl AmazonS3Builder {
/// Create a new [`AmazonS3Builder`] with default values.
pub fn new() -> Self {
Expand Down Expand Up @@ -407,28 +575,11 @@ 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, 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
Expand All @@ -442,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
Expand Down Expand Up @@ -472,6 +623,50 @@ impl AmazonS3Builder {
self
}

/// Set an option on the builder via a key - value pair.
pub fn with_option(
mut self,
key: impl Into<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

@roeap thanks for the super quick turn around time on this one. This is pretty close to what I am thinking, I would like to suggest one more change?

As an user I would like to have my configuration fully checked by the compiler. So I would like to either be able to pass in one of the AmazonS3ConfigKeys as the key or have a helper function that returns one of the string representations. This way in my code I never have to type a string constant for the key.

Let me know what you think....

Copy link
Contributor

Choose a reason for hiding this comment

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

I ported my PR to the code so far. I had to toggle back and forth between the 2 PRs to make sure I get these constants correctly.
https://github.com/pola-rs/polars/pull/5972/files#diff-28ad3830b06d49511094951431beea1edcf6a567f646250482e9f00f769ace27R16

This is error prone specially since you have a type safe Enum sitting right there...

Copy link
Contributor Author

@roeap roeap Jan 4, 2023

Choose a reason for hiding this comment

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

We now have some conversions implemented for the config key enums, so you should be able to pass in the variants directly, or constuct a map with the variants as keys. Is that what you are looking for here?

roeap marked this conversation as resolved.
Show resolved Hide resolved
value: impl Into<String>,
) -> Self {
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(_) => (),
tustvold marked this conversation as resolved.
Show resolved Hide resolved
};
self
}

/// Hydrate builder from key value pairs
pub fn with_options(
mut self,
options: &HashMap<impl Into<String> + Clone, String>,
tustvold marked this conversation as resolved.
Show resolved Hide resolved
) -> Self {
for (key, value) in options {
self = self.with_option(key.clone(), 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 @@ -915,6 +1110,56 @@ 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),
("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);
}

#[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()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very ergonomic, many thanks 🎉

(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!();
Expand Down
Loading