From fdf401d5cbcc2533ec5490cf5046a6d75d121494 Mon Sep 17 00:00:00 2001 From: Xujia Zhou Date: Fri, 31 Mar 2023 13:24:13 +0200 Subject: [PATCH] feat: add retry to sendTestPayload Co-authored-by: Carlos Cu-authored-by: Lucian --- src/lib/errors/bad-gateway-error.ts | 14 +++++ src/lib/errors/index.ts | 2 + src/lib/errors/service-unavailable-error.ts | 14 +++++ src/lib/snyk-test/common.ts | 3 + src/lib/snyk-test/run-test.ts | 47 +++++++++++++-- test/acceptance/fake-server.ts | 28 ++++++++- test/jest/acceptance/snyk-fix/fix.spec.ts | 2 +- .../snyk-test/retry-mechanism.spec.ts | 60 +++++++++++++++++++ test/tap/cli-test.acceptance.test.ts | 2 + test/tap/cli-test/cli-test.generic.spec.ts | 2 +- test/tap/remote-package.test.ts | 2 +- 11 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 src/lib/errors/bad-gateway-error.ts create mode 100644 src/lib/errors/service-unavailable-error.ts create mode 100644 test/jest/acceptance/snyk-test/retry-mechanism.spec.ts diff --git a/src/lib/errors/bad-gateway-error.ts b/src/lib/errors/bad-gateway-error.ts new file mode 100644 index 0000000000..77d6c84938 --- /dev/null +++ b/src/lib/errors/bad-gateway-error.ts @@ -0,0 +1,14 @@ +import { CustomError } from './custom-error'; + +export class BadGatewayError extends CustomError { + private static ERROR_CODE = 502; + private static ERROR_STRING_CODE = 'BAD_GATEWAY_ERROR'; + private static ERROR_MESSAGE = 'Bad gateway error'; + + constructor(userMessage) { + super(BadGatewayError.ERROR_MESSAGE); + this.code = BadGatewayError.ERROR_CODE; + this.strCode = BadGatewayError.ERROR_STRING_CODE; + this.userMessage = userMessage || BadGatewayError.ERROR_MESSAGE; + } +} diff --git a/src/lib/errors/index.ts b/src/lib/errors/index.ts index b2bb7312ea..205f3bb38a 100644 --- a/src/lib/errors/index.ts +++ b/src/lib/errors/index.ts @@ -10,6 +10,8 @@ export { ConnectionTimeoutError } from './connection-timeout-error'; export { FailedToLoadPolicyError } from './failed-to-load-policy-error'; export { PolicyNotFoundError } from './policy-not-found-error'; export { InternalServerError } from './internal-server-error'; +export { BadGatewayError } from './bad-gateway-error'; +export { ServiceUnavailableError } from './service-unavailable-error'; export { FailedToGetVulnerabilitiesError } from './failed-to-get-vulnerabilities-error'; export { FailedToGetVulnsFromUnavailableResource } from './failed-to-get-vulns-from-unavailable-resource'; export { UnsupportedPackageManagerError } from './unsupported-package-manager-error'; diff --git a/src/lib/errors/service-unavailable-error.ts b/src/lib/errors/service-unavailable-error.ts new file mode 100644 index 0000000000..60497ebefe --- /dev/null +++ b/src/lib/errors/service-unavailable-error.ts @@ -0,0 +1,14 @@ +import { CustomError } from './custom-error'; + +export class ServiceUnavailableError extends CustomError { + private static ERROR_CODE = 503; + private static ERROR_STRING_CODE = 'SERVICE_UNAVAILABLE_ERROR'; + private static ERROR_MESSAGE = 'Service unavailable error'; + + constructor(userMessage) { + super(ServiceUnavailableError.ERROR_MESSAGE); + this.code = ServiceUnavailableError.ERROR_CODE; + this.strCode = ServiceUnavailableError.ERROR_STRING_CODE; + this.userMessage = userMessage || ServiceUnavailableError.ERROR_MESSAGE; + } +} diff --git a/src/lib/snyk-test/common.ts b/src/lib/snyk-test/common.ts index 286ab98238..ac0230bffe 100644 --- a/src/lib/snyk-test/common.ts +++ b/src/lib/snyk-test/common.ts @@ -65,3 +65,6 @@ export enum FAIL_ON { } export type FailOn = 'all' | 'upgradable' | 'patchable'; + +export const RETRY_ATTEMPTS = 3; +export const RETRY_DELAY = 500; diff --git a/src/lib/snyk-test/run-test.ts b/src/lib/snyk-test/run-test.ts index e4a9a7d67b..21340aaac9 100644 --- a/src/lib/snyk-test/run-test.ts +++ b/src/lib/snyk-test/run-test.ts @@ -30,6 +30,8 @@ import { NoSupportedManifestsFoundError, NotFoundError, errorMessageWithRetry, + BadGatewayError, + ServiceUnavailableError, } from '../errors'; import * as snyk from '../'; import { isCI } from '../is-ci'; @@ -68,6 +70,8 @@ import { assembleEcosystemPayloads } from './assemble-payloads'; import { makeRequest } from '../request'; import { spinner } from '../spinner'; import { hasUnknownVersions } from '../dep-graph'; +import { sleep } from '../common'; +import { RETRY_ATTEMPTS, RETRY_DELAY } from './common'; const debug = debugModule('snyk:run-test'); @@ -234,13 +238,38 @@ async function sendAndParseResults( response: any; }; const requests: (() => Promise)[] = []; - for (const payload of payloads) { + for (const originalPayload of payloads) { const request = async (): Promise => { - /** sendTestPayload() deletes the request.body from the payload once completed. */ - const originalPayload = Object.assign({}, payload); - const response = await sendTestPayload(payload); - return { payload, originalPayload, response }; + let step = 0; + let error; + + while (step < RETRY_ATTEMPTS) { + debug(`sendTestPayload retry step ${step} out of ${RETRY_ATTEMPTS}`); + try { + /** sendTestPayload() deletes the request.body from the payload once completed. */ + const payload = Object.assign({}, originalPayload); + const response = await sendTestPayload(payload); + + return { payload, originalPayload, response }; + } catch (err) { + error = err; + step++; + + if ( + err instanceof InternalServerError || + err instanceof BadGatewayError || + err instanceof ServiceUnavailableError + ) { + await sleep(RETRY_DELAY); + } else { + break; + } + } + } + + throw error; }; + requests.push(request); } @@ -516,6 +545,14 @@ function handleTestHttpErrorResponse(res, body) { err = new InternalServerError(userMessage); err.innerError = body.stack; break; + case 502: + err = new BadGatewayError(userMessage); + err.innerError = body.stack; + break; + case 503: + err = new ServiceUnavailableError(userMessage); + err.innerError = body.stack; + break; default: err = new FailedToGetVulnerabilitiesError(userMessage, statusCode); err.innerError = body.error; diff --git a/test/acceptance/fake-server.ts b/test/acceptance/fake-server.ts index af5a29903d..ab4d241679 100644 --- a/test/acceptance/fake-server.ts +++ b/test/acceptance/fake-server.ts @@ -21,6 +21,8 @@ export type FakeServer = { setDepGraphResponse: (next: Record) => void; setNextResponse: (r: any) => void; setNextStatusCode: (c: number) => void; + setStatusCode: (c: number) => void; + setStatusCodes: (c: number[]) => void; setFeatureFlag: (featureFlag: string, enabled: boolean) => void; unauthorizeAction: (action: string, reason?: string) => void; listen: (port: string | number, callback: () => void) => void; @@ -39,12 +41,17 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { let requests: express.Request[] = []; let featureFlags: Map = featureFlagDefaults(); let unauthorizedActions = new Map(); + // the status code to return for the next request, overriding statusCode let nextStatusCode: number | undefined = undefined; + // the status code to return for all the requests + let statusCode: number | undefined = undefined; + let statusCodes: number[] = []; let nextResponse: any = undefined; let depGraphResponse: Record | undefined = undefined; let server: http.Server | undefined = undefined; const restore = () => { + statusCode = undefined; requests = []; depGraphResponse = undefined; featureFlags = featureFlagDefaults(); @@ -79,6 +86,14 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { nextStatusCode = code; }; + const setStatusCode = (code: number) => { + statusCode = code; + }; + + const setStatusCodes = (codes: number[]) => { + statusCodes = codes; + }; + const setFeatureFlag = (featureFlag: string, enabled: boolean) => { featureFlags.set(featureFlag, enabled); }; @@ -129,7 +144,7 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { if ( req.url?.includes('/iac-org-settings') || req.url?.includes('/cli-config/feature-flags/') || - (!nextResponse && !nextStatusCode) + (!nextResponse && !nextStatusCode && !statusCode) ) { return next(); } @@ -139,7 +154,10 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { const code = nextStatusCode; nextStatusCode = undefined; res.status(code); + } else if (statusCode) { + res.status(statusCode); } + res.send(response); }); @@ -195,6 +213,12 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { return next(); } + const statusCode = statusCodes.shift(); + if (statusCode && statusCode !== 200) { + res.sendStatus(statusCode); + return next(); + } + if (depGraphResponse) { res.send(depGraphResponse); return next(); @@ -574,6 +598,8 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => { setDepGraphResponse, setNextResponse, setNextStatusCode, + setStatusCode, + setStatusCodes, setFeatureFlag, unauthorizeAction, listen, diff --git a/test/jest/acceptance/snyk-fix/fix.spec.ts b/test/jest/acceptance/snyk-fix/fix.spec.ts index 4748c551ff..5b90c39f0e 100644 --- a/test/jest/acceptance/snyk-fix/fix.spec.ts +++ b/test/jest/acceptance/snyk-fix/fix.spec.ts @@ -90,7 +90,7 @@ describe('snyk fix', () => { it('fails when api requests fail', async () => { const project = await createProjectFromWorkspace('no-vulns'); - server.setNextStatusCode(500); + server.setStatusCode(500); const { code, stdout, stderr } = await runSnykCLI( 'fix --org=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', { diff --git a/test/jest/acceptance/snyk-test/retry-mechanism.spec.ts b/test/jest/acceptance/snyk-test/retry-mechanism.spec.ts new file mode 100644 index 0000000000..9e7e7d5618 --- /dev/null +++ b/test/jest/acceptance/snyk-test/retry-mechanism.spec.ts @@ -0,0 +1,60 @@ +import { fakeServer } from '../../../acceptance/fake-server'; +import { createProjectFromWorkspace } from '../../util/createProject'; +import { runSnykCLI } from '../../util/runSnykCLI'; +import { RETRY_ATTEMPTS } from '../../../../src/lib/snyk-test/common'; + +jest.setTimeout(2000 * 60); + +describe('snyk test retry mechanism', () => { + let server; + let env: Record; + + beforeAll((done) => { + const port = process.env.PORT || process.env.SNYK_PORT || '12345'; + const baseApi = '/api/v1'; + env = { + ...process.env, + SNYK_API: 'http://localhost:' + port + baseApi, + SNYK_HOST: 'http://localhost:' + port, + SNYK_TOKEN: '123456789', + SNYK_DISABLE_ANALYTICS: '1', + }; + server = fakeServer(baseApi, env.SNYK_TOKEN); + server.listen(port, () => { + done(); + }); + }); + + afterAll((done) => { + server.close(() => { + done(); + }); + }); + + test('run `snyk test` on an arbitrary cocoapods project and expect retries in case of failures', async () => { + const project = await createProjectFromWorkspace('cocoapods-app'); + const statuses = Array(RETRY_ATTEMPTS - 1) + .fill(500) + .concat([200]); + server.setStatusCodes(statuses); + + const { code } = await runSnykCLI('test', { + cwd: project.path(), + env, + }); + + expect(code).toEqual(0); + }); + + test('run `snyk test` on an arbitrary cocoapods project and expect failure when the retry budget is consumed', async () => { + const project = await createProjectFromWorkspace('cocoapods-app'); + server.setStatusCodes(Array(RETRY_ATTEMPTS).fill(500)); + + const { code } = await runSnykCLI('test', { + cwd: project.path(), + env, + }); + + expect(code).toEqual(2); + }); +}); diff --git a/test/tap/cli-test.acceptance.test.ts b/test/tap/cli-test.acceptance.test.ts index de6ccdd2ca..0340727765 100644 --- a/test/tap/cli-test.acceptance.test.ts +++ b/test/tap/cli-test.acceptance.test.ts @@ -113,6 +113,7 @@ test(GenericTests.language, async (t) => { { chdirWorkspaces }, ), ); + server.restore(); } }); @@ -125,6 +126,7 @@ test(AllProjectsTests.language, async (t) => { { chdirWorkspaces }, ), ); + server.restore(); } }); diff --git a/test/tap/cli-test/cli-test.generic.spec.ts b/test/tap/cli-test/cli-test.generic.spec.ts index a27efb4464..60d10b2818 100644 --- a/test/tap/cli-test/cli-test.generic.spec.ts +++ b/test/tap/cli-test/cli-test.generic.spec.ts @@ -405,7 +405,7 @@ export const GenericTests: AcceptanceTests = { 'error 500 handling': (params, utils) => async (t) => { utils.chdirWorkspaces(); - params.server.setNextStatusCode(500); + params.server.setStatusCode(500); try { await params.cli.test('ruby-app-thresholds'); diff --git a/test/tap/remote-package.test.ts b/test/tap/remote-package.test.ts index a0c63e9cf7..916e70f8b8 100644 --- a/test/tap/remote-package.test.ts +++ b/test/tap/remote-package.test.ts @@ -187,7 +187,7 @@ test('test for existing remote package with dev-deps only', async (t) => { test('test for non-existing', async (t) => { try { - server.setNextStatusCode(500); + server.setStatusCode(500); const res = await cli.test('@123'); t.fail('should fail, instead received ' + res); } catch (error) {