Skip to content

Conversation

@pahud
Copy link
Contributor

@pahud pahud commented Aug 15, 2025

Issue

Closes #35244

Problem Statement

The current KubectlProvider implementation uses the AmazonEC2ContainerRegistryReadOnly managed policy for ECR access, which has two key limitations:

  1. Too broad: Includes unnecessary permissions like ecr:ListImages that kubectl doesn't require
  2. Too narrow: Missing the ecr:BatchImportUpstreamImage action required for ECR pull-through cache functionality

This prevents users from leveraging ECR pull-through caches when installing Helm charts from ECR repositories via the CDK.

Solution

Replace AmazonEC2ContainerRegistryReadOnly with AmazonEC2ContainerRegistryPullOnly in the KubectlProvider's IAM role. The AmazonEC2ContainerRegistryPullOnly policy:

• Provides the exact permissions needed for container image pulling
• Includes support for ecr:BatchImportUpstreamImage enabling pull-through cache functionality
• Follows the principle of least privilege by removing unnecessary permissions

Changes Made

• Updated kubectl-provider.ts to use AmazonEC2ContainerRegistryPullOnly instead of AmazonEC2ContainerRegistryReadOnly
• Regenerated integration test snapshots to reflect the policy change across all affected EKS test cases

Impact

Enables ECR pull-through cache support for Helm chart installations via kubectl
Improves security posture by applying principle of least privilege
Maintains backward compatibility - all existing functionality continues to work
No breaking changes - this is a drop-in replacement with enhanced capabilities

Testing

• All existing integration tests pass with updated snapshots
• The change affects multiple EKS integration test scenarios including ALB controller, custom addons, and various node group
configurations
• Verified that the new policy provides all necessary permissions for kubectl operations

References

AmazonEC2ContainerRegistryPullOnly Policy Documentation
AmazonEC2ContainerRegistryReadOnly Policy Documentation


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

@aws-cdk-automation aws-cdk-automation requested a review from a team August 15, 2025 17:43
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Aug 15, 2025
@pahud pahud changed the title fix(eks): eks kubectlProvider should use the AmazonEC2ContainerRegistryPullOnly managed policy fix(eks): kubectlProvider should use the AmazonEC2ContainerRegistryPullOnly managed policy Aug 15, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 15, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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)

@aws-cdk-automation aws-cdk-automation dismissed their stale review August 16, 2025 09:50

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

@pahud pahud marked this pull request as ready for review August 16, 2025 15:15
Copy link
Contributor

@kumvprat kumvprat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required to set : '@aws-cdk/aws-lambda:useCdkManagedLogGroup': false,

Since the integ test stacks deploy as new stacks, we should leave it at default value to test out that change in behaviour holds with the default feature flag values.

postCliContext: {
[IAM_OIDC_REJECT_UNAUTHORIZED_CONNECTIONS]: false,
'@aws-cdk/aws-lambda:useCdkManagedLogGroup': false,
'@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy': false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For feature flag : '@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy' the default value seems to be false
Setting it explicitly shouldn't be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. All removed.

const app = new App({
context: {
'@aws-cdk/aws-lambda:useCdkManagedLogGroup': false,
'@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy': false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above
For feature flag : '@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy' the default value seems to be false
Setting it explicitly shouldn't be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. All removed.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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)

@kumvprat kumvprat added the needs-security-review Related to feature or issues that needs security review label Aug 19, 2025
@pahud
Copy link
Contributor Author

pahud commented Aug 19, 2025

Exemption request

Mostly calling out that no integ test changes are needed as the change is in a policy being used, not the exact functionality of the module.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Aug 19, 2025
@pahud
Copy link
Contributor Author

pahud commented Aug 19, 2025

Clarification Request

please clarify why snapshots update is still required

@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Aug 19, 2025
@kumvprat kumvprat added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 20, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 20, 2025 13:03

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

@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2025

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

@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2025

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

@mergify mergify bot merged commit 1b6e962 into aws:main Aug 20, 2025
17 of 18 checks passed
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-security-review Related to feature or issues that needs security review p1 pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eks: KubectlProvider should use the AmazonEC2ContainerRegistryPullOnly managed policy

3 participants