From 7fe0736636f59db2a511bda09c168f18e4602e6e Mon Sep 17 00:00:00 2001 From: drduhe Date: Fri, 31 Jan 2025 11:15:28 -0700 Subject: [PATCH] fix: correctly parse ECR URIs and tags to OSMLContainer --- .eslintrc.yaml | 1 - lib/osml/osml_container.ts | 226 ++++++++++++++++++--------- lib/osml/tile_server/ts_dataplane.ts | 4 +- test/osml/osml_container.test.ts | 140 ++++++++++++++--- 4 files changed, 277 insertions(+), 94 deletions(-) diff --git a/.eslintrc.yaml b/.eslintrc.yaml index fe04127..c826bdd 100644 --- a/.eslintrc.yaml +++ b/.eslintrc.yaml @@ -32,7 +32,6 @@ rules: require-await: off "@typescript-eslint/no-unused-expressions": - error - - allowShortCircuit: true - allowTernary: true "@typescript-eslint/no-unsafe-assignment": warn "@typescript-eslint/interface-name-prefix": off diff --git a/lib/osml/osml_container.ts b/lib/osml/osml_container.ts index 9fbe859..05f21cc 100644 --- a/lib/osml/osml_container.ts +++ b/lib/osml/osml_container.ts @@ -1,5 +1,5 @@ /* - * Copyright 2023-2024 Amazon.com, Inc. or its affiliates. + * Copyright 2023-2025 Amazon.com, Inc. or its affiliates. */ import { RemovalPolicy, SymlinkFollowMode } from "aws-cdk-lib"; @@ -21,6 +21,7 @@ import { RegionalConfig, RegionConfig } from "./utils/regional_config"; export class OSMLContainerConfig extends BaseConfig { /** * The default container image. + * Can only be specified if REPOSITORY_ARN is not set. */ public CONTAINER_URI?: string; @@ -39,12 +40,25 @@ export class OSMLContainerConfig extends BaseConfig { */ public CONTAINER_DOCKERFILE?: string; + /** + * (Optional) ARN of an existing container to be used. + * Can only be specified if CONTAINER_URI is not set. + */ + public ECR_REPOSITORY_ARN?: string; + + /** + * (Optional) ARN of an existing container to be used. + * Only used if REPOSITORY_ARN is set and will default to "latest". + */ + public ECR_REPOSITORY_TAG?: string; + /** * Creates an instance of OSMLContainerConfig. * @param config - The configuration object for OSMLContainer. */ constructor(config: ConfigType = {}) { super({ + REPOSITORY_TAG: "latest", ...config }); } @@ -212,95 +226,161 @@ export class OSMLContainer extends Construct { } /** - * Builds the container from an existing repository using the specified repository URI in the config. - * Validates the configuration to ensure that the necessary parameters are provided. + * Builds the container from an existing repository using the specified repository URI or ARN in the config. + * Ensures that only one of `CONTAINER_URI` or `CONTAINER_ARN` is provided. * - * @throws {Error} If CONTAINER_URI is not set in the configuration. + * @throws {Error} If both `CONTAINER_URI` and `CONTAINER_ARN` are provided or neither is set. */ private buildFromRepository(): void { - // Validate that the CONTAINER_URI is provided in the configuration. - if (!this.config.CONTAINER_URI) { + const { CONTAINER_URI, ECR_REPOSITORY_ARN } = this.config; + + this.validateContainerConfig(CONTAINER_URI, ECR_REPOSITORY_ARN); + + if (ECR_REPOSITORY_ARN) { + this.importRepositoryByArn(ECR_REPOSITORY_ARN); + } else { + this.importContainerByUri(CONTAINER_URI!); + } + } + + /** + * Validates the configuration to ensure only one of CONTAINER_URI or CONTAINER_ARN is provided. + * + * @param {string | undefined} uri - The container URI. + * @param {string | undefined} arn - The container ARN. + * @throws {Error} If both or neither are provided. + */ + private validateContainerConfig(uri?: string, arn?: string): void { + if (uri && arn) { throw new Error( - "CONTAINER_URI must be set in the configuration to use a pre-built container image." + "Only one of CONTAINER_URI or ECR_REPOSITORY_ARN can be set." ); } - - // Determine the tag to use for the container image, defaulting to "latest" if not provided. - const tag = "latest"; - - // Check if the CONTAINER_URI indicates an Amazon ECR repository by checking the ARN format. - const ecrArnRegex = - /^arn:([^:]+):ecr:([^:]+):[^:]+:repository\/[a-zA-Z0-9-._]+(:[a-zA-Z0-9-._]+)?$/; - - if (ecrArnRegex.test(this.config.CONTAINER_URI)) { - let repositoryArn = this.config.CONTAINER_URI; - let extractedTag = tag; - - // Extract tag if present - const tagSeparatorIndex = this.config.CONTAINER_URI.lastIndexOf(":"); - if ( - tagSeparatorIndex > this.config.CONTAINER_URI.indexOf("repository/") - ) { - repositoryArn = this.config.CONTAINER_URI.substring( - 0, - tagSeparatorIndex - ); - extractedTag = this.config.CONTAINER_URI.substring( - tagSeparatorIndex + 1 - ); - } - - // Import the existing ECR repository using the ARN provided in the CONTAINER_URI. - this.repository = Repository.fromRepositoryArn( - this, - "ImportedECRRepo", - repositoryArn + if (!uri && !arn) { + throw new Error( + "Either CONTAINER_URI or ECR_REPOSITORY_ARN must be set in the configuration." ); + } + } - // Create a ContainerImage object from the imported ECR repository and specified tag. - this.containerImage = ContainerImage.fromEcrRepository( - this.repository, - extractedTag - ); + /** + * Imports an ECR repository by ARN. + * + * @param {string} arn - The ECR repository ARN. + */ + private importRepositoryByArn(arn: string): void { + this.repository = Repository.fromRepositoryArn( + this, + "ImportedECRRepo", + arn + ); + this.containerImage = ContainerImage.fromEcrRepository( + this.repository, + this.config.ECR_REPOSITORY_TAG + ); + this.containerUri = this.repository.repositoryUri; - // Set the containerUri to the full URI of the container image in ECR. - this.containerUri = this.repository.repositoryUriForTag(extractedTag); + if (this.buildDockerImageCode) { + this.dockerImageCode = DockerImageCode.fromEcr(this.repository); + } + } - if (this.buildDockerImageCode) { - // Create a DockerImageCode object for Lambda using the imported ECR repository. - this.dockerImageCode = DockerImageCode.fromEcr(this.repository, { - tagOrDigest: extractedTag - }); - } + /** + * Imports a container by URI, handling ECR repositories and external registries. + * + * @param {string} uri - The container URI. + */ + private importContainerByUri(uri: string): void { + const ecrUriRegex = + /^[0-9]{12}\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com\/[a-zA-Z0-9-_]+/; + + if (ecrUriRegex.test(uri)) { + this.importEcrContainer(uri); } else { - // If the CONTAINER_URI does not indicate an ECR repository, assume it is a public or private Docker registry. - this.repositoryAccessMode = "Vpc"; + this.importExternalContainer(uri); + } + } - // Create a ContainerImage object from the provided Docker registry URI and tag. - this.containerImage = ContainerImage.fromRegistry( - this.config.CONTAINER_URI - ); + /** + * Imports a container from an ECR repository. + * + * @param {string} uri - The container URI. + */ + private importEcrContainer(uri: string): void { + // Extract the repository name and tag from the URI + const tag = this.extractTagFromUri(uri); + const repoName = uri.split("/").pop()?.split(":")[0] ?? ""; + + this.repository = Repository.fromRepositoryName( + this, + "ImportedECRRepo", + repoName + ); + this.containerImage = ContainerImage.fromEcrRepository( + this.repository, + tag + ); + this.containerUri = `${this.repository.repositoryUri}:${tag}`; - // Set the containerUri to the full URI of the container image in the Docker registry. - this.containerUri = this.config.CONTAINER_URI; + if (this.buildDockerImageCode) { + this.dockerImageCode = DockerImageCode.fromEcr(this.repository, { + tagOrDigest: tag + }); + } + } - if (this.buildDockerImageCode) { - // Define the Dockerfile content dynamically based on containerUri - const dockerfileContent = `FROM ${this.config.CONTAINER_URI}`; + /** + * Imports a container from an external registry such as Docker Hub. + * + * @param {string} uri - The container URI. + */ + private importExternalContainer(uri: string): void { + this.repositoryAccessMode = "Vpc"; + this.containerImage = ContainerImage.fromRegistry(uri); + this.containerUri = uri; - // Create a temporary Dockerfile to build the Docker image with - const tmpDockerfile = "Dockerfile.tmp"; + if (this.buildDockerImageCode) { + this.createDockerImageFromRegistry(uri); + } + } + + /** + * Creates a temporary Dockerfile to use an external registry image. + * + * @param {string} uri - The container URI. + */ + private createDockerImageFromRegistry(uri: string): void { + const tmpDockerfile = path.join(__dirname, "Dockerfile.tmp"); + fs.writeFileSync(tmpDockerfile, `FROM ${uri}`); - // Write the temp Dockerfile to the build directory - const dockerfilePath = path.join(__dirname, tmpDockerfile); - fs.writeFileSync(dockerfilePath, dockerfileContent); + this.dockerImageCode = DockerImageCode.fromImageAsset(__dirname, { + file: "Dockerfile.tmp", + followSymlinks: SymlinkFollowMode.ALWAYS + }); + } - // Create a DockerImageCode object for Lambda using the DockerImageAsset - this.dockerImageCode = DockerImageCode.fromImageAsset(__dirname, { - file: tmpDockerfile, - followSymlinks: SymlinkFollowMode.ALWAYS - }); - } + /** + * Extracts the tag from a container URI. + * If no tag is found, returns `"latest"` as the default. + * + * @param {string} uri - The container URI. + * @returns {string} The extracted tag or `"latest"` if none is found. + */ + private extractTagFromUri(uri: string): string { + if (!uri) { + throw new Error("Container URI is undefined or empty."); } + + // Find the last `:` in the URI to check for a tag + const tagSeparatorIndex = uri.lastIndexOf(":"); + const lastSlashIndex = uri.lastIndexOf("/"); + + // Ensure the colon comes after the last slash (indicating a tag, not a protocol separator) + if (tagSeparatorIndex > lastSlashIndex) { + return uri.substring(tagSeparatorIndex + 1); + } + + // Default if no tag is found + return "latest"; } } diff --git a/lib/osml/tile_server/ts_dataplane.ts b/lib/osml/tile_server/ts_dataplane.ts index ceb9dfd..d267197 100644 --- a/lib/osml/tile_server/ts_dataplane.ts +++ b/lib/osml/tile_server/ts_dataplane.ts @@ -617,7 +617,8 @@ export class TSDataplane extends Construct { "TSServiceApplicationLoadBalancer", { vpc: props.osmlVpc.vpc, - vpcSubnets: props.osmlVpc.selectedSubnets + vpcSubnets: props.osmlVpc.selectedSubnets, + securityGroup: this.securityGroup } ); @@ -637,7 +638,6 @@ export class TSDataplane extends Construct { } ); this.fargateService.node.addDependency(this.tsContainer); - this.fargateService.node.addDependency(this.alb); // Allow access to EFS from Fargate ECS this.fileSystem.grantRootAccess( diff --git a/test/osml/osml_container.test.ts b/test/osml/osml_container.test.ts index cd3eb43..244c9f8 100644 --- a/test/osml/osml_container.test.ts +++ b/test/osml/osml_container.test.ts @@ -1,43 +1,147 @@ /* - * Copyright 2024 Amazon.com, Inc. or its affiliates. + * Copyright 2024-2025 Amazon.com, Inc. or its affiliates. */ -import { App, Stack } from "aws-cdk-lib"; +import { App, RemovalPolicy, Stack } from "aws-cdk-lib"; +import { ContainerImage } from "aws-cdk-lib/aws-ecs"; import { OSMLContainer } from "../../lib"; import { test_account } from "../test_account"; -describe("MEContainer", () => { +jest.mock("aws-cdk-lib/aws-ecr-assets", () => { + return { + DockerImageAsset: jest.fn().mockImplementation(() => ({ + imageUri: "mock-image-uri" + })) + }; +}); + +describe("OSMLContainer", () => { let app: App; let stack: Stack; - let container: OSMLContainer; beforeEach(() => { app = new App(); - stack = new Stack(app, "MEContainerStack"); + stack = new Stack(app, "OSMLContainerStack"); + }); - // Mock dependencies - container = new OSMLContainer(stack, "OSMLContainer", { + it("sets correct removal policy based on environment", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { account: test_account, - buildDockerImageCode: true, - buildFromSource: false, + config: { CONTAINER_URI: "awsosml/test-uri" } + }); + + expect(container.removalPolicy).toBe( + test_account.prodLike ? RemovalPolicy.RETAIN : RemovalPolicy.DESTROY + ); + }); + + it("throws an error if neither ECR_REPOSITORY_ARN nor CONTAINER_URI is provided", () => { + expect(() => { + new OSMLContainer(stack, "OSMLContainer", { + account: test_account + }); + }).toThrow( + "Either CONTAINER_URI or ECR_REPOSITORY_ARN must be set in the configuration." + ); + }); + + it("throws error when both CONTAINER_URI and REPOSITORY_ARN are set", () => { + expect(() => { + new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + config: { + CONTAINER_URI: "awsosml/test-uri", + ECR_REPOSITORY_ARN: + "arn:aws:ecr:us-east-1:123456789012:repository/my-repo" + } + }); + }).toThrow("Only one of CONTAINER_URI or ECR_REPOSITORY_ARN can be set."); + }); + + it("uses container image from CONTAINER_URI", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + config: { CONTAINER_URI: "awsosml/test-uri:latest" } + }); + + expect(container.containerImage).toBeDefined(); + expect(container.containerUri).toBe("awsosml/test-uri:latest"); + }); + + it("uses container image from REPOSITORY_ARN", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + config: { + ECR_REPOSITORY_ARN: + "arn:aws:ecr:us-east-1:123456789012:repository/my-repo" + } + }); + + expect(container.repository).toBeDefined(); + expect(container.containerImage).toBeDefined(); + expect(container.containerUri).toContain("123456789012.dkr.ecr.us-east-1"); + expect(container.containerUri).toContain("/my-repo"); + }); + + it("builds Docker image when buildFromSource is enabled", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + buildFromSource: true, config: { - CONTAINER_URI: "awsosml/test-uri" + CONTAINER_DOCKERFILE: "Dockerfile", + CONTAINER_BUILD_PATH: "./path/to/build" } }); + + expect(container.dockerImageAsset).toBeDefined(); + expect(container.containerImage).toBeDefined(); + expect(container.containerUri).toBe(container.dockerImageAsset.imageUri); }); - it("sets removal policy", () => { - expect(container.removalPolicy).toBeDefined(); + it("throws error when buildFromSource is enabled but missing required fields", () => { + expect(() => { + new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + buildFromSource: true, + config: {} + }); + }).toThrow( + "CONTAINER_DOCKERFILE and CONTAINER_BUILD_PATH must be set in the configuration to build from source." + ); }); - it("creates config if not provided", () => { - expect(container.config).toBeDefined(); + it("uses external registry image when given a non-ECR URI", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + config: { CONTAINER_URI: "docker.io/library/nginx:1.21" } + }); + + expect(container.containerImage).toBeInstanceOf(ContainerImage); + expect(container.containerUri).toBe("docker.io/library/nginx:1.21"); }); - it("sets container image", () => { - expect(container.containerImage).toBeDefined(); - expect(container.config).toBeDefined(); - expect(container.dockerImageCode).toBeDefined(); + it("creates Docker image code for Lambda when enabled", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + buildFromSource: true, + buildDockerImageCode: true, + config: { + CONTAINER_DOCKERFILE: "Dockerfile", + CONTAINER_BUILD_PATH: "./path/to/build" + } + }); + + expect(container.dockerImageCode).toBeInstanceOf(Object); + }); + + it("does not create Docker image code when buildDockerImageCode is false", () => { + const container = new OSMLContainer(stack, "OSMLContainer", { + account: test_account, + buildDockerImageCode: false, + config: { CONTAINER_URI: "awsosml/test-uri:latest" } + }); + + expect(container.dockerImageCode).toBeUndefined(); }); });