feat(eks): make kubectlLayer property required from optional #32930

xazhao · 2025-01-15T01:22:34Z

Issue

Reason for this change

aws-cdk-lib has a very outdated version of kubectl layer as dependency https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/package.json#L123. It uses an outdated helm version which is involved in a CVE.

The dependency was added because if users do not provide a kubectl layer version for EKS cluster, it will use v20 as the default. CDK itself shouldn't use a specific version of kubectl layer as dependency.

To remove the dependency, kubectlLayer will become a required property instead of optional. The default version v20 is too old to work with current EKS supported version v24+. However, if you're not using the property, you will see an error saying it's a required property. Please uses a kubectl layer version that's compatible with your cluster.

Description of changes

Make the property required from options
Update unit tests and integration tests
Remove the dependency of "@aws-cdk/asset-kubectl-v20": "^2.1.3"

Describe any new or updated permissions being added

Description of how you validated changes

unit tests/integration tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

BREAKING CHANGE: kubectlLayer property is now required in EKS Cluster and FargateCluster constructs. The default value for kubectlLayer is outdated and hence being removed. You can specify your own kubectlLayer version based on your Kubernetes version.

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

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.

codecov · 2025-01-15T01:42:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.40%. Comparing base (3fbc785) to head (4275cea).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32930   +/-   ##
=======================================
  Coverage   81.40%   81.40%           
=======================================
  Files         223      223           
  Lines       13727    13727           
  Branches     2411     2411           
=======================================
  Hits        11175    11175           
  Misses       2274     2274           
  Partials      278      278

Flag	Coverage Δ
suite.unit	`81.40% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.74% <ø> (ø)`
packages/aws-cdk-lib/core	`82.10% <ø> (ø)`

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

github-actions · 2025-01-15T23:42:36Z

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.

aws-cdk-automation

(This review is outdated)

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

This reverts commit 2e6f4ff.

aws-cdk-automation

(This review is outdated)

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

GavinZZ

Left some comments to help me understand.

GavinZZ · 2025-02-12T23:34:14Z

packages/@aws-cdk-testing/framework-integ/test/lambda-layer-kubectl/test/integ.kubectl-layer.ts


 /**
 * Test verifies that kubectl and helm are invoked successfully inside Lambda runtime.
 */

 const app = new cdk.App();
 const stack = new cdk.Stack(app, 'lambda-layer-kubectl-integ-stack');
-const layer = new KubectlLayer(stack, 'KubectlLayer');
+const layer = new KubectlV31Layer(stack, 'KubectlLayer');


For my understanding, why do we need to change this test file? Does it not work with KubectlLayer?
edit: nvm, the old layer version is outdated and have securtity concerns.

KubectlLayer construct is being removed completely from aws-cdk-lib repo. We should import it from @aws-cdk if required.

GavinZZ · 2025-02-12T23:36:02Z

packages/aws-cdk-lib/aws-eks/lib/cluster.ts

@@ -144,7 +144,6 @@ export interface ICluster extends IResource, ec2.IConnectable {
  /**
   * An AWS Lambda layer that includes `kubectl` and `helm`
   *
-   * If not defined, a default layer will be used containing Kubectl 1.20 and Helm 3.8


Why did we choose to make this resource requried instead of coming up with a feature flag and change the default layer to a valid layer?

The motivation is that old version has some security risks and should be removed from CDK dependency. So adding a feature flag doesn't solve this issue.

Changing the default layer is also a breaking change. After discussion, an error during synth is better than deploying a new version and then find something is wrong. See cdklabs/awscdk-asset-kubectl#1534 (comment)

GavinZZ · 2025-02-12T23:41:16Z

packages/aws-cdk-lib/aws-eks/lib/kubectl-provider.ts

@@ -150,7 +149,9 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {

    // allow user to customize the layers with the tools we need
    handler.addLayers(props.cluster.awscliLayer ?? new AwsCliLayer(this, 'AwsCliLayer'));
-    handler.addLayers(props.cluster.kubectlLayer ?? new KubectlLayer(this, 'KubectlLayer'));


Similar question to above: Why don't we keep doing this but use a newer version?

See my comment above.

GavinZZ

Based on offline discussion yesterday, I think this change looks good to me.

aws-cdk-automation · 2025-02-13T19:16:22Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation requested a review from a team January 15, 2025 01:22

github-actions bot added the p2 label Jan 15, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 15, 2025

aws-cdk-automation previously requested changes Jan 15, 2025

View reviewed changes

aws deleted a comment from aws-cdk-automation Jan 15, 2025

xazhao added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 15, 2025

xazhao closed this Jan 15, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2025

xazhao reopened this Jan 23, 2025

xazhao added 2 commits January 30, 2025 16:35

feat(eks): make kubectlLayer property required from optional

7020781

update tests

6f15da8

xazhao force-pushed the remove-default-kubectlLayer branch from dc8d9d0 to 6f15da8 Compare January 31, 2025 01:04

xazhao marked this pull request as ready for review January 31, 2025 01:04

xazhao and others added 7 commits January 30, 2025 17:04

Merge branch 'main' into remove-default-kubectlLayer

423f544

nit

514fb79

Update integration tests

2e6f4ff

add to allowed breaking change list

fc93ffa

Merge branch 'main' into remove-default-kubectlLayer

b3e3a4f

add breaking change text

56dc770

update readme

fc79e17

mrgrain changed the title ~~feat(eks): make kubectlLayer property required from optional~~ feat(eks)!: make kubectlLayer property required from optional Feb 4, 2025

aws-cdk-automation previously requested changes Feb 4, 2025

View reviewed changes

mrgrain changed the title ~~feat(eks)!: make kubectlLayer property required from optional~~ feat(eks): make kubectlLayer property required from optional Feb 4, 2025

godwingrs22 assigned godwingrs22 and unassigned godwingrs22 Feb 7, 2025

Revert "Update integration tests"

e7963b1

This reverts commit 2e6f4ff.

xazhao and others added 5 commits February 10, 2025 16:55

Merge branch 'main' into remove-default-kubectlLayer

f4b2726

update readme

e6887fe

update integ tests

0caaeac

update readme

f2288d9

update readme again

934537e

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 11, 2025

aws-cdk-automation previously requested changes Feb 11, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 11, 2025

Merge branch 'main' into remove-default-kubectlLayer

4cf6ffb

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 11, 2025

xazhao and others added 5 commits February 11, 2025 20:26

optimize

8a8738f

nit

b417593

Merge branch 'main' into remove-default-kubectlLayer

d051357

Merge branch 'main' into remove-default-kubectlLayer

4e35559

update readme

9a5cc95

GavinZZ self-assigned this Feb 12, 2025

GavinZZ requested changes Feb 13, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 13, 2025

GavinZZ approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'main' into remove-default-kubectlLayer

366d088

mergify bot merged commit b11f663 into aws:main Feb 13, 2025
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks): make kubectlLayer property required from optional #32930

feat(eks): make kubectlLayer property required from optional #32930

xazhao commented Jan 15, 2025 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

GavinZZ left a comment •

edited

Loading

GavinZZ Feb 12, 2025 •

edited

Loading

xazhao Feb 13, 2025

GavinZZ Feb 12, 2025

xazhao Feb 13, 2025

GavinZZ Feb 12, 2025

xazhao Feb 13, 2025

GavinZZ left a comment

aws-cdk-automation commented Feb 13, 2025

feat(eks): make kubectlLayer property required from optional #32930

feat(eks): make kubectlLayer property required from optional #32930

Conversation

xazhao commented Jan 15, 2025 • edited Loading

Issue

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 15, 2025

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

GavinZZ left a comment • edited Loading

Choose a reason for hiding this comment

GavinZZ Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

xazhao Feb 13, 2025

Choose a reason for hiding this comment

GavinZZ Feb 12, 2025

Choose a reason for hiding this comment

xazhao Feb 13, 2025

Choose a reason for hiding this comment

GavinZZ Feb 12, 2025

Choose a reason for hiding this comment

xazhao Feb 13, 2025

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 13, 2025

AWS CodeBuild CI Report

xazhao commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

GavinZZ left a comment •

edited

Loading

GavinZZ Feb 12, 2025 •

edited

Loading