Allow specifying aws_s3_bucket_ownership_controls #109
Conversation
max-lobur
force-pushed
the
ownership
branch
5 times, most recently
from
8bed945 to
237146d
Compare
max-lobur
force-pushed
the
ownership
branch
5 times, most recently
from
66d3987 to
a60ff28
Compare
|
/test all |
Co-authored-by: nitrocode <[email protected]>
Co-authored-by: nitrocode <[email protected]>
| } | ||
|
|
||
| # Workaround S3 eventual consistency for settings objects | ||
| resource "time_sleep" "wait_for_aws_s3_bucket_settings" { |
There was a problem hiding this comment.
can we set the name as default like all the others?
| resource "time_sleep" "wait_for_aws_s3_bucket_settings" { | |
| resource "time_sleep" "default" { |
There was a problem hiding this comment.
Will leave as is, as its more readable. Also I'm afraid there will be more sleeps
There was a problem hiding this comment.
S3 went a weird way introducing a separate api/resource for every setting, which then get merged on the backend and races occur
|
|
||
| # Per https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html | ||
| resource "aws_s3_bucket_ownership_controls" "default" { | ||
| count = local.enabled ? 1 : 0 |
There was a problem hiding this comment.
what if users do not want to specify the s3 bucket ownership controls? can we make this toggleable and default it to false to maintain backwards compatibility?
There was a problem hiding this comment.
It's always specified implicitly to the value which is the default here. So creating this with default is equal to not creating at all
| # Workaround S3 eventual consistency for settings objects | ||
| resource "time_sleep" "wait_for_aws_s3_bucket_settings" { | ||
| count = local.enabled ? 1 : 0 | ||
| depends_on = [aws_s3_bucket_public_access_block.default, aws_s3_bucket_policy.default] |
There was a problem hiding this comment.
what happens when the sleep is removed? does s3 bucket ownership controls resource error out?
There was a problem hiding this comment.
yes S3 api race condition
There was a problem hiding this comment.
also when destroying, the aws_s3_bucket_public_access_block resource errors out - a reverse race
| count = local.enabled ? 1 : 0 | ||
| depends_on = [aws_s3_bucket_public_access_block.default, aws_s3_bucket_policy.default] | ||
| create_duration = "30s" | ||
| destroy_duration = "30s" |
There was a problem hiding this comment.
if time sleep has to be used here, can we make these configurable?
There was a problem hiding this comment.
I have doubts it's needed. The test covers this pretty well, if it ever fails we'd notice that. In my own tests even 3s was enough to avoid it, this should be bulletproof. I crashes when you have two near-simultaneous requests
There was a problem hiding this comment.
I wouldn't want people to experiment with increasing timeouts if they have errors related to this. 99% this will be another new resource race in S3, not the timeout itself.
|
/test all |
|
/test all |
mergify
bot
dismissed
nitrocode’s stale review
This Pull Request has been updated, so we're dismissing all reviews.
what
why
bucket-owner-full-controlACL is specified, the bucket account take ownership, otherwise the writer account keeps ownership. Bucket on its side may enforce presence of thebucket-owner-full-controlACL which we already do when needed. So this setting was the only missing piece to make ownership work like we expectedObjectWriter. It corresponds to legacy S3 behavior which was broken for us.bucket-owner-full-controlin their request and then still expect to own the object. To preserve legacy behavior I made this a variable, and the default corresponds to S3 legacy behavior. This is a new feature of AWS and we should wait for some time before enforcing the new default.references