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

(ecr): Repository.fromRepositoryArn does not validate ARN which leads to wasted time #16223

Closed
markusl opened this issue Aug 25, 2021 · 5 comments · Fixed by #25302
Closed

(ecr): Repository.fromRepositoryArn does not validate ARN which leads to wasted time #16223

markusl opened this issue Aug 25, 2021 · 5 comments · Fixed by #25302
Assignees
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry @aws-cdk/pipelines CDK Pipelines library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@markusl
Copy link
Contributor

markusl commented Aug 25, 2021

Ok, so the following code with the new CDK Pipelines produces an unexpected result:

    const ecrRepoArn = 'arn:aws:ecr:eu-central-1:1234567891234:base-image-repo-from-ecr';
    const pipeline = new pipelines.CodePipeline(this, id, {
      pipelineName,
      crossAccountKeys: true,
      dockerEnabledForSelfMutation: true,
      dockerCredentials: [pipelines.DockerCredential.ecr(
        [
          ecr.Repository.fromRepositoryArn(this, 'ecr', ecrRepoArn)
        ]
      )],

The code will produce an error when running the pipeline:

error : [100%] fail: docker --config /tmp/cdkDockerConfignkKBqd build --tag cdkasset-699e5231c66d33f2bbdcd748964cd36874b9dc39c369f2f6d521381bd97afda0 . exited with error code 1: pull access denied for 1234567891234.dkr.ecr.eu-central-1.amazonaws.com/base-image-repo-from-ecr, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::0987654376:assumed-role/PipelinePi-L9HGIT7CY91W/AWSCodeBuild-6054835c-9a09-4c3c-8f34-401787bd9553 is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:eu-central-1:1234567891234:repository/base-image-repo-from-ecr

Can you spot the problem? Neither did I. So, I navigated to the ECR console to look for the ARN, but there are no ARNs. Googling ECR ARN produces AWS documentation page https://docs.aws.amazon.com/AmazonECR/latest/APIReference/API_Repository.html which does not have any ARNs.

After navigating to the IAM console, I see the following code which looks about right:

        {
            "Action": [
                "ecr:BatchCheckLayerAvailability",
                "ecr:GetDownloadUrlForLayer",
                "ecr:BatchGetImage"
            ],
            "Resource": "arn:aws:ecr:eu-central-1:1234567891234:base-image-repo-from-ecr",
            "Effect": "Allow"
        },

However, if you look very closely you can spot an error: Ln 47, Col 24 Invalid ARN Resource: Resource ARN does not match the expected ARN format. Update the resource portion of the ARN. Learn more .

The learn more button leads to a page https://docs.aws.amazon.com/IAM/latest/UserGuide/access-analyzer-reference-policy-checks.html#access-analyzer-reference-policy-checks-error-invalid-arn-resource which does not document what an ECR ARN looks like.

I proceed to launch a Cloud Shell and start learning AWS CLI. It looks like running aws ecr describe-repositories outputs the ARN format of the repositories.

What did you expect to happen?

Maybe the repository name given for ecr.Repository.fromRepositoryArn could be validated at synth time.

However, as you can see this waste of time was clearly caused by user error so feel free to close the ticket if you want :)

I'm documenting this here if anyone else runs to the same issue.

Environment

  • CDK CLI Version : 2.0.0-rc.17 (build fb5dc58)
  • Framework Version: 2.0.0-rc.17 (build fb5dc58)
  • Node.js Version: 14
  • OS : macOS
  • Language (Version): all

ECR ARN FORMAT

For future reference:

arn:aws:ecr:<REGION>:<ACCOUNT>:repository/<ACTUAL_REPOSITORY_NAME>

This is 🐛 Bug Report

@markusl markusl added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 25, 2021
@github-actions github-actions bot added @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry @aws-cdk/pipelines CDK Pipelines library labels Aug 25, 2021
@rix0rrr rix0rrr changed the title (pipelines): DockerCredential.ecr with Repository.fromRepositoryArn not working as expected (ecr): Repository.fromRepositoryArn does not validate ARN which can lead to user error Sep 7, 2021
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 7, 2021
@rix0rrr rix0rrr removed their assignment Sep 7, 2021
@rix0rrr rix0rrr changed the title (ecr): Repository.fromRepositoryArn does not validate ARN which can lead to user error (ecr): Repository.fromRepositoryArn does not validate ARN which leads to wasted time Sep 7, 2021
@johnschultz
Copy link

Just ran into this myself. Would have been very confused if I had not found this issue.

@madeline-k madeline-k removed their assignment Nov 18, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 19, 2022
@markusl
Copy link
Contributor Author

markusl commented Nov 19, 2022

Arn validation would still be useful.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 19, 2022
@vinayak-kukreja vinayak-kukreja self-assigned this Apr 24, 2023
@mergify mergify bot closed this as completed in #25302 May 31, 2023
mergify bot pushed a commit that referenced this issue May 31, 2023
This PR is adding a regex to check the `arn` structure of an ECR repository. 

Closes #16223

----

*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

⚠️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.

@runev-admin
Copy link

#25302 Actually breaks the scenario below at synth time:

    const repoArn = ssm.StringParameter.valueFromLookup(
      this,
      configs.RepoArnParameterName
    );
    taskDefinition
      .addContainer("Container", {
        image: ecs.ContainerImage.fromEcrRepository(
          ecr.Repository.fromRepositoryArn(this, "EcrRepo", repoArn),
          releaseVersion
        ),
  ....

Error: Repository arn should be in the format 'arn::ecr:::repository/', got dummy-value-for-/xxx/us-east-1/xx/xxx/EcrRepoArn.

You can't validate the ARN at synth time. repoArn will be a dummy value then!

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 @aws-cdk/pipelines CDK Pipelines library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants