s3: add Content-MD5 header in S3Uploader for compatibility options#65457
s3: add Content-MD5 header in S3Uploader for compatibility options#65457ti-chi-bot[bot] merged 7 commits intopingcap:masterfrom
Conversation
|
Hi @GMHDBJD. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #65457 +/- ##
================================================
- Coverage 77.8399% 77.6554% -0.1846%
================================================
Files 1983 1907 -76
Lines 542760 530455 -12305
================================================
- Hits 422484 411927 -10557
+ Misses 118616 118522 -94
+ Partials 1660 6 -1654
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
pkg/objstore/s3store/s3.go
Outdated
| input := buildPutObjectInput(options, file, []byte("check")) | ||
| _, err = svc.PutObject(ctx, input) | ||
| var optFns []func(*s3.Options) | ||
| if options.Provider != "" && options.Provider != "aws" { |
There was a problem hiding this comment.
please merge master, we can also check through s3Compatible now
# Conflicts: # pkg/objstore/s3store/s3.go
There was a problem hiding this comment.
Pull request overview
This PR ensures S3-compatible object stores (e.g., KS3) receive a Content-MD5 header on uploads, fixing compatibility issues encountered during import job cleanup (missing Content-Md5 header errors).
Changes:
- Updated
s3Client.CheckPutAndDeleteObjectands3Client.PutObjectto pass a conditional client option that addsContent-MD5whens3Compatibleis true. - Propagated the
s3Compatibleflag intomultipartWriterand configured both the low-level multipart writer (UploadPart) and the high-levelmanager.Uploader(MultipartUploader) to use thewithContentMD5option for compatible backends. - Centralized checksum behavior using the new
withContentMD5helper, which disables the AWS flexible checksum stack and installsAddContentChecksumMiddlewareinstead.
| var optFns []func(*s3.Options) | ||
| if c.s3Compatible { | ||
| optFns = []func(*s3.Options){withContentMD5} | ||
| } | ||
| _, err := c.svc.PutObject(ctx, input, optFns...) |
There was a problem hiding this comment.
The new s3Compatible branch and use of withContentMD5 in PutObject are not covered by tests; existing tests in client_test.go only exercise the default (AWS) path where s3Compatible remains false. Consider adding unit tests that construct an s3Client with s3Compatible set to true and assert that PutObject invokes the S3API mock with the expected client option (e.g., that at least one option is passed, or that withContentMD5 is used).
| var optFns []func(*s3.Options) | ||
| if u.s3Compatible { | ||
| optFns = []func(*s3.Options){withContentMD5} | ||
| } | ||
| uploadResult, err := u.svc.UploadPart(ctx, partInput, optFns...) |
There was a problem hiding this comment.
The multipart upload writer path now conditionally applies withContentMD5 when s3Compatible is true, but there are no tests covering this new behavior; current tests only validate multipartWriter against the default configuration. Please add tests that create a multipartWriter with s3Compatible set to true and verify that UploadPart is called on the S3API mock with an additional client option (ensuring the compatibility-specific checksum middleware is exercised).
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #65331
Problem Summary:
What changed and how does it work?
Check List
Tests
tested on KS3, before this PR when cleanup import jobs we meet error. after this PR there is no such error.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.