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.

Sign up for GitHub

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

Jump to bottom

resource/aws_sqs_queue_policy: Prevent missing policy error on read #2739

Merged
merged 2 commits into from
Jan 22, 2018
Merged

resource/aws_sqs_queue_policy: Prevent missing policy error on read #2739

merged 2 commits into from
Jan 22, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Dec 21, 2017

SQS attribute creation/updates can take up to 60 seconds for propagation. This PR addresses two facets:

  • Most importantly, new queue policies can currently error out when the new Policy attribute may not have propagated everywhere yet, which is a bad user experience. Instead of raising error, we can just set the policy to empty string in the state. Likely the next read will pick up the correct policy.
  • To help mitigate eventual consistency issues, add a resource.StateChangeConf on upsert to try to wait for the new/updated policy. Arguably this could be removed or moved into the read as well.

Closes #2669

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSQSQueuePolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSQSQueuePolicy -timeout 120m
=== RUN   TestAccAWSSQSQueuePolicy_basic
--- PASS: TestAccAWSSQSQueuePolicy_basic (17.68s)
=== RUN   TestAccAWSSQSQueuePolicy_import
--- PASS: TestAccAWSSQSQueuePolicy_import (16.96s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	34.673s

@bflad bflad mentioned this pull request Dec 21, 2017
Closed
@jen20 jen20 added the bug Addresses a defect in current functionality. label Dec 21, 2017
radeksimko
radeksimko reviewed Jan 8, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just one question to consider - otherwise the PR looks reasonable.

Arguably this could be removed or moved into the read as well.

I would not put it there. We have very little control over where is method going to be called from - e.g. on refresh - in such case it's desirable to show difference when the resource was modified outside of Terraform.

aws/resource_aws_sqs_queue_policy.go Outdated
return out, "", nil
},
}
_, err = wait.WaitForState()
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using the resource.Retry helper instead of this wrapper? We'd still need to make up the error message when policies aren't equivalent, but I feel that it'd be a little bit cleaner as the whole thing isn't really "stateful". 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Also we're catching all errors here and treating them as "retryable" which (IMO) isn't ideal. I assume the goal is to only catch AWS.SimpleQueueService.NonExistentQueue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on both of these -- I'll get this PR updated shortly.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2018
@radeksimko radeksimko added the service/sqs Issues and PRs that pertain to the sqs service. label Jan 16, 2018
@radeksimko radeksimko changed the title r/aws_sqs_queue_policy: Add StateChangeConf for create/update, remove missing policy error on read resource/aws_sqs_queue_policy: Add StateChangeConf for create/update, remove missing policy error on read Jan 16, 2018
@bflad
resource/aws_sqs_queue_policy: Use resource.Retry instead of resource…
91a723f 
….StateChangeConf

Additionally:
* Supplement debug logging
* Use sqs.QueueAttributeNamePolicy constant
@bflad bflad changed the title resource/aws_sqs_queue_policy: Add StateChangeConf for create/update, remove missing policy error on read resource/aws_sqs_queue_policy: Prevent missing policy error on read Jan 18, 2018
@bflad
Copy link
Contributor Author

bflad commented Jan 18, 2018

Updated 😄

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSQSQueuePolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSQSQueuePolicy -timeout 120m
=== RUN   TestAccAWSSQSQueuePolicy_basic
--- PASS: TestAccAWSSQSQueuePolicy_basic (14.96s)
=== RUN   TestAccAWSSQSQueuePolicy_import
--- PASS: TestAccAWSSQSQueuePolicy_import (15.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	30.173s

@bflad bflad requested a review from radeksimko January 18, 2018 19:42
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 19, 2018
radeksimko
radeksimko approved these changes Jan 22, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 👍

@radeksimko radeksimko added this to the v1.8.0 milestone Jan 22, 2018
@bflad bflad merged commit 0047f61 into hashicorp:master Jan 22, 2018
@bflad bflad deleted the sqs-queue-policy-eventual-consistency branch January 22, 2018 13:49
bflad added a commit that referenced this pull request Jan 22, 2018
@bflad
Update CHANGELOG for #2739
93f016a
@bflad
Copy link
Contributor Author

bflad commented Jan 29, 2018

This has been released in terraform-provider-aws version 1.8.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 8, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/sqs Issues and PRs that pertain to the sqs service.
Projects
None yet
Milestone
v1.8.0
Development

Successfully merging this pull request may close these issues.

SQS Queue created without Policy cannot have policy attached
3 participants
@bflad @radeksimko @jen20