Skip to content

(feat) CloudFormation ApiGateway support design #1234

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

Merged

Conversation

viksrivat
Copy link
Contributor

Issue #, if available:

Description of changes:

Checklist:

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

@viksrivat viksrivat changed the title Feature/cloud formation api gateway design feature/cloud formation api gateway design Jun 18, 2019
@viksrivat viksrivat changed the title feature/cloud formation api gateway design feature/CloudFormation api gateway design Jun 18, 2019
@viksrivat viksrivat changed the title feature/CloudFormation api gateway design feature/CloudFormation ApiGateway design Jun 18, 2019
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

A lot of this is just work smithing. You should do a pass through the design to make sure naming are consistent. For example sam-cli should be SAM CLI.


## The Problem

Customers use sam-cli to run/test their applications by defining their resources in the SAM template. The raw CloudFormation ApiGateway resources are not currently supported to run and test locally in sam-cli. The resource types include AWS::ApiGateway::RestApi, AWS::ApiGateway::Stage, etc while SAM only supports the AWS::Serverless::Api flag. This prevents people that are not defining their resources using the SAM(AWS::Serverless::Api) Api Gateway resources from running and testing locally through sam-cli. Specifically, Customers that generate their CloudFormation code using tools and have hand written CloudFormation templates have AWS::ApiGateway::* types preventing them from running locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • SAM CLI instead of sam-cli
  • ... their resources in the SAM template to ... their resources in a SAM template.
  • include AWS::ApiGateway::RestApi, AWS::ApiGateway::Stage can be shorted to AWS::ApiGateway::*
  • "while SAM only supports the AWS::Serverless::Api flag." This doesn't really make sense. SAM is a super set of CloudFormation so SAM defines the AWS::Serverless::Api Resource. I think what you are trying to say here is that sam local start-api only supports AWS::Serverless::Api. AWS::Serverless::Api is a Resource not a flag.
  • "This prevents people that are not defining their resources using the SAM(AWS::Serverless::Api) Api Gateway resources from running and testing locally through sam-cli." -> This prevents customers who have built/deployed services using the raw ApiGateway Resources or who have used tools to generate CloudFormation, like AWS CDK, from testing locally through SAM CLI.


Customers use sam-cli to run/test their applications by defining their resources in the SAM template. The raw CloudFormation ApiGateway resources are not currently supported to run and test locally in sam-cli. The resource types include AWS::ApiGateway::RestApi, AWS::ApiGateway::Stage, etc while SAM only supports the AWS::Serverless::Api flag. This prevents people that are not defining their resources using the SAM(AWS::Serverless::Api) Api Gateway resources from running and testing locally through sam-cli. Specifically, Customers that generate their CloudFormation code using tools and have hand written CloudFormation templates have AWS::ApiGateway::* types preventing them from running locally.

Customers that are able to test and run locally can find errors early, reduce development time, etc. If customers are not able to test, errors will begin manifesting in breaking bugs only found when hurting their customers in production.
Copy link
Contributor

Choose a reason for hiding this comment

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

"If customers are not able to test, errors will begin manifesting in breaking bugs only found when hurting their customers in production. "
Partly. It is not so much the manifesting in production but the development cycle is very long (build/write code, deploy, deploy failed, investigate, build/write code, deploy, test, test failed, build/write code ....). This what we are really targeting.


## Who are the Customers?

* People who work with tools such as aws cdk, terraform,etc. to generate their CloudFormation code
Copy link
Contributor

Choose a reason for hiding this comment

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

"terraform,etc. to generate their CloudFormation code" -> Terraform, and others to generate their CloudFormation templates.

## Who are the Customers?

* People who work with tools such as aws cdk, terraform,etc. to generate their CloudFormation code
* People that have long hand written CloudFormation templates
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean here? Why is long hand written CloudFormation templates a customer for support raw ApiGateway Resources?

## Success criteria for the change

* Customers would be able to author SAM templates with Api Gateway Resources and test locally through start-api
* Features that are supported currently through SAM also are supported with CloudFormation resources
Copy link
Contributor

Choose a reason for hiding this comment

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

More specific. Feature parity with what is currently supported with start-api on the AWS::Serverless::Api Resource.


## What will be changed?

CloudFormation ApiGateway types will be supported in the resource template definition such as AWS::ApiGatway::RestApi in the SAM templates. The CloudFormation resource will be parsed and then apis/stages specified will be processed. Then the cli will be able to spin the api with the resource up. The RestApis that are supported will now parse both the swagger and the pure vanilla implementation of the ApiGateway Resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • CloudFormation ApiGateway types -> CloudFormation API Gateway Resources
  • Then the cli will be able to spin the api with the resource up. -> Not sure I understand this.

I think this can be simplified to:
When customers run sam local start-api with a template that uses raw CloudFormation AWS::ApiGateway::RestApi and AWS::ApiGateway::Stage resources, they will be able to interact and test their lambda functions as if they were using AWS::Serverless::Api.
Or something to that flavor. Something short of what will change.


## Out-of-Scope

Anything that sam-cli doesn't currently support in SAM, the design doesn't plan to support it. This includes parts such as authorization and cors. Proper validation of the CloudFormation template so that it does smart validation and not just yaml parsing.
Copy link
Contributor

Choose a reason for hiding this comment

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

A list like you did for "Success Criteria" would be useful here instead of a paragraph


## User Experience Walkthrough

There are two main types of users who are going to benefit from this code.
Copy link
Contributor

Choose a reason for hiding this comment

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

from this change


There are two main types of users who are going to benefit from this code.

* Customers can use tools such as aws cdk to generate a template if they meet certain conditions. The customer can create their aws cdk project with `cdk init app` and then generate their cloud formation code using `cdk synth.`They can input their CloudFormation code to test it locally using the sam-cli command.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "if they meet certain conditions"
AWS CDK not aws cdk
SAM CLI not sam-cli


There are a few approaches to supporting the new CloudFormation ApiGateway types such as ApiGateway::RestApi.

### *Approach #1*: Parsing both CloudFormation and SAM Template
Copy link
Contributor

Choose a reason for hiding this comment

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

It's more about Parsing both Resources not the template. Since SAM is a superset of CloudFormation they are the same template, even though we tend to talk about they as different things (mainly to indicate what resources are within the template).

@jfuss jfuss self-assigned this Jun 24, 2019

## The Problem

Customers use SAM CLI to run/test their applications by defining their resources in a SAM template. The raw CloudFormation ApiGateway resources are not currently supported to run and test locally in SAM CLI. The resource types include AWS::ApiGateway::* while ```sam local start-api``` only supports the AWS::Serverless::Api type. This prevents customers who have built/deployed services using the raw ApiGateway Resources or who have used tools to generate CloudFormation, like AWS CDK, from testing locally through SAM CLI. Specifically, Customers that generate their CloudFormation code using tools and have hand written CloudFormation templates have AWS::ApiGateway::* types preventing them from running locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:
CloudFormation code -> CloudFormation template.


Customers use SAM CLI to run/test their applications by defining their resources in a SAM template. The raw CloudFormation ApiGateway resources are not currently supported to run and test locally in SAM CLI. The resource types include AWS::ApiGateway::* while ```sam local start-api``` only supports the AWS::Serverless::Api type. This prevents customers who have built/deployed services using the raw ApiGateway Resources or who have used tools to generate CloudFormation, like AWS CDK, from testing locally through SAM CLI. Specifically, Customers that generate their CloudFormation code using tools and have hand written CloudFormation templates have AWS::ApiGateway::* types preventing them from running locally.

Customers that are able to test and run locally can find errors early, reduce development time, etc. If customers are not able to test, the development cycles will be very long with write code, deploy, deploy failed, investigate, fix and repeat.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Development cycles will be very long with write code", doesn't read right to me.

## Out-of-Scope

* Anything that SAM CLI doesn't currently support in SAM
* ApiGateway Authorization
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these seem like sub bullets on "Anything that SAM CLI doesn't currently support".

Thinking about this more, we should be as explicit as we can be here. Someone in the future is going to read this and what 'currently is supported' will be different/hard to backtrack on what was actually the current scope.


There are two main types of users who are going to benefit from this change.

* Customers can use tools such as aws cdk to generate a template. The customer can create their AWS CDK project with `cdk init app` and then generate their CloudFormation code using `cdk synth.`They can input their CloudFormation code to test it locally using the SAM CLI command.
Copy link
Contributor

Choose a reason for hiding this comment

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

AWS CDK

This simplifies a lot of the issues with approach #1. This will make it much easier to add and extend the system such as adding ApiGatewayV2, web sockets. The feature would only need to be added once instead of duplicated effort.

Cons:
This will require more work in order to restructure the application such that the template is processed once after it processed to CloudFormation. This could also produce bugs with issues if the SAM to CloudFormation transformation isn't correct in local testing. However, this may not matter as much since there is a direct translation of SAM resources to CloudFormation Resources and only a single point where the bug needs to be fixed. Some other issues are the possible imperfections of the SAM to CloudFormation conversion. In the past, some version of the transformation caused incompatible changes with the SAM CLI. If the conversion fails, it could cause users that are currently running their code locally to break.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another one you should mention is that the SAM Translator currently requires credentials.

This goes through all the Pros/Cons in all the sections:
It's much easier to read and digest if you bullet each of the pros/cons.


### **AWS::ApiGateway::Stages**

The stage allows for defining environments for different parts of the pipeline in CloudFormation. Since a single stage is attached to a deployment, the behavior of multiple deployment/stages locally is currently undefined, which is similar to the current support or syntax in the SAM template.
Copy link
Contributor

Choose a reason for hiding this comment

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

"the behavior of multiple deployment/stages locally is currently undefined, which is similar to the current support or syntax in the SAM template."

Kind of. Since AWS::Serverless::Api only allows one stage, we never have to pick if other stages are in the template.

Since the stages can show up in any order as the code is iterating through the resources, There are two approaches to consider too quickly stitch the api and stages together.

*Approach #1:*
Arbitrarily pick one of the stages and use that on all the Api
Copy link
Contributor

Choose a reason for hiding this comment

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

Picking arbitrarily is fine but we should make sure it is reproducible. Meaning if the customers gives the same template in two different command runs, the same stage should be selected, otherwise this could become frustrating to customers.

I think Arbitrarily here means we pick the first one in the template we find?


One approach is to support multiple stages with multiple deployments and the code will filter out the stages that are not deployed or defined. One problem with this approach is that the RestApiId, a characteristic of the RestApi, for a deployment is only localhost and a single endpoint and the current schema doesn't allow deploying on multiple ports and multiple local domains. This will bring in unneeded complexity such as requiring the customer defining N valid ports and N valid domains.

Another approach, which I recommend, is to ignore the flag altogether as it always associated with a stage and to randomly pick a stage.
Copy link
Contributor

Choose a reason for hiding this comment

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

"ignore the flag" Deployment is not a flag, it is a resource


## **Updating the code**

First, the Apis in AWS::ApiGateway::RestApi will be parsed in the SAM Template in a similar way to the current AWS::Serverless:Api code. Stages and other Meta information in the Api will be parsed to support the CloudFormation template. The code will then be refactored to first translate the SAM templates inputted into the CloudFormation template using the SamTranslator. Once the Api code has been refactored, the function code can be refactored so that Serverless code is refactored out.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not now but when you get to doing the Transformation to CFN, we can expand this doc or create a new one. I think there is enough interesting tradeoffs and decisions in that to warrant it.


# What is your Testing Plan (QA)?

## Validation
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation isn't in scope right? Is this section needed then?

@viksrivat viksrivat changed the title feature/CloudFormation ApiGateway design (feat) CloudFormation ApiGateway support design Jun 27, 2019
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Just one comment to update the pseudo classes to match a littler closer on what you ended up implementing.

sam_translator = Translator(managed_policy_map=self.__managed_policy_map(),sam_parser=sam_parser, plugins=self.extra_plugins)
return sam_translator.translate(sam_template=template_copy, parameter_values=parameter_values)

class CloudFormationFunctionProvider(SamBaseProvider):
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't parse the Method and resources right? That happens within the CfnProvider.

Can you update this section to match the implementation?

@jfuss jfuss added the stage/accepted Accepted and will be fixed label Aug 7, 2019
@jfuss jfuss merged commit 0c6c50e into aws:develop Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/accepted Accepted and will be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants