feat(rest): support AWS SIGv4 #1241
Conversation
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
xxchan
force-pushed
the
xxchan/cheerful-mite
branch
from
8a99af2 to
e698d79
Compare
| const EMPTY_STRING_SHA256: &str = | ||
| "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; | ||
| request.headers_mut().insert( | ||
| "x-amz-content-sha256", | ||
| HeaderValue::from_str(EMPTY_STRING_SHA256).unwrap(), | ||
| ); |
There was a problem hiding this comment.
Why do we have to hardcode this here?
| @@ -220,6 +225,39 @@ impl HttpClient { | |||
| /// Executes the given `Request` and returns a `Response`. | |||
| pub async fn execute(&self, mut request: Request) -> Result<Response> { | |||
| request.headers_mut().extend(self.extra_headers.clone()); | |||
|
|
|||
| if let Some((loader, signer)) = &self.signer { | |||
| match loader.load().await { | |||
There was a problem hiding this comment.
Do we want to load credentials with every request?
let config = AwsConfig::default().from_profile().from_env();
println!("access_key_id {:?}", config.access_key_id);
let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config);
| @@ -93,6 +93,7 @@ port_scanner = "0.1.5" | |||
| pretty_assertions = "1.4" | |||
| rand = "0.8.5" | |||
| regex = "1.10.5" | |||
| reqsign = { version = "0.16.3" } | |||
There was a problem hiding this comment.
Is there a reason for not using https://crates.io/crates/aws-sigv4 ?
There was a problem hiding this comment.
reqsign is lightweight, with minimal dependency footprints. And it's already depended by us (transitively via opendal ). So it's good not to introduce new heavy dependencies.
@Xuanwo could you share your opinions on this?
| if warehouse_path.starts_with("arn:aws:") { | ||
| let file_io = FileIOBuilder::new("s3").with_props(&props).build()?; | ||
| return Ok(file_io); | ||
| } |
There was a problem hiding this comment.
Is there a better way to know the rest catalog service e.g. using rest.signing-name?
crates/catalog/rest/src/catalog.rs
Outdated
| let config = AwsConfig::default().from_profile().from_env(); | ||
| println!("access_key_id {:?}", config.access_key_id); | ||
| let loader = AwsDefaultLoader::new(self.client().unwrap_or_default(), config); |
There was a problem hiding this comment.
Can we support customizing this like passing or reusing the aws profile / credentials configuration or better introduce a new set of rest specific configurations.
Signed-off-by: xxchan [email protected]
Which issue does this PR close?
close #1236
There's one missing piece:
reqsigngot canonical request like:/iceberg/v1/arn%3Aaws%3As3tables%3Aap-southeast-1.../iceberg/v1/arn%253Aaws%253As3tables%253Aap-southeast-1%..., where reqsign wrongly decoded%3.During my local testing, I tried to remove this
percent_decode_strinreqsignhttps://github.com/Xuanwo/reqsign/blob/26a1e224b5498a59ad53d9508be1e85df322f043/src/aws/v4.rs#L223 , then it runs successfully (I tested with the iceberg-rust CLI from #1220). But I'm not sure about the decode reason here and what's the best way to change it. cc @XuanwoWhat changes are included in this PR?
Are these changes tested?