r/aws_sns_topic: Provider now errors with fake iam role

[mbfrahry]

This PR is a migration from hashicorp/terraform#14226 and fixes #738.

make testacc TEST=./aws TESTARGS="-run=TestAccAWSSNSTopic_withFakeIAMRole"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopic_withFakeIAMRole -timeout 120m
=== RUN   TestAccAWSSNSTopic_withFakeIAMRole
--- PASS: TestAccAWSSNSTopic_withFakeIAMRole (8.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	8.375s

[Ninir]

Hi @mbfrahry

Seems good to me! 👍
Ran all the SnsTopic* tests, and found a small issue:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopic -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_importBasic
--- PASS: TestAccAWSSNSTopicSubscription_importBasic (43.31s)
=== RUN   TestAccAWSSNSTopic_importBasic
--- PASS: TestAccAWSSNSTopic_importBasic (32.88s)
=== RUN   TestAccAWSSNSTopicPolicy_basic
--- PASS: TestAccAWSSNSTopicPolicy_basic (30.70s)
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (42.49s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (76.98s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (116.68s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (34.14s)
=== RUN   TestAccAWSSNSTopic_policy
--- PASS: TestAccAWSSNSTopic_policy (34.95s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- FAIL: TestAccAWSSNSTopic_withIAMRole (36.00s)
	testing.go:435: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_sns_topic.test_topic: 1 error(s) occurred:
		
		* aws_sns_topic.test_topic: InvalidParameter: Invalid parameter: Policy Error: PrincipalNotFound
			status code: 400, request id: 653c854c-bdec-5599-8f12-d76d0c373c3e
=== RUN   TestAccAWSSNSTopic_withFakeIAMRole
--- PASS: TestAccAWSSNSTopic_withFakeIAMRole (21.62s)
=== RUN   TestAccAWSSNSTopic_withDeliveryPolicy
--- PASS: TestAccAWSSNSTopic_withDeliveryPolicy (22.23s)
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	491.98s
make: *** [testacc] Error 1

Can you fix the related test? thanks!

[radeksimko]

I agree the error message mentioned in #738 is very confusing, but we have the retry logic in place because IAM is eventually consistent, so that needs to stay there (i.e. we still need to retry, not stop on first error which may just be an effect of delayed replication).

The ideal solution would be to just replace the unnecessarily complex resource.StateChangeConf which is lacking support for "retryable errors" and other things we need here and requires pending/target state to be a thing (which we're just making up here to satisfy the interface).

resource.Retry is IMO better suited for this job: https://github.com/hashicorp/terraform/blob/master/helper/resource/wait.go#L53

Let me know what you think.

[catsby]

Returning err here instead of nil will cause this WaitForState method to quit immediately. We have to return nil here because the intent is to retry right here:

https://github.com/terraform-providers/terraform-provider-aws/blob/068ff1beeb935dc6034b52a6ee38cef9f6fd60bd/vendor/github.com/hashicorp/terraform/helper/resource/state.go#L41

I agree with Radek, reworking this into resource.Retry may be better. I realize this isn't your code or change so let me know if you want to hand it off. Until StateChangeConf can surface "error before last", we're in this spot.

[mbfrahry]

Alright, I swapped it to use resource.Retry and got all passing tests.

make testacc TEST=./aws TESTARGS="-run=TestAccAWSSNSTop"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTop -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_importBasic
--- PASS: TestAccAWSSNSTopicSubscription_importBasic (17.87s)
=== RUN   TestAccAWSSNSTopic_importBasic
--- PASS: TestAccAWSSNSTopic_importBasic (14.94s)
=== RUN   TestAccAWSSNSTopicPolicy_basic
--- PASS: TestAccAWSSNSTopicPolicy_basic (13.65s)
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (16.03s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (47.91s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (74.01s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (13.92s)
=== RUN   TestAccAWSSNSTopic_policy
--- PASS: TestAccAWSSNSTopic_policy (12.64s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- PASS: TestAccAWSSNSTopic_withIAMRole (32.41s)
=== RUN   TestAccAWSSNSTopic_withFakeIAMRole
--- PASS: TestAccAWSSNSTopic_withFakeIAMRole (69.48s)
=== RUN   TestAccAWSSNSTopic_withDeliveryPolicy
--- PASS: TestAccAWSSNSTopic_withDeliveryPolicy (12.96s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	325.855s

[mbfrahry]

Thanks for pointing me towards that retry function!

[radeksimko]

Thanks for making those changes, retryOnAwsCode is probably even better.

Do you mind replacing this in the two other operations (resourceAwsSnsTopicPolicyUpsert & resourceAwsSnsTopicPolicyDelete) too & removing resourceAwsSNSUpdateRefreshFunc altogether?

[mbfrahry]

Alright! It's all gone and tests are passing

[radeksimko]

LGTM, thanks.

[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!