-
Notifications
You must be signed in to change notification settings - Fork 670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[draft] disable etag verify for S3 buckets #4931
Conversation
Signed-off-by: ddl-rliu <[email protected]>
This reverts commit 1110d29. Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
logger.Infof(ctx, "file already exists at location [%v], specify a matching hash if you wish to rewrite", knownLocation) | ||
log.Printf("Skipping error :^) Continuing...") | ||
logger.Infof(ctx, "Skipping error :^) Continuing...") | ||
// return nil, errors.NewFlyteAdminErrorf(codes.AlreadyExists, "file already exists at location [%v], specify a matching hash if you wish to rewrite", knownLocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only relevant change in this PR - poc disable this check and verified that file upload to kms bucket now works.
TODO: gate this behind some check.
- [Most likely] Option 1. Selectively disable the check for S3 buckets, since it seems S3 already implements this logic to "deny request if md5 doesn't match"
- [???] Option 2. Selectively disable the check when the bucket is KMS encrypted (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caveat: #2328 flyteorg/flyteadmin#587
See #4971 |
Tracking issue
https://github.com/flyteorg/flyte/issues/
Why are the changes needed?
Needed to allow KMS buckets to work with S3. See https://docs.gitlab.com/ee/administration/object_storage.html#etag-mismatch for a summary of the issue, and why the current md5=etag check causes uploads to KMS-encrypted buckets to fail.
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link