NodeRouteController should account for PodCIDR updates

[antoninbas]

Describe the bug The PodCIDR field in NodeSpec is "immutable": once set, it cannot be changed. However, because the NodeRouteController uses a workqueue (with the Node name as the key), it is possible to observe the same Node name in addNodeRoute, but with a different PodCIDR. This is not something we account for today, which means the controller may be buggy.

More precisely, the following sequence of events would trigger a bug, where the datapath uses a stale PodCIDR:

1. Node X with name "foo" PodCIDR 1.1.1.0/24 is created
2. Node X is deleted
3. Node Y with name "foo" (same name) and PodCIDR 1.1.1.1/24 is created

The Delete handler (step 2) and the Add handler (step 3) will both enqueue the Node name ("foo"). If the time between steps 2 and 3 is small enough, the events will be "merged" in the workqueue, and syncNodeRoute will be called only once. At that point, syncNodeRoute will get Node Y from the lister and addNodeRoute will be called. There will still be an entry in the installedNodes indexer (corresponding to Node Y) and because we do not check that the PodCIDRs match, we will skip updating the datapath:

https://github.com/antrea-io/antrea/blob/e4aedec275d16e3bd228a86ff70e1336f6cf9300/pkg/agent/controller/noderoute/node_route_controller.go#L524-L528

Note that because we do check that the Node IPs match, Node Y must have the same IPs as Node X in order for the issue to be observed (i.e., same name and same IPs). This "constraint", coupled with timing considerations, mean that this issue is quite unlikely.

To Reproduce The following unit test can be used (add it to pkg/agent/controller/noderoute/node_route_controller_test.go):

func TestNodeRecreateNewPodCIDR(t *testing.T) {
	c := newController(t, &config.NetworkConfig{})
	defer c.queue.ShutDown()

	stopCh := make(chan struct{})
	defer close(stopCh)
	c.informerFactory.Start(stopCh)
	c.informerFactory.WaitForCacheSync(stopCh)

	newNode := node1.DeepCopy()
	newNode.Spec.PodCIDR = podCIDR2.String()
	newNode.Spec.PodCIDRs = []string{podCIDR2.String()}

	c.clientset.CoreV1().Nodes().Create(context.TODO(), node1, metav1.CreateOptions{})
	c.ofClient.EXPECT().InstallNodeFlows("node1", gomock.Any(), &dsIPs1, uint32(0), nil).Times(1)
	c.routeClient.EXPECT().AddRoutes(podCIDR, "node1", nodeIP1, podCIDRGateway).Times(1)
	c.processNextWorkItem()

	// Node is deleted, then shortly after is "recreated" (same name) with a different PodCIDR
	c.clientset.CoreV1().Nodes().Delete(context.TODO(), node1.Name, metav1.DeleteOptions{})
	time.Sleep(100 * time.Millisecond)
	c.clientset.CoreV1().Nodes().Create(context.TODO(), newNode, metav1.CreateOptions{})

	// At the moment, the test will fail because these expectations are not met.
	c.ofClient.EXPECT().InstallNodeFlows("node1", gomock.Any(), &dsIPs1, uint32(0), nil).Times(1)
	c.routeClient.EXPECT().AddRoutes(podCIDR2, "node1", nodeIP1, podCIDR2Gateway).Times(1)

	// Using a sleep for convenience here.
	time.Sleep(1 * time.Second)

	c.processNextWorkItem()
}

Versions: v2.2