add retries and fix up tests

2024-08-23 13:17:07 +01:00
parent 72b670f356
commit 1b9faf628d
7 changed files with 654 additions and 419 deletions
@@ -1,14 +1,12 @@
-import {
-  uploadOCIImageManifest,
-  uploadOCIIndexManifest
-  // uploadOCIIndexManifest
-} from '../src/ghcr-client'
+import { Client } from '../src/ghcr-client'
 import * as ociContainer from '../src/oci-container'
 import * as crypto from 'crypto'

 // Mocks
 let fetchMock: jest.SpyInstance

+let client: Client
+
 const token = 'test-token'
 const registry = new URL('https://ghcr.io')
 const repository = 'test-org/test-repo'
@@ -156,22 +154,75 @@ type MethodHandlers = {
  putBlobMock?: (url: string, options: { method: string }) => object
 }

+type ForcedRetries = {
+  checkBlob: number
+  initiateBlobUpload: number
+  putBlob: number
+  putManifest: number
+}
+
 function configureFetchMock(
  fetchMockInstance: jest.SpyInstance,
-  methodHandlers: MethodHandlers
+  methodHandlers: MethodHandlers,
+  forcedRetries: ForcedRetries = {
+    checkBlob: 0,
+    initiateBlobUpload: 0,
+    putBlob: 0,
+    putManifest: 0
+  }
 ): void {
+  const retriableError = async (retries: number): Promise<object> => {
+    if (retries % 2 === 0) {
+      throw new Error('Network Error')
+    } else {
+      return {
+        status: 429,
+        statusText: 'Too Many Requests',
+        headers: {
+          get: (header: string) => {
+            if (header === 'retry-after') {
+              return '0.1'
+            }
+          }
+        }
+      }
+    }
+  }
+
  fetchMockInstance.mockImplementation(
    async (url: string, options: { method: string }) => {
+      // Simulate retries for every request until the number of forced retries is exhausted.
+      // We'll simulate both failing status codes and network errors for full coverage.
      validateRequestConfig(url, options)
      switch (options.method) {
        case 'HEAD':
+          if (forcedRetries.checkBlob > 0) {
+            forcedRetries.checkBlob--
+            return retriableError(forcedRetries.checkBlob)
+          }
+
          return methodHandlers.checkBlobMock?.(url, options)
        case 'POST':
+          if (forcedRetries.initiateBlobUpload > 0) {
+            forcedRetries.initiateBlobUpload--
+            return retriableError(forcedRetries.initiateBlobUpload)
+          }
+
          return methodHandlers.initiateBlobUploadMock?.(url, options)
        case 'PUT':
          if (url.includes('manifest')) {
+            if (forcedRetries.putManifest > 0) {
+              forcedRetries.putManifest--
+              return retriableError(forcedRetries.putManifest)
+            }
+
            return methodHandlers.putManifestMock?.(url, options)
          } else {
+            if (forcedRetries.putBlob > 0) {
+              forcedRetries.putBlob--
+              return retriableError(forcedRetries.putBlob)
+            }
+
            return methodHandlers.putBlobMock?.(url, options)
          }
      }
@@ -183,6 +234,11 @@ describe('uploadOCIIndexManifest', () => {
  beforeEach(() => {
    jest.clearAllMocks()
    fetchMock = jest.spyOn(global, 'fetch').mockImplementation()
+
+    client = new Client(token, registry, {
+      retries: 5,
+      backoff: 1
+    })
  })

  it('uploads the tagged manifest with the appropriate tag', async () => {
@@ -193,7 +249,7 @@ describe('uploadOCIIndexManifest', () => {
      putManifestMock: putManifestSuccessful(sha, tag)
    })

-    await uploadOCIIndexManifest(token, registry, repository, manifest, tag)
+    await client.uploadOCIIndexManifest(repository, manifest, tag)

    expect(fetchMock).toHaveBeenCalledTimes(1)
    expect(
@@ -212,24 +268,26 @@ describe('uploadOCIIndexManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(
      'Unexpected 400 Bad Request response from manifest upload. Errors: BAD_REQUEST - tag already exists.'
    )
  })

  it('throws an error if the returned digest does not match the precalculated one', async () => {
-    const { manifest, sha, blobs } = testImageManifest()
+    const { manifest, sha } = testIndexManifest()
+
+    const tag = 'sha-1234'

    configureFetchMock(fetchMock, {
      checkBlobMock: checkBlobAllExistingBlobs,
      initiateBlobUploadMock: initiateBlobUploadSuccessForAllBlobs,
      putBlobMock: putBlobSuccess,
-      putManifestMock: putManifestSuccessful('some-garbage-digest', sha)
+      putManifestMock: putManifestSuccessful('some-garbage-digest', tag)
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIIndexManifest(repository, manifest, tag)
    ).rejects.toThrow(
      `Digest mismatch. Expected ${sha}, got some-garbage-digest.`
    )
@@ -252,7 +310,7 @@ describe('uploadOCIImageManifest', () => {
      putManifestMock: putManifestSuccessful(sha, sha)
    })

-    await uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+    await client.uploadOCIImageManifest(repository, manifest, blobs)

    expect(fetchMock).toHaveBeenCalledTimes(10)
    expect(
@@ -276,14 +334,7 @@ describe('uploadOCIImageManifest', () => {
      putManifestMock: putManifestSuccessful(sha, semver)
    })

-    await uploadOCIImageManifest(
-      token,
-      registry,
-      repository,
-      manifest,
-      blobs,
-      semver
-    )
+    await client.uploadOCIImageManifest(repository, manifest, blobs, semver)

    expect(fetchMock).toHaveBeenCalledTimes(10)
    expect(
@@ -297,6 +348,40 @@ describe('uploadOCIImageManifest', () => {
    ).toHaveLength(4)
  })

+  it('uploads everything to the provided registry by retrying requests', async () => {
+    const { manifest, sha, blobs } = testImageManifest()
+
+    configureFetchMock(
+      fetchMock,
+      {
+        checkBlobMock: checkBlobNoExistingBlobs,
+        initiateBlobUploadMock: initiateBlobUploadSuccessForAllBlobs,
+        putBlobMock: putBlobSuccess,
+        putManifestMock: putManifestSuccessful(sha, sha)
+      },
+      {
+        checkBlob: 2,
+        initiateBlobUpload: 2,
+        putBlob: 2,
+        putManifest: 2
+      }
+    ) // Fail each request twice before succeeding
+
+    await client.uploadOCIImageManifest(repository, manifest, blobs)
+
+    // 8 Additional requests - 2 for each of the 4 failed request types
+    expect(fetchMock).toHaveBeenCalledTimes(18)
+    expect(
+      fetchMock.mock.calls.filter(call => call[1].method === 'HEAD')
+    ).toHaveLength(5)
+    expect(
+      fetchMock.mock.calls.filter(call => call[1].method === 'POST')
+    ).toHaveLength(5)
+    expect(
+      fetchMock.mock.calls.filter(call => call[1].method === 'PUT')
+    ).toHaveLength(8)
+  })
+
  it('skips blob uploads if all blobs already exist', async () => {
    const { manifest, sha, blobs } = testImageManifest()

@@ -307,7 +392,7 @@ describe('uploadOCIImageManifest', () => {
      putManifestMock: putManifestSuccessful(sha, sha)
    })

-    await uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+    await client.uploadOCIImageManifest(repository, manifest, blobs)

    expect(fetchMock).toHaveBeenCalledTimes(4)
    expect(
@@ -331,7 +416,7 @@ describe('uploadOCIImageManifest', () => {
      putManifestMock: putManifestSuccessful(sha, sha)
    })

-    await uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+    await client.uploadOCIImageManifest(repository, manifest, blobs)

    expect(fetchMock).toHaveBeenCalledTimes(8)
    expect(
@@ -356,12 +441,31 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(
      /^Unexpected 503 Service Unavailable response from check blob/
    )
  })

+  it('throws an error if a blob file is not provided', async () => {
+    const { manifest, sha } = testImageManifest()
+
+    configureFetchMock(fetchMock, {
+      checkBlobMock: checkBlobNoExistingBlobs,
+      initiateBlobUploadMock: initiateBlobUploadSuccessForAllBlobs,
+      putBlobMock: putBlobSuccess,
+      putManifestMock: putManifestSuccessful(sha, sha)
+    })
+
+    await expect(
+      client.uploadOCIImageManifest(
+        repository,
+        manifest,
+        new Map<string, Buffer>()
+      )
+    ).rejects.toThrow(/^Blob for layer sha256:[a-zA-Z0-9]+ not found/)
+  })
+
  it('throws an error if initiating layer upload fails', async () => {
    const { manifest, sha, blobs } = testImageManifest()

@@ -373,7 +477,7 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(
      'Unexpected 503 Service Unavailable response from initiate layer upload. Response Body: 503 Service Unavailable.'
    )
@@ -390,7 +494,7 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(/^No location header in response from upload post/)
  })

@@ -405,7 +509,7 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(/^Unexpected 400 Bad Request response from layer/)
  })

@@ -420,7 +524,7 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(
      'Unexpected 400 Bad Request response from manifest upload. Errors: BAD_REQUEST - tag already exists.'
    )
@@ -437,7 +541,7 @@ describe('uploadOCIImageManifest', () => {
    })

    await expect(
-      uploadOCIImageManifest(token, registry, repository, manifest, blobs)
+      client.uploadOCIImageManifest(repository, manifest, blobs)
    ).rejects.toThrow(
      `Digest mismatch. Expected ${sha}, got some-garbage-digest.`
    )
@@ -31,6 +31,9 @@ let readFileContentsMock: jest.SpyInstance
 let calculateManifestDigestMock: jest.SpyInstance

 // Mock GHCR client
+let client: ghcr.Client
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+let createGHCRClient: jest.SpyInstance
 let uploadOCIImageManifestMock: jest.SpyInstance
 let uploadOCIIndexManifestMock: jest.SpyInstance

@@ -44,6 +47,8 @@ describe('run', () => {
  beforeEach(() => {
    jest.clearAllMocks()

+    client = new ghcr.Client('token', ghcrUrl)
+
    // Core mocks
    setFailedMock = jest.spyOn(core, 'setFailed').mockImplementation()
    setOutputMock = jest.spyOn(core, 'setOutput').mockImplementation()
@@ -71,11 +76,15 @@ describe('run', () => {
      .mockImplementation()

    // GHCR Client mocks
+    createGHCRClient = jest
+      .spyOn(ghcr, 'Client')
+      .mockImplementation(() => client)
+
    uploadOCIImageManifestMock = jest
-      .spyOn(ghcr, 'uploadOCIImageManifest')
+      .spyOn(client, 'uploadOCIImageManifest')
      .mockImplementation()
    uploadOCIIndexManifestMock = jest
-      .spyOn(ghcr, 'uploadOCIIndexManifest')
+      .spyOn(client, 'uploadOCIIndexManifest')
      .mockImplementation()

    // Config mocks
@@ -428,7 +437,7 @@ describe('run', () => {
    })

    uploadOCIImageManifestMock.mockImplementation(
-      (token, registry, repo, manifest, blobs, tag) => {
+      (repo, manifest, blobs, tag) => {
        if (tag === undefined) {
          return 'attestation-digest'
        } else {
@@ -485,9 +494,7 @@ describe('run', () => {
    })

    uploadOCIImageManifestMock.mockImplementation(
-      (token, registry, repository, manifest, blobs, tag) => {
-        expect(token).toBe(options.token)
-        expect(registry).toBe(options.containerRegistryUrl)
+      (repository, manifest, blobs, tag) => {
        expect(repository).toBe(options.nameWithOwner)
        expect(tag).toBe('1.2.3')
        expect(blobs.size).toBe(3)
@@ -572,9 +579,7 @@ describe('run', () => {
    })

    uploadOCIIndexManifestMock.mockImplementation(
-      async (token, registry, repository, manifest, tag) => {
-        expect(token).toBe(options.token)
-        expect(registry).toBe(options.containerRegistryUrl)
+      async (repository, manifest, tag) => {
        expect(repository).toBe(options.nameWithOwner)
        expect(tag).toBe('sha256-my-test-digest')
        expect(manifest.mediaType).toBe(ociContainer.imageIndexMediaType)
@@ -583,7 +588,7 @@ describe('run', () => {
    )

    uploadOCIImageManifestMock.mockImplementation(
-      (token, registry, repository, manifest, blobs, tag) => {
+      (repository, manifest, blobs, tag) => {
        let expectedBlobKeys: string[] = []
        let expectedAnnotationValue = ''
        let expectedTagValue: string | undefined = undefined
@@ -606,8 +611,6 @@ describe('run', () => {
          returnValue = 'sha256:my-test-digest'
        }

-        expect(token).toBe(options.token)
-        expect(registry).toBe(options.containerRegistryUrl)
        expect(repository).toBe(options.nameWithOwner)
        expect(manifest.mediaType).toBe(ociContainer.imageManifestMediaType)
        expect(manifest.annotations['com.github.package.type']).toBe(
@@ -76,6 +76,13 @@ describe('createActionPackageManifest', () => {
    const manifestJSON = JSON.stringify(manifest)
    expect(manifestJSON).toEqual(expectedJSON.replace(/\s/g, ''))
  })
+
+  it('uses the current time if no created date is provided', () => {
+    const { manifest } = testActionPackageManifest(false)
+    expect(
+      manifest.annotations['org.opencontainers.image.created']
+    ).toBeDefined()
+  })
 })

 describe('createSigstoreAttestationManifest', () => {
@@ -116,6 +123,13 @@ describe('createSigstoreAttestationManifest', () => {

    expect(manifestJSON).toEqual(expectedJSON.replace(/\s/g, ''))
  })
+
+  it('uses the current time if no created date is provided', () => {
+    const manifest = testAttestationManifest(false)
+    expect(
+      manifest.annotations['org.opencontainers.image.created']
+    ).toBeDefined()
+  })
 })

 describe('createReferrerIndexManifest', () => {
@@ -151,9 +165,16 @@ describe('createReferrerIndexManifest', () => {

    expect(manifestJSON).toEqual(expectedJSON.replace(/\s/g, ''))
  })
+
+  it('uses the current time if no created date is provided', () => {
+    const manifest = testReferrerIndexManifest(false)
+    expect(
+      manifest.annotations['org.opencontainers.image.created']
+    ).toBeDefined()
+  })
 })

-function testActionPackageManifest(): {
+function testActionPackageManifest(setCreated = true): {
  manifest: OCIImageManifest
  tarFile: FileMetadata
  zipFile: FileMetadata
@@ -183,7 +204,7 @@ function testActionPackageManifest(): {
    ownerId,
    sourceCommit,
    version,
-    date
+    setCreated ? date : undefined
  )

  return {
@@ -193,21 +214,23 @@ function testActionPackageManifest(): {
  }
 }

-function testAttestationManifest(): OCIImageManifest {
+function testAttestationManifest(setCreated = true): OCIImageManifest {
+  const date = new Date(createdTimestamp)
  return createSigstoreAttestationManifest(
    10,
    'bundleDigest',
    100,
    'subjectDigest',
-    new Date(createdTimestamp)
+    setCreated ? date : undefined
  )
 }

-function testReferrerIndexManifest(): OCIIndexManifest {
+function testReferrerIndexManifest(setCreated = true): OCIIndexManifest {
+  const date = new Date(createdTimestamp)
  return createReferrerTagManifest(
    'attDigest',
    100,
-    new Date(createdTimestamp),
-    new Date(createdTimestamp)
+    date,
+    setCreated ? date : undefined
  )
 }