Fix encrypted recording uploads in S3#60542
Conversation
…ad part; Support skipping padded part in streamer.
…turing and combining them.
|
@Joerger Manual test plan looks great. Just to confirm you tested against actual S3 right? |
Yes! |
rosstimothy
left a comment
There was a problem hiding this comment.
Can we add test coverage to prevent a regression?
5f579c7 to
34f1f23
Compare
|
@eriktate @rosstimothy I made significant changes in recent commits, mostly to accommodate the new tests. I re-ran the manual test plan in the PR description. Can you give it a second look? |
| // NewMemoryUploader returns a new memory uploader implementing multipart | ||
| // upload | ||
| func NewMemoryUploader(eventsC ...chan events.UploadEvent) *MemoryUploader { | ||
| func NewMemoryUploader(cfg ...MemoryUploaderConfig) *MemoryUploader { |
There was a problem hiding this comment.
It sure would be nice to clean this API up one day 😬.
| @@ -438,26 +445,26 @@ func TestUploadBackoff(t *testing.T) { | |||
|
|
|||
| // Fix the streamer, make sure the upload succeeds | |||
| terminateConnectionAt.Store(0) | |||
| p.clock.BlockUntil(2) | |||
| p.clock.BlockUntilContext(ctx, 2) | |||
There was a problem hiding this comment.
The Flaky Test Detector looks to maybe be hanging here?
goroutine 17077 [select]:
github.com/jonboulle/clockwork.(*FakeClock).BlockUntilContext(0xc00521f4a0, {0x4597898, 0xc0004c1110}, 0x2)
/go/pkg/mod/github.com/jonboulle/clockwork@v0.5.0/clockwork.go:244 +0xfb
github.com/gravitational/teleport/lib/events/filesessions.TestUploadBackoff(0xc001ca0fc0)
/__w/teleport/teleport/lib/events/filesessions/fileasync_test.go:448 +0xcb8
testing.tRunner(0xc001ca0fc0, 0x42389d0)
/opt/go/src/testing/testing.go:1934 +0x21d
created by testing.(*T).Run in goroutine 1
/opt/go/src/testing/testing.go:1997 +0x9d3
There was a problem hiding this comment.
It's just hitting the flaky test timeout, this is the slowest test of those that are run for this PR:
go test -run "^TestUploadBackoff$" github.com/gravitational/teleport/lib/events/filesessions --race --count=100
ok github.com/gravitational/teleport/lib/events/filesessions 164.958s
|
/excludeflake * |
Co-authored-by: Erik Tate <erik.tate@goteleport.com>
* Add necessary padding to encrypted recordings for s3. * Add padded part instead of editing existing part; Don't pad last upload part; Support skipping padded part in streamer. * Reconstruct encrypted uploads from agent. * Concatenate upload parts into a single upload part instead of restructuring and combining them. * Make iterator buffer safe. * Remove padding from 128kb parts during concatenation. * Fix padding through gRPC service. * Cleanup; Fix discarded padding logic. * Add tests. * Fix bad upload test. * Expand test, add comments. * Minor edits. * Add padding helper with test; Add comments. * Fix tests, lint. * Don't use TELEPORT_UNSTABLE_GRPC_RECV_SIZE. * Apply suggestions from code review Co-authored-by: Erik Tate <erik.tate@goteleport.com> --------- Co-authored-by: Erik Tate <erik.tate@goteleport.com>
* Add necessary padding to encrypted recordings for s3. * Add padded part instead of editing existing part; Don't pad last upload part; Support skipping padded part in streamer. * Reconstruct encrypted uploads from agent. * Concatenate upload parts into a single upload part instead of restructuring and combining them. * Make iterator buffer safe. * Remove padding from 128kb parts during concatenation. * Fix padding through gRPC service. * Cleanup; Fix discarded padding logic. * Add tests. * Fix bad upload test. * Expand test, add comments. * Minor edits. * Add padding helper with test; Add comments. * Fix tests, lint. * Don't use TELEPORT_UNSTABLE_GRPC_RECV_SIZE. * Apply suggestions from code review --------- Co-authored-by: Erik Tate <erik.tate@goteleport.com>
* Add necessary padding to encrypted recordings for s3. * Add padded part instead of editing existing part; Don't pad last upload part; Support skipping padded part in streamer. * Reconstruct encrypted uploads from agent. * Concatenate upload parts into a single upload part instead of restructuring and combining them. * Make iterator buffer safe. * Remove padding from 128kb parts during concatenation. * Fix padding through gRPC service. * Cleanup; Fix discarded padding logic. * Add tests. * Fix bad upload test. * Expand test, add comments. * Minor edits. * Add padding helper with test; Add comments. * Fix tests, lint. * Don't use TELEPORT_UNSTABLE_GRPC_RECV_SIZE. * Apply suggestions from code review Co-authored-by: Erik Tate <erik.tate@goteleport.com> --------- Co-authored-by: Erik Tate <erik.tate@goteleport.com>
* Add necessary padding to encrypted recordings for s3. * Add padded part instead of editing existing part; Don't pad last upload part; Support skipping padded part in streamer. * Reconstruct encrypted uploads from agent. * Concatenate upload parts into a single upload part instead of restructuring and combining them. * Make iterator buffer safe. * Remove padding from 128kb parts during concatenation. * Fix padding through gRPC service. * Cleanup; Fix discarded padding logic. * Add tests. * Fix bad upload test. * Expand test, add comments. * Minor edits. * Add padding helper with test; Add comments. * Fix tests, lint. * Don't use TELEPORT_UNSTABLE_GRPC_RECV_SIZE. * Apply suggestions from code review Co-authored-by: Erik Tate <erik.tate@goteleport.com> --------- Co-authored-by: Erik Tate <erik.tate@goteleport.com>
Changelog: Fix a bug where encrypted sessions recordings could not be uploaded to S3.
S3 buckets enforce a 5MB minimum for multipart uploads, but currently Encrypted recordings are uploaded in 128kb parts due to technical limitations.
This PR fixes this error by:
PartSize=0 && PaddingSize=1MBThis is a short term fix as the 1MB of padding per upload part is not ideal. It is also tangentially related to encrypted recordings not being processed for session summaries, metadata, or thumbnails. Note that due to the lack of metadata, this bug also results in encrypted recordings being unplayable in the WebUI in v18.2.3+. A more complete fix will be handled in a follow up PR.
Manual Testing (34f1f23):
tsh playand checked that it was the expected size in the S3 bucket.[ ] SettingTELEPORT_UNSTABLE_GRPC_RECV_SIZE=5MBshould result in the 1MB padding being skipped, as the client will create 5MB aggregated upload parts itself.[ ] SettingTELEPORT_UNSTABLE_GRPC_RECV_SIZE=5MBshould result in the 1MB padding being skipped, as the client will create 5MB aggregated upload parts itself.