feat(sns): add delivery policy to sns subscriptions #30830

LaurenceWarne · 2024-07-11T19:36:45Z

Issue # (if applicable)

Reason for this change

To allow setting of delivery policy for appropriate sns subscriptions.

Description of changes

Subscriptions can now take a new parameter deliveryPolicy which consists of a healthyRetryPolicy, a throttlePolicy, and a requestPolicy (each having the same parameters described here: https://docs.aws.amazon.com/sns/latest/dg/sns-message-delivery-retries.html#creating-delivery-policy).

Description of how you validated changes

New integration test along with unit tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

LaurenceWarne · 2024-07-11T19:40:56Z

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ * Algorithms which can be used by SNS to calculate the delays associated with all of the retry attempts between the first and last retries in the backoff phase.
+ */
+export enum BackoffFunction {
+ /** Arithmetic.


Not the best doc I've ever written 😅 , the best indicator of to what these each of these do that I found was an image at the very bottom of the doc page here: https://docs.aws.amazon.com/sns/latest/dg/sns-message-delivery-retries.html - suggestions welcome.

I couldn't come up with a good description either, but I thought it would be good to at least add a link to this diagram, which shows the differences in the behavior of each backoff function.

However, it seems that this diagram doesn't exist in English😓

In Japanese: https://docs.aws.amazon.com/de_de/sns/latest/dg/sns-message-delivery-retries.html#creating-delivery-policy
In English: https://docs.aws.amazon.com/sns/latest/dg/sns-message-delivery-retries.html

I would appreciate it if you could consult with the maintainer again.

I would appreciate it if you could consult with the maintainer again.

As in a CDK maintainer?

Yes! After my approval, you need to get approval from the CDK maintainer. I would appreciate it if you could kindly check it at that time!

Ah right np, I thought you might have meant someone in the SNS team or something 👍

Ref: https:/aws/aws-cdk/wiki/CDK-Community-PR-Reviews

LaurenceWarne · 2024-07-11T19:42:29Z

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** The minimum delay for a retry (in seconds). Must be at least 1 and not exceed `maxDelayTarget`.
+ * @default 20
+ */
+ readonly minDelayTarget: number;


Only, this and the next two appear to be mandatory if healthyRetryPolicy is specified, thought I can't find any specific doc for this.

Would it be correct to assume that if these are not set, it will result in an error during deployment? If so, I am curious when the default values would be set.

If I recall correctly from testing, it's required if and only if healthyRetryPolicy itself is specified, and so the default values would be set in the case healthyRetryPolicy is not specified (perhaps it's confusing to mention a default in the doc 🤔)

How about making these two props optional and explicitly setting the default values documented within the L2 construct? These properties may be required in CloudFormation, but I felt there is no need to make them mandatory in the L2 construct.

readonly minDelayTarget?: Duration; readonly maxDelayTarget?: Duration;

const minDelayTarget = healthyRetryPolicy.minDelayTarget ?? Duration.seconds(20); const maxDelayTarget = healthyRetryPolicy.maxDelayTarget ?? Duration.seconds(20);

pahud · 2024-07-30T15:29:12Z

Hi

This PR has been pending for community review for a while. Please consider posting it on the #contributing channel in cdk.dev. Community members will be on the lookout there as well for possible reviews.

Check How to get your P2 PR community reviewed for more details.

LaurenceWarne · 2024-07-31T16:02:55Z

Thanks, I've posted on the channel.

badmintoncryer

Thank you for creating such a wonderful PR! I’ve quickly reviewed it!

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** Linear.
+ */


packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** Linear.
+ */


packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** Geometric.
+ */


badmintoncryer · 2024-09-21T00:30:12Z

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ * Algorithms which can be used by SNS to calculate the delays associated with all of the retry attempts between the first and last retries in the backoff phase.
+ */
+export enum BackoffFunction {
+ /** Arithmetic.


I couldn't come up with a good description either, but I thought it would be good to at least add a link to this diagram, which shows the differences in the behavior of each backoff function.

However, it seems that this diagram doesn't exist in English😓

In Japanese: https://docs.aws.amazon.com/de_de/sns/latest/dg/sns-message-delivery-retries.html#creating-delivery-policy
In English: https://docs.aws.amazon.com/sns/latest/dg/sns-message-delivery-retries.html

I would appreciate it if you could consult with the maintainer again.

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** The maximum number of deliveries per second, per subscription.
+ * @default - no throttling
+ */


badmintoncryer · 2024-09-21T01:00:03Z

packages/aws-cdk-lib/aws-sns/lib/delivery-policy.ts

+ /** The minimum delay for a retry (in seconds). Must be at least 1 and not exceed `maxDelayTarget`.
+ * @default 20
+ */
+ readonly minDelayTarget: number;


Would it be correct to assume that if these are not set, it will result in an error during deployment? If so, I am curious when the default values would be set.

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ if (healthyRetryPolicy.minDelayTarget < 1 || healthyRetryPolicy.minDelayTarget > delayTargetLimit) {
+ throw new Error(`minDelayTarget must be between 1 and ${delayTargetLimit} inclusive`);
+ }


packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ if (healthyRetryPolicy.maxDelayTarget < 1 || healthyRetryPolicy.maxDelayTarget > delayTargetLimit) {
+ throw new Error(`maxDelayTarget must be between 1 and ${delayTargetLimit} inclusive`);
+ }
+ if (healthyRetryPolicy.minDelayTarget > healthyRetryPolicy.maxDelayTarget) {
+ throw new Error('minDelayTarget must not exceed maxDelayTarget');
+ }


LaurenceWarne · 2024-09-22T10:26:51Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ throw new Error('minDelayTarget must not exceed maxDelayTarget');
+ }
+ const numRetriesLimit = 100;
+ if (healthyRetryPolicy.numRetries < 0 || healthyRetryPolicy.numRetries > numRetriesLimit) {


Yeah good catch

Has the integer check been added?

Sorry, forgot to push a commit 😅

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

@@ -136,6 +144,43 @@ export class Subscription extends Resource {
 throw new Error('Subscription role arn is required field for subscriptions with a firehose protocol.');
 }

+ if (props.deliveryPolicy) {


LaurenceWarne · 2024-09-22T09:58:38Z

Thank you for creating such a wonderful PR! I’ve quickly reviewed it!

Thanks for the review! Hopefully I've addressed everything

badmintoncryer

Thank you for the revisions. I’ve added a few more comments. Once you address those, I plan to approve.

badmintoncryer · 2024-09-22T14:34:14Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ if (healthyRetryPolicy.numRetries < 0 || healthyRetryPolicy.numRetries > numRetriesLimit) {
+ throw new Error(`numRetries must be between 0 and ${numRetriesLimit} inclusive`);
+ }
+ if (healthyRetryPolicy.numNoDelayRetries && healthyRetryPolicy.numNoDelayRetries < 0) {
+ throw new Error('numNoDelayRetries must be zero or greater');
+ }
+ if (healthyRetryPolicy.numMinDelayRetries && healthyRetryPolicy.numMinDelayRetries < 0) {
+ throw new Error('numMinDelayRetries must be zero or greater');
+ }
+ if (healthyRetryPolicy.numMaxDelayRetries && healthyRetryPolicy.numMaxDelayRetries < 0) {
+ throw new Error('numMaxDelayRetries must be zero or greater');
+ }


Cloud you please show the props value like below?

throw new Error(`numRetries must be between 0 and ${numRetriesLimit} inclusive, got: ${healthyRetryPolicy.numRetries}`);

badmintoncryer

Thank you for you contribution! I've approved this, but I kindly request a minor correction.

badmintoncryer · 2024-09-25T00:46:12Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ minDelayTarget: (healthyRetryPolicy.minDelayTarget === undefined)? 20 : healthyRetryPolicy.minDelayTarget.toSeconds(),
+ maxDelayTarget: (healthyRetryPolicy.maxDelayTarget === undefined)? 20 : healthyRetryPolicy.maxDelayTarget.toSeconds(),
+ numRetries: (healthyRetryPolicy.numRetries === undefined)? 3: healthyRetryPolicy.numRetries,


Suggested change

minDelayTarget: (healthyRetryPolicy.minDelayTarget === undefined)? 20 : healthyRetryPolicy.minDelayTarget.toSeconds(),

maxDelayTarget: (healthyRetryPolicy.maxDelayTarget === undefined)? 20 : healthyRetryPolicy.maxDelayTarget.toSeconds(),

numRetries: (healthyRetryPolicy.numRetries === undefined)? 3: healthyRetryPolicy.numRetries,

minDelayTarget: (healthyRetryPolicy.minDelayTarget === undefined) ? 20 : healthyRetryPolicy.minDelayTarget.toSeconds(),

maxDelayTarget: (healthyRetryPolicy.maxDelayTarget === undefined) ? 20 : healthyRetryPolicy.maxDelayTarget.toSeconds(),

numRetries: (healthyRetryPolicy.numRetries === undefined) ? 3: healthyRetryPolicy.numRetries,

LaurenceWarne · 2024-09-25T12:37:47Z

Thank you for you contribution! I've approved this, but I kindly request a minor correction.

👍 And thanks again, I really appreciate you taking the time 🙂

GavinZZ

Thanks for contributing! Left some feedback.

GavinZZ · 2024-10-10T22:27:54Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

 });

 }

+ private renderDeliveryPolicy(deliveryPolicy: DeliveryPolicy, protocol: SubscriptionProtocol): any {
+ if (protocol !== SubscriptionProtocol.HTTP && protocol !== SubscriptionProtocol.HTTPS) {


nit:

Suggested change

if (protocol !== SubscriptionProtocol.HTTP && protocol !== SubscriptionProtocol.HTTPS) {

if (![SubscriptionProtocol.HTTP, SubscriptionProtocol.HTTPS].includes(protocol)) {

GavinZZ · 2024-10-10T22:30:27Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ const minDelayTarget = healthyRetryPolicy.minDelayTarget;
+ const maxDelayTarget = healthyRetryPolicy.maxDelayTarget;
+ if (minDelayTarget !== undefined) {
+ if (minDelayTarget.toMilliseconds() % 1000 !== 0) {


I think you can directly use toSeconds() which will check if it's a integer second

I thought about it, but I thought this would be better as the error toSeconds raises doesn't have the same context we can add here?

GavinZZ · 2024-10-10T22:32:27Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ }
+ }
+
+ const numRetriesLimit = 100;


It's good intention to not use the magic value and define it explicitly. But given the number of validations, I actually think it's more readable to directly use the value like 100 and 3600 in the if condition checks. What do you think?

I think I prefer it how it is now? I agree it might be more readable, but I'd prefer keeping hard-coding to a minimum to make refactors safer/simplier, etc

GavinZZ · 2024-10-10T22:33:25Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ }
+ return {
+ healthyRetryPolicy: healthyRetryPolicy ? {
+ // minDelayTarget, maxDelayTarget and numRetries are (empirically) mandatory when healthyRetryPolicy is specified,


can you explain what you meant by empirically mandatory)? I do see that they do have default values set.

Does #30830 (comment) answer your question (also see last comment in that thread as well as to why they're optional)?

GavinZZ · 2024-10-10T22:35:07Z

packages/aws-cdk-lib/aws-sns/lib/subscription.ts

+ numNoDelayRetries: healthyRetryPolicy.numNoDelayRetries,
+ numMinDelayRetries: healthyRetryPolicy.numMinDelayRetries,
+ numMaxDelayRetries: healthyRetryPolicy.numMaxDelayRetries,


Should these have default values?

Similar to #30830 (comment) question, I guess it would depend on your view on that 😅?

My thinking was that if Cloudformation doesn't require these to be specified when healthyRetryPolicy is specified, we can omit them here and have Cloudformation figure the defaults for us?

Pull request has been modified.

aws-cdk-automation · 2024-10-14T19:14:51Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 442d8c9
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

feat(sns): add delivery policy to sns subscriptions

e801ab3

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jul 11, 2024

aws-cdk-automation requested a review from a team July 11, 2024 19:36

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 11, 2024

LaurenceWarne commented Jul 11, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 11, 2024

github-actions bot mentioned this pull request Jul 15, 2024

Weekly PR metrics report #30858

Closed

github-actions bot mentioned this pull request Aug 1, 2024

Monthly PR metrics report #30999

Open

badmintoncryer suggested changes Sep 21, 2024

View reviewed changes

LaurenceWarne added 2 commits September 21, 2024 10:26

Merge branch 'main' into feat/add-delivery-policy-to-sns-subscriptions

209fc7d

Address comments

6b23261

Add more validation

2ea4006

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 22, 2024

badmintoncryer suggested changes Sep 22, 2024

View reviewed changes

Address more comments

9499d82

badmintoncryer approved these changes Sep 25, 2024

View reviewed changes

Tweak ternary operator spacing

50b1614

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 25, 2024

GavinZZ previously requested changes Oct 10, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 10, 2024

Tweak subscription protocol conditional

442d8c9

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 14, 2024

	if (protocol !== SubscriptionProtocol.HTTP && protocol !== SubscriptionProtocol.HTTPS) {
	if (![SubscriptionProtocol.HTTP, SubscriptionProtocol.HTTPS].includes(protocol)) {

feat(sns): add delivery policy to sns subscriptions #30830

Are you sure you want to change the base?

feat(sns): add delivery policy to sns subscriptions #30830

Conversation

LaurenceWarne commented Jul 11, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

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

badmintoncryer Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

pahud commented Jul 30, 2024

LaurenceWarne commented Jul 31, 2024

badmintoncryer left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

LaurenceWarne commented Sep 22, 2024

badmintoncryer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badmintoncryer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurenceWarne commented Sep 25, 2024

GavinZZ 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

LaurenceWarne Oct 14, 2024 • edited Loading

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

aws-cdk-automation commented Oct 14, 2024

AWS CodeBuild CI Report

LaurenceWarne commented Jul 11, 2024 •

edited

Loading

badmintoncryer Sep 22, 2024 •

edited

Loading

badmintoncryer left a comment •

edited

Loading

LaurenceWarne Oct 14, 2024 •

edited

Loading