-
Notifications
You must be signed in to change notification settings - Fork 1.2k
(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
(feat) CloudFormation ApiGateway support design #1234
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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).
|
||
## 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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this 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): |
There was a problem hiding this comment.
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?
Issue #, if available:
Description of changes:
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.