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

ApolloGateway: Construct and use RemoteGraphQLDataSource to issue introspection query to Federated Services #3120

Merged

Conversation

mcohen75
Copy link
Contributor

@mcohen75 mcohen75 commented Aug 1, 2019

This is meant to address #2891.

RemoteGraphQLDataSource provides a nice way to enhance requests before they are sent to federated services. This same mechanism is suitable for enhancing requests sent to federated services when issuing an enhanced introspection query.

ApolloGateway currently accepts an optional HeadersInit parameter that facilitates enhancing the request with headers. This is sufficient if all you need to do is send the same headers to all federated services. In many environments, however, services participating in federation will have diverse requirements. Note that this change is compatible with the HeadersInit parameter.

This change provides:

  1. Access to the full range of request enhancement provided by RemoteGraphQLDataSource's willSendRequest function. Notably, this allows enhancing the url, headers and extensions.
  2. The ability to enhance requests on a per service basis.

A side benefit is a small savings in duplicated code as RemoteGraphQLDataSource already knows how to issue a GraphQL request over HTTP.

@mcohen75
Copy link
Contributor Author

mcohen75 commented Aug 2, 2019

@JacksonKearl given your involvement in #2891 and the introduction of the introspectionHeaders parameter I think this change may be interesting to you.

@mcohen75 mcohen75 changed the title Construct and use RemoteGraphQLDataSource to issue introspection query ApolloGateway: Construct and use RemoteGraphQLDataSource to issue introspection query to Federated Services Aug 2, 2019
@mcohen75
Copy link
Contributor Author

mcohen75 commented Aug 6, 2019

Is this something that we could incorporate @JakeDawkins @zionts? I believe this provides very useful functionality and is a sane way to enrich introspection requests. If there is anything else I can do here to move this forward please do let me know.

@JacksonKearl
Copy link

Hey @mcohen75, thanks for this! We're definitely interested in expanding the configurability of the introspection request. One concern I have with this as it stands is that it is a breaking change on the old API (by changing the parameters of protected methods). While we are still in pre-1.0 and thus could in theory adopt this with a minor, it would be nice to have backwards compatibility. @jbaxleyiii what do you think re. backwards compatibility?

…e here is that we end up creating a RemoteGraphQLDataSource especially for the introspection request.
@mcohen75 mcohen75 force-pushed the load-service-via-datasource branch from b84a25b to b715381 Compare August 7, 2019 16:35
@mcohen75 mcohen75 force-pushed the load-service-via-datasource branch from b715381 to 1bc68d9 Compare August 7, 2019 16:37
@mcohen75
Copy link
Contributor Author

mcohen75 commented Aug 7, 2019

Thank you for following up @JacksonKearl! I've updated my solution here to preserve backwards compatibility. The only downside is that I end up creating the remote data source just to issue the enhanced introspection query. This seems like a reasonable tradeoff to me if we'd like to preserve backwards compatibility.

I chose to go this route since services are only added to the serviceMap after the schema is loaded and validated. I wasn't sure if this constraint is one that we wanted to violate.

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to put this PR together! This seems like a reasonable change to me. My only major request would be to add a few tests that demonstrate the successful addition of headers on a per-service basis. Is that something you would feel comfortable doing? If you'd like any guidance, please don't hesitate to ask.

packages/apollo-gateway/src/index.ts Outdated Show resolved Hide resolved
@mcohen75
Copy link
Contributor Author

mcohen75 commented Aug 8, 2019

add a few tests that demonstrate the successful addition of headers on a per-service basis

Will do!

@mcohen75 mcohen75 force-pushed the load-service-via-datasource branch from feba535 to a9e4753 Compare August 21, 2019 16:16
@mcohen75
Copy link
Contributor Author

@trevor-scheer I have added tests to confirm that 1) enhanced introspection requests are made using the data source and 2) requests can be customized per service. I've also addressed the other feedback you provided.

@trevor-scheer
Copy link
Member

Tests LGTM, thank you for taking the time @mcohen75 🙇

@trevor-scheer trevor-scheer merged commit a1ddc95 into apollographql:master Aug 22, 2019
@zionts
Copy link
Contributor

zionts commented Aug 22, 2019

@trevor-scheer @mcohen75 I wonder if we might be able to use this functionality to enable some simple retry semantics on federation introspection as well, since services going down or experiencing brief interruptions to service shouldn't cause the gateway to crash on startup. I'm assuming that a RemoteGraphQLDataSource will not automatically issue retries due to 5xx errors, but including some basic timeouts and retries would likely go a long way :)

@alex-hioperator
Copy link

Hey guys! Super excited this feature is getting added--its something we need to handle authentication on introspection since google cloud IAM requires getting an auth token for a specific URL for each query.

I'm a bit newer to the community here, so I was just wondering if there's an example of the new way to set the introspection query?

@trevor-scheer
Copy link
Member

@alex-hioperator, we do have some docs with an example. Let me know if you have any questions beyond what's covered here 👍

https://www.apollographql.com/docs/apollo-server/federation/implementing/#sharing-context-across-services

@alex-hioperator
Copy link

@trevor-scheer thanks for the super fast reply!

The bit I'm unclear on is how to use the flexibility of willSendRequest in that initial introspection query. Though looking at the docs now, I do see the recommendation of using the schema from the registry for production rather than introspection--so that would actually work. I'd just need to configure it as such.

@cluedtke
Copy link

The bit I'm unclear on is how to use the flexibility of willSendRequest in that initial introspection query.

Agree with this ^^

Our federated endpoints are behind auth, so even introspection queries require an auth header. introspectionHeaders works, but user tokens (for non-introspection queries) are also sent to gateway and need to be forwarded to the federated services, which requires willSendRequest(). Implementing willSendRequest() causes introspectionHeaders not to work..

So we had to remove introspectionHeaders and use willSendRequest() exclusively. The smelly part of this is that now in willSendRequest() we have a conditional, which generates an introspection auth header if the request.query is GetServiceDefinition.

abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…rospection query to Federated Services (apollographql/apollo-server#3120)

* Construct and use a GraphQLDataSource when performing the enhanced introspection request.

* Preserve backwards compatibility of protected methods. The consequence here is that we end up creating a RemoteGraphQLDataSource especially for the introspection request.

* Add test to demonstrate that different headers can be passed based on the service.
Apollo-Orig-Commit-AS: apollographql/apollo-server@a1ddc95
@pitazzo
Copy link

pitazzo commented Dec 13, 2020

@cluedtke would you show me how do you generate those introspection auth headers?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants