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

aws-ecr: repositoryName of imported repository may be incorrect #1232

Closed
eladb opened this issue Nov 22, 2018 · 1 comment · Fixed by #1233
Closed

aws-ecr: repositoryName of imported repository may be incorrect #1232

eladb opened this issue Nov 22, 2018 · 1 comment · Fixed by #1233
Assignees

Comments

@eladb
Copy link
Contributor

eladb commented Nov 22, 2018

Repro:

const repo = Repository.import(this, 'MyRepo', { 
  repositoryArn: 'arn:aws:ecr:us-east-1:585695036304:repository/foo/bar/foo/fooo'
});

Now, we expect repo.repositoryName to resolve to foo/bar/foo/fooo but it will resolve to foo.

This is because ImportedRepository uses CFN's split/select to extract the first component after "/".

@eladb
Copy link
Contributor Author

eladb commented Nov 22, 2018

If the user provides a concrete (non-token) string, we can just use synth-time string manipulation to extract the full repository name. If repositoryArn is a token, we will have to require that repositoryName will also be passed.

eladb pushed a commit that referenced this issue Nov 22, 2018
This change enables using Docker assets as CodeBuild images.

In order to enable that, a few changes were required in how Docker assets
are modeled:

Extracted a low-level `DockerImageAsset` class into a new module called
@aws-cdk/assets-docker. This module provides the basic interaction with
the toolkit and allows higher level APIs such as ECR or CodeBuild to
manage permissions on the asset repository.

Since CodeBuild needs permissions at the resource policy level (since
images are not pulled via an assumed role), the adopted repository
custom resource was extended to support specifying an arbitrary policy
document for the repository. This is made available via an overload of
`addToResourcePolicy` implemented by `AdoptedRepository`.

In order to fix #1232 and simplify, the protocol between the toolkit and
the asset class have been changed to only pass in the _repository name_
and the _tag_ (together). Previously the ARN of the repository was used,
but it is impossible to parse the repository name from the ARN using
CFN's split/select. It is possible to produce the ARN from a name, and
the repository is ensured to be "local" to the account/region.

Modernized ECR import/export and converted `RepositoryRef` to `IRepository`,
and modified `ImportedRepository` to only parse the repository name from 
ARN in case the ARN is concrete (not a token). Otherwise, the name is
also required.

Renamed `grantUseImage` to `grantPull` and added `grantPullPush`.

Added `repositoryUriForTag(tag)`.

Fixes #1219

BREAKING CHANGE: `ecr.RepositoryRef` has been replaced by `ecr.IRepository`, which
means that `RepositoryRef.import` is now `Repository.import`. Futhermore, the CDK 
Toolkit must also be upgraded since the docker asset protocol was modified. 
`IRepository.grantUseImage` was renamed to `IRepository.grantPull`.
eladb pushed a commit that referenced this issue Nov 30, 2018
…1233)

This change enables using Docker assets as CodeBuild images.

In order to enable that, a few changes were required in how Docker assets
are modeled:

Extracted a low-level `DockerImageAsset` class into a new module called
@aws-cdk/assets-docker. This module provides the basic interaction with
the toolkit and allows higher level APIs such as ECR or CodeBuild to
manage permissions on the asset repository.

Since CodeBuild needs permissions at the resource policy level (since
images are not pulled via an assumed role), the adopted repository
custom resource was extended to support specifying an arbitrary policy
document for the repository. This is made available via an overload of
`addToResourcePolicy` implemented by `AdoptedRepository`.

In order to fix #1232 and simplify, the protocol between the toolkit and
the asset class have been changed to only pass in the _repository name_
and the _tag_ (together). Previously the ARN of the repository was used,
but it is impossible to parse the repository name from the ARN using
CFN's split/select. It is possible to produce the ARN from a name, and
the repository is ensured to be "local" to the account/region.

Modernized ECR import/export and converted `RepositoryRef` to `IRepository`,
and modified `ImportedRepository` to only parse the repository name from 
ARN in case the ARN is concrete (not a token). Otherwise, the name is
also required.

- Bump cxapi version to 0.19.0 (since protocol changed)
- In integration tests, use local modules instead of "npm install"
- Remove init template tests. Since they use "npm install" they can't
  really be validated like this against the local bits. We have integration
  tests for init template in our pipeline that work against the latest build.

Renamed `grantUseImage` to `grantPull` and added `grantPullPush`.

Added `repositoryUriForTag(tag)`.

Fixes #1219

BREAKING CHANGE: `ecr.RepositoryRef` has been replaced by `ecr.IRepository`, which
means that `RepositoryRef.import` is now `Repository.import`. Futhermore, the CDK 
Toolkit must also be upgraded since the docker asset protocol was modified. 
`IRepository.grantUseImage` was renamed to `IRepository.grantPull`.
@eladb eladb self-assigned this Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant