feat: add support for VPCEndpointIds in EndpointConfiguration by scbrown · Pull Request #1524 · aws/serverless-application-model

scbrown · 2020-03-20T17:30:45Z

*Issue #1327
Description of changes:
Adds support for configuring a PRIVATE RestApi Endpoint
Description of how you validated changes:
added unit tests with expected input and output
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 · 2020-03-20T19:21:18Z

Codecov Report

Merging #1524 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1524   +/-   ##
========================================
  Coverage    94.10%   94.10%           
========================================
  Files           78       78           
  Lines         4951     4956    +5     
  Branches       999     1001    +2     
========================================
+ Hits          4659     4664    +5     
  Misses         135      135           
  Partials       157      157

Impacted Files	Coverage Δ
samtranslator/model/sam_resources.py	`94.32% <ø> (ø)`
samtranslator/model/api/api_generator.py	`95.12% <100.00%> (+0.05%)`	⬆️

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 1b2d6f2...8702af7. Read the comment docs.

scbrown · 2020-03-20T21:12:08Z

sorry for all the commits :)

In the expected output JSON I added, I'm unsure of how the Parameters section of AWS::ApiGateway::RestApi is supposed to look. Could someone point me in the right direction?

scbrown · 2020-03-23T18:59:31Z

AWS::ApiGateway::RestApi Parameters only allows simple string:string mappings, so I've kept it so that EndpointConfiguration only allows specifying 1 Type. I've tested this in my environment and it works, but I'm still not sure of the significance of setting these custom headers.

Type

qmg-drettie · 2020-04-09T11:18:17Z

would be great to get this merged! we are going to start using private gateways soon and require this

mjdouble · 2020-05-19T17:59:15Z

Any ETA for this being released?

sriram-mv · 2020-06-04T16:53:16Z

This looks good overall, taking a deeper look to make sure there are no breaking changes from the current implementation.

markdwags · 2020-06-17T22:26:09Z

Any update on this PR @sriram-mv? I'm currently running into the issue this PR will address, which is, in AWS::Serverless::Api not only being able to assign the endpoint as PRIVATE, but to be able to assign a VPC Endpoint to it as well to avoid this manual step.

Thanks!

zkhan96 · 2020-06-23T19:32:43Z

Would a command such as:

aws apigateway update-rest-api --rest-api-id <api-id> \
--patch-operations "op='add',path='/endpointConfiguration/vpcEndpointIds',value='vpce-<vpce-id>'" \
--region <region>

After a deployment be a viable workaround until this is merged?

Edit: Yes it does, so after a sam deploy just run this command and your endpoint will follow the following pattern:

https://{rest-api-id}-{vpce-id}.execute-api.{region}.amazonaws.com/{stage}

sriram-mv · 2020-06-24T16:31:16Z

Is is possible to move VPCEndpointIds to be its own property, because the way this is structured, changes the EndpointConfiguration to be more than a string, and this could be a breaking change.

scbrown · 2020-06-26T14:30:29Z

That may be possible if someone wants to start over, but this implementation was @ShreyaGangishetty recommendation from #1327

jfuss · 2020-07-10T21:13:45Z

@sriram-mv I am just trying to understand your push back here? Personally, I think how this PR is laid out now is ok. My main thought process behind this is: EndpointConfiguration maps to https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-restapi-endpointconfiguration.html

Long ago, we made the decision to make EndpointConfiguration should be a string and for a couple years the only property of EndpointConfiguration was the "Types". In SAM, we simplified that definition to be just the value of EndpointConfiguration. Hindsight is 20/20, but now that there is another property I think it makes sense to keep this mapping to it. My main reason for that is, if tomorrow API Gateway adds another feature under EndpointConfiguration, where do we put it? I don't think we want to keep endlessly deduping these properties. Honestly, there is a lot of SAM that can just be pass through and can get out of the way, instead of us trying to always keep up with CloudFormation (which we know is hard).

So if we have a strong reason to not put this under EnpointConfiguration in SAM, lets come up with a way to do that and not go down this endless "if another Property is added, we need to add it to AWS::Serverless::Api.Properties.NewPropName.

@scbrown Thanks you for getting us started here! I will work with @sriram-mv to come to a resolution and update this PR accordingly.

sriram-mv · 2020-07-13T17:57:43Z

@jfuss My primary concern was with backward compatibility, but if endpoint configuration being private under SAM was not supported anyway and the remaining endpoint configurations are backward compatible, I'm ok moving forward with this. I also 100% agree to be conformant with the CFN spec and not continuously add properties as needed.

sriram-mv · 2020-07-15T19:57:48Z

According to https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-restapi-endpointconfiguration.html, Types is a list, but I cant find a real world use case where it actually is a list, whereas compared to SAM its just a string. With that in mind, I'm approving this PR.

saurabhshri-evive · 2020-07-17T06:50:15Z

This is great news! 🎉 Thanks everyone for working on this, subscribed to 'watch' releases.

ariftahir · 2020-07-17T15:33:35Z

Excellent news, been waiting for this 🥳

saurabhshri · 2020-08-28T11:08:07Z

+        elif endpoint not in ["EDGE", "REGIONAL", "PRIVATE"]:
            raise InvalidResourceException(
                self.logical_id,
-                "EndpointConfiguration for Custom Domains must be" " one of {}.".format(["EDGE", "REGIONAL"]),
+                "EndpointConfiguration for Custom Domains must be"
+                " one of {}.".format(["EDGE", "REGIONAL", "PRIVATE"]),


@sriram-mv / @scbrown Custom Domains are not supported on private APIs, right?

https://docs.aws.amazon.com/apigateway/latest/developerguide/how-to-custom-domains.html

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-domainname-endpointconfiguration.html

saurabhshri · 2020-08-28T11:08:14Z

+        if isinstance(value, dict) and value.get("Type"):
+            rest_api.Parameters = {"endpointConfigurationTypes": value.get("Type")}
+            rest_api.EndpointConfiguration = {"Types": [value.get("Type")]}
+            if "VPCEndpointIds" in value.keys():


I didn't see this earlier, but I wished it was VpcEndpointIds instead of VPCEndpointIds, just for consistency. :) I did not realise and kept trying with VpcEndpointIds and could not get it to work (there's no error, so it deployed successfully, without VPCe).

I agree with @saurabhshri. Would be better if the name can be made consistence as per cloudformation EndpointConfiguration properties.

This one confuses us, especially, because the documentation says the wrong thing.
https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-resource-api.html#sam-api-endpointconfiguration

Steve Brown added 2 commits March 20, 2020 13:16

feat: add support for VPCEndpointIds in EndpointConfiguration

f5e64eb

fix: update formatting with black

23180cf

scbrown changed the title ~~feat: add support for VPCEndpointIds in EndpointConfiguration #1327~~ feat: add support for VPCEndpointIds in EndpointConfiguration Mar 20, 2020

ShreyaGangishetty assigned praneetap Mar 20, 2020

Steve Brown added 2 commits March 20, 2020 14:38

docs: update 2016-10-31.md

7c5b25b

docs: added api endpointconfiguration example

8702af7

Steve Brown added 2 commits March 20, 2020 17:07

docs: make example more generic

7c5c9a4

fix: remove nested EndpointConfiguration types from output

2f94396

fix: only allow one EndpointConfiguration Type

5a1cdda

doc: update example to reflect only allowing one EndpointConfiguration

5f22876

Type

mjdouble mentioned this pull request May 28, 2020

Secrets manager resolve for VPC endpoint ids in Rest API #1600

Closed

markdwags mentioned this pull request Jun 18, 2020

Add VpcEndpointIds to EndpointConfiguration Parameter #1327

Closed

sriram-mv self-requested a review July 15, 2020 19:58

sriram-mv approved these changes Jul 15, 2020

View reviewed changes

sriram-mv merged commit cb8e179 into aws:develop Jul 16, 2020

jamesiri unassigned praneetap Aug 24, 2020

saurabhshri reviewed Aug 28, 2020

View reviewed changes

Uh oh!

Conversation

scbrown commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

scbrown commented Mar 20, 2020

Uh oh!

scbrown commented Mar 23, 2020

Uh oh!

qmg-drettie commented Apr 9, 2020

Uh oh!

mjdouble commented May 19, 2020

Uh oh!

sriram-mv commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdwags commented Jun 17, 2020

Uh oh!

zkhan96 commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sriram-mv commented Jun 24, 2020

Uh oh!

scbrown commented Jun 26, 2020

Uh oh!

jfuss commented Jul 10, 2020

Uh oh!

sriram-mv commented Jul 13, 2020

Uh oh!

sriram-mv commented Jul 15, 2020

Uh oh!

saurabhshri-evive commented Jul 17, 2020

Uh oh!

ariftahir commented Jul 17, 2020

Uh oh!

saurabhshri Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

saurabhshri Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

esukirno Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

scbrown commented Mar 20, 2020 •

edited

Loading

codecov-io commented Mar 20, 2020 •

edited

Loading

sriram-mv commented Jun 4, 2020 •

edited

Loading

zkhan96 commented Jun 23, 2020 •

edited

Loading