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

apollo-server-lambda: When invoking directly TypeError: Cannot read property 'endsWith' of undefined #5415

Closed
rnoack1 opened this issue Jul 2, 2021 · 20 comments

Comments

@rnoack1
Copy link

rnoack1 commented Jul 2, 2021

apollo-server-lambda v2.25.1

Ran into this error after updating apollo-server-lambda package. Some investigation led me to the following closed ticket: #5016

However, I think the change that caused this original problem, also caused another problem that is still not fixed.

The case of invoking lambda via API-Gateway is solved, but there is still a use case to invoke the lambda directly. In our case, clients access the Federated gateway via Api-Gateway, but the schema manager needs to invoke Subgraphs directly. When the Schema manager invokes subgraphs, event.path / event.rawPath will not be set.

It seems like the last working version for us was somewhere around 2.21.0 ? As after that the line:

if (event.path === '/.well-known/apollo/server-health') {

was changed to use .endsWith.

The result before was undefined === '/.well-known/apollo/server-health'. (false) when the lambda was invoked directly., and the schema manager could still get a response.

However, with the move to "endsWith", the code now only works when called from API Gateway (when event.path or event.rawPath is populated). So, we can fix this by manually adding some code in our subgraph lambda to set the rawPath to something like empty string '', which will support the "endsWith". It should be noted that setting event.path to empty string also will result in error due to false-y coalescing in the eventPath function of ApolloServer.

I don't think that apollo-server-lambda should require API Gateway, and should also support direct lambda invoke.

Is it possible to make a patch which does something like this, so that even if eventPath is undefined we wont crash?

const eventPath = eventPath(event);
if (eventPath && eventPath.endsWith('/.well-known/apollo/server-health')) {
@glasser
Copy link
Member

glasser commented Jul 2, 2021

We've completely rewritten apollo-server-lambda for Apollo Server 3. Instead of trying to parse Lambda events directly, we trust the @vendia/serverless-express package to do that for us and have made apollo-server-lambda into a layer on top of apollo-server-express.

AS3 is in release candidate mode now, with 3.0.0 expected within the week. Can you see if it works for you? Install the RC with npm install apollo-server-lambda@rc.

V3 Lambda tutorial: https://www.apollographql.com/docs/apollo-server/v3/deployment/lambda/
V3 migration section about Lambda: https://www.apollographql.com/docs/apollo-server/v3/migration/#apollo-server-lambda

Can you test this and see if it fixes your issue? This sort of thing where our vague knowledge of the various ways that Lambda functions can be invoked led us to writing code that doesn't work in all scenarios is exactly why we did the rewrite so hopefully this helps. Note that the createHandler arguments are different in V3 as described in the migration section.

@glasser glasser closed this as completed Jul 2, 2021
@jcarver989
Copy link

jcarver989 commented Jul 11, 2021

@glasser -- Hey for what it's worth, I have a similar use-case (gateway directly invokes an AWS Lambda graphQL sub-service) and hit the same issue.

Can you see if it works for you?

Upgrading to 3.x doesn't fix the issue as the serverless-express package you're relying on makes similar assumptions about how Lambdas are invoked. More specifically, directly invoking a Lambda from the gateway layer will trigger this error : https://github.com/vendia/serverless-express/blob/mainline/src/event-sources/utils.js#L79

It doesn't look too hard to make a change to support this use-case (or fake the incoming event's properties), but in my case I just ended up using the graphql package directly. And, if this is a rare use-case you're not too keen on having to support, it might be a reasonable alternative for folks who aren't using Apollo Federation.

@glasser
Copy link
Member

glasser commented Jul 12, 2021

I'm not sure what you mean by invoking directly. That means you're not going through one of the AWS layers that implement HTTP, right? So you're not expressing your request as an HTTP request (using the semi-standard format)... so you're just making up your own serialization for a GraphQL request that isn't JSON-over-HTTP? Which is fine, but that does mean it's going to be hard for any off-the-shelf library to implement it?

(One alternative is just to invoke your function with an event shape that's close enough to API Gateway or ALB or something?)

I do understand that it would be nice to be able to access the Apollo Server request pipeline for non-HTTP requests, though...

@jcarver989
Copy link

jcarver989 commented Jul 12, 2021

I'm not sure what you mean by invoking directly. That means you're not going through one of the AWS layers that implement HTTP, right?

By "invoke directly" what I'm referring to is using the AWS sdk invoke method, which issues an HTTP request to execute a Lambda function, given a JSON payload.

That means you're not going through one of the AWS layers that implement HTTP, right?

Technically the call to invoke does issue an HTTP request (with a JSON payload).

But, what API gateway does conceptually is: map an incoming HTTP request to a JSON payload + call invoke with that payload. A Lambda attached to the API gateway then operates on that JSON object (not an HTTP request), which contains properties extracted from the original HTTP, e.g: headers, body etc.

And, the serverless-express package knows how to work with that JSON object.

So you're not expressing your request as an HTTP request (using the semi-standard format)... so you're just making up your own serialization for a GraphQL request that isn't JSON-over-HTTP?

Ah actually, I am expressing the request as a JSON payload that mostly* matches what API gateway would have generated (i.e. a JSON object with parameters for headers, body etc).

The rub is the serverless-express package is reliant on metadata API gateway inserts that's not related to to the incoming graphQL request. Specifically requestContext.

Callers could of course just set requestContext appropriately (to more closely mimic what API gateway does), but it'd be nicer to not have to do that as it doesn't really have anything to do with the incoming graphQL request.

@glasser
Copy link
Member

glasser commented Jul 12, 2021

I guess I'm not sure why you can't set requestContext? If you're trying to simulate the way that HTTP events are encoded by AWS services, you can go all the way? We define GraphQL's encoding in terms of HTTP.

@rnoack1
Copy link
Author

rnoack1 commented Jul 12, 2021

Just to give my thoughts, Api gateway is far from the only valid way to invoke a lambda function: https://aws.amazon.com/blogs/architecture/understanding-the-different-ways-to-invoke-lambda-functions/

It seems like, for completeness, the requirement on API Gateway doesn't make a lot of sense.

I don't think that its about simulating the way HTTP events are encoded by AWS, actually the problem IMO is that it's not generic enough to cover all AWS services, rather imposing a requirement on using API Gateway by attempting to read fields that only API Gateway checks without any safety checks.

@glasser
Copy link
Member

glasser commented Jul 13, 2021

I guess my confusion is this.

The GraphQL spec describes how to execute GraphQL in a generic sense independent of transport / encoding.

You can transport your GraphQL however you want! But the only "standard" way to do so is "JSON over HTTP" which has non-rigorous docs and a working draft spec.

There's no standard way of encoding GraphQL as a Lambda event. So apollo-server-lambda implements the "JSON over HTTP" spec, using @vendia/serverless-express to interpret the various ways that Amazon has defined for one to encode HTTP requests as Lambda events.

If there was a "standard" way of encoding GraphQL as a Lambda event other than as "JSON over HTTP, encoded as one of AWS's ways of doing HTTP over Lambda" then it would make sense for apollo-server-lambda to implement that standard. But I don't think there's such a standard?

@rnoack1
Copy link
Author

rnoack1 commented Jul 13, 2021

I'm pretty sure I agree with you, but not sure we are saying the same thing. As far as I can tell, there is nothing in the spec about requirements to set the "path" or "version" property when making an HTTP request with GraphQL?

The root of the problem is because "path"/"version" are specific to API Gateway; API Gateway is adding those properties into the request body, and the apollo-server-lambda or dependencies of it are checking the values of those properties in a way that if they are not present results in a fatal error.

I think it's a perfectly fair statement and fine solution to say that the package should be completely agnostic of AWS service-specific added properties. Since the request seems to work fine when we provide (hard-code) dummy/blank values, such a solution seems possible. As far as I know, the core properties of the request (the ones in the GraphQL spec) should be consistent through all ways of invoking lambda.

Anything additional behavior when those properties are set (metrics? logging?) seems to be nice to have added features, but feels to me like the core implementation shouldn't rely on that, and those features should gracefully turn off then when invoked outside API Gateway.

@jcarver989
Copy link

jcarver989 commented Jul 13, 2021

@glasser -- Yea, the way I'd summarize the current state of things is:

  1. Apollo server expects JSON over HTTP (per the draft spec)

  2. AWS Lambda functions operate on JSON payloads (not HTTP requests).

  3. There are many different ways to trigger a Lambda function. And various AWS services (e.g. API Gateway, Application Load Balancers etc) can map incoming HTTP events to JSON + invoke a Lambda function for you. But, each service sends slightly different JSON payload to the Lambda function[1]. However there's commonalities -- e.g. headers, body, httpMethod and path properties are typically included.

  4. We're relying on the serverless-express package to map Lambda JSON payloads to express request objects. The issue is this package only supports a subset of ways you can invoke a Lambda function (e.g. direct invocations are not supported) because it's relying upon metadata in that JSON payload that's a) only set by certain AWS services and b) not actually necessary to construct the express request object.

Given the above it's easy to blame the serverless-express package. But, I think there's a pertinent design consideration here: If ApolloServer had a method that accepted a node or express http request object and let you easily trigger a single request/response cycle -- I think that'd solve the issue more cleanly as callers could just map whatever JS object they have to the request object. Maybe this already exists and I just haven't seen it?

The executeOperation method on ApolloServer looks close-ish to that. But with the caveat that it reads an already parsed (no longer an http request) GraphQLRequest object.

[1] For example Lambda functions triggered by ALBs and API Gateway receive different JSON payloads.

@rnoack1
Copy link
Author

rnoack1 commented Jul 13, 2021

Agree with above too but would like to point out that path is not always included. For example when one lambda invokes another, or potentially invoking through AWS console (need to check to know for sure).

@glasser
Copy link
Member

glasser commented Jul 13, 2021

I just don't understand how one can have an HTTP request without a path. Every HTTP 1.1 request starts with a method and a path. It's a pretty core part of what it means to be an HTTP request. So if you are trying to construct a Lambda event that simulates an HTTP request (because there's no standard way to encode GraphQL operations other than HTTP requests) then it seems reasonable that you should include the path as well.

@glasser
Copy link
Member

glasser commented Jul 14, 2021

And yes:

If ApolloServer had a method that accepted a node or express http request object and let you easily trigger a single request/response cycle

This was part of our original goal for Apollo Server 3 (#3184) but was cut for scope.

@rnoack1
Copy link
Author

rnoack1 commented Jul 14, 2021

I don't think it is correct to think about event as an HTTP request. Rather it is an object representing information about the event which invoked the lambda function.

In the case of API Gateway, information about the request to API Gateway is passed along inside the event object. In this case path is added to the information about the request, and the path is referring to the path which used to invoke the API Gateway.

In the case of a lambda invoking another lambda, where there was no API gateway, path is not really relevant in the context of the event invoking the lambda.

In both scenarios, under the hood, I guess the Lambda is also invoked via HTTP, API-Gateway makes an additional HTTP call to Lambda for example. But, if it worked the way you were describing and path were populated with the path that API Gateway called the lambda function you would see path something like /2015-03-31/functions/FunctionName/invocations?Qualifier=Qualifier rather than /graphql or whatever you put on your API Gateway. The path of the HTTP request used to invoke the lambda is not included in any scenario of invocation, even API-Gateway.

In other words, when calling a lambda with API gateway there are two HTTP requests involved: The one between the client and API-Gateway, and another between API-Gateway and lambda. The path property is populated in event with the path sent by the client, because this is considered useful information. But it has nothing to do with the HTTP request used to invoke the Lambda itself.

You can see more details about the HTTP Request used under the hood between services to invoke lambda: https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html

@glasser
Copy link
Member

glasser commented Jul 14, 2021

I think we are talking past each other.

The only spec that exists for encoding a GraphQL operation in some sort of format is the "JSON over HTTP" spec. So Apollo Server only knows how to execute GraphQL operations that are encoded as HTTP, and it's fair for Apollo Server to assume that every incoming HTTP request contains a path.

If there were a standard format for encoding GraphQL operations directly as Lambda executions, it might be reasonable for Apollo Server to implement that. But I don't think there is.

@glasser
Copy link
Member

glasser commented Jul 14, 2021

(I understand that it might not seem valuable for Apollo Server to care about the path. It primarily cares about the path in order to implement health checks, a feature that I personally find of questionable importance (in that it probably could more easily be implemented outside of Apollo Server, or you can just run a {__typename} query as a health check). But it is a feature of Apollo Server and we did not remove it in AS3.)

@rnoack1
Copy link
Author

rnoack1 commented Jul 14, 2021

I guess in opening the ticket in the first place, I was hoping that apollo-server-lambda would want to support all lambda invoke types, but it seems like what you are saying is that the only officially supported mechanism for using apollo-server-lambda is with API Gateway, which is valid. It's just a little unfortunate because other mechanisms were working fine on v2.21.0 and below.

It's not a huge deal for us to mimic API Gateway in the way we write our calls, but it does feel like a hack. It would introduce significant latency to change our architecture to use API Gateway instead of direct invoke in certain scenarios, and also not acceptable for us to lock to a lower version for an extended period of time due to possibility for security vulnerabilities. I would be surprised if there are not others in the same boat that have not appeared yet, considering the popularity of the project and lambda itself.

Regardless, thanks for the information, and we will discuss in our team our path forward.

@glasser
Copy link
Member

glasser commented Jul 14, 2021

I'm just not sure why it's any more of a hack for you to put the path in the form that API Gateway does it than to put the body in the form that API Gateway does it?

@glasser
Copy link
Member

glasser commented Jul 14, 2021

That said, I think it would be reasonable if we explicitly documented and tested a minimal event shape guaranteed to be supported by apollo-server-lambda. Would that help?

@rnoack1
Copy link
Author

rnoack1 commented Jul 14, 2021

I guess so? But in v2.25 it's required to set path, In 3.0.x according to jcarver989, it's required to set version (event property containing information about which version of API Gateway was used to invoke the lambda). So personally, unless you intend to fix full lambda support, I think you might as well just say it's required to call via API-Gateway and other ways are not officially supported.

@glasser
Copy link
Member

glasser commented Jul 15, 2021

Well, we do also support ALB and "Lambda Edge" (which weren't supported in v2).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants