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.

Sign up for GitHub

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

Jump to bottom

Bump AWS sdk 2.28.15 #667

Merged
merged 11 commits into from
Oct 9, 2024
Merged

Bump AWS sdk 2.28.15 #667

merged 11 commits into from
Oct 9, 2024

Conversation

pditommaso
Copy link
Collaborator

This PR bump AWS SDK 2.28.15

@pditommaso
Bump AWS sdk 2.28.15
70d3b1a 
Signed-off-by: Paolo Di Tommaso <[email protected]>
@pditommaso
Copy link
Collaborator Author

@munishchouhan can you have a look at failing tests when you have chance?

@munishchouhan
Copy link
Member

@munishchouhan can you have a look at failing tests when you have chance?

ok sure

@pditommaso
Copy link
Collaborator Author

What the problem was?

@munishchouhan
Copy link
Member

What the problem was?

Still working on it, i think there is new check has been added, which is throwing NPE

java.lang.NullPointerException: Cannot invoke "software.amazon.awssdk.core.ClientEndpointProvider.isEndpointOverridden()" because "ep" is null
	at software.amazon.awssdk.core.interceptor.SdkExecutionAttribute.lambda$static$3(SdkExecutionAttribute.java:122)
	at software.amazon.awssdk.core.interceptor.ExecutionAttribute$DerivationValueStorage.lambda$set$0(ExecutionAttribute.java:277)
	at java.base/java.util.HashMap.compute(HashMap.java:1324)
	at software.amazon.awssdk.core.interceptor.ExecutionAttribute$DerivationValueStorage.set(ExecutionAttribute.java:277)
	at software.amazon.awssdk.core.interceptor.ExecutionAttributes.putAttribute(ExecutionAttributes.java:75)
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.invokeInterceptorsAndCreateExecutionContext(DefaultS3Presigner.java:341)
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.presign(DefaultS3Presigner.java:308)
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.presignGetObject(DefaultS3Presigner.java:230)
	at io.seqera.wave.service.blob.signing.AwsS3BlobSigningService.createPresignedGetUrl(AwsS3BlobSigningService.groovy:79)
	at io.seqera.wave.service.blob.signing.AwsS3BlobSigningService.createSignedUri(AwsS3BlobSigningService.groovy:57)
	at io.seqera.wave.service.blob.impl.BlobCacheServiceImpl.blobDownloadUri(BlobCacheServiceImpl.groovy:216)
	at io.seqera.wave.service.blob.impl.BlobCacheServiceImplTest2.should create service(BlobCacheServiceImplTest2.groovy:62)

@munishchouhan munishchouhan mentioned this pull request Oct 8, 2024
Closed
1 task
@munishchouhan
Copy link
Member

I have also created a bug with aws sdk team about this
aws/aws-sdk-java-v2#5646

@munishchouhan
Copy link
Member

I think we are getting NPE from here

https://github.com/aws/aws-sdk-java-v2/blob/a9c5f266ceb1405ec17b898b287c782e5656ce80/core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java#L352

which is changed in this new version of SDK

@pditommaso
Copy link
Collaborator Author

Maybe it can be downgraded to a prior version. The goal here is to use a version that supports EKS pod identity. @gavinelder was mentioning 2.21.30 is enough

pditommaso
pditommaso commented Oct 9, 2024
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@munishchouhan
Copy link
Member

now tests are failing with this error:

'software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy$Builder software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy$Builder.defaultCredentialsProvider(software.amazon.awssdk.auth.credentials.AwsCredentialsProvider)'
java.lang.NoSuchMethodError: 'software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy$Builder software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy$Builder.defaultCredentialsProvider(software.amazon.awssdk.auth.credentials.AwsCredentialsProvider)'
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.invokeInterceptorsAndCreateExecutionContext(DefaultS3Presigner.java:362)
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.presign(DefaultS3Presigner.java:308)
	at software.amazon.awssdk.services.s3.internal.signing.DefaultS3Presigner.presignGetObject(DefaultS3Presigner.java:230)
	at io.seqera.wave.service.blob.signing.AwsS3BlobSigningService.createPresignedGetUrl(AwsS3BlobSigningService.groovy:79)
	at io.seqera.wave.service.blob.signing.AwsS3BlobSigningService.createSignedUri(AwsS3BlobSigningService.groovy:57)
	at io.seqera.wave.service.blob.impl.BlobCacheServiceImpl.blobDownloadUri(BlobCacheServiceImpl.groovy:216)
	at io.seqera.wave.service.blob.impl.BlobCacheServiceImplTest2.should create service(BlobCacheServiceImplTest2.groovy:62)

I will change the credential provider and try again

@munishchouhan
fixed tests
f33cc6c 
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member

This turn out to be dependency issue, when we are mentioning version, we need to have the whole sdk

@munishchouhan
corrected dependency
7f726e8 
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member

now failing to create shadowjar because of the files added by new dependency, i will try to enable zip64 

> shadow.org.apache.tools.zip.Zip64RequiredException: archive contains more than 65535 entries.
  
  To build this archive, please enable the zip64 extension.
  See: https://docs.gradle.org/8.8/dsl/org.gradle.api.tasks.bundling.Zip.html#org.gradle.api.tasks.bundling.Zip:zip64

@munishchouhan
enabled zip64
5dee002 
Signed-off-by: munishchouhan <[email protected]>
@pditommaso
Copy link
Collaborator Author

we need to have the whole sdk

No, that's too big. Likely some other dep is missing

@munishchouhan
Copy link
Member

we need to have the whole sdk

No, that's too big. Likely some other dep is missing

now I only included core and not the whole sdk

pditommaso
pditommaso commented Oct 9, 2024
View reviewed changes
build.gradle Outdated
@@ -204,3 +205,6 @@ jacocoTestReport {
}))
}
}
shadowJar {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this still needed ?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@pditommaso
Remove all aws sdk
9fbd8e3 
Signed-off-by: Paolo Di Tommaso <[email protected]>
@pditommaso
Copy link
Collaborator Author

All sdk was still there, lets try this ☝️

@munishchouhan
Copy link
Member

All sdk was still there, lets try this ☝️

oops sorry, my bad

@pditommaso
Copy link
Collaborator Author

damn, still failing

@munishchouhan
fixed tests
8b0dba5 
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member

damn, still failing

got the right one, fixed now

@pditommaso
Copy link
Collaborator Author

What a team work! 😆

@pditommaso
Copy link
Collaborator Author

Finally

@pditommaso pditommaso merged commit 0c487ad into master Oct 9, 2024
4 of 5 checks passed
@pditommaso pditommaso deleted the bump-aws-sdk branch October 9, 2024 21:19
pditommaso added a commit that referenced this pull request Oct 10, 2024
@pditommaso
Revert "Bump AWS SDK 2.28.15 (#667)"
3f19713 
This reverts commit 0c487ad.
pditommaso added a commit that referenced this pull request Oct 10, 2024
@pditommaso
Revert "Bump AWS SDK 2.28.15 (#667)"
da2f075 
This reverts commit 0c487ad.
@pditommaso
Copy link
Collaborator Author

This was reverted 3f19713 because aws sms was failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munishchouhan munishchouhan munishchouhan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pditommaso @munishchouhan