-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(eks-v2-alpha): EKS Auto Mode support #33373
base: main
Are you sure you want to change the base?
Conversation
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.
The pull request linter fails with the following errors:
❌ The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``".
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 #33373 +/- ##
=======================================
Coverage 82.20% 82.20%
=======================================
Files 119 119
Lines 6862 6862
Branches 1158 1158
=======================================
Hits 5641 5641
Misses 1118 1118
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
I like the idea using simple boolean flag to keep it simple and also keep those auto mode required fields configurable. I will align v1 implementation with this
* | ||
* @default - ['system', 'general-purpose'] | ||
*/ | ||
readonly nodePools?: string[]; |
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.
At this moment, EKS only allows to opt in system
and/or general-purpose
. For future-proofing, I am making it a string[]
with validations rather then an enum. This allows us to have the freedom to support more types in the future without BCs.
/** | ||
* Configuration for compute settings in Auto Mode. | ||
* When enabled, EKS will automatically manage compute resources. | ||
* @default - Auto Mode compute disabled |
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.
nit: the default value should be Auto Mode compute enabled
right?
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 should respect autoMode
flag
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.
I've removed autoMode
prop in favor of eks.DefaultCapacityType.AUTOMODE
* | ||
* @default - true | ||
*/ | ||
readonly autoMode?: boolean; |
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.
Do we need this flag? Customers can disable auto mode by setting defaultCapacity
or other related properties.
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.
Good callout.
As we discussed offline, AutoMode and Nodegroup can both exist. I am thinking maybe we should make then not mutual exclusive. For example
autoMode
: true || false
defaultCapacity*
: can be defined as well
Then we have all the use cases as below:
autoMode
on, noNG
autoMode
off with defaultNG
definedautoMode
on with defaultNG
definedautoMode
off with defaultNG
undefined and thencluster.addNodegroupCapacity()
to explicitly create one using the construct add* method.
Make sense?
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.
Yes, I think this makes sense!
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.
I've removed autoMode
prop in favor of eks.DefaultCapacityType.AUTOMODE
```ts | ||
// Create EKS cluster with Auto Mode explicitly disabled | ||
const cluster = new eks.Cluster(this, 'EksAutoCluster', { | ||
version: eks.KubernetesVersion.V1_32, | ||
defaultCapacity: 2 // implicitly disable Auto Mode and opt in the a nodegroup | ||
}); | ||
``` | ||
|
||
You can't opt in both Auto Mode and a default nodegroup | ||
|
||
```ts | ||
// Create EKS cluster with Auto Mode explicitly disabled | ||
const cluster = new eks.Cluster(this, 'EksAutoCluster', { | ||
version: eks.KubernetesVersion.V1_32, | ||
autoMode: true, | ||
defaultCapacity: 2 // will throw an error | ||
}); | ||
``` |
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.
These 2 cases need to be updated as NG can be created on auto mode cluster as well
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.
DONE
|
||
### Using Auto Mode | ||
|
||
By default, the Cluster construct enables EKS Auto mode. |
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.
shall we specifically mention this is the behavior for v2 CDK? as v1 might differ
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.
DONE
// attach required managed policy for the cluster role in EKS Auto Mode | ||
// see - https://docs.aws.amazon.com/eks/latest/userguide/auto-cluster-iam-role.html | ||
['AmazonEKSComputePolicy', 'AmazonEKSBlockStoragePolicy', 'AmazonEKSLoadBalancingPolicy', | ||
'AmazonEKSNetworkingPolicy', 'AmazonEKSClusterPolicy'].forEach((policyName) => { | ||
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName(policyName)); |
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.
move this to the same place when add sts:TagSession
?
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.
AmazonEKSClusterPolicy already exist in the base role, will add it throw error or just no-op?
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.
no-op but yeah let's remove the dup
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.
DONE
managedPolicies: [ | ||
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEKSWorkerNodePolicy'), | ||
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEC2ContainerRegistryReadOnly'), | ||
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEKS_CNI_Policy'), |
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.
I don't think we need this policy, referring to this doc https://docs.aws.amazon.com/eks/latest/userguide/automode-get-started-cli.html#auto-mode-create-roles
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.
DONE
enabled: !autoModeEnabled ? false : true, | ||
// If the computeConfig enabled flag is set to false when creating a cluster with Auto Mode, | ||
// the request must not include values for the nodeRoleArn or nodePools fields. | ||
nodePools: !autoModeEnabled ? undefined : props.compute?.nodePools ?? ['system', 'general-purpose'], | ||
nodeRoleArn: !autoModeEnabled ? undefined : props.compute?.nodeRole?.roleArn ?? this.addNodePoolRole(`${id}nodePoolRole`).roleArn, |
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.
If computeConfig
is specified while autoMode
is disabled, does it give an error, a warning or just ignoring the property?
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.
Well, I think it's weird to have
defaultCapacityType.NODEGROUP
with computeConfig.nodePools
or nodeRole
defined
Let's throw and block them.
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.
DONE with more tests added.
}); | ||
``` | ||
|
||
### Hybrid Mode with Auto Mode and Node Groups |
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.
The Hybrid Mode
might confuse customer as there is another feature called Hybrid Nodes
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.
nit: there are duplicate cases and some contradictory cases, pending clean up
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.
DONE. Let me know if there's still anything missing.
* When enabled, EKS will automatically manage networking resources. | ||
* @default - Auto Mode networking disabled | ||
*/ | ||
readonly kubernetesNetwork?: KubernetesNetworkConfig; |
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.
IpFamily
and ServiceIpv4Cidr
are currently flattened properties. In the L1 resource, they are in KubernetesNetworkConfig. I think we should be consistent here. Either moving them to KubernetesNetworkConfig
or making ElasticLoadBalancingConfig
a direct property.
@@ -1055,6 +1125,49 @@ export class Cluster extends ClusterBase { | |||
], | |||
}); | |||
|
|||
const autoModeEnabled = !(props.defaultCapacityType !== undefined && props.defaultCapacityType !== DefaultCapacityType.AUTOMODE); |
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.
const autoModeEnabled = !(props.defaultCapacityType !== undefined && props.defaultCapacityType !== DefaultCapacityType.AUTOMODE); | |
const autoModeEnabled = !props.defaultCapacityType || props.defaultCapacityType == DefaultCapacityType.AUTOMODE; |
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.
thanks but I think this will be even better
props.defaultCapacityType === undefined || props.defaultCapacityType == DefaultCapacityType.AUTOMODE
-
Explicit Intent: It clearly communicates that we're specifically checking for undefined. The ! operator can be ambiguous as it will convert various falsy values (undefined, null, 0, "", false) to true.
-
Type Safety: In TypeScript especially, explicit undefined checks are preferred as they make type inference more accurate. The compiler can better understand your intent.
-
Prevents Bugs: Using ! could accidentally match other falsy values like null, 0, or empty string, which might not be what you want. The explicit check ensures you're only matching undefined.
const autoModeEnabled = !(props.defaultCapacityType !== undefined && props.defaultCapacityType !== DefaultCapacityType.AUTOMODE); | ||
|
||
// Throw when using nodePools or nodeRole without using AUTOMODE | ||
if (!autoModeEnabled && (props.compute?.nodePools || props.compute?.nodeRole)) { |
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.
can this be simplified by checking if (!autoModeEnabled && props.compute)
? The error message could be updated as well.
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.
Makes sense! But there is a risk if eks team creates a new compute config which can be used when autoMode is disabled. The suggested code change might be a problem?
}, | ||
storageConfig: { | ||
blockStorage: { | ||
enabled: !autoModeEnabled ? false : true, |
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.
can blockStorage
be disabled
when auto mode is enabled
?
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.
No, all the three booleans have to be all enabled or all disabled. We learn this from cloudformation errors.
if (autoModeEnabled && (props.defaultCapacity !== undefined || props.defaultCapacityInstance !== undefined)) { | ||
throw new Error('Cannot specify defaultCapacity or defaultCapacityInstance when using Auto Mode. Auto Mode manages compute resources automatically.'); | ||
} | ||
|
||
// Node pool values are case-sensitive and must be general-purpose and/or system | ||
if (props.compute?.nodePools) { | ||
const validNodePools = ['general-purpose', 'system']; | ||
const invalidPools = props.compute.nodePools.filter(pool => !validNodePools.includes(pool)); | ||
if (invalidPools.length > 0) { | ||
throw new Error(`Invalid node pool values: ${invalidPools.join(', ')}. Valid values are: ${validNodePools.join(', ')}`); | ||
} | ||
} |
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.
We can move these to if (autoModeEnabled)
below
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.
DONE
Co-authored-by: Xia Zhao <78883180+xazhao@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Address #32364 in aws-eks-v2-alpha.
For EKS Auto Mode, all required configs, including
computeConfig
,kubernetesNetworkConfig
, andblockStorage
are managed through thedefaultCapacityType
enum. When set toDefaultCapacityType.AUTOMODE
(which is the default), these configurations are automatically enabled. TheCluster
construct in aws-eks-v2-alpha enables EKS Auto Mode by default, managing compute resources through node pools instead of creating default capacity or nodegroups. Users can still opt-in to traditional nodegroup management by settingdefaultCapacityType
toNODEGROUP
orEC2
.User Experience:
Update Summary
Cluster
construct in V2. When enabled:defaultCapacity
as aNODEGROUP
(major difference from aws-eks module)outputConfigCommand
support previously inaws-eks
moduleDescription of how you validated changes
On deploy the autoMode enabled cluster using the code above.
% kubectl create deployment nginx --image=nginx % kubectl get events --sort-by='.lastTimestamp'
verify the nodes and pods
Checklist
References
eksctl YAML experience
Terraform experience:
Pulumi experience
Links
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license