Add support for HTTP and WebSocket API Types #319
Conversation
tehnrd
changed the title
tehnrd
force-pushed
the
http-ws-support
branch
2 times, most recently
from
f8ea0f2
to
1d0fedb
Compare
|
Thanks for implementing this! I've tried it out today and can verify it works for the HTTP API type. Something I've noticed is that subsequent runs of Ideally it would recognize this situation and proceed as normal. I worked around this in my local testing by doing: // Make API call
try {
yield this.apigatewayV2.createApiMapping(params).promise();
this.serverless.cli.log("Created basepath mapping.");
} catch (err) {
if (err.code === "ConflictException") {
this.serverless.cli.log(
"Basepath already exists",
"Serverless Domain Manager"
);
return;
}
this.logIfDebug(err, domain.givenDomainName);
throw new Error(`Error: Unable to create basepath mapping.\n`);
}I have no idea if that is reasonable for all cases or not, but it worked for my situation. |
tehnrd
force-pushed
the
http-ws-support
branch
2 times, most recently
from
5df655c
to
eb274c0
Compare
|
@lee-reinhardt That issue should be fixed now, but you may need to manually delete the existing mappings. The problem was API mappings were being created with V2 of the AWS API but then attempting to update with V1 on subsequent deploys. I also added unit tests. Thanks for jumping on this and doing a little testing, really appreciate it! |
tehnrd
force-pushed
the
http-ws-support
branch
2 times, most recently
from
2605d34
to
da5fab4
Compare
|
When can we expect this to be merged? |
|
Curious; does anyone know if we are able to install @tehnrd 's PR directly via NPM. I installed it with but I end up with the error: Bummer! Also, super excited to see this PR pushed into a new release |
|
@cbschuld Unfortunately, that is not going to work because Amplify does not keep the compiled distribution files in the repository. They seem to have some sort of separate NPM publishing process not included in this repo. Here is something you can do for testing, but it isn't really scalable.
The full path will be different for you, but you can easily get it by opening the project in VSCode or similar, right click on the /dist/index.js file and "Copy Path" |
|
@tehnrd - awesome; thank you! This will get my by while I wait for this PR to be eaten. |
|
@tehnrd - I am bumping into a few things - unsure if I am missing something and being stupid or if it is underscoring something else converted customDomain to: It will not deploy with error: While in debug mode I am seeing this: any help is both appreciated & awesome! Thx! |
|
@cbschuld Looks like you pulled down and ran build on the master branch which is currently the released version and with no support for http/ws. If you pull down the whole repo you'll need to switch to the Can also clone/pull this branch directly, If you find any problems/issues/bugs in testing, let me know! |
|
@tehnrd - [ shakes head in shame ] - thank you - that was it; apologies; was in the wrong branch - I'll push ahead |
|
@tehnrd - apologies, I keep bumping into issues ; I was hoping I could be a good test case for the updates and I feel like the whiny dev w/o enough context to help. Here is the error I am seeing: I checked the stack and the resource is definitely not there. Here is additional context on the error: again, I am in hopes I am just not thinking crisply around this challenge |
|
No stress on issues, happy to find and fix! Was this for an existing domain that was already created or a new one? Did you run |
|
@tehnrd - yeah; domain is definitely created. Also, I removed everything and started from a clean slate and ended up w/ the same error. Here is the call from create_domain for sanity: |
Existing domain. |
|
An existing domain might be the issue. How long ago and how was it created? Older domains will not work as they are created with AWS APIGateway API (v1) that is not compatible with HTTP APIs as the Domains needed to be created with AWS ApiGatewayV2 API. AWS doesn't make this obvious at all as there is no way to see in the console what API version was used to create a domain. Even their APIs don't tell you what version a Domain Name was created with! If it was created and used with an older REST API, it won't be usable with the new HTTP APIs. Let me know if this was the issue and maybe I can think of a creative way to solve this. |
|
@tehnrd - copy that. It was definitely created with the previous version. The domain's route53 record is pretty packed though so re-creating will bit a bit of a pain. I suspect I won't be the only one looking to convert REST subdomains over to HTTP either. Bummer. I am contemplating blowing away the domain just for pure testing reasons. |
|
Ya, it's definitely an issue, but possibly more of an AWS issue. Let me think about the best way to handle it. My first thought is there probably isn't a good way as this needs to be done in a controlled manner due to all the Route 53 and DNS propagation stuff that needs to happen. It would be pretty bad if a deploy swapped out DNS stuff unexpectedly. EDIT: Hmmmm, it looks like AWS recently updated the API Gateway console for Custom Domains. It still doesn't show what version a domain was created with, but I was able to create a domain mapping to an HTTP API with an older Domain created with APIGateway (v1). I swore it threw an error before but is now working. I'll investigate tonight. |
|
@tehnrd - resetting everything back to zero I am noticing that the v2 version * may * not be dealing with edge vs regional certificates in us-east-1 vs let's say us-west-2 for certificates. Obviously the provider is I have yet to get it to create a domain correctly. |
|
Edge vs Regional certificate? Are there different types of certs? Or do you mean certs stored in two different regions? FWIW, Domains that use an HTTP API can only be Regional, not Edge. (not sure if that is related) |
|
@cbschuld Should be fixed now, but let me know. Route 53 is one of my weaker areas of AWS knowledge and the issue was that I/code was not handling the new response object correctly for domains created with ApiGatewatV2 and it was setting the Thanks for testing, catching this, and jumping on Zoom to dig into details! |
|
I started to look into how this could support migration from an older REST API to an HTTP API as an older domain even created with APIGateway (v1) should still be able to create base path mappings to HTTP APIS if it is a Regional Domain. Investigating this I discovered an issue (easily fixable) with the current implementation. It assumes if setting up a REST API, use APIGateway (V1) and if HTTP or WebSocket API use ApiGatewayV2. Unfortunately, It's not quite this simple as Domains can be of the type Edge Optimized or Regional, but the REST APIs can also be configured Regional/Edge/Private. This creates a little matrix of possibilities of 1) is the combination even valid/allowed, and 2) if the combination is valid what APIGateway API version should be used for configuration, v1 or v2. I think this table handles all the scenarios but I need to perform actual testing. Not sure about Private REST APIs. If it is correct, I will add upfront validation to throw an error the
|
Not going to make any changes (unless someone finds a bug), I decided to hold off. I or @ConradKurth can do different PR to address #327. While I think it would be easy to add, it is technically a different feature, so a separate clean commit would be good in case a revert is needed. And would also make viewing the changes to support multiple domains easier to see rather than burying them in this. Edit: and I realize this PR is a bit of a beast, so let me know if I can clarify or need to change anything and I'll jump on it. |
|
@tehnrd I agree, lets get this in and then we address the other PR |
| @@ -630,26 +768,23 @@ class ServerlessCustomDomain { | |||
| * Logs message if SLS_DEBUG is set | |||
| * @param message message to be printed | |||
| */ | |||
| public logIfDebug(message: any): void { | |||
| public logIfDebug(message: any, domain?: string): void { | |||
There was a problem hiding this comment.
If this is only used for logging errors then maybe this could be called logErrorIfDebug
There was a problem hiding this comment.
It can, totally up to the Amplify team. I can rename it but I will wait for confirmation from you.
This was an already existing method I just added the domain argument to so the user would know which domain was failing on a multi API deployment.
There was a problem hiding this comment.
I was referring to the fact that the message now always includes the word Error so we could make the function name show that. It's a nitpick so leaving it as is for now is fine too
There was a problem hiding this comment.
Ah, gotcha. I checked master it is only using for errors, but I'm inclined to keep it as is for now as it could theoretically be used for other messages.
And there isn't a dedicated serverless.cli method specifically for errors:
...so probably best if this is handled in its own separate commit.
index.ts
Outdated
|
|
||
| // Make API call to create domain | ||
| try { | ||
| // If creating REST api use v1 of api gateway, else use v2 for HTTP and Websocket |
There was a problem hiding this comment.
Is this comment still accurate? This seems to decide which API to use based on endpoint type and not API type
There was a problem hiding this comment.
Also, why can't EDGE domains/rest APIs be created using the v2 API?
There was a problem hiding this comment.
There was a problem hiding this comment.
@aoskotsky-amplify @claygorman That inline comment is incorrect and should say if creating an EDGE API (only REST can be EDGE, so it's a little confusing, but if REST is regional it can use the v2 API), but...
...the AWS docs are wrong, and not yet updated. I opened a case with AWS Support about this issue exactly.
v2 API cannot create EDGE domain names.
There was a problem hiding this comment.
I think we are good to resolve this one? @aoskotsky-amplify
|
Thanks @aoskotsky-amplify, I'll give those a look this evening. Do you have a preference for the process of implementing/fixing feedback?
(embarrassed about all those typos and just installed VSCode spellchecker!) |
|
@tehnrd I think option 2 makes sense. Just send a new commit(s) with the changes. Thanks again! |
|
@aoskotsky-amplify I fixed and resolved the simple changes. I left open the others that are a little more complex or need more discussion and will let Amplify mark as Resolved when appropriate. |
There was a problem hiding this comment.
Found a couple of more typos and one question. Let me know what you think. I'll try to get this merged as soon as possible.
I ran the integration tests locally (I had to make a bunch of changes to get them working again that are unrelated to this PR which I'll commit in a future PR) Looks like they all pass against this PR which is great.
index.ts
Outdated
| API Mappings may need to be manually removed.`, "Serverless Domain Manager"); | ||
| } else { | ||
| this.logIfDebug(err, domain.givenDomainName); | ||
| throw new Error(`Error: Unable to remove base bath mappings for domain ${domain.givenDomainName}`); |
There was a problem hiding this comment.
This is actually changing some of the old behavior. This used to log an error without throwing an exception. I ran into this by accident by running delete_domain and remove out of order. For example:
sls create_domain
sls deploy
sls delete_domain
sls remove
The sls remove command above started throwing errors and I wasn't able to remove the API until running sls create_domain and sls_deploy again.
There was a problem hiding this comment.
Hmm...ya...I'm looking at that and can't recall a good reason why. I think I was just following the same approach used in other try/catch blocks and may have been a copy/paste error.
I removed the throw and added a log.
| @@ -630,26 +768,23 @@ class ServerlessCustomDomain { | |||
| * Logs message if SLS_DEBUG is set | |||
| * @param message message to be printed | |||
| */ | |||
| public logIfDebug(message: any): void { | |||
| public logIfDebug(message: any, domain?: string): void { | |||
There was a problem hiding this comment.
I was referring to the fact that the message now always includes the word Error so we could make the function name show that. It's a nitpick so leaving it as is for now is fine too
|
Kudos, SonarCloud Quality Gate passed!
|
|
@aoskotsky-amplify I think I addressed the issues found. Let me know when you want this squashed down to one commit before any merging (or other issues to fix) |
|
No prob, happy to help! You beat me to it! I was going to smash those commits into one for a little cleaner history, but not a big deal. You can do it on master if you want before anyone else starts working off of it. |
|
I think it's fine having the longer history. Thanks. Working on updating the package version and change log now. |
|
Hey @tehnrd hoping you can help me here. Getting this error when deploying - have tried to delete the domain, mappings, etc, and recreate using SLS. `Serverless: [AWS cloudformation 400 0.168s 0 retries] describeStackResource({ LogicalResourceId: 'HttpApi', StackName: 'api' }) Error -------------------------------------------------- Error: Error: Unable to setup base domain mappings for api.domain.com Any advice where to go next? |
|
@reganjohnson You using 4.x? https://www.npmjs.com/package/serverless-domain-manager What does your config look like? |
|
@tehnrd Yes to 4.0 |
|
Try dropping the If there is specific need for it, let me know as I'm curious. Related to #334 |
No need for the stage on the new httpApi - was using it for the REST setup I migrated from. This appears to have worked! Thank you much for the quick reply. |
|
Hi there, it appears this PR accomplished exactly what I need, however in practice I am unable to receive a Cloudfront distribution link through the process of running I am attempting to deploy an Api Gateway V2, as in an http api vs rest api. From my reading, it appears I can only do this with regional endpoint types, since edge-optimized are not supported. I created my certificate in the correct region, and validated successfully through DNS validation. I am NOT using Route53, but instead using Cloudflare. As such, I have set Here is the relevant part of my serverless.yml file. When I run So I believe this has worked successfully, however when I then run It is my understanding that I should be receiving a Cloudfront distribution url in the Target Domain section of this output, which I can use to configure my DNS in Cloudflare as an Alias/CNAME to point to it. Why is this not happening? It appears to me that @reganjohnson is not creating a route53 record either, so I don't think this should be my issue, what gives? |
|
same here, target domain for rest seems to work fine, but for http api target domain is not the same as endpoints listed after sls deploy. |
Fixes #312 #207 #223
Description of Issue Fixed
This PR adds support for both HTTP and Websocket API Types for the AWS API Gateway. For these types, domain names must use the ApiGatewayV2 AWS API.
This PR essentially bulkifies the entire process of defining and creating domains. Instead of config values being defined on variables in the index.ts file an array of DomainConfigs is created. Then for all operations it will process all of the domain configs. This allows the mixing of different API types and domains in one serverless.yml file.
It is backward compatible with the current customdomain configuration format. It is also structured in a way that will easily scale should AWS add new API types in the future.
This can be tested by adding the following branch to your dependencies.
Changes proposed in this pull request:
createDomains()have been bulkified to process all DomainConfigs in thedomainsarray.createCustomDomain()now take an argument of aDomainConfig.Unit tests have been updated and a few new ones have been added. I also performed some functional testing and everything appears to be working. I was unable to get the integration tests to run but will try to work on that while this PR is pending review.
I understand this is a massive PR and a huge refactor but I would love to see this get some attention as it was a decent effort. Even being backward compatible it might be worth a 4.x release given potential impact and the number of people that have become dependent on this very popular package.
P.S. Can do normal online PR review, but I actually live and work near DUMBO and could do in-person walkthrough of the changes if that makes sense.