Added support for http/https endpoints that auto confirms SNS topic subscription. #4711

srikalyan · 2016-01-17T20:57:26Z

Added support for http/https endpoints that auto confirms SNS topic subscription.

http and https SNS topic subscription endpoints require confirmation to set a valid arn otherwise
arn would be set to "pending confirmation". If the endpoints auto confirm then arn is set
asynchronously but if we try to create another subscription with same parameters then api returns
"pending subscription" as arn but does not create another a duplicate subscription. In order to
solve this we should be fetching the subscription list for the topic and identify the subscription
with same parameters i.e., protocol, topic_arn, endpoint and extract the subscription arn.

Following changes were made to support the http/https endpoints that auto confirms

Added 3 extra parameters i.e.,
1. endpoint_auto_confirms -> boolean indicates if end points auto confirms
2. max_fetch_retries -> number of times to fetch subscription list for the topic to get the subscription arn
3. fetch_retry_delay -> delay b/w fetch subscription list call as the confirmation is done asynchronously.
With these parameters help added support http and https protocol based endpoints that auto confirm.
Update website doc appropriately

…ubscription. http and https SNS topic subscription endpoints require confirmation to set a valid arn otherwise arn would be set to "pending confirmation". If the endpoints auto confirm then arn is set asynchronously but if we try to create another subscription with same parameters then api returns "pending subscription" as arn but does not create another a duplicate subscription. In order to solve this we should be fetching the subscription list for the topic and identify the subscription with same parameters i.e., protocol, topic_arn, endpoint and extract the subscription arn. Following changes were made to support the http/https endpoints that auto confirms 1. Added 3 extra parameters i.e., 1. endpoint_auto_confirms -> boolean indicates if end points auto confirms 2. max_fetch_retries -> number of times to fetch subscription list for the topic to get the subscription arn 3. fetch_retry_delay -> delay b/w fetch subscription list call as the confirmation is done asynchronously. With these parameters help added support http and https protocol based endpoints that auto confirm. 2. Update website doc appropriately

srikalyan · 2016-01-17T21:00:29Z

builtin/providers/aws/resource_aws_sns_topic_subscription.go

+ max_fetch_retries := d.Get("max_fetch_retries").(int)
+ fetch_retry_delay := time.Duration(d.Get("fetch_retry_delay").(int))
+
+ if strings.Contains(protocol, "http") && !endpoint_auto_confirms {


I really like to move this to validateFunc but I need a function which can give me whole map. I need help on this.

Yeah, I see your point. This will be eventually possible after merging #3641 but I wouldn't want to block this PR on that.

The only suggestion I have regarding this is that we should rather be comparing protocols as IDs (i.e. (protocol == "http" || protocol == "https")), not arbitrary strings.

I almost thought on first sight you forgot about https.
Also there might be a new service in the future which will actually contain http in the name and that would make things even more confusing.

Yeah, I agree that there might be newer protocol which has .http. or .email. etc in it but then I saw logic around line 33 and thought that I should follow on the similar footsteps just to be consistent.

I guess the logic around line 33 should be changed too then... if you want to do it, go ahead, but I won't push back on this PR if you won't. It would be nice to get it consistent (and only use equal sign).

srikalyan · 2016-01-17T21:03:27Z

My golang skills are quite minimal (I did a quick turotial and added this PR) so please let me know if you see any problem or something needs to be updated.

…ion subscription is returned.

"pending confirmation" such as "PendingConfirmation", "Pending Confirmation" etc.

srikalyan · 2016-01-20T17:29:32Z

@packplusplus, thats what my intention was. Am I missing something to get this pulled?

packplusplus · 2016-01-20T18:02:50Z

/me is embarrassed

At the time I searched and didn't see a PR (I commented on the fork not the pr directly).

srikalyan · 2016-01-25T18:10:14Z

@jen20, What is the usual turn around time for PRs?

srikalyan · 2016-02-02T23:17:40Z

Guys any update on this?

radeksimko · 2016-02-10T13:22:28Z

Hi @srikalyan
it would be nice to get this more automated somehow.

Can you provide any public examples of services which provide the auto-confirmation? I was hoping that Slack would be able to do this, but apparently not. 😢

From what I've read the SNS subscription confirmation works in a way that it requires the other end to call AWS API with a token that was sent to the provided HTTP(S) endpoint. This inherently means that the other end needs AWS access keys in order to call the API and confirm the subscription. These keys need to have the privilege to call Sns:ConfirmSubscription API method on that specific subscription ARN which would be created by Terraform.

Am I right?

srikalyan · 2016-02-11T06:48:34Z

Hello @radeksimko,

There are quite a few public services that auto confirm subscription for e.g. Pagerduty. Cloud watch events can be sent via SNS topic to Pagerduty service which can call proper members.

As far as I can tell, we third party don't need AWS access keys and I definitely tested it with Pagerduty.

Hope I've answered your questions and please let me know if need anything else.

radeksimko · 2016-02-11T07:51:43Z

I see, thanks for the example, that's very helpful.

I searched thru AWS Docs again to find more details about this and found this page:
http://docs.aws.amazon.com/sns/latest/dg/SendMessageToHttp.html#SendMessageToHttp.prepare

I think we should add a link to the docs and probably also mention PagerDuty as an example e.g.:

endpoint_auto_confirms - (Optional) Boolean indicating whether the end point is capable of auto confirming subscription, e.g. PagerDuty (default is false)

I will give this a full review in couple of days.

…ields added in the PR and add the link to amazon describing the auto confirming subscription.

srikalyan · 2016-02-11T17:26:19Z

@radeksimko,

I've updated the documentation as per your suggestion. Please let me know if you have any more suggestions for this PR.

radeksimko · 2016-02-12T09:56:39Z

builtin/providers/aws/resource_aws_sns_topic_subscription.go

+ "endpoint_auto_confirms": &schema.Schema{
+ Type: schema.TypeBool,
+ Optional: true,
+ ForceNew: false,


I believe ForceNew: false is a default behaviour so we shouldn't need to specify it here.

which applies to the other 2 fields below too.

I see your point but almost all the schema elements have ForceNew: false, I guess they made a bad copy :). I will fix it.

Not everything you see elsewhere in the codebase is worth copying, yet many things do! 😃

radeksimko · 2016-02-12T10:59:06Z

It is probably impossible to write a reliable acceptance test for this, unless we just hardcode a PagerDuty URL which would be generated manually I assume, so I won't insist on having one. 😃

I tried testing this manually with the following simple config:

resource "aws_sns_topic" "user_updates" {
  name = "cw-sns-topic"
}

resource "aws_sns_topic_subscription" "user_updates_sqs_target" {
    topic_arn = "${aws_sns_topic.user_updates.arn}"
    protocol  = "https"
    endpoint_auto_confirms = true
    endpoint  = "https://events.pagerduty.com/adapter/cloudwatch_sns/v1/*REDACTED*"
}

and in my first attempt Terraform crashed, here's the interesting part of the crash log:

panic: runtime error: invalid memory address or nil pointer dereference
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: [signal 0xb code=0x1 addr=0x0 pc=0x1de3ee]
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: goroutine 113 [running]:
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: github.com/hashicorp/terraform/builtin/providers/aws.resourceAwsSnsTopicSubscriptionUpdate(0x821f92540, 0x96dfc0, 0x821d8e000, 0x0, 0x0)
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:     /Users/radek/gopath/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_sns_topic_subscription.go:130 +0x8ce
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: github.com/hashicorp/terraform/builtin/providers/aws.resourceAwsSnsTopicSubscriptionCreate(0x821f92540, 0x96dfc0, 0x821d8e000, 0x0, 0x0)
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:     /Users/radek/gopath/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_sns_topic_subscription.go:110 +0x379
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0x821e75580, 0x821d2ef30, 0x822137840, 0x96dfc0, 0x821d8e000, 0x10101, 0x0, 0x0)
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:     /Users/radek/gopath/src/github.com/hashicorp/terraform/helper/schema/resource.go:145 +0x28e
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: github.com/hashicorp/terraform/helper/schema.(*Provider).Apply(0x821f17440, 0x821e33540, 0x821d2ef30, 0x822137840, 0x6d3055446e746f01, 0x0, 0x0)
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:     /Users/radek/gopath/src/github.com/hashicorp/terraform/helper/schema/provider.go:162 +0x1ed
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws: github.com/hashicorp/terraform/rpc.(*ResourceProviderServer).Apply(0x82213a900, 0x8220d22a0, 0x822137b30, 0x0, 0x0)
2016/02/12 10:09:29 [DEBUG] terraform-provider-aws:     /Users/radek/gopath/src/github.com/hashicorp/terraform/rpc/resource_provider.go:323 +0x76

it looks like there was no ARN returned in the response from API.

on my second attempt I just hit timeout (I assume because PagerDuty didn't confirm the subscription in 3 secs):

* aws_sns_topic_subscription.user_updates_sqs_target: Endpoint (https://events.pagerduty.com/adapter/cloudwatch_sns/v1/*REDACTED*) did not autoconfirm the subscription for topic arn:aws:sns:us-west-2:*REDACTED*:cw-sns-topic

One way to get around the timeout issue is to use the resource.Retry with exponential backoff instead of for loop, as I mentioned above.

Would you mind making the modifications I mentioned?

srikalyan · 2016-02-12T17:54:59Z

Yeah np. I will make them asap.

One question though, what did you mean when you said terraform crashed? Did you had any extra setting or is there a way that I can reproduce?

Thanks for helping me out :).

radeksimko · 2016-02-12T18:25:49Z

One question though, what did you mean when you said terraform crashed? Did you had any extra setting or is there a way that I can reproduce?

Nope, no special settings, if there is nil-dereference, Terraform just crashes and prints out the stack trace.

In terms of reproduction I guess it may be tricky with the AWS API... if sometimes it returns A and minutes after it returns B. 😞 I guess you'll have to either rerun an example couple of times until you hit the problem or just trust my crash log above.

1. Used resource.Retry instead of custom solution 2. Removed unnecessary variables and added required variable to resource.Retry.

srikalyan · 2016-02-12T20:34:48Z

@radeksimko,

I've made almost all the changes you have suggested except protocol contains check (as you have suggested it as optional ;). Also, I've tested it several times and could not reproduce the error stack trace :(.

Just curios though, what go version are you using?

I will keep an eye on it but It seems ok for now.

radeksimko · 2016-02-12T23:03:22Z

Thanks, this looks much better on the first sight!

I will give it another look during the weekend or next week.

srikalyan · 2016-02-13T00:43:37Z

Thanks @radeksimko for all your help :).

radeksimko · 2016-02-13T09:40:13Z

builtin/providers/aws/resource_aws_sns_topic_subscription.go

@@ -178,6 +187,12 @@ func subscribeToSNSTopic(d *schema.ResourceData, snsconn *sns.SNS) (output *sns.
 protocol := d.Get("protocol").(string)
 endpoint := d.Get("endpoint").(string)
 topic_arn := d.Get("topic_arn").(string)
+ endpoint_auto_confirms := d.Get("endpoint_auto_confirms").(bool)
+ confirmation_timeout_in_minutes := time.Duration(d.Get("confirmation_timeout_in_minutes").(int))


Is there any reason for the time.Duration wrapper here? I think that keeping it as integer should be ok.
It has no functional effect as I see you're actually multiplying this variable below with time.Minutes, but the default representation of time.Duration are nanoseconds which makes the variable name wrong.

If I don't convert it then I get the follow error
mismatched types time.Duration and int
On the other hand, I see your concern that the variable does not make sense
I will change it to
confirmation_timeout_in_minutes := d.Get("confirmation_timeout_in_minutes").(int)
and use time.Duration(int(time.Minute) * confirmation_timeout_in_minutes)

I have few other options too but I think this makes most sense.

I will change it to
confirmation_timeout_in_minutes := d.Get("confirmation_timeout_in_minutes").(int)
and use time.Duration(int(time.Minute) * confirmation_timeout_in_minutes)

That's exactly what I had in mind. 👍

radeksimko · 2016-02-13T10:19:02Z

Besides one nitpick about code readability I'd be happy to merge this. I tried rerunning my previous example and it worked aok.

Thanks for making all the modifications.

…ht value.

Added support for http/https endpoints that auto confirms SNS topic subscription.

ghost · 2020-04-28T02:02:18Z

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.

srikalyan reviewed Jan 17, 2016
View reviewed changes

Srikalyan Swayampakula added 2 commits January 18, 2016 14:00

Forgot to add one last condition to ensure that non pending confirmat…

63d6d8d

…ion subscription is returned.

Found an issue with more testing aws api is responding with various of

3d256dd

"pending confirmation" such as "PendingConfirmation", "Pending Confirmation" etc.

jen20 added enhancement provider/aws labels Jan 19, 2016

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 10, 2016

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 11, 2016

radeksimko self-assigned this Feb 11, 2016

Updated the documentation to include the defaults for the remaining f…

5f558c0

…ields added in the PR and add the link to amazon describing the auto confirming subscription.

radeksimko reviewed Feb 12, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 12, 2016

Update code based on the review suggestions.

f21dc99

1. Used resource.Retry instead of custom solution 2. Removed unnecessary variables and added required variable to resource.Retry.

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 12, 2016

radeksimko reviewed Feb 13, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 13, 2016

Made the necessary changes to ensure the variable name represents rig…

345dbce

…ht value.

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 13, 2016

radeksimko added a commit that referenced this pull request Feb 14, 2016

Merge pull request #4711 from srikalyan/master

fdd9c3b

Added support for http/https endpoints that auto confirms SNS topic subscription.

radeksimko merged commit fdd9c3b into hashicorp:master Feb 14, 2016

scheffield mentioned this pull request May 13, 2016

endpoint_auto_confirms and confirmation_timeout_in_minutes for sqs endpoints #6652

Closed

hashibot mentioned this pull request Jun 13, 2017

endpoint_auto_confirms and confirmation_timeout_in_minutes for sqs endpoints hashicorp/terraform-provider-aws#166

Closed

ghost locked and limited conversation to collaborators Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for http/https endpoints that auto confirms SNS topic subscription. #4711

Added support for http/https endpoints that auto confirms SNS topic subscription. #4711

srikalyan commented Jan 17, 2016

srikalyan Jan 17, 2016

radeksimko Feb 12, 2016

srikalyan Feb 12, 2016

radeksimko Feb 12, 2016

srikalyan commented Jan 17, 2016

srikalyan commented Jan 20, 2016

packplusplus commented Jan 20, 2016

srikalyan commented Jan 25, 2016

srikalyan commented Feb 2, 2016

radeksimko commented Feb 10, 2016

srikalyan commented Feb 11, 2016

radeksimko commented Feb 11, 2016

srikalyan commented Feb 11, 2016

radeksimko Feb 12, 2016

radeksimko Feb 12, 2016

srikalyan Feb 12, 2016

radeksimko Feb 12, 2016

radeksimko commented Feb 12, 2016

srikalyan commented Feb 12, 2016

radeksimko commented Feb 12, 2016

srikalyan commented Feb 12, 2016

radeksimko commented Feb 12, 2016

srikalyan commented Feb 13, 2016

radeksimko Feb 13, 2016

srikalyan Feb 13, 2016

radeksimko Feb 13, 2016

radeksimko commented Feb 13, 2016

ghost commented Apr 28, 2020

Added support for http/https endpoints that auto confirms SNS topic subscription. #4711

Added support for http/https endpoints that auto confirms SNS topic subscription. #4711

Conversation

srikalyan commented Jan 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikalyan commented Jan 17, 2016

srikalyan commented Jan 20, 2016

packplusplus commented Jan 20, 2016

srikalyan commented Jan 25, 2016

srikalyan commented Feb 2, 2016

radeksimko commented Feb 10, 2016

srikalyan commented Feb 11, 2016

radeksimko commented Feb 11, 2016

srikalyan commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Feb 12, 2016

srikalyan commented Feb 12, 2016

radeksimko commented Feb 12, 2016

srikalyan commented Feb 12, 2016

radeksimko commented Feb 12, 2016

srikalyan commented Feb 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Feb 13, 2016

ghost commented Apr 28, 2020