AWS,Core: Add S3 REST Signer client + REST Spec #6169

nastra · 2022-11-10T14:29:40Z

This introduces an S3 REST signer client and defines a REST spec (s3-signer-open-api.yml) for a server implementation. Below is a high-level overview of the introduced changes:

the main logic and functionality resides in the S3V4RestSignerClient class
- it uses the same credential/token exchange flow as we have in RESTSessionCatalog and also uses the same token refresh mechanism. In order to achieve that, a few refactorings have been done in RESTSessionCatalog / OAuth2Util.
- the default endpoint the signer connects to is v1/aws/s3/sign but can be customized.
- The server decides which headers to sign and can indicate to the S3V4RestSignerClient whether a response with signed headers can be cached by sending a Cache-Control: private header
AwsProperties introduce s3.signer.class that allows to dynamically load an S3 Signer implementation and apply it when creating an S3 client. This can be any Signer class that implements software.amazon.awssdk.core.signer.Signer.
S3SignRequest and S3SignResponse classes define how the request and response looks like
an S3ObjectMapper class has been introduced that is similar to RESTObjectMapper but only contains what's necessary for the S3 REST signer, which are the request/response classes with OAuth-related classes and error handling.
Testing is done by using MinioContainer (TestContainers + MinIO) in TestS3RestSigner
- The S3SignerServlet defines the minimum amount of work that a server-side implementation might have. It is by no means complete and only serves the purpose of testing

nastra · 2022-11-10T14:30:30Z

/cc @rdblue @danielcweeks @jackye1995 @amogh-jahagirdar

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

open-api/s3-signer-open-api.yaml

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

jackye1995 · 2022-11-15T17:12:53Z

1 general question regarding this PR, @nastra @rdblue @danielcweeks this is a feature very specific to AWS S3. What is the general guideline in the community for adding this as a part of the OpenAPI spec? In my mind the spec should be something generic for Iceberg, and not related to any specific cloud provider. Although many cloud storage providers have built S3 compatible API, but mostly just do it to some extent to support basic operations like get and put object. I would imaging signing with sigv4 is still a very AWS-specific thing, but maybe I am wrong about that.

Overall, have we thought about making this spec more generic, or is that not possible to achieve without being cloud provider specific?

danielcweeks · 2022-11-16T16:14:39Z

1 general question regarding this PR, @nastra @rdblue @danielcweeks this is a feature very specific to AWS S3. What is the general guideline in the community for adding this as a part of the OpenAPI spec? In my mind the spec should be something generic for Iceberg, and not related to any specific cloud provider. Although many cloud storage providers have built S3 compatible API, but mostly just do it to some extent to support basic operations like get and put object. I would imaging signing with sigv4 is still a very AWS-specific thing, but maybe I am wrong about that.

Overall, have we thought about making this spec more generic, or is that not possible to achieve without being cloud provider specific?

@jackye1995 I think the purpose behind the open-api doc is just to be clear about what the expectations are from a call/response/error perspective. While it has some overlap with the other open-api doc, they are separate entities.

This signer is specific to AWS and I think that's fine since the signer implementation is in the aws module. It may make more sense to put the open-api doc in the aws module since it really only applies there (@nastra was going to look into that option).

As a general pattern, all cloud providers sign requests in some form and we may see similar implementations using different protocols in the future, but it may also be that this can be generalized in the future.

nastra · 2022-11-22T07:20:18Z

I have moved the S3 Signer REST Spec to the iceberg-aws module.

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

rdblue · 2022-11-23T19:25:17Z

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

+    }
+  }
+
+  @SuppressWarnings("checkstyle:NestedTryDepth")


Using more standard loading would avoid this problem so we don't need to suppress it.

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

danielcweeks

+1. There are some very minor things (like how we set kebab case is deprecated), but other than that, this looks good.

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

amogh-jahagirdar · 2023-01-30T19:01:31Z

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

+  private static final Cache<Key, SignedComponent> SIGNED_COMPONENT_CACHE =
+      Caffeine.newBuilder().expireAfterWrite(30, TimeUnit.SECONDS).maximumSize(100).build();


Just curious, is it worth making the cache expiration and size configurable?

I think for now we'd probably go as is and follow up and make it configurable if we see that it helps with certain use cases.

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

aws/src/test/java/org/apache/iceberg/aws/s3/signer/S3SignerServlet.java

This introduces an S3 REST signer client and defines a REST spec (`s3-signer-open-api.yml`) for a server implementation. Below is a high-level overview of the introduced changes: * the main logic and functionality resides in the `S3V4RestSignerClient` class * it uses the same **credential/token** exchange flow as we have in `RESTSessionCatalog` and also uses the same token refresh mechanism. In order to achieve that, a few refactorings have been done in `RESTSessionCatalog` / `OAuth2Util`. * the default endpoint the signer connects to is `v1/aws/s3/sign` but can be customized. * The server decides which headers to sign and can indicate to the `S3V4RestSignerClient` whether a response with signed headers can be cached by sending a `Cache-Control: private` header * `AwsProperties` introduce `s3.signer.class` that allows to dynamically load an S3 Signer implementation and apply it when creating an S3 client. This can be any Signer class that implements `software.amazon.awssdk.core.signer.Signer`. * `S3SignRequest` and `S3SignResponse` classes define how the request and response looks like * an `S3ObjectMapper` class has been introduced that is similar to `RESTObjectMapper` but only contains what's necessary for the S3 REST signer, which are the request/response classes with OAuth-related classes and error handling. * Testing is done by using `MinioContainer` (`TestContainers` + `MinIO`) in `TestS3RestSigner` * The `S3SignerServlet` defines the minimum amount of work that a server-side implementation might have. It is by no means complete and only serves the purpose of testing

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

jackye1995

Thanks for the fixes, looks good to me!

amogh-jahagirdar

Looks great to me @nastra!

danielcweeks · 2023-02-03T18:45:01Z

Thanks @nastra !!

* AWS,Core: Add S3 Signer client + REST Spec This introduces an S3 REST signer client and defines a REST spec (`s3-signer-open-api.yml`) for a server implementation. Below is a high-level overview of the introduced changes: * the main logic and functionality resides in the `S3V4RestSignerClient` class * it uses the same **credential/token** exchange flow as we have in `RESTSessionCatalog` and also uses the same token refresh mechanism. In order to achieve that, a few refactorings have been done in `RESTSessionCatalog` / `OAuth2Util`. * the default endpoint the signer connects to is `v1/aws/s3/sign` but can be customized. * The server decides which headers to sign and can indicate to the `S3V4RestSignerClient` whether a response with signed headers can be cached by sending a `Cache-Control: private` header * `AwsProperties` introduce `s3.signer.class` that allows to dynamically load an S3 Signer implementation and apply it when creating an S3 client. This can be any Signer class that implements `software.amazon.awssdk.core.signer.Signer`. * `S3SignRequest` and `S3SignResponse` classes define how the request and response looks like * an `S3ObjectMapper` class has been introduced that is similar to `RESTObjectMapper` but only contains what's necessary for the S3 REST signer, which are the request/response classes with OAuth-related classes and error handling. * Testing is done by using `MinioContainer` (`TestContainers` + `MinIO`) in `TestS3RestSigner` * The `S3SignerServlet` defines the minimum amount of work that a server-side implementation might have. It is by no means complete and only serves the purpose of testing * make http client static * review feedback * change dynamic loading of signer

nastra changed the title ~~AWS,Core: Add S3 Signer client + REST Spec~~ AWS,Core: Add S3 REST Signer client + REST Spec Nov 10, 2022

github-actions bot added AWS build core labels Nov 10, 2022

nastra force-pushed the s3-signer branch 2 times, most recently from 07dc9f2 to b2f7900 Compare November 11, 2022 07:46

nastra requested review from rdblue and danielcweeks November 14, 2022 07:46

jackye1995 reviewed Nov 14, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Outdated Show resolved Hide resolved

jackye1995 reviewed Nov 14, 2022

View reviewed changes

open-api/s3-signer-open-api.yaml Outdated Show resolved Hide resolved

jackye1995 reviewed Nov 14, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Show resolved Hide resolved

nastra force-pushed the s3-signer branch from b2f7900 to c70bfdb Compare November 15, 2022 16:27

github-actions bot added the INFRA label Nov 15, 2022

nastra closed this Nov 15, 2022

nastra reopened this Nov 15, 2022

nastra closed this Nov 15, 2022

nastra reopened this Nov 15, 2022

nastra force-pushed the s3-signer branch from c70bfdb to 0fb145d Compare November 22, 2022 07:19

rdblue reviewed Nov 22, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Outdated Show resolved Hide resolved

rdblue reviewed Nov 22, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Outdated Show resolved Hide resolved

nastra force-pushed the s3-signer branch from 0fb145d to eadfd80 Compare November 23, 2022 08:12

rdblue reviewed Nov 23, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Outdated Show resolved Hide resolved

rdblue reviewed Nov 23, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Show resolved Hide resolved

rdblue reviewed Nov 23, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Outdated Show resolved Hide resolved

rdblue reviewed Nov 23, 2022

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Outdated Show resolved Hide resolved

nastra force-pushed the s3-signer branch from e1433e0 to 7abb073 Compare January 25, 2023 07:21

nastra requested review from danielcweeks, jackye1995 and rdblue January 25, 2023 07:28

nastra closed this Jan 25, 2023

nastra reopened this Jan 25, 2023

nastra closed this Jan 25, 2023

nastra reopened this Jan 25, 2023

danielcweeks approved these changes Jan 27, 2023

View reviewed changes

nastra force-pushed the s3-signer branch from e39160a to 2f40157 Compare January 28, 2023 05:07

jackye1995 reviewed Jan 28, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Outdated Show resolved Hide resolved

amogh-jahagirdar reviewed Jan 30, 2023

View reviewed changes

nastra force-pushed the s3-signer branch from 2f40157 to d0f8c2c Compare February 1, 2023 15:23

nastra closed this Feb 1, 2023

nastra reopened this Feb 1, 2023

nastra added 3 commits February 1, 2023 18:15

make http client static

136843c

review feedback

990e066

nastra force-pushed the s3-signer branch from d0f8c2c to 990e066 Compare February 1, 2023 17:15

jackye1995 reviewed Feb 1, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Show resolved Hide resolved

change dynamic loading of signer

5634bfe

jackye1995 approved these changes Feb 1, 2023

View reviewed changes

nastra closed this Feb 1, 2023

nastra reopened this Feb 1, 2023

amogh-jahagirdar approved these changes Feb 1, 2023

View reviewed changes

danielcweeks approved these changes Feb 3, 2023

View reviewed changes

danielcweeks merged commit 8076672 into apache:master Feb 3, 2023

nastra deleted the s3-signer branch June 1, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS,Core: Add S3 REST Signer client + REST Spec #6169

AWS,Core: Add S3 REST Signer client + REST Spec #6169

nastra commented Nov 10, 2022

nastra commented Nov 10, 2022

jackye1995 commented Nov 15, 2022

danielcweeks commented Nov 16, 2022

nastra commented Nov 22, 2022

rdblue Nov 23, 2022

danielcweeks left a comment

amogh-jahagirdar Jan 30, 2023

nastra Feb 1, 2023

jackye1995 left a comment

amogh-jahagirdar left a comment

danielcweeks commented Feb 3, 2023

		private static final Cache<Key, SignedComponent> SIGNED_COMPONENT_CACHE =
		Caffeine.newBuilder().expireAfterWrite(30, TimeUnit.SECONDS).maximumSize(100).build();

AWS,Core: Add S3 REST Signer client + REST Spec #6169

AWS,Core: Add S3 REST Signer client + REST Spec #6169

Conversation

nastra commented Nov 10, 2022

nastra commented Nov 10, 2022

jackye1995 commented Nov 15, 2022

danielcweeks commented Nov 16, 2022

nastra commented Nov 22, 2022

rdblue Nov 23, 2022

Choose a reason for hiding this comment

danielcweeks left a comment

Choose a reason for hiding this comment

amogh-jahagirdar Jan 30, 2023

Choose a reason for hiding this comment

nastra Feb 1, 2023

Choose a reason for hiding this comment

jackye1995 left a comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

danielcweeks commented Feb 3, 2023