fix(ecr): policytext errors when includes resource #24401

ahammond · 2023-03-01T17:23:27Z

ECR does not allow resource to be included in private repository resource policies.
CFN largely swallows the error message.
Most resources require or at least allow a resource in their policies, so we should at least warn.

See issue #24314

aws-cdk-automation

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.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra

Thanks for picking this up!

Looks like hasWarnings should be hasWarning, which is causing the build failure. Also just a note that on a fix the PR title should state the bug, not the solution implemented. Can you adjust that, please?

Pull request has been modified.

ahammond · 2023-03-01T20:01:38Z

I tried to test this locally, but

❯ ../../../scripts/buildup         
************************************************************
 buildup usage:
 - execute 'buildup --resume' to resume after failure
 - execute 'buildup' to restart the build from the start

 for advanced usage, see /Users/ahammond/Documents/aws/aws-cdk/scripts/foreach.sh
************************************************************
state cleared. you are free to start a new command.
starting new session ('@aws-cdk/aws-ecr' and its dependencies)
---------------------------------------------------------------------------------
/Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/eslint-plugin: yarn build  (23 remaining)
---------------------------------------------------------------------------------
/Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/node-bundle: yarn build  (22 remaining)
---------------------------------------------------------------------------------
/Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/pkglint: yarn build  (21 remaining)
yarn run v1.22.19
$ tsc -b && eslint . --ext=.ts && chmod +x bin/pkglint

/Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/pkglint/lib/rules.ts
  20:25  error  `../package.json` import should occur before import of `./aws-service-official-names.json`  import/order

/Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/pkglint/test/rules.test.ts
  3:1  error  `./fake-module` import should occur after import of `../lib/rules`  import/order

✖ 2 problems (2 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error: last command failed. fix problem and resume by executing: /Users/ahammond/Documents/aws/aws-cdk/scripts/foreach.sh
directory:    /Users/ahammond/Documents/aws/aws-cdk/tools/@aws-cdk/pkglint

TheRealAmazonKendra · 2023-03-01T20:52:32Z

Oh, we just added some linting checks are you're running up against, probably because your branch is from before we did that. It's annoying so gimme an hour or two and I'll resolve the issues on this PR.

TheRealAmazonKendra · 2023-03-01T21:21:47Z

@Mergifyio update

mergify · 2023-03-01T21:22:04Z

update

✅ Branch has been successfully updated

packages/@aws-cdk/aws-ecr/lib/repository.ts

ahammond · 2023-03-06T00:11:07Z

@TheRealAmazonKendra I see that the codebuild failed, however when I try to click through to see the issue I end up at an AWS login. I don't imagine I have the necessary credentials. :) Anyway, I've tried and failed to run the build and test locally and can't see the output of the failed build. I'm feeling a little bit blocked here.

ahammond · 2023-03-06T00:13:51Z

Ah, I see the build logs link above. I'll chase that down.

ahammond · 2023-03-06T00:15:01Z

@Mergifyio update

mergify · 2023-03-06T00:15:04Z

update

☑️ Nothing to do

#commits-behind>0 [:pushpin: update requirement]
-closed [:pushpin: update requirement]

packages/@aws-cdk/aws-ecr/test/repository.test.ts

ECR does not allow resource to be included in private repository resource policies. CFN largely swallows the error message. Most resources require or at least allow a resource in their policies, so we should at least warn. See issue aws#24314

Co-authored-by: Joshua Weber <[email protected]>

ahammond · 2023-03-07T21:40:05Z

@Mergifyio update

packages/@aws-cdk/aws-ecr/README.md

aws-cdk-automation · 2023-03-22T00:08:24Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

ahammond · 2023-03-22T03:35:31Z

My thought is that there’s nothing more fact based than a direct link to the issue. If it makes it easier for customers to share their voice around an issue, that’s a bonus.

On Fri, Mar 10, 2023 at 1:25 PM Joshua Weber ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/aws-ecr/README.md <#24401 (comment)>: > +a `resources` section in the `PolicyStatement`. See AWS internal ticket +`#WRITEME` for details around the ECR and Cfn team working together to @ahammond <https://github.com/ahammond> My opinion: The documentation is technical and fact-based. The purpose is not to put pressure on a team to get a desired feature. — Reply to this email directly, view it on GitHub <#24401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADM2RER3DIXGIMQW4XUL6DW3OL5RANCNFSM6AAAAAAVMKXHUI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- MiniLockID: uX4VrN5FvyFxFCxgTksGxJqvKa16iBhqseYxxA1UkZVJw GPG: 773A 6BDD 71CE 0AB8 0F5A 1176 8679 A114 FB1A 69BD

aws-cdk-automation · 2023-03-23T00:08:44Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-24T00:08:37Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-25T00:08:33Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-26T00:09:17Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-27T00:08:54Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-28T00:09:35Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-28T20:01:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-03-29T00:10:22Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

ahammond · 2023-03-30T22:30:48Z

@TheRealAmazonKendra PR updated.

aws-cdk-automation · 2023-04-03T00:08:01Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-04-04T00:08:07Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

ahammond · 2023-04-04T15:52:28Z

@TheRealAmazonKendra updated again. Please merge.

TheRealAmazonKendra

Looks good!

mergify · 2023-04-04T18:36:29Z

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-cdk-automation · 2023-04-04T19:27:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c2f9212
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-04-04T19:27:34Z

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).

github-actions bot added the p2 label Mar 1, 2023

aws-cdk-automation requested a review from a team March 1, 2023 17:23

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Mar 1, 2023

aws-cdk-automation previously requested changes Mar 1, 2023

View reviewed changes

ahammond force-pushed the warn-for-resource-in-ecr-policy branch 2 times, most recently from 4970dbe to 55450ae Compare March 1, 2023 17:40

ahammond changed the title ~~fix(aws-ecr): warn when unsupported resource is present~~ fix(ecr): warn when unsupported resource is present Mar 1, 2023

ahammond force-pushed the warn-for-resource-in-ecr-policy branch from 55450ae to 3889fe5 Compare March 1, 2023 17:44

TheRealAmazonKendra previously requested changes Mar 1, 2023

View reviewed changes

ahammond changed the title ~~fix(ecr): warn when unsupported resource is present~~ fix(ecr): policytext errors when includes resource Mar 1, 2023

TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Mar 1, 2023

daschaa reviewed Mar 5, 2023

View reviewed changes

packages/@aws-cdk/aws-ecr/lib/repository.ts Outdated Show resolved Hide resolved

daschaa approved these changes Mar 6, 2023

View reviewed changes

packages/@aws-cdk/aws-ecr/test/repository.test.ts Outdated Show resolved Hide resolved

ahammond and others added 4 commits March 6, 2023 07:49

hasWarning

421f60b

lint

73cc83a

Update packages/@aws-cdk/aws-ecr/lib/repository.ts

a71d1cc

Co-authored-by: Joshua Weber <[email protected]>

ahammond force-pushed the warn-for-resource-in-ecr-policy branch from f8d2d27 to a71d1cc Compare March 6, 2023 15:49

add principal

a4293e2

Naumel previously requested changes Mar 8, 2023

View reviewed changes

packages/@aws-cdk/aws-ecr/README.md Outdated Show resolved Hide resolved

merge main

4d26f49

ahammond force-pushed the warn-for-resource-in-ecr-policy branch from a32ca7c to 4d26f49 Compare April 4, 2023 15:43

ahammond added 2 commits April 4, 2023 08:45

Merge branch 'main' into warn-for-resource-in-ecr-policy

7007550

move comment to docstring

717fe5f

TheRealAmazonKendra approved these changes Apr 4, 2023

View reviewed changes

Merge branch 'main' into warn-for-resource-in-ecr-policy

c2f9212

mergify bot merged commit a9d6966 into aws:main Apr 4, 2023

michael-k mentioned this pull request Apr 11, 2023

(ecr): (false positive policytext errors from Python since #24401) #25028

Closed

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

paulhcsun mentioned this pull request Dec 5, 2023

aws-ecr: RepositoryPolicyText should error if includes Resource #24314

Closed

2 tasks

ahammond deleted the warn-for-resource-in-ecr-policy branch April 18, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecr): policytext errors when includes resource #24401

fix(ecr): policytext errors when includes resource #24401

ahammond commented Mar 1, 2023 •

edited

Loading

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment •

edited

Loading

ahammond commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

mergify bot commented Mar 1, 2023

ahammond commented Mar 6, 2023

ahammond commented Mar 6, 2023

ahammond commented Mar 6, 2023

mergify bot commented Mar 6, 2023

ahammond commented Mar 7, 2023

aws-cdk-automation commented Mar 22, 2023

ahammond commented Mar 22, 2023 via email

aws-cdk-automation commented Mar 23, 2023

aws-cdk-automation commented Mar 24, 2023

aws-cdk-automation commented Mar 25, 2023

aws-cdk-automation commented Mar 26, 2023

aws-cdk-automation commented Mar 27, 2023

aws-cdk-automation commented Mar 28, 2023

aws-cdk-automation commented Mar 28, 2023

aws-cdk-automation commented Mar 29, 2023

ahammond commented Mar 30, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 4, 2023

ahammond commented Apr 4, 2023

TheRealAmazonKendra left a comment

mergify bot commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

mergify bot commented Apr 4, 2023

fix(ecr): policytext errors when includes resource #24401

fix(ecr): policytext errors when includes resource #24401

Conversation

ahammond commented Mar 1, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment • edited Loading

Choose a reason for hiding this comment

ahammond commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

TheRealAmazonKendra commented Mar 1, 2023

mergify bot commented Mar 1, 2023

✅ Branch has been successfully updated

ahammond commented Mar 6, 2023

ahammond commented Mar 6, 2023

ahammond commented Mar 6, 2023

mergify bot commented Mar 6, 2023

☑️ Nothing to do

ahammond commented Mar 7, 2023

aws-cdk-automation commented Mar 22, 2023

ahammond commented Mar 22, 2023 via email

aws-cdk-automation commented Mar 23, 2023

aws-cdk-automation commented Mar 24, 2023

aws-cdk-automation commented Mar 25, 2023

aws-cdk-automation commented Mar 26, 2023

aws-cdk-automation commented Mar 27, 2023

aws-cdk-automation commented Mar 28, 2023

aws-cdk-automation commented Mar 28, 2023

aws-cdk-automation commented Mar 29, 2023

ahammond commented Mar 30, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 4, 2023

ahammond commented Apr 4, 2023

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

AWS CodeBuild CI Report

mergify bot commented Apr 4, 2023

ahammond commented Mar 1, 2023 •

edited

Loading

TheRealAmazonKendra left a comment •

edited

Loading