From f98fb75b803ec4fac0e859edf7f95b59f6adb5ee Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Mon, 20 Sep 2021 11:51:47 +0100 Subject: [PATCH 1/4] Fix token used for marking job as processed --- src/api-client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api-client.ts b/src/api-client.ts index 9f1186d..5bdc056 100644 --- a/src/api-client.ts +++ b/src/api-client.ts @@ -78,7 +78,7 @@ export class ApiClient { async markJobAsProcessed(): Promise { const markAsProcessedURL = `/update_jobs/${this.params.jobId}/mark_as_processed` const res = await this.client.get(markAsProcessedURL, { - headers: {Authorization: this.params.credentialsToken} + headers: {Authorization: this.params.jobToken} }) if (res.status !== 200) { throw new Error(`Unexpected status code: ${res.status}`) From 67ff543ce0fd467d3a7e17b076b720082ffab397 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Mon, 20 Sep 2021 12:21:17 +0100 Subject: [PATCH 2/4] Fix error detail returned to the API --- __tests__/main.test.ts | 16 ++++++++++++---- src/api-client.ts | 38 +++++++++++++++++++++++++------------- src/main.ts | 4 +++- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/__tests__/main.test.ts b/__tests__/main.test.ts index b9ef95e..eec35ca 100644 --- a/__tests__/main.test.ts +++ b/__tests__/main.test.ts @@ -139,7 +139,9 @@ describe('run', () => { expect(reportJobErrorSpy).toHaveBeenCalledWith({ 'error-type': 'actions_workflow_unknown', - 'error-detail': 'error getting job details' + 'error-details': { + 'action-error': 'error getting job details' + } }) expect(markJobAsProcessedSpy).toHaveBeenCalled() }) @@ -173,7 +175,9 @@ describe('run', () => { expect(reportJobErrorSpy).toHaveBeenCalledWith({ 'error-type': 'actions_workflow_unknown', - 'error-detail': 'error getting credentials' + 'error-details': { + 'action-error': 'error getting credentials' + } }) expect(markJobAsProcessedSpy).toHaveBeenCalled() }) @@ -207,7 +211,9 @@ describe('run', () => { expect(reportJobErrorSpy).toHaveBeenCalledWith({ 'error-type': 'actions_workflow_image', - 'error-detail': 'error pulling an image' + 'error-details': { + 'action-error': 'error pulling an image' + } }) expect(markJobAsProcessedSpy).toHaveBeenCalled() }) @@ -241,7 +247,9 @@ describe('run', () => { expect(reportJobErrorSpy).toHaveBeenCalledWith({ 'error-type': 'actions_workflow_updater', - 'error-detail': 'error running the update' + 'error-details': { + 'action-error': 'error running the update' + } }) expect(markJobAsProcessedSpy).toHaveBeenCalled() }) diff --git a/src/api-client.ts b/src/api-client.ts index 5bdc056..fbd7878 100644 --- a/src/api-client.ts +++ b/src/api-client.ts @@ -22,7 +22,9 @@ export type JobDetails = { export type JobError = { 'error-type': string - 'error-detail': any + 'error-details': { + 'action-error': string + } } export type Credential = { @@ -39,6 +41,12 @@ export class ApiClient { readonly params: JobParameters ) {} + // We use a static unknown SHA when making a job as complete from the action + // to remain in parity with the existing runner. + UnknownSha = { + 'base-commit-sha': 'unknown' + } + async getJobDetails(): Promise { const detailsURL = `/update_jobs/${this.params.jobId}/details` const res = await this.client.get(detailsURL, { @@ -65,25 +73,29 @@ export class ApiClient { async reportJobError(error: JobError): Promise { const recordErrorURL = `/update_jobs/${this.params.jobId}/record_update_job_error` - const res = await this.client.post(recordErrorURL, error, { - headers: {Authorization: this.params.jobToken} - }) - if (res.status !== 200) { + const res = await this.client.post( + recordErrorURL, + {data: error}, + { + headers: {Authorization: this.params.jobToken} + } + ) + if (res.status !== 204) { throw new Error(`Unexpected status code: ${res.status}`) } - - return res.data.data.attributes } async markJobAsProcessed(): Promise { const markAsProcessedURL = `/update_jobs/${this.params.jobId}/mark_as_processed` - const res = await this.client.get(markAsProcessedURL, { - headers: {Authorization: this.params.jobToken} - }) - if (res.status !== 200) { + const res = await this.client.patch( + markAsProcessedURL, + {data: this.UnknownSha}, + { + headers: {Authorization: this.params.jobToken} + } + ) + if (res.status !== 204) { throw new Error(`Unexpected status code: ${res.status}`) } - - return res.data.data.attributes } } diff --git a/src/main.ts b/src/main.ts index 62e6693..f8d339a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -92,7 +92,9 @@ async function failJob( ): Promise { await apiClient.reportJobError({ 'error-type': errorType, - 'error-detail': error.message + 'error-details': { + 'action-error': error.message + } }) await apiClient.markJobAsProcessed() core.setFailed(error.message) From c3afd33adae518ecadf116c5fc415615c7af0966 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Mon, 20 Sep 2021 18:45:15 +0100 Subject: [PATCH 3/4] ContainerService raises if the container process fails --- __tests__/container-service.test.ts | 38 +++++++++++++++++++++-------- src/container-service.ts | 14 +++++++++-- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/__tests__/container-service.test.ts b/__tests__/container-service.test.ts index 64bdc38..5b0782f 100644 --- a/__tests__/container-service.test.ts +++ b/__tests__/container-service.test.ts @@ -7,18 +7,36 @@ describe('ContainerService', () => { const docker = new Docker() let container: any - beforeAll(async () => { - await ImageService.pull('alpine') - container = await docker.createContainer({ - Image: 'alpine', - AttachStdout: true, - AttachStderr: true, - Cmd: ['/bin/sh', '-c', 'echo $VAR'], - Env: ['VAR=env-var'] + describe('when a container runs successfully', () => { + beforeEach(async () => { + await ImageService.pull('alpine') + container = await docker.createContainer({ + Image: 'alpine', + AttachStdout: true, + AttachStderr: true, + Cmd: ['/bin/sh', '-c', 'echo $VAR'], + Env: ['VAR=env-var'] + }) + }) + + test('it returns true', async () => { + expect(await ContainerService.run(container)).toBe(true) }) }) - test('runs containers', async () => { - await ContainerService.run(container) + describe('when a container runs unsuccessfully', () => { + beforeEach(async () => { + await ImageService.pull('alpine') + container = await docker.createContainer({ + Image: 'alpine', + AttachStdout: true, + AttachStderr: true, + Cmd: ['/bin/sh', '-c'] + }) + }) + + test('raises an exception', async () => { + await expect(ContainerService.run(container)).rejects.toThrow() + }) }) }) diff --git a/src/container-service.ts b/src/container-service.ts index 9984e93..b6bf4c1 100644 --- a/src/container-service.ts +++ b/src/container-service.ts @@ -4,6 +4,8 @@ import {pack} from 'tar-stream' import {FileFetcherInput, FileUpdaterInput, ProxyConfig} from './config-types' import {outStream, errStream} from './utils' +class ContainerRuntimeError extends Error {} + export const ContainerService = { async storeInput( name: string, @@ -29,7 +31,7 @@ export const ContainerService = { await container.putArchive(tar, {path}) }, - async run(container: Container): Promise { + async run(container: Container): Promise { try { const stream = await container.attach({ stream: true, @@ -43,7 +45,15 @@ export const ContainerService = { ) await container.start() - await container.wait() + const outcome = await container.wait() + + if (outcome.StatusCode === 0) { + return true + } else { + throw new ContainerRuntimeError( + `Failure running container ${container.id}` + ) + } } finally { await container.remove() core.info(`Cleaned up container ${container.id}`) From 52968720c5cf76d34018af491058e165a0da4715 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Mon, 20 Sep 2021 21:01:00 +0100 Subject: [PATCH 4/4] Updater now raises on container failures or empty output Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- __fixtures__/output/empty/.keep | 0 __fixtures__/output/happy_path/output.json | 1 + __fixtures__/output/malformed/output.json | 1 + __tests__/container-service.test.ts | 2 +- __tests__/updater.test.ts | 182 +++++++++++++++++++-- src/api-client.ts | 2 +- src/updater.ts | 53 +++--- 7 files changed, 194 insertions(+), 47 deletions(-) create mode 100644 __fixtures__/output/empty/.keep create mode 100644 __fixtures__/output/happy_path/output.json create mode 100644 __fixtures__/output/malformed/output.json diff --git a/__fixtures__/output/empty/.keep b/__fixtures__/output/empty/.keep new file mode 100644 index 0000000..e69de29 diff --git a/__fixtures__/output/happy_path/output.json b/__fixtures__/output/happy_path/output.json new file mode 100644 index 0000000..50e114e --- /dev/null +++ b/__fixtures__/output/happy_path/output.json @@ -0,0 +1 @@ +{"base64_dependency_files":[{"name":"go.mod","content":"bW9kdWxlIGdpdGh1Yi5jb20vZGVwZW5kYWJvdC92Z290ZXN0CgpnbyAxLjEy\nCgpyZXF1aXJlIHJzYy5pby9xciB2MC4xLjAKCg==\n","directory":"/","type":"file","support_file":false,"content_encoding":"utf-8","deleted":false,"operation":"update"},{"name":"go.sum","content":"cnNjLmlvL3FyIHYwLjEuMCBoMTpNL3NBeHNVMko1bWxRNFc4NEJ4Z2EyRWdk\nUXFPYUFsaWlwY2pQbU1VTTVRPQpyc2MuaW8vcXIgdjAuMS4wL2dvLm1vZCBo\nMTpJRit1WmprYjlmcXllRi80dGxCb3lucW1ReFVvUGZXRUtoOTIxY29PdVhz\nPQo=\n","directory":"/","type":"file","support_file":false,"content_encoding":"utf-8","deleted":false,"operation":"update"}],"base_commit_sha":"818a3756444cb0d997a1e11820563105db1c24c4"} \ No newline at end of file diff --git a/__fixtures__/output/malformed/output.json b/__fixtures__/output/malformed/output.json new file mode 100644 index 0000000..ea9e681 --- /dev/null +++ b/__fixtures__/output/malformed/output.json @@ -0,0 +1 @@ +C'est ne pas un json diff --git a/__tests__/container-service.test.ts b/__tests__/container-service.test.ts index 5b0782f..698ad41 100644 --- a/__tests__/container-service.test.ts +++ b/__tests__/container-service.test.ts @@ -31,7 +31,7 @@ describe('ContainerService', () => { Image: 'alpine', AttachStdout: true, AttachStderr: true, - Cmd: ['/bin/sh', '-c'] + Cmd: ['/bin/sh', '-c', 'nosuchccommand'] }) }) diff --git a/__tests__/updater.test.ts b/__tests__/updater.test.ts index 591e312..98fe9a8 100644 --- a/__tests__/updater.test.ts +++ b/__tests__/updater.test.ts @@ -1,5 +1,12 @@ -import {UPDATER_IMAGE_NAME, PROXY_IMAGE_NAME} from '../src/main' import {Updater} from '../src/updater' +import Docker from 'dockerode' +import {ContainerService} from '../src/container-service' +import {ProxyBuilder} from '../src/proxy' + +// We do not need to build actual containers or run updates for this test.) +jest.mock('dockerode') +jest.mock('../src/container-service') +jest.mock('../src/proxy') describe('Updater', () => { const mockApiClient: any = { @@ -7,9 +14,9 @@ describe('Updater', () => { getCredentials: jest.fn(), params: { jobId: 1, - jobToken: process.env.JOB_TOKEN, - credentialsToken: process.env.CREDENTIALS_TOKEN, - dependabotApiUrl: 'http://host.docker.internal:3001' + jobToken: 'job-token', + credentialsToken: 'job-credentials-token', + dependabotApiUrl: 'http://localhost:3001' } } @@ -23,18 +30,163 @@ describe('Updater', () => { 'package-manager': 'npm-and-yarn' } - const updater = new Updater( - UPDATER_IMAGE_NAME, - PROXY_IMAGE_NAME, - mockApiClient, - mockJobDetails, - [] - ) + const mockProxy: any = { + container: { + start: jest.fn() + }, + network: jest.fn(), + networkName: 'mockNetworkName', + url: 'http://localhost', + cert: 'mockCertificate', + shutdown: jest.fn() + } + + const mockContainer: any = { + id: 1 + } + + afterEach(async () => { + jest.clearAllMocks() // Reset any mocked classes + }) + + describe('when there is a happy path update', () => { + const updater = new Updater( + 'MOCK_UPDATER_IMAGE_NAME', + 'MOCK_PROXY_IMAGE_NAME', + mockApiClient, + mockJobDetails, + [], + '__fixtures__/output/happy_path' + ) + + beforeEach(async () => { + jest + .spyOn(Docker.prototype, 'createContainer') + .mockResolvedValue(mockContainer) + + jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy) + jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn()) + }) + + it('should be successful', async () => { + expect(await updater.runUpdater()).toBe(true) + }) + }) + + describe('when the file fetch container fails', () => { + const updater = new Updater( + 'MOCK_UPDATER_IMAGE_NAME', + 'MOCK_PROXY_IMAGE_NAME', + mockApiClient, + mockJobDetails, + [], + '__fixtures__/output/happy_path' + ) + + beforeEach(async () => { + jest + .spyOn(Docker.prototype, 'createContainer') + .mockResolvedValue(mockContainer) + + jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy) + + jest + .spyOn(ContainerService, 'run') + .mockImplementationOnce( + jest.fn(async () => + Promise.reject(new Error('First call to container service errored')) + ) + ) + }) + + it('should raise an error', async () => { + await expect(updater.runUpdater()).rejects.toThrow() + }) + }) + + describe('when file updater container fails', () => { + const updater = new Updater( + 'MOCK_UPDATER_IMAGE_NAME', + 'MOCK_PROXY_IMAGE_NAME', + mockApiClient, + mockJobDetails, + [], + '__fixtures__/output/happy_path' + ) + + beforeEach(async () => { + jest + .spyOn(Docker.prototype, 'createContainer') + .mockResolvedValue(mockContainer) + + jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy) + + jest + .spyOn(ContainerService, 'run') + .mockImplementationOnce(jest.fn()) + .mockImplementationOnce( + jest.fn(async () => + Promise.reject( + new Error('Second call to container service errored') + ) + ) + ) + }) + + it('should raise an error', async () => { + await expect(updater.runUpdater()).rejects.toThrow() + }) + }) + + describe('when the file fetch step results in empty output', () => { + const updater = new Updater( + 'MOCK_UPDATER_IMAGE_NAME', + 'MOCK_PROXY_IMAGE_NAME', + mockApiClient, + mockJobDetails, + [], + '__fixtures__/output/empty' + ) + + beforeEach(async () => { + jest + .spyOn(Docker.prototype, 'createContainer') + .mockResolvedValue(mockContainer) + + jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy) + + jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn()) + }) + + it('should raise an error', async () => { + await expect(updater.runUpdater()).rejects.toThrow( + new Error('No output.json created by the fetcher container') + ) + }) + }) + + describe('when the file fetch step results in malformed output', () => { + const updater = new Updater( + 'MOCK_UPDATER_IMAGE_NAME', + 'MOCK_PROXY_IMAGE_NAME', + mockApiClient, + mockJobDetails, + [], + '__fixtures__/output/malformed' + ) + + beforeEach(async () => { + jest + .spyOn(Docker.prototype, 'createContainer') + .mockResolvedValue(mockContainer) + + jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy) + + jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn()) + }) - it('should fetch job details', async () => { - mockApiClient.getJobDetails.mockImplementation(() => { - throw new Error('kaboom') + it('should raise an error', async () => { + await expect(updater.runUpdater()).rejects.toThrow() }) - updater.runUpdater() }) }) diff --git a/src/api-client.ts b/src/api-client.ts index fbd7878..1f639ee 100644 --- a/src/api-client.ts +++ b/src/api-client.ts @@ -41,7 +41,7 @@ export class ApiClient { readonly params: JobParameters ) {} - // We use a static unknown SHA when making a job as complete from the action + // We use a static unknown SHA when marking a job as complete from the action // to remain in parity with the existing runner. UnknownSha = { 'base-commit-sha': 'unknown' diff --git a/src/updater.ts b/src/updater.ts index ec40007..c1e8553 100644 --- a/src/updater.ts +++ b/src/updater.ts @@ -24,7 +24,8 @@ export class Updater { private readonly proxyImage: string, private readonly apiClient: ApiClient, private readonly details: JobDetails, - private readonly credentials: Credential[] + private readonly credentials: Credential[], + private readonly outputFolder = 'output/' ) { this.docker = new Docker() } @@ -32,37 +33,24 @@ export class Updater { /** * Execute an update job and report the result to Dependabot API. */ - async runUpdater(): Promise { + async runUpdater(): Promise { + const proxy = await new ProxyBuilder(this.docker, this.proxyImage).run( + this.details, + this.credentials + ) + proxy.container.start() + try { - const proxy = await new ProxyBuilder(this.docker, this.proxyImage).run( - this.details, - this.credentials - ) - proxy.container.start() - - try { - const files = await this.runFileFetcher(proxy) - if (!files) { - core.error(`failed during fetch, skipping updater`) - // TODO: report job runner_error? - return - } - - await this.runFileUpdater(proxy, files) - } catch (e) { - // TODO: report job runner_error? - core.error(`Error ${e}`) - } finally { - await proxy.shutdown() - await this.docker.pruneNetworks() - } - } catch (e) { - // TODO: report job runner_error? - core.error(`Error ${e}`) + const files = await this.runFileFetcher(proxy) + await this.runFileUpdater(proxy, files) + return true + } finally { + await proxy.shutdown() + await this.docker.pruneNetworks() } } - private async runFileFetcher(proxy: Proxy): Promise { + private async runFileFetcher(proxy: Proxy): Promise { const container = await this.createContainer(proxy, 'fetch_files') await ContainerService.storeInput( JOB_INPUT_FILENAME, @@ -79,9 +67,14 @@ export class Updater { await ContainerService.run(container) - const outputPath = path.join(__dirname, '../output/output.json') + const outputPath = path.join( + __dirname, + '../', + this.outputFolder, + 'output.json' + ) if (!fs.existsSync(outputPath)) { - return + throw new Error('No output.json created by the fetcher container') } const fileFetcherSync = fs.readFileSync(outputPath).toString()