Move GetNodeAddr function to k8s util package

[tnqn]

The function is consumed by antctl and several places in antrea-agent. Importing it from "pkg/agent/controller/noderoute" is weird and complicate the module dependency. This patch moves it to k8s util package which contains K8s specific helper functions. With it, antctl binary size is reduced about 8KB.

Besides, it moves pkg/k8s to pkg/util/k8s.

Signed-off-by: Quan Tian [email protected]

[codecov-commenter]

Codecov Report

Merging #2191 (6bbaedb) into main (a9c7744) will decrease coverage by 0.10%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##             main    #2191      +/-   ##
==========================================
- Coverage   61.30%   61.19%   -0.11%     
==========================================
  Files         273      274       +1     
  Lines       20646    20646              
==========================================
- Hits        12656    12635      -21     
- Misses       6688     6699      +11     
- Partials     1302     1312      +10     

Flag	Coverage Δ
e2e-tests	∅ <ø> (?)
kind-e2e-tests	52.09% <56.25%> (-0.22%)	⬇️
unit-tests	40.95% <75.00%> (-0.09%)	⬇️

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	46.04% <50.00%> (-0.35%)	⬇️
pkg/k8s/node.go	84.61% <84.61%> (ø)
pkg/agent/agent.go	47.74% <100.00%> (ø)
...gent/controller/networkpolicy/status_controller.go	72.60% <0.00%> (-5.48%)	⬇️
pkg/agent/cniserver/ipam/ipam_service.go	63.15% <0.00%> (-5.27%)	⬇️
pkg/agent/cniserver/ipam/ipam_delegator.go	48.83% <0.00%> (-4.66%)	⬇️
pkg/agent/cniserver/pod_configuration.go	53.90% <0.00%> (-3.52%)	⬇️
pkg/agent/cniserver/server.go	68.91% <0.00%> (-1.93%)	⬇️
pkg/controller/networkpolicy/status_controller.go	86.45% <0.00%> (-0.65%)	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	69.35% <0.00%> (-0.65%)	⬇️
... and 3 more

[tnqn]

/test-all

[antoninbas]

I wonder if k8s should be kept as a top-level package, or should be moved to pkg/util/k8s

LGTM

[antoninbas]

I notice that this issue (kubernetes/kubernetes#91940) has been fixed upstream, so maybe we should open an issue to address this and update GetNodeAddr

[tnqn]

While I was about to open an issue, I found I was not sure what change we need to make here. I see current usages of this function expects a Node IP so they can use it to create tunnel, identify network interface, or connect to the server. So it seems fine if there are two internal addresses or two external addresses, except the returned address family is not determinate. What’s the required change on your mind?

[antoninbas]

I may be wrong, but don't we need to get both an IPv4 address and an IPv6 address for the Node if we want to support dual-stack in noEncap mode? So this function should be able to return both addresses?

Right now we use the one Node IP to install both routes in noEncap mode, but that seems wrong to me. I don't think we have tested noEncap for dual-stack clusters and my expectation is that it may not work:

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

Lines 485 to 490 in fa54190

	for peerPodCIDR, peerGatewayIP := range peerConfig {
	if err := c.routeClient.AddRoutes(peerPodCIDR, nodeName, peerNodeIP, peerGatewayIP); err != nil {
	return err
	}
	peerGatewayIPs = append(peerGatewayIPs, peerGatewayIP)
	}

Adding @lzhecheng as this may be related to #136 (comment)

[tnqn]

I see, you mean we need to setup one IPv4 route and one IPv6 route for dual-stack clusters. Since #136 is still open, maybe we could just use that issue to track.

[antoninbas]

That works for me. Thanks for updating the issue (I mean leaving a comment there).

[tnqn]

@antoninbas thanks for the review.

I wonder if k8s should be kept as a top-level package, or should be moved to pkg/util/k8s

Moving to pkg/util/k8s is fine by me, I have made the change.

[tnqn]

/test-all

[tnqn]

/test-conformance
/test-e2e

[tnqn]

/test-e2e