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

[#540] Add md5 S3Tests compatability #546

Merged
alexvanin merged 1 commit from pogpp/frostfs-s3-gw:bugfix/540_md5_header_check into master 2024-11-13 12:33:41 +00:00
Conversation 0 Commits 1 Files changed 6 +53 -9
pogpp commented 2024-11-11 11:24:37 +00:00
Member
Copy link

close #540
Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

close #540 Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp added 1 commit 2024-11-11 11:24:37 +00:00
[#540] Add md5 S3Tests compatability
Some checks failed
/ DCO (pull_request) Successful in 1m20s
Details
/ Vulncheck (pull_request) Successful in 1m37s
Details
/ Builds (pull_request) Successful in 1m31s
Details
/ Lint (pull_request) Successful in 2m37s
Details
/ Tests (pull_request) Failing after 1m30s
Details
bd1b6f30bb 
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp requested review from alexvanin 2024-11-11 11:25:09 +00:00
pogpp requested review from dkirillov 2024-11-11 11:25:09 +00:00
pogpp requested review from mbiryukova 2024-11-11 11:25:10 +00:00
pogpp requested review from r.loginov 2024-11-11 11:25:11 +00:00
pogpp requested review from nzinkevich 2024-11-11 11:25:11 +00:00
dkirillov reviewed 2024-11-11 11:52:00 +00:00
dkirillov left a comment
Member
Copy link

Please, fix make tests

Please, fix `make tests`
👍 1
api/handler/encryption_test.go Outdated
@ -51,0 +64,4 @@
api.ContentMD5: "YWJjMTIzIT8kKiYoKSctPUB+",
}
putEncryptedObjectWithHeadersErr(t, tc, bktName, objName, content, headers, errors.ErrBadDigest)
dkirillov commented 2024-11-11 11:51:35 +00:00
Member
Copy link

Why don't we have the same test for regular PutObject?

Why don't we have the same test for regular `PutObject`?
👍 1
dkirillov marked this conversation as resolved
api/layer/object.go Outdated
@ -299,2 +299,3 @@
return nil, apierr.GetAPIError(apierr.ErrInvalidDigest)
return nil, apierr.GetAPIError(apierr.ErrBadDigest)
}
} else if !p.Encryption.Enabled() && len(p.ContentMD5) == 0 {
dkirillov commented 2024-11-11 11:48:30 +00:00
Member
Copy link

I suppose we should distinguish cases where Contet-Md5 isn't set and set but empty. Error should be occurred in the second case

I suppose we should distinguish cases where `Contet-Md5` isn't set and set but empty. Error should be occurred in the second case
👍 1
dkirillov marked this conversation as resolved
pogpp force-pushed bugfix/540_md5_header_check from bd1b6f30bb to c5737b1c1a 2024-11-12 20:08:08 +00:00 Compare
alexvanin approved these changes 2024-11-13 08:55:20 +00:00
Dismissed
api/handler/put_test.go Outdated
@ -501,2 +507,2 @@
//awsCreds := credentials.NewStaticCredentials(AWSAccessKeyID, AWSSecretAccessKey, "")
//signer := v4.NewSigner(awsCreds)
// awsCreds := credentials.NewStaticCredentials(AWSAccessKeyID, AWSSecretAccessKey, "")
// signer := v4.NewSigner(awsCreds)
alexvanin commented 2024-11-13 08:52:12 +00:00
Owner
Copy link

Do we even need this? Check git blame and remove unused code.

Do we even need this? Check git blame and remove unused code.
👍 1
api/layer/object.go Outdated
@ -289,2 +289,2 @@
if !p.Encryption.Enabled() && len(p.ContentMD5) > 0 {
headerMd5Hash, err := base64.StdEncoding.DecodeString(p.ContentMD5)
if !p.Encryption.Enabled() && p.ContentMD5 != nil && len(*p.ContentMD5) > 0 {
headerMd5Hash, err := base64.StdEncoding.DecodeString(*p.ContentMD5)
alexvanin commented 2024-11-13 08:55:15 +00:00
Owner
Copy link

Looks good, I would write

if !p.Encryption.Enabled() && p.ContentMD5 != nil  {
	if len(*p.ContentMD5) == 0 {
		return nil, apierr.GetAPIError(apierr.ErrInvalidDigest)
	}
	headerMd5Hash, err := base64.StdEncoding.DecodeString(*p.ContentMD5)
	...
}
Looks good, I would write ```go if !p.Encryption.Enabled() && p.ContentMD5 != nil { if len(*p.ContentMD5) == 0 { return nil, apierr.GetAPIError(apierr.ErrInvalidDigest) } headerMd5Hash, err := base64.StdEncoding.DecodeString(*p.ContentMD5) ... } ```
👍 2
pogpp force-pushed bugfix/540_md5_header_check from c5737b1c1a to ccc8d72758 2024-11-13 11:33:11 +00:00 Compare
pogpp dismissed alexvanin's review 2024-11-13 11:33:11 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

pogpp force-pushed bugfix/540_md5_header_check from ccc8d72758 to 8a5b5cb2bb 2024-11-13 11:37:36 +00:00 Compare
pogpp force-pushed bugfix/540_md5_header_check from 8a5b5cb2bb to fb00dff83b 2024-11-13 11:50:28 +00:00 Compare
dkirillov approved these changes 2024-11-13 12:11:40 +00:00
alexvanin approved these changes 2024-11-13 12:33:38 +00:00
alexvanin merged commit fb00dff83b into master 2024-11-13 12:33:41 +00:00
alexvanin deleted branch bugfix/540_md5_header_check 2024-11-13 12:33:45 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
mbiryukova
r.loginov
nzinkevich
dkirillov
alexvanin
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#546
No description provided.
Delete branch "pogpp/frostfs-s3-gw:bugfix/540_md5_header_check"

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?