Skip to content

Commit

Permalink
feat(ecs): Add warning message when pulling ECR image (#4334)
Browse files Browse the repository at this point in the history
* Add warning message when pulling ECR image using fromRegistry()

* Refactor the regex to bind method, add token validation, and add more test case
  • Loading branch information
iamhopaul123 authored and mergify[bot] committed Oct 2, 2019
1 parent 53db8bc commit bd36c6c
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 2 deletions.
16 changes: 14 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/images/repository.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import secretsmanager = require('@aws-cdk/aws-secretsmanager');
import { Construct } from '@aws-cdk/core';
import { Construct, Token } from '@aws-cdk/core';
import { ContainerDefinition } from "../container-definition";
import { ContainerImage, ContainerImageConfig } from "../container-image";

/**
* Regex pattern to check if it is an ECR image URL.
*
* @experimental
*/
const ECR_IMAGE_REGEX = /(^[a-zA-Z0-9][a-zA-Z0-9-_]*).dkr.ecr.([a-zA-Z0-9][a-zA-Z0-9-_]*).amazonaws.com(.cn)?\/.*/;

/**
* The properties for an image hosted in a public or private repository.
*/
Expand All @@ -27,7 +34,12 @@ export class RepositoryImage extends ContainerImage {
super();
}

public bind(_scope: Construct, containerDefinition: ContainerDefinition): ContainerImageConfig {
public bind(scope: Construct, containerDefinition: ContainerDefinition): ContainerImageConfig {
// name could be a Token - in that case, skip validation altogether
if (!Token.isUnresolved(this.imageName) && ECR_IMAGE_REGEX.test(this.imageName)) {
scope.node.addWarning("Proper policies need to be attached before pulling from ECR repository, or use 'fromEcrRepository'.");
}

if (this.props.credentials) {
this.props.credentials.grantRead(containerDefinition.taskDefinition.obtainExecutionRole());
}
Expand Down
37 changes: 37 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,43 @@ export = {
test.done();
},

"warns when setting containers from ECR repository using fromRegistry method"(test: Test) {
// GIVEN
const stack = new cdk.Stack();

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');

// WHEN
const container = taskDefinition.addContainer("web", {
image: ecs.ContainerImage.fromRegistry("ACCOUNT.dkr.ecr.REGION.amazonaws.com/REPOSITORY"),
memoryLimitMiB: 512
});

// THEN
test.deepEqual(container.node.metadata[0].data, "Proper policies need to be attached before pulling from ECR repository, or use 'fromEcrRepository'.");
test.done();
},

"warns when setting containers from ECR repository by creating a RepositoryImage class"(test: Test) {
// GIVEN
const stack = new cdk.Stack();

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');

const repo = new ecs.RepositoryImage("ACCOUNT.dkr.ecr.REGION.amazonaws.com/REPOSITORY");

// WHEN
const container = taskDefinition.addContainer("web", {
image: repo,
memoryLimitMiB: 512
});

// THEN
test.deepEqual(container.node.metadata[0].data, "Proper policies need to be attached before pulling from ECR repository, or use 'fromEcrRepository'.");

test.done();
},

"correctly sets containers from asset using default props"(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down

0 comments on commit bd36c6c

Please sign in to comment.