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

Custom Directives #66

Open
StevenACoffman opened this issue Jul 13, 2019 · 6 comments
Open

Custom Directives #66

StevenACoffman opened this issue Jul 13, 2019 · 6 comments
Labels

Comments

@StevenACoffman
Copy link

Follow-up from #64

Authentication is a shared concern where putting some logic in the gateway may be unavoidable. What we mostly want to avoid is every service change requiring changes to the gateway (which is the case for schema stitching), so the gateway acts as a development bottleneck and every service change can bring down the entire gateway if it adds broken code.

However, many times our permissions (authorization) checks for fields in one service require fetching data from a separate service. I would prefer to avoid having each resolver do the work of looking up the permission because it would be:

  1. much slower, since it happens in serial
  2. super wasteful, since multiple resolvers often need the same permissions data.

I’m not sure what the best solution to this common problem is, but there’s a number of ways I can think of to address this common problem.

  1. Directives could modify the query plan generated by the Planner.
  2. A login mutation could issue a JWT token that contains all the permissions for subsequent requests.
  3. A gateway middleware similar to graphql-shield that implemented a pattern for RBAC such as @knotel/authorize. The gateway owns the rules and contains a JSON structure of authorization checks mapped to resolvers.
  4. A general way to pass context to underlying services or somehow share context between them
@StevenACoffman
Copy link
Author

@AlecAivazis
Copy link
Member

@AlecAivazis I agree with your opinion in all points. On the other hand I often myself in situations, where I don't want to create separate logic just to merge multiple services/schemas together and link specific types to specific fields. I've created method for stitching things together by providing type extensions with special directives to gateway, but the downside was that I always had to redeploy the gateway with updated stitching.

For example:

extend type Comment {
createdByUser: User @link(fetchField: "user", reference: "createdBy")
}
Basically the gateway just fetched it by delegating to same schema this query:

query {
user(id:"{{createdBy}}"): { ... }
}
But this is very basic functionality...that's why I was thrilled while watching the Federation integration :)

Do you think that creating directives that could be parsed from schema (from underlying services) could handle the logic of stitching. I think it should be possible without the need of modifying the current implementation, although being able to integrate it with the query planner would have some major benefits.

Also to support Apollo Federation all you have to do is expose _service: _Service! field and type on query so it's not a big deal for underlying services :) (of course if you need to support reference resolver, you have to implement the _entities: [_Entity]! field/union)

Originally posted by @jakubknejzlik in #64 (comment)

@AlecAivazis
Copy link
Member

Hey @jakubknejzlik! I moved your comment over to this issue to keep the auth/directive conversation all in one place - hope you don't mind!

I've created method for stitching things together by providing type extensions with special directives to gateway, but the downside was that I always had to redeploy the gateway with updated stitching.

This gateway should be automatically stitching types together when their names match. This should save you from having to write that complicated (and error prone logic) in almost all cases.

On the topic of automatically restarting the gateway, this is one of those things I'm very excited to add support for. While the details of when it makes sense for the gateway to recompute the merged schema vary from situation to situation, it wouldn't be very hard to add a method to the gateway object that reinitializes itself, allowing for you to "listen" for updates in your service in a way that makes sense for your situation and always keep your gateway up to date. I will probably write a blog post eventually showing how this could work. But, before I get there, I think gateway-level directives are a higher priority since you can kind of hack around the current API to get automatic-restarts to work.

Do you think that creating directives that could be parsed from schema (from underlying services) could handle the logic of stitching.

I'm not quite sure what you mean exactly. The stitching logic happens by threading together queries very similar to what you showed (as explained here). Unfortunately, this has the effect of limiting the application of a directive to only subqueries to services that define the directive. There is an issue check for this kind of error (and similar ones) before we start planning the query, but I haven't gotten to it yet.

That being said, I do think there is a very good story for gateway-level directives provided that they don't encourage too much logic in the gateway. I think too much flexibility can be very damaging to the API when taken advantage of so I am trying to find the right flavor of flexibility. An example of this balance is that gateway fields only resolve to a type with a specific ID. This allows for you to add specific fields (like viewer) to the root of the API without being able to go crazy and end up with a bunch of logic in the gateway and bringing us right back where we were with schema stitching (see here for an example of exactly what I want to avoid).

I think a gateway-level directive should for be able to modify the query created by the planner but finding the exact API that doesn't allow for too much flexibility is the tricky part. I think there is a very good reason to allow the definition of directives that filter fields based on some context value (either allow the field or return an error). I am not sure how best to allow you to declare what fields that directive is applied to or how to specify this logic, but I am definitely convinced we need some kind of a solution. Do you guys have any ideas?

Sorry this reply got kind of long. I realized that I have done A LOT of thinking on the role of a federated gateway in a larger organization and none of it is written down anywhere. If you got this far, thanks for reading all of this!!

@AlecAivazis AlecAivazis changed the title Feature Request: Gateway Support for First Class Authorization Custom Directives Aug 14, 2019
@jakubknejzlik
Copy link

I see! Didn't know it's possible to merge it this way and it sounds cool! If I understand correctly the approach using Query.node is very similar to what's Apollo doing on their gateway under the hood (except they call it _entities field)...in more "generic" fashion. To be honest, both approaches seems to add additional constraints (@key/@external/... directives for Apollo and node interface and field for Nautilus). I'll definitely try your approach to discover the benefits 👍 .

@AlecAivazis
Copy link
Member

Yea that's for sure true.

Nautilus assumes that each service supports the Node interface/field and uses that to dramatically simplify the number of extra things that have to be supported by each service. The beauty of the id field is that it can contain any information you want as long as it is globally unique. If for example, your type needs two different pieces of information to be unique, just concat them with some character and use that as your record's ID. This seems much cleaner to me than having to decorate your schema with orchestration information that wouldn't have to be there if it wasn't getting merged. Imo, a service should not know that it's part of a larger API, the moment that happens, we're asking for leaking abstractions.

Definitely interested to hear your opinions after working with this!

@jakubknejzlik
Copy link

One little idea/question. Would you consider updating/adding method to be able to fetch multiple nodes in one field? It's possible to make it more efficient by running sending multiple fields at one and using DataLoader, but creating single field to fetch multiple entities at once (by list of IDs) could be easier to implement. What do you think?

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

No branches or pull requests

3 participants