Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): control plane logs #8497
feat(eks): control plane logs #8497
Changes from 28 commits
e38402e
71f59a1
494330f
e82e8a6
8b582d3
e8bf3aa
83f0cb4
72226b0
f777ef4
0ec6571
e37d673
a250704
b15bdea
8c79e70
8628281
d84610e
bad1672
b4cdc58
3e53574
e799afb
438e3d5
a609ab9
5c70951
86af207
d538205
876a012
da23fc4
4d65eb3
abb5780
53d647f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Revert
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 is actually needed because the provide framework is not be handling it anymore
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.
What happens if I enabled logs and then set to undefined (remove it). Would it set all the logging to false?
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.
it will send an empty value for logging in the call to
updateClusterConfig()
, and it would result in setting the default value from AWSThere 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.
Have you tested this? My intuition is that if I send an
updateClusterConfig
request with an empty value forlogging
it will not perform any changes to my logging configuration, which is not what users expect if they enable logs and then remove this property in their CDK app.I suspect that we may either need to always send the two enabled/disable lists or add heuristics to the UPDATE handler to calculate the "diff" from the previous value and produce an appropriate update.
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 will try this out and let you know, but this diff is being done in the runtime code here:
aws-cdk/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts
Line 293 in 53d647f
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 only determines that there was a change, not which log types should now be enabled/disabled based on that change.
For example, say, "oldProps" included two enabled types (say
["typeA", "typeB"]
) and now "newProps" include only a single type (say["typeB"]
). Then, the request we need to send toupdateClusterConfig
is only to disable (we can, if we want also include an "enable" fortypeB
, but technically it's not required because it is already enabled).LMK if 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 believe we will need to send the whole configuration explicitly, including the disabled one. i am getting the following error when sending the exact same configuration to
updateClusterConfig
: