Detect Network Instance IP subnet conflict #3823

milan-zededa · 2024-03-15T18:11:43Z

Zedrouter is able to detect if NI IP subnet overlaps with the subnet of another NI or with a device port network. When a conflict is detected for a new NI, it is blocked from being created. If IP conflict arises for an already created NI (e.g. a device port later received IP from an overlapping subnet), the NI is unconfigured and VIFs are set DOWN (not deleted). In both cases an error is reported for the NI.

This is crucial because, previously, a NI with a subnet overlapping with a device port network would result in the device permanently losing controller connectivity (due to routing conflicts), with very limited options to restore connectivity and make the device manageable again. Instead, the device should remain online, inform the user about the issue, and allow to correct the IP configuration.

When IP conflict is detected for an already created NI with an app connected to it, we intentionally do not halt and undeploy the app. Instead, the app is left running, just VIFs connected to the problematic NI lose their connectivity. This allows the user to fix the network config or at least backup the application data when IP conflict arises.

To enable the deletion of NI when an IP conflict is detected, followed by its later recreation when the conflict is resolved, and to reconnect already running apps, few enhancements had to be made to the NI Reconciler, particularly in the area of VIF bridging.

codecov · 2024-03-15T18:26:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (1cd8f6a) to head (7aa32d4).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3823   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

milan-zededa · 2024-03-18T14:54:18Z

Note that I will be backporting this to 11.0 and 10.4 (in 9.4 the zedrouter is vastly different and would require to make the patch from the ground up).

eriknordmark · 2024-03-18T22:11:18Z

pkg/pillar/cmd/zedrouter/validation.go

 	if config.Subnet.IP == nil || config.Subnet.IP.IsUnspecified() {
-		return false, fmt.Errorf("subnet unspecified for %s-%s: %+v",
+		return fmt.Errorf("subnet unspecified for %s-%s: %+v",


This assumes we never call this function on a switch network instance or on some future NI type where the IP addresses can be assigned after the NI has been created.
Why do we need to report it as a conflict?

This function doNetworkInstanceSubnetSanityCheck is called only if config.IpType is not AddressTypeNone, so it will not be called for switch network instance.
Note that IP conflict detection was now moved to a separate function: checkNetworkInstanceIPConflicts.

eriknordmark · 2024-03-18T22:12:38Z

pkg/pillar/cmd/zedrouter/zedrouter.go

@@ -244,22 +244,22 @@ func (z *zedrouter) init() (err error) {

 	z.peUsagePersist, err = persistcache.New(types.PersistCachePatchEnvelopesUsage)
 	if err != nil {
-		log.Fatal(err)
+		return err


Is this a separate fix (which would make sense in a separate commit)?

Yes, it is a minor unrelated fix, I will move it to a separate commit.

eriknordmark · 2024-03-18T22:18:41Z

To enable the deletion of NI when an IP conflict is detected, followed by its later recreation when the conflict is resolved, and to reconnect already running apps, few enhancements had to be made to the NI Reconciler, particularly in the area of VIF bridging.

Is this the reason for the introduction of the BridgePort and the various BridgeIfName changes?

eriknordmark

Run tests

milan-zededa · 2024-03-19T09:54:02Z

To enable the deletion of NI when an IP conflict is detected, followed by its later recreation when the conflict is resolved, and to reconnect already running apps, few enhancements had to be made to the NI Reconciler, particularly in the area of VIF bridging.

Is this the reason for the introduction of the BridgePort and the various BridgeIfName changes?

Yes, correct. Previously we didn't have the "capability" to attach VIF to a bridge and were relying on the hypervisor to do it for us. Now, to support reconnecting VIFs after the NI (and the bridge) was recreated, we have to re-attach VIFs to the bridge from the zedrouter. This is represented by BridgePort.

Zedrouter is able to detect if NI IP subnet overlaps with the subnet of another NI or with a device port network. When a conflict is detected for a new NI, it is blocked from being created. If IP conflict arises for an already created NI (e.g. a device port later received IP from an overlapping subnet), the NI is unconfigured and VIFs are set DOWN (not deleted). In both cases an error is reported for the NI. This is crucial because, previously, an NI with a subnet overlapping with a device port network would result in the device permanently losing controller connectivity (due to routing conflicts), with very limited options to restore connectivity and make the device manageable again. Instead, the device should remain online, inform the user about the issue, and allow to correct the IP configuration. When IP conflict is detected for an already created NI with an app connected to it, we intentionally do not halt and undeploy the app. Instead, the app is left running, just VIFs connected to the problematic NI lose their connectivity. This allows the user to fix the network config or at least backup the application data when IP conflict arises. To enable the deletion of NI when an IP conflict is detected, followed by its later recreation when the conflict is resolved, and to reconnect already running apps, few enhancements had to be made to the NI Reconciler, particularly in the area of VIF bridging. Signed-off-by: Milan Lenco <milan@zededa.com>

zedrouter.Run() already calls log.Fatal (with os.Exit inside) when error is returned from init() or run() methods. Signed-off-by: Milan Lenco <milan@zededa.com>

christoph-zededa · 2024-03-19T12:51:08Z

Instead of detecting the subnet overlap, wouldn't it also be possible to bind the socket to the interface (syscall.SetsockoptString(int(fd), syscall.SOL_SOCKET, syscall.SO_BINDTODEVICE, device)) when sending requests?

eriknordmark · 2024-03-19T18:29:57Z

Instead of detecting the subnet overlap, wouldn't it also be possible to bind the socket to the interface (syscall.SetsockoptString(int(fd), syscall.SOL_SOCKET, syscall.SO_BINDTODEVICE, device)) when sending requests?

@christoph-zededa
The EVE management traffic already does that, but using a different mechanism (binding to the IP address of the interface and then using an IP rule based on that source IP address to select the routing table, and that routing table contains the routes out that interface.)
But that is insufficient when there are some IP subnet conflicts.

milan-zededa · 2024-03-19T18:48:55Z

@christoph-zededa @eriknordmark Note that I plan to investigate options how to isolate network instances from uplinks and allow even overlapping subnets. In the past, I tested VRFs and network namespaces. Both work but introduce quite some complexity and data-plane overhead. Alternative solution could be to rework policy-based routing and to experiment with SO_BINDTODEVICE as you suggested. The problem with interface binding is that it does not cover all cases (e.g. not only pillar uses the mgmt IP) and app traffic from the conflicting NI will still have broken routing. In any case, I would need more time to research all the options and plan to do it in the next development cycle. This PR is a quick patch just to prevent device loosing connectivity (and all other smaller problems that subnet overlap creates). Simple enough so that it can be backported to LTS versions.

christoph-zededa · 2024-03-19T23:40:13Z

@christoph-zededa @eriknordmark Note that I plan to investigate options how to isolate network instances from uplinks and allow even overlapping subnets. In the past, I tested VRFs and network namespaces. Both work but introduce quite some complexity and data-plane overhead. Alternative solution could be to rework policy-based routing and to experiment with SO_BINDTODEVICE as you suggested. The problem with interface binding is that it does not cover all cases (e.g. not only pillar uses the mgmt IP) and app traffic from the conflicting NI will still have broken routing. In any case, I would need more time to research all the options and plan to do it in the next development cycle. This PR is a quick patch just to prevent device loosing connectivity (and all other smaller problems that subnet overlap creates). Simple enough so that it can be backported to LTS versions.

Thank you for the explanation - I see it would take some more time to implement.

github-actions bot requested review from christoph-zededa, eriknordmark and rouming March 15, 2024 18:12

milan-zededa requested a review from rene March 18, 2024 09:09

milan-zededa added the stable Should be backported to stable release(s) label Mar 18, 2024

milan-zededa force-pushed the detect-subnet-overlap2 branch 3 times, most recently from 1816ee2 to 46c0d0d Compare March 18, 2024 10:50

milan-zededa marked this pull request as ready for review March 18, 2024 14:52

eriknordmark reviewed Mar 18, 2024

View reviewed changes

eriknordmark approved these changes Mar 19, 2024

View reviewed changes

milan-zededa added 2 commits March 19, 2024 11:04

Return error from zedrouter.init (instead of os.Exit)

7aa32d4

zedrouter.Run() already calls log.Fatal (with os.Exit inside) when error is returned from init() or run() methods. Signed-off-by: Milan Lenco <milan@zededa.com>

milan-zededa force-pushed the detect-subnet-overlap2 branch from 46c0d0d to 7aa32d4 Compare March 19, 2024 10:10

github-actions bot requested a review from eriknordmark March 19, 2024 10:10

eriknordmark merged commit 6cc6762 into lf-edge:master Mar 20, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect Network Instance IP subnet conflict #3823

Detect Network Instance IP subnet conflict #3823

milan-zededa commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

milan-zededa commented Mar 18, 2024

eriknordmark Mar 18, 2024

milan-zededa Mar 19, 2024

eriknordmark Mar 18, 2024

milan-zededa Mar 19, 2024

milan-zededa Mar 19, 2024

eriknordmark commented Mar 18, 2024

eriknordmark left a comment

milan-zededa commented Mar 19, 2024 •

edited

Loading

christoph-zededa commented Mar 19, 2024

eriknordmark commented Mar 19, 2024

milan-zededa commented Mar 19, 2024 •

edited

Loading

christoph-zededa commented Mar 19, 2024

Detect Network Instance IP subnet conflict #3823

Detect Network Instance IP subnet conflict #3823

Conversation

milan-zededa commented Mar 15, 2024 • edited Loading

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

milan-zededa commented Mar 18, 2024

eriknordmark Mar 18, 2024

Choose a reason for hiding this comment

milan-zededa Mar 19, 2024

Choose a reason for hiding this comment

eriknordmark Mar 18, 2024

Choose a reason for hiding this comment

milan-zededa Mar 19, 2024

Choose a reason for hiding this comment

milan-zededa Mar 19, 2024

Choose a reason for hiding this comment

eriknordmark commented Mar 18, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

milan-zededa commented Mar 19, 2024 • edited Loading

christoph-zededa commented Mar 19, 2024

eriknordmark commented Mar 19, 2024

milan-zededa commented Mar 19, 2024 • edited Loading

christoph-zededa commented Mar 19, 2024

milan-zededa commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

milan-zededa commented Mar 19, 2024 •

edited

Loading

milan-zededa commented Mar 19, 2024 •

edited

Loading