Upgrade code to use aws sdk go v2 #6486

nopcoder · 2023-08-25T21:26:48Z

Upgrade AWS Go SDK to v2: s3 adapter implementation, Dynamodb client and additional STS and direct client code
Configuration parameters streaming_chunk_timeout and streaming_chunk_size removed from S3.
Configuration parameter access_secret_key was removed from S3 credentials.
Configuration parameters client_log_request and client_log_retries added to s3 block adapter to control logging level of the s3 adapter.

TODO:

Pass: Test metastore client commands using trino and dbt
Pass: Test lakeFS with Spark 2.X + Spark 3.x
Pre-signed URL expiry window
Server side encrypted bucket
Upload using client SDKs

Close #2684
Close #6553

github-actions · 2023-08-26T22:03:22Z

♻️ PR Preview 204c1f4 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

pkg/block/s3/adapter.go

cmd/lakectl/cmd/metastore.go

pkg/api/controller.go

pkg/block/s3/adapter.go

pkg/config/config_test.go

nopcoder · 2023-09-05T15:44:03Z

@guy-har can you review only the adapter part that before had to stream data to s3 - the s3 client I'm using should provide the same functionality.

pkg/block/s3/adapter_test.go

pkg/block/s3/inventory.go

pkg/cloud/aws/metadata.go

pkg/block/s3/adapter.go

johnnyaug

Wow! Amazing work.

…ming_chunk_timeout

arielshaqed

Only nits. It's a neat way of getting at the expiry time.

arielshaqed · 2023-09-11T04:20:57Z

pkg/block/s3/adapter.go

-	// TODO(barak): handle expiry window of the client credentials when pre-signed
-	// support enabled
-	expiry := time.Now().Add(a.preSignedExpiry)
+	expiry := time.Now().UTC().Add(a.preSignedExpiry)


UTC is a timezone. The best kind of timezone, but still a timezone. Unless Golang defaults yo something line SQL TIMESTAMP (which is without TIMEZONE), I don't think UTC is needed or required here.
Timezones belong when formatting time, not when processing it.

true, I will remove it.
it was easy to debug as the aws credentials expire time was in this tz.

arielshaqed · 2023-09-11T04:22:42Z

pkg/block/s3/adapter.go

+
+func (c *CaptureExpiresPresigner) PresignHTTP(ctx context.Context, credentials aws.Credentials, r *http.Request, payloadHash string, service string, region string, signingTime time.Time, optFns ...func(*v4.SignerOptions)) (url string, signedHeader http.Header, err error) {
+	// capture credentials expiry
+	c.CredentialsCanExpire = credentials.CanExpire


I think this could just capture credentials and make them available later on. No need to run the processing logic here. I prefer to keep the signing path as simple and unmodified as possible.

I was worried about capturing something that holds a state that can change and instead capture the same information the signer accepted.
Let me know if you think. if the risk is low I'll update the code.

arielshaqed · 2023-09-11T04:26:47Z

Also... I understand that the v2 SDK will refresh credentials, and do so on time? The v1 SDK doesn't refresh credentials if it only presigns, and we needed refreshClientIfNeeded.

nopcoder · 2023-09-11T05:54:01Z

Also... I understand that the v2 SDK will refresh credentials, and do so on time? The v1 SDK doesn't refresh credentials if it only presigns, and we needed refreshClientIfNeeded.

Yes, the sdk v2 presign_middleware.go calls s.credentialsProvider.Retrieve(ctx) to refresh the current credentials. It is depends on the credentials provider and the window we configure to make sure we get enough time for our signed URL.

arielshaqed · 2023-09-12T11:45:26Z

pkg/block/s3/adapter.go

 	if captureExpiresPresigner.CredentialsCanExpire && captureExpiresPresigner.CredentialsExpireAt.Before(expiry) {
-		expiry = captureExpiresPresigner.CredentialsExpireAt
+		expiry = captureExpiresPresigner.CredentialsExpireAt.Add(a.sessionExpiryWindow)


PLEASE add a comment before this. I did not understand the logic until you explained it to me!
Perhaps something like

// AWS Go SDK v2 stores the time to renew credentials in `CredentialsExpireAt`. This is // a.sessionExpiryWindow before actual credentials expiry.

Thanks @arielshaqed !

nopcoder added the exclude-changelog PR description should not be included in next release changelog label Aug 25, 2023

nopcoder self-assigned this Aug 25, 2023

nopcoder force-pushed the chore/aws-sdk-go-v2 branch from a737512 to bad1f51 Compare August 26, 2023 22:02

github-advanced-security bot found potential problems Aug 26, 2023

View reviewed changes

pkg/block/s3/adapter.go Fixed Show fixed Hide fixed

nopcoder force-pushed the chore/aws-sdk-go-v2 branch from e971157 to eea0f60 Compare August 28, 2023 16:02

nopcoder added area/block-adapter include-changelog PR description should be included in next release changelog AWS and removed exclude-changelog PR description should not be included in next release changelog labels Aug 28, 2023

nopcoder force-pushed the chore/aws-sdk-go-v2 branch from a82f8c8 to 9de5ecb Compare August 30, 2023 07:04

nopcoder requested a review from johnnyaug September 1, 2023 10:34

nopcoder marked this pull request as ready for review September 1, 2023 10:34