Make Hadoop 2/3 compatible with Java SDK for IAM #7634

Isan-Rivkin · 2024-04-07T11:19:51Z

Our hadoop-lakeFS supports hadoop 2 and 3 contracts and introduces the following dependencies:

Hadoop-AWS 2 - AWS SDK 1.7.7
Hadoop-AWS 3 - AWS SDK 1.11+

Those AWS SDK versions has breaking changes between them.
When working with AWS SDK that means the code will not compile.
To solve this issue I copied the minimal as possible code required to make it work on both versions.

github-actions · 2024-04-07T11:20:04Z

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

github-actions · 2024-04-07T11:24:39Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks!

This one is hard to test; I suppose the real test will be that a system test works with lakeFS.

arielshaqed · 2024-04-08T08:27:31Z

clients/hadoopfs/src/main/java/io/lakefs/auth/GetCallerIdentityV4Presigner.java

+ * GetCallerIdentityV4Presigner is that knows how to generate a presigned URL for the GetCallerIdentity API. The presigned URL is signed using SigV4.
+ * This class is extending AWS4Signer of AWS SDK version 1.7.4 and copies some functions from https://github.com/aws/aws-sdk-java/blob/1.7.4/src/main/java/com/amazonaws/auth/AWS4Signer.java
+ * The reason we copy some functions is that we need to support aws-hadoop-2 which depends on aws sdk 1.7.4 while aws-hadoop-3 depends on aws sdk 1.11.375.
+ * Everything that is copied starts with "overridden" prefix, a reasonable alternative would be to use @Override but, AWS made those functions final.


Nit:

Suggested change

* GetCallerIdentityV4Presigner is that knows how to generate a presigned URL for the GetCallerIdentity API. The presigned URL is signed using SigV4.

* This class is extending AWS4Signer of AWS SDK version 1.7.4 and copies some functions from https://github.com/aws/aws-sdk-java/blob/1.7.4/src/main/java/com/amazonaws/auth/AWS4Signer.java

* The reason we copy some functions is that we need to support aws-hadoop-2 which depends on aws sdk 1.7.4 while aws-hadoop-3 depends on aws sdk 1.11.375.

* Everything that is copied starts with "overridden" prefix, a reasonable alternative would be to use @Override but, AWS made those functions final.

* GetCallerIdentityV4Presigner is generates a presigned URL for the GetCallerIdentity API, signed using SigV4.

* This class extends AWS4Signer of AWS SDK version 1.7.4 and copies some functions from https://github.com/aws/aws-sdk-java/blob/1.7.4/src/main/java/com/amazonaws/auth/AWS4Signer.java

* The copied functions exist in AWS SDK 1.7.4 but not AWS SDK 1.11.375, so they are

not available on Hadoop AWS 3.

* Copied code has an "overridden" prefix: cannot use @Override as those functions are final.

updated in the new implementation

arielshaqed · 2024-04-08T08:28:16Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

@@ -108,15 +107,39 @@ public Request<GeneratePresignGetCallerIdentityRequest> newPresignedRequest() th

    public String newPresignedGetCallerIdentityToken() throws Exception {
        Request<GeneratePresignGetCallerIdentityRequest> signedRequest = this.newPresignedRequest();
+        Map<String, ?> rawQueryParams = signedRequest.getParameters();
+        Map<String, String> params = new HashMap<>();
+        // check if the value is an array and join it with commas depends on the AWS SDK version


Not sure what "depends on the AWS SDK version" means here.

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

arielshaqed · 2024-04-08T08:32:32Z

clients/hadoopfs/src/main/java/io/lakefs/auth/GetCallerIdentityV4Presigner.java

+        StringBuilder pattern = new StringBuilder();
+
+        pattern
+                .append(Pattern.quote("+"))
+                .append("|")
+                .append(Pattern.quote("*"))
+                .append("|")
+                .append(Pattern.quote("%7E"))
+                .append("|")
+                .append(Pattern.quote("%2F"));


Consider:

Suggested change

StringBuilder pattern = new StringBuilder();

pattern

.append(Pattern.quote("+"))

.append("|")

.append(Pattern.quote("*"))

.append("|")

.append(Pattern.quote("%7E"))

.append("|")

.append(Pattern.quote("%2F"));

StringBuilder pattern = new StringBuilder()

.append(Pattern.quote("+"))

.append("|")

.append(Pattern.quote("*"))

.append("|")

.append(Pattern.quote("%7E"))

.append("|")

.append(Pattern.quote("%2F"));

clients/hadoopfs/src/main/java/io/lakefs/auth/STSGetCallerIdentityPresigner.java

arielshaqed · 2024-04-08T08:36:34Z

clients/hadoopfs/src/main/java/io/lakefs/auth/TemporaryAWSCredentialsLakeFSTokenProvider.java

@@ -28,7 +28,7 @@ public TemporaryAWSCredentialsLakeFSTokenProvider(String scheme, Configuration c
        }
        AWSCredentialsProvider awsProvider = new AWSCredentialsProvider() {
            @Override
-            public AWSCredentials getCredentials() {
+            public AWSCredentials getCredentials() {    


nit:

Suggested change

public AWSCredentials getCredentials() {

public AWSCredentials getCredentials() {

arielshaqed · 2024-04-08T08:36:54Z

clients/hadoopfs/src/test/java/io/lakefs/auth/AWSLakeFSTokenProviderTest.java

+    @Test
+    public void name() {
+    }
+


Why do we need this?

Isan-Rivkin · 2024-04-08T22:14:48Z

Hey @arielshaqed, thanks for the review so far, sorry in advance for this but after you mentioned the CI failed I noticed a big flaw in what I did, so then I had to rethink the issue and do something else.
Please take a look - I completely re-wrote clients/hadoopfs/src/main/java/io/lakefs/auth/GetCallerIdentityV4Presigner.java to a minimal common version between the AWS SDK's.

Specifically regarding GetCallerIdentityV4Presigner - there's no need in comparing changes, it's all new.
🙏

arielshaqed

Yay, this is good stuff!

Please avoid "copy from v2", and prefer instead to specify the specific version of the AWS SDK from which code was copied, or the specific version of Hadoop if that is relevant.

arielshaqed · 2024-04-10T03:48:40Z

clients/hadoopfs/src/main/java/io/lakefs/auth/GetCallerIdentityV4Presigner.java

- * GetCallerIdentityV4Presigner is that knows how to generate a presigned URL for the GetCallerIdentity API.
- * The presigned URL is signed using SigV4.
- * TODO: when we move to AWS SDK v2, we can use the AWS SDK's implementation of this (depends on hadoop-aws upgrading their own AWS SDK dependency).
+ *  * GetCallerIdentityV4Presigner is generates a presigned URL for the GetCallerIdentity API, signed using SigV4.


Suggested change

* * GetCallerIdentityV4Presigner is generates a presigned URL for the GetCallerIdentity API, signed using SigV4.

* * GetCallerIdentityV4Presigner generates a presigned URL for the GetCallerIdentity API, signed using SigV4.

arielshaqed · 2024-04-10T03:53:55Z

clients/hadoopfs/src/test/java/io/lakefs/auth/AWSLakeFSTokenProviderTest.java

@@ -17,14 +18,22 @@ public void testProviderIdentityTokenSerde() throws Exception {
        conf.set("fs.lakefs." + Constants.TOKEN_AWS_CREDENTIALS_PROVIDER_SESSION_TOKEN_KEY_SUFFIX, "sessionToken");
        conf.set("fs.lakefs." + Constants.TOKEN_AWS_STS_ENDPOINT, "https://sts.amazonaws.com");

-        AWSLakeFSTokenProvider provider = (AWSLakeFSTokenProvider)LakeFSTokenProviderFactory.newLakeFSTokenProvider(Constants.DEFAULT_SCHEME, conf);
+        AWSLakeFSTokenProvider provider = (AWSLakeFSTokenProvider) LakeFSTokenProviderFactory.newLakeFSTokenProvider(Constants.DEFAULT_SCHEME, conf);


Not sure we need to cast here, it seems like an upcast that is usually implicit.

i need it here because the newLakeFSTokenProvider() returns LakeFSTokenProvider
while im testing specific code in AWSLakeFSTokenProvider (AWS) and i need the method newPresignedGetCallerIdentityToken

test run

6f70060

Isan-Rivkin self-assigned this Apr 7, 2024

Isan-Rivkin added 3 commits April 7, 2024 14:32

custom time

ab81168

update time

972e942

update align

de2fa51

Isan-Rivkin requested a review from arielshaqed April 8, 2024 07:12

Isan-Rivkin changed the base branch from master to feature/m1-lakefsfs-iam April 8, 2024 07:13

Isan-Rivkin added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Apr 8, 2024

Isan-Rivkin added 2 commits April 8, 2024 10:18

clean code

37cb537

update array check

df8df47

arielshaqed approved these changes Apr 8, 2024

View reviewed changes

update

3e17c1f

Isan-Rivkin requested a review from arielshaqed April 8, 2024 22:14

update review comments

b871f2f

arielshaqed approved these changes Apr 10, 2024

View reviewed changes

minor comment update

a032f53

Isan-Rivkin merged commit 280599a into feature/m1-lakefsfs-iam Apr 10, 2024
10 of 11 checks passed

Isan-Rivkin deleted the temp-test-migrate-sdkl branch April 10, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Hadoop 2/3 compatible with Java SDK for IAM #7634

Make Hadoop 2/3 compatible with Java SDK for IAM #7634

Isan-Rivkin commented Apr 7, 2024 •

edited

Loading

github-actions bot commented Apr 7, 2024

github-actions bot commented Apr 7, 2024

arielshaqed left a comment

arielshaqed Apr 8, 2024

Isan-Rivkin Apr 8, 2024

arielshaqed Apr 8, 2024

Isan-Rivkin Apr 8, 2024

arielshaqed Apr 8, 2024

arielshaqed Apr 8, 2024

arielshaqed Apr 8, 2024

Isan-Rivkin commented Apr 8, 2024

arielshaqed left a comment

arielshaqed Apr 10, 2024

arielshaqed Apr 10, 2024

Isan-Rivkin Apr 10, 2024

	public AWSCredentials getCredentials() {
	public AWSCredentials getCredentials() {

	* * GetCallerIdentityV4Presigner is generates a presigned URL for the GetCallerIdentity API, signed using SigV4.
	* * GetCallerIdentityV4Presigner generates a presigned URL for the GetCallerIdentity API, signed using SigV4.

Make Hadoop 2/3 compatible with Java SDK for IAM #7634

Make Hadoop 2/3 compatible with Java SDK for IAM #7634

Conversation

Isan-Rivkin commented Apr 7, 2024 • edited Loading

github-actions bot commented Apr 7, 2024

github-actions bot commented Apr 7, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Apr 8, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Apr 7, 2024 •

edited

Loading