-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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
kube-controller-manager: Add configure-cloud-routes option #25614
kube-controller-manager: Add configure-cloud-routes option #25614
Conversation
|
58a4162
to
007f40a
Compare
Finally passed the integration tests - thanks for the heads-up @bprashanth :-) Going to leave the e2e flake until I hear whether the PR is something we'll consider - not least because I suspect even if we accept the idea, we'll likely change the name. |
@thockin any thoughts? Could support the general disaggregation of KCM; perhaps in that light it should be "enable-route-controller" (default true)? |
LGTM @mikedanese @cjcullen @Q-Lee FYI if you rebase and get green before Monday, you're in. |
also permission to self-lgtm when rebased and green |
LGTM |
Looks fine to me. |
This allows kube-controller-manager to allocate CIDRs to nodes (with allocate-node-cidrs=true), but will not try to configure them on the cloud provider, even if the cloud provider supports Routes. The default is configure-cloud-routes=true, and it will only try to configure routes if allocate-node-cidrs is also configured, so the default behaviour is unchanged. This is useful because on AWS the cloud provider configures routes by setting up VPC routing table entries, but there is a limit of 50 entries. So setting configure-cloud-routes on AWS would allow us to continue to allocate node CIDRs as today, but replace the VPC route-table mechanism with something not limited to 50 nodes. We can't just turn off the cloud-provider entirely because it also controls other things - node discovery, load balancer creation etc. Fix kubernetes#25602
007f40a
to
6c66764
Compare
Re-adding lgtm after rebase. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 6c66764. |
Automatic merge from submit-queue |
Is there any reason to not allow the case where:
It's a use-case that we ran into today, it's mostly to unblock some testing of some half-baked functionality, but if I squint I could imagine someone wanting that configuration in a real cluster. |
This allows kube-controller-manager to allocate CIDRs to nodes (with
allocate-node-cidrs=true), but will not try to configure them on the
cloud provider, even if the cloud provider supports Routes.
The default is configure-cloud-routes=true, and it will only try to
configure routes if allocate-node-cidrs is also configured, so the
default behaviour is unchanged.
This is useful because on AWS the cloud provider configures routes by
setting up VPC routing table entries, but there is a limit of 50
entries. So setting configure-cloud-routes on AWS would allow us to
continue to allocate node CIDRs as today, but replace the VPC
route-table mechanism with something not limited to 50 nodes.
We can't just turn off the cloud-provider entirely because it also
controls other things - node discovery, load balancer creation etc.
Fix #25602