-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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 1 commit
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,6 +9,7 @@ import ( | |
|
||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/aws/aws-sdk-go/service/sns" | ||
"time" | ||
) | ||
|
||
const awsSNSPendingConfirmationMessage = "pending confirmation" | ||
|
@@ -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( | ||
|
@@ -44,6 +45,24 @@ func resourceAwsSnsTopicSubscription() *schema.Resource { | |
Required: true, | ||
ForceNew: false, | ||
}, | ||
"endpoint_auto_confirms": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
ForceNew: false, | ||
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, | ||
|
@@ -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 { | ||
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 +218,64 @@ func subscribeToSNSTopic(d *schema.ResourceData, snsconn *sns.SNS) (output *sns. | |
return nil, fmt.Errorf("Error creating SNS topic: %s", err) | ||
} | ||
|
||
if strings.Contains(protocol, "http") && (output.SubscriptionArn == nil || *output.SubscriptionArn == awsSNSPendingConfirmationMessage) { | ||
|
||
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 && output.SubscriptionArn != nil && *output.SubscriptionArn == awsSNSPendingConfirmationMessage; i++ { | ||
|
||
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 output.SubscriptionArn == nil || *output.SubscriptionArn == awsSNSPendingConfirmationMessage { | ||
return nil, fmt.Errorf("Endpoint (%s) did not autoconfirm the subscription for topic %s", endpoint, topic_arn) | ||
} | ||
} | ||
|
||
log.Printf("[DEBUG] Created new subscription!") | ||
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 { | ||
if *subscription.Endpoint == endpoint && *subscription.Protocol == protocol && *subscription.TopicArn == topic_arn { | ||
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 | ||
} | ||
} | ||
} |
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.
I believe
ForceNew: false
is a default behaviour so we shouldn't need to specify it here.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.
which applies to the other 2 fields below too.
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.
I see your point but almost all the schema elements have ForceNew: false, I guess they made a bad copy :). I will fix it.
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.
Not everything you see elsewhere in the codebase is worth copying, yet many things do! 😃