From fa0b0042b2d7b77a2123ab5115de83430d4c5aca Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 8 Jun 2015 18:12:47 +0100 Subject: [PATCH] Eradicate the last vestiges of CIDR in Allocator --- ipam/address/address.go | 12 ++++-------- ipam/allocator.go | 12 +++++------- ipam/allocator_test.go | 26 +++++++++++++------------- ipam/http.go | 16 ++++++++-------- ipam/http_test.go | 15 +++++++++------ ipam/testutils_test.go | 14 +++++++------- prog/weaver/main.go | 2 +- 7 files changed, 47 insertions(+), 50 deletions(-) diff --git a/ipam/address/address.go b/ipam/address/address.go index 377e283f..194a09db 100644 --- a/ipam/address/address.go +++ b/ipam/address/address.go @@ -49,19 +49,15 @@ func (cidr CIDR) Size() Offset { return 1 << uint(32-cidr.PrefixLen) } // End is exclusive, same as in Range func (cidr CIDR) End() Address { return Add(cidr.Start, cidr.Size()) } func (cidr CIDR) Range() Range { return Range{Start: cidr.Start, End: Add(cidr.Start, cidr.Size())} } +func (cidr CIDR) HostRange() Range { + // Respect RFC1122 exclusions of first and last addresses + return Range{Start: cidr.Start + 1, End: cidr.End() - 1} +} func (cidr CIDR) String() string { return fmt.Sprintf("%s/%d", cidr.Start.String(), cidr.PrefixLen) } -func (cidr1 CIDR) Overlaps(cidr2 CIDR) bool { - return !(cidr1.Start >= cidr2.End() || cidr1.End() <= cidr2.Start) -} - -func (cidr CIDR) Contains(addr Address) bool { - return addr >= cidr.Start && addr < cidr.End() -} - // FromIP4 converts an ipv4 address to our integer address type func FromIP4(ip4 net.IP) (r Address) { for _, b := range ip4.To4() { diff --git a/ipam/allocator.go b/ipam/allocator.go index 398b05bc..036b8e31 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -178,22 +178,20 @@ func hasBeenCancelled(cancelChan <-chan bool) func() bool { // Allocate (Sync) - get IP address for container with given name // if there isn't any space we block indefinitely -func (alloc *Allocator) Allocate(ident string, subnet address.CIDR, cancelChan <-chan bool) (address.CIDR, error) { +func (alloc *Allocator) Allocate(ident string, r address.Range, cancelChan <-chan bool) (address.Address, error) { resultChan := make(chan allocateResult) - // Respect RFC1122 exclusions of first and last addresses - r := address.Range{Start: subnet.Start + 1, End: subnet.End() - 1} op := &allocate{r: r, resultChan: resultChan, ident: ident, hasBeenCancelled: hasBeenCancelled(cancelChan)} alloc.doOperation(op, &alloc.pendingAllocates) result := <-resultChan - return address.CIDR{Start: result.addr, PrefixLen: subnet.PrefixLen}, result.err + return result.addr, result.err } -func (alloc *Allocator) Lookup(ident string, subnet address.CIDR) (address.CIDR, error) { +func (alloc *Allocator) Lookup(ident string, r address.Range) (address.Address, error) { resultChan := make(chan allocateResult) alloc.actionChan <- func() { for _, addr := range alloc.owned[ident] { - if subnet.Contains(addr) { + if r.Contains(addr) { resultChan <- allocateResult{addr: addr} return } @@ -201,7 +199,7 @@ func (alloc *Allocator) Lookup(ident string, subnet address.CIDR) (address.CIDR, resultChan <- allocateResult{err: fmt.Errorf("lookup: no address found")} } result := <-resultChan - return address.CIDR{Start: result.addr, PrefixLen: subnet.PrefixLen}, result.err + return result.addr, result.err } // Claim an address that we think we should own (Sync) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 03b30f83..57a66b2f 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -27,8 +27,8 @@ func TestAllocFree(t *testing.T) { universe = "10.0.3.0/26" subnet1 = "10.0.3.0/28" subnet2 = "10.0.3.32/28" - testAddr1 = "10.0.3.1/28" - testAddr2 = "10.0.3.33/28" + testAddr1 = "10.0.3.1" + testAddr2 = "10.0.3.33" spaceSize = 62 // 64 IP addresses in /26, minus .0 and .63 ) @@ -38,31 +38,31 @@ func TestAllocFree(t *testing.T) { _, cidr2, _ := address.ParseCIDR(subnet2) alloc.claimRingForTesting() - addr1, err := alloc.Allocate(container1, cidr1, nil) + addr1, err := alloc.Allocate(container1, cidr1.HostRange(), nil) wt.AssertNoErr(t, err) wt.AssertEqualString(t, addr1.String(), testAddr1, "address") - addr2, err := alloc.Allocate(container1, cidr2, nil) + addr2, err := alloc.Allocate(container1, cidr2.HostRange(), nil) wt.AssertNoErr(t, err) wt.AssertEqualString(t, addr2.String(), testAddr2, "address") // Ask for another address for a different container and check it's different - addr1b, _ := alloc.Allocate(container2, cidr1, nil) + addr1b, _ := alloc.Allocate(container2, cidr1.HostRange(), nil) if addr1b.String() == testAddr1 { t.Fatalf("Expected different address but got %s", addr1b.String()) } // Ask for the first container again and we should get the same addresses again - addr1a, _ := alloc.Allocate(container1, cidr1, nil) + addr1a, _ := alloc.Allocate(container1, cidr1.HostRange(), nil) wt.AssertEqualString(t, addr1a.String(), testAddr1, "address") - addr2a, _ := alloc.Allocate(container1, cidr2, nil) + addr2a, _ := alloc.Allocate(container1, cidr2.HostRange(), nil) wt.AssertEqualString(t, addr2a.String(), testAddr2, "address") // Now delete the first container, and we should get its addresses back wt.AssertSuccess(t, alloc.Delete(container1)) - addr3, _ := alloc.Allocate(container3, cidr1, nil) + addr3, _ := alloc.Allocate(container3, cidr1.HostRange(), nil) wt.AssertEqualString(t, addr3.String(), testAddr1, "address") - addr4, _ := alloc.Allocate(container3, cidr2, nil) + addr4, _ := alloc.Allocate(container3, cidr2.HostRange(), nil) wt.AssertEqualString(t, addr4.String(), testAddr2, "address") alloc.ContainerDied(container2) @@ -134,7 +134,7 @@ func TestAllocatorClaim(t *testing.T) { container2 = "baddf00d" container3 = "b01df00d" universe = "10.0.3.0/30" - testAddr1 = "10.0.3.1/30" // first address allocated should be .1 because .0 is network addr + testAddr1 = "10.0.3.1" // first address allocated should be .1 because .0 is network addr ) alloc, subnet := makeAllocatorWithMockGossip(t, "01:00:00:01:00:00", universe, 1) @@ -384,7 +384,7 @@ func TestAllocatorFuzz(t *testing.T) { alloc := allocs[res.alloc] //common.Info.Printf("Freeing %s (%s) on allocator %d", res.name, addr, res.alloc) - oldAddr, _, err := address.ParseCIDR(addr) + oldAddr, err := address.ParseIP(addr) if err != nil { panic(err) } @@ -406,8 +406,8 @@ func TestAllocatorFuzz(t *testing.T) { //common.Info.Printf("Asking for %s on allocator %d again", addr, res.alloc) newAddr, _ := alloc.Allocate(res.name, subnet, nil) - oldAddr, _, _ := address.ParseCIDR(addr) - if newAddr.Start != oldAddr { + oldAddr, _ := address.ParseIP(addr) + if newAddr != oldAddr { panic(fmt.Sprintf("Got different address for repeat request for %s: %s != %s", res.name, newAddr, oldAddr)) } diff --git a/ipam/http.go b/ipam/http.go index cdb858a0..a9de0988 100644 --- a/ipam/http.go +++ b/ipam/http.go @@ -41,21 +41,21 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID badRequest(w, err) return } - addr, err := alloc.Lookup(vars["id"], subnet) + addr, err := alloc.Lookup(vars["id"], subnet.HostRange()) if err != nil { http.NotFound(w, r) return } - fmt.Fprint(w, addr) + fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen) }) router.Methods("GET").Path("/ip/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - addr, err := alloc.Lookup(mux.Vars(r)["id"], defaultSubnet) + addr, err := alloc.Lookup(mux.Vars(r)["id"], defaultSubnet.HostRange()) if err != nil { http.NotFound(w, r) return } - fmt.Fprint(w, addr) + fmt.Fprintf(w, "%s/%d", addr, defaultSubnet.PrefixLen) }) router.Methods("POST").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -72,25 +72,25 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID badRequest(w, fmt.Errorf("Invalid subnet %s - bits after network prefix are not all zero", cidrStr)) return } - addr, err := alloc.Allocate(ident, cidr, closedChan) + addr, err := alloc.Allocate(ident, cidr.HostRange(), closedChan) if err != nil { badRequest(w, fmt.Errorf("Unable to allocate: %s", err)) return } - fmt.Fprint(w, addr) + fmt.Fprintf(w, "%s/%d", addr, cidr.PrefixLen) }) router.Methods("POST").Path("/ip/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { closedChan := w.(http.CloseNotifier).CloseNotify() ident := mux.Vars(r)["id"] - newAddr, err := alloc.Allocate(ident, defaultSubnet, closedChan) + newAddr, err := alloc.Allocate(ident, defaultSubnet.HostRange(), closedChan) if err != nil { badRequest(w, err) return } - fmt.Fprint(w, newAddr) + fmt.Fprintf(w, "%s/%d", newAddr, defaultSubnet.PrefixLen) }) router.Methods("DELETE").Path("/ip/{id}/{ip}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/ipam/http_test.go b/ipam/http_test.go index bdf48277..b93360c6 100644 --- a/ipam/http_test.go +++ b/ipam/http_test.go @@ -80,8 +80,9 @@ func TestHttp(t *testing.T) { testAddr2 = "10.2.0.1/16" ) - alloc, subnet := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", universe, 1) - port := listenHTTP(alloc, subnet) + alloc, _ := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", universe, 1) + _, cidr, _ := address.ParseCIDR(universe) + port := listenHTTP(alloc, cidr) alloc.claimRingForTesting() // Allocate an address in each subnet, and check we got what we expected @@ -120,9 +121,10 @@ func TestBadHttp(t *testing.T) { testCIDR1 = "10.0.0.0/8" ) - alloc, subnet := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", testCIDR1, 1) + alloc, _ := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", testCIDR1, 1) defer alloc.Stop() - port := listenHTTP(alloc, subnet) + _, cidr, _ := address.ParseCIDR(testCIDR1) + port := listenHTTP(alloc, cidr) alloc.claimRingForTesting() cidr1 := HTTPPost(t, allocURL(port, testCIDR1, containerID)) @@ -155,10 +157,11 @@ func impTestHTTPCancel(t *testing.T) { testCIDR1 = "10.0.3.0/29" ) - alloc, subnet := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", testCIDR1, 1) + alloc, _ := makeAllocatorWithMockGossip(t, "08:00:27:01:c3:9a", testCIDR1, 1) defer alloc.Stop() alloc.claimRingForTesting() - port := listenHTTP(alloc, subnet) + _, cidr, _ := address.ParseCIDR(testCIDR1) + port := listenHTTP(alloc, cidr) // Stop the alloc so nothing actually works unpause := alloc.pause() diff --git a/ipam/testutils_test.go b/ipam/testutils_test.go index 8d7da37a..6fd369c7 100644 --- a/ipam/testutils_test.go +++ b/ipam/testutils_test.go @@ -128,7 +128,7 @@ func CheckAllExpectedMessagesSent(allocs ...*Allocator) { } } -func makeAllocator(name string, cidrStr string, quorum uint) (*Allocator, address.CIDR) { +func makeAllocator(name string, cidrStr string, quorum uint) (*Allocator, address.Range) { peername, err := router.PeerNameFromString(name) if err != nil { panic(err) @@ -142,10 +142,10 @@ func makeAllocator(name string, cidrStr string, quorum uint) (*Allocator, addres alloc := NewAllocator(peername, router.PeerUID(rand.Int63()), "nick-"+name, cidr.Range(), quorum) - return alloc, cidr + return alloc, cidr.HostRange() } -func makeAllocatorWithMockGossip(t *testing.T, name string, universeCIDR string, quorum uint) (*Allocator, address.CIDR) { +func makeAllocatorWithMockGossip(t *testing.T, name string, universeCIDR string, quorum uint) (*Allocator, address.Range) { alloc, subnet := makeAllocator(name, universeCIDR, quorum) gossip := &mockGossipComms{t: t, name: name} alloc.SetInterfaces(gossip) @@ -162,10 +162,10 @@ func (alloc *Allocator) claimRingForTesting(allocs ...*Allocator) { alloc.space.AddRanges(alloc.ring.OwnedRanges()) } -func (alloc *Allocator) NumFreeAddresses(subnet address.CIDR) address.Offset { +func (alloc *Allocator) NumFreeAddresses(r address.Range) address.Offset { resultChan := make(chan address.Offset) alloc.actionChan <- func() { - resultChan <- alloc.space.NumFreeAddressesInRange(address.MakeRange(subnet.Start+1, subnet.Size()-2)) + resultChan <- alloc.space.NumFreeAddressesInRange(r) } return <-resultChan } @@ -287,11 +287,11 @@ func (client TestGossipRouterClient) GossipBroadcast(update router.GossipData) e return client.router.GossipBroadcast(update) } -func makeNetworkOfAllocators(size int, cidr string) ([]*Allocator, TestGossipRouter, address.CIDR) { +func makeNetworkOfAllocators(size int, cidr string) ([]*Allocator, TestGossipRouter, address.Range) { gossipRouter := TestGossipRouter{make(map[router.PeerName]chan gossipMessage), 0.0} allocs := make([]*Allocator, size) - var subnet address.CIDR + var subnet address.Range for i := 0; i < size; i++ { var alloc *Allocator diff --git a/prog/weaver/main.go b/prog/weaver/main.go index 0ab87b66..0ce26cd3 100644 --- a/prog/weaver/main.go +++ b/prog/weaver/main.go @@ -214,7 +214,7 @@ func createAllocator(router *weave.Router, apiPath string, ipRangeStr string, de defaultSubnet := ipRange if defaultSubnetStr != "" { defaultSubnet = parseAndCheckCIDR(defaultSubnetStr) - if !ipRange.Overlaps(defaultSubnet) { + if !ipRange.Range().Overlaps(defaultSubnet.Range()) { log.Fatalf("Default subnet %s out of bounds: %s", defaultSubnet, ipRange) } }