-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🌱 api: relax validation for Machine .status.addresses to maximum of 128 instead of 32 items #13060
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
base: main
Are you sure you want to change the base?
🌱 api: relax validation for Machine .status.addresses to maximum of 128 instead of 32 items #13060
Conversation
… instead of 32 items
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.12 |
|
@chrischdi: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-1.11 |
|
@chrischdi: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
neolit123
left a comment
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 think we might want to remove this validation or set it to a higher value like 256, which is in the CIDR limits for most CNI plugins...or even 512 to be safer.
what are the security implication of the field not having a limit?
is it just a foot gun for the admin to somehow self-ddos, or something in those lines?
the case with CAPZ here i think was that they decided to add a test for CAPZ+Azure CNI, making sure that the CNI can work at it's limits. yet, reading here the actual maximum is 250:
https://learn.microsoft.com/en-us/azure/aks/azure-cni-overlay
110 is a kubelet's maxPods default:
https://kubernetes.io/docs/setup/best-practices/cluster-large/
but the field has no limit.
We should not remove this validation as we want it for CEL to be able to calculate the cost of a validation. The number in Azure I think does not corelate with number of pods able to run, as these are pod IPs and not machine ips. We should be okay with also setting 256 or even 512, but I'd like to understand the use case. It not really makes sense to CAPI to know all pod ips. It is easy to relax the validation, which then is not a breaking change. Making validation more strict on the other hand is a breaking change we should try to prevent. |
|
I'm ok in raising the value, but I would prefer to not increase it too much without having a better understanding of use cases. |
damdo
left a comment
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.
Looks reasonable to me, thanks @chrischdi
What this PR does / why we need it:
This PR relaxes the validation which was added in v1.11 to allow more entries in the Machine's
.status.addressesslice.This came up when trying to bump CAPZ to Cluster API 1.11 at:
kubernetes-sigs/cluster-api-provider-azure#5979
CAPZ has a test which uses Azure CNI which leads to have 111 entries at
AzureMachine.status.addresses.Patching that up to the Machine fails due to the validation added in v1.11.
The entries are:
InternalDNSInternalIP'sInternalIP'sTo still get more information about the reasoning on why CAPZ has this use case of having more addresses then 32, I asked the question at https://kubernetes.slack.com/archives/CEX9HENG7/p1764231240877609
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
/area api