-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Changes from 5 commits
9d12594
63d6d8d
3d256dd
5f558c0
f21dc99
345dbce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,12 @@ import ( | |
|
||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/aws/aws-sdk-go/service/sns" | ||
"github.com/hashicorp/terraform/helper/resource" | ||
"time" | ||
) | ||
|
||
const awsSNSPendingConfirmationMessage = "pending confirmation" | ||
const awsSNSPendingConfirmationMessageWithoutSpaces = "pendingconfirmation" | ||
|
||
func resourceAwsSnsTopicSubscription() *schema.Resource { | ||
return &schema.Resource{ | ||
|
@@ -27,7 +30,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( | ||
|
@@ -42,22 +45,28 @@ func resourceAwsSnsTopicSubscription() *schema.Resource { | |
"endpoint": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: false, | ||
}, | ||
"endpoint_auto_confirms": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: false, | ||
}, | ||
"confirmation_timeout_in_minutes": &schema.Schema{ | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
Default: 1, | ||
}, | ||
"topic_arn": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: false, | ||
}, | ||
"delivery_policy": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: false, | ||
}, | ||
"raw_message_delivery": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
ForceNew: false, | ||
Default: false, | ||
}, | ||
"arn": &schema.Schema{ | ||
|
@@ -77,7 +86,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 | ||
} | ||
|
@@ -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)) | ||
|
||
if strings.Contains(protocol, "http") && !endpoint_auto_confirms { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. I almost thought on first sight you forgot about There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
||
|
@@ -192,6 +207,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 subscription is pending so fetching the subscription list for topic : %s (%s) @ '%s'", endpoint, protocol, topic_arn) | ||
|
||
err = resource.Retry(time.Minute*confirmation_timeout_in_minutes, func() error { | ||
|
||
subscription, err := findSubscriptionByNonID(d, snsconn) | ||
|
||
if subscription != nil { | ||
output.SubscriptionArn = subscription.SubscriptionArn | ||
return nil | ||
} | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error fetching subscriptions for SNS topic %s: %s", topic_arn, err) | ||
} | ||
|
||
return fmt.Errorf("Endpoint (%s) did not autoconfirm the subscription for topic %s", endpoint, topic_arn) | ||
}) | ||
|
||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.Replace(strings.ToLower(*arn), " ", "", -1), awsSNSPendingConfirmationMessageWithoutSpaces) { | ||
return false | ||
} | ||
|
||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 oftime.Duration
are nanoseconds which makes the variable name wrong.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I had in mind. 👍