fix(gateway): Resolve gateway retry issues (`UplinkFetcher failed to …

…update...`) (`version-0.x`)(#1504) Forward port of #1503 The current gateway supergraph fetch retry logic doesn't handle the `null` case correctly. It treats `null` as a retry-able result, when it's actually the most common case (meaning no schema change). In the failure case we now throw the most frequently occurring error (via the `async-retry`). This also introduces the `async-retry` package which nicely wraps the retry logic for us and manages exponential backoff.
apollographql · Feb 10, 2022 · f8f198f · f8f198f
1 parent 17b9ca5
commit f8f198f
Show file tree

Hide file tree

Showing 12 changed files with 128 additions and 27 deletions.
diff --git a/docs/source/api/apollo-gateway.mdx b/docs/source/api/apollo-gateway.mdx
@@ -239,6 +239,42 @@ The default value is `false`.
 </tr>
 
 
+<tr>
+<td>
+
+###### `uplinkEndpoints`
+
+`Array<string>`
+</td>
+<td>
+
+A list of Apollo Uplink URLs the gateway uses to poll for its managed configuration. For details and defaults, see [Apollo Uplink](/managed-federation/uplink/).
+
+Provide this field **only** if you are using managed federation _and_ your use case specifically requires it (this is uncommon).
+
+</td>
+</tr>
+
+
+<tr>
+<td>
+
+###### `uplinkMaxRetries`
+
+`number`
+</td>
+<td>
+
+The maximum number of times the gateway retries a failed poll request to Apollo Uplink, cycling through its list of [`uplinkEndpoints`](#uplinkendpoints).
+
+The default value is to try each of your uplinkEndpoints three times (i.e., 5 retries with the default list of two endpoints).
+
+Provide this field **only** if you are using managed federation.
+
+</td>
+</tr>
+
+
 <tr>
 <td>
 

diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md
@@ -7,6 +7,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
 - Use specific error classes when throwing errors due Apollo Uplink being unreacheable or returning an invalid response [PR #1473](https://github.com/apollographql/federation/pull/1473)
+- __FIX__ Correct retry logic while fetching the supergraph schema from Uplink [PR #1503](https://github.com/apollographql/federation/pull/1503)
+
 
 ## v2.0.0-alpha.5
 

diff --git a/gateway-js/package.json b/gateway-js/package.json
@@ -38,6 +38,7 @@
     "apollo-server-errors": "^2.5.0 || ^3.0.0",
     "apollo-server-types": "^0.9.0 || ^3.0.0",
     "apollo-utilities": "^1.3.0",
+    "async-retry": "^1.3.3",
     "loglevel": "^1.6.1",
     "make-fetch-happen": "^8.0.0",
     "pretty-format": "^27.0.0",

diff --git a/gateway-js/src/__tests__/integration/nockMocks.ts b/gateway-js/src/__tests__/integration/nockMocks.ts
@@ -121,9 +121,10 @@ export function mockSupergraphSdlRequestSuccessIfAfter(
 }
 
 export function mockSupergraphSdlRequestIfAfterUnchanged(
-    ifAfter: string | null = null,
+  ifAfter: string | null = null,
+  url: string = mockCloudConfigUrl1,
 ) {
-  return mockSupergraphSdlRequestIfAfter(ifAfter).reply(
+  return mockSupergraphSdlRequestIfAfter(ifAfter, url).reply(
       200,
       JSON.stringify({
         data: {

diff --git a/gateway-js/src/index.ts b/gateway-js/src/index.ts
@@ -402,7 +402,7 @@ export class ApolloGateway implements GraphQLService {
           apiKey: this.apolloConfig!.key!,
           uplinkEndpoints,
           maxRetries:
-            this.config.uplinkMaxRetries ?? uplinkEndpoints.length * 3,
+            this.config.uplinkMaxRetries ?? uplinkEndpoints.length * 3 - 1, // -1 for the initial request
           subgraphHealthCheck: this.config.serviceHealthCheck,
           fetcher: this.fetcher,
           logger: this.logger,

diff --git a/gateway-js/src/supergraphManagers/LegacyFetcher/index.ts b/gateway-js/src/supergraphManagers/LegacyFetcher/index.ts
@@ -219,7 +219,7 @@ export class LegacyFetcher implements SupergraphManager {
 
   private logUpdateFailure(e: any) {
     this.config.logger?.error(
-      'UplinkFetcher failed to update supergraph with the following error: ' +
+      'LegacyFetcher failed to update supergraph with the following error: ' +
         (e.message ?? e),
     );
   }

diff --git a/gateway-js/src/supergraphManagers/LocalCompose/index.ts b/gateway-js/src/supergraphManagers/LocalCompose/index.ts
@@ -72,7 +72,7 @@ export class LocalCompose implements SupergraphManager {
 
   private logUpdateFailure(e: any) {
     this.config.logger?.error(
-      'UplinkFetcher failed to update supergraph with the following error: ' +
+      'LocalCompose failed to update supergraph with the following error: ' +
         (e.message ?? e),
     );
   }

diff --git a/...ay-js/src/supergraphManagers/UplinkFetcher/__tests__/loadSupergraphSdlFromStorage.test.ts b/...ay-js/src/supergraphManagers/UplinkFetcher/__tests__/loadSupergraphSdlFromStorage.test.ts
@@ -63,7 +63,8 @@ describe('loadSupergraphSdlFromStorage', () => {
       errorReportingEndpoint: undefined,
       fetcher,
       compositionId: "originalId-1234",
-      maxRetries: 1
+      maxRetries: 1,
+      roundRobinSeed: 0,
     });
 
     expect(result).toMatchObject({
@@ -85,7 +86,8 @@ describe('loadSupergraphSdlFromStorage', () => {
         errorReportingEndpoint: undefined,
         fetcher,
         compositionId: "originalId-1234",
-        maxRetries: 1
+        maxRetries: 1,
+        roundRobinSeed: 0,
       }),
     ).rejects.toThrowError(
       new UplinkFetcherError(
@@ -368,3 +370,28 @@ describe('loadSupergraphSdlFromStorage', () => {
   });
 });
 
+
+describe("loadSupergraphSdlFromUplinks", () => {
+  beforeEach(nockBeforeEach);
+  afterEach(nockAfterEach);
+
+  it("doesn't retry in the unchanged / null case", async () => {
+    mockSupergraphSdlRequestIfAfterUnchanged("id-1234", mockCloudConfigUrl1);
+
+    const fetcher = jest.fn(getDefaultFetcher());
+    const result = await loadSupergraphSdlFromUplinks({
+      graphRef,
+      apiKey,
+      endpoints: [mockCloudConfigUrl1, mockCloudConfigUrl2],
+      errorReportingEndpoint: mockOutOfBandReporterUrl,
+      fetcher: fetcher as any,
+      compositionId: "id-1234",
+      maxRetries: 5,
+      roundRobinSeed: 0,
+    });
+
+    expect(result).toBeNull();
+    expect(fetcher).toHaveBeenCalledTimes(1);
+  });
+});
+
diff --git a/gateway-js/src/supergraphManagers/UplinkFetcher/index.ts b/gateway-js/src/supergraphManagers/UplinkFetcher/index.ts
@@ -30,6 +30,7 @@ export class UplinkFetcher implements SupergraphManager {
   private errorReportingEndpoint: string | undefined =
     process.env.APOLLO_OUT_OF_BAND_REPORTER_ENDPOINT ?? undefined;
   private compositionId?: string;
+  private fetchCount: number = 0;
 
   constructor(options: UplinkFetcherOptions) {
     this.config = options;
@@ -81,6 +82,7 @@ export class UplinkFetcher implements SupergraphManager {
       fetcher: this.config.fetcher,
       compositionId: this.compositionId ?? null,
       maxRetries: this.config.maxRetries,
+      roundRobinSeed: this.fetchCount++,
     });
 
     if (!result) {

diff --git a/gateway-js/src/supergraphManagers/UplinkFetcher/loadSupergraphSdlFromStorage.ts b/gateway-js/src/supergraphManagers/UplinkFetcher/loadSupergraphSdlFromStorage.ts
@@ -1,5 +1,6 @@
 import { fetch, Response, Request } from 'apollo-server-env';
 import { GraphQLError } from 'graphql';
+import retry from 'async-retry';
 import { SupergraphSdlUpdate } from '../../config';
 import { submitOutOfBandReportIfConfigured } from './outOfBandReporter';
 import { SupergraphSdlQuery } from '../../__generated__/graphqlTypes';
@@ -39,8 +40,6 @@ const { name, version } = require('../../../package.json');
 
 const fetchErrorMsg = "An error occurred while fetching your schema from Apollo: ";
 
-let fetchCounter = 0;
-
 export class UplinkFetcherError extends Error {
   constructor(message: string) {
     super(message);
@@ -56,36 +55,35 @@ export async function loadSupergraphSdlFromUplinks({
   fetcher,
   compositionId,
   maxRetries,
+  roundRobinSeed,
 }: {
   graphRef: string;
   apiKey: string;
   endpoints: string[];
   errorReportingEndpoint: string | undefined,
   fetcher: typeof fetch;
   compositionId: string | null;
-  maxRetries: number
+  maxRetries: number,
+  roundRobinSeed: number,
 }) : Promise<SupergraphSdlUpdate | null> {
-  let retries = 0;
-  let lastException = null;
-  let result: SupergraphSdlUpdate | null = null;
-  while (retries++ <= maxRetries && result == null) {
-    try {
-      result = await loadSupergraphSdlFromStorage({
+  // This Promise resolves with either an updated supergraph or null if no change.
+  // This Promise can reject in the case that none of the retries are successful,
+  // in which case it will reject with the most frequently encountered error.
+  return retry(
+    () =>
+      loadSupergraphSdlFromStorage({
         graphRef,
         apiKey,
-        endpoint: endpoints[fetchCounter++ % endpoints.length],
+        endpoint: endpoints[roundRobinSeed++ % endpoints.length],
         errorReportingEndpoint,
         fetcher,
-        compositionId
-      });
-    } catch (e) {
-      lastException = e;
-    }
-  }
-  if (result === null && lastException !== null) {
-    throw lastException;
-  }
-  return result;
+        compositionId,
+      }),
+    {
+      retries: maxRetries,
+    },
+  );
+
 }
 
 export async function loadSupergraphSdlFromStorage({

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -55,6 +55,7 @@
     "@rollup/plugin-commonjs": "21.0.1",
     "@rollup/plugin-json": "4.1.0",
     "@rollup/plugin-node-resolve": "13.1.3",
+    "@types/async-retry": "^1.4.3",
     "@types/bunyan": "1.8.8",
     "@types/deep-equal": "1.0.1",
     "@types/deep-freeze": "0.1.2",