Fix for broken healthcheck in apollo-server-lambda

[iannickb]

Use endsWith except of === to check if the healthcheck endpoint is called. This should fix #4891.

[apollo-cla]

@iannickb: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[glasser]

Is this something that used to work and has changed in Lambda? Or is it an issue specific to custom domains and that's why it wasn't caught before? Are there docs you can point me at that make it clear what the expected value of event.path is? What does the value look like in your custom domain use case? Do we have to apply a similar change to the other use of event.path in the file (which brings in graphql-playground)?

[iannickb]

Thank you for the quick response @glasser !

Is this something that used to work and has changed in Lambda?

My guess is that it never worked. It was introduced when onHealthCheck was added 84e1aa5#diff-859ee6c9b8a50783a3f6358ec930fd105d32832ffd678c836f58e02ee3ed60d9

Or is it an issue specific to custom domains and that's why it wasn't caught before?

It is specific to Custom Domains and how the event.path is populated when behind a Cloudfront distribution

Are there docs you can point me at that make it clear what the expected value of event.path is?

For some reason, I could not find any documentation related to that on the aWS website. I did find a good explanation on the serverless-express repo from someone who encountered the same thing.
CodeGenieApp/serverless-express#216

What does the value look like in your custom domain use case?

Everything works fine if I am calling the APIG generated endpoint. The value is different when running behind a custom domain and a non-default base path mapping.

Do we have to apply a similar change to the other use of event.path in the file (which brings in graphql-playground)?

It would be better to be consistent and change it across the board.

[iannickb]

I reviewed the code for playground and I am wondering why there is a check for event.path. That value should always be there for apollo-server-lambda.
https://github.com/iannickb/apollo-server/blob/f1a8d518339e98d429d414288b3c2e3eed15b488/packages/apollo-server-lambda/src/ApolloServer.ts#L208-L211

Also, since the condition is on method and the playground setting, maybe we could leave as is?

[kamaz]

@iannickb I believe that is a real problem and doesn't apply only on api gateway. I use it with ALB where Apollo Server is bound by custom context. In my scenario, I get /bff/.well-known/apollo/server-health.

[glasser]

Just want to note that I am still tracking this for the next day of "merge a bunch of PRs that cover parts of the codebase where I need to do some research before I can evaluate them"; this hasn't happened yet largely due to some vacation.

[glasser]

OK. I find this whole thing to be pretty confusing. Sounds like the way event.path works in Lambda is very inconsistent depending on which particular service your Lambda lives behind. I'm also rather confused that #4687 is asking for the opposite resolution (they're saying event.path is shorter than it should be rather than longer).

That said, this particular change does seem unlikely to break anything: nobody should be depending on any URL ending with /.well-known/apollo/server-health doing anything other than triggering a health check. So I'll merge it, though I'm still pretty confused about #4687.

[glasser]

I've published a release with this to version 2.21.1-alpha.0. My plan is to release 2.21.1 on Monday. If you want to test that this fix works before then, try running the prerelease yourself, and provide feedback on #4990