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

Support DLQ configuration for Lambda CDK Construct #660

Closed
SeekerWing opened this issue Sep 4, 2018 · 8 comments
Closed

Support DLQ configuration for Lambda CDK Construct #660

SeekerWing opened this issue Sep 4, 2018 · 8 comments

Comments

@SeekerWing
Copy link
Contributor

AWS Lambda has support for DLQ as defined in https://docs.aws.amazon.com/lambda/latest/dg/dlq.html

This is defined in CFN via https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-function.html#cfn-lambda-function-deadletterconfig

This issue is to add the feature to the Lambda CDK Construct. Some discussion points before implementing the feature:

  • Should we add support for SQS only or SNS as well as DLQ? (SNS will be tricky since current L2 implementation of aws-sns has a dependency on aws-lambda to add lambda subscription to a SNS topic)
  • Should we as user to provide a Queue/Topic object or should the user only have to provide a boolean value and we can create the Queue/Topic in the Lambda function.
  • In case we go with the Boolean approach, should we set definitions/default definitions for Name; Retention Period; Visibility Timeout or ask user to provide all of them.
@eladb
Copy link
Contributor

eladb commented Sep 4, 2018

Should we add support for SQS only or SNS as well as DLQ? (SNS will be tricky since current L2 implementation of aws-sns has a dependency on aws-lambda to add lambda subscription to a SNS topic)

Good point about SNS <=> Lambda. Since we built the SNS L2 we have evolved our approach/guidelines for L2s a little and I think the correct way to model this relationship is using another module aws-sns-targets which will only include an interface e.g. ISubscriptionTarget and then this interface will be implemented by Lambda (then both SNS and Lambda depend on the interfaces module).

In the meantime, you can start with SQS. I think that's the most common use case anyway.

Should we as user to provide a Queue/Topic object or should the user only have to provide a boolean value and we can create the Queue/Topic in the Lambda function.
In case we go with the Boolean approach, should we set definitions/default definitions for Name; Retention Period; Visibility Timeout or ask user to provide all of them.

Our main design tenet is to optimize for the common use case with minimum amount of configuration. So I think it should be possible to just "flip a switch" and enable DLQ with sensible defaults (something like { deadLetterQueue: true }).

However, we always make sure to expose the surface area for customizations, so perhaps we can also include { deadLetterQueueOptions?: DeadLetterQueueOptions } which will allow users to customize the behavior, including supplying their own QueueRef.

In such cases ("enable + options"), I would recommend to automatically "enable" if the user supplies only "options", so just specifying { deadLetterQueueOptions: { bla } } should auto-enable the DLQ, because that's likely what the user expects.

@SeekerWing
Copy link
Contributor Author

PR #663

@SeekerWing SeekerWing changed the title Support DLQ configuration for Lambda CDK Constrcut Support DLQ configuration for Lambda CDK Construct Sep 5, 2018
@eladb eladb closed this as completed Sep 13, 2018
@pierreozoux
Copy link

Can we have sns now? should I open a new issue to track that? Thanks :)

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 18, 2019

Yes, please do open a new issue.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 4, 2019

I think the only way to feasibly do this is by moving the SNS subscriptions into an aws-sns-subscribers package.

@eladb?

@eladb
Copy link
Contributor

eladb commented Jun 4, 2019

@rix0rrr I am not sure I fully understand the problem

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 4, 2019

Because of

class Topic {
  public subscribeLambda(fn: lambda.IFunction) { }
}

We have aws-sns => aws-lambda.

To be able to have:

interface FunctionProps {
  deadLetterQueueTopic: sns.ITopic;
}

We need to have aws-lambda => aws-sns.

We can't have both, because circular reference.


I suppose we could also have aws-lambda-deadletter-targets, but honestly feels like the SNS subscribers package seems like a more appropriate one to make.

Downside: it does reduce the catchiness of our "hello world" sample somewhat. But it's better long-term, and consistent with the rest of the packages. Choices choices :(

@eladb
Copy link
Contributor

eladb commented Jun 4, 2019

Yap, I agree. The subscribeXxx methods are ancient. We should definitely separate SNS integrations into their own module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants