wrap s3 calls with retry to avoid race in s3 bucket creation calls

[xchapter7x]

issue: [https://github.com//issues/877]

[xchapter7x]

tested locally with:

TF_ACC=1 go test ./aws -run TestAccAWSS3MultiBucket_withTags -v

[abbyachau]

hi @radeksimko, could you please give us an indication of when this pr will be reviewed? thanks!

[radeksimko]

@xchapter7x thank you for raising the PR & issue.

While I believe there are some eventual consistency issues that need addressing in S3 I'm in doubts whether this is the right place as I'm not sure the error in #877 is coming from that specific place.

I did notice we get a bunch of test failures once in a while in our nightly acceptance tests, but the error usually came from a followup API call, e.g. setting policy, CORS, website, versioning, ...

Would you mind providing debug logs we can go through to understand the nature of the problem better (i.e. find out which API request is actually returning the error)?

TL;DR I believe that this is a real problem which needs fixing, but I'm not convinced this PR is actually fixing it. Maybe it is, but we need more data before moving forward.

Thanks.

[xchapter7x]

@radeksimko
You are correct about the errors coming from those followup API calls. We are seeing the same behavior (intermittently). This was the reasoning behind putting a wait before that follow on call, so it will block until the bucket resource is actually available for the following calls, avoiding any race.

Let me spin up an environment to replicate the issue and get you the requested logs.
Stay tuned.

[radeksimko]

This was the reasoning behind putting a wait before that follow on call, so it will block until the bucket resource is actually available for the following calls, avoiding any race.

I'm afraid we'll have to add retries to each followup call as S3 is eventually consistent and the API doesn't do client pinning, AFAIK - which means that 1st success (200 OK) does not mean the change (new bucket) was propagated fully. I think we might still get NoSuchBucket from any API call which follows one that succeeded. At least this is a behaviour I did observe number of times in IAM and KMS.

In fact KMS was so badly affected by this that I had to implement "guessing" into the retry logic:
hashicorp/terraform#4447

btw. all this behaviour is pretty much undocumented, it's just what I remember from my past experience with the AWS APIs and it's hard to test. 😄

I hope that's not too discouraging - I'd love to get such intermittent issues fixed and I understand it's tricky to do so.

Regarding the code:

• we have helpers (resource.Retry, resource.StateChangeConf) for retries which are used throughout most of the codebase and we'd probably prefer those over anything else, unless there's a good reason to use AWS SDK's ones
• we tend to use a simple fmt.Sprintf instead of templates, unless there's a good reason to use templates

[xchapter7x]

i have a large debug output log, tfplan & tfstate all reproducing the failure.
there might be some passwords and things, so i'd rather not post here. What's the best way to get you the files (email, secure gist, slack...)?

In regards to the feedback I'll take a look at implementing resource.Retry and others suggested instead of direct sdk calls.

I decided to use the template instead of Sprintf, b/c it was a lot larger than the other use cases in the tests that i saw, and made the implementation a bit more readable/manageable. Happy to switch it to use sprintf, if that is a more consistent implementation.

[radeksimko]

@xchapter7x feel free to send me the debug log to radek@hashicorp.com You can also GPG encrypt it using my pubkey published at https://keybase.io/radeksimko

Re templating - Admittedly the nice thing about templates is that we get a crash straight away if we can't compile the template (e.g. forgotten placeholder/variable), whereas we get invalid tf config with %d or %!(EXTRA string=variable) when using Sprintf. That said I'd be worried about potential unwanted HTML escaping and other behaviour as this is supposed to be used for HTML templates. How about we use text/template then? It's still one extra import, but I can see the benefit as I did run into invalid templates caused by Sprintf in the past. We could decouple this into a function in a separate file perhaps - to avoid that extra import.

[xchapter7x]

I sent everything over in keybase. Let me know if you're able to find anything.

Im going to switch this PR back to WIP and, taking into account your feedback, make some changes. Ill let you know when thats done.

Thanks for your continued support in working through this.

[radeksimko]

Thanks for providing the debug log, that's very useful. 👌

On first sight there seems to be quite a lot of 404s there, even s3/HeadBucket. 😞
I'll see if I can make a head and tail of this with some help of unix filtering tools, so we just get a sequence of API calls and error codes, ideally.

Based on the log it seems worse (404 appearing more often) than anything I've seen so far. On the positive side the worse the situation is the easier it should be to fix it. 😄

[xchapter7x]

hi @radeksimko sorry for the long silence on this. I got pulled away. I found some time to revisit this today and I have a question around the resource.Retry functions currently in use that you suggested.

If I'm understanding the use case properly, this resource.Retry function is intended to wrap a call to the s3 client and provide retry logic in the event of an error.

However, the client calls currently being wrapped provide a request.Retryer in the client struct. This built in request.Retryer seems intended to be used in similar scenarios as the resource.Retry wrapper used throughout the terraform codebase.

Im curious why one would choose to use resource.Retry over the provided request.Retryer interface?

Specific to this PR, would it be an acceptable implementation to leverage request.Retryer instead? I ask b/c it doesn't seem to fit with the current implementation and wouldn't want to invest effort in a divergent, undesired path.

Your thoughts and feedback are greatly appreciated. Thanks.

[xchapter7x]

hi @radeksimko , sorry for the long delay. This should be ready to be reviewed. Let me know your thoughts and feedback. Thanks.

[radeksimko]

LGTM, thanks for the patience.

[abbyachau]

Thanks @radeksimko very much. Do you know which release this fix will be included on please?

[esilva-everbridge]

Have these changes been released? I'm seeing this issue with 0.11.1

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!