flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

Add s3 server_side_encryption config

Open pingsutw opened this issue 2 years ago • 2 comments

TL;DR

Users failed to write the data to the bucket because they are enable Serverside Encryption

https://github.com/fsspec/s3fs/pull/90/files Add support passing server_side_encryption to s3fs

Type

  • [x] Bug Fix
  • [ ] Feature
  • [ ] Plugin

Are all requirements met?

  • [x] Code completed
  • [ ] Smoke tested
  • [ ] Unit tests added
  • [ ] Code documentation added
  • [ ] Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

NA

Follow-up issue

NA

pingsutw avatar Oct 11 '23 19:10 pingsutw

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3afcbb4) 55.05% compared to head (f631975) 55.05%. Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1887      +/-   ##
==========================================
- Coverage   55.05%   55.05%   -0.01%     
==========================================
  Files         296      296              
  Lines       22254    22258       +4     
  Branches     3358     3359       +1     
==========================================
+ Hits        12253    12255       +2     
- Misses       9838     9839       +1     
- Partials      163      164       +1     
Files Coverage Δ
flytekit/configuration/__init__.py 73.57% <100.00%> (+0.15%) :arrow_up:
flytekit/configuration/internal.py 82.75% <100.00%> (+0.20%) :arrow_up:
flytekit/core/data_persistence.py 38.50% <0.00%> (-0.39%) :arrow_down:

... and 1 file with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 11 '23 19:10 codecov[bot]

Looking at this a little closer, I see that https://github.com/flyteorg/stow/blob/master/s3/config.go#L136-L141 doesn't allow this to be set either... I think it will need to be changed to do something like https://github.com/aws/aws-sdk-go/blob/63e7f600c268b0ef0c1e700b956097f4b18795f9/service/s3/s3manager/upload_test.go#L127-L128 to support it in the Go code as well

ddl-ebrown avatar Jan 22 '24 18:01 ddl-ebrown