feat(aws-ecr): add public repository construct #16229
Conversation
|
|
RomainMuller
force-pushed
the
master
branch
from
924c117 to
ebfd5f2
Compare
peterwoodworth
changed the title
github-actions
bot
added
the
@aws-cdk/aws-ecr
|
@madeline-k any update on this? |
|
@skinny85 can we get some assistance here? |
There was a problem hiding this comment.
Thanks for submitting this PR @yerzhan7! After a first pass, my biggest feedback is that there is a bunch of similarities between PublicRepository and Repository. I think there is an opportunity to use a base class to share some logic here. I will make another pass with more detailed feedback soon, but thought you might want to get started on that!
|
Thanks for reviewing @madeline-k
I'll refactor into separate class in a few days. |
| const rule = new events.Rule(this, id, options); | ||
| rule.addTarget(options.target); | ||
| rule.addEventPattern({ | ||
| source: ['aws.ecr-public'], | ||
| detailType: ['AWS API Call via CloudTrail'], | ||
| detail: { | ||
| requestParameters: { | ||
| repositoryName: [this.publicRepositoryName], | ||
| }, | ||
| }, | ||
| }); | ||
| return rule; |
There was a problem hiding this comment.
This is the exact same logic as onCloudTrailEvent in RepositoryBase.
There was a problem hiding this comment.
See argument below.
| /** | ||
| * The name of the repository. | ||
| * @attribute | ||
| */ | ||
| readonly publicRepositoryName: string; | ||
|
|
||
| /** | ||
| * The ARN of the repository. | ||
| * @attribute | ||
| */ | ||
| readonly publicRepositoryArn: string; |
There was a problem hiding this comment.
Why not move these up to IBaseRepository and use repositoryName and repositoryArn for both public and private repositories?
There was a problem hiding this comment.
I cannot do that because linter requires resource name to be of form ${CfnResource.name}Name.
| public onCloudTrailImagePushed(id: string, options: OnCloudTrailImagePushedOptions = {}): events.Rule { | ||
| const rule = this.onCloudTrailEvent(id, options); | ||
| rule.addEventPattern({ | ||
| detail: { | ||
| eventName: ['PutImage'], | ||
| requestParameters: { | ||
| imageTag: options.imageTag ? [options.imageTag] : undefined, | ||
| }, | ||
| }, | ||
| }); | ||
| return rule; | ||
| } |
There was a problem hiding this comment.
This is the same implementation as RepositoryBase.onCloudTrailImagePushed. This should move to a shared base class.
There was a problem hiding this comment.
See argument below.
| const rule = new events.Rule(this, id, options); | ||
| rule.addEventPattern({ | ||
| source: ['aws.ecr-public'], | ||
| resources: [this.publicRepositoryArn], | ||
| }); | ||
| rule.addTarget(options.target); | ||
| return rule; |
There was a problem hiding this comment.
Also the same as RepositoryBase
There was a problem hiding this comment.
To still differentiate the validation errors needed for public repos, you can use the template pattern.
There was a problem hiding this comment.
Same argument as below: I cannot make onEvent method to be defined in parent class as Typescript does not support multiple inheritance.
| public grant(grantee: iam.IGrantable, ...actions: string[]) { | ||
| return iam.Grant.addToPrincipalAndResource({ | ||
| grantee, | ||
| actions, | ||
| resourceArns: [this.publicRepositoryArn], | ||
| resourceSelfArns: [], | ||
| resource: this, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can be shared with RepositoryBase
There was a problem hiding this comment.
I cannot make grantmethod to be defined in parent class because PublicRepositoryBase class already extends Resource class, and Typescript does not support multiple inheritance.
If you have alternative ideas let me know.
| * | ||
| * @default Automatically generated name. | ||
| */ | ||
| readonly publicRepositoryName?: string; |
There was a problem hiding this comment.
This should move up to BaseRepositoryProps
There was a problem hiding this comment.
I cannot do that because linter requires resource name to be of form ${CfnResource.name}Name.
| /** | ||
| * ECR Public Repository attributes. | ||
| */ | ||
| export interface PublicRepositoryAttributes { | ||
|
|
||
| /** | ||
| * The name of the repository. | ||
| */ | ||
| readonly publicRepositoryName: string; | ||
|
|
||
| /** | ||
| * The ARN of the repository. | ||
| */ | ||
| readonly publicRepositoryArn: string; | ||
| } |
There was a problem hiding this comment.
We can re-use the RepositoryAttributes interface since they are the same.
There was a problem hiding this comment.
I cannot do this because of linter rule violation.
| private static validateRepositoryName(physicalName: string) { | ||
| const repositoryName = physicalName; | ||
| if (!repositoryName || Token.isUnresolved(repositoryName)) { | ||
| // the name is a late-bound value, not a defined string, | ||
| // so skip validation | ||
| return; | ||
| } | ||
|
|
||
| const errors: string[] = []; | ||
|
|
||
| // Rules codified from https://docs.aws.amazon.com/AmazonECRPublic/latest/APIReference/API_CreateRepository.html#ecrpublic-CreateRepository-request-repositoryName | ||
| if (repositoryName.length < 2 || repositoryName.length > 205) { | ||
| errors.push('Repository name must be at least 2 and no more than 205 characters'); | ||
| } | ||
| const isPatternMatch = /^(?:[a-z0-9]+(?:[._-][a-z0-9]+)*\/)*[a-z0-9]+(?:[._-][a-z0-9]+)*$/.test(repositoryName); | ||
| if (!isPatternMatch) { | ||
| errors.push('Repository name must follow the specified pattern: (?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*'); | ||
| } | ||
|
|
||
| if (errors.length > 0) { | ||
| throw new Error(`Invalid ECR repository name (value: ${repositoryName})${EOL}${errors.join(EOL)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This validation is really similar to the Repository.validateRepositoryName. I think we should use the template pattern here as well.
There was a problem hiding this comment.
It is similar. Same argument about multiple inheritance.
I can extract it into separate base-repository.ts method if you want.
| /** | ||
| * Add a policy statement to the repository's resource policy | ||
| */ | ||
| public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { | ||
| if (this.resourcePolicy === undefined) { | ||
| this.resourcePolicy = new iam.PolicyDocument(); | ||
| } | ||
| this.resourcePolicy.addStatements(statement); | ||
| return { statementAdded: true, policyDependable: this.resourcePolicy }; | ||
| } | ||
|
|
||
| protected validate(): string[] { | ||
| const errors = super.validate(); | ||
| errors.push(...this.resourcePolicy?.validateForResourcePolicy() || []); | ||
| return errors; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is all the same as Repository as well.
|
Thanks for reviewing!
I cannot do that due to Typescript not supporting multiple inheritance. |
mergify
bot
dismissed
madeline-k’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@madeline-k would it be able to just live, for a while, with a little bit of duplication and change that in follow-up? |
rix0rrr
added
feature-request
Add L2 construct for
AWS::ECR::PublicRepository.https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecr-publicrepository.html
Closes #12162
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license