chore: AzureCluster subnet default flow comments #5817
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/S
api/v1beta1/azurecluster_default.go
Outdated
| // We need at least one subnet for nodes. | ||
| // If no node subnets are defined, and there is no cluster subnet defined, | ||
| // create a default 10.1.0.0/16 node subnet. | ||
| // TODO why do we prefer to fallback to "create a single, large node subnet" |
There was a problem hiding this comment.
@nojnhuh can you answer this question?
There was a problem hiding this comment.
Is this asking "why separate subnets for control plane and worker nodes?" One reason would be so that each subnet can refer to a different NSG. The control plane subnet is tied to a NSG that exposes SSH and the API server port and the worker node subnet doesn't expose any ports by default.
There was a problem hiding this comment.
No, not that. It's more that we seem to have this breakdown:
- Cluster subnet (can also be overloaded to be the subnet for all nodes)
- Zero or more "node" subnets
I understand the value of having multiple node subnets (different pools having discrete subnets), and I understand the value of not requiring any node subnets (just use a single subnet for everything [what we call the "cluster" subnet]). What I don't understand is why we have logic to say: if we have neither, create a single node subnet. It seems more intuitive to create a single "cluster" subnet in this case.
(And yes ACK on the differences between CP subnet and node subnets)
There was a problem hiding this comment.
I can't find any discussion related to that in #4253. Possibly to keep the same behavior as before CAPZ had a notion of a "cluster" subnet.
There was a problem hiding this comment.
That's good enough for me, removed the TODO, I think keeping back-compat is the way forward.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5817 +/- ##
==========================================
+ Coverage 53.06% 53.08% +0.02%
==========================================
Files 279 279
Lines 29677 29686 +9
==========================================
+ Hits 15747 15758 +11
+ Misses 13114 13112 -2
Partials 816 816 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jackfrancis
force-pushed
the
azurecluster-subnet-comments
branch
from
359c054 to
d2fb23f
Compare
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: e3d833b8b9b4cdc54898455713f8f65b930284c3
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The current defaults flow for
AzureClustersubnet configuration is tricky. This adds some clarifying comments.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 #
Special notes for your reviewer:
TODOs:
Release note: