From 11f8886f1278d5569d9d992113cad4e8e0e47f80 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Wed, 14 Jun 2017 22:34:15 -0400 Subject: [PATCH] AWS: Remove blackhole routes in our managed range Blackhole routes otherwise acccumulate unboundedly. We also are careful to ensure that we do so only within the managed range, which requires enlisting the help of the routecontroller. Fix #47524 --- pkg/cloudprovider/cloud.go | 3 + pkg/cloudprovider/providers/aws/aws_routes.go | 33 +++++--- pkg/controller/route/routecontroller.go | 8 +- pkg/controller/route/routecontroller_test.go | 77 ++++++++++++++----- 4 files changed, 88 insertions(+), 33 deletions(-) diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 247cb15ce78..2fb837b7192 100644 --- a/pkg/cloudprovider/cloud.go +++ b/pkg/cloudprovider/cloud.go @@ -150,6 +150,9 @@ type Route struct { // DestinationCIDR is the CIDR format IP range that this routing rule // applies to. DestinationCIDR string + // Blackhole is set to true if this is a blackhole route + // The node controller will delete the route if it is in the managed range. + Blackhole bool } // Routes is an abstract, pluggable interface for advanced routing rules. diff --git a/pkg/cloudprovider/providers/aws/aws_routes.go b/pkg/cloudprovider/providers/aws/aws_routes.go index fffcdf42c12..f24220d78fa 100644 --- a/pkg/cloudprovider/providers/aws/aws_routes.go +++ b/pkg/cloudprovider/providers/aws/aws_routes.go @@ -90,21 +90,34 @@ func (c *Cloud) ListRoutes(clusterName string) ([]*cloudprovider.Route, error) { } for _, r := range table.Routes { - instanceID := orEmpty(r.InstanceId) - destinationCIDR := orEmpty(r.DestinationCidrBlock) - - if instanceID == "" || destinationCIDR == "" { + destinationCIDR := aws.StringValue(r.DestinationCidrBlock) + if destinationCIDR == "" { continue } - instance, found := instances[instanceID] - if !found { - glog.Warningf("unable to find instance ID %s in the list of instances being routed to", instanceID) + route := &cloudprovider.Route{ + Name: clusterName + "-" + destinationCIDR, + DestinationCIDR: destinationCIDR, + } + + // Capture blackhole routes + if aws.StringValue(r.State) == ec2.RouteStateBlackhole { + route.Blackhole = true + routes = append(routes, route) continue } - nodeName := mapInstanceToNodeName(instance) - routeName := clusterName + "-" + destinationCIDR - routes = append(routes, &cloudprovider.Route{Name: routeName, TargetNode: nodeName, DestinationCIDR: destinationCIDR}) + + // Capture instance routes + instanceID := aws.StringValue(r.InstanceId) + if instanceID != "" { + instance, found := instances[instanceID] + if found { + route.TargetNode = mapInstanceToNodeName(instance) + routes = append(routes, route) + } else { + glog.Warningf("unable to find instance ID %s in the list of instances being routed to", instanceID) + } + } } return routes, nil diff --git a/pkg/controller/route/routecontroller.go b/pkg/controller/route/routecontroller.go index 827deaa6e2a..d46137d1b88 100644 --- a/pkg/controller/route/routecontroller.go +++ b/pkg/controller/route/routecontroller.go @@ -118,7 +118,9 @@ func (rc *RouteController) reconcile(nodes []*v1.Node, routes []*cloudprovider.R // routeMap maps routeTargetNode->route routeMap := make(map[types.NodeName]*cloudprovider.Route) for _, route := range routes { - routeMap[route.TargetNode] = route + if route.TargetNode != "" { + routeMap[route.TargetNode] = route + } } wg := sync.WaitGroup{} @@ -171,8 +173,8 @@ func (rc *RouteController) reconcile(nodes []*v1.Node, routes []*cloudprovider.R } for _, route := range routes { if rc.isResponsibleForRoute(route) { - // Check if this route applies to a node we know about & has correct CIDR. - if nodeCIDRs[route.TargetNode] != route.DestinationCIDR { + // Check if this route is a blackhole, or applies to a node we know about & has an incorrect CIDR. + if route.Blackhole || (nodeCIDRs[route.TargetNode] != route.DestinationCIDR) { wg.Add(1) // Delete the route. go func(route *cloudprovider.Route, startTime time.Time) { diff --git a/pkg/controller/route/routecontroller_test.go b/pkg/controller/route/routecontroller_test.go index 5f41c0eac73..b861a6a06b3 100644 --- a/pkg/controller/route/routecontroller_test.go +++ b/pkg/controller/route/routecontroller_test.go @@ -97,12 +97,12 @@ func TestReconcile(t *testing.T) { &node2, }, initialRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedNetworkUnavailable: []bool{true, true}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), @@ -114,11 +114,11 @@ func TestReconcile(t *testing.T) { &node2, }, initialRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, }, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedNetworkUnavailable: []bool{true, true}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), @@ -131,8 +131,8 @@ func TestReconcile(t *testing.T) { }, initialRoutes: []*cloudprovider.Route{}, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedNetworkUnavailable: []bool{true, true}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), @@ -144,14 +144,14 @@ func TestReconcile(t *testing.T) { &node2, }, initialRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, - {cluster + "-03", "node-3", "10.120.2.0/24"}, - {cluster + "-04", "node-4", "10.120.3.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, + {cluster + "-03", "node-3", "10.120.2.0/24", false}, + {cluster + "-04", "node-4", "10.120.3.0/24", false}, }, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedNetworkUnavailable: []bool{true, true}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), @@ -163,12 +163,12 @@ func TestReconcile(t *testing.T) { &node2, }, initialRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-03", "node-3", "10.120.2.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-03", "node-3", "10.120.2.0/24", false}, }, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, - {cluster + "-02", "node-2", "10.120.1.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, }, expectedNetworkUnavailable: []bool{true, true}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), @@ -181,11 +181,48 @@ func TestReconcile(t *testing.T) { }, initialRoutes: []*cloudprovider.Route{}, expectedRoutes: []*cloudprovider.Route{ - {cluster + "-01", "node-1", "10.120.0.0/24"}, + {cluster + "-01", "node-1", "10.120.0.0/24", false}, }, expectedNetworkUnavailable: []bool{true, false}, clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, nodeNoCidr}}), }, + // 2 nodes, an extra blackhole route in our range + { + nodes: []*v1.Node{ + &node1, + &node2, + }, + initialRoutes: []*cloudprovider.Route{ + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, + {cluster + "-03", "", "10.120.2.0/24", true}, + }, + expectedRoutes: []*cloudprovider.Route{ + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, + }, + expectedNetworkUnavailable: []bool{true, true}, + clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), + }, + // 2 nodes, an extra blackhole route not in our range + { + nodes: []*v1.Node{ + &node1, + &node2, + }, + initialRoutes: []*cloudprovider.Route{ + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, + {cluster + "-03", "", "10.1.2.0/24", true}, + }, + expectedRoutes: []*cloudprovider.Route{ + {cluster + "-01", "node-1", "10.120.0.0/24", false}, + {cluster + "-02", "node-2", "10.120.1.0/24", false}, + {cluster + "-03", "", "10.1.2.0/24", true}, + }, + expectedNetworkUnavailable: []bool{true, true}, + clientset: fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{node1, node2}}), + }, } for i, testCase := range testCases { cloud := &fakecloud.FakeCloud{RouteMap: make(map[string]*fakecloud.FakeRoute)}