liurenjie1024 commented on code in PR #1241:
URL: https://github.com/apache/iceberg-rust/pull/1241#discussion_r2063661454
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
Review Comment:
I don't think we should credential store loader, why not store credential?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
+ if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s|
s.as_str()) {
Review Comment:
Why not use `bool::from_str` here?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
+ if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s|
s.as_str()) {
+ let Some(signing_region) = self.props.get("rest.signing-region")
else {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "rest.signing-region is not set when rest.sigv4-enabled is
true",
+ ));
+ };
+ let Some(signing_name) = self.props.get("rest.signing-name") else {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "rest.signing-name is not set when rest.sigv4-enabled is
true",
+ ));
+ };
+
+ let config = AwsConfig::default().from_profile().from_env();
+ let loader =
AwsDefaultLoader::new(self.client().unwrap_or_default(), config);
Review Comment:
Not only from profile, we should also allow user to config using properties,
for example aws_access_id
##########
crates/catalog/rest/src/client.rs:
##########
@@ -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 {
+ Ok(Some(credential)) => {
+ const EMPTY_STRING_SHA256: &str =
+
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
+ request.headers_mut().insert(
+ "x-amz-content-sha256",
+ HeaderValue::from_str(EMPTY_STRING_SHA256).unwrap(),
+ );
Review Comment:
I'm also quite confused.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
Review Comment:
All the keys should use constants rather magic strings.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -306,6 +330,13 @@ impl RestCatalog {
None => None,
};
+ if let Some(warehouse_path) = warehouse_path {
Review Comment:
Look at below codes, we should also consider `metadata_location`. Also
please add some comments to explain this change.
##########
Cargo.toml:
##########
@@ -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" }
Review Comment:
I'm fine with `reqsign` as its api will be easier to get adapted to
different vendors, and also `aws-sigv4` itself says it's not designed for used
directly, see
> Low-level SigV4 request signing implementations.
>
> This crate is part of the [AWS SDK for
Rust](https://awslabs.github.io/aws-sdk-rust/) and the
[smithy-rs](https://github.com/smithy-lang/smithy-rs) code generator. In most
cases, it should not be used directly.
##########
crates/catalog/rest/src/client.rs:
##########
@@ -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 {
Review Comment:
We also need to change `request` method.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
+ if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s|
s.as_str()) {
+ let Some(signing_region) = self.props.get("rest.signing-region")
else {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "rest.signing-region is not set when rest.sigv4-enabled is
true",
Review Comment:
This string should be formatted using constants
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -84,6 +85,29 @@ impl RestCatalogConfig {
}
}
+ pub(crate) fn get_signer(&self) -> Result<Option<(AwsDefaultLoader,
AwsV4Signer)>> {
+ if let Some("true") = self.props.get("rest.sigv4-enabled").map(|s|
s.as_str()) {
+ let Some(signing_region) = self.props.get("rest.signing-region")
else {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "rest.signing-region is not set when rest.sigv4-enabled is
true",
+ ));
+ };
+ let Some(signing_name) = self.props.get("rest.signing-name") else {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "rest.signing-name is not set when rest.sigv4-enabled is
true",
Review Comment:
Ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
