TrueCloudLab/frostfs-s3-gw
17
3
Fork 19
Code Issues 28 Pull requests 7 Projects Releases 18 Packages Activity Actions

Add chunk uploading #126

Merged
alexvanin merged 1 commit from ironbee/frostfs-s3-gw:add-chunk-reader into master 2023-06-21 11:26:38 +00:00
Conversation 0 Commits 1 Files changed 9 +404 -18
ironbee commented 2023-06-02 07:45:20 +00:00
Contributor
Copy link

Signed-off-by: Artem Tataurov a.tataurov@yadro.com

Closes #106

Signed-off-by: Artem Tataurov <a.tataurov@yadro.com> Closes #106
ironbee force-pushed add-chunk-reader from e6381badf5 to 01c32351f3 2023-06-06 16:08:52 +00:00 Compare
ironbee force-pushed add-chunk-reader from 01c32351f3 to e469e2fc40 2023-06-07 17:00:00 +00:00 Compare
ironbee changed title from WIP: Temporary commit to WIP: Add chunk uploader 2023-06-09 06:45:14 +00:00
ironbee changed title from WIP: Add chunk uploader to WIP: Add chunk uploading 2023-06-09 06:45:28 +00:00
ironbee force-pushed add-chunk-reader from e469e2fc40 to 27256db8ed 2023-06-13 06:40:43 +00:00 Compare
ironbee force-pushed add-chunk-reader from 27256db8ed to e47e50a4df 2023-06-13 11:11:07 +00:00 Compare
ironbee force-pushed add-chunk-reader from e47e50a4df to 583259c598 2023-06-13 12:43:28 +00:00 Compare
ironbee force-pushed add-chunk-reader from 583259c598 to ffb1f60161 2023-06-13 13:29:34 +00:00 Compare
ironbee force-pushed add-chunk-reader from ffb1f60161 to 01f119c6ca 2023-06-13 14:56:35 +00:00 Compare
ironbee force-pushed add-chunk-reader from 01f119c6ca to f659d0f98d 2023-06-13 15:11:29 +00:00 Compare
ironbee changed title from WIP: Add chunk uploading to Add chunk uploading 2023-06-13 15:13:10 +00:00
ironbee force-pushed add-chunk-reader from f659d0f98d to b603a34c28 2023-06-13 15:40:43 +00:00 Compare
dkirillov reviewed 2023-06-14 09:33:27 +00:00
api/handler/put.go Outdated
@ -232,6 +232,24 @@ func (h *handler) PutObjectHandler(w http.ResponseWriter, r *http.Request) {
Encryption: encryptionParams,
}
if api.IsSignedStreamingV4(r) {
dkirillov commented 2023-06-14 09:33:27 +00:00
Member
Copy link

Let's move this to separate function

func (h *handler) getBodyReader(r *http.Request) (io.Reader, error) {
    //...
}
Let's move this to separate function ```golang func (h *handler) getBodyReader(r *http.Request) (io.Reader, error) { //... } ```
ironbee commented 2023-06-15 09:17:09 +00:00
Author
Contributor
Copy link

What is meant by "this" here?

What is meant by "this" here?
dkirillov commented 2023-06-15 10:36:09 +00:00
Member
Copy link

b603a34c28/api/handler/put.go (L235-L251)

https://git.frostfs.info/ironbee/frostfs-s3-gw/src/commit/b603a34c2833e4a614f6f8b5bc69e7adec192fcb/api/handler/put.go#L235-L251
dkirillov reviewed 2023-06-14 09:34:19 +00:00
api/handler/put.go Outdated
@ -235,0 +240,4 @@
errors.GetAPIError(errors.ErrMissingContentLength))
}
} else {
h.logAndSendError(w, "expecting decode content length information", reqInfo,
dkirillov commented 2023-06-14 09:34:19 +00:00
Member
Copy link

After h.logAndSendError we must use return statement

After `h.logAndSendError` we must use `return` statement
dkirillov commented 2023-06-14 09:49:20 +00:00
Member
Copy link

To check this we can use for example test:

func TestPutObjectWithStreamBodyError(t *testing.T) {
	tc := prepareHandlerContext(t)

	bktName, objName := "bucket-for-put", "object-for-put"
	createTestBucket(tc, bktName)

	content := []byte("content")
	w, r := prepareTestPayloadRequest(tc, bktName, objName, bytes.NewReader(content))
	r.Header.Set(api.AmzContentSha256, api.StreamingContentSHA256)
	tc.Handler().PutObjectHandler(w, r)
	assertS3Error(t, w, errors.GetAPIError(errors.ErrMissingContentLength))

	checkNotFound(t, tc, bktName, objName, emptyVersion)
}
To check this we can use for example test: ```golang func TestPutObjectWithStreamBodyError(t *testing.T) { tc := prepareHandlerContext(t) bktName, objName := "bucket-for-put", "object-for-put" createTestBucket(tc, bktName) content := []byte("content") w, r := prepareTestPayloadRequest(tc, bktName, objName, bytes.NewReader(content)) r.Header.Set(api.AmzContentSha256, api.StreamingContentSHA256) tc.Handler().PutObjectHandler(w, r) assertS3Error(t, w, errors.GetAPIError(errors.ErrMissingContentLength)) checkNotFound(t, tc, bktName, objName, emptyVersion) } ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-06-14 10:48:28 +00:00
api/handler/s3reader.go Outdated
@ -0,0 +169,4 @@
// that the received signature matches our computed signature.
calculatedSignature, err := c.streamSigner.GetSignature(nil, c.buffer, c.requestTime)
if string(signature[16:]) != hex.EncodeToString(calculatedSignature) {
dkirillov commented 2023-06-14 10:48:28 +00:00
Member
Copy link

Why don't we check the err here?

Why don't we check the `err` here?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-06-14 10:52:44 +00:00
api/auth/center_test.go Outdated
@ -100,2 +105,4 @@
require.Equal(t, "dfbe886241d9e369cf4b329ca0f15eb27306c97aa1022cc0bb5a914c4ef87634", signature)
}
func TestChunkUploadSigning(t *testing.T) {
dkirillov commented 2023-06-14 10:52:44 +00:00
Member
Copy link

Actually this test doesn't check signature.
Let's rework it to:

func TestPutObjectWithStreamBodyAWSExample(t *testing.T) {
	tc := prepareHandlerContext(t)

	bktName, objName := "examplebucket", "chunkObject.txt"
	createTestBucket(tc, bktName)

	chunk := make([]byte, 65*1024)
	for i, _ := range chunk {
		chunk[i] = 'a'
	}
	chunk1 := chunk[:64*1024]
	chunk2 := chunk[64*1024:]

	AWSAccessKeyId := "AKIAIOSFODNN7EXAMPLE"
	AWSSecretAccessKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"

	awsCreds := credentials.NewStaticCredentials(AWSAccessKeyId, AWSSecretAccessKey, "")
	signer := v4.NewSigner(awsCreds)

	reqBody := bytes.NewBufferString("10000;chunk-signature=ad80c730a21e5b8d04586a2213dd63b9a0e99e0e2307b0ade35a65485a288648\r\n")
	_, err := reqBody.Write(chunk1)
	require.NoError(t, err)
	_, err = reqBody.WriteString("\r\n400;chunk-signature=0055627c9e194cb4542bae2aa5492e3c1575bbb81b612b7d234b86a503ef5497\r\n")
	require.NoError(t, err)
	_, err = reqBody.Write(chunk2)
	require.NoError(t, err)
	_, err = reqBody.WriteString("\r\n0;chunk-signature=b6c6ea8a5354eaf15b3cb7646744f4275b71ea724fed81ceb9323e279d449df9\r\n\r\n")
	require.NoError(t, err)

	req, err := http.NewRequest("PUT", "https://s3.amazonaws.com/"+bktName+"/"+objName, nil)
	require.NoError(t, err)
	req.Header.Set("content-encoding", "aws-chunked")
	req.Header.Set("content-length", "66824")
	req.Header.Set("x-amz-content-sha256", "STREAMING-AWS4-HMAC-SHA256-PAYLOAD")
	req.Header.Set("x-amz-decoded-content-length", "66560")
	req.Header.Set("x-amz-storage-class", "REDUCED_REDUNDANCY")

	signTime, err := time.Parse("20060102T150405Z", "20130524T000000Z")
	require.NoError(t, err)

	_, err = signer.Sign(req, nil, "s3", "us-east-1", signTime)
	require.NoError(t, err)

	req.Body = io.NopCloser(reqBody)

	w := httptest.NewRecorder()
	reqInfo := api.NewReqInfo(w, req, api.ObjectRequest{Bucket: bktName, Object: objName})
	req = req.WithContext(api.SetReqInfo(tc.Context(), reqInfo))
	req = req.WithContext(context.WithValue(req.Context(), api.ClientTime, signTime))
	req = req.WithContext(context.WithValue(req.Context(), api.AuthHeaders, &auth.AuthHeader{
		AccessKeyID: AWSAccessKeyId,
		SignatureV4: "4f232c4386841ef735655705268965c44a0e4690baa4adea153f7db9fa80a0a9",
		Service:     "s3",
		Region:      "us-east-1",
	}))
	req = req.WithContext(context.WithValue(req.Context(), api.BoxData, &accessbox.Box{
		Gate: &accessbox.GateData{
			AccessKey: AWSSecretAccessKey,
		},
	}))
	tc.Handler().PutObjectHandler(w, req)
	assertStatus(t, w, http.StatusOK)

	data := getObjectRange(t, tc, bktName, objName, 0, 66824)
	for i := range chunk {
		require.Equal(t, chunk[i], data[i])
	}
}
Actually this test doesn't check signature. Let's rework it to: ```golang func TestPutObjectWithStreamBodyAWSExample(t *testing.T) { tc := prepareHandlerContext(t) bktName, objName := "examplebucket", "chunkObject.txt" createTestBucket(tc, bktName) chunk := make([]byte, 65*1024) for i, _ := range chunk { chunk[i] = 'a' } chunk1 := chunk[:64*1024] chunk2 := chunk[64*1024:] AWSAccessKeyId := "AKIAIOSFODNN7EXAMPLE" AWSSecretAccessKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" awsCreds := credentials.NewStaticCredentials(AWSAccessKeyId, AWSSecretAccessKey, "") signer := v4.NewSigner(awsCreds) reqBody := bytes.NewBufferString("10000;chunk-signature=ad80c730a21e5b8d04586a2213dd63b9a0e99e0e2307b0ade35a65485a288648\r\n") _, err := reqBody.Write(chunk1) require.NoError(t, err) _, err = reqBody.WriteString("\r\n400;chunk-signature=0055627c9e194cb4542bae2aa5492e3c1575bbb81b612b7d234b86a503ef5497\r\n") require.NoError(t, err) _, err = reqBody.Write(chunk2) require.NoError(t, err) _, err = reqBody.WriteString("\r\n0;chunk-signature=b6c6ea8a5354eaf15b3cb7646744f4275b71ea724fed81ceb9323e279d449df9\r\n\r\n") require.NoError(t, err) req, err := http.NewRequest("PUT", "https://s3.amazonaws.com/"+bktName+"/"+objName, nil) require.NoError(t, err) req.Header.Set("content-encoding", "aws-chunked") req.Header.Set("content-length", "66824") req.Header.Set("x-amz-content-sha256", "STREAMING-AWS4-HMAC-SHA256-PAYLOAD") req.Header.Set("x-amz-decoded-content-length", "66560") req.Header.Set("x-amz-storage-class", "REDUCED_REDUNDANCY") signTime, err := time.Parse("20060102T150405Z", "20130524T000000Z") require.NoError(t, err) _, err = signer.Sign(req, nil, "s3", "us-east-1", signTime) require.NoError(t, err) req.Body = io.NopCloser(reqBody) w := httptest.NewRecorder() reqInfo := api.NewReqInfo(w, req, api.ObjectRequest{Bucket: bktName, Object: objName}) req = req.WithContext(api.SetReqInfo(tc.Context(), reqInfo)) req = req.WithContext(context.WithValue(req.Context(), api.ClientTime, signTime)) req = req.WithContext(context.WithValue(req.Context(), api.AuthHeaders, &auth.AuthHeader{ AccessKeyID: AWSAccessKeyId, SignatureV4: "4f232c4386841ef735655705268965c44a0e4690baa4adea153f7db9fa80a0a9", Service: "s3", Region: "us-east-1", })) req = req.WithContext(context.WithValue(req.Context(), api.BoxData, &accessbox.Box{ Gate: &accessbox.GateData{ AccessKey: AWSSecretAccessKey, }, })) tc.Handler().PutObjectHandler(w, req) assertStatus(t, w, http.StatusOK) data := getObjectRange(t, tc, bktName, objName, 0, 66824) for i := range chunk { require.Equal(t, chunk[i], data[i]) } } ```
alexvanin commented 2023-06-15 14:39:55 +00:00
Owner
Copy link

Or just replace printlns with asserts.

Or just replace printlns with asserts.
dkirillov commented 2023-06-16 06:27:17 +00:00
Member
Copy link

Yes, but then we check only validity of v4 stream signature (already existed code) rather than validity of handlers and proper invoking v4 stream signature ( new code in this PR)

Yes, but then we check only validity of v4 stream signature (already existed code) rather than validity of handlers and proper invoking v4 stream signature ( new code in this PR)
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-06-14 10:54:27 +00:00
api/headers.go Outdated
@ -109,1 +115,4 @@
}
func IsSignedStreamingV4(r *http.Request) bool {
return r.Header.Get(AmzContentSha256) == StreamingContentSHA256 &&
dkirillov commented 2023-06-14 10:54:27 +00:00
Member
Copy link

Shouldn't we also check the content-encoding for containing aws-chunked?

Shouldn't we also check the `content-encoding` for containing `aws-chunked`?
dkirillov marked this conversation as resolved
alexvanin reviewed 2023-06-14 10:58:41 +00:00
api/user_auth.go
@ -42,6 +45,7 @@ func AuthMiddleware(log *zap.Logger, center auth.Center) mux.MiddlewareFunc {
if !box.ClientTime.IsZero() {
alexvanin commented 2023-06-14 10:58:41 +00:00
Owner
Copy link

Client time is required to work with chunk payloads. However center.Authenticate might not set it depending on needClientTime flag which is set when X-Amz-Algorithm != AWS4-HMAC-SHA256. So my question to @dkirillov, would it be a bit more error prone if we set client time for all requests as long as it is available in X-Amz-Date?

Client time is required to work with chunk payloads. However `center.Authenticate` might not set it depending on `needClientTime` flag which is set when `X-Amz-Algorithm != AWS4-HMAC-SHA256`. So my question to @dkirillov, would it be a bit more error prone if we set client time for all requests as long as it is available in `X-Amz-Date`?
dkirillov commented 2023-06-14 12:20:37 +00:00
Member
Copy link

which is set when X-Amz-Algorithm != AWS4-HMAC-SHA256

Actually, we don't use client time when we get request with presigned url. But I wouldn't say that it be more error prone if we set client (signature) time for all request with X-Amz-Date (probably we should use just Date if X-Amz-Date is missed)

And we can use s3-gw time for some specific cases

> which is set when `X-Amz-Algorithm != AWS4-HMAC-SHA256` Actually, we don't use client time when we get request with presigned url. But I wouldn't say that it be more error prone if we set client (signature) time for all request with `X-Amz-Date` (probably we should use just `Date` if `X-Amz-Date` [is missed](https://docs.aws.amazon.com/AmazonECR/latest/APIReference/CommonParameters.html)) And we can use s3-gw time for some [specific cases](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/8fcaf76f419fc9a3cf6d1b9e3004412093a3d415/api/handler/put.go#L531)
alexvanin marked this conversation as resolved
alexvanin approved these changes 2023-06-15 14:40:59 +00:00
alexvanin left a comment
Owner
Copy link

LGTM, see 1, 2, 3 and 4 comments.

LGTM, see [1](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/126/files#issuecomment-12993), [2](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/126/files#issuecomment-12964), [3](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/126/files#issuecomment-12995) and [4](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/126/files#issuecomment-12988) comments.
ironbee force-pushed add-chunk-reader from b603a34c28 to f3239461b3 2023-06-20 13:04:15 +00:00 Compare
ironbee force-pushed add-chunk-reader from f3239461b3 to 7cc0c4822a 2023-06-20 13:34:55 +00:00 Compare
ironbee requested review from alexvanin 2023-06-20 14:10:05 +00:00
ironbee requested review from dkirillov 2023-06-20 14:10:24 +00:00
alexvanin reviewed 2023-06-20 15:13:26 +00:00
api/handler/multipart_upload.go Outdated
@ -235,0 +237,4 @@
_, err := strconv.Atoi(decodeContentSize)
if err != nil {
h.logAndSendError(w, "cannot parse decode content length information", reqInfo,
errors.GetAPIError(errors.ErrMissingContentLength))
alexvanin commented 2023-06-20 15:13:01 +00:00
Owner
Copy link

I think you missed return here too, as it is done in api/handler/put.go

I think you missed `return` here too, as it is done in `api/handler/put.go`
alexvanin marked this conversation as resolved
ironbee force-pushed add-chunk-reader from 7cc0c4822a to 614d703726 2023-06-21 07:24:07 +00:00 Compare
alexvanin approved these changes 2023-06-21 07:30:45 +00:00
alexvanin left a comment
Owner
Copy link

LGTM. We will consider compatibility profiles later.

LGTM. We will consider compatibility profiles later.
dkirillov approved these changes 2023-06-21 11:23:11 +00:00
alexvanin merged commit 614d703726 into master 2023-06-21 11:26:38 +00:00
alexvanin deleted branch add-chunk-reader 2023-06-21 11:26:41 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
dkirillov
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#126
No description provided.
Delete branch "ironbee/frostfs-s3-gw:add-chunk-reader"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?