Fix SNS subscription sub/unsub/sub bug.

[handlerbot]

The Terraform SNS subscriptions implementation has a bug where it:

• creates a new subscription
• verifies the subscription as created
• evaluates the newly created subscription object
• sees that some of the fields of the object have "changed" (this is technically correct, strings have changed from their initial empty value of "" to the values configured by the Terraform configuration)
• decides that the subscription configuration has been "updated"
• destroys the subscription and recreates it!

I noticed this when implementing SNS subscription auto-confirmation in our servers; when I created a new SNS subscription via Terraform, my server got two SNS subscription confirmation requests when I only expected one.

With TF_LOG=debug, the AWS provider at HEAD shows these outgoing requests:

[DEBUG] [aws-sdk-go] DEBUG: Request sns/Subscribe Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/ListSubscriptionsByTopic Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/Unsubscribe Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/Subscribe Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/ListSubscriptionsByTopic Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/GetSubscriptionAttributes Details:  

I instrumented the AWS provider to show what it thought was happening with the protocol, endpoint, and topic_arn fields:

[DEBUG] protocol original , new https
[DEBUG] endpoint original , new https://<mydomain>/<myurl>
[DEBUG] topic_arn original , new arn:aws:sns:<region>:<account_id>:<topic_name>

With my PR, these are the outgoing requests I see:

[DEBUG] [aws-sdk-go] DEBUG: Request sns/Subscribe Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/ListSubscriptionsByTopic Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/ListSubscriptionsByTopic Details:
[DEBUG] [aws-sdk-go] DEBUG: Request sns/GetSubscriptionAttributes Details:

It's arguable that something like wasSetAndHasChange belongs upstream in https://github.com/hashicorp/terraform/blob/master/helper/schema/resource_data.go but I'd rather get this fixed here first, and then we can assess whether that is worth trying to uplift to main Terraform.

[radeksimko]

Thanks for the catch! This looks like a reasonable bugfix - I just left you one comment about simplicity.

I'm assuming we can't test the bugfix as the difference between old/new behaviour is only visible from log?

[radeksimko]

I think we can avoid adding the wrapper function and use d.IsNewResource() instead here.

e.g.

if !d.IsNewResource() && (d.HasChange("protocol") || d.HasChange("endpoint") || d.HasChange("topic_arn")) {

[handlerbot]

@radeksimko Thx for getting back so quick. :-) Revised as requested, and TY for the suggestion; I don't know the schema object functions well enough to have known about IsNewResource() yet...

Re: tests, I've tested it live in my AWS development environment multiple times, including the revised change, and it does the right thing. The existing tests for the module didn't pick this up because it only tests for successful creation of the resource at the end of complete execution, and the create/destroy/create bug does result in successful creation, except with a little extra work in the middle. The testing framework does not look conducive to adding a test to prevent against this recurring without a bunch of refactoring, and I bet this kind of bug is a (potentially) widespread issue in the AWS provider...

[radeksimko]

LGTM, thanks for making the requested change.

I bet this kind of bug is a (potentially) widespread issue in the AWS provider...

I'm not sure if I understand correctly what do you mean by "this kind of bug" in a wider context. 🤔

Almost each service API often works differently in AWS and I think SNS is fairly unique with its (potentially destructive) "Subscribe/Unsubscribe" method.

Some other resources/APIs have Put* (which cover both create & update), some have Ensure*, some Update* and in most of these cases it's quite harmless to call such methods during both updates & creation.

Please do let us know if you're aware of similar bugs in other resources by submitting new issues. Thanks.

[stevehorsfield]

👍

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