resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy #3289

ddcprg · 2018-02-08T10:46:44Z

…urce

ddcprg · 2018-02-08T10:49:58Z

aws/resource_aws_sns_topic_subscription_test.go

 `, i, i, policy)
 }

+func testAccAWSSNSTopicSubscriptionConfig_deliveryPolicy(i int, policy string) string {


Due to security restrictions in the accounts I have access to, I'm unable to run this acceptance test. Please let me know whether you prefer to have this deleted or run it in your end. Note this is a copy of one of the existing tests with the addition of delivery_policy in the SNS subscription

We generally require acceptance testing for new enhancements and this one does look pretty good overall. It does need to be updated to a more valid delivery policy JSON though, as AWS is currently returning:

2018/02/08 11:56:37 [DEBUG] [aws-sdk-go] DEBUG: Validate Response sns/SetSubscriptionAttributes failed, not retrying, error InvalidParameter: Invalid parameter: DeliveryPolicy: healthyRetryPolicy.minDelayTarget must be specified status code: 400, request id: 4d4b2c18-4d55-5c2b-af81-7eeeced101fe

Oh yeah, I forgot the example on the doc is wrong. I'll update this with a valid policy

aws/resource_aws_sns_topic_subscription_test.go

ddcprg · 2018-02-08T10:57:51Z

@bflad I've opened this PR with the changes for the delivery policy picked from the other PR. Cheers

bflad · 2018-02-08T16:43:49Z

@ddcprg thanks so much for being flexible with rebasing this PR! Taking a look now.

bflad

@ddcprg this PR is pretty close, thanks for adding the function to centralize the SetSubscriptionAttribute logic. Check out below and let me know if you can adjust the items or if you'd like one of us to finish this up (since I know you can't run the acceptance testing).

--- FAIL: TestAccAWSSNSTopicSubscription_deliveryPolicy (37.48s)
	testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_sns_topic_subscription.test_subscription: 1 error(s) occurred:

		* aws_sns_topic_subscription.test_subscription: Unable to set DeliveryPolicy attribute on subscription

bflad · 2018-02-08T16:49:38Z

aws/resource_aws_sns_topic_subscription.go

@@ -328,3 +328,18 @@ func obfuscateEndpoint(endpoint string) string {
 	}
 	return obfuscatedEndpoint
 }
+
+func snsSubscriptionAttributeUpdate(snsconn *sns.SNS, tfId string, tfName string, val string) error {


This is very handy here. Can we update the signature of this function to just take in the expected values?

func snsSubscriptionAttributeUpdate(snsconn *sns.SNS, subscriptionArn, attributeName, attributeValue string) error {

Thanks!

No problem, done. This syntax is cleaner in fact

bflad · 2018-02-08T16:52:11Z

aws/resource_aws_sns_topic_subscription.go

+	_, err := snsconn.SetSubscriptionAttributes(req)
+
+	if err != nil {
+		return fmt.Errorf("Unable to set %s attribute on subscription", awsAttrName)


We should definitely return the err string here too 😄

return fmt.Errorf("Unable to set %s attribute on subscription: %s", attributeName, err)

Otherwise you get this, which isn't so helpful:

=== RUN TestAccAWSSNSTopicSubscription_deliveryPolicy --- FAIL: TestAccAWSSNSTopicSubscription_deliveryPolicy (37.69s) testing.go:513: Step 0 error: Error applying: 1 error(s) occurred: * aws_sns_topic_subscription.test_subscription: 1 error(s) occurred: * aws_sns_topic_subscription.test_subscription: Unable to set DeliveryPolicy attribute on subscription

Count on this

aws/resource_aws_sns_topic_subscription_test.go

bflad · 2018-02-08T16:55:25Z

website/docs/r/sns_topic_subscription.html.markdown

@@ -242,6 +242,8 @@ The following arguments are supported:
 * `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) e.g., PagerDuty (default is false)
 * `confirmation_timeout_in_minutes` - (Optional) Integer indicating number of minutes to wait in retying mode for fetching subscription arn before marking it as failure. Only applicable for http and https protocols (default is 1 minute).
 * `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) (default is false).
+* `filter_policy` - (Optional) JSON String with the filter policy that will be used in the subscription to filter messages seen by the target resource. Refer to the SNS docs for more details.


Thank you!!! I totally missed this yesterday. 😢 Can you please also link to the appropriate documentation for these two policies please? e.g. https://docs.aws.amazon.com/sns/latest/dg/message-filtering.html

No problem. The delivery policy doc is not specific about the format https://docs.aws.amazon.com/sns/latest/dg/DeliveryPolicies.html . There is another page with an example to set this via CLI but is less specific and the policy is incomplete :) https://docs.aws.amazon.com/sns/latest/dg/json-formats.html

bflad · 2018-02-08T16:59:55Z

aws/resource_aws_sns_topic_subscription_test.go

 `, i, i, policy)
 }

+func testAccAWSSNSTopicSubscriptionConfig_deliveryPolicy(i int, policy string) string {


We generally require acceptance testing for new enhancements and this one does look pretty good overall. It does need to be updated to a more valid delivery policy JSON though, as AWS is currently returning:

2018/02/08 11:56:37 [DEBUG] [aws-sdk-go] DEBUG: Validate Response sns/SetSubscriptionAttributes failed, not retrying, error InvalidParameter: Invalid parameter: DeliveryPolicy: healthyRetryPolicy.minDelayTarget must be specified status code: 400, request id: 4d4b2c18-4d55-5c2b-af81-7eeeced101fe

bflad · 2018-02-08T17:00:34Z

aws/resource_aws_sns_topic_subscription.go

 		}
 	}

 	if d.HasChange("filter_policy") {
-		_, n := d.GetChange("filter_policy")


FYI: n here is a specific naming convention as d.GetChange() returns old, new 😄 (also makes it easier to duplicate the logic if necessary).

makes sense :) I'll change it

bflad · 2018-02-08T17:02:00Z

aws/resource_aws_sns_topic_subscription.go

-		if err != nil {
-			return fmt.Errorf("Unable to set filter policy attribute on subscription: %s", err)
+	if d.HasChange("delivery_policy") {
+		_, dp := d.GetChange("delivery_policy")


Same note about n here 👍

ddcprg · 2018-02-08T17:12:12Z

@bflad no problem, thanks for the fast feedback. I'll tidy up the code and let you know so you guys can run the acceptance tests

…urce

ddcprg · 2018-02-08T17:54:37Z

Hey @bflad I've made the requested changes. I'll leave this with you now for the acceptance tests. Cheers

bflad · 2018-02-08T18:07:13Z

🙁 oh this is fun, AWS will return a fully populated policy back. I'm not sure there is an easy way to handle the difference here, since guaranteed is a populated boolean attribute.

--- FAIL: TestAccAWSSNSTopicSubscription_deliveryPolicy (37.69s)
	testing.go:513: Step 0 error: Check failed: aws_sns_topic_subscription.test_subscription: Attribute 'delivery_policy' expected "{\"healthyRetryPolicy\": {\"minDelayTarget\": 5, \"maxDelayTarget\": 20, \"numRetries\": 5}}", got "{\"healthyRetryPolicy\":{\"minDelayTarget\":5,\"maxDelayTarget\":20,\"numRetries\":5,\"numMaxDelayRetries\":null,\"numNoDelayRetries\":null,\"numMinDelayRetries\":null,\"backoffFunction\":null},\"sicklyRetryPolicy\":null,\"throttlePolicy\":null,\"guaranteed\":false}"

ddcprg · 2018-02-08T18:27:34Z

we could unmarshall the response into a map/struct and create a custom assertion

ddcprg · 2018-02-08T18:39:19Z

Something like this:

package main

import (
	"encoding/json"
	"fmt"
)

type HealthyRetryPolicy struct {
	MinDelayTarget int
	MaxDelayTarget int
	NumRetries     int
}
type DeliveryPolicy struct {
	HealthyRetryPolicy HealthyRetryPolicy
}

func main() {
	response := "{\"healthyRetryPolicy\":{\"minDelayTarget\":5,\"maxDelayTarget\":20,\"numRetries\":5,\"numMaxDelayRetries\":null,\"numNoDelayRetries\":null,\"numMinDelayRetries\":null,\"backoffFunction\":null},\"sicklyRetryPolicy\":null,\"throttlePolicy\":null,\"guaranteed\":false}"
	var policy DeliveryPolicy
	_ = json.Unmarshal([]byte(response), &policy)
	fmt.Print(policy)
}

bflad · 2018-02-08T19:40:01Z

This seems to work, although I'm not sure the full structures of healthyRetryPolicy, sicklyRetryPolicy, and throttlePolicy:

type snsTopicSubscriptionDeliveryPolicy struct {
	Guaranteed         bool                                                  `json:"guaranteed,omitempty"`
	HealthyRetryPolicy *snsTopicSubscriptionDeliveryPolicyHealthyRetryPolicy `json:"healthyRetryPolicy,omitempty"`
	SicklyRetryPolicy  *snsTopicSubscriptionDeliveryPolicySicklyRetryPolicy  `json:"sicklyRetryPolicy,omitempty"`
	ThrottlePolicy     *snsTopicSubscriptionDeliveryPolicyThrottlePolicy     `json:"throttlePolicy,omitempty"`
}

type snsTopicSubscriptionDeliveryPolicyHealthyRetryPolicy struct {
	BackoffFunction    string `json:"backoffFunction,omitempty"`
	MaxDelayTarget     int    `json:"maxDelayTarget,omitempty"`
	MinDelayTarget     int    `json:"minDelayTarget,omitempty"`
	NumMaxDelayRetries int    `json:"numMaxDelayRetries,omitempty"`
	NumMinDelayRetries int    `json:"numMinDelayRetries,omitempty"`
	NumNoDelayRetries  int    `json:"numNoDelayRetries,omitempty"`
	NumRetries         int    `json:"numRetries,omitempty"`
}

type snsTopicSubscriptionDeliveryPolicySicklyRetryPolicy struct {
	BackoffFunction    string `json:"backoffFunction,omitempty"`
	MaxDelayTarget     int    `json:"maxDelayTarget,omitempty"`
	MinDelayTarget     int    `json:"minDelayTarget,omitempty"`
	NumMaxDelayRetries int    `json:"numMaxDelayRetries,omitempty"`
	NumMinDelayRetries int    `json:"numMinDelayRetries,omitempty"`
	NumNoDelayRetries  int    `json:"numNoDelayRetries,omitempty"`
	NumRetries         int    `json:"numRetries,omitempty"`
}

type snsTopicSubscriptionDeliveryPolicyThrottlePolicy struct {
	MaxReceivesPerSecond int `json:"maxReceivesPerSecond,omitempty"`
}

func normalizeSnsTopicSubscriptionDeliveryPolicy(v interface{}) string {
	structureNormalizedJson, _ := structure.NormalizeJsonString(v)

	var dp snsTopicSubscriptionDeliveryPolicy

	err := json.Unmarshal([]byte(structureNormalizedJson), &dp)
	if err != nil {
		return structureNormalizedJson
	}

	bytes, _ := json.Marshal(dp)
	return string(bytes[:])
}

func TestNormalizeSnsTopicSubscriptionDeliveryPolicy(t *testing.T) {
	var testCases = []struct {
		Input    string
		Expected string
	}{
		{
			Input:    `{"healthyRetryPolicy":{"minDelayTarget":5,"maxDelayTarget":20,"numRetries":5,"numMaxDelayRetries":null,"numNoDelayRetries":null,"numMinDelayRetries":null,"backoffFunction":null},"sicklyRetryPolicy":null,"throttlePolicy":null,"guaranteed":false}`,
			Expected: `{"healthyRetryPolicy":{"maxDelayTarget":20,"minDelayTarget":5,"numRetries":5}}`,
		},
		{
			Input:    `{"healthyRetryPolicy":null,"sicklyRetryPolicy":null,"throttlePolicy":null,"guaranteed":true}`,
			Expected: `{"guaranteed":true}`,
		},
	}

	for _, tc := range testCases {
		actual := normalizeSnsTopicSubscriptionDeliveryPolicy(tc.Input)
		if actual != tc.Expected {
			t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, tc.Expected)
		}
	}
}

go test ./aws -v -run=TestNormalizeSnsTopicSubscriptionDeliveryPolicy
=== RUN   TestNormalizeSnsTopicSubscriptionDeliveryPolicy
--- PASS: TestNormalizeSnsTopicSubscriptionDeliveryPolicy (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.041s

ddcprg · 2018-02-09T09:56:01Z

Thanks for that, much more elaborated. It looks complete to me but I'm afraid I can't confirm whether the structures are complete since the structures seem to be barely documented by Amazon. IMO this is good enough for this test :)

bflad · 2018-02-09T17:30:30Z

@ddcprg do you have an AWS account with higher than the basic support plan that could ask them for the (full) documentation of these? If we're going to support this functionality, we should probably try to get it right the first time rather than guess. If you don't, that's okay, its going to take me a little bit to get back to this PR myself.

ddcprg · 2018-02-09T21:03:29Z

We do. Anything specific you'd like to ask them apart from the structure of the in and out JSON objects? I'll raise a ticket with them when I'm back in the office next Monday

bflad · 2018-02-09T21:21:17Z

I think just knowing all the possible keys/values in the DeliveryPolicy attribute would suffice since they're not documented anywhere well that I could find. Thanks!

ddcprg · 2018-02-16T11:51:32Z

@bflad I'm still waiting for AWS support to get back to me with this info. They told me last Monday they were going to get in touch with the development team to get the details and ask them to update update the docs if necessary.

ddcprg · 2018-03-21T14:12:49Z

Hey @bflad it seems like is going to take a while until we can get any feedback from the AWS team. I've just pinged support again and this is what they said:

...
The team are in the process of re-evaluating the use of key-pairs at present and so they are not in a position to provide the list until this evaluation process is complete.
...

bflad

Hi @ddcprg 👋 Since it doesn't seem like we'll receive full documentation for this, so I'm opting to pull this in with the struct handling I outlined in #3289 (comment) (except using DiffSuppressFunc instead of StateFunc for the attribute). We can iterate as necessary. With code and testing updates:

--- PASS: TestAccAWSSNSTopicSubscription_basic (14.24s)
--- PASS: TestAccAWSSNSTopicSubscription_importBasic (15.24s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (23.04s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (23.49s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (42.79s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (76.68s)

ddcprg · 2018-10-01T08:24:07Z

Hi @bflad yeah, it'll probably take long until we hear from them, thanks for letting me know

ghost · 2020-04-03T17:21:56Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

[issue-3288] Add configurable DeliveryPolicy to SNS subscription reso…

48d738a

…urce

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018

ddcprg mentioned this pull request Feb 8, 2018

[issue-2916] Add DeliveryPolicy and FilterPolicy to SNS resource #3257

Closed

ddcprg commented Feb 8, 2018

View reviewed changes

aws/resource_aws_sns_topic_subscription_test.go Show resolved Hide resolved

radeksimko changed the title ~~[issue-3288] Add configurable DeliveryPolicy to SNS subscription resource~~ resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy Feb 8, 2018

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. service/sns Issues and PRs that pertain to the sns service. labels Feb 8, 2018

bflad self-requested a review February 8, 2018 16:43

bflad suggested changes Feb 8, 2018

View reviewed changes

[issue-3288] Add configurable DeliveryPolicy to SNS subscription reso…

05b981a

…urce

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018

bflad self-requested a review February 8, 2018 17:57

Merge branch 'master' into issue-3288

ec7dacf

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 21, 2018

bflad approved these changes Sep 28, 2018

View reviewed changes

bflad added this to the v1.39.0 milestone Sep 28, 2018

bflad merged commit ec7dacf into hashicorp:master Sep 28, 2018

bflad added a commit that referenced this pull request Sep 28, 2018

Update CHANGELOG for #3289

8123783

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy #3289

resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy #3289

ddcprg commented Feb 8, 2018

ddcprg Feb 8, 2018 •

edited

Loading

bflad Feb 8, 2018

ddcprg Feb 8, 2018

ddcprg commented Feb 8, 2018

bflad commented Feb 8, 2018

bflad left a comment

bflad Feb 8, 2018

ddcprg Feb 8, 2018

bflad Feb 8, 2018

ddcprg Feb 8, 2018

bflad Feb 8, 2018

ddcprg Feb 8, 2018

bflad Feb 8, 2018

bflad Feb 8, 2018

ddcprg Feb 8, 2018

bflad Feb 8, 2018

ddcprg Feb 8, 2018

ddcprg commented Feb 8, 2018

ddcprg commented Feb 8, 2018 •

edited

Loading

bflad commented Feb 8, 2018

ddcprg commented Feb 8, 2018

ddcprg commented Feb 8, 2018

bflad commented Feb 8, 2018 •

edited

Loading

ddcprg commented Feb 9, 2018

bflad commented Feb 9, 2018

ddcprg commented Feb 9, 2018

bflad commented Feb 9, 2018

ddcprg commented Feb 16, 2018

ddcprg commented Mar 21, 2018 •

edited

Loading

bflad left a comment

ddcprg commented Oct 1, 2018 •

edited

Loading

ghost commented Apr 3, 2020

resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy #3289

resource/aws_sns_topic_subscription: Add configurable DeliveryPolicy #3289

Conversation

ddcprg commented Feb 8, 2018

ddcprg Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcprg commented Feb 8, 2018

bflad commented Feb 8, 2018

bflad left a comment

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

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

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

ddcprg commented Feb 8, 2018

ddcprg commented Feb 8, 2018 • edited Loading

bflad commented Feb 8, 2018

ddcprg commented Feb 8, 2018

ddcprg commented Feb 8, 2018

bflad commented Feb 8, 2018 • edited Loading

ddcprg commented Feb 9, 2018

bflad commented Feb 9, 2018

ddcprg commented Feb 9, 2018

bflad commented Feb 9, 2018

ddcprg commented Feb 16, 2018

ddcprg commented Mar 21, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

ddcprg commented Oct 1, 2018 • edited Loading

ghost commented Apr 3, 2020

ddcprg Feb 8, 2018 •

edited

Loading

ddcprg commented Feb 8, 2018 •

edited

Loading

bflad commented Feb 8, 2018 •

edited

Loading

ddcprg commented Mar 21, 2018 •

edited

Loading

ddcprg commented Oct 1, 2018 •

edited

Loading