fix (network-manager): correct a bug (ref #912) which prevented the n…

…etwork manager to restart

Subnets in CIDR notation can be passed to the network manager as parameters
in order to prevent them to be used for remote clusters. A bug prevented
the network manager to restart when networks to be reserved are passed as
arguments.  The bug introduced in PR #868 has been fixed. If a subnet has to be
removed from the reserved networks we just remove the parameter from the network
manager and restart the operator.

apis/net/v1alpha1/ipamstorage_types.go

@@ -55,6 +55,8 @@ type IpamSpec struct {
 	Prefixes map[string][]byte `json:"prefixes"`
 	// Network pools.
 	Pools []string `json:"pools"`
+	// Reserved Networks. Subnets listed in this field are excluded from the list of possible subnets used for natting POD CIDR.
+	ReservedSubnets []string `json:"reservedSubnets"`
 	// Map used to keep track of networks assigned to clusters. Key is the remote cluster ID, value is a the set of
 	// networks used by the remote cluster.
 	ClusterSubnets map[string]Subnets `json:"clusterSubnets"`

cmd/liqonet/network-manager.go

@@ -55,7 +55,8 @@ func addNetworkManagerFlags(managerFlags *networkManagerFlags) {
 }
 
 func validateNetworkManagerFlags(managerFlags *networkManagerFlags) error {
-	cidrRegex := regexp.MustCompile(`^(\d{1,3}.){3}\d{1,3}(/(\d|[12]\d|3[012]))$`)
+	cidrRegex := regexp.MustCompile(`^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]` +
+		`|1[0-9]{2}|2[0-4][0-9]|25[0-5])(\/(3[0-2]|[1-2][0-9]|[0-9]))$`)
 
 	if !cidrRegex.MatchString(managerFlags.podCIDR) {
 		return fmt.Errorf("pod CIDR is empty or invalid (%q)", managerFlags.podCIDR)
@@ -172,10 +173,8 @@ func initializeIPAM(client dynamic.Interface, managerFlags *networkManagerFlags)
 		}
 	}
 
-	for _, pool := range managerFlags.reservedPools.StringList {
-		if err := ipam.AcquireReservedSubnet(pool); err != nil {
-			return nil, err
-		}
+	if err := ipam.SetReservedSubnets(managerFlags.reservedPools.StringList); err != nil {
+		return nil, err
 	}
 
 	return ipam, nil

deployments/liqo/crds/net.liqo.io_ipamstorages.yaml

@@ -121,6 +121,12 @@ spec:
                   Important: Run "make" to regenerate code after modifying this file
                   Map consumed by go-ipam module. Key is prefic cidr, value is a Prefix.'
                 type: object
+              reservedSubnets:
+                description: Reserved Networks. Subnets listed in this field are excluded
+                  from the list of possible subnets used for natting POD CIDR.
+                items:
+                  type: string
+                type: array
               serviceCIDR:
                 description: ServiceCIDR
                 type: string
@@ -132,6 +138,7 @@ spec:
             - podCIDR
             - pools
             - prefixes
+            - reservedSubnets
             - serviceCIDR
             type: object
         type: object

pkg/liqonet/ipam/ipam.go

@@ -457,22 +457,24 @@ func (liqoIPAM *IPAM) freePoolInHalves(pool string) error {
 		return err
 	}
 
-	klog.Infof("Network %s is equal to a network pool, freeing first half..", pool)
-	err = liqoIPAM.ipam.ReleaseChildPrefix(liqoIPAM.ipam.PrefixFrom(halfCidr))
-	if err != nil {
-		return fmt.Errorf("cannot free first half of pool %s: %w", pool, err)
+	klog.Infof("Network %s is equal to a network pool, freeing it in two steps...", pool)
+	for i := 1; i <= 2; i++ {
+		if i == 2 {
+			// Get second half CIDR
+			halfCidr, err = utils.Next(halfCidr)
+			if err != nil {
+				return err
+			}
+		}
+		klog.Infof("freeing half %d...", i)
+		if prefix := liqoIPAM.ipam.PrefixFrom(halfCidr); prefix != nil {
+			err = liqoIPAM.ipam.ReleaseChildPrefix(prefix)
+			if err != nil {
+				return fmt.Errorf("cannot free first half of pool %s: %w", pool, err)
+			}
+		}
 	}
 
-	// Get second half CIDR
-	halfCidr, err = utils.Next(halfCidr)
-	if err != nil {
-		return err
-	}
-	klog.Infof("Freeing second half..")
-	err = liqoIPAM.ipam.ReleaseChildPrefix(liqoIPAM.ipam.PrefixFrom(halfCidr))
-	if err != nil {
-		return fmt.Errorf("cannot free second half of pool %s: %w", pool, err)
-	}
 	klog.Infof("Network %s has successfully been freed", pool)
 	return nil
 }
@@ -1188,3 +1190,35 @@ func (liqoIPAM *IPAM) SetServiceCIDR(serviceCIDR string) error {
 	}
 	return nil
 }
+
+// SetReservedSubnets acquires and/or frees the reserved networks.
+func (liqoIPAM *IPAM) SetReservedSubnets(subnets []string) error {
+	reserved := liqoIPAM.ipamStorage.getReservedSubnets()
+
+	// Free all the reserved networks not needed anymore.
+	for _, r := range reserved {
+		if !slice.ContainsString(subnets, r) {
+			klog.Infof("freeing old reserved subnet %s", r)
+			if err := liqoIPAM.FreeReservedSubnet(r); err != nil {
+				return fmt.Errorf("una error occurred while freeing reserved subnet {%s}: %w", r, err)
+			}
+			if err := liqoIPAM.ipamStorage.updateReservedSubnets(r, updateOpRemove); err != nil {
+				return err
+			}
+		}
+	}
+
+	// Reserve the newly added subnets.
+	for _, s := range subnets {
+		if !slice.ContainsString(reserved, s) {
+			klog.Infof("acquiring reserved subnet %s", s)
+			if err := liqoIPAM.AcquireReservedSubnet(s); err != nil {
+				return fmt.Errorf("an error occurred while reserving subnet {%s}: %w", s, err)
+			}
+			if err := liqoIPAM.ipamStorage.updateReservedSubnets(s, updateOpAdd); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}

pkg/liqonet/ipam/ipamStorage.go

@@ -33,18 +33,22 @@ import (
 
 	netv1alpha1 "github.com/liqotech/liqo/apis/net/v1alpha1"
 	"github.com/liqotech/liqo/pkg/consts"
+	"github.com/liqotech/liqo/pkg/utils/slice"
 )
 
 const (
 	ipamNamePrefix              = "ipamstorage-"
 	clusterSubnetUpdate         = "clusterSubnets"
 	poolsUpdate                 = "pools"
+	reservedSubnetsUpdate       = "reservedSubnets"
 	prefixesUpdate              = "prefixes"
 	externalCIDRUpdate          = "externalCIDR"
 	endpointMappingsUpdate      = "endpointMappings"
 	podCIDRUpdate               = "podCIDR"
 	serviceCIDRUpdate           = "serviceCIDR"
 	natMappingsConfiguredUpdate = "natMappingsConfigured"
+	updateOpAdd                 = "add"
+	updateOpRemove              = "remove"
 )
 
 // IpamStorage is the interface to be implemented to enforce persistency in IPAM.
@@ -55,13 +59,15 @@ type IpamStorage interface {
 	updateEndpointMappings(endpoints map[string]netv1alpha1.EndpointMapping) error
 	updatePodCIDR(podCIDR string) error
 	updateServiceCIDR(serviceCIDR string) error
+	updateReservedSubnets(subnet, operation string) error
 	updateNatMappingsConfigured(natMappingsConfigured map[string]netv1alpha1.ConfiguredCluster) error
 	getClusterSubnets() map[string]netv1alpha1.Subnets
 	getPools() []string
 	getExternalCIDR() string
 	getEndpointMappings() map[string]netv1alpha1.EndpointMapping
 	getPodCIDR() string
 	getServiceCIDR() string
+	getReservedSubnets() []string
 	getNatMappingsConfigured() map[string]netv1alpha1.ConfiguredCluster
 	goipam.Storage
 }
@@ -206,21 +212,38 @@ func (ipamStorage *IPAMStorage) DeletePrefix(prefix goipam.Prefix) (goipam.Prefi
 func (ipamStorage *IPAMStorage) updateClusterSubnets(clusterSubnets map[string]netv1alpha1.Subnets) error {
 	return ipamStorage.updateConfig(clusterSubnetUpdate, clusterSubnets)
 }
+
 func (ipamStorage *IPAMStorage) updatePools(pools []string) error {
 	return ipamStorage.updateConfig(poolsUpdate, pools)
 }
+
+func (ipamStorage *IPAMStorage) updateReservedSubnets(subnet, operation string) error {
+	subnets := ipamStorage.getReservedSubnets()
+	switch operation {
+	case updateOpAdd:
+		subnets = append(subnets, subnet)
+	case updateOpRemove:
+		subnets = slice.RemoveString(subnets, subnet)
+	}
+	return ipamStorage.updateConfig(reservedSubnetsUpdate, subnets)
+}
+
 func (ipamStorage *IPAMStorage) updatePrefixes(prefixes map[string][]byte) error {
 	return ipamStorage.updateConfig(prefixesUpdate, prefixes)
 }
+
 func (ipamStorage *IPAMStorage) updateExternalCIDR(externalCIDR string) error {
 	return ipamStorage.updateConfig(externalCIDRUpdate, externalCIDR)
 }
+
 func (ipamStorage *IPAMStorage) updateEndpointMappings(endpoints map[string]netv1alpha1.EndpointMapping) error {
 	return ipamStorage.updateConfig(endpointMappingsUpdate, endpoints)
 }
+
 func (ipamStorage *IPAMStorage) updatePodCIDR(podCIDR string) error {
 	return ipamStorage.updateConfig(podCIDRUpdate, podCIDR)
 }
+
 func (ipamStorage *IPAMStorage) updateServiceCIDR(serviceCIDR string) error {
 	return ipamStorage.updateConfig(serviceCIDRUpdate, serviceCIDR)
 }
@@ -273,16 +296,23 @@ func (ipamStorage *IPAMStorage) getClusterSubnets() map[string]netv1alpha1.Subne
 func (ipamStorage *IPAMStorage) getExternalCIDR() string {
 	return ipamStorage.getConfig().Spec.ExternalCIDR
 }
+
 func (ipamStorage *IPAMStorage) getEndpointMappings() map[string]netv1alpha1.EndpointMapping {
 	return ipamStorage.getConfig().Spec.EndpointMappings
 }
+
 func (ipamStorage *IPAMStorage) getPodCIDR() string {
 	return ipamStorage.getConfig().Spec.PodCIDR
 }
+
 func (ipamStorage *IPAMStorage) getServiceCIDR() string {
 	return ipamStorage.getConfig().Spec.ServiceCIDR
 }
 
+func (ipamStorage *IPAMStorage) getReservedSubnets() []string {
+	return ipamStorage.getConfig().Spec.ReservedSubnets
+}
+
 func (ipamStorage *IPAMStorage) getNatMappingsConfigured() map[string]netv1alpha1.ConfiguredCluster {
 	return ipamStorage.getConfig().Spec.NatMappingsConfigured
 }
@@ -342,6 +372,7 @@ func (ipamStorage *IPAMStorage) createConfig() (*netv1alpha1.IpamStorage, error)
 			ClusterSubnets:        make(map[string]netv1alpha1.Subnets),
 			EndpointMappings:      make(map[string]netv1alpha1.EndpointMapping),
 			NatMappingsConfigured: make(map[string]netv1alpha1.ConfiguredCluster),
+			ReservedSubnets:       []string{},
 		},
 	}
 

pkg/liqonet/ipam/ipam_test.go

@@ -1559,8 +1559,82 @@ var _ = Describe("Ipam", func() {
 			})
 		})
 	})
+
+	Describe("SetReservedSubnets", func() {
+		var (
+			toBeReservedSubnets1          []string
+			toBeReservedSubnets2          []string
+			toBeReservedSubnetsIncorrect1 []string
+		)
+
+		BeforeEach(func() {
+			toBeReservedSubnets1 = []string{"192.168.0.0/16", "100.200.0.0/16", "10.200.250.0/24"}
+			toBeReservedSubnets2 = []string{"192.168.1.0/24", "100.200.0.0/16", "172.16.34.0/24"}
+			toBeReservedSubnetsIncorrect1 = []string{"192.168.0.0/16", "100.200.0/16", "10.200.250.0/24"}
+		})
+		Context("Reserving subnets", func() {
+			When("Reserving subnets for the first time", func() {
+				It("should reserve all the the subnets and return nil", func() {
+					Expect(ipam.SetReservedSubnets(toBeReservedSubnets1)).To(BeNil())
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElements(toBeReservedSubnets1))
+					checkForPrefixes(toBeReservedSubnets1)
+				})
+			})
+
+			When("Reserving subnets multiple times", func() {
+				It("should return nil", func() {
+					// Reserving the first time.
+					Expect(ipam.SetReservedSubnets(toBeReservedSubnets1)).To(BeNil())
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElements(toBeReservedSubnets1))
+					// Reserving the second time.
+					Expect(ipam.SetReservedSubnets(toBeReservedSubnets1)).To(BeNil())
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElements(toBeReservedSubnets1))
+					checkForPrefixes(toBeReservedSubnets1)
+				})
+			})
+
+			When("Reserving a list of subnets with incorrect ones", func() {
+				It("should reserve all the correct ones that comes before the incorrect one", func() {
+					Expect(ipam.SetReservedSubnets(toBeReservedSubnetsIncorrect1)).To(HaveOccurred())
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(HaveLen(1))
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElement(toBeReservedSubnetsIncorrect1[0]))
+				})
+			})
+		})
+
+		Context("Making available subnets previously reserved", func() {
+			JustBeforeEach(func() {
+				// Reserve the subnets.
+				Expect(ipam.SetReservedSubnets(toBeReservedSubnets1)).To(BeNil())
+				Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElements(toBeReservedSubnets1))
+				checkForPrefixes(toBeReservedSubnets1)
+			})
+			When("reserved subnets are no more needed", func() {
+				It("should remove all the previously reserved networks", func() {
+					Expect(ipam.SetReservedSubnets(nil)).To(BeNil())
+					Expect(ipam.ipamStorage.getReservedSubnets()).Should(HaveLen(0))
+				})
+			})
+
+			When("new subnets are reserved and existing ones are freed", func() {
+				It("should return nil and update the reserved subnets", func() {
+					Expect(ipam.SetReservedSubnets(toBeReservedSubnets2)).To(BeNil())
+					Expect(ipam.ipamStorage.getReservedSubnets()).To(ContainElements(toBeReservedSubnets2))
+					checkForPrefixes(toBeReservedSubnets2)
+				})
+			})
+		})
+	})
 })
 
+func checkForPrefixes(subnets []string) {
+	for _, s := range subnets {
+		prefix, err := ipam.ipamStorage.ReadPrefix(s)
+		Expect(err).To(BeNil())
+		Expect(prefix.Cidr).To(Equal(s))
+	}
+}
+
 func getNatMappingResourcePerCluster(clusterID string) (*liqonetapi.NatMapping, error) {
 	nm := &liqonetapi.NatMapping{}
 	list, err := dynClient.Resource(liqonetapi.NatMappingGroupResource).List(