fix(ecr): grants for cross-account principals result in failed deployments

[skinny85]

When performing grants in ECR's Repository class for principals from other accounts, we put the ARN of the principal inside the Resource Policy of the Repository. However, ECR validates that all principals included in its Policy exist at the time of deploying the Repository, so if this cross-account principal was not created before the Repository, its deployment would fail.

Detect that situation in the Repository class, and trust the entiure account of the principal if this situation happens.

This was spotted by a customer when using the TagParameterContainerImage class.

Fixes #15070

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io[bot]]

[skinny85]

@rix0rrr I want to resurrect this old PR, if you don't mind 🙂.

I'm not sure I agree with this approach. We're actually now doing something different than what the user asked for, and they could equally well just do what we're doing, but explicitly. So we can add a warning for it, and/or add it to the docs. Or tell the user to do authz based on tags.

Either that, or we potentially add the stack dependency automatically.

But trusting the whole account "just because" it happens to work out, although it does something different than what the user requested... I don't think that's a good idea.

I'm not sure there's something else we can do. If we don't change the logic inside the ECR Repository, then, regardless of what the customer tries to do "manually" themselves, we already add the Role from the Service Stack (which doesn't exist yet) to the resource policy of the ECR repo that lives in the pipeline Stack. Which means, regardless of what else the customer wants to do there (trust the entire account, use tags for permissions, etc.), the problem is already there, and the repository will fail to deploy - so, no amount of tinkering (short of something really low-level, like escape hatches) can fix that.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED 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.

[skinny85]

@rix0rrr you want to reply here, before the PR gets closed? 🙂 https://github.com/aws/aws-cdk/pull/16376#issuecomment-1113640454

[TheRealAmazonKendra]

Psssst: If you resolve the conflicts, it resets the timer.

[skinny85]

Psssst: If you resolve the conflicts, it resets the timer.

Thanks, I really hope so, because solving these conflicts was super irritating 😃.

[Naumel]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED 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.

[skinny85]

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.

Yep. The validation error is because of missing integration tests:

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

But, this is a cross-account scenario, and CDK integration tests are meant to run in a single account.

The functionality has unit tests that confirm the cross-account scenario works as expected.

[skinny85]

@rix0rrr incorporated your comments, would appreciate another review!

[aws-cdk-automation]

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]

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.

[skinny85]

Is the build for main broken right now? This is what I get:

aws-cdk: In package package.json
aws-cdk: - [@aws-cdk/node-bundle => outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)
aws-cdk: Error: Some package.json files had errors
aws-cdk:     at main (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:30:15)
aws-cdk:     at Object.<anonymous> (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:33:1)
aws-cdk:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
aws-cdk:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
aws-cdk:     at Module.load (internal/modules/cjs/loader.js:950:32)
aws-cdk:     at Function.Module._load (internal/modules/cjs/loader.js:790:12)
aws-cdk:     at Module.require (internal/modules/cjs/loader.js:974:19)
aws-cdk:     at require (internal/modules/cjs/helpers.js:101:18)
aws-cdk:     at Object.<anonymous> (/codebuild/output/src765710805/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint:2:1)
aws-cdk:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
aws-cdk: Error: pkglint exited with error code 1
aws-cdk: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
aws-cdk: error Command failed with exit code 1.
aws-cdk: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
aws-cdk: error Command failed with exit code 1.
aws-cdk: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build+test exited 1 in 'aws-cdk'

I haven't touched the CLI in this PR, but I also had to update the third-party license attribution for two other packages to make them build, so I suspect something is up with main.

[aws-cdk-automation]

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]

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]

AWS CodeBuild CI Report

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

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

[skinny85]

OK, looks like main was fixed with regard to the 3rd-party license issues, and the build is passing.

@rix0rrr this is ready for another round of reviews!

[mergify[bot]]

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

[rix0rrr]

(Sorry for the late reply 😓 )

[skinny85]

(Sorry for the late reply 😓 )

It's all good brother ❤️. I needed a few months to address your comments, so I don't blame you at all for taking a few days to review this one!

[skinny85]

(BTW, I had to merge from main, as Mergify didn't want to add the PR to the merge queue, as it claimed there was a difference in a GitHub Actions workflow - I guess one of the workflows changed on main in the meantime... so if you could approve again, I would appreciate it!)

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify[bot]]

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