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://github.com/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

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

Pull request has been modified.

mergify · 2024-10-21T20:45:12Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-10-21T20:49:18Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

badmintoncryer · 2024-10-22T01:14:31Z

@mergify update

mergify · 2024-10-22T01:14:55Z

update

❌ Command disallowed due to command restrictions in the Mergify configuration.

any of:
- sender = {{author}}
- sender-permission >= write

adoblas · 2024-11-07T13:10:32Z

@GavinZZ do we have an ETA for this merge?

aws-cdk-automation · 2024-11-12T00:10:33Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

badmintoncryer · 2024-11-12T00:15:33Z

@GavinZZ Could you please merge main branch and approve this PR again?

Pull request has been modified.

codecov · 2024-11-13T18:06:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.17%. Comparing base (be33610) to head (73f7c59).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30830   +/-   ##
=======================================
  Coverage   77.17%   77.17%           
=======================================
  Files         105      105           
  Lines        7169     7169           
  Branches     1315     1315           
=======================================
  Hits         5533     5533           
  Misses       1455     1455           
  Partials      181      181

Flag	Coverage Δ
suite.unit	`77.17% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`77.17% <ø> (ø)`

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

mergify · 2024-11-18T15:37:15Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mwebber · 2024-11-18T15:55:19Z

@LaurenceWarne , @GavinZZ it looks like the reason this PR hasn't proceeded to the next is that we have 2 code coverage test failures (see https://app.codecov.io/gh/aws/aws-cdk/pull/30830/indirect-changes). It looks like the 2 failures are a single problem.

The failures just look like some strange side-effect of the change, not an actual problem with the PR. However, I don't think the system will let you move forward until they are addressed.

Is someone able to resolve those failures? Sorry I have no idea how to fix them myself.

Pull request has been modified.

LaurenceWarne · 2024-11-18T18:03:36Z

The failures just look like some strange side-effect of the change, not an actual problem with the PR. However, I don't think the system will let you move forward until they are addressed.

Is someone able to resolve those failures? Sorry I have no idea how to fix them myself.

Merged master and it looks to have fixed itself

GavinZZ · 2024-11-20T22:18:06Z

Yes, apologize that I didn't get back to you sooner. We've fixed issue with codecov now. Will approve and merge again.

mergify · 2024-11-20T22:18:53Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-11-20T22:23:30Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-11-20T23:43:16Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-11-20T23:43:19Z

AWS CodeBuild CI Report

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

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

github-actions · 2024-11-20T23:43:30Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

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

Closed

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

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

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

93d7130

GavinZZ previously approved these changes Nov 13, 2024

View reviewed changes

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

0d5e979

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

GavinZZ approved these changes Nov 20, 2024

View reviewed changes

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

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

73f7c59

mergify bot merged commit 27babe6 into aws:main Nov 20, 2024
17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2024

LaurenceWarne deleted the feat/add-delivery-policy-to-sns-subscriptions branch November 21, 2024 07:53

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

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

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

mergify bot commented Oct 21, 2024

mergify bot commented Oct 21, 2024

badmintoncryer commented Oct 22, 2024

mergify bot commented Oct 22, 2024

❌ Command disallowed due to command restrictions in the Mergify configuration.

adoblas commented Nov 7, 2024

aws-cdk-automation commented Nov 12, 2024

badmintoncryer commented Nov 12, 2024

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

mergify bot commented Nov 18, 2024

mwebber commented Nov 18, 2024

LaurenceWarne commented Nov 18, 2024

GavinZZ commented Nov 20, 2024

mergify bot commented Nov 20, 2024

mergify bot commented Nov 20, 2024

mergify bot commented Nov 20, 2024

aws-cdk-automation commented Nov 20, 2024

AWS CodeBuild CI Report

github-actions bot commented Nov 20, 2024

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

codecov bot commented Nov 13, 2024 •

edited

Loading