Sanitize newprefix (#52)

.gitignore

@@ -1,2 +1,3 @@
 .vscode
+.idea
 coverage.out

go.mod

@@ -7,8 +7,7 @@ require (
 	github.com/jmoiron/sqlx v1.3.1
 	github.com/lib/pq v1.10.0
 	github.com/stretchr/testify v1.7.0
-	github.com/testcontainers/testcontainers-go v0.9.0
-	golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83 // indirect
+	github.com/testcontainers/testcontainers-go v0.10.0
 	golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 // indirect
-	inet.af/netaddr v0.0.0-20210310211742-da5a5c0aa558
+	inet.af/netaddr v0.0.0-20210313195008-843b4240e319
 )

ipam.go

@@ -1,5 +1,7 @@
 package ipam
 
+import "sync"
+
 // Ipamer can be used to do IPAM stuff.
 type Ipamer interface {
 	// NewPrefix create a new Prefix from a string notation.
@@ -31,6 +33,7 @@ type Ipamer interface {
 }
 
 type ipamer struct {
+	mu      sync.Mutex
 	storage Storage
 }
 

memory.go

@@ -50,6 +50,16 @@ func (m *memory) ReadAllPrefixes() ([]Prefix, error) {
 	}
 	return ps, nil
 }
+func (m *memory) ReadAllPrefixCidrs() ([]string, error) {
+	m.lock.RLock()
+	defer m.lock.RUnlock()
+
+	ps := make([]string, 0, len(m.prefixes))
+	for cidr := range m.prefixes {
+		ps = append(ps, cidr)
+	}
+	return ps, nil
+}
 func (m *memory) UpdatePrefix(prefix Prefix) (Prefix, error) {
 	m.lock.Lock()
 	defer m.lock.Unlock()

prefix.go

@@ -140,10 +140,20 @@ type Usage struct {
 }
 
 func (i *ipamer) NewPrefix(cidr string) (*Prefix, error) {
+	i.mu.Lock()
+	defer i.mu.Unlock()
+	existingPrefixes, err := i.storage.ReadAllPrefixCidrs()
+	if err != nil {
+		return nil, err
+	}
 	p, err := i.newPrefix(cidr, "")
 	if err != nil {
 		return nil, err
 	}
+	err = i.PrefixesOverlapping(existingPrefixes, []string{p.Cidr})
+	if err != nil {
+		return nil, err
+	}
 	newPrefix, err := i.storage.CreatePrefix(*p)
 	if err != nil {
 		return nil, err
@@ -280,7 +290,11 @@ func (i *ipamer) releaseChildPrefixInternal(child *Prefix) error {
 }
 
 func (i *ipamer) PrefixFrom(cidr string) *Prefix {
-	prefix, err := i.storage.ReadPrefix(cidr)
+	ipprefix, err := netaddr.ParseIPPrefix(cidr)
+	if err != nil {
+		return nil
+	}
+	prefix, err := i.storage.ReadPrefix(ipprefix.Masked().String())
 	if err != nil {
 		return nil
 	}
@@ -324,7 +338,7 @@ func (i *ipamer) acquireSpecificIPInternal(prefixCidr, specificIP string) (*IP,
 		}
 		_, ok := prefix.ips[specificIPnet.String()]
 		if ok {
-			return nil, fmt.Errorf("%w: given ip:%s is already allocated", ErrAlreadyAllocated, specificIP)
+			return nil, fmt.Errorf("%w: given ip:%s is already allocated", ErrAlreadyAllocated, specificIPnet)
 		}
 	}
 
@@ -397,7 +411,7 @@ func (i *ipamer) PrefixesOverlapping(existingPrefixes []string, newPrefixes []st
 				return fmt.Errorf("parsing prefix %s failed:%w", np, err)
 			}
 			if eip.Overlaps(nip) || nip.Overlaps(eip) {
-				return fmt.Errorf("%s overlaps %s", np, ep)
+				return fmt.Errorf("%s overlaps %s", nip, eip)
 			}
 		}
 	}
@@ -410,8 +424,16 @@ func (i *ipamer) newPrefix(cidr, parentCidr string) (*Prefix, error) {
 	if err != nil {
 		return nil, fmt.Errorf("unable to parse cidr:%s %w", cidr, err)
 	}
+	if parentCidr != "" {
+		ipnetParent, err := netaddr.ParseIPPrefix(parentCidr)
+		if err != nil {
+			return nil, fmt.Errorf("unable to parse parent cidr:%s %w", cidr, err)
+		}
+		parentCidr = ipnetParent.Masked().String()
+	}
+
 	p := &Prefix{
-		Cidr:                   cidr,
+		Cidr:                   ipnet.Masked().String(),
 		ParentCidr:             parentCidr,
 		ips:                    make(map[string]bool),
 		availableChildPrefixes: make(map[string]bool),

prefix_test.go

@@ -43,6 +43,18 @@ func (i *ipamer) getHostAddresses(prefix string) ([]string, error) {
 	}
 }
 
+func TestIPRangeOverlapping(t *testing.T) {
+	i := New()
+
+	cidr := "10.10.10.0/24"
+	_, err := i.NewPrefix(cidr)
+	require.Nil(t, err)
+
+	cidr = "10.10.10.1/24"
+	_, err = i.NewPrefix(cidr)
+	require.NotNil(t, err)
+}
+
 func TestIpamer_AcquireIP(t *testing.T) {
 
 	type fields struct {
@@ -180,13 +192,13 @@ func TestIpamer_ReleaseIPFromPrefixIPv6(t *testing.T) {
 		require.NotNil(t, err)
 		require.True(t, errors.As(err, &NotFoundError{}), "error must be of correct type")
 		require.True(t, errors.Is(err, ErrNotFound), "error must be NotFound")
-		require.Equal(t, "NotFound: unable to release ip:1.2.3.4 because it is not allocated in prefix:2001:0db8:85a3::/120", err.Error())
+		require.Equal(t, "NotFound: unable to release ip:1.2.3.4 because it is not allocated in prefix:2001:db8:85a3::/120", err.Error())
 
 		err = ipam.ReleaseIPFromPrefix(prefix.Cidr, "1001:0db8:85a3::1")
 		require.NotNil(t, err)
 		require.True(t, errors.As(err, &NotFoundError{}), "error must be of correct type")
 		require.True(t, errors.Is(err, ErrNotFound), "error must be NotFound")
-		require.Equal(t, "NotFound: unable to release ip:1001:0db8:85a3::1 because it is not allocated in prefix:2001:0db8:85a3::/120", err.Error())
+		require.Equal(t, "NotFound: unable to release ip:1001:0db8:85a3::1 because it is not allocated in prefix:2001:db8:85a3::/120", err.Error())
 
 		err = ipam.ReleaseIPFromPrefix("1001:0db8:85a3::/120", "1.2.3.4")
 		require.NotNil(t, err)
@@ -273,7 +285,7 @@ func TestIpamer_AcquireSpecificIP(t *testing.T) {
 		ip3, err = ipam.AcquireSpecificIP(prefix.Cidr, "2001:0db8:85a4::1")
 		require.Nil(t, ip3)
 		require.NotNil(t, err)
-		require.Equal(t, "given ip:2001:0db8:85a4::1 is not in 2001:0db8:85a3::/120", err.Error())
+		require.Equal(t, "given ip:2001:0db8:85a4::1 is not in 2001:db8:85a3::/120", err.Error())
 
 		// Cidr is invalid
 		ip5, err = ipam.AcquireSpecificIP("2001:0db8:95a3::/120", "2001:0db8:95a3::invalid")
@@ -631,7 +643,7 @@ func TestIpamer_AcquireChildPrefixIPv6(t *testing.T) {
 		require.NotNil(t, cp)
 		cp, err = ipam.AcquireChildPrefix(prefix.Cidr, 117)
 		require.NotNil(t, err)
-		require.Equal(t, "no prefix found in 2001:0db8:85a3::/116 with length:117", err.Error())
+		require.Equal(t, "no prefix found in 2001:db8:85a3::/116 with length:117", err.Error())
 		require.Nil(t, cp)
 
 		// Prefix has ips
@@ -645,7 +657,7 @@ func TestIpamer_AcquireChildPrefixIPv6(t *testing.T) {
 		require.NotNil(t, ip)
 		cp2, err := ipam.AcquireChildPrefix(p2.Cidr, 121)
 		require.NotNil(t, err)
-		require.Equal(t, "prefix 2001:0db8:95a3::/120 has ips, acquire child prefix not possible", err.Error())
+		require.Equal(t, "prefix 2001:db8:95a3::/120 has ips, acquire child prefix not possible", err.Error())
 		require.Nil(t, cp2)
 
 		// Prefix has Childs, AcquireIP wont work
@@ -660,13 +672,13 @@ func TestIpamer_AcquireChildPrefixIPv6(t *testing.T) {
 		p3 = ipam.PrefixFrom(p3.Cidr)
 		ip, err = ipam.AcquireIP(p3.Cidr)
 		require.NotNil(t, err)
-		require.Equal(t, "prefix 2001:0db8:75a3::/120 has childprefixes, acquire ip not possible", err.Error())
+		require.Equal(t, "prefix 2001:db8:75a3::/120 has childprefixes, acquire ip not possible", err.Error())
 		require.Nil(t, ip)
 
 		// Release Parent Prefix must not work
 		err = ipam.ReleaseChildPrefix(p3)
 		require.NotNil(t, err)
-		require.Equal(t, "prefix 2001:0db8:75a3::/120 is no child prefix", err.Error())
+		require.Equal(t, "prefix 2001:db8:75a3::/120 is no child prefix", err.Error())
 	})
 }
 
@@ -809,7 +821,7 @@ func TestIpamer_PrefixesOverlapping(t *testing.T) {
 			existingPrefixes: []string{"2001:0db8:85a3::/126", "2001:0db8:85a4::/126"},
 			newPrefixes:      []string{"2001:0db8:85a4::/126"},
 			wantErr:          true,
-			errorString:      "2001:0db8:85a4::/126 overlaps 2001:0db8:85a4::/126",
+			errorString:      "2001:db8:85a4::/126 overlaps 2001:db8:85a4::/126",
 		},
 		{
 			name:             "one overlap",
@@ -871,15 +883,27 @@ func TestIpamer_NewPrefix(t *testing.T) {
 	tests := []struct {
 		name        string
 		cidr        string
-		parentCidr  string
+		wantcidr    string
 		wantErr     bool
 		errorString string
 	}{
 		{
-			name:       "valid Prefix",
-			cidr:       "192.168.0.0/24",
-			parentCidr: "",
-			wantErr:    false,
+			name:     "valid Prefix",
+			cidr:     "192.168.0.0/24",
+			wantcidr: "192.168.0.0/24",
+			wantErr:  false,
+		},
+		{
+			name:     "valid Prefix, not in canocical form",
+			cidr:     "192.169.0.1/24",
+			wantcidr: "192.169.0.0/24",
+			wantErr:  false,
+		},
+		{
+			name:     "valid Prefix, not in canocical form",
+			cidr:     "192.167.10.0/16",
+			wantcidr: "192.167.0.0/16",
+			wantErr:  false,
 		},
 		{
 			name:        "invalid Prefix",
@@ -888,9 +912,16 @@ func TestIpamer_NewPrefix(t *testing.T) {
 			errorString: "unable to parse cidr:192.168.0.0/33 netaddr.ParseIPPrefix(\"33\"): prefix length out of range",
 		},
 		{
-			name:    "valid IPv6 Prefix",
-			cidr:    "2001:0db8:85a3::/120",
-			wantErr: false,
+			name:     "valid IPv6 Prefix",
+			cidr:     "2001:0db8:85a3::/120",
+			wantcidr: "2001:db8:85a3::/120",
+			wantErr:  false,
+		},
+		{
+			name:     "valid IPv6 Prefix, not in canocical form",
+			cidr:     "2001:0db8:85a4::2/120",
+			wantcidr: "2001:db8:85a4::/120",
+			wantErr:  false,
 		},
 		{
 			name:        "invalid IPv6 Prefix length",
@@ -922,11 +953,8 @@ func TestIpamer_NewPrefix(t *testing.T) {
 			if err != nil {
 				return
 			}
-			if got.Cidr != test.cidr {
-				t.Errorf("Ipamer.NewPrefix() = %v, want %v", got.Cidr, test.cidr)
-			}
-			if got.ParentCidr != test.parentCidr {
-				t.Errorf("Ipamer.NewPrefix() = %v, want %v", got.ParentCidr, test.parentCidr)
+			if got.Cidr != test.wantcidr {
+				t.Errorf("Ipamer.NewPrefix() cidr = %v, want %v", got.Cidr, test.wantcidr)
 			}
 		})
 	}
@@ -965,7 +993,7 @@ func TestIpamer_DeletePrefix(t *testing.T) {
 
 		_, err = ipam.DeletePrefix(prefix.Cidr)
 		require.NotNil(t, err)
-		require.Equal(t, "prefix 2001:0db8:85a3::/120 has ips, delete prefix not possible", err.Error())
+		require.Equal(t, "prefix 2001:db8:85a3::/120 has ips, delete prefix not possible", err.Error())
 
 		_, err = ipam.ReleaseIP(ip)
 		require.Nil(t, err)
@@ -986,6 +1014,18 @@ func TestIpamer_PrefixFrom(t *testing.T) {
 
 		prefix = ipam.PrefixFrom("192.168.0.0/20")
 		require.NotNil(t, prefix)
+
+		// non canonical form still returns the same prefix
+		prefix2 := ipam.PrefixFrom("10.0.5.0/8")
+		require.Nil(t, prefix2)
+
+		prefix2a, err := ipam.NewPrefix("10.8.0.0/8")
+		require.Nil(t, err)
+		require.NotNil(t, prefix2a)
+
+		prefix2b := ipam.PrefixFrom("10.2.0.0/8")
+		require.NotNil(t, prefix2b)
+		require.Equal(t, prefix2a.Cidr, prefix2b.Cidr)
 	})
 }
 
@@ -1062,7 +1102,7 @@ func TestIpamerAcquireAlreadyAquiredIPv6(t *testing.T) {
 		require.Equal(t, ip.IP.String(), "2001:db8:85a3::1")
 		_, err = ipam.AcquireSpecificIP(p.Cidr, "2001:0db8:85a3::1")
 		require.ErrorIs(t, err, ErrAlreadyAllocated)
-		require.EqualError(t, err, "AlreadyAllocatedError: given ip:2001:0db8:85a3::1 is already allocated")
+		require.EqualError(t, err, "AlreadyAllocatedError: given ip:2001:db8:85a3::1 is already allocated")
 
 		_, err = ipam.ReleaseIP(ip)
 		require.NoError(t, err)
@@ -1112,7 +1152,7 @@ func TestGetHostAddressesIPv6(t *testing.T) {
 		require.Error(t, err)
 		require.True(t, errors.As(err, &NoIPAvailableError{}), "error must be of correct type")
 		require.True(t, errors.Is(err, ErrNoIPAvailable), "error must be NoIPAvailable")
-		require.Equal(t, "NoIPAvailableError: no more ips in prefix: 2001:0db8:85a3::/120 left, length of prefix.ips: 256", err.Error())
+		require.Equal(t, "NoIPAvailableError: no more ips in prefix: 2001:db8:85a3::/120 left, length of prefix.ips: 256", err.Error())
 		require.Nil(t, ip)
 
 		cidr = "2001:0db8:95a3::/122"
@@ -1125,7 +1165,7 @@ func TestGetHostAddressesIPv6(t *testing.T) {
 		require.Error(t, err)
 		require.True(t, errors.As(err, &NoIPAvailableError{}), "error must be of correct type")
 		require.True(t, errors.Is(err, ErrNoIPAvailable), "error must be NoIPAvailable")
-		require.Equal(t, "NoIPAvailableError: no more ips in prefix: 2001:0db8:95a3::/122 left, length of prefix.ips: 64", err.Error())
+		require.Equal(t, "NoIPAvailableError: no more ips in prefix: 2001:db8:95a3::/122 left, length of prefix.ips: 64", err.Error())
 		require.Nil(t, ip)
 	})
 }

sql.go

@@ -96,6 +96,7 @@ func (s *sql) ReadPrefix(prefix string) (Prefix, error) {
 	return pre.toPrefix(), nil
 }
 
+// ReadAllPrefixes returns all known prefixes.
 func (s *sql) ReadAllPrefixes() ([]Prefix, error) {
 	var prefixes [][]byte
 	err := s.db.Select(&prefixes, "SELECT prefix FROM prefixes")
@@ -115,6 +116,16 @@ func (s *sql) ReadAllPrefixes() ([]Prefix, error) {
 	return result, nil
 }
 
+// ReadAllPrefixCidrs is cheaper that ReadAllPrefixes because it only returns the Cidrs.
+func (s *sql) ReadAllPrefixCidrs() ([]string, error) {
+	cidrs := []string{}
+	err := s.db.Select(&cidrs, "SELECT cidr FROM prefixes")
+	if err != nil {
+		return nil, fmt.Errorf("unable to read prefixes:%w", err)
+	}
+	return cidrs, nil
+}
+
 // UpdatePrefix tries to update the prefix.
 // Returns OptimisticLockError if it does not succeed due to a concurrent update.
 func (s *sql) UpdatePrefix(prefix Prefix) (Prefix, error) {

sql_test.go

@@ -60,7 +60,7 @@ func Test_sql_CreatePrefix(t *testing.T) {
 		require.NotNil(t, p)
 		require.Equal(t, prefix.Cidr, p.Cidr)
 
-		ps, err := db.ReadAllPrefixes()
+		ps, err := db.ReadAllPrefixCidrs()
 		require.Nil(t, err)
 		require.NotNil(t, ps)
 		require.Equal(t, 1, len(ps))
@@ -94,7 +94,7 @@ func Test_sql_ReadAllPrefix(t *testing.T) {
 		require.NotNil(t, db)
 
 		// no Prefixes
-		ps, err := db.ReadAllPrefixes()
+		ps, err := db.ReadAllPrefixCidrs()
 		require.Nil(t, err)
 		require.NotNil(t, ps)
 		require.Equal(t, 0, len(ps))
@@ -104,15 +104,15 @@ func Test_sql_ReadAllPrefix(t *testing.T) {
 		p, err := db.CreatePrefix(prefix)
 		require.Nil(t, err)
 		require.NotNil(t, p)
-		ps, err = db.ReadAllPrefixes()
+		ps, err = db.ReadAllPrefixCidrs()
 		require.Nil(t, err)
 		require.NotNil(t, ps)
 		require.Equal(t, 1, len(ps))
 
 		// no Prefixes again
 		_, err = db.DeletePrefix(prefix)
 		require.Nil(t, err)
-		ps, err = db.ReadAllPrefixes()
+		ps, err = db.ReadAllPrefixCidrs()
 		require.Nil(t, err)
 		require.NotNil(t, ps)
 		require.Equal(t, 0, len(ps))

storage.go

@@ -5,6 +5,7 @@ type Storage interface {
 	CreatePrefix(prefix Prefix) (Prefix, error)
 	ReadPrefix(prefix string) (Prefix, error)
 	ReadAllPrefixes() ([]Prefix, error)
+	ReadAllPrefixCidrs() ([]string, error)
 	UpdatePrefix(prefix Prefix) (Prefix, error)
 	DeletePrefix(prefix Prefix) (Prefix, error)
 }