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

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

Merged
merged 6 commits into from
Feb 14, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 99 additions & 3 deletions builtin/providers/aws/resource_aws_sns_topic_subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/sns"
"time"
)

const awsSNSPendingConfirmationMessage = "pending confirmation"
Expand All @@ -27,7 +28,7 @@ func resourceAwsSnsTopicSubscription() *schema.Resource {
ForceNew: false,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
forbidden := []string{"email", "sms", "http"}
forbidden := []string{"email", "sms"}
for _, f := range forbidden {
if strings.Contains(value, f) {
errors = append(
Expand All @@ -44,6 +45,24 @@ func resourceAwsSnsTopicSubscription() *schema.Resource {
Required: true,
ForceNew: false,
},
"endpoint_auto_confirms": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
ForceNew: false,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

which applies to the other 2 fields below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Default: false,
},
"max_fetch_retries": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
ForceNew: false,
Default: 3,
},
"fetch_retry_delay": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
ForceNew: false,
Default: 1,
},
"topic_arn": &schema.Schema{
Type: schema.TypeString,
Required: true,
Expand Down Expand Up @@ -77,7 +96,7 @@ func resourceAwsSnsTopicSubscriptionCreate(d *schema.ResourceData, meta interfac
return err
}

if output.SubscriptionArn != nil && *output.SubscriptionArn == awsSNSPendingConfirmationMessage {
if subscriptionHasPendingConfirmation(output.SubscriptionArn) {
log.Printf("[WARN] Invalid SNS Subscription, received a \"%s\" ARN", awsSNSPendingConfirmationMessage)
return nil
}
Expand Down Expand Up @@ -178,6 +197,13 @@ 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)
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

return nil, fmt.Errorf("Protocol http/https is only supported for endpoints which auto confirms!")
}

log.Printf("[DEBUG] SNS create topic subscription: %s (%s) @ '%s'", endpoint, protocol, topic_arn)

Expand All @@ -192,6 +218,76 @@ func subscribeToSNSTopic(d *schema.ResourceData, snsconn *sns.SNS) (output *sns.
return nil, fmt.Errorf("Error creating SNS topic: %s", err)
}

log.Printf("[DEBUG] Created new subscription!")
log.Printf("[DEBUG] Finished subscribing to topic %s with subscription arn %s", topic_arn, *output.SubscriptionArn)

if strings.Contains(protocol, "http") && subscriptionHasPendingConfirmation(output.SubscriptionArn) {

log.Printf("[DEBUG] SNS create topic subscritpion is pending so fetching the subscription list for topic : %s (%s) @ '%s'", endpoint, protocol, topic_arn)

for i := 0; i < max_fetch_retries && subscriptionHasPendingConfirmation(output.SubscriptionArn); i++ {
Copy link
Member

Choose a reason for hiding this comment

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

We have a nice helper for this which also uses exponential back-off, see resource.Retry, here's an example:

err := resource.Retry(10*time.Minute, func() error {
out, err := conn.DeleteCluster(&ecs.DeleteClusterInput{
Cluster: aws.String(d.Id()),
})
if err == nil {
log.Printf("[DEBUG] ECS cluster %s deleted: %s", d.Id(), out)
return nil
}
awsErr, ok := err.(awserr.Error)
if !ok {
return resource.RetryError{Err: err}
}
if awsErr.Code() == "ClusterContainsContainerInstancesException" {
log.Printf("[TRACE] Retrying ECS cluster %q deletion after %q", d.Id(), awsErr.Code())
return err
}
if awsErr.Code() == "ClusterContainsServicesException" {
log.Printf("[TRACE] Retrying ECS cluster %q deletion after %q", d.Id(), awsErr.Code())
return err
}
return resource.RetryError{Err: err}
})

Then max_fetch_retries & fetch_retry_delay would turn into confirmation_timeout_in_minutes. I think having 1min as default timeout is sensible settings, in most cases it will be confirmed much sooner anyway, I assume.

Let me know if you need any help implementing that helper.


subscription, err := findSubscriptionByNonID(d, snsconn)

if err != nil {
return nil, fmt.Errorf("Error fetching subscriptions for SNS topic %s: %s", topic_arn, err)
}

if subscription != nil {
output.SubscriptionArn = subscription.SubscriptionArn
break
}

time.Sleep(time.Second * fetch_retry_delay)
}

if subscriptionHasPendingConfirmation(output.SubscriptionArn) {
return nil, fmt.Errorf("Endpoint (%s) did not autoconfirm the subscription for topic %s", endpoint, topic_arn)
}
}

log.Printf("[DEBUG] Created new subscription! %s", *output.SubscriptionArn)
return output, nil
}

// finds a subscription using protocol, endpoint and topic_arn (which is a key in sns subscription)
func findSubscriptionByNonID(d *schema.ResourceData, snsconn *sns.SNS) (*sns.Subscription, error) {
protocol := d.Get("protocol").(string)
endpoint := d.Get("endpoint").(string)
topic_arn := d.Get("topic_arn").(string)

req := &sns.ListSubscriptionsByTopicInput{
TopicArn: aws.String(topic_arn),
}

for {

res, err := snsconn.ListSubscriptionsByTopic(req)

if err != nil {
return nil, fmt.Errorf("Error fetching subscripitions for topic %s : %s", topic_arn, err)
}

for _, subscription := range res.Subscriptions {
log.Printf("[DEBUG] check subscription with EndPoint %s, Protocol %s, topicARN %s and SubscriptionARN %s", *subscription.Endpoint, *subscription.Protocol, *subscription.TopicArn, *subscription.SubscriptionArn)
if *subscription.Endpoint == endpoint && *subscription.Protocol == protocol && *subscription.TopicArn == topic_arn && !subscriptionHasPendingConfirmation(subscription.SubscriptionArn) {
return subscription, nil
}
}

// if there are more than 100 subscriptions then go to the next 100 otherwise return nil
if res.NextToken != nil {
req.NextToken = res.NextToken
} else {
return nil, nil
}
}
}

// returns true if arn is nil or has both pending and confirmation words in the arn
func subscriptionHasPendingConfirmation(arn *string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

👍 Good idea to separate this out into a function!

if arn != nil && !strings.Contains(strings.ToLower(*arn), "pending") && !strings.Contains(strings.ToLower(*arn), "confirmation") {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why not just do this? Have you spotted any edge cases where the casing was different?

if arn != nil && *arn != "Pending confirmation" {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah amazon is behaving weird here. Sometimes it sends "Pending Confirmation" and some times "PendingConfirmation" and some times "pending confirmation". I can probably dig deeper and figure out when but in all the cases I have to retry so just used this logic to handle the situation.

Copy link
Member

Choose a reason for hiding this comment

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

um, that isn't nice from Amazon... 😞

Can we at least make this piece of code a bit more readable then? e.g. lowercase the whole string we get from AWS + remove all spaces and then use it for comparison as == pendingconfirmation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure.

return false
}

return true
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ resource "aws_sns_topic_subscription" "user_updates_sqs_target" {
The following arguments are supported:

* `topic_arn` - (Required) The ARN of the SNS topic to subscribe to
* `protocol` - (Required) The protocol to use. The possible values for this are: `sqs`, `lambda`, or `application`. (`email`, `http`, `https`, `sms`, are options but unsupported, see below)
* `protocol` - (Required) The protocol to use. The possible values for this are: `sqs`, `lambda`, `application`. (`http` or `https` are partially supported, see below) (`email`, `sms`, are options but unsupported, see below).
* `endpoint` - (Required) The endpoint to send data to, the contents will vary with the protocol. (see below for more information)
* `endpoint_auto_confirms` - (Optional) Boolean indicating whether the end point is capable of [auto confirming subscription](http://docs.aws.amazon.com/sns/latest/dg/SendMessageToHttp.html#SendMessageToHttp.prepare) (default is false)
* `max_fetch_retries` - (Optional) Integer indicating number of times the subscriptions list for a topic to be fetched to get the subscription arn for auto confirming end points (default is 3 times).
* `fetch_retry_delay` - (Optional) Integer indicating number of seconds to sleep before re-fetching the subscription list for the topic (default is 1 second).
* `raw_message_delivery` - (Optional) Boolean indicating whether or not to enable raw message delivery (the original message is directly passed, not wrapped in JSON with the original message in the message property).

### Protocols supported
Expand All @@ -61,12 +64,15 @@ Supported SNS protocols include:
* `sqs` -- delivery of JSON-encoded message to an Amazon SQS queue
* `application` -- delivery of JSON-encoded message to an EndpointArn for a mobile app and device

Partially supported SNS protocols include:

* `http` -- delivery of JSON-encoded messages via HTTP. Supported only for the end points that auto confirms the subscription.
* `https` -- delivery of JSON-encoded messages via HTTPS. Supported only for the end points that auto confirms the subscription.

Unsupported protocols include the following:

* `email` -- delivery of message via SMTP
* `email-json` -- delivery of JSON-encoded message via SMTP
* `http` -- delivery via HTTP
* `http(s)` -- delivery via HTTPS
* `sms` -- delivery text message

These are unsupported because the endpoint needs to be authorized and does not
Expand Down