Merge pull request #876 from nilo19/bug/route

fix: remove outdated ipv4 route when the corresponding node is deleted
kubernetes-sigs · Oct 26, 2021 · 84eb889 · 84eb889
2 parents 026fd1e + 4edce73
commit 84eb889
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 2 deletions.
diff --git a/pkg/provider/azure_routes.go b/pkg/provider/azure_routes.go
@@ -107,6 +107,7 @@ func (d *delayedRouteUpdater) updateRoutes() {
 
 	// No need to do any updating.
 	if len(d.routesToUpdate) == 0 {
+		klog.V(4).Info("updateRoutes: nothing to update, returning")
 		return
 	}
 
@@ -216,6 +217,8 @@ func (d *delayedRouteUpdater) cleanupOutdatedRoutes(existingRoutes []network.Rou
 		existingRouteName := to.String(existingRoutes[i].Name)
 		split := strings.Split(existingRouteName, consts.RouteNameSeparator)
 
+		klog.V(4).Infof("cleanupOutdatedRoutes: checking route %s", existingRouteName)
+
 		// filter out unmanaged routes
 		deleteRoute := false
 		if d.az.nodeNames.Has(split[0]) {
@@ -466,9 +469,8 @@ func (az *Cloud) DeleteRoute(ctx context.Context, clusterName string, kubeRoute
 		return nil
 	}
 
-	klog.V(2).Infof("DeleteRoute: deleting route. clusterName=%q instance=%q cidr=%q", clusterName, kubeRoute.TargetNode, kubeRoute.DestinationCIDR)
-
 	routeName := mapNodeNameToRouteName(az.ipv6DualStackEnabled, kubeRoute.TargetNode, kubeRoute.DestinationCIDR)
+	klog.V(2).Infof("DeleteRoute: deleting route. clusterName=%q instance=%q cidr=%q routeName=%q", clusterName, kubeRoute.TargetNode, kubeRoute.DestinationCIDR, routeName)
 	route := network.Route{
 		Name:                  to.StringPtr(routeName),
 		RoutePropertiesFormat: &network.RoutePropertiesFormat{},
@@ -486,6 +488,28 @@ func (az *Cloud) DeleteRoute(ctx context.Context, clusterName string, kubeRoute
 		return err
 	}
 
+	// Remove outdated ipv4 routes as well
+	if az.ipv6DualStackEnabled {
+		routeNameWithoutIPV6Suffix := strings.Split(routeName, consts.RouteNameSeparator)[0]
+		klog.V(2).Infof("DeleteRoute: deleting route. clusterName=%q instance=%q cidr=%q routeName=%q", clusterName, kubeRoute.TargetNode, kubeRoute.DestinationCIDR, routeNameWithoutIPV6Suffix)
+		route := network.Route{
+			Name:                  to.StringPtr(routeNameWithoutIPV6Suffix),
+			RoutePropertiesFormat: &network.RoutePropertiesFormat{},
+		}
+		op, err := az.routeUpdater.addRouteOperation(routeOperationDelete, route)
+		if err != nil {
+			klog.Errorf("DeleteRoute failed for node %q with error: %v", kubeRoute.TargetNode, err)
+			return err
+		}
+
+		// Wait for operation complete.
+		err = op.wait()
+		if err != nil {
+			klog.Errorf("DeleteRoute failed for node %q with error: %v", kubeRoute.TargetNode, err)
+			return err
+		}
+	}
+
 	klog.V(2).Infof("DeleteRoute: route deleted. clusterName=%q instance=%q cidr=%q", clusterName, kubeRoute.TargetNode, kubeRoute.DestinationCIDR)
 	isOperationSucceeded = true
 

diff --git a/pkg/provider/azure_routes_test.go b/pkg/provider/azure_routes_test.go
@@ -109,6 +109,76 @@ func TestDeleteRoute(t *testing.T) {
 	}
 }
 
+func TestDeleteRouteDualStack(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	routeTableClient := mockroutetableclient.NewMockInterface(ctrl)
+
+	cloud := &Cloud{
+		RouteTablesClient: routeTableClient,
+		Config: Config{
+			RouteTableResourceGroup: "foo",
+			RouteTableName:          "bar",
+			Location:                "location",
+		},
+		unmanagedNodes:       sets.NewString(),
+		nodeInformerSynced:   func() bool { return true },
+		ipv6DualStackEnabled: true,
+	}
+	cache, _ := cloud.newRouteTableCache()
+	cloud.rtCache = cache
+	cloud.routeUpdater = newDelayedRouteUpdater(cloud, 100*time.Millisecond)
+	go cloud.routeUpdater.run()
+
+	route := cloudprovider.Route{
+		TargetNode:      "node",
+		DestinationCIDR: "1.2.3.4/24",
+	}
+	routeName := mapNodeNameToRouteName(true, route.TargetNode, route.DestinationCIDR)
+	routeNameIPV4 := mapNodeNameToRouteName(false, route.TargetNode, route.DestinationCIDR)
+	routeTables := network.RouteTable{
+		Name:     &cloud.RouteTableName,
+		Location: &cloud.Location,
+		RouteTablePropertiesFormat: &network.RouteTablePropertiesFormat{
+			Routes: &[]network.Route{
+				{
+					Name: &routeName,
+				},
+				{
+					Name: &routeNameIPV4,
+				},
+			},
+		},
+	}
+	routeTablesAfterFirstDeletion := network.RouteTable{
+		Name:     &cloud.RouteTableName,
+		Location: &cloud.Location,
+		RouteTablePropertiesFormat: &network.RouteTablePropertiesFormat{
+			Routes: &[]network.Route{
+				{
+					Name: &routeNameIPV4,
+				},
+			},
+		},
+	}
+	routeTablesAfterSecondDeletion := network.RouteTable{
+		Name:     &cloud.RouteTableName,
+		Location: &cloud.Location,
+		RouteTablePropertiesFormat: &network.RouteTablePropertiesFormat{
+			Routes: &[]network.Route{},
+		},
+	}
+	routeTableClient.EXPECT().Get(gomock.Any(), cloud.RouteTableResourceGroup, cloud.RouteTableName, "").Return(routeTables, nil).AnyTimes()
+	routeTableClient.EXPECT().CreateOrUpdate(gomock.Any(), cloud.RouteTableResourceGroup, cloud.RouteTableName, routeTablesAfterFirstDeletion, "").Return(nil)
+	routeTableClient.EXPECT().CreateOrUpdate(gomock.Any(), cloud.RouteTableResourceGroup, cloud.RouteTableName, routeTablesAfterSecondDeletion, "").Return(nil)
+	err := cloud.DeleteRoute(context.TODO(), "cluster", &route)
+	if err != nil {
+		t.Errorf("unexpected error deleting route: %v", err)
+		t.FailNow()
+	}
+
+}
+
 func TestCreateRoute(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()