Antrea1.4: Windows node service route logic looks to be broken

[shettyg]

Describe the bug

There looks to be a few issues with SVC routes in windows in Antrea 1.4. Atleast by reading the code, the same issues look to exist in the master branch too.

1. The Reconcile() function in pkg/agent/route/route_windows.go is called by passing argument "svcIPs map[string]bool". The contents of this data structure look like this:
   map[10.96.0.1:true 10.96.0.10:true 10.96.1.70:true 10.96.10.217:true

But, the "routes" from c.listRoutes() has serviceIPs with their subnet specified. Example: 10.96.0.1/32. So you will always end up with calling util.RemoveNetRoute() for each of the serviceIPs in that function

2. The Reconcile() function and addServiceRoute() in pkg/agent/route/route_windows.go can be called in parallel threads. So while addServiceRoute() adds routes, Reconcile() deletes routes.

3. addServiceRoute() does: obj, found := c.hostRoutes.Load(svcIP.String()), but at the end of function loads it with: c.hostRoutes.Store(route.DestinationSubnet.String(), route). So the key is different types.

To Reproduce
Restart antrea-agent and you will see that some services are missing in the windows routes.

Tangential question: Why do we need to add each service IP route? Why not just put the serviceCIDR?

CC: @wenyingd @hongliangl

[hongliangl]

I have a PR #3889 that unifies some functions of Windows and Linux. This PR may fix the issue. In this PR,

• when starting Antrea proxy, aggregated route is calculated from all existing svc IPs.
• when adding a new svc, if the new aggregated route doesn't contain the svc IP, extend the aggregated route to contain the svc IP; otherwise, do nothing.
• when deleting a svc, do nothing.

[hongliangl]

This PR #3889 plans to be merged in Antrea 1.11.

[hongliangl]

It is not easy to backport for PR #3389. I made another PR #4470 to fix the issue and it is easy to backport. @shettyg

[tnqn]

Tangential question: Why do we need to add each service IP route? Why not just put the serviceCIDR?

@shettyg it's because K8s doesn't expose the serviceCIDR information anywhere officially. We wanted to avoid asking user to configure the value when we already watch the service API with AntreaProxy enabled, and to avoid a potential misconfiguration. Instead, we dynamically calculate the serviceCIDR based on clusterIPs we already know, and it should be a subset of the real serviceCIDR used by K8s. But for historical reason, windows implementation didn't do it and we are going to unify the implementation via the PR @hongliangl mentioned.