[Ingest Manager] Split Registry errors into Connection & Response (#7…

…6558) (#76634) * Split Registry errors into Connection & Response * Ensure a url in ResponseError message. Add tests
elastic · Sep 3, 2020 · cad95fc · cad95fc
1 parent d62bed0
commit cad95fc
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 33 deletions.
diff --git a/x-pack/plugins/ingest_manager/server/errors.ts b/x-pack/plugins/ingest_manager/server/errors.ts
@@ -17,15 +17,14 @@ export const getHTTPResponseCode = (error: IngestManagerError): number => {
     return 502; // Bad Gateway
   }
   if (error instanceof PackageNotFoundError) {
-    return 404;
-  }
-  if (error instanceof PackageOutdatedError) {
-    return 400;
-  } else {
-    return 400; // Bad Request
+    return 404; // Not Found
   }
+
+  return 400; // Bad Request
 };
 
 export class RegistryError extends IngestManagerError {}
+export class RegistryConnectionError extends RegistryError {}
+export class RegistryResponseError extends RegistryError {}
 export class PackageNotFoundError extends IngestManagerError {}
 export class PackageOutdatedError extends IngestManagerError {}
diff --git a/x-pack/plugins/ingest_manager/server/services/epm/registry/requests.test.ts b/x-pack/plugins/ingest_manager/server/services/epm/registry/requests.test.ts
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 import { fetchUrl } from './requests';
-import { RegistryError } from '../../../errors';
+import { RegistryError, RegistryConnectionError, RegistryResponseError } from '../../../errors';
 jest.mock('node-fetch');
 
 const { Response, FetchError } = jest.requireActual('node-fetch');
@@ -53,13 +53,7 @@ describe('setupIngestManager', () => {
             throw new FetchError('message 3', 'system', { code: 'ESOMETHING' });
           })
           // this one succeeds
-          .mockImplementationOnce(() => Promise.resolve(new Response(successValue)))
-          .mockImplementationOnce(() => {
-            throw new FetchError('message 5', 'system', { code: 'ESOMETHING' });
-          })
-          .mockImplementationOnce(() => {
-            throw new FetchError('message 6', 'system', { code: 'ESOMETHING' });
-          });
+          .mockImplementationOnce(() => Promise.resolve(new Response(successValue)));
 
         const promise = fetchUrl('');
         await expect(promise).resolves.toEqual(successValue);
@@ -69,7 +63,7 @@ describe('setupIngestManager', () => {
         expect(actualResultsOrder).toEqual(['throw', 'throw', 'throw', 'return']);
       });
 
-      it('or error after 1 failure & 5 retries with RegistryError', async () => {
+      it('or error after 1 failure & 5 retries with RegistryConnectionError', async () => {
         fetchMock
           .mockImplementationOnce(() => {
             throw new FetchError('message 1', 'system', { code: 'ESOMETHING' });
@@ -88,21 +82,93 @@ describe('setupIngestManager', () => {
           })
           .mockImplementationOnce(() => {
             throw new FetchError('message 6', 'system', { code: 'ESOMETHING' });
-          })
-          .mockImplementationOnce(() => {
-            throw new FetchError('message 7', 'system', { code: 'ESOMETHING' });
-          })
-          .mockImplementationOnce(() => {
-            throw new FetchError('message 8', 'system', { code: 'ESOMETHING' });
           });
 
         const promise = fetchUrl('');
-        await expect(promise).rejects.toThrow(RegistryError);
+        await expect(promise).rejects.toThrow(RegistryConnectionError);
         // doesn't retry after 1 failure & 5 failed retries
         expect(fetchMock).toHaveBeenCalledTimes(6);
         const actualResultsOrder = fetchMock.mock.results.map(({ type }: { type: string }) => type);
         expect(actualResultsOrder).toEqual(['throw', 'throw', 'throw', 'throw', 'throw', 'throw']);
       });
     });
+
+    describe('4xx or 5xx from Registry become RegistryResponseError', () => {
+      it('404', async () => {
+        fetchMock.mockImplementationOnce(() => ({
+          ok: false,
+          status: 404,
+          statusText: 'Not Found',
+          url: 'https://example.com',
+        }));
+        const promise = fetchUrl('');
+        await expect(promise).rejects.toThrow(RegistryResponseError);
+        await expect(promise).rejects.toThrow(
+          `'404 Not Found' error response from package registry at https://example.com`
+        );
+        expect(fetchMock).toHaveBeenCalledTimes(1);
+      });
+
+      it('429', async () => {
+        fetchMock.mockImplementationOnce(() => ({
+          ok: false,
+          status: 429,
+          statusText: 'Too Many Requests',
+          url: 'https://example.com',
+        }));
+        const promise = fetchUrl('');
+        await expect(promise).rejects.toThrow(RegistryResponseError);
+        await expect(promise).rejects.toThrow(
+          `'429 Too Many Requests' error response from package registry at https://example.com`
+        );
+        expect(fetchMock).toHaveBeenCalledTimes(1);
+      });
+
+      it('500', async () => {
+        fetchMock.mockImplementationOnce(() => ({
+          ok: false,
+          status: 500,
+          statusText: 'Internal Server Error',
+          url: 'https://example.com',
+        }));
+        const promise = fetchUrl('');
+        await expect(promise).rejects.toThrow(RegistryResponseError);
+        await expect(promise).rejects.toThrow(
+          `'500 Internal Server Error' error response from package registry at https://example.com`
+        );
+        expect(fetchMock).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    describe('url in RegistryResponseError message is response.url || requested_url', () => {
+      it('given response.url, use that', async () => {
+        fetchMock.mockImplementationOnce(() => ({
+          ok: false,
+          status: 404,
+          statusText: 'Not Found',
+          url: 'https://example.com/?from_response=true',
+        }));
+        const promise = fetchUrl('https://example.com/?requested=true');
+        await expect(promise).rejects.toThrow(RegistryResponseError);
+        await expect(promise).rejects.toThrow(
+          `'404 Not Found' error response from package registry at https://example.com/?from_response=true`
+        );
+        expect(fetchMock).toHaveBeenCalledTimes(1);
+      });
+
+      it('no response.url, use requested url', async () => {
+        fetchMock.mockImplementationOnce(() => ({
+          ok: false,
+          status: 404,
+          statusText: 'Not Found',
+        }));
+        const promise = fetchUrl('https://example.com/?requested=true');
+        await expect(promise).rejects.toThrow(RegistryResponseError);
+        await expect(promise).rejects.toThrow(
+          `'404 Not Found' error response from package registry at https://example.com/?requested=true`
+        );
+        expect(fetchMock).toHaveBeenCalledTimes(1);
+      });
+    });
   });
 });
diff --git a/x-pack/plugins/ingest_manager/server/services/epm/registry/requests.ts b/x-pack/plugins/ingest_manager/server/services/epm/registry/requests.ts
@@ -7,7 +7,7 @@
 import fetch, { FetchError, Response } from 'node-fetch';
 import pRetry from 'p-retry';
 import { streamToString } from './streams';
-import { RegistryError } from '../../../errors';
+import { RegistryError, RegistryConnectionError, RegistryResponseError } from '../../../errors';
 
 type FailedAttemptErrors = pRetry.FailedAttemptError | FetchError | Error;
 
@@ -19,10 +19,13 @@ async function registryFetch(url: string) {
     return response;
   } else {
     // 4xx & 5xx responses
-    // exit without retry & throw RegistryError
-    throw new pRetry.AbortError(
-      new RegistryError(`Error connecting to package registry at ${url}: ${response.statusText}`)
-    );
+    const { status, statusText, url: resUrl } = response;
+    const message = `'${status} ${statusText}' error response from package registry at ${
+      resUrl || url
+    }`;
+    const responseError = new RegistryResponseError(message);
+
+    throw new pRetry.AbortError(responseError);
   }
 }
 
@@ -38,17 +41,24 @@ export async function getResponse(url: string): Promise<Response> {
         // and let the others through without retrying
         //
         // throwing in onFailedAttempt will abandon all retries & fail the request
-        // we only want to retry system errors, so throw a RegistryError for everything else
+        // we only want to retry system errors, so re-throw for everything else
         if (!isSystemError(error)) {
-          throw new RegistryError(
-            `Error connecting to package registry at ${url}: ${error.message}`
-          );
+          throw error;
         }
       },
     });
     return response;
-  } catch (e) {
-    throw new RegistryError(`Error connecting to package registry at ${url}: ${e.message}`);
+  } catch (error) {
+    // isSystemError here means we didn't succeed after max retries
+    if (isSystemError(error)) {
+      throw new RegistryConnectionError(`Error connecting to package registry: ${error.message}`);
+    }
+    // don't wrap our own errors
+    if (error instanceof RegistryError) {
+      throw error;
+    } else {
+      throw new RegistryError(error);
+    }
   }
 }