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

Add API Gateway endpoint configuration #4531

Merged
merged 3 commits into from Dec 10, 2017
Merged

Add API Gateway endpoint configuration #4531

merged 3 commits into from Dec 10, 2017

Conversation

alexdebrie
Copy link
Contributor

What did you implement:

Closes #4440.

Adds ability to specify regional endpoints for API Gateway. :

# serverless.yml

provider:
  name: aws
  endpointType: regional
...

How did you implement it:

Adds the EndpointConfiguration attribute when we compile the RestApi resource. By default, it sets to EDGE, which is the default API GW setting.

How can we verify it:

I used this example:

service: endpoint-test

# The `provider` block defines where your service will be deployed
provider:
  name: aws
  runtime: nodejs6.10
  endpointType: regional

functions:
  helloWorld:
    handler: handler.helloWorld
    events:
      - http:
          path: hello-world
          method: get
          cors: true

Then run sls package and check the .serverless/cloudformation-template-update-stack.json file that the RestAPI resource has the EndpointConfiguration defined as desired.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

let endpointType = 'EDGE';

if (this.serverless.service.provider.endpointType) {
const validEndpointTypes = ['REGIONAL', 'EDGE'];
Copy link
Member

Choose a reason for hiding this comment

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

According to the AWS docs, an endpoint type can also be a domain name. Should we support this? I think it could be important as soon as you map a custom domain names to your API - but I did not test anything in that area yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HyperBrain I think it's slightly different than that. A domain name that you register with API Gateway can also have an endpoint configuration of Edge / Regional. Thus, if you set up api.mycompany.com in API Gateway, you could configure it to be Regional.

Currently, the Framework doesn't configure API GW domains in core, so I don't think we can do anything with that. It could be a feature for the serverless-domain-manager plugin to add.

I'm not 100% sure I'm correct on this so feel free to correct me :)

Copy link
Member

Choose a reason for hiding this comment

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

The reason why I came to the assumption that it could be defined here is, that the CF AWS::ApiGateway::RestApi resource defines its EndpointConfiguration property here in that way: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-restapi-endpointconfiguration.html

There is written "... or its custom domain name". Additionally it seems that the property (which is an array) can contain multiple values. But don't ask me how exactly this is intended to work or when exactly you should add multiple values to the array.

So this is for me a bit unclear too 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. That's confusing.

For what it's worth, the EndpointConfiguration documentation for the RestApi resource is nearly identical to the EndpointConfiguration for the DomainName resource. I wonder if that sentence -- A list of endpoint types of an API or its custom domain name refers to the fact that this configuration is similar for both a RestApi resource and a DomainName resource?

Agreed that the array makes no sense 😄

Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

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

I think we can go for the REGIONAL or EDGE solution for now.
The further procedure could be:

  • If someone comes back with a real-life scenario where a domain name is necessarily needed, we can add that functionality
  • If someone finds out or can explain, when exactly to use multiple different values (array) in one API we can add that too
    Imo the 2-value solution will get users onto the feature, so that the rest will happen automatically 😄
    What do you think?

@HyperBrain
Copy link
Member

HyperBrain commented Dec 7, 2017

@alexdebrie Just saw that there are still ESLint failures:

/home/travis/build/serverless/serverless/lib/plugins/aws/package/compile/events/apiGateway/lib/restApi.test.js
  19:13  error  Strings must use singlequote             quotes
  19:19  error  Missing trailing comma                   comma-dangle
  62:70  error  Strings must use singlequote             quotes
  71:1   error  Block must not be padded by blank lines  padded-blocks

and you could additionally add a unit test for the type setting to bring the coverage up again.

@horike37 horike37 added this to the 1.25 milestone Dec 8, 2017
Copy link
Member

@horike37 horike37 left a comment

Choose a reason for hiding this comment

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

I tested it locally, but sls deploy has failed with the following error. Do you work fine on your end?

Seems that CloudFormation setting would not go wrong 🤔 That's a little strange...

  Serverless Error ---------------------------------------
 
  An error occurred: ApiGatewayRestApi - Value of property EndpointConfiguration must be an object.
 
  Stack Trace --------------------------------------------
 
ServerlessError: An error occurred: ApiGatewayRestApi - Value of property EndpointConfiguration must be an object.
    at provider.request.then (/Users/horike/src/serverless/lib/plugins/aws/lib/monitorStack.js:114:33)
From previous event:
    at AwsDeploy.monitorStack (/Users/horike/src/serverless/lib/plugins/aws/lib/monitorStack.js:26:12)
    at provider.request.then (/Users/horike/src/serverless/lib/plugins/aws/lib/updateStack.js:88:30)
From previous event:

serverless.yml

service: testesttest
provider:
  name: aws
  runtime: nodejs6.10
  endpointType: regional

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: users/create
          method: get

@alexdebrie
Copy link
Contributor Author

@HyperBrain:

Imo the 2-value solution will get users onto the feature, so that the rest will happen automatically

That's a great point. Sort of a paraphrase Linus's Law -- "Given enough eyeballs, all bugs bad AWS documention is shallow. 😄

I fixed the linting errors -- my mistake for not grabbing those initially. I'm still not sure why code coverage it thought code coverage was dropping, but it fixed now.

@horike37

Great catch on that and big mistake on my part. I missed that EndpointConfiguration has a Types attribute where you list the endpoint types. I could have sworn I tested it with a real deploy. Either way, I have fixed it now. Please let me know if it works for you.

@horike37
Copy link
Member

Thank you for the update @alexdebrie 👍
just tested it again, and confirmed working fine as expected.

Seems everything is OK. merging...

$aws apigateway get-rest-api --rest-api-id j75f4zgmzb
{
    "name": "dev-testesttestss", 
    "endpointConfiguration": {
        "types": [
            "REGIONAL"
        ]
    }, 
    "id": "j75f4zgmzb", 
    "createdDate": 1512918617
}

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

3 participants