Skip to content

Conversation

@chrischdi
Copy link
Member

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.addresses slice.

  • Before: maximum of 32 items
  • After: maximum of 128 items

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:

  • 1x InternalDNS
  • 1x InternalIP's
  • 109 additional InternalIP's

To 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

@k8s-ci-robot k8s-ci-robot added the area/api Issues or PRs related to the APIs label Nov 27, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 27, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 27, 2025
@chrischdi
Copy link
Member Author

/cherry-pick release-1.12

@k8s-infra-cherrypick-robot

@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.12

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.

@chrischdi
Copy link
Member Author

/cherry-pick release-1.11

@k8s-infra-cherrypick-robot

@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.11

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.

Copy link
Member

@neolit123 neolit123 left a 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.

@chrischdi
Copy link
Member Author

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: learn.microsoft.com/en-us/azure/aks/azure-cni-overlay

110 is a kubelet's maxPods default: 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.
We added it everywhere on purpose and tried to be sensible finding the right defaults.

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.
But to be sure I asked in the CAPZ channel.

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.

@fabriziopandini
Copy link
Member

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.
Let's wait for beginning next week when most of the US folks will be back.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2025
Copy link
Member

@damdo damdo left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Issues or PRs related to the APIs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants