From eedbe162d1164d4aa95fb428f2a0cb4f9b4e0041 Mon Sep 17 00:00:00 2001 From: docktofuture Date: Tue, 27 Jan 2026 11:15:03 +0100 Subject: [PATCH 1/2] Fix route controller condition update when external CNI sets NetworkUnavailable --- .../controllers/route/route_controller.go | 4 +- .../route/route_controller_test.go | 238 ++++++++++++++++++ 2 files changed, 240 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller.go b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller.go index c7e6c57e7da..b4e8aabee55 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller.go @@ -497,12 +497,12 @@ func (rc *RouteController) reconcile(ctx context.Context, nodes []*v1.Node, rout func (rc *RouteController) updateNetworkingCondition(node *v1.Node, routesCreated bool) error { _, condition := nodeutil.GetNodeCondition(&(node.Status), v1.NodeNetworkUnavailable) - if routesCreated && condition != nil && condition.Status == v1.ConditionFalse { + if routesCreated && condition != nil && condition.Status == v1.ConditionFalse && condition.Reason == "RouteCreated" { klog.V(2).Infof("set node %v with NodeNetworkUnavailable=false was canceled because it is already set", node.Name) return nil } - if !routesCreated && condition != nil && condition.Status == v1.ConditionTrue { + if !routesCreated && condition != nil && condition.Status == v1.ConditionTrue && condition.Reason == "NoRouteCreated" { klog.V(2).Infof("set node %v with NodeNetworkUnavailable=true was canceled because it is already set", node.Name) return nil } diff --git a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go index 7f7257e219c..c7b1e480aeb 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go +++ b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go @@ -636,3 +636,241 @@ func flatten(list []*cloudprovider.Route) []cloudprovider.Route { } return structList } + +func TestUpdateNetworkingCondition(t *testing.T) { + cluster := "test-cluster" + _, clusterCIDR, _ := netutils.ParseCIDRSloppy("10.244.0.0/16") + + testCases := []struct { + description string + existingCondition *v1.NodeCondition + routesCreated bool + expectedUpdate bool + expectedConditionType v1.ConditionStatus + expectedReason string + expectedMessage string + }{ + { + description: "No existing condition, routes created - should update", + existingCondition: nil, + routesCreated: true, + expectedUpdate: true, + expectedConditionType: v1.ConditionFalse, + expectedReason: "RouteCreated", + expectedMessage: "RouteController created a route", + }, + { + description: "No existing condition, routes not created - should update", + existingCondition: nil, + routesCreated: false, + expectedUpdate: true, + expectedConditionType: v1.ConditionTrue, + expectedReason: "NoRouteCreated", + expectedMessage: "RouteController failed to create a route", + }, + { + description: "Existing condition with same status and reason (RouteCreated) - should not update", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionFalse, + Reason: "RouteCreated", + Message: "RouteController created a route", + }, + routesCreated: true, + expectedUpdate: false, + }, + { + description: "Existing condition with same status but different reason (CalicoIsUp) - should update", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionFalse, + Reason: "CalicoIsUp", + Message: "Calico is running on this node", + }, + routesCreated: true, + expectedUpdate: true, + expectedConditionType: v1.ConditionFalse, + expectedReason: "RouteCreated", + expectedMessage: "RouteController created a route", + }, + { + description: "Existing condition with Status=False but different reason - should update when routes created", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionFalse, + Reason: "ExternalCNI", + Message: "External CNI configured", + }, + routesCreated: true, + expectedUpdate: true, + expectedConditionType: v1.ConditionFalse, + expectedReason: "RouteCreated", + expectedMessage: "RouteController created a route", + }, + { + description: "Existing condition with Status=True and different reason - should update when routes not created", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionTrue, + Reason: "SomeOtherReason", + Message: "Some other message", + }, + routesCreated: false, + expectedUpdate: true, + expectedConditionType: v1.ConditionTrue, + expectedReason: "NoRouteCreated", + expectedMessage: "RouteController failed to create a route", + }, + { + description: "Existing condition with Status=False, transitioning to routes not created - should update", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionFalse, + Reason: "RouteCreated", + Message: "RouteController created a route", + }, + routesCreated: false, + expectedUpdate: true, + expectedConditionType: v1.ConditionTrue, + expectedReason: "NoRouteCreated", + expectedMessage: "RouteController failed to create a route", + }, + { + description: "Existing condition with Status=True and NoRouteCreated reason - should not update", + existingCondition: &v1.NodeCondition{ + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionTrue, + Reason: "NoRouteCreated", + Message: "RouteController failed to create a route", + }, + routesCreated: false, + expectedUpdate: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + node := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + UID: "test-uid", + }, + Spec: v1.NodeSpec{ + PodCIDR: "10.244.0.0/24", + PodCIDRs: []string{"10.244.0.0/24"}, + }, + } + + // Set existing condition if specified + if tc.existingCondition != nil { + node.Status.Conditions = []v1.NodeCondition{*tc.existingCondition} + } + + client := fake.NewSimpleClientset(node) + informerFactory := informers.NewSharedInformerFactory(client, 0) + + rc, err := New(nil, client, informerFactory.Core().V1().Nodes(), cluster, []*net.IPNet{clusterCIDR}) + require.NoError(t, err) + rc.nodeListerSynced = alwaysReady + + // Call updateNetworkingCondition + err = rc.updateNetworkingCondition(node, tc.routesCreated) + require.NoError(t, err) + + if tc.expectedUpdate { + // Verify that a patch was called + actions := client.Actions() + patchFound := false + for _, action := range actions { + if action.GetVerb() == "patch" { + patchFound = true + break + } + } + assert.True(t, patchFound, "Expected a patch action but none found") + + // Get the updated node + updatedNode, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{}) + require.NoError(t, err) + + // Verify the condition was updated correctly + _, condition := nodeutil.GetNodeCondition(&updatedNode.Status, v1.NodeNetworkUnavailable) + require.NotNil(t, condition, "Expected NodeNetworkUnavailable condition to exist") + assert.Equal(t, tc.expectedConditionType, condition.Status, "Unexpected condition status") + assert.Equal(t, tc.expectedReason, condition.Reason, "Unexpected condition reason") + assert.Equal(t, tc.expectedMessage, condition.Message, "Unexpected condition message") + } else { + // Verify that no patch was called (or only initial operations) + actions := client.Actions() + patchCount := 0 + for _, action := range actions { + if action.GetVerb() == "patch" { + patchCount++ + } + } + assert.Equal(t, 0, patchCount, "Expected no patch action but found %d", patchCount) + } + }) + } +} + +func TestUpdateNetworkingConditionWithCalicoScenario(t *testing.T) { + // This test specifically covers the bug where Calico sets NetworkUnavailable=False + // and RouteController should still be able to update it with its own reason + cluster := "test-cluster" + _, clusterCIDR, _ := netutils.ParseCIDRSloppy("10.244.0.0/16") + + node := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + UID: "test-uid", + }, + Spec: v1.NodeSpec{ + PodCIDR: "10.244.0.0/24", + PodCIDRs: []string{"10.244.0.0/24"}, + }, + Status: v1.NodeStatus{ + Conditions: []v1.NodeCondition{ + { + Type: v1.NodeNetworkUnavailable, + Status: v1.ConditionFalse, + Reason: "CalicoIsUp", + Message: "Calico is running on this node", + }, + }, + }, + } + + client := fake.NewSimpleClientset(node) + informerFactory := informers.NewSharedInformerFactory(client, 0) + + rc, err := New(nil, client, informerFactory.Core().V1().Nodes(), cluster, []*net.IPNet{clusterCIDR}) + require.NoError(t, err) + rc.nodeListerSynced = alwaysReady + + // Call updateNetworkingCondition with routes created + err = rc.updateNetworkingCondition(node, true) + require.NoError(t, err) + + // Verify that a patch was called + actions := client.Actions() + patchFound := false + for _, action := range actions { + if action.GetVerb() == "patch" { + patchFound = true + break + } + } + assert.True(t, patchFound, "Expected a patch action to update the condition") + + // Get the updated node + updatedNode, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{}) + require.NoError(t, err) + + // Verify the condition was updated with RouteCreated reason + _, condition := nodeutil.GetNodeCondition(&updatedNode.Status, v1.NodeNetworkUnavailable) + require.NotNil(t, condition, "Expected NodeNetworkUnavailable condition to exist") + assert.Equal(t, v1.ConditionFalse, condition.Status, "Expected Status to remain False") + assert.Equal(t, "RouteCreated", condition.Reason, "Expected Reason to be updated to RouteCreated") + assert.Equal(t, "RouteController created a route", condition.Message, "Expected Message to be updated") +} From 57315c197438f5feac63e529db095472c5e4cce9 Mon Sep 17 00:00:00 2001 From: docktofuture Date: Tue, 27 Jan 2026 13:00:47 +0100 Subject: [PATCH 2/2] call fake.NewClientset instead of fake.NewSimpleClientset --- .../cloud-provider/controllers/route/route_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go index c7b1e480aeb..d11dec1abdc 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go +++ b/staging/src/k8s.io/cloud-provider/controllers/route/route_controller_test.go @@ -766,7 +766,7 @@ func TestUpdateNetworkingCondition(t *testing.T) { node.Status.Conditions = []v1.NodeCondition{*tc.existingCondition} } - client := fake.NewSimpleClientset(node) + client := fake.NewClientset(node) informerFactory := informers.NewSharedInformerFactory(client, 0) rc, err := New(nil, client, informerFactory.Core().V1().Nodes(), cluster, []*net.IPNet{clusterCIDR}) @@ -841,7 +841,7 @@ func TestUpdateNetworkingConditionWithCalicoScenario(t *testing.T) { }, } - client := fake.NewSimpleClientset(node) + client := fake.NewClientset(node) informerFactory := informers.NewSharedInformerFactory(client, 0) rc, err := New(nil, client, informerFactory.Core().V1().Nodes(), cluster, []*net.IPNet{clusterCIDR})