From 1beaf79e474d07a77c2b112080dc2ebc4bff1cf8 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Sep 2015 12:14:51 +0100 Subject: [PATCH 1/3] Refactor: extract duplicated code into helper functions --- ipam/http.go | 88 ++++++++++++++++++++++++---------------------------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/ipam/http.go b/ipam/http.go index 669bc41d82..a8e6288060 100644 --- a/ipam/http.go +++ b/ipam/http.go @@ -16,6 +16,35 @@ func badRequest(w http.ResponseWriter, err error) { common.Log.Warningln("[allocator]:", err.Error()) } +func parseCIDR(w http.ResponseWriter, cidrStr string) (address.CIDR, bool) { + subnetAddr, cidr, err := address.ParseCIDR(cidrStr) + if err != nil { + badRequest(w, err) + return address.CIDR{}, false + } + if cidr.Start != subnetAddr { + badRequest(w, fmt.Errorf("Invalid subnet %s - bits after network prefix are not all zero", cidrStr)) + return address.CIDR{}, false + } + return cidr, true +} + +func (alloc *Allocator) handleHTTPAllocate(dockerCli *docker.Client, w http.ResponseWriter, ident string, checkAlive bool, subnet address.CIDR) { + closedChan := w.(http.CloseNotifier).CloseNotify() + addr, err := alloc.Allocate(ident, subnet.HostRange(), closedChan) + if err != nil { + badRequest(w, err) + return + } + if checkAlive && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) { + common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, addr) + alloc.Free(ident, addr) + return + } + + fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen) +} + // HandleHTTP wires up ipams HTTP endpoints to the provided mux. func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CIDR, dockerCli *docker.Client) { router.Methods("PUT").Path("/ip/{id}/{ip}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -36,18 +65,14 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID router.Methods("GET").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) - cidr := vars["ip"] + "/" + vars["prefixlen"] - _, subnet, err := address.ParseCIDR(cidr) - if err != nil { - badRequest(w, err) - return - } - addr, err := alloc.Lookup(vars["id"], subnet.HostRange()) - if err != nil { - http.NotFound(w, r) - return + if subnet, ok := parseCIDR(w, vars["ip"]+"/"+vars["prefixlen"]); ok { + addr, err := alloc.Lookup(vars["id"], subnet.HostRange()) + if err != nil { + http.NotFound(w, r) + return + } + fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen) } - fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen) }) router.Methods("GET").Path("/ip/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -60,48 +85,15 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID }) router.Methods("POST").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - closedChan := w.(http.CloseNotifier).CloseNotify() vars := mux.Vars(r) - ident := vars["id"] - cidrStr := vars["ip"] + "/" + vars["prefixlen"] - subnetAddr, cidr, err := address.ParseCIDR(cidrStr) - if err != nil { - badRequest(w, err) - return - } - if cidr.Start != subnetAddr { - badRequest(w, fmt.Errorf("Invalid subnet %s - bits after network prefix are not all zero", cidrStr)) - return - } - addr, err := alloc.Allocate(ident, cidr.HostRange(), closedChan) - if err != nil { - badRequest(w, fmt.Errorf("Unable to allocate: %s", err)) - return + if subnet, ok := parseCIDR(w, vars["ip"]+"/"+vars["prefixlen"]); ok { + alloc.handleHTTPAllocate(dockerCli, w, vars["id"], r.FormValue("check-alive") == "true", subnet) } - if r.FormValue("check-alive") == "true" && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) { - common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, addr) - alloc.Free(ident, addr) - return - } - - 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.HostRange(), closedChan) - if err != nil { - badRequest(w, err) - return - } - if r.FormValue("check-alive") == "true" && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) { - common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, newAddr) - alloc.Free(ident, newAddr) - return - } - - fmt.Fprintf(w, "%s/%d", newAddr, defaultSubnet.PrefixLen) + vars := mux.Vars(r) + alloc.handleHTTPAllocate(dockerCli, w, vars["id"], r.FormValue("check-alive") == "true", defaultSubnet) }) router.Methods("DELETE").Path("/ip/{id}/{ip}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 98b80c59d92f1f3531c868925b17496427de86d8 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 14 Sep 2015 14:35:29 +0100 Subject: [PATCH 2/3] Detect cancelled allocation more cleanly to avoid the weave script trying to parse an error message as a CIDR --- ipam/allocate.go | 2 +- ipam/allocator.go | 9 +++++++++ ipam/claim.go | 2 +- ipam/http.go | 6 ++++++ weave | 11 +++++++++-- 5 files changed, 26 insertions(+), 4 deletions(-) diff --git a/ipam/allocate.go b/ipam/allocate.go index ebb30217b6..e8950e3722 100644 --- a/ipam/allocate.go +++ b/ipam/allocate.go @@ -59,7 +59,7 @@ func (g *allocate) Try(alloc *Allocator) bool { } func (g *allocate) Cancel() { - g.resultChan <- allocateResult{0, fmt.Errorf("Allocate request for %s cancelled", g.ident)} + g.resultChan <- allocateResult{0, &errorCancelled{"Allocate", g.ident}} } func (g *allocate) ForContainer(ident string) bool { diff --git a/ipam/allocator.go b/ipam/allocator.go index 28f2e449a3..a55e35c71e 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -184,6 +184,15 @@ func hasBeenCancelled(cancelChan <-chan bool) func() bool { } } +type errorCancelled struct { + kind string + ident string +} + +func (e *errorCancelled) Error() string { + return fmt.Sprintf("%s request for %s cancelled", e.kind, e.ident) +} + // Actor client API // Allocate (Sync) - get new IP address for container with given name in range diff --git a/ipam/claim.go b/ipam/claim.go index 5e79d7d10e..2515afd2ad 100644 --- a/ipam/claim.go +++ b/ipam/claim.go @@ -95,7 +95,7 @@ func (c *claim) deniedBy(alloc *Allocator, owner router.PeerName) { } func (c *claim) Cancel() { - c.sendResult(fmt.Errorf("Operation cancelled.")) + c.sendResult(&errorCancelled{"Claim", c.ident}) } func (c *claim) ForContainer(ident string) bool { diff --git a/ipam/http.go b/ipam/http.go index a8e6288060..40874108da 100644 --- a/ipam/http.go +++ b/ipam/http.go @@ -33,12 +33,18 @@ func (alloc *Allocator) handleHTTPAllocate(dockerCli *docker.Client, w http.Resp closedChan := w.(http.CloseNotifier).CloseNotify() addr, err := alloc.Allocate(ident, subnet.HostRange(), closedChan) if err != nil { + if _, ok := err.(*errorCancelled); ok { // cancellation is not really an error + common.Log.Infoln("[allocator]:", err.Error()) + fmt.Fprint(w, "cancelled") + return + } badRequest(w, err) return } if checkAlive && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) { common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, addr) alloc.Free(ident, addr) + fmt.Fprint(w, "cancelled") return } diff --git a/weave b/weave index f3220ff257..4b132a4e07 100755 --- a/weave +++ b/weave @@ -919,6 +919,8 @@ ipam_cidrs() { if [ "$IPAM_CIDRS" = "404 page not found" ] ; then echo "No IP address supplied (use the -iprange option on 'weave launch' to enable IP address allocation)" >&2 return 1 + elif [ "$IPAM_CIDRS" = "cancelled" ] ; then + return 0 fi ALL_CIDRS="$IPAM_CIDRS" fi @@ -933,8 +935,9 @@ ipam_cidrs() { if [ "$CIDR" = "404 page not found" ] ; then echo "IP address allocation must be enabled to use 'net:'" >&2 return 1 - fi - if ! is_cidr "$CIDR" ; then + elif [ "$CIDR" = "cancelled" ] ; then + return 0 + elif ! is_cidr "$CIDR" ; then echo "$CIDR" >&2 return 1 fi @@ -1486,6 +1489,10 @@ case "$COMMAND" in CONTAINER=$(container_id $1) create_bridge ipam_cidrs$ATTACH_TYPE $CONTAINER $CIDR_ARGS + if [ "$IPAM_CIDRS" = "cancelled" ] ; then + echo "cancelled" >&2 + exit 0 + fi if [ -n "$REWRITE_HOSTS" ] && command_exists weavehosts ; then PATH_AND_NAME=$(docker inspect -f '{{.HostsPath}} {{.Config.Hostname}}' $CONTAINER) weavehosts $PATH_AND_NAME $ALL_CIDRS From abf13c70260269cbf9014246f0cb35dc7f4758c0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 22 Sep 2015 15:57:45 +0100 Subject: [PATCH 3/3] Simplify detection of issues --- weave | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/weave b/weave index 4b132a4e07..5b1cf5fea2 100755 --- a/weave +++ b/weave @@ -919,8 +919,9 @@ ipam_cidrs() { if [ "$IPAM_CIDRS" = "404 page not found" ] ; then echo "No IP address supplied (use the -iprange option on 'weave launch' to enable IP address allocation)" >&2 return 1 - elif [ "$IPAM_CIDRS" = "cancelled" ] ; then - return 0 + elif ! is_cidr "$IPAM_CIDRS" ; then + echo "$IPAM_CIDRS" >&2 + return 1 fi ALL_CIDRS="$IPAM_CIDRS" fi @@ -935,8 +936,6 @@ ipam_cidrs() { if [ "$CIDR" = "404 page not found" ] ; then echo "IP address allocation must be enabled to use 'net:'" >&2 return 1 - elif [ "$CIDR" = "cancelled" ] ; then - return 0 elif ! is_cidr "$CIDR" ; then echo "$CIDR" >&2 return 1 @@ -1489,10 +1488,6 @@ case "$COMMAND" in CONTAINER=$(container_id $1) create_bridge ipam_cidrs$ATTACH_TYPE $CONTAINER $CIDR_ARGS - if [ "$IPAM_CIDRS" = "cancelled" ] ; then - echo "cancelled" >&2 - exit 0 - fi if [ -n "$REWRITE_HOSTS" ] && command_exists weavehosts ; then PATH_AND_NAME=$(docker inspect -f '{{.HostsPath}} {{.Config.Hostname}}' $CONTAINER) weavehosts $PATH_AND_NAME $ALL_CIDRS