Detect Network Instance IP subnet conflict

pkg/pillar/cmd/zedrouter/networkinstance.go

@@ -66,6 +66,7 @@ func (z *zedrouter) getNIBridgeConfig(
 		MACAddress: status.BridgeMac,
 		IPAddress:  ipAddr,
 		Uplink:     z.getNIUplinkConfig(status),
+		IPConflict: status.IPConflict,
 	}
 }
 
@@ -297,26 +298,57 @@ func (z *zedrouter) delNetworkInstance(status *types.NetworkInstanceStatus) {
 	z.deleteNetworkInstanceMetrics(status.Key())
 }
 
-// Called when a NetworkInstance is deleted or modified to check if there are other
-// NIs that previously could not be configured due to inter-NI config conflicts.
-func (z *zedrouter) checkConflictingNetworkInstances() {
+// Called when a NetworkInstance is deleted or modified, or when a device port IP is
+// added or removed, to check if there are new IP conflicts or if some existing
+// have been resolved.
+func (z *zedrouter) checkAllNetworkInstanceIPConflicts() {
 	for _, item := range z.pubNetworkInstanceStatus.GetAll() {
 		niStatus := item.(types.NetworkInstanceStatus)
-		if !niStatus.NIConflict {
-			continue
-		}
 		niConfig := z.lookupNetworkInstanceConfig(niStatus.Key())
 		if niConfig == nil {
 			continue
 		}
-		niConflict, err := z.doNetworkInstanceSanityCheck(niConfig)
-		if err == nil && niConflict == false {
-			// Try to re-create the network instance now that the conflict is gone.
-			z.log.Noticef("Recreating NI %s (%s) now that inter-NI conflict "+
-				"is not present anymore", niConfig.UUID, niConfig.DisplayName)
-			// First release whatever has been already allocated for this NI.
-			z.delNetworkInstance(&niStatus)
-			z.handleNetworkInstanceCreate(nil, niConfig.Key(), *niConfig)
+		conflictErr := z.checkNetworkInstanceIPConflicts(niConfig)
+		if conflictErr == nil && niStatus.IPConflict {
+			// IP conflict was resolved.
+			if niStatus.Activated {
+				// Local NI was initially activated prior to the IP conflict.
+				// Subsequently, when the IP conflict arose, it was almost completely
+				// un-configured (only preserving app VIFs) to keep device connectivity
+				// unaffected. Now, it can be restored to full functionality.
+				z.log.Noticef("Updating NI %s (%s) now that IP conflict "+
+					"is not present anymore", niConfig.UUID, niConfig.DisplayName)
+				niStatus.IPConflict = false
+				niStatus.ClearError()
+				// This also publishes the new status.
+				z.doUpdateActivatedNetworkInstance(*niConfig, &niStatus)
+			} else {
+				// NI is not in an active state (nothing configured in the network stack).
+				// We can simply re-create the network instance now that the IP conflict
+				// is gone.
+				z.log.Noticef("Recreating NI %s (%s) now that IP conflict "+
+					"is not present anymore", niConfig.UUID, niConfig.DisplayName)
+				// First release whatever has been already allocated for this NI.
+				z.delNetworkInstance(&niStatus)
+				z.handleNetworkInstanceCreate(nil, niConfig.Key(), *niConfig)
+			}
+		}
+		if conflictErr != nil && !niStatus.IPConflict {
+			// New IP conflict arose.
+			z.log.Error(conflictErr)
+			niStatus.IPConflict = true
+			niStatus.SetErrorNow(conflictErr.Error())
+			z.publishNetworkInstanceStatus(&niStatus)
+			if niStatus.Activated {
+				// Local NI is already activated. Instead of removing it and halting
+				// all connected applications (which can lead to loss of data), we
+				// un-configure everything but app VIFs, which will be set DOWN
+				// on the host side. User has a chance to fix the configuration.
+				// When IP conflict is removed, NI will be automatically fully restored.
+				z.log.Noticef("Updating NI %s (%s) after detecting an IP conflict (%s)",
+					niConfig.UUID, niConfig.DisplayName, conflictErr)
+				z.doUpdateActivatedNetworkInstance(*niConfig, &niStatus)
+			}
 		}
 	}
 }

pkg/pillar/cmd/zedrouter/pubsubhandlers.go

@@ -108,6 +108,10 @@ func (z *zedrouter) handleDNSImpl(ctxArg interface{}, key string,
 		z.initReconcileDone = true
 	}
 
+	// A new IP address may have been assigned to a device port, or a previously existing
+	// one may have been removed, potentially creating or resolving an IP conflict.
+	z.checkAllNetworkInstanceIPConflicts()
+
 	// Update uplink config for network instances.
 	// Also handle (dis)appearance of uplink interfaces.
 	// Note that even if uplink interface disappears, we do not revert activated NI.
@@ -181,11 +185,19 @@ func (z *zedrouter) handleNetworkInstanceCreate(ctxArg interface{}, key string,
 		return
 	}
 
-	if niConflict, err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+	if err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+		z.log.Error(err)
+		status.SetErrorNow(err.Error())
+		status.ChangeInProgress = types.ChangeInProgressTypeNone
+		z.publishNetworkInstanceStatus(&status)
+		return
+	}
+
+	if err := z.checkNetworkInstanceIPConflicts(&config); err != nil {
 		z.log.Error(err)
 		status.SetErrorNow(err.Error())
 		status.ChangeInProgress = types.ChangeInProgressTypeNone
-		status.NIConflict = niConflict
+		status.IPConflict = true
 		z.publishNetworkInstanceStatus(&status)
 		return
 	}
@@ -301,15 +313,25 @@ func (z *zedrouter) handleNetworkInstanceModify(ctxArg interface{}, key string,
 
 	prevPortLL := status.PortLogicalLabel
 	status.NetworkInstanceConfig = config
-	if niConflict, err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+	if err := z.doNetworkInstanceSanityCheck(&config); err != nil {
+		z.log.Error(err)
+		status.SetErrorNow(err.Error())
+		status.WaitingForUplink = false
+		status.ChangeInProgress = types.ChangeInProgressTypeNone
+		z.publishNetworkInstanceStatus(status)
+		return
+	}
+
+	if err := z.checkNetworkInstanceIPConflicts(&config); err != nil {
 		z.log.Error(err)
 		status.SetErrorNow(err.Error())
+		status.IPConflict = true
 		status.WaitingForUplink = false
 		status.ChangeInProgress = types.ChangeInProgressTypeNone
-		status.NIConflict = niConflict
 		z.publishNetworkInstanceStatus(status)
 		return
 	}
+	status.IPConflict = false
 
 	// Get or (less likely) allocate a bridge number.
 	bridgeNumKey := types.UuidToNumKey{UUID: status.UUID}
@@ -399,8 +421,8 @@ func (z *zedrouter) handleNetworkInstanceModify(ctxArg interface{}, key string,
 		z.doUpdateActivatedNetworkInstance(config, status)
 	}
 
-	// Check if some inter-NI conflicts were resolved by this modification.
-	z.checkConflictingNetworkInstances()
+	// Check if some IP conflicts were resolved by this modification.
+	z.checkAllNetworkInstanceIPConflicts()
 	z.log.Functionf("handleNetworkInstanceModify(%s) done", key)
 }
 
@@ -417,8 +439,8 @@ func (z *zedrouter) handleNetworkInstanceDelete(ctxArg interface{}, key string,
 	z.publishNetworkInstanceStatus(status)
 
 	done := z.maybeDelOrInactivateNetworkInstance(status)
-	// Check if some inter-NI conflicts were resolved by this delete.
-	z.checkConflictingNetworkInstances()
+	// Check if some IP conflicts were resolved by this NI deletion.
+	z.checkAllNetworkInstanceIPConflicts()
 	z.log.Functionf("handleNetworkInstanceDelete(%s) done %t", key, done)
 }
 

pkg/pillar/cmd/zedrouter/validation.go

@@ -9,11 +9,11 @@ import (
 	"time"
 
 	"github.com/lf-edge/eve/pkg/pillar/types"
+	"github.com/lf-edge/eve/pkg/pillar/utils/netutils"
 	uuid "github.com/satori/go.uuid"
 )
 
-func (z *zedrouter) doNetworkInstanceSanityCheck(
-	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
+func (z *zedrouter) doNetworkInstanceSanityCheck(config *types.NetworkInstanceConfig) error {
 	z.log.Functionf("Sanity Checking NetworkInstance(%s-%s): type:%d, IpType:%d",
 		config.DisplayName, config.UUID, config.Type, config.IpType)
 
@@ -24,7 +24,7 @@ func (z *zedrouter) doNetworkInstanceSanityCheck(
 	case types.NetworkInstanceTypeSwitch:
 		// Do nothing
 	default:
-		return false, fmt.Errorf("network instance type %d is not supported", config.Type)
+		return fmt.Errorf("network instance type %d is not supported", config.Type)
 	}
 
 	// IpType - Check for valid types
@@ -33,63 +33,32 @@ func (z *zedrouter) doNetworkInstanceSanityCheck(
 		// Do nothing
 	case types.AddressTypeIPV4, types.AddressTypeIPV6,
 		types.AddressTypeCryptoIPV4, types.AddressTypeCryptoIPV6:
-		niConflict, err = z.doNetworkInstanceSubnetSanityCheck(config)
+		err := z.doNetworkInstanceSubnetSanityCheck(config)
 		if err != nil {
-			return niConflict, err
+			return err
 		}
 		err = z.doNetworkInstanceDhcpRangeSanityCheck(config)
 		if err != nil {
-			return false, err
+			return err
 		}
 		err = z.doNetworkInstanceGatewaySanityCheck(config)
 		if err != nil {
-			return false, err
+			return err
 		}
 
 	default:
-		return false, fmt.Errorf("IpType %d not supported", config.IpType)
+		return fmt.Errorf("IpType %d not supported", config.IpType)
 	}
-	return false, nil
+	return nil
 }
 
 func (z *zedrouter) doNetworkInstanceSubnetSanityCheck(
-	config *types.NetworkInstanceConfig) (niConflict bool, err error) {
+	config *types.NetworkInstanceConfig) error {
 	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",
 			config.Key(), config.DisplayName, config.Subnet)
 	}
-
-	items := z.subNetworkInstanceConfig.GetAll()
-	for key2, config2 := range items {
-		niConfig2 := config2.(types.NetworkInstanceConfig)
-		if config.Key() == key2 {
-			continue
-		}
-
-		// We check for overlapping subnets by checking the
-		// SubnetAddr ( first address ) is not contained in the subnet of
-		// any other NI and vice-versa ( Other NI Subnet addrs are not
-		// contained in the current NI subnet)
-
-		// Check if config.Subnet is contained in iterStatusEntry.Subnet
-		if niConfig2.Subnet.Contains(config.Subnet.IP) {
-			return true, fmt.Errorf("subnet(%s, IP:%s) overlaps with another "+
-				"network instance(%s-%s) Subnet(%s)",
-				config.Subnet.String(), config.Subnet.IP.String(),
-				niConfig2.DisplayName, niConfig2.UUID,
-				niConfig2.Subnet.String())
-		}
-
-		// Reverse check: check if iterStatusEntry.Subnet is contained in config.subnet
-		if config.Subnet.Contains(niConfig2.Subnet.IP) {
-			return true, fmt.Errorf("another network instance(%s-%s) Subnet(%s) "+
-				"overlaps with Subnet(%s)",
-				niConfig2.DisplayName, niConfig2.UUID,
-				niConfig2.Subnet.String(),
-				config.Subnet.String())
-		}
-	}
-	return false, nil
+	return nil
 }
 
 func (z *zedrouter) doNetworkInstanceDhcpRangeSanityCheck(
@@ -131,6 +100,33 @@ func (z *zedrouter) doNetworkInstanceGatewaySanityCheck(
 	return nil
 }
 
+func (z *zedrouter) checkNetworkInstanceIPConflicts(
+	config *types.NetworkInstanceConfig) error {
+	// Check for overlapping subnets between NIs.
+	items := z.subNetworkInstanceConfig.GetAll()
+	for key2, config2 := range items {
+		niConfig2 := config2.(types.NetworkInstanceConfig)
+		if config.Key() == key2 {
+			continue
+		}
+		if netutils.OverlappingSubnets(&config.Subnet, &niConfig2.Subnet) {
+			return fmt.Errorf("subnet (%s) overlaps with another "+
+				"network instance (%s-%s) subnet (%s)",
+				config.Subnet.String(), niConfig2.DisplayName, niConfig2.UUID,
+				niConfig2.Subnet.String())
+		}
+	}
+	// Check for overlapping subnets between the NI and device ports.
+	for _, port := range z.deviceNetworkStatus.Ports {
+		if netutils.OverlappingSubnets(&config.Subnet, &port.Subnet) {
+			return fmt.Errorf("subnet (%s) overlaps with device port %s "+
+				"subnet (%s)", config.Subnet.String(), port.Logicallabel,
+				port.Subnet.String())
+		}
+	}
+	return nil
+}
+
 func (z *zedrouter) validateAppNetworkConfig(appNetConfig types.AppNetworkConfig) error {
 	z.log.Functionf("AppNetwork(%s), check for duplicate port map acls",
 		appNetConfig.DisplayName)

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
 	}
 
 	// restore cached patchEnvelopeUsage counters
 	for _, key := range z.peUsagePersist.Objects() {
 		cached, err := z.peUsagePersist.Get(key)
 		if err != nil {
-			log.Fatal(err)
+			return err
 		}
 		buf := bytes.NewBuffer(cached)
 		dec := gob.NewDecoder(buf)
 
 		var peUsage types.PatchEnvelopeUsage
 
 		if err := dec.Decode(&peUsage); err != nil {
-			log.Fatal(err)
+			return err
 		}
 
 		z.patchEnvelopesUsage.Store(key, peUsage)
@@ -963,7 +963,7 @@ func (z *zedrouter) processNIReconcileStatus(recStatus nireconciler.NIReconcileS
 			changed = true
 		}
 	} else {
-		if niStatus.HasError() {
+		if niStatus.HasError() && !niStatus.IPConflict {
 			niStatus.ClearError()
 			changed = true
 		}