Conversation
|
|
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Do we need a new integ-test? |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| /** | ||
| * Grant the given identity permissions to use the images in this repository | ||
| */ | ||
| public grantPush(grantee: iam.IGrantable) { |
There was a problem hiding this comment.
This feels reasonable to me but I don't have too much context in ecr so just going to make this comment here.
If we go this route, remember to change grantPullPush to call grantPush -- there's no reason to duplicate the code in both functions.
But then we also have to audit ecr:BatchCheckLayerAvailability, because that's not part of the permissions for grantPullPush. Why is it part of grantPush? And if it is necessary, it should be a part of grantPullPush as well.
There was a problem hiding this comment.
There was an issue of duplication with the getauthorizationtoken when executing grantPull and grantPush within grantPullPush. I have ensured readability by extracting and commonizing other Actions. I am referencing the S3Bucket. I am not fully aware of the details of BatchCheckLayerAvailability, but it is an Action in accordance with the following official documentation. I have made it compatible with both Pull and PullPush.
https://docs.aws.amazon.com/ja_jp/AmazonECR/latest/userguide/image-push.html#image-push-iam
There was a problem hiding this comment.
I'd like to confirm if the duplication is really an issue here. iam.Grant is wise enough to get rid of any duplication, e.g. even if you allow ecr:GetAuthorizationToken twice, the policy is synthesized without any error and it has just a single allow policy. Afaik it should be valid and safe to call grantPull and grantPush in grantPullPush.
| @@ -0,0 +1,15 @@ | |||
| export const REPO_PULL_ACTIONS: string[] = [ | |||
There was a problem hiding this comment.
Given that these constants are implementation details, I would keep them as private variables in repository.ts.
| ...perm.REPO_PULL_ACTIONS, | ||
| ...perm.REPO_PUSH_ACTIONS, | ||
| ); | ||
| iam.Grant.addToPrincipal({ |
There was a problem hiding this comment.
To deal with the duplication issue, you can declare both sets of actions as Set<string>, both of which containing 'ecr:GetAuthorizationToken'. Then you can transform them to arrays before passing them to this.grant().
There was a problem hiding this comment.
Thank you for the suggestion. Unfortunately, the ecr:GetAuthorizationToken action is invoked not against a registry, but against the entire ECR service, requiring its resource to be set as '*'. Therefore, I believe it's difficult to handle it along with other Actions within the grant. With this in mind, if there are any necessary refactorings, I would appreciate your guidance.
…into ecr-grant-push
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
For example, I realized that the role given to perform a push from Github Actions to ECR is excessive if using grantPullPush. The Readme was temporarily updated to fulfill the conditions of a 'feat' commit.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license