fix(assets): parallel docker image publishing fails on macOS (#20117)

misterjoshua · web-flow · commit a58a8037b796 · 2022-05-18T14:44:13.000Z
Changes container image publishing so that publishing doesn't re-run docker logins for the same repository and so logins are run one at a time. Fixes #20116 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
diff --git a/packages/cdk-assets/lib/private/asset-handler.ts b/packages/cdk-assets/lib/private/asset-handler.ts
@@ -1,5 +1,6 @@
 import { IAws } from '../aws';
 import { EventType } from '../progress';
+import { DockerFactory } from './docker';
 
 export interface IAssetHandler {
   publish(): Promise<void>;
@@ -8,6 +9,7 @@ export interface IAssetHandler {
 export interface IHandlerHost {
   readonly aws: IAws;
   readonly aborted: boolean;
+  readonly dockerFactory: DockerFactory;
 
   emitMessage(type: EventType, m: string): void;
 }
diff --git a/packages/cdk-assets/lib/private/docker.ts b/packages/cdk-assets/lib/private/docker.ts
@@ -3,6 +3,7 @@ import * as os from 'os';
 import * as path from 'path';
 import { cdkCredentialsConfig, obtainEcrCredentials } from './docker-credentials';
 import { Logger, shell, ShellOptions } from './shell';
+import { createCriticalSection } from './util';
 
 interface BuildOptions {
   readonly directory: string;
@@ -146,6 +147,65 @@ export class Docker {
   }
 }
 
+export interface DockerFactoryOptions {
+  readonly repoUri: string;
+  readonly ecr: AWS.ECR;
+  readonly logger: (m: string) => void;
+}
+
+/**
+ * Helps get appropriately configured Docker instances during the container
+ * image publishing process.
+ */
+export class DockerFactory {
+  private enterLoggedInDestinationsCriticalSection = createCriticalSection();
+  private loggedInDestinations = new Set<string>();
+
+  /**
+   * Gets a Docker instance for building images.
+   */
+  public async forBuild(options: DockerFactoryOptions): Promise<Docker> {
+    const docker = new Docker(options.logger);
+
+    // Default behavior is to login before build so that the Dockerfile can reference images in the ECR repo
+    // However, if we're in a pipelines environment (for example),
+    // we may have alternative credentials to the default ones to use for the build itself.
+    // If the special config file is present, delay the login to the default credentials until the push.
+    // If the config file is present, we will configure and use those credentials for the build.
+    let cdkDockerCredentialsConfigured = docker.configureCdkCredentials();
+    if (!cdkDockerCredentialsConfigured) {
+      await this.loginOncePerDestination(docker, options);
+    }
+
+    return docker;
+  }
+
+  /**
+   * Gets a Docker instance for pushing images to ECR.
+   */
+  public async forEcrPush(options: DockerFactoryOptions) {
+    const docker = new Docker(options.logger);
+    await this.loginOncePerDestination(docker, options);
+    return docker;
+  }
+
+  private async loginOncePerDestination(docker: Docker, options: DockerFactoryOptions) {
+    // Changes: 012345678910.dkr.ecr.us-west-2.amazonaws.com/tagging-test
+    // To this: 012345678910.dkr.ecr.us-west-2.amazonaws.com
+    const repositoryDomain = options.repoUri.split('/')[0];
+
+    // Ensure one-at-a-time access to loggedInDestinations.
+    await this.enterLoggedInDestinationsCriticalSection(async () => {
+      if (this.loggedInDestinations.has(repositoryDomain)) {
+        return;
+      }
+
+      await docker.login(options.ecr);
+      this.loggedInDestinations.add(repositoryDomain);
+    });
+  }
+}
+
 function flatten(x: string[][]) {
   return Array.prototype.concat([], ...x);
 }
diff --git a/packages/cdk-assets/lib/private/handlers/container-images.ts b/packages/cdk-assets/lib/private/handlers/container-images.ts
@@ -8,8 +8,6 @@ import { replaceAwsPlaceholders } from '../placeholders';
 import { shell } from '../shell';
 
 export class ContainerImageAssetHandler implements IAssetHandler {
-  private readonly docker = new Docker(m => this.host.emitMessage(EventType.DEBUG, m));
-
   constructor(
     private readonly workDir: string,
     private readonly asset: DockerImageManifestEntry,
@@ -31,32 +29,60 @@ export class ContainerImageAssetHandler implements IAssetHandler {
     if (await this.destinationAlreadyExists(ecr, destination, imageUri)) { return; }
     if (this.host.aborted) { return; }
 
-    // Default behavior is to login before build so that the Dockerfile can reference images in the ECR repo
-    // However, if we're in a pipelines environment (for example),
-    // we may have alternative credentials to the default ones to use for the build itself.
-    // If the special config file is present, delay the login to the default credentials until the push.
-    // If the config file is present, we will configure and use those credentials for the build.
-    let cdkDockerCredentialsConfigured = this.docker.configureCdkCredentials();
-    if (!cdkDockerCredentialsConfigured) { await this.docker.login(ecr); }
+    const containerImageDockerOptions = {
+      repoUri,
+      logger: (m: string) => this.host.emitMessage(EventType.DEBUG, m),
+      ecr,
+    };
+
+    const dockerForBuilding = await this.host.dockerFactory.forBuild(containerImageDockerOptions);
 
-    const localTagName = this.asset.source.executable
-      ? await this.buildExternalAsset(this.asset.source.executable)
-      : await this.buildDirectoryAsset();
+    const builder = new ContainerImageBuilder(dockerForBuilding, this.workDir, this.asset, this.host);
+    const localTagName = await builder.build();
 
     if (localTagName === undefined || this.host.aborted) {
       return;
     }
 
     this.host.emitMessage(EventType.UPLOAD, `Push ${imageUri}`);
     if (this.host.aborted) { return; }
-    await this.docker.tag(localTagName, imageUri);
 
-    if (cdkDockerCredentialsConfigured) {
-      this.docker.resetAuthPlugins();
-      await this.docker.login(ecr);
+    await dockerForBuilding.tag(localTagName, imageUri);
+
+    const dockerForPushing = await this.host.dockerFactory.forEcrPush(containerImageDockerOptions);
+    await dockerForPushing.push(imageUri);
+  }
+
+  /**
+   * Check whether the image already exists in the ECR repo
+   *
+   * Use the fields from the destination to do the actual check. The imageUri
+   * should correspond to that, but is only used to print Docker image location
+   * for user benefit (the format is slightly different).
+   */
+  private async destinationAlreadyExists(ecr: AWS.ECR, destination: DockerImageDestination, imageUri: string): Promise<boolean> {
+    this.host.emitMessage(EventType.CHECK, `Check ${imageUri}`);
+    if (await imageExists(ecr, destination.repositoryName, destination.imageTag)) {
+      this.host.emitMessage(EventType.FOUND, `Found ${imageUri}`);
+      return true;
     }
 
-    await this.docker.push(imageUri);
+    return false;
+  }
+}
+
+class ContainerImageBuilder {
+  constructor(
+    private readonly docker: Docker,
+    private readonly workDir: string,
+    private readonly asset: DockerImageManifestEntry,
+    private readonly host: IHandlerHost) {
+  }
+
+  async build(): Promise<string | undefined> {
+    return this.asset.source.executable
+      ? this.buildExternalAsset(this.asset.source.executable)
+      : this.buildDirectoryAsset();
   }
 
   /**
@@ -84,7 +110,6 @@ export class ContainerImageAssetHandler implements IAssetHandler {
    * and is expected to return the generated image identifier on stdout.
    */
   private async buildExternalAsset(executable: string[], cwd?: string): Promise<string | undefined> {
-
     const assetPath = cwd ?? this.workDir;
 
     this.host.emitMessage(EventType.BUILD, `Building Docker image using command '${executable}'`);
@@ -93,23 +118,6 @@ export class ContainerImageAssetHandler implements IAssetHandler {
     return (await shell(executable, { cwd: assetPath, quiet: true })).trim();
   }
 
-  /**
-   * Check whether the image already exists in the ECR repo
-   *
-   * Use the fields from the destination to do the actual check. The imageUri
-   * should correspond to that, but is only used to print Docker image location
-   * for user benefit (the format is slightly different).
-   */
-  private async destinationAlreadyExists(ecr: AWS.ECR, destination: DockerImageDestination, imageUri: string): Promise<boolean> {
-    this.host.emitMessage(EventType.CHECK, `Check ${imageUri}`);
-    if (await imageExists(ecr, destination.repositoryName, destination.imageTag)) {
-      this.host.emitMessage(EventType.FOUND, `Found ${imageUri}`);
-      return true;
-    }
-
-    return false;
-  }
-
   private async buildImage(localTagName: string): Promise<void> {
     const source = this.asset.source;
     if (!source.directory) {
diff --git a/packages/cdk-assets/lib/private/util.ts b/packages/cdk-assets/lib/private/util.ts
@@ -0,0 +1,12 @@
+/**
+ * Creates a critical section, ensuring that at most one function can
+ * enter the critical section at a time.
+ */
+export function createCriticalSection() {
+  let lock = Promise.resolve();
+  return async (criticalFunction: () => Promise<void>) => {
+    const res = lock.then(() => criticalFunction());
+    lock = res.catch(e => e);
+    return res;
+  };
+};
diff --git a/packages/cdk-assets/lib/publishing.ts b/packages/cdk-assets/lib/publishing.ts
@@ -1,6 +1,7 @@
 import { AssetManifest, IManifestEntry } from './asset-manifest';
 import { IAws } from './aws';
 import { IHandlerHost } from './private/asset-handler';
+import { DockerFactory } from './private/docker';
 import { makeAssetHandler } from './private/handlers';
 import { EventType, IPublishProgress, IPublishProgressListener } from './progress';
 
@@ -76,6 +77,7 @@ export class AssetPublishing implements IPublishProgress {
       aws: this.options.aws,
       get aborted() { return self.aborted; },
       emitMessage(t, m) { self.progressEvent(t, m); },
+      dockerFactory: new DockerFactory(),
     };
   }
 
diff --git a/packages/cdk-assets/test/docker-images.test.ts b/packages/cdk-assets/test/docker-images.test.ts
@@ -36,6 +36,37 @@ beforeEach(() => {
         },
       },
     }),
+    '/multi/cdk.out/assets.json': JSON.stringify({
+      version: Manifest.version(),
+      dockerImages: {
+        theAsset1: {
+          source: {
+            directory: 'dockerdir',
+          },
+          destinations: {
+            theDestination: {
+              region: 'us-north-50',
+              assumeRoleArn: 'arn:aws:role',
+              repositoryName: 'repo',
+              imageTag: 'theAsset1',
+            },
+          },
+        },
+        theAsset2: {
+          source: {
+            directory: 'dockerdir',
+          },
+          destinations: {
+            theDestination: {
+              region: 'us-north-50',
+              assumeRoleArn: 'arn:aws:role',
+              repositoryName: 'repo',
+              imageTag: 'theAsset2',
+            },
+          },
+        },
+      },
+    }),
     '/external/cdk.out/assets.json': JSON.stringify({
       version: Manifest.version(),
       dockerImages: {
@@ -295,3 +326,72 @@ test('when external credentials are present, explicit Docker config directories
 
   expectAllSpawns();
 });
+
+test('logging in only once for two assets', async () => {
+  const pub = new AssetPublishing(AssetManifest.fromPath('/multi/cdk.out'), { aws, throwOnError: false });
+  aws.mockEcr.describeImages = mockedApiFailure('ImageNotFoundException', 'File does not exist');
+  aws.mockEcr.getAuthorizationToken = mockedApiResult({
+    authorizationData: [
+      { authorizationToken: 'dXNlcjpwYXNz', proxyEndpoint: 'https://proxy.com/' },
+    ],
+  });
+
+  const expectAllSpawns = mockSpawn(
+    { commandLine: ['docker', 'login', '--username', 'user', '--password-stdin', 'https://proxy.com/'] },
+    { commandLine: ['docker', 'inspect', 'cdkasset-theasset1'], exitCode: 1 },
+    { commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset1', '.'], cwd: '/multi/cdk.out/dockerdir' },
+    { commandLine: ['docker', 'tag', 'cdkasset-theasset1', '12345.amazonaws.com/repo:theAsset1'] },
+    { commandLine: ['docker', 'push', '12345.amazonaws.com/repo:theAsset1'] },
+    { commandLine: ['docker', 'inspect', 'cdkasset-theasset2'], exitCode: 1 },
+    { commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset2', '.'], cwd: '/multi/cdk.out/dockerdir' },
+    { commandLine: ['docker', 'tag', 'cdkasset-theasset2', '12345.amazonaws.com/repo:theAsset2'] },
+    { commandLine: ['docker', 'push', '12345.amazonaws.com/repo:theAsset2'] },
+  );
+
+  await pub.publish();
+
+  expectAllSpawns();
+  expect(true).toBeTruthy(); // Expect no exception, satisfy linter
+});
+
+test('logging in twice for two repository domains (containing account id & region)', async () => {
+  const pub = new AssetPublishing(AssetManifest.fromPath('/multi/cdk.out'), { aws, throwOnError: false });
+  aws.mockEcr.describeImages = mockedApiFailure('ImageNotFoundException', 'File does not exist');
+
+  let repoIdx = 12345;
+  aws.mockEcr.describeRepositories = jest.fn().mockReturnValue({
+    promise: jest.fn().mockImplementation(() => Promise.resolve({
+      repositories: [
+        // Usually looks like: 012345678910.dkr.ecr.us-west-2.amazonaws.com/aws-cdk/assets
+        { repositoryUri: `${repoIdx++}.amazonaws.com/aws-cdk/assets` },
+      ],
+    })),
+  });
+
+  let proxyIdx = 12345;
+  aws.mockEcr.getAuthorizationToken = jest.fn().mockReturnValue({
+    promise: jest.fn().mockImplementation(() => Promise.resolve({
+      authorizationData: [
+        { authorizationToken: 'dXNlcjpwYXNz', proxyEndpoint: `https://${proxyIdx++}.proxy.com/` },
+      ],
+    })),
+  });
+
+  const expectAllSpawns = mockSpawn(
+    { commandLine: ['docker', 'login', '--username', 'user', '--password-stdin', 'https://12345.proxy.com/'] },
+    { commandLine: ['docker', 'inspect', 'cdkasset-theasset1'], exitCode: 1 },
+    { commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset1', '.'], cwd: '/multi/cdk.out/dockerdir' },
+    { commandLine: ['docker', 'tag', 'cdkasset-theasset1', '12345.amazonaws.com/aws-cdk/assets:theAsset1'] },
+    { commandLine: ['docker', 'push', '12345.amazonaws.com/aws-cdk/assets:theAsset1'] },
+    { commandLine: ['docker', 'login', '--username', 'user', '--password-stdin', 'https://12346.proxy.com/'] },
+    { commandLine: ['docker', 'inspect', 'cdkasset-theasset2'], exitCode: 1 },
+    { commandLine: ['docker', 'build', '--tag', 'cdkasset-theasset2', '.'], cwd: '/multi/cdk.out/dockerdir' },
+    { commandLine: ['docker', 'tag', 'cdkasset-theasset2', '12346.amazonaws.com/aws-cdk/assets:theAsset2'] },
+    { commandLine: ['docker', 'push', '12346.amazonaws.com/aws-cdk/assets:theAsset2'] },
+  );
+
+  await pub.publish();
+
+  expectAllSpawns();
+  expect(true).toBeTruthy(); // Expect no exception, satisfy linter
+});
diff --git a/packages/cdk-assets/test/util.test.ts b/packages/cdk-assets/test/util.test.ts
@@ -0,0 +1,32 @@
+import { createCriticalSection } from '../lib/private/util';
+
+test('critical section', async () => {
+  // GIVEN
+  const criticalSection = createCriticalSection();
+
+  // WHEN
+  const arr = new Array<string>();
+  void criticalSection(async () => {
+    await new Promise(res => setTimeout(res, 500));
+    arr.push('first');
+  });
+  await criticalSection(async () => {
+    arr.push('second');
+  });
+
+  // THEN
+  expect(arr).toEqual([
+    'first',
+    'second',
+  ]);
+});
+
+test('exceptions in critical sections', async () => {
+  // GIVEN
+  const criticalSection = createCriticalSection();
+
+  // WHEN/THEN
+  await expect(() => criticalSection(async () => {
+    throw new Error('Thrown');
+  })).rejects.toThrow('Thrown');
+});
