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

[#219] Return ETag in quotes #255

Merged
alexvanin merged 1 commit from mbiryukova/frostfs-s3-gw:feature/return_etag_in_quotes into master 2023-11-22 11:30:53 +00:00
Conversation 0 Commits 1 Files changed 17 +97 -57
mbiryukova commented 2023-10-30 07:27:49 +00:00
Member
Copy link

Closes #219

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Closes #219 Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>
mbiryukova self-assigned this 2023-10-30 07:27:49 +00:00
mbiryukova requested review from storage-services-committers 2023-10-30 07:37:54 +00:00
mbiryukova requested review from storage-services-developers 2023-10-30 07:37:54 +00:00
dkirillov reviewed 2023-10-30 14:11:52 +00:00
dkirillov left a comment
Member
Copy link

Please don't forget update changelog

Please don't forget update changelog
api/data/info.go
@ -119,9 +120,9 @@ func (o *ObjectInfo) Address() oid.Address {
func (o *ObjectInfo) ETag(md5Enabled bool) string {
dkirillov commented 2023-10-30 14:11:14 +00:00
Member
Copy link

It seems we have to use this method in all places where we use HashSum or MD5Sum for read directly. See 1, 2, 3 etc.
Probably this also related to #205 (we return md5 not in all cases where we should @alexvanin )

It seems we have to use this method in all places where we use `HashSum` or `MD5Sum` for read directly. See [1](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/4f5f5fb5c832ba122b71dae53a03867c9469cea0/api/handler/attributes.go#L188), [2](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/4f5f5fb5c832ba122b71dae53a03867c9469cea0/api/data/info.go#L91), [3](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/4f5f5fb5c832ba122b71dae53a03867c9469cea0/api/handler/put.go#L577) etc. Probably this also related to https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/205 (we return md5 not in all cases where we should @alexvanin )
alexvanin commented 2023-10-31 07:30:47 +00:00
Owner
Copy link

Agree. All places with hashsum should use ETag value according to md5 setting.

Agree. All places with hashsum should use ETag value according to md5 setting.
dkirillov marked this conversation as resolved
api/data/info.go Outdated
@ -123,2 +123,3 @@
return fmt.Sprintf("%q", o.MD5Sum)
}
return o.HashSum
return fmt.Sprintf("%q", o.HashSum)
dkirillov commented 2023-10-30 13:55:25 +00:00
Member
Copy link

I would suggest to use

return "\"" + o.HashSum + "\""

because benchmarks

func BenchmarkPrintf(b *testing.B) {
	obj := &data.ObjectInfo{
		ID: oidtest.ID(),
	}
	for i := 0; i < b.N; i++ {
		obj.ETag(false)
	}
}

func BenchmarkConcat(b *testing.B) {
	obj := &data.ObjectInfo{
		ID: oidtest.ID(),
	}
	for i := 0; i < b.N; i++ {
		obj.ETag2(false)
	}
}

gives

BenchmarkPrintf
BenchmarkPrintf-8   	14357821	        79.66 ns/op
BenchmarkConcat
BenchmarkConcat-8   	68286234	        18.37 ns/op
I would suggest to use ```golang return "\"" + o.HashSum + "\"" ``` because benchmarks ```golang func BenchmarkPrintf(b *testing.B) { obj := &data.ObjectInfo{ ID: oidtest.ID(), } for i := 0; i < b.N; i++ { obj.ETag(false) } } func BenchmarkConcat(b *testing.B) { obj := &data.ObjectInfo{ ID: oidtest.ID(), } for i := 0; i < b.N; i++ { obj.ETag2(false) } } ``` gives ``` BenchmarkPrintf BenchmarkPrintf-8 14357821 79.66 ns/op BenchmarkConcat BenchmarkConcat-8 68286234 18.37 ns/op ```
👍 1
dkirillov marked this conversation as resolved
api/handler/encryption_test.go Outdated
@ -304,2 +304,3 @@
return w.Header().Get(api.ETag), partBody
etag := w.Header().Get(api.ETag)
return etag[1 : len(etag)-1], partBody
dkirillov commented 2023-10-30 13:57:48 +00:00
Member
Copy link

It's better to handle quoted parts etag in CompleteMultipartUploadHandler because it's valid to pass quoted parts etag in complete multipart request (see examples)

It's better to handle quoted parts etag in `CompleteMultipartUploadHandler` because it's valid to pass quoted parts etag in complete multipart request (see [examples](https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html#API_CompleteMultipartUpload_Examples))
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/return_etag_in_quotes from a183622a6f to ee4637c6db 2023-10-31 09:51:40 +00:00 Compare
mbiryukova force-pushed feature/return_etag_in_quotes from ee4637c6db to 11e99be812 2023-10-31 13:22:04 +00:00 Compare
mbiryukova force-pushed feature/return_etag_in_quotes from 11e99be812 to d2cdf1e88d 2023-11-01 07:59:21 +00:00 Compare
mbiryukova force-pushed feature/return_etag_in_quotes from d2cdf1e88d to 03746c9361 2023-11-01 10:41:11 +00:00 Compare
dkirillov reviewed 2023-11-01 14:24:01 +00:00
api/handler/get.go Outdated
@ -243,1 +241,3 @@
return fmt.Errorf("%w: etag mismatched: '%s', '%s'", errors.GetAPIError(errors.ErrPreconditionFailed), args.IfMatch, info.HashSum)
func checkPreconditions(info *data.ObjectInfo, args *conditionalArgs, md5Enabled bool) error {
etag := strings.Trim(info.ETag(md5Enabled), "\"")
if len(args.IfMatch) > 0 && args.IfMatch != etag {
dkirillov commented 2023-11-01 14:24:01 +00:00
Member
Copy link

To compare etag this way, we have to be sure that args.IfMatch/args.IfNoneMatch don't contain quotes. But not it isn't true. See 03746c9361/api/handler/copy.go (L293)

To compare etag this way, we have to be sure that `args.IfMatch`/`args.IfNoneMatch` don't contain quotes. But not it isn't true. See https://git.frostfs.info/mbiryukova/frostfs-s3-gw/src/commit/03746c9361598829606e2911a8d964779cc0b266/api/handler/copy.go#L293
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-11-01 14:31:53 +00:00
api/data/info.go Outdated
@ -120,3 +120,3 @@
func (o *ObjectInfo) ETag(md5Enabled bool) string {
if md5Enabled && len(o.MD5Sum) > 0 {
return o.MD5Sum
return "\"" + o.MD5Sum + "\""
dkirillov commented 2023-11-01 14:31:53 +00:00
Member
Copy link

What about return values from this function without quotes?
It allows us not to trim in cases like this.

We can introduce new functions, for example

func Quote(val string) string {
    return "\"" + val + "\""
}
func UnQuote(val string) string {
    return strings.Trim(val, "\"")
}

and use them in places like this and this

The same for PartInfo struct

What about return values from this function without quotes? It allows us not to trim in cases like [this](https://git.frostfs.info/mbiryukova/frostfs-s3-gw/src/commit/03746c9361598829606e2911a8d964779cc0b266/api/handler/get.go#L242). We can introduce new functions, for example ```golang func Quote(val string) string { return "\"" + val + "\"" } func UnQuote(val string) string { return strings.Trim(val, "\"") } ``` and use them in places like [this](https://git.frostfs.info/mbiryukova/frostfs-s3-gw/src/commit/03746c9361598829606e2911a8d964779cc0b266/api/handler/multipart_upload.go#L380) and [this](https://git.frostfs.info/mbiryukova/frostfs-s3-gw/src/commit/03746c9361598829606e2911a8d964779cc0b266/api/handler/get.go#L271) The same for `PartInfo` struct
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/return_etag_in_quotes from 03746c9361 to 2813c3adfe 2023-11-02 10:43:51 +00:00 Compare
mbiryukova force-pushed feature/return_etag_in_quotes from 2813c3adfe to f35602da16 2023-11-02 10:49:18 +00:00 Compare
dkirillov approved these changes 2023-11-02 14:16:31 +00:00
dkirillov left a comment
Member
Copy link

LGTM

LGTM
alexvanin force-pushed feature/return_etag_in_quotes from f35602da16 to b28ecef43b 2023-11-22 11:12:44 +00:00 Compare
alexvanin approved these changes 2023-11-22 11:18:41 +00:00
alexvanin merged commit b28ecef43b into master 2023-11-22 11:30:53 +00:00
alexvanin deleted branch feature/return_etag_in_quotes 2023-11-22 11:30:55 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
alexvanin
TrueCloudLab/storage-services-developers
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
mbiryukova
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#255
No description provided.
Delete branch "mbiryukova/frostfs-s3-gw:feature/return_etag_in_quotes"

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?