Merge pull request #87 from github/brrygrdn/test-docker-response

Fix the relaying of container errors to Dependabot API

__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"}

__fixtures__/output/malformed/output.json

@@ -0,0 +1 @@
+C'est ne pas un json

__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', 'nosuchccommand']
+      })
+    })
+
+    test('raises an exception', async () => {
+      await expect(ContainerService.run(container)).rejects.toThrow()
+    })
   })
 })

__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()
     })

__tests__/updater.test.ts

@@ -1,15 +1,22 @@
-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 = {
     getJobDetails: jest.fn(),
     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()
   })
 })

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 marking a job as complete from the action
+  // to remain in parity with the existing runner.
+  UnknownSha = {
+    'base-commit-sha': 'unknown'
+  }
+
   async getJobDetails(): Promise<JobDetails> {
     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<void> {
     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<void> {
     const markAsProcessedURL = `/update_jobs/${this.params.jobId}/mark_as_processed`
-    const res = await this.client.get(markAsProcessedURL, {
-      headers: {Authorization: this.params.credentialsToken}
-    })
-    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
   }
 }

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<void> {
+  async run(container: Container): Promise<boolean> {
     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}`)