[Windows][AntreaProxy] Fix route reconcile()

[lzhecheng]

listRoutes() only considers routes with antrea-gw0 as interface
so no need to consider kube-proxy Service routes. Also for AntreaProxy
Service routes, all of them should be returned and be compared with
desiredClusterIPSvcIPs.

NodePort and LoadBalancer Services also have ClusterIPs so the check
if Service is ClusterIP Service is unnecessary.

Signed-off-by: Zhecheng Li lzhecheng@vmware.com

[codecov-commenter]

Codecov Report

Merging #2939 (b5819eb) into main (12f8395) will decrease coverage by 2.35%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2939      +/-   ##
==========================================
- Coverage   61.61%   59.25%   -2.36%     
==========================================
  Files         283      283              
  Lines       23810    23811       +1     
==========================================
- Hits        14671    14110     -561     
- Misses       7558     8198     +640     
+ Partials     1581     1503      -78     

Flag	Coverage Δ
kind-e2e-tests	46.35% <0.00%> (-2.89%)	⬇️
unit-tests	40.59% <0.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...gent/controller/noderoute/node_route_controller.go	54.91% <0.00%> (+0.14%)	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	0.00% <0.00%> (-82.36%)	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	19.54% <0.00%> (-55.18%)	⬇️
pkg/support/dump_others.go	0.00% <0.00%> (-51.73%)	⬇️
pkg/support/dump.go	8.13% <0.00%> (-49.60%)	⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go	0.00% <0.00%> (-40.00%)	⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go	0.00% <0.00%> (-40.00%)	⬇️
pkg/apiserver/handlers/loglevel/handler.go	0.00% <0.00%> (-38.47%)	⬇️
pkg/agent/util/net.go	16.32% <0.00%> (-22.45%)	⬇️
pkg/agent/route/route_linux.go	28.48% <0.00%> (-21.45%)	⬇️
... and 31 more

[wenyingd]

LGTM

[lzhecheng]

/test-windows-all /test-all

[tnqn]

I don't quite understand what the PR fixes from the description. I can see it originally matched kube-proxy route and now it matches antrea-proxy route, but it doesn't say why we should skip the latter now. We don't expect listRoutes return service routes?

[lzhecheng]

@tnqn The routes returned by listRoutes() are to be deleted so we should skip those AntreaProxy Service routes.

Before AntreaProxy change, the if statement in listRoutes() for kube-proxy routes is actually unnecessary. The reason is that the first if statement in listRoutes() checks if gateway is antrea-gw0 and kube-proxy routes are actually not.

[tnqn]

@tnqn The routes returned by listRoutes() are to be deleted so we should skip those AntreaProxy Service routes.

Before AntreaProxy change, the if statement in listRoutes() for kube-proxy routes is actually unnecessary. The reason is that the first if statement in listRoutes() checks if gateway is antrea-gw0 and kube-proxy routes are actually not.

Then what's the point of the below code that checks svc IP and how will it delete stale Service routes?

antrea/pkg/agent/route/route_windows.go

Lines 124 to 145 in ae2b53d

	func (c *Client) Reconcile(podCIDRs []string, svcIPs map[string]bool) error {
	desiredPodCIDRs := sets.NewString(podCIDRs...)
	routes, err := c.listRoutes()
	if err != nil {
	return err
	}
	for dst, rt := range routes {
	if desiredPodCIDRs.Has(dst) {
	c.hostRoutes.Store(dst, rt)
	continue
	}
	if _, ok := svcIPs[dst]; ok {
	c.hostRoutes.Store(dst, rt)
	continue
	}
	err := util.RemoveNetRoute(rt)
	if err != nil {
	return err
	}
	}
	return nil
	}

[lzhecheng]

@tnqn stale Service routes, flows should be removed here:

antrea/pkg/agent/proxy/proxier.go

Line 480 in 0ce671b

if needRemoval {

The svcIPs in your mentioned code block refer to this route:

antrea/pkg/agent/controller/noderoute/node_route_controller.go

Line 203 in 0ce671b

// The route for virtual IP -> antrea-gw0 should be always kept.

So, route_windows.go should not remove routes related to Service.

[tnqn]

1. Route deletion triggered by AntreaProxy is normal workflow that agent receives the service deletion event. The point of Reconcile is to delete stale routes for the services that are deleted when the agent is not running.
2. The code is self-contradictory: Reconcile takes all clusterIPs as the 2nd argument and uses it to exclude valid service routes but listRoutes doesn't even return any service routes. Why [Windows] Use antrea-proxy for Cluster IP Service traffic from Windows host #2235 even added the second argument?

[lzhecheng]

@tnqn you're right. I let listRoutes() returns AntreaProxy Service routes as well.

[tnqn]

LGTM

[tnqn]

/test-all

[tnqn]

/test-windows-all

[tnqn]

@lzhecheng "jenkins-windows-proxyall-e2e" failed, could you check?

[lzhecheng]

@tnqn it's not "failed" but not triggered. Currently, test-windows-all doesn't has proxyall-e2e.
/test-windows-proxyall-e2e /test-windows-conformance /test-windows-networkpolicy

[lzhecheng]

@tnqn as your can see, current design is test-windows-all for proxyall disabled and test-windows-proxyall-all for proxyall enabled. Do you think it is better to include proxy-e2e in test-windows-all and remove test-windows-proxyall-all since no proxyall-conformance and proxyall-netpol?

https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-your-pr-verified-by-ci

[tnqn]

@lzhecheng Including proxyAll test in test-windows-all sounds good to me.