Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(cli): ImagePublishingRoleDefaultPolicy in bootstrap template should have docker pull permissions #14656

Closed
1 of 2 tasks
jogold opened this issue May 12, 2021 · 1 comment · Fixed by #14662
Closed
1 of 2 tasks
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@jogold
Copy link
Contributor

jogold commented May 12, 2021

Using a common docker asset as base image for other docker assets requires the image publishing role to have the ecr:BatchGetImage, ecr:GetDownloadUrlForLayer and ecr:InitiateLayerUpload permissions.

The comment here implies that it should be possible:

// Login before build so that the Dockerfile can reference images in the ECR repo
await this.docker.login(ecr);

This is #6466 but for the new style stack synthesis.

Use Case

Consider the following pattern to "reuse" a common docker asset as base image in other docker assets:

/**
 * A construct that creates a common docker asset that can be used
 * in multiple stacks as base image
 */
class CommonDockerAsset extends cdk.Construct {
  public readonly imageTag: string;

  constructor(scope: cdk.Construct, id: string) {
    super(scope, id);

    const asset = new assets.DockerImageAsset(this, 'Image', {
      directory: path.join(__dirname, '../common-docker'),
    });

    this.imageTag = asset.sourceHash;
  }
}
# Dockerfile for asset in StackA that should use the common docker
# asset as base image
ARG TAG
FROM 123456789012.dkr.ecr.eu-west-1.amazonaws.com/cdk-hnb659fds-container-assets-123456789012-eu-west-1:$TAG

COPY ...
/**
 * Use it in StackA and pass TAG as build arg
 */
class StackA extends TaggedStack {
  constructor(scope: cdk.Construct, id: string) {
    super(scope, id);

    const commonDockerAsset = new CommonDockerAsset(this, 'CommonDockerAsset');
    const taskDefinition = new ecs.FargateTaskDefinition(this, 'TaskDef');
    taskDefinition.addContainer('Container', {
      image: ecs.ContainerImage.fromAsset(path.join(__dirname, '../stack-docker'), {
        buildArgs: { TAG: commonDockerAsset.imageTag },
      }),
    });
  }
}

Proposed Solution

Add the ecr:BatchGetImage, ecr:GetDownloadUrlForLayer and ecr:InitiateLayerUpload in the bootstrap template.

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@jogold jogold added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 12, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry label May 12, 2021
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label May 12, 2021
@jogold jogold changed the title (cli): ImagePublishingRoleDefaultPolicy in bootstrap template should have ecr:BatchGetImage permission (cli): ImagePublishingRoleDefaultPolicy in bootstrap template should have docker pull permissions May 12, 2021
jogold added a commit to jogold/aws-cdk that referenced this issue May 12, 2021
…sions

Using a common docker asset as base image for other docker assets requires
the image publishing role to have the `ecr:BatchGetImage`, `ecr:GetDownloadUrlForLayer`
and `ecr:InitiateLayerUpload` permissions.

Closes aws#14656
@MrArnoldPalmer MrArnoldPalmer removed their assignment May 17, 2021
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 4, 2021
@rix0rrr rix0rrr removed their assignment Jun 4, 2021
@mergify mergify bot closed this as completed in #14662 Jun 4, 2021
mergify bot pushed a commit that referenced this issue Jun 4, 2021
…14662)

Using a common docker asset as base image for other docker assets requires
the image publishing role to have the `ecr:BatchGetImage`, `ecr:GetDownloadUrlForLayer`
and `ecr:InitiateLayerUpload` permissions.

Closes #14656


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…ws#14662)

Using a common docker asset as base image for other docker assets requires
the image publishing role to have the `ecr:BatchGetImage`, `ecr:GetDownloadUrlForLayer`
and `ecr:InitiateLayerUpload` permissions.

Closes aws#14656


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants