feat: Support S3 batch export encryption #17401
Conversation
tomasfarias
changed the title
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
tomasfarias
force-pushed
the
feat/s3-batch-exports-encryption
branch
from
530503c to
1e16446
Compare
|
Rebased on |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
tomasfarias
force-pushed
the
feat/s3-batch-exports-encryption
branch
from
767c964 to
91ed465
Compare
|
Rebased on |
| upload_id: str = multipart_response["UploadId"] | ||
| self.upload_id = upload_id |
There was a problem hiding this comment.
why not
| upload_id: str = multipart_response["UploadId"] | |
| self.upload_id = upload_id | |
| self.upload_id = multipart_response["UploadId"] |
There was a problem hiding this comment.
Type checking fails as self.upload_id is str | None and this function returns str, not None, so we need the extra upload_id: str to return
There was a problem hiding this comment.
shouldn't that work just fine - if it's str | None you can assign it str, I'm confused
There was a problem hiding this comment.
You can assign it str but then you cannot return it, so you need the extra variable. Not the only way to solve this though, you could have some isinstance checks, adding an extra variable seemed like the easiest
Co-authored-by: Tiina Turban <[email protected]>
tomasfarias
force-pushed
the
feat/s3-batch-exports-encryption
branch
from
7e2dc1b to
2981ba9
Compare
|
Rebased on |
Problem
S3 batch export cannot be configured to support encryption, which is required by users.
Changes
None, so we have to conditionally setServerSideEncryption, andSSEKMSKeyId. Not super happy how this turned out, but for only two arguments this should be fine. Happy to take comments.kms_key_idform field is conditionally added and required ifaws:kmsencryption is selected.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?
Testing is hard as our development setup with MinIO is not configured with a KMS. So, what I did was write a new unit test for encryption that checks if AWS credentials are available and that the
S3_TEST_BUCKETenvironment variable is set. This allows developers to test encryption locally by:S3_TEST_BUCKET=your-test-bucket DEBUG=1 pytest posthog/temporal/tests/batch_exports/test_s3_batch_export.py::test_s3_export_workflow_with_s3_bucket