Skip to content
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

backend/s3: Configure AWS Client MaxRetries and provide enhanced S3 NoSuchBucket error message #20338

Closed
wants to merge 1 commit into from

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Feb 14, 2019

Closes #19992

This is a backport of commit (ed37d07) which was merged into master via #19951 and was to resolve a fairly common issue as noted there.

The AWS Go SDK automatically provides a default request retryer with exponential backoff that is invoked via setting MaxRetries or leaving it nil will default to 3. The terraform-aws-provider config.Client() sets MaxRetries to 0 unless explicitly configured above 0. Previously, we were not overriding this behavior by setting the configuration and therefore not invoking the default request retryer.

The default retryer already handles HTTP error codes above 500, including S3's InternalError response, so the extraneous handling can be removed. This will also start automatically retrying many additional cases, such as temporary networking issues or other retryable AWS service responses.

Changes:

  • s3/backend: Add max_retries argument
  • s3/backend: Enhance S3 NoSuchBucket error to include additional information

…oSuchBucket error message

The AWS Go SDK automatically provides a default request retryer with exponential backoff that is invoked via setting `MaxRetries` or leaving it `nil` will default to 3. The terraform-aws-provider `config.Client()` sets `MaxRetries` to 0 unless explicitly configured above 0. Previously, we were not overriding this behavior by setting the configuration and therefore not invoking the default request retryer.

The default retryer already handles HTTP error codes above 500, including S3's InternalError response, so the extraneous handling can be removed. This will also start automatically retrying many additional cases, such as temporary networking issues or other retryable AWS service responses.

Changes:
* s3/backend: Add `max_retries` argument
* s3/backend: Enhance S3 NoSuchBucket error to include additional information
@apparentlymart
Copy link
Contributor

We've generally been considering v0.11 functionality to be frozen as we work towards the v0.12.0 release, with an exception for the new "remote" backend used by Terraform Enterprise as part of the free state storage rollout, so I'm a little reluctant to merge this for that reason.

However, since it appears that this change reduces the risk of a failed state write, and is thus a safety improvement for S3 backend users, I think we could make an exception if we're confident that this will not cause any visible change in behavior for those who were not experiencing this problem. I don't have enough context to make that call, but if you do then I'm happy to have this merged into the 0.11 branch, though note that 0.11 releases are currently being driven entirely by the free state storage feature rollout (since the Terraform Core team is 100% focused on 0.12.0 release preparation) and so we will need to wait until there's a release driven by that work before this fix would become available to v0.11 users.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with the provisos I mentioned in my previous comment.

bflad added a commit that referenced this pull request Mar 12, 2019
…ider-aws to github.com/hashicorp/aws-sdk-go-base

References:

* #20374
* #20338
* #19992

Backports the S3 backend code from master to v0.11 relating to the AWS Go SDK, which removes the circular dependency with the Terraform AWS Provider and adds retry logic to the backend to prevent temporary networking or AWS service issues from failing unnecessarily.

Updated via code change then:

```
go mod tidy
go mod vendor
```
@bflad
Copy link
Contributor Author

bflad commented Mar 13, 2019

Superseded by #20659 👍

@bflad bflad closed this Mar 13, 2019
@bflad bflad deleted the v0.11-backend-s3-maxretries-backport branch March 13, 2019 16:52
apparentlymart pushed a commit that referenced this pull request May 7, 2019
…ider-aws to github.com/hashicorp/aws-sdk-go-base

References:

* #20374
* #20338
* #19992

Backports the S3 backend code from master to v0.11 relating to the AWS Go SDK, which removes the circular dependency with the Terraform AWS Provider and adds retry logic to the backend to prevent temporary networking or AWS service issues from failing unnecessarily.

Updated via code change then:

```
go mod tidy
go mod vendor
```
@ghost
Copy link

ghost commented Aug 13, 2019

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants