feat(eks): make kubectlLayer property required from optional#32930
feat(eks): make kubectlLayer property required from optional#32930mergify[bot] merged 22 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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 ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Comments on closed issues and PRs are hard for our team to see. |
dc8d9d0 to
6f15da8
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
This reverts commit 2e6f4ff.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| 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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
KubectlLayer construct is being removed completely from aws-cdk-lib repo. We should import it from @aws-cdk if required.
| /** | ||
| * 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
|
|
||
| // 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')); |
There was a problem hiding this comment.
Similar question to above: Why don't we keep doing this but use a newer version?
There was a problem hiding this comment.
See my comment above.
GavinZZ
left a comment
There was a problem hiding this comment.
Based on offline discussion yesterday, I think this change looks good to me.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue
#33261
Reason for this change
aws-cdk-libhas 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,
kubectlLayerwill 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
"@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
BREAKING CHANGE:
kubectlLayerproperty is now required in EKSClusterandFargateClusterconstructs. The default value forkubectlLayeris 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