feat: Support make-fetch-happen fetcher when communicating with services

[Bhoomikapanwar]

The custom fetcher that the RemoteGraphQLDataSource accepts doesn't currently support make fetch happen fetcher. This PR is to add support for the same i.e add support for passing make fetch happen fetcher (any other custom fetcher other than node ) to downstream services which is required for use cases where we want to provide fetcher other than node-fetch in order to leverage functionalities provided by them.
To specifically speak of the concerned use case, our organisation wants to use a fetcher where we can configure default request options such as timeout and make fetch happen fetcher exactly fits our requirement. But with the existing codebase, passing that custom fetcher doesn't work correctly. (I debugged a lot and found the ultimate cause - which is we need the Request to be constructed by the fetcher's own Request class instead of node fetch's Request class as done currently) Hence, these are the proposed changes along with previous tests fixed and new tests added. Please look into the changes, it will really help us unblock and move forward with using Apollo gateway in production. Also, let me know if any other changes are required, I'll be happy to accommodate.

P.S. This to me feels as bug cum feature, let me know what should be correct category and I'll update the version and CHANGEME accordingly.

Thanks a ton!
Bhoomika Panwar

Closes #193
Closes #191

[apollo-cla]

@Bhoomikapanwar: 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/

[Bhoomikapanwar]

To further add, this will ultimately help us to make make-fetch-happen as the default fetcher for downstream services and at the same time facilitate passing of gateway's fetcher to downstream services smoothly which aligns with Apollo's vision, but this change being the pre-requisite. I'll be more than happy to contribute to these coming features as well.

[Bhoomikapanwar]

Here is the issue I created for the same: https://github.com/apollographql/apollo-server/issues/4607

[abernix]

Thank you so much for tracking this down and putting this together! Very much appreciated. I've offered a couple in-line suggestions below which I'll commit before merging this, and I've made a couple tweaks to the CHANGELOG.md, but we'll get this released soon. Relatedly, I'll also call attention to #193 (comment), since you're probably about to use make-fetch-happen. I believe you're only suggesting you'd be relying on its timeout behavior, but there's an important caveat around retries.

[abernix]

While we should have a "major" bump, our bumping apparatus will handle this!

Suggested change

	"version": "0.21.0",
	"version": "0.20.4",

[Bhoomikapanwar]

Thanks a ton for picking this up so soon 👍
Moreover, Thank you for specifically pointing out the issue around retries, I absolutely agree to it. For our case, we just need timeout configurations at the moment, so it should be fine.

One more thing, do we plan to make make-fetch-happen as the default fetcher for downstream services in this release or in the coming ones?

[abernix]

I really enjoy that this pattern now matches up with the standard invocation syntax!

[Bhoomikapanwar]

Thanks a lot for reviewing and merging it! 👍

Just to clarify, I would like to bring your attention to the following points:

1. The above changes will add support for make-fetch-happen as a custom fetcher to be passed to downstream services; so this will definitely fix Apollo gateway: Custom fetcher to RemoteGraphQLDataSource doesn't work correctly #191.

2. The default fetcher for RemoteGraphQLDataSource continues to be node-fetch. If we plan to make it as the default fetcher, I can make the required change after I get a thumbs up to go-ahead.

3. Moreover as far my understanding goes, Apollo gateway: override the default fetcher with timeout/retry options not working #193 is bit different and is still unresolved, it points out the issue that there's a conflict between what the gateway's documentation suggests about the fetcher option and what effect does it have; quoting it from there "When specified, overrides the default Fetch API implementation which is used when communicating with downstream services."

You can see the reproduction setup here --> #193 (comment)
^^The fetcher options won't have any effect on the request that happens between the gateway and the RemoteGraphQLDataSource which is different from what the doc says.

After quite amount of debugging; it looks like the gateway never passes its fetcher to the RemoteGraphQLDataSource, that's why whatever fetcher configurations (default options) that we pass to the gateway are only utilised at the time of loading of schema from storage (schema registry) and are not passed down further to the services and instead RemoteGraphQLDataSource constructs its own fetcher and hence both are unrelated.
See here --> https://github.com/apollographql/federation/blob/main/gateway-js/src/index.ts#L572

In order to fix it and if we really plan to forward gateway's fetcher to RemoteGraphQLDataSource at the time of service initialisation. We'll need to do something like this;

return this.config.buildService
      ? this.config.buildService(serviceDef, this.fetcher)
      : new RemoteGraphQLDataSource({
          url: serviceDef.url,
          fetcher: this.fetcher
        });

And in case, we don't intend to do so, I think we should update the documentation for gateway to avoid any confusion around fetchers.

Please let me know your thoughts on it.

[ankit-joinwal]

Thanks a lot @Bhoomikapanwar for fixing this. This improves resiliency of Gateway when downstream services go crazy and in turn affect the entire data graph. @abernix thank you for proactively supporting this in no time. Looking forward to getting the fix in a release.

[abernix]

Released in @apollo/[email protected]!

[abernix]

Thank you for the clarity.

I don't think we should make make-fetch-happen the default fetcher yet, but we can consider doing that soon. I would like us to be explicit about what potential side-effects of doing that might be and, honestly, any learnings you have here (now that you can override it!) would be super valuable. (Mostly I'd just like to take some time to think about it, which I don't quite have right now, so let me know if you find it to be a successful transition. It largely revolves around thinking carefully about side-effects, e.g. #193 (comment))

Moreover as far my understanding goes, #193 is bit different and is still unresolved, it points out the issue that there's a conflict between what the gateway's documentation suggests about the fetcher option and what effect does it have; quoting it from there "When specified, overrides the default Fetch API implementation which is used when communicating with downstream services."

I think my "Closing" of #193 wasn't correct in this regard. I've re-opened #193 until we can correct that.

it looks like the gateway never passes its fetcher to the RemoteGraphQLDataSource, that's why whatever fetcher configurations (default options) that we pass to the gateway are only utilised at the time of loading of schema from storage (schema registry)

This is actually the expected behavior right now — in that fetcher on ApolloGateway only applies to managed federation's fetching of the schema. The intended mode of setting a custom fetcher on a downstream RemoteGraphQLDataSources is that it be done within the buildService(service) => RemoteGraphQLDataSource function that can be passed to ApolloGateway. Essentially, something along the lines of (untested, but roughly):

const gateway = new ApolloGateway({
  buildService({ url, name }) {
    return new (class extends RemoteGraphQLDataSource {
      fetcher = require('make-fetch-happen').defaults();
    })({
      url,
      name,
    });
  }
});

If this works, it sounds like just a documentation PR might be in order for the time-being? (In case anyone is tempted to make a PR, I will point out that currently, while #142 is in progress, the published docs continue to ship from the apollo-server repository's docs/source/federation directory, even though this code-base has a copy of the documentation too. The version in this repository is the version we intend on being the published version so PRs should be against this repo. I'm still working out the details of that with our Developer Experience team, but sometime next week is what I expect right now.

[Bhoomikapanwar]

Yeah, I understand your concerns regarding making make-fetch-happen fetcher as the default one for now. I think we can handle it two ways, either we figure out what all fetcher options are not safe to be passed by the application; and silently override it with the correct ones; for eg. if they pass any retries, we just do retries: false or we can take the other approach where we throw an exception and let the application explicitly know that they are doing something tricky here and it can have side effects.
P.S. On that note, I think even timeout as an option can pose similar issues as retries for non-idempotent mutations.
Nevertheless, I'll be using it as a custom fetcher so if there are any other issues that I find, will surely update you!

On #193, In my opinion, keeping the fetchers different for gateway and downstream services makes more sense as they have very different purposes, and applications will want in the flexibility to have different fetcher configuration for both and not worry about figuring out a set of configurations that fits both (which I think would be not really possible, for eg. for schema fetching, we'll definitely want to have caching or multiple retries but not for the downstream services) and as you said, If that's the expected behaviour right now, I think I can help with the documentation PR against the repo that you pointed out. 👍