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

resource/aws_sns_topic_subscription: Use paginated ListSubscriptionsByTopic and return immediately on errors #14262

Merged
merged 2 commits into from
Jul 24, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
69 changes: 41 additions & 28 deletions aws/resource_aws_sns_topic_subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,16 @@ func subscribeToSNSTopic(d *schema.ResourceData, snsconn *sns.SNS) (output *sns.

subscription, err := findSubscriptionByNonID(d, snsconn)

if subscription != nil {
output.SubscriptionArn = subscription.SubscriptionArn
return nil
if err != nil {
return resource.NonRetryableError(err)
}

if err != nil {
return resource.RetryableError(
fmt.Errorf("Error fetching subscriptions for SNS topic %s: %s", topic_arn, err))
if subscription == nil {
return resource.RetryableError(fmt.Errorf("Endpoint (%s) did not autoconfirm the subscription for topic %s", endpoint, topic_arn))
}

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

if isResourceTimeoutError(err) {
Expand All @@ -268,37 +266,52 @@ func subscribeToSNSTopic(d *schema.ResourceData, snsconn *sns.SNS) (output *sns.
}

// 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) {
func findSubscriptionByNonID(d *schema.ResourceData, conn *sns.SNS) (*sns.Subscription, error) {
protocol := d.Get("protocol").(string)
endpoint := d.Get("endpoint").(string)
topic_arn := d.Get("topic_arn").(string)
topicARN := d.Get("topic_arn").(string)
obfuscatedEndpoint := obfuscateEndpoint(endpoint)

req := &sns.ListSubscriptionsByTopicInput{
TopicArn: aws.String(topic_arn),
input := &sns.ListSubscriptionsByTopicInput{
TopicArn: aws.String(topicARN),
}
var result *sns.Subscription

for {
res, err := snsconn.ListSubscriptionsByTopic(req)

if err != nil {
return nil, fmt.Errorf("Error fetching subscriptions for topic %s : %s", topic_arn, err)
err := conn.ListSubscriptionsByTopicPages(input, func(page *sns.ListSubscriptionsByTopicOutput, lastPage bool) bool {
if page == nil {
return !lastPage
}

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

if aws.StringValue(subscription.Endpoint) != obfuscatedEndpoint {
continue
}
}

// if there are more than 100 subscriptions then go to the next 100 otherwise return an error
if res.NextToken != nil {
req.NextToken = res.NextToken
} else {
return nil, fmt.Errorf("Error finding subscription for topic %s with endpoint %s and protocol %s", topic_arn, endpoint, protocol)
if aws.StringValue(subscription.Protocol) != protocol {
continue
}

if aws.StringValue(subscription.TopicArn) != topicARN {
continue
}

if subscriptionHasPendingConfirmation(subscription.SubscriptionArn) {
continue
}

result = subscription

return false
}
}

return !lastPage
})

return result, err
}

// returns true if arn is nil or has both pending and confirmation words in the arn
Expand Down
1 change: 0 additions & 1 deletion aws/resource_aws_sns_topic_subscription_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,6 @@ resource "aws_api_gateway_authorizer" "test" {
name = "tf-acc-test-api-gw-authorizer-%d"
rest_api_id = "${aws_api_gateway_rest_api.test.id}"
authorizer_uri = "${aws_lambda_function.authorizer.invoke_arn}"
authorizer_result_ttl_in_seconds = "0"
Copy link
Contributor

@anGie44 anGie44 Jul 24, 2020

Choose a reason for hiding this comment

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

this fix looks good! I also noticed in this test step https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_api_gateway_authorizer_test.go#L190 we're able to update the TTL to 0 without a problem so it looks like the error here is attributed to the d.GetOk call instead of d.Get when setting this value at creation time https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_api_gateway_authorizer.go#L111 . might be out of scope to address it in this PR but I can make an issue/see if there already is one stating the authorizer resource w/ a value of 0 (caching disabled) results in a non-empty plan on creation

Copy link
Contributor

@anGie44 anGie44 Jul 24, 2020

Choose a reason for hiding this comment

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

Found: #12633 with linked PR so all good 👍

authorizer_credentials = "${aws_iam_role.invocation_role.arn}"
}

Expand Down