- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.3k
feat(ecr-assets): add custom ECR repository and image tagging sup… #35251
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
base: main
Are you sure you want to change the base?
feat(ecr-assets): add custom ECR repository and image tagging sup… #35251
Conversation
…port This commit implements custom ECR repository support for Docker Image Assets, addressing issue aws#33228. The implementation adds three new optional properties to DockerImageAssetOptions: - ecrRepository: Allows specifying a custom ECR repository instead of using the default synthesizer repository - imageTag: Enables setting explicit custom tags for Docker images - imageTagPrefix: Allows adding a prefix to the asset hash for image tags Key Features: - Full backward compatibility with existing code - Proper asset hash invalidation when properties change - Tag precedence: imageTag > imageTagPrefix > assetHash - Support for custom repositories with custom tagging - Comprehensive test coverage with unit and integration tests Implementation Details: - Three execution paths: custom repository, custom tagging, default behavior - Helper method for custom tag synthesis with default repositories - Enhanced hash calculation including new properties - Updated README with usage examples and migration guide Tests: - 10 comprehensive unit tests covering all functionality - Integration tests with real-world usage examples - Test fixtures with complete Docker application - Backward compatibility verification Resolves: aws#33228
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
…port - Add comprehensive integration test demonstrating custom ECR repository usage - Include test snapshots for PR linter validation - Test custom repository, imageTag, and imageTagPrefix features - Verify precedence handling between imageTag and imageTagPrefix Addresses aws#33228
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @oyiz-michael , thank you for working on this! There are a few changes needed.
Pull request has been modified.
| 
 | 
Co-authored-by: A. Abdel-Rahman <[email protected]>
Pull request has been modified.
- Fix prefer-const ESLint error in image-asset.ts - Update integration test for custom repository feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
- Remove integ.custom-repository.ts from aws-cdk-lib/aws-ecr-assets/test/ - Remove associated fixtures directory - Integration test already exists in proper location at @aws-cdk-testing/framework-integ - Resolves circular dependency: framework-integ -> integ-tests-alpha -> aws-cdk-lib -> integ-tests-alpha
…ion test file - Rename dependencies-pnpm.ts to dependencies-pnpm.ts.deactivated - File was commented out but still causing TypeScript parser to detect integ-tests dependency - Resolves circular dependency: aws-cdk-lib:build -> @aws-cdk/integ-tests-alpha:build -> aws-cdk-lib:build - Build system now passes without circular dependency errors
Pull request has been modified.
When using custom tags with the default repository, the synthesizer returns imageUri with CDK tokens (like Asset1$1). The previous implementation would return the base location unchanged when tokens were detected, preventing custom tags from being applied. This fix ensures that even when imageUri contains tokens, the custom imageTag is still properly set in the returned location.
Add imageTag and imageTagPrefix properties to DockerImageAssetSource to enable per-asset custom tags. This allows meaningful Docker image tagging for security scanning (AWS Inspector) and image management, addressing the core use case from aws#33228. The implementation properly extends the CDK synthesizer architecture by: 1. Adding optional imageTag/imageTagPrefix to DockerImageAssetSource interface 2. Updating AssetManifestBuilder to respect per-asset tag overrides 3. Passing custom tags through synthesizer flow for both custom and default repos This solution follows CDK patterns and avoids breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor editing the manifest builder directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is yet another lint failure:
aws-cdk-lib: /codebuild/output/src2987853504/src/actions-runner/_work/aws-cdk/aws-cdk/packages/aws-cdk-lib/core/lib/stack-synthesizers/asset-manifest-builder.ts
aws-cdk-lib:   66:1  error  Trailing spaces not allowed  no-trailing-spaces
Use yarn lint --fix whenever possible to avoid this. See https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#linters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint passes but the integration test doesn't. Following snapshot needs to be updated:
@aws-cdk-testing/framework-integ:   CHANGED    aws-ecr-assets/test/integ.custom-repository 1.021s
@aws-cdk-testing/framework-integ:       Outputs
@aws-cdk-testing/framework-integ:       [~] Output BasicCustomAssetUri: {"Value":{"Fn::Join":["",[{"Fn::Select":[4,{"Fn::Split":[":",{"Fn::GetAtt":["CustomRepo98CFBAD2","Arn"]}]}]},".dkr.ecr.",{"Fn::Select":[3,{"Fn::Split":[":",{"Fn::GetAtt":["CustomRepo98CFBAD2","Arn"]}]}]},".",{"Ref":"AWS::URLSuffix"},"/",{"Ref":"CustomRepo98CFBAD2"},":Asset1$1"]]}} to {"Value":{"Fn::Join":["",[{"Fn::Select":[4,{"Fn::Split":[":",{"Fn::GetAtt":["CustomRepo98CFBAD2","Arn"]}]}]},".dkr.ecr.",{"Fn::Select":[3,{"Fn::Split":[":",{"Fn::GetAtt":["CustomRepo98CFBAD2","Arn"]}]}]},".",{"Ref":"AWS::URLSuffix"},"/",{"Ref":"CustomRepo98CFBAD2"},":2aac91d10652d5f4ee8e2b9135e10afb5c6ca351007e0b0c3109bf754ea46224"]]}}
@aws-cdk-testing/framework-integ:       [~] Output PrefixedCustomAssetTag: {"Value":"branch-main-Asset1$1"} to {"Value":"branch-main-4766dd6ca48737524a7667b729a979f74792c72f39366e460813c9a040303dd3"}
@aws-cdk-testing/framework-integ:       [~] Output DefaultRepoPrefixedTag: {"Value":"feature-Asset1$1"} to {"Value":"feature-Asset2$1"}
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:
Pull request has been modified.
…rties Reverts earlier change that removed ecrRepository, imageTag, and imageTagPrefix from the asset hash calculation. While these don't directly affect Docker image content, they need to be included in the hash for consistent asset identification and to avoid breaking changes in asset naming.
| @oyiz-michael I'll admit I'm asking this without looking much into the code, but how does this work when you're creating the repo in the same application that the image is being built for? My understanding is CDK publishes all assets before the stack deployment even begins, hence the CDK behavior to use a bootstrapped repo that must be there before making any deployments. Again, maybe just a naive view but clearly trying to publish to a repo that hasn't been created yet would be problematic. | 
| From the PR description, it assumes that there is an ECR repository created outside of the CDK application: 
 So the use case for this PR is for using an externally created repository before deploying the CDK docker assets. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECR assets mirror S3 assets by current design.
There is no "push this S3 file to a specific bucket with a specific filename" feature for files; I don't see convincing reasons why ECR assets should be different.
I know the lifetime management concerns are different, and that there are improvements to be made. However, that's a discussion we should have with a good all-around proposal and design first, preferably via RFC, rather than patching in an option here and an option there in PRs.
If your goal is:
- I want to have an image uploaded a well-known ECR repository with a fixed name (perhaps so that a different app/team can consume it from its well-known location): use cdk-docker-image-deployment
- I want to clean up resources on a per-app basis: use the app staging synthesizer
- I want to put images into custom places that already exist and are managed externally to my app: write a custom stack synthesizer
- I have a good idea for how ECR asset handling should be done (taking into account CLI deployments, pipeline deployments, life cycle management, and repository cleanup): submit an RFC
| @oyiz-michael Thanks. I think the example in the README.md is misleading then if an externally created repo is assumed: import * as ecr from 'aws-cdk-lib/aws-ecr';
// Custom ECR repository
const customRepo = new ecr.Repository(this, 'MyRepo', {
  repositoryName: 'my-custom-repo'
});
const asset = new DockerImageAsset(this, 'MyAsset', {
  directory: path.join(__dirname, 'my-image'),
  ecrRepository: customRepo,           // Use custom repository
  imageTag: 'v1.2.3',                 // Custom tag
  // OR
  imageTagPrefix: 'feature-branch-',   // Tag prefix + asset hash
});@rix0rrr I think you pretty much covered the desired use cases, but we should acknowledge: 
 Two reasons that likely contribute to the popularity of the issue referenced from this PR. | 
…ories - Make it explicit that repositories must exist before deployment - Show proper usage with Repository.fromRepositoryArn/fromRepositoryName - Remove misleading example that creates repo in same app - Add separate section for custom tags with CDK-managed repos - Reorganize alternative solutions section for better clarity - Address feedback from PR reviewers @AdamVD and @rix0rrr
…anges Update asset hashes in integ.custom-repository snapshots to reflect README.md changes included in Docker build context. Hash changes: - BasicCustomAssetUri: b4832ce0... → 05184bb3... - PrefixedCustomAssetTag: c24e5555... → 0750f79e...
| Hi @oyiz-michael ! Since the PR is modifying the core ECR assets interface, an RFC will be needed as @rix0rrr suggested: 
 Unfortunately, I can't accept this PR until an RFC is created and accepted for this change. I apologize for the inconvenience this creates, but this ensures no backwards-incompatible change or design flaw is inadvertently made from this PR. The guide to submit an RFC can be read here: https://github.com/aws/aws-cdk-rfcs?tab=readme-ov-file#what-is-an-rfc | 
| This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure: 
 This PR will automatically close in 14 days if no action is taken. | 
Custom ECR Repository Support for Docker Image Assets
Issue # (if applicable)
Closes #33228
Reason for this change
Currently,
DockerImageAssetonly supports using the default ECR repository managed by the CDK synthesizer, and image tags are limited to the asset hash or global synthesizer prefixes. This limitation prevents developers from:v1.2.3) or branch-based tagging (e.g.,main-abc123) instead of just asset hashesThis has been a highly requested feature with 39+ community upvotes and is marked as P1 priority.
Description of changes
This PR adds three new optional properties to
DockerImageAssetOptions:New Properties:
ecrRepository?: ecr.IRepository- Allows specifying a custom ECR repository instead of using the default synthesizer repositoryimageTag?: string- Enables setting explicit custom tags for Docker images (e.g.,'v1.2.3','latest')imageTagPrefix?: string- Allows adding a prefix to the asset hash for image tags (e.g.,'feature-'→'feature-abc123')Implementation Details:
1. Three Execution Paths:
ecrRepositoryis provided, bypasses synthesizer and uses the custom repository directlyimageTagorimageTagPrefixis provided, uses default repository with custom tag via helper method2. Tag Precedence Logic:
Priority:
imageTag>imageTagPrefix + assetHash>assetHash3. Enhanced Asset Hash Calculation:
4. Helper Method for Custom Tagging:
addDockerImageAssetWithCustomTag()- Integrates with synthesizer for default repository but overrides tag generationCode Changes:
Core Implementation (
image-asset.ts):DockerImageAssetOptionsinterface with 3 new optional propertiesDocumentation (
README.md):repositoryNamepropertyTesting:
custom-repository.test.ts): 10 comprehensive tests covering all functionalityinteg.custom-repository.ts): Real-world usage examples with CloudFormation outputsfixtures/custom-dockerfile/): Complete Docker application for testingAlternatives Considered:
Design Decisions:
imageTagwins overimageTagPrefixfor predictable behaviorDescribe any new or updated permissions being added
No new IAM permissions are required.
When users specify a custom ECR repository via
ecrRepository, they become responsible for ensuring their CDK execution role has appropriate permissions to push to that repository. The implementation uses existing ECR repository methods (repositoryUriForTag) and synthesizer patterns.Typical permissions needed for custom repositories (user's responsibility):
ecr:GetAuthorizationTokenecr:BatchCheckLayerAvailabilityecr:GetDownloadUrlForLayerecr:BatchGetImageecr:DescribeRepositoriesecr:InitiateLayerUploadecr:UploadLayerPartecr:CompleteLayerUploadecr:PutImageDescription of how you validated changes
Unit Testing:
custom-repository.test.ts:imageTag>imageTagPrefix)Integration Testing:
integ.custom-repository.ts):exportValueCompatibility Testing:
DockerImageAssetusage patterns continue to workLogic Testing:
Manual Testing:
Usage Examples
Basic Custom Repository:
Custom Tags:
Tag Prefixes:
Combined Usage:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license