feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for EKS Cluster #32400

xazhao · 2024-12-06T01:04:55Z

Reason for this change

Get rid of the custom resource to provision EKS cluster by using L1 CfnCluster.

Description of changes

Replace custom resource with native L1 CfnCluster

Description of how you validated changes

unit tests and integration tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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.

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

codecov · 2024-12-06T01:17:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (59e96a3) to head (1fc5fda).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32400   +/-   ##
=======================================
  Coverage   78.67%   78.67%           
=======================================
  Files         107      107           
  Lines        7237     7237           
  Branches     1329     1329           
=======================================
  Hits         5694     5694           
  Misses       1357     1357           
  Partials      186      186

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

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

Components	Coverage Δ
packages/aws-cdk	`78.67% <ø> (ø)`

pahud · 2024-12-06T01:26:36Z

packages/@aws-cdk/aws-eks-v2-alpha/lib/cluster.ts

    if (props.serviceIpv4Cidr && props.ipFamily == IpFamily.IP_V6) {
      throw new Error('Cannot specify serviceIpv4Cidr with ipFamily equal to IpFamily.IP_V6');
    }

    this.authenticationMode = props.authenticationMode;

-    const resource = this._clusterResource = new ClusterResource(this, 'Resource', {
+    const resource = this._clusterResource = new CfnCluster(this, 'Resource', {


Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The Kubernetes API server for the Amazon EKS cluster is exposed if endpointAccess is set to EndpointAccess.PUBLIC in the Cluster resource or endpointPublicAccess is true in the CfnCluster resource. This configuration allows unauthorized internet access, increasing security risks such as data breaches and malicious activities. To secure the cluster, set endpointAccess to EndpointAccess.PRIVATE in the Cluster resource and endpointPublicAccess to false in the CfnCluster resource. Implement network access controls to restrict access to trusted sources within the AWS VPC. For more information refer to the documentation https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-eks.EndpointAccess.html.

iliapolo

I'm seeing some files I didn't expect changing, but I can't see the change because the PR is too big. Lets talk about this.

GavinZZ

Generally looks good as most of the changes are tests changes and files related to custom resource removal. A few questions:

In the design document, we talked about updating fromKubectlProviderAttributes. I don't see the change here, is this intended for a separate PR?
We also talked about updating ICluster interface to remove the unused properties but I don't see that here either.

packages/@aws-cdk/aws-eks-v2-alpha/lib/cluster.ts

xazhao · 2024-12-09T19:12:19Z

@GavinZZ Thanks for reviewing. Yes those changes will be in separate PRs.

…EKS Cluster

xazhao · 2024-12-10T20:14:29Z

Synced with @iliapolo offline. Will remove the integration tests from the repo and add a few TODOs to the code

mergify · 2024-12-10T22:28:12Z

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 · 2024-12-11T06:45:18Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2024-12-11T07:05:39Z

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

aws-cdk-automation · 2024-12-11T08:06:08Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-11T08:06:32Z

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

github-actions · 2024-12-11T08:06:54Z

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 requested a review from a team December 6, 2024 01:05

github-actions bot added the p2 label Dec 6, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 6, 2024

aws-cdk-automation previously requested changes Dec 6, 2024

View reviewed changes

xazhao added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 6, 2024

pahud reviewed Dec 6, 2024

View reviewed changes

xazhao marked this pull request as ready for review December 6, 2024 01:43

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 6, 2024

iliapolo requested changes Dec 9, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 9, 2024

GavinZZ reviewed Dec 9, 2024

View reviewed changes

packages/@aws-cdk/aws-eks-v2-alpha/lib/cluster.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-eks-v2-alpha/lib/cluster.ts Show resolved Hide resolved

packages/@aws-cdk/aws-eks-v2-alpha/lib/cluster.ts Outdated Show resolved Hide resolved

feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for …

6a49ba7

…EKS Cluster

xazhao force-pushed the cluster-l1 branch from 696aaf4 to 6a49ba7 Compare December 10, 2024 00:39

Merge branch 'main' into cluster-l1

0be88db

iliapolo added the pr/do-not-merge This PR should not be merged at this time. label Dec 10, 2024

iliapolo approved these changes Dec 10, 2024

View reviewed changes

GavinZZ approved these changes Dec 10, 2024

View reviewed changes

xazhao added 2 commits December 10, 2024 13:22

Remove integration tests

a7d3ae8

minor adjustment

5d7685d

xazhao removed the pr/do-not-merge This PR should not be merged at this time. label Dec 10, 2024

Merge branch 'main' into cluster-l1

ac7582b

wip

1fc5fda

mergify bot merged commit d0a4a78 into aws:main Dec 11, 2024
17 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for EKS Cluster #32400

feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for EKS Cluster #32400

xazhao commented Dec 6, 2024

aws-cdk-automation left a comment

codecov bot commented Dec 6, 2024 •

edited

Loading

pahud Dec 6, 2024

iliapolo left a comment

GavinZZ left a comment

xazhao commented Dec 9, 2024

xazhao commented Dec 10, 2024

mergify bot commented Dec 10, 2024

mergify bot commented Dec 11, 2024

mergify bot commented Dec 11, 2024

aws-cdk-automation commented Dec 11, 2024

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for EKS Cluster #32400

feat(eks-v2-alpha): use L1 CfnCluster to replace custom resource for EKS Cluster #32400

Conversation

xazhao commented Dec 6, 2024

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

pahud Dec 6, 2024

Choose a reason for hiding this comment

iliapolo left a comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

xazhao commented Dec 9, 2024

xazhao commented Dec 10, 2024

mergify bot commented Dec 10, 2024

mergify bot commented Dec 11, 2024

The merge conditions cannot be satisfied due to failing checks:

mergify bot commented Dec 11, 2024

aws-cdk-automation commented Dec 11, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading