Skip to content

Conversation

praneetap
Copy link
Contributor

@praneetap praneetap commented Dec 13, 2019

Issue #, if available:
#1269
Description of changes:
Adding support for Event Destinations and async invocation settings (max retry and event max age). More about the Cloudformation property here.
There are four destination resources supported - SQS, SNS, EventBridge and Lambda Function.

MyFunction:
   Type: 'AWS::Serverless::Function'
   Properties:
     EventInvokeConfig:
       MaximumEventAgeInSeconds: Integer (Min: 60, Max: 21600)
       MaximumRetryAttempts: Integer (Min: 0, Max: 2)
       DestinationConfig:
         OnSuccess:
           Type: [SQS | SNS | EventBridge | Function]
           Destination: ARN of [SQS | SNS | EventBridge | Function]
         OnFailure: 
           Type: [SQS | SNS | EventBridge | Function]
           Destination: ARN of [SQS | SNS | EventBridge | Function]

Destination property is required for EventBridge event bus and Lambda function. But if the type is SQS/SNS, and the destination is not specified, SAM will auto generate a SQS Queue/ SNS topic for you. To refer the auto generated resources, you can use FunctionLogicalId.DestinationQueue or FunctionLogicalId.DestinationTopic.

The destination property also supports intrinsics. Special handling for single level nested If has been added.

Description of how you validated changes:
Deployed end to end template in examples/
Updated the stack with flipped condition in the same template.

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected
  • Add/update example to examples/2016-10-31

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

@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #1321 into develop will decrease coverage by 0.07%.
The diff coverage is 92.46%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1321      +/-   ##
===========================================
- Coverage    94.48%   94.41%   -0.08%     
===========================================
  Files           78       78              
  Lines         4388     4529     +141     
  Branches       871      903      +32     
===========================================
+ Hits          4146     4276     +130     
- Misses         116      119       +3     
- Partials       126      134       +8
Impacted Files Coverage Δ
samtranslator/plugins/globals/globals.py 99.05% <ø> (ø) ⬆️
samtranslator/model/intrinsics.py 95.83% <100%> (+0.52%) ⬆️
samtranslator/model/iam.py 94.28% <100%> (+1.69%) ⬆️
samtranslator/translator/translator.py 99.03% <100%> (ø) ⬆️
samtranslator/model/lambda_.py 93.1% <100%> (+0.79%) ⬆️
samtranslator/model/sns.py 100% <100%> (ø) ⬆️
samtranslator/model/sam_resources.py 93.97% <90.83%> (-1.16%) ⬇️
...lator/plugins/application/serverless_app_plugin.py 83.55% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd10d3...0edaa93. Read the comment docs.

Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Diving into the main section next, also going to run some additional tests with conditions.

Something that seems to be missing with conditions is applying a condition to a Function at the resource level when this function (a) references an event bus and other resources and (b) when this function creates either an SQS or SNS resource and make sure the condition is applied to that resources as well:

MyFunction:
  Condition: C1
  Type: AWS::Serverless::Function
  Properties:
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just repeating the previous make_conditional- we could override that one instead like this:

def make_conditional(condition, true_data, false_data={'Ref': 'AWS::NoValue'}):

Keeps the same functionality and doesn't copy code.

@praneetap
Copy link
Contributor Author

Diving into the main section next, also going to run some additional tests with conditions.

Something that seems to be missing with conditions is applying a condition to a Function at the resource level when this function
(a) references an event bus and other resources and
(b) when this function creates either an SQS or SNS resource and make sure the condition is applied to that resources as well:

MyFunction:
  Condition: C1
  Type: AWS::Serverless::Function
  Properties:
    ...

(a) references an event bus and other resources and
In this case the function passes through the intrinsic to cfn, and SAM wont try to resolve it since we don't need to.
(b) when this function creates either an SQS or SNS resource and make sure the condition is applied to that resources as well:
This should exist. Since the destination is an either/or, either an existing sqs resource or a created one, the if conditions on the generated policy/ resource is applied (i tested this)

@keetonian
Copy link
Contributor

Here's what I was talking about. The SQS/SNS resources (if SAM generates them) need to have the condition applied to them that's on the Function.

Input:

Conditions:
  MyCondition:
    Fn::Equals:
      - true
      - true
Parameters:
  SNSArn:
    Type: String
    Default: my-sns-arn
Globals:
  Function:
    AutoPublishAlias: live
    EventInvokeConfig:
      MaximumEventAgeInSeconds: 70
      MaximumRetryAttempts: 1
      DestinationConfig:
        OnSuccess:
          Type: SQS
        OnFailure: 
          Type: SNS
          Destination: !Ref SNSArn

Resources:
  MyTestFunction:
    Condition: MyCondition
    Type: AWS::Serverless::Function
    Properties:
      InlineCode: |
        exports.handler = function(event, context, callback) {
            var event_received_at = new Date().toISOString();
            console.log('Event received at: ' + event_received_at);
            console.log('Received event:', JSON.stringify(event, null, 2));
            if (event.Success) {
                console.log("Success");
                context.callbackWaitsForEmptyEventLoop = false;
                callback(null);
            } else {
                console.log("Failure");
                context.callbackWaitsForEmptyEventLoop = false;
                callback(new Error("Failure from event, Success = false, I am failing!"), 'Destination Function Error Thrown');
            }
        };      
      Handler: index.handler
      Runtime: nodejs10.x
      MemorySize: 1024

Output:

---
Conditions:
  MyCondition:
    Fn::Equals:
    - true
    - true
Resources:
  MyTestFunctionEventInvokeConfigOnSuccessQueue: # needs to have the condition applied to it
    Type: AWS::SQS::Queue
    Properties: {}
  MyTestFunctionRole:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Version: '2012-10-17'
        Statement:
        - Action:
          - sts:AssumeRole
          Effect: Allow
          Principal:
            Service:
            - lambda.amazonaws.com
      ManagedPolicyArns:
      - arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole
      Policies:
      - PolicyName: MyTestFunctionEventInvokeConfigSQSPolicy
        PolicyDocument:
          Statement:
          - Action: sqs:SendMessage
            Resource:
              Fn::GetAtt:
              - MyTestFunctionEventInvokeConfigOnSuccessQueue
              - Arn
            Effect: Allow
      - PolicyName: MyTestFunctionEventInvokeConfigSNSPolicy
        PolicyDocument:
          Statement:
          - Action: sns:publish
            Resource: my-sns-arn
            Effect: Allow
      Tags:
      - Value: SAM
        Key: lambda:createdBy
    Condition: MyCondition
  MyTestFunctionEventInvokeConfig:
    Type: AWS::Lambda::EventInvokeConfig
    Properties:
      MaximumEventAgeInSeconds: 70
      MaximumRetryAttempts: 1
      DestinationConfig:
        OnSuccess:
          Destination:
            Fn::GetAtt:
            - MyTestFunctionEventInvokeConfigOnSuccessQueue
            - Arn
        OnFailure:
          Destination: my-sns-arn
      FunctionName:
        Ref: MyTestFunction
      Qualifier: live
    Condition: MyCondition
  MyTestFunction:
    Type: AWS::Lambda::Function
    Properties:
      Code:
        ZipFile: "exports.handler = function(event, context, callback) {\n    var
          event_received_at = new Date().toISOString();\n    console.log('Event received
          at: ' + event_received_at);\n    console.log('Received event:', JSON.stringify(event,
          null, 2));\n    if (event.Success) {\n        console.log(\"Success\");\n
          \       context.callbackWaitsForEmptyEventLoop = false;\n        callback(null);\n
          \   } else {\n        console.log(\"Failure\");\n        context.callbackWaitsForEmptyEventLoop
          = false;\n        callback(new Error(\"Failure from event, Success = false,
          I am failing!\"), 'Destination Function Error Thrown');\n    }\n};      \n"
      Tags:
      - Value: SAM
        Key: lambda:createdBy
      MemorySize: 1024
      Handler: index.handler
      Role:
        Fn::GetAtt:
        - MyTestFunctionRole
        - Arn
      Runtime: nodejs10.x
    Condition: MyCondition
  MyTestFunctionAliaslive:
    Type: AWS::Lambda::Alias
    Properties:
      FunctionVersion:
        Fn::GetAtt:
        - MyTestFunctionVersionefc149382f
        - Version
      FunctionName:
        Ref: MyTestFunction
      Name: live
    Condition: MyCondition
  MyTestFunctionVersionefc149382f:
    DeletionPolicy: Retain
    Type: AWS::Lambda::Version
    Properties:
      FunctionName:
        Ref: MyTestFunction
    Condition: MyCondition
Parameters:
  SNSArn:
    Default: my-sns-arn
    Type: String

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

So excited about the SNS/SQS auto-generation! This is going to be a powerful feature. 😊

@praneetap
Copy link
Contributor Author

@jlhood and @keetonian Thanks a ton for the comments, cleaned it all up and its ready for review again.

@praneetap praneetap requested a review from jlhood December 16, 2019 05:48
Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Almost there. Mostly documentation-related feedback, but I think there's one miss in the naming of the inline IAM policy for destination permissions.

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Excellent work!

Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Looks good, this should fix failing tests.

@praneetap praneetap requested a review from keetonian December 26, 2019 21:49
@keetonian keetonian merged commit 34ac03e into aws:develop Dec 26, 2019
praneetap added a commit to praneetap/serverless-application-model that referenced this pull request Dec 26, 2019
praneetap added a commit to praneetap/serverless-application-model that referenced this pull request Jan 3, 2020
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.

4 participants