cloud-controller-manager: routes controller should not depend on --allocate-node-cidrs #97029
Conversation
k8s-ci-robot
added
release-note
|
@andrewsykim: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
k8s-ci-robot
added
needs-triage
k8s-ci-robot
added
the
sig/cloud-provider
|
/assign @cheftako @cici37 @nicolehanjing |
k8s-ci-robot
added
approved
| @@ -104,15 +104,15 @@ func startServiceController(ctx *config.CompletedConfig, cloud cloudprovider.Int | |||
| } | |||
|
|
|||
| func startRouteController(ctx *config.CompletedConfig, cloud cloudprovider.Interface, stopCh <-chan struct{}) (http.Handler, bool, error) { | |||
| if !ctx.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs || !ctx.ComponentConfig.KubeCloudShared.ConfigureCloudRoutes { | |||
| klog.Infof("Will not configure cloud provider routes for allocate-node-cidrs: %v, configure-cloud-routes: %v.", ctx.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs, ctx.ComponentConfig.KubeCloudShared.ConfigureCloudRoutes) | |||
| if !ctx.ComponentConfig.KubeCloudShared.ConfigureCloudRoutes { | |||
There was a problem hiding this comment.
Contemplating if this is considered a breaking change since AllocateNodeCIDRs is false by default and ConfigureCloudRoutes is true by default. This means if you previously set neither of these flags, this check is now passing. I think it isn't because:
- the cloud provider still needs to implement
Routes()to enable the controller. - even if the cloud provider implements
Routes(), routes controller itself is no-op ifnode.spec.podCIDRis not set. Ifnode.spec.podCIDRis set, it means--allocate-node-cidris enabled on kube-controller-manager, which means we should be running this controller.
There was a problem hiding this comment.
@cheftako would still like your thoughts/opinions on this
There was a problem hiding this comment.
My worry would be the managed vs unmanaged distinction. Take the GCP Cloud Provider; it has to implement Routes() as the same cloud provider impl is used in both managed (GKE) and unmanaged releases. The managed is not an issue as we have a dedicated team of experts keeping it working.
For unmanaged releases allocate-node-cidrs defaults to true and enable-ip-aliases to false (i.e. configure-cloud-routes to true). Given that allocate-node-cidrs defaults to true and routes() is implemented, the unmanaged instance would have failed this case but we're saved by configure-cloud-routes being true. Not sure I'm sanguine about being safe in the other cases where we may want different behaviors between managed and unmanaged kubernetes deployments.
|
/lgtm I think this is a reasonable fix for the function |
|
/retest |
|
Failing CI looks legit, I'll take a look soon |
…locate-node-cidrs Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
…ud-routes Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
…roller-manager secure serving tests Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
k8s-ci-robot
added
size/S
andrewsykim
force-pushed
the
routes-controller-allocate-node-cidr-check
branch
from
ca0a7ef
to
0c90d8d
Compare
k8s-ci-robot
added
the
sig/testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim 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 |
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Enabling routes controller should be independent from
--allocate-node-cidrs. Checking--allocate-node-cidrsmade a lot of sense inkube-controller-manager, where node IPAM was required to ensure routes controller hadnode.spec.podCIDRset correctly. In cloud-controller-manager, this is not necessarily true since it does not run node IPAM (in most clusters) and instead depends on nodeipam controller running in kube-controller-manager. In some cases, an external controller may actually be allocated CIDRs tonode.spec.podCIDR. Enablement of the routes controller should only depend on the--configure-cloud-routesflag.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: