-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
(aws-eks): Only one type of update can be allowed with updateVersion #33452
Comments
Yes, I can see the bug now. The issue is in how the code validates multiple updates: type UpdateTypes = {
updateLogging: boolean;
updateAccess: boolean;
updateVpc: boolean;
updateAuthMode: boolean;
}; But when it checks for multiple updates, it's using Object.keys(updates) which gets ALL keys from the updates object, including:
This causes the validation to incorrectly count ALL update types, not just the four that should be mutually exclusive (logging, access, vpc, auth mode). The fix would be to explicitly check only the relevant update types: const relevantUpdateTypes = ['updateLogging', 'updateAccess', 'updateVpc', 'updateAuthMode'];
const enabledUpdateTypes = relevantUpdateTypes.filter(type => updates[type as keyof UpdateMap]); This way, version updates and other non-conflicting updates would not be included in the validation check. Making this a p1 and we welcome PRs. |
Thanks for confirming that this is indeed a bug. I can work on this as it looks like to be a pretty easy fix. |
@ariksidney yes that would be awesome! I am requesting the team to confirm this but feel free to draft the PR if you like. |
@pahud It looks like the eks update API does also not allow version and AuthenticationMode update at the same time. Can you confirm that? I'm not sure if it would be good practice to wait for the version update in the cluster custom resource before continuing with the other upgrades? Or do you see a better way for implementing that? |
@ariksidney Yes it's always a good practice to update version solo before update anything else. Unfortunately I didn't see any eks API document that "version" and "AuthenticationMode" can't update at the same time but if you run that with SDK calls and see the error, it's a limitation there. Have you tried to use CLI or SDK call to update the cluster for both |
@pahud yes, I just tried it with the CLI commands (first trigger the asynchronous
So I think the only way it could be achieved in the cdk would be to wait for the version update to complete in the custom cluster resource before continuing with the cluster config updates in the same custom resource. Not a good idea I guess? |
Describe the bug
I'm not really sure if this is a bug or if the error message is not really clear (at least to me).
In the latest version of our custom EKS product we're updating the authMode from
configMap
toapi and configMap
, however at the same time we would also like to update the control plane version to 1.31. This leads to the following error:Only one type of update - VpcConfigUpdate, LoggingUpdate, EndpointAccessUpdate, or AuthModeUpdate can be allowed
However, we are only updating one type of the mentioned types (which is
AuthModeUpdate
), the other type we're updating isupdateVersion
.According to my understanding, this should be possible as
versionUpdate
is not mentioned here?aws-cdk/packages/@aws-cdk/custom-resource-handlers/lib/aws-eks/cluster-resource-handler/cluster.ts
Lines 131 to 137 in 523e0f0
Regression Issue
Last Known Working CDK Version
No response
Expected Behavior
If updating
version
andauthMode
at the same time should be possible, it shouldn't fail.If this shouldn't be possible,
versionUpdate
should be in the list ofupdateTypes
as well.Current Behavior
Updating the following properties fails with
Reproduction Steps
Create a cluster with
And update it with:
Possible Solution
No response
Additional Information/Context
No response
CDK CLI Version
2.160.0
Framework Version
No response
Node.js Version
20.18.1
OS
Linux
Language
Python
Language Version
Python 3.10.16
Other information
No response
The text was updated successfully, but these errors were encountered: