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

DeadLetterConfig support #3609

Merged
merged 3 commits into from May 23, 2017
Merged

DeadLetterConfig support #3609

merged 3 commits into from May 23, 2017

Conversation

pmuens
Copy link
Contributor

@pmuens pmuens commented May 12, 2017

What did you implement:

Closes #2982

Add config variable for function-based DeadLetterConfig support.

How did you implement it:

Add the onError config variable for functions.

How can we verify it:

Deploy this service. Make your Lambda error out.

service: service

provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
    onError: arn:aws:sns:us-east-1:XXXXX:test # SNS

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens added this to the 1.14 milestone May 12, 2017
@pmuens pmuens changed the title First pass on DeadLetterConfig support DeadLetterConfig support May 12, 2017
@pmuens pmuens force-pushed the dead-letter-config-support branch from 40ec011 to a0c8042 Compare May 16, 2017 09:14
@pmuens
Copy link
Contributor Author

pmuens commented May 17, 2017

Just a quick update on this one.

Debugged it today and it seems like the IamRoleLambdaExectuion which includes the inline policies for SNS and SQS is not updated by CloudFormation when deploying.

The initial deployment (from scratch) will suceed, however changing an existing stack errors out since the IAM Role is not updated.

The weird part is that the generated CloudFormation template is correct and the CloudFormation console tells you that everything was updated correctly. But if you look into the policies for the role in the IAM console you can see that it was not updated. 🤔

Don't know what's wrong here. A DependsOn is already in place for the Lambda function resource so that the function is created / updated after the IamRoleLambdaExecution is created / updated.

@eahefnawy do you have any idea what might be missing here?


Step-by-step list to reproduce the behavior above (:top:)

[0. Make sure that you use a brand new service]

  1. Create a new SNS topic (save the arn since you need to use it later)
  2. Create a new SQS topic (save the arn since you need to use it later)
  3. Add the onError config (just ONE) with the arn as described in the PR description (it doesn't matter if you use sns or sqs)
  4. Run serverless deploy
  5. Change the onError config so that it uses the sns arn instead of the sqs arn (or the other way around)
  6. Run serverless deploy
  7. Deployment will fail with an error message that the permissions are missing
  8. Inspect the update template in the .serverless directory to see that the inline policy is set correctly
  9. Look into the CloudFormation AWS console to see that the IamRoleLamdaExecution resource was updated before the Lambda function was updated
  10. Look into the inline policies in the IAM AWS console for the IamRoleLambdaExecution to see that it was not updated

@pmuens pmuens force-pushed the dead-letter-config-support branch from a0c8042 to b2e21b6 Compare May 17, 2017 11:13
@pmuens
Copy link
Contributor Author

pmuens commented May 22, 2017

I did another deep dive into this.

IMHO this is definitely smth. on AWS end. The weird thing is that the update of the IamRoleLambdaExecution only takes a few seconds (~2 seconds in my case) and won't update the inline policy. The Lambda function is always updated adter the IamRoleLambdaExecution is updated, so the DependsOn works quite fine.

Another indicator for a problem on AWS end is that you'll see the correct inline-policy if you look into the compiled CloudFormation template which is uploaded to the deployment bucket and used during the stack update process.

Not quite sure how we should proceed here...

We could use permission resources we merge into the template. I tested this but it's also not working flawlessly on my machine and it looks like it's not the best practice when dealing with DLQ setup through CloudFormation (see: http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-deadletterconfig.html).

Do you have an idea how we can make this work @eahefnawy ?

@HyperBrain
Copy link
Member

Couldn't it just be that we set a fixed name for the inline policy? Did you try to change the inline policy name to have some unique appendix? IMO the update might work if the inline policy gets a different name on each build.

The inline policy name is set here:
https://github.com/serverless/serverless/blob/master/lib/plugins/aws/lib/naming.js#L94-L106

@pmuens
Copy link
Contributor Author

pmuens commented May 22, 2017

@HyperBrain that's a good hint. Thanks for that! 👍

The only thing which makes me curious is that we also add other inline policies (e.g. for stream events or when merging iamRoleStatements --> https://github.com/pmuens/serverless-crud/blob/master/serverless.yml#L8-L18) and everything works fine in that case. 🤔

@pmuens pmuens force-pushed the dead-letter-config-support branch from b2e21b6 to 48a4167 Compare May 23, 2017 06:44
@pmuens pmuens force-pushed the dead-letter-config-support branch from 48a4167 to 0db25b7 Compare May 23, 2017 06:50
Copy link
Contributor Author

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Just updated the code so that only SNS topic arns are supported.

Tested everything again and it's working fine. Will merge after the build passed :shipit:

@pmuens pmuens merged commit e72743d into master May 23, 2017
@pmuens pmuens deleted the dead-letter-config-support branch May 23, 2017 06:58
@neowulf
Copy link

neowulf commented Oct 31, 2017

Couldn't find the ticket related to supporting SQS. Could that ticket be kindly link here for tracking?

What's the current status on supporting SQS as a dead letter queue? Is it recommended to implement the corresponding cloudformation instructions?

Thanks!

@neowulf
Copy link

neowulf commented Nov 3, 2017

@pmuens - silly question - was the code generating the cloudformation template by referring to the return value of the SQS resource?

I ran into similar symptoms as described earlier in the ticket. The return value of the SQS resource is the http endpoint! 🙄 One would have to refer to the Arn attribute of the SQS resource.

Simply adding the following to my serverless.yml, I was able to configure a Dead Letter Queue for my lambda function using Cloudformation within serverless.yml:

provider:
  iamRoleStatements:
    - Sid: LambdaDLQPermissions
      Effect: Allow
      Action:
        - "sqs:SendMessage*"
      Resource:
        - "Fn::GetAtt":
          - LambdaFunctionDeadLetterQueue
          - Arn

functions:
  MyFunction:
    handler: handler.XXXXXX

resources:
  Resources:
    MyFunctionLambdaFunction:
      Type: "AWS::Lambda::Function"
      Properties:
        DeadLetterConfig:
          TargetArn:
            "Fn::GetAtt":
              - LambdaFunctionDeadLetterQueue
              - Arn

    LambdaFunctionDeadLetterQueue:
      Type: "AWS::SQS::Queue"
      Properties:
        QueueName: "MyFunctionDLQ"
        MessageRetentionPeriod: 1209600 # 14 days in seconds

@kalinchernev
Copy link

kalinchernev commented Oct 11, 2018

@neowulf thank you for sharing your solution, it worked well for me! Even removed serverless-plugin-lambda-dead-letter totally from the stack, no need.

This part from the iamRoleStatements was also not needed. I believe it's stayed from a previous set of attempts as this section is "CloudFormation-aware".

      Resource:
        - "Fn::GetAtt":
          - LambdaFunctionDeadLetterQueue
          - Arn

yhuard pushed a commit to ec-europa/eubfr-data-lake that referenced this pull request Dec 14, 2018
…mbda handlers - EUBFR-204 (#160)

# PR description

The goal of this pull request is to solve the hard limitations in execution time in AWS Lambda.

PR overview:

- new service `@eubfr/ingestion-dead-letter-queue` is a regular serverless service which provides an SQS Dead Letter Queue for general use of other services. The service exposes a dead letter SQS queue (it also exports a value in CF outputs) to which other services push messages when they fail because of time limitation.
- new service `@eubfr/compute-ecs` which is not a serverless service, but a general node project with a Docker container which can be deployed to AWS ECS. The role of the container is to execute a `runner.js` node script which simply downloads the code of a dynamically input lambda function and executes it without time limitations.
- service changes: all business critical services have been updated to be able to make use of the new setup. The way dead letter queues are "attached" is by `Resources` section and not serverless' `onError`, because of [this](serverless/serverless#3609 (comment)).
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

Successfully merging this pull request may close these issues.

None yet

4 participants