TrueCloudLab
/
frostfs-s3-gw
15
2
Fork 15
Code Issues 20 Pull Requests 6 Actions Packages Projects Releases 7 Activity

Add chunk uploading #126

Merged
alexvanin merged 1 commits 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
ironbee commented 2023-06-02 07:45:20 +00:00

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
Collaborator

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
Poster

What is meant by "this" here?

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

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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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

Or just replace printlns with asserts.

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

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
Collaborator

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

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
Collaborator

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

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

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

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 Label
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 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
There is no content yet.
Delete Branch "ironbee/frostfs-s3-gw:add-chunk-reader"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?