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

RFC: Combine Test and Prod permissions for Api Events #1102

Closed
ShreyaGangishetty opened this issue Aug 27, 2019 · 9 comments
Closed

RFC: Combine Test and Prod permissions for Api Events #1102

ShreyaGangishetty opened this issue Aug 27, 2019 · 9 comments

Comments

@ShreyaGangishetty
Copy link

ShreyaGangishetty commented Aug 27, 2019

Issue Summary
Currently, SAM creates two permissions (one for production and one for test) for every API event defined in the CloudFormation stack. The test permission is needed for the Test button to work on the console. Because of this extra permission for the test button, the limit of 200 resources per stack can be quickly reached. Related Issues: #285, #337

Design Spec
The approach is to combine both permisions into a single permission and give the exact same functionality without a spec change. This is achieved by using a "*" in place of the stage name in the single permission, which is exactly what the "Test" permission does today.

Sample Template:

AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31
Resources:
  HelloWorldFunction:
    Type: AWS::Serverless::Function
    Properties:
      InlineCode: |
        print("hello world") 
      Handler: app.lambda_handler
      Runtime: python3.6
      Events:
        HttpGetUserExample:
          Type: Api
          Properties:
            Path: '/users/userid'
            Method: GET

For this sample template, the resources generated by existing approach look like:

    ...
    "HelloWorldFunctionHttpGetUserExamplePermissionProd": {
      "Type": "AWS::Lambda::Permission",
      "Properties": {
        "Action": "lambda:invokeFunction",
        "Principal": "apigateway.amazonaws.com",
        "FunctionName": {
          "Ref": "HelloWorldFunction"
        },
        "SourceArn": {
          "Fn::Sub": [
            "arn:aws:execute-api:${AWS::Region}:${AWS::AccountId}:${__ApiId__}/${__Stage__}/GET/users/userid",
            {
              "__Stage__": "Prod",
              "__ApiId__": {
                "Ref": "ServerlessRestApi"
              }
            }
          ]
        }
      }
    },
    "HelloWorldFunctionHttpGetUserExamplePermissionTest": {
      "Type": "AWS::Lambda::Permission",
      "Properties": {
        "Action": "lambda:invokeFunction",
        "Principal": "apigateway.amazonaws.com",
        "FunctionName": {
          "Ref": "HelloWorldFunction"
        },
        "SourceArn": {
          "Fn::Sub": [
            "arn:aws:execute-api:${AWS::Region}:${AWS::AccountId}:${__ApiId__}/${__Stage__}/GET/users/userid",
            {
              "__Stage__": "*",
              "__ApiId__": {
                "Ref": "ServerlessRestApi"
              }
            }
          ]
        }
      }
    },
    ...

With the proposed approach the resources generated would look like:

    ...
    "HelloWorldFunctionHttpGetUserExamplePermissionProd": {
      "Type": "AWS::Lambda::Permission", 
      "Properties": {
        "Action": "lambda:invokeFunction", 
        "Principal": "apigateway.amazonaws.com", 
        "FunctionName": {
          "Ref": "HelloWorldFunction"
        }, 
        "SourceArn": {
          "Fn::Sub": [
            "arn:aws:execute-api:${AWS::Region}:${AWS::AccountId}:${__ApiId__}/${__Stage__}/GET/users/userid", 
            {
              "__Stage__": "*", 
              "__ApiId__": {
                "Ref": "ServerlessRestApi"
              }
            }
          ]
        }
      }
    },
    ...

This approach will reduce the number of permissions created for api events by half. This approach gives the same functionality as the existing approach without any change in the spec. Hence, it is backward compatible.

@sudeepd
Copy link

sudeepd commented Aug 28, 2019

This looks good. Thanks for getting to this eventually.

@ferdingler
Copy link

ferdingler commented Aug 28, 2019

Could SAM just create one permission that works for both without specifying any flag? like this

{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "HelloLambdaPermissionApiGateway-123",
      "Effect": "Allow",
      "Principal": {
        "Service": "apigateway.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:us-east-1:0123456789:function:hello",
      "Condition": {
        "ArnLike": {
          "AWS:SourceArn": "arn:aws:execute-api:us-east-1:0123456789:{api-id}/*/*"
        }
      }
    }
  ]
}

@jjonek
Copy link

jjonek commented Sep 2, 2019

I'd be happy with whatever way to reduce the number of generated permissions, the situation is getting unbearable with some deployments, especially in cases when the permission resources end up being replaced. In addition to disabling the test permissions I'd also like having the option to disable permission generation completely and bringing my own permissions instead.

@ShreyaGangishetty
Copy link
Author

@ferdingler This is an interesting approach. We had a discussion about this approach in the team and we did a deep dive on this approach. Both the permissions look similar except for the change in {stage} part of the arn.

ProdPermission Arn: ("AWS:SourceArn": "arn:aws:execute-api:us-east-1:0123456789:{api-id}/{stage}/{path}")
TestPermission Arn: ("AWS:SourceArn": "arn:aws:execute-api:us-east-1:0123456789:{api-id}/*/{path}")

If we make a change to the ProdPermission arn by replacing {stage} with *, we can combine both permissions into one without changing the spec.
The single permission arn would look like this:
"AWS:SourceArn": "arn:aws:execute-api:us-east-1:0123456789:{api-id}/*/{path}"
This reduces the number of permissions by half by giving exact same functionality with two permissions. As we are not removing/revoking any permissions, this approach seems to be backward compatible.

@ShreyaGangishetty ShreyaGangishetty changed the title RFC: Disable Test permissions for Api Events RFC: Combine Test and Prod permissions for Api Events Sep 5, 2019
@ferdingler
Copy link

@ShreyaGangishetty awesome, thanks for considering my input!

@jlhood
Copy link
Contributor

jlhood commented Sep 6, 2019

@ferdingler We ended up going with your approach (see #1119), which is a much simpler change. Really appreciate your feedback!

@kriztoph
Copy link

Any idea when this will be released?

@praneetap
Copy link
Contributor

@kriztoph this is going out in v1.15, which is our next planned release. It will be coming out soon!

@ShreyaGangishetty
Copy link
Author

Closing this issue as v1.15.0 is released

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

No branches or pull requests

8 participants