validate individual part ranges/size in upload/download #3202
Conversation
feature/s3/manager/upload.go
Outdated
|
|
||
| u.expectParts = u.totalSize / u.cfg.PartSize | ||
| if u.totalSize%u.cfg.PartSize != 0 { | ||
| u.expectParts += 1 |
There was a problem hiding this comment.
chill linter, that's like, your opinion
lucix-aws
force-pushed
the
tm-validateparts
branch
from
c2e7f56 to
ccb4f13
Compare
| return 0, err | ||
| } | ||
|
|
||
| if !d.cfg.DisableValidateParts && params.Range != nil && resp.ContentRange != nil { |
There was a problem hiding this comment.
nit: IIRC params.Range will always be set at old line 361 in downloadChunk
|
change seems fine, but should we add some tests for it? who tests the validators? who watches the watchmen? |
It basically seemed to me like the existing unit tests all do cover it, since the new logic is now running for all of them. |
I'm not sure how hard it is to mock a case where we get a different number of parts and I'll be fine if that's a medium/hard thing to do, but we're adding the |
feature/s3/manager/download.go
Outdated
| return false // we don't know, one of the ranges was missing or unparseable | ||
| } | ||
|
|
||
| return expectStart != actualStart && expectEnd != actualEnd |
There was a problem hiding this comment.
Shouldn't it be "||" here? Either start or end bytes mismatch should return true
Well evidently based on Tianyi's last comment, we do absolutely need a positive test for this, so I'll need to figure it out. |
| c, _, _ := newDownloadBadRangeClient(buf12MB) | ||
|
|
||
| d := manager.NewDownloader(c, func(d *manager.Downloader) { | ||
| d.Concurrency = 1 |
There was a problem hiding this comment.
can we add a test when we useDisableValidateParts and ensure this does not fail?
| // for the final chunk (or the first chunk if it's smaller) we still | ||
| // request a full chunk but we get back the actual final part of the | ||
| // object, which will be smaller | ||
| if expectStart == actualStart && actualEnd < expectEnd { |
3 participants
Adds new SEP-specified durability checks to the existing transfer manager:
I have also elected to add a disable switch for these, since this is technically a behavioral break and there is a possibility that folks out there might actually be hitting these failure points without realizing. I'll add a pinned announcement issue to highlight this change when we ship it.
These changes are effectively verified by the existing unit/integration tests passing.