Add ARP/NDP responders to external IP assigner #3318

hty690 · 2022-02-15T11:18:58Z

Apart from assigning IPs to the dummy interface, this commit will also create
raw sockets and listen for ARP requests packets (IPv4) and Neighbor Solicitation
packets (IPv6). This fixes the issue that Egress cannot work in IPv6 mode
as the system would not reply to Neighbor Advertisement from external
interfaces if the IP is assigned to the dummy interface. The IP assigner
will skip managing the dummy device if dummyDeviceName is empty. This avoids
the kernel creating a local route, which has a higher priority than the routes
installed by antrea-proxy in proxyAll mode.

Signed-off-by: Tianyi Huang hty690@126.com

pkg/agent/ipassigner/virtual/arp_responder.go

jianjuns · 2022-02-16T00:13:12Z

pkg/agent/ipassigner/virtual/arp_responder.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package virtual


But how if we need both of binding IP to an interface and replying ARP (e.g. to fix Egress IPv6 support)? Should we make binding IP and ARP two configurable feature of a single IP assigner?

I have merged the assigners into one to fix the Egress issue as well.

wenqiq · 2022-02-16T09:35:04Z

go.mod

+	github.com/mdlayher/arp v0.0.0-20191213142603-f72070a231fc
+	github.com/mdlayher/ethernet v0.0.0-20190606142754-0394541c37b7
+	github.com/mdlayher/ndp v0.0.0-20210831201139-f982b8766fb5
+	github.com/mdlayher/raw v0.0.0-20211126142749-4eae47f3d54b


Why do we import those packages as we have implemented some functions in agent/util/arping and agent/util/arping?

This feature does not only need to send GARP or NS over the network interface. We also need to listen to ARP and NS requests and process the queries accordingly. Without these packages, we may need to manage the raw sockets, parse the ethernet headers, etc.

jianjuns

In commit message:

if the IP were assigned to the dummy interface.

were -> is

This avoids the kernel creating local routes for the external IP, which have

"creates a local route", "which has"

jianjuns · 2022-02-16T19:29:40Z

pkg/agent/ipassigner/ip_assigner_linux.go

@@ -173,3 +215,14 @@ func (a *ipAssigner) AssignedIPs() sets.String {
 	// Return a copy.
 	return a.assignedIPs.Union(nil)
 }
+
+// Run starts the IP assigner.


starts ARP responder and NDP responder?

jianjuns · 2022-02-16T19:33:04Z

pkg/agent/ipassigner/ip_assigner_linux.go

-	if err := a.loadIPAddresses(); err != nil {
-		return nil, fmt.Errorf("error when loading IP addresses from the system: %v", err)
+	if ipv4 != nil {
+		arpResonder, err := responder.NewARPResponder(externalInterface)


As ARP is not always needed, can we pass a boolean to enable ARP responder? We can add another for NDP too.

Or another we do not enable ARP when dummyDeviceName is not "".

I do not see you changed this?

jianjuns · 2022-02-16T19:33:20Z

pkg/agent/ipassigner/ip_assigner_linux.go

+	if ipv6 != nil {
+		ndpResponder, err := responder.NewNDPResponder(externalInterface)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create ARP responder for link %s: %v", externalInterface.Name, err)


jianjuns · 2022-02-16T19:34:46Z

pkg/agent/ipassigner/ip_assigner_linux.go

+	if dummyDeviceName != "" {
+		dummyDevice, err := ensureDummyDevice(dummyDeviceName)
+		if err != nil {
+			return nil, fmt.Errorf("error when ensuring dummy device exist: %v", err)


exist -> exists

ensuring -> checking

ensureDummyDevice creates the dummy device if it doesn't exist, not just checks its existence

jianjuns · 2022-02-16T19:39:24Z

pkg/agent/ipassigner/responder/arp_responder.go

+	return r.iface.Name
+}
+
+func (r *arpResponder) AssignIP(ip net.IP) error {


Probably rename to "AddIP", "RemoveIP"? It is not really about assignment.

update. thanks.

jianjuns · 2022-02-16T19:40:53Z

pkg/agent/ipassigner/responder/arp_responder.go

+}
+
+// GratuitousARP sends an gratuitous ARP packet for the IP.
+func (r *arpResponder) Gratuitous(ip net.IP) error {


How about naming it Advertise()?

update. thanks.

jianjuns · 2022-02-16T19:41:47Z

pkg/agent/ipassigner/responder/arp_responder.go

+		if err != nil {
+			return err
+		}
+		return r.conn.WriteTo(pkt, ethernet.Broadcast)


Probably we should send out multiple GARPs. We can do it in another PR.

I am not clear for multiple GARPs. Could you help to provide more context for this?

Similar to

antrea/pkg/agent/cniserver/interface_configuration_linux.go

Lines 312 to 326 in 15de4cf

ticker := time.NewTicker(50 * time.Millisecond)

defer ticker.Stop()

count := 0

for {

// Send gratuitous ARP to network in case of stale mappings for this IP address

// (e.g. if a previous - deleted - Pod was using the same IP).

if err := arping.GratuitousARPOverIface(targetIP, iface); err != nil {

klog.Warningf("Failed to send gratuitous ARP #%d: %v", count, err)

}

count++

if count == 3 {

break

}

<-ticker.C

}

Thanks for the pointer. Do we need this for external IPs to send multiple GARPs to the transport interface?

jianjuns · 2022-02-16T19:42:50Z

pkg/agent/ipassigner/responder/arp_responder.go

+	if pkt.Operation != arp.OperationRequest {
+		return nil
+	}
+	if _, ok := r.assignedIPs[pkt.TargetIP.String()]; !ok {


Need to protect the map?

If it is just a set, we can use a thread-safe type. @tnqn might have a recommendation.

suggested to use sets.String but it's not thread-safe either. It still needs a lock, but it's a common pattern in our code.

If we need not a lock for other reasons, we can use sync.Map (which should work well for a set that no need to read the value).

codecov-commenter · 2022-02-17T03:59:52Z

Codecov Report

Merging #3318 (4918998) into main (15de4cf) will decrease coverage by 7.32%.
The diff coverage is 55.92%.

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   60.95%   53.62%   -7.33%     
==========================================
  Files         266      374     +108     
  Lines       26508    51235   +24727     
==========================================
+ Hits        16159    27477   +11318     
- Misses       8597    21345   +12748     
- Partials     1752     2413     +661

Flag	Coverage Δ
e2e-tests	`50.93% <34.04%> (?)`
integration-tests	`35.79% <ø> (?)`
kind-e2e-tests	`47.38% <29.08%> (-0.46%)`	⬇️
unit-tests	`41.76% <60.30%> (+0.12%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/ipassigner/ip_assigner_linux.go	`52.55% <43.83%> (-7.72%)`	⬇️
pkg/agent/ipassigner/responder/ndp_responder.go	`47.05% <47.05%> (ø)`
pkg/agent/ipassigner/responder/arp_responder.go	`70.58% <70.58%> (ø)`
...g/agent/controller/serviceexternalip/controller.go	`80.34% <78.78%> (-0.52%)`	⬇️
pkg/agent/controller/egress/egress_controller.go	`69.16% <100.00%> (+30.35%)`	⬆️
pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (-40.36%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`48.71% <0.00%> (-31.57%)`	⬇️
pkg/agent/flowexporter/connections/conntrack.go	`45.71% <0.00%> (-30.48%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`61.46% <0.00%> (-29.97%)`	⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go	`63.29% <0.00%> (-29.31%)`	⬇️
... and 339 more

cmd/antrea-agent/agent.go

pkg/agent/controller/serviceexternalip/controller.go

pkg/agent/ipassigner/ip_assigner_linux.go

tnqn · 2022-02-17T15:18:36Z

pkg/agent/ipassigner/ip_assigner_linux.go

-	if err := func() error {
-		a.mutex.Lock()
-		defer a.mutex.Unlock()
+	if a.isIPAssigned(ip) {


In Egress, it's possible that multiple Egresses share same static egress IP, then it could happen that two workers are assigning the same IP concurrently. Without the previous lock, the following code may be executed twice for an IP. Is it ok?

As the responders are thread-safe, do you think it is enough by adding checks of the error codes of netlink.AddrAdd/netlink.AddrDel?

I wonder if the race condition can happen between AssignIP and UnassignIP. When an IP is released from one Resource and reallocated to another Resource.
worker1 wants to remove an IP while worker2 wants to add it. worker1 starts first.

worker1 sees IP is assigned and starts removing it without holding the lock.

worker2 sees IP is assigned and returns directly.

worker1 removes the IP.

But it seems if worker2 starts first, worker1 may still remove the IP even with the lock. This is not an issue when there is an localIPDetector as removing IP could trigger resync. As it's no longer the case for Service external IP, maybe it needs to track reference number for such case, as well as holding the lock. Then:
If worker1 starts first:

worker1 sees IP is assigned and starts removing it.

worker2 waits for the lock.

worker1 removes IP.

worker2 gets the lock, and adds the IP back.

If worker2 starts first:

worker2 sees IP is assigned and increase reference count, returns.

worker1 sees IP is assigned and decrease reference count, returns.

Reference count can be counted from a set of entities names which are associated with the IP.

I have added the reference count mechanism for the agent Service external IP controller.

The issue could still happen without the lock when it's used in EgressController. When Egress A is removed and its IP is allocated to Egress B. worker1 handles removal of A, worker2 handles sync of B.

worker1 sees IP is assigned and starts removing it without holding the lock.

worker2 sees IP is already assigned and returns directly.

worker1 removes the IP, which triggers resync of B.

worker2 resyncs B, sees IP is already assigned and returns directly.

Make sense. I revert the changes to make the assign/unassign operation atomic. Thanks!

pkg/agent/ipassigner/ip_assigner_linux.go

tnqn · 2022-02-17T15:22:28Z

pkg/agent/ipassigner/ip_assigner_linux.go

-	defer a.mutex.Unlock()
-
-	if !a.assignedIPs.Has(ip) {
+	if !a.isIPAssigned(ip) {


ditto, there may be a race condition between Assign and Unassign as well.

tnqn · 2022-02-17T15:58:53Z

pkg/agent/ipassigner/responder/arp_responder.go

+}
+
+// Advertise sends an gratuitous ARP packet for the IP.
+func (r *arpResponder) Advertise(ip net.IP) error {


Should AddIP calls advertise directly since it's by design assigning an IP should advertise it as well, which could make IPAssigner simpler?

Changed accordingly. Thanks.

Then it can be private? And we can remove the IP version check as it's an internal method whose argument has been checked by public methods.

tnqn · 2022-02-17T16:05:49Z

pkg/agent/ipassigner/responder/arp_responder.go

+		if err != nil {
+			return err
+		}
+		return r.conn.WriteTo(pkt, ethernet.Broadcast)


Similar to

antrea/pkg/agent/cniserver/interface_configuration_linux.go

Lines 312 to 326 in 15de4cf

ticker := time.NewTicker(50 * time.Millisecond)

defer ticker.Stop()

count := 0

for {

// Send gratuitous ARP to network in case of stale mappings for this IP address

// (e.g. if a previous - deleted - Pod was using the same IP).

if err := arping.GratuitousARPOverIface(targetIP, iface); err != nil {

klog.Warningf("Failed to send gratuitous ARP #%d: %v", count, err)

}

count++

if count == 3 {

break

}

<-ticker.C

}

pkg/agent/ipassigner/responder/arp_responder.go

pkg/agent/ipassigner/responder/ndp_responder.go

test/e2e/service_externalip_test.go

tnqn · 2022-02-18T14:46:23Z

pkg/agent/ipassigner/ip_assigner_linux.go

-	if err := func() error {
-		a.mutex.Lock()
-		defer a.mutex.Unlock()
+	if a.isIPAssigned(ip) {


I wonder if the race condition can happen between AssignIP and UnassignIP. When an IP is released from one Resource and reallocated to another Resource.
worker1 wants to remove an IP while worker2 wants to add it. worker1 starts first.

worker1 sees IP is assigned and starts removing it without holding the lock.

worker2 sees IP is assigned and returns directly.

worker1 removes the IP.

But it seems if worker2 starts first, worker1 may still remove the IP even with the lock. This is not an issue when there is an localIPDetector as removing IP could trigger resync. As it's no longer the case for Service external IP, maybe it needs to track reference number for such case, as well as holding the lock. Then:
If worker1 starts first:

worker1 sees IP is assigned and starts removing it.

worker2 waits for the lock.

worker1 removes IP.

worker2 gets the lock, and adds the IP back.

If worker2 starts first:

worker2 sees IP is assigned and increase reference count, returns.

worker1 sees IP is assigned and decrease reference count, returns.

Reference count can be counted from a set of entities names which are associated with the IP.

tnqn · 2022-02-18T14:49:42Z

pkg/agent/ipassigner/responder/arp_responder.go

+}
+
+// Advertise sends an gratuitous ARP packet for the IP.
+func (r *arpResponder) Advertise(ip net.IP) error {


Then it can be private? And we can remove the IP version check as it's an internal method whose argument has been checked by public methods.

tnqn · 2022-02-18T14:51:55Z

pkg/agent/ipassigner/responder/arp_responder.go

+		klog.InfoS("Assigned IP to ARP responder", "ip", ip, "interface", r.iface.Name)
+		err := r.Advertise(ip)
+		if err != nil {
+			klog.Warningf("failed to advertise for IP %s: %v", ip, err)


Why don't log error?

tnqn · 2022-02-18T14:59:20Z

pkg/agent/ipassigner/responder/ndp_responder.go

+	return r.iface.Name
+}
+
+func (r *ndpResponder) Advertise(ip net.IP) error {


test/e2e/service_externalip_test.go

jianjuns · 2022-02-22T00:53:05Z

pkg/agent/ipassigner/responder/arp_responder.go

+	iface       *net.Interface
+	conn        *arp.Client
+	assignedIPs sets.String
+	mutex       sync.Mutex


sync.Map is indeed a simpler choice here, but I know you need a mutex in ndpResponder for other reasons, so maybe you prefer to use the same way for both.

Yes. I think the mutex here is better for consistency.

jianjuns · 2022-02-22T05:52:18Z

pkg/agent/ipassigner/ip_assigner_linux.go

+func (a *ipAssigner) Run(ch <-chan struct{}) {
+	// Start the ARP responder only when the dummy device is not created. The kernel will handle ARP requests
+	// for IPs assigned to the dummy devices by default.
+	// TODO: Check arp_ignore sysctl parameter of the transport interface to determin whether to start


determin -> determine

the arp_ignore sysctl parameter

pkg/agent/controller/serviceexternalip/controller.go

pkg/agent/controller/serviceexternalip/controller_test.go

tnqn · 2022-02-22T05:43:02Z

pkg/agent/controller/serviceexternalip/controller_test.go

@@ -416,21 +437,38 @@ func TestUpdateService(t *testing.T) {
 			},
 			expectError: false,
 		},
+		{
+			name:            "Service updated external IP and local Node selected but other Service still owns the assigned IP",


good test case

tnqn · 2022-02-22T05:55:52Z

pkg/agent/ipassigner/ip_assigner_linux.go

-	if err := func() error {
-		a.mutex.Lock()
-		defer a.mutex.Unlock()
+	if a.isIPAssigned(ip) {


The issue could still happen without the lock when it's used in EgressController. When Egress A is removed and its IP is allocated to Egress B. worker1 handles removal of A, worker2 handles sync of B.

worker1 sees IP is assigned and starts removing it without holding the lock.

worker2 sees IP is already assigned and returns directly.

worker1 removes the IP, which triggers resync of B.

worker2 resyncs B, sees IP is already assigned and returns directly.

Apart from assigning IPs to the dummy interface, this commit will also create raw sockets and listen for ARP requests packets (IPv4) and Neighbor Solicitation packets (IPv6). This fixes the issue that Egress cannot work in IPv6 mode as the system would not reply to Neighbor Advertisement from external interfaces if the IP is assigned to the dummy interface. The IP assigner will skip managing the dummy device if dummyDeviceName is empty. This avoids the kernel creating a local route, which has a higher priority than the routes installed by antrea-proxy in proxyAll mode. Signed-off-by: Tianyi Huang <hty690@126.com>

tnqn

LGTM, thanks

hty690 · 2022-02-22T11:10:37Z

/test-ipv6-all
/test-ipv6-only-all

hty690 · 2022-02-22T22:59:27Z

/test-integration

hty690 · 2022-02-22T23:10:55Z

/test-integration

jianjuns · 2022-02-23T03:55:34Z

/test-e2e

hty690 · 2022-02-23T09:04:19Z

/test-ipv6-e2e
/test-ipv6-only-e2e

Apart from assigning IPs to the dummy interface, this commit will also create raw sockets and listen for ARP requests packets (IPv4) and Neighbor Solicitation packets (IPv6). This fixes the issue that Egress cannot work in IPv6 mode as the system would not reply to Neighbor Advertisement from external interfaces if the IP is assigned to the dummy interface. The IP assigner will skip managing the dummy device if dummyDeviceName is empty. This avoids the kernel creating a local route, which has a higher priority than the routes installed by antrea-proxy in proxyAll mode. Signed-off-by: Tianyi Huang <hty690@126.com>

hty690 force-pushed the arp_ndp_socket branch 2 times, most recently from ea5bd69 to c8a5073 Compare February 15, 2022 13:48

jianjuns reviewed Feb 16, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch 2 times, most recently from 1561e81 to f1f3f5c Compare February 16, 2022 07:55

hty690 changed the title ~~Fix ServiceExternalIP feature cannot work with antrea-proxy in proxyAll mode~~ Add ARP/NDP responders to external IP assigner Feb 16, 2022

hty690 force-pushed the arp_ndp_socket branch from f1f3f5c to 50f2015 Compare February 16, 2022 07:57

wenqiq reviewed Feb 16, 2022

View reviewed changes

jianjuns reviewed Feb 16, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch from 50f2015 to 91d822f Compare February 17, 2022 02:34

hty690 force-pushed the arp_ndp_socket branch 5 times, most recently from 8de0057 to 72f5bf1 Compare February 17, 2022 10:46

tnqn reviewed Feb 17, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch 3 times, most recently from 42e1912 to 3e427cd Compare February 18, 2022 07:27

tnqn reviewed Feb 18, 2022

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Feb 18, 2022

hty690 force-pushed the arp_ndp_socket branch from 3e427cd to 848c108 Compare February 21, 2022 08:39

jianjuns reviewed Feb 22, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch from 848c108 to 83a86d0 Compare February 22, 2022 01:08

jianjuns reviewed Feb 22, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch from 83a86d0 to c9a89ae Compare February 22, 2022 05:56

tnqn reviewed Feb 22, 2022

View reviewed changes

hty690 force-pushed the arp_ndp_socket branch from c9a89ae to 4918998 Compare February 22, 2022 06:20

tnqn approved these changes Feb 22, 2022

View reviewed changes

antrea-io deleted a comment from tnqn Feb 23, 2022

jianjuns approved these changes Feb 23, 2022

View reviewed changes

jianjuns merged commit a87ef55 into antrea-io:main Feb 23, 2022

hty690 deleted the arp_ndp_socket branch February 24, 2022 00:35

	ticker := time.NewTicker(50 * time.Millisecond)
	defer ticker.Stop()
	count := 0
	for {
	// Send gratuitous ARP to network in case of stale mappings for this IP address
	// (e.g. if a previous - deleted - Pod was using the same IP).
	if err := arping.GratuitousARPOverIface(targetIP, iface); err != nil {
	klog.Warningf("Failed to send gratuitous ARP #%d: %v", count, err)
	}
	count++
	if count == 3 {
	break
	}
	<-ticker.C
	}

Add ARP/NDP responders to external IP assigner #3318

Add ARP/NDP responders to external IP assigner #3318

Conversation

hty690 commented Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hty690 Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 17, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

hty690 commented Feb 22, 2022

hty690 commented Feb 22, 2022

hty690 commented Feb 22, 2022

jianjuns commented Feb 23, 2022

hty690 commented Feb 23, 2022

hty690 commented Feb 15, 2022 •

edited

Loading

hty690 Feb 17, 2022 •

edited

Loading

codecov-commenter commented Feb 17, 2022 •

edited

Loading