Properly return Conflict error on specific IP allocation. (#489)

Co-authored-by: Stefan Majer <[email protected]>
metal-stack · Feb 15, 2024 · 789c06c · 789c06c
1 parent 9458c9d
commit 789c06c
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 88 deletions.
diff --git a/cmd/metal-api/internal/service/ip-service.go b/cmd/metal-api/internal/service/ip-service.go
@@ -21,6 +21,8 @@ import (
 
 	v1 "github.com/metal-stack/metal-api/cmd/metal-api/internal/service/v1"
 
+	goipam "github.com/metal-stack/go-ipam"
+
 	restfulspec "github.com/emicklei/go-restful-openapi/v2"
 	restful "github.com/emicklei/go-restful/v3"
 	"github.com/metal-stack/metal-lib/httperrors"
@@ -90,18 +92,6 @@ func (r *ipResource) webService() *restful.WebService {
 		Returns(http.StatusOK, "OK", []v1.IPResponse{}).
 		DefaultReturns("Error", httperrors.HTTPErrorResponse{}))
 
-	ws.Route(ws.POST("/free/{id}").
-		To(editor(r.freeIP)).
-		Operation("freeIPDeprecated").
-		Doc("frees an ip").
-		Param(ws.PathParameter("id", "identifier of the ip").DataType("string")).
-		Consumes(restful.MIME_JSON, "*/*").
-		Metadata(restfulspec.KeyOpenAPITags, tags).
-		Writes(v1.IPResponse{}).
-		Returns(http.StatusOK, "OK", v1.IPResponse{}).
-		DefaultReturns("Error", httperrors.HTTPErrorResponse{}).
-		Deprecate())
-
 	ws.Route(ws.DELETE("/free/{id}").
 		To(editor(r.freeIP)).
 		Operation("freeIP").
@@ -320,13 +310,26 @@ func (r *ipResource) allocateIP(request *restful.Request, response *restful.Resp
 
 	// TODO: Following operations should span a database transaction if possible
 
-	ipAddress, ipParentCidr, err := allocateIP(nw, specificIP, r.ipamer)
-	if err != nil {
-		r.sendError(request, response, defaultError(err))
-		return
+	var (
+		ipAddress    string
+		ipParentCidr string
+	)
+
+	if specificIP == "" {
+		ipAddress, ipParentCidr, err = allocateRandomIP(nw, r.ipamer)
+		if err != nil {
+			r.sendError(request, response, defaultError(err))
+			return
+		}
+	} else {
+		ipAddress, ipParentCidr, err = allocateSpecificIP(nw, specificIP, r.ipamer)
+		if err != nil {
+			r.sendError(request, response, defaultError(err))
+			return
+		}
 	}
 
-	r.logger(request).Debugw("found an ip to allocate", "ip", ipAddress, "network", nw.ID)
+	r.logger(request).Debugw("allocated ip in ipam", "ip", ipAddress, "network", nw.ID)
 
 	ipType := metal.Ephemeral
 	if requestPayload.Type == metal.Static {
@@ -397,43 +400,48 @@ func (r *ipResource) updateIP(request *restful.Request, response *restful.Respon
 	r.send(request, response, http.StatusOK, v1.NewIPResponse(&newIP))
 }
 
-func allocateIP(parent *metal.Network, specificIP string, ipamer ipam.IPAMer) (string, string, error) {
-	var ee []error
-	var err error
-	var ipAddress string
-	var parentPrefixCidr string
+func allocateSpecificIP(parent *metal.Network, specificIP string, ipamer ipam.IPAMer) (ipAddress, parentPrefixCidr string, err error) {
+	parsedIP, err := netip.ParseAddr(specificIP)
+	if err != nil {
+		return "", "", fmt.Errorf("unable to parse specific ip: %w", err)
+	}
+
 	for _, prefix := range parent.Prefixes {
-		if specificIP == "" {
-			ipAddress, err = ipamer.AllocateIP(prefix)
-		} else {
-			pfx, perr := netip.ParsePrefix(prefix.String())
-			if perr != nil {
-				return "", "", fmt.Errorf("unable to parse prefix %w", perr)
-			}
-			sip, serr := netip.ParseAddr(specificIP)
-			if serr != nil {
-				return "", "", fmt.Errorf("unable to parse specific ip %w", serr)
-			}
-			if !pfx.Contains(sip) {
-				continue
-			}
-			ipAddress, err = ipamer.AllocateSpecificIP(prefix, specificIP)
-		}
+		pfx, err := netip.ParsePrefix(prefix.String())
 		if err != nil {
-			ee = append(ee, err)
+			return "", "", fmt.Errorf("unable to parse prefix: %w", err)
+		}
+
+		if !pfx.Contains(parsedIP) {
 			continue
 		}
-		if ipAddress != "" {
-			parentPrefixCidr = prefix.String()
-			break
+
+		ipAddress, err = ipamer.AllocateSpecificIP(prefix, specificIP)
+		if err != nil && errors.Is(err, goipam.ErrAlreadyAllocated) {
+			return "", "", metal.Conflict("ip already allocated")
+		}
+		if err != nil {
+			return "", "", err
 		}
+
+		return ipAddress, prefix.String(), nil
 	}
-	if ipAddress == "" {
-		if len(ee) > 0 {
-			return "", "", fmt.Errorf("cannot allocate free ip in ipam: %v", ee)
+
+	return "", "", fmt.Errorf("specific ip not contained in any of the defined prefixes")
+}
+
+func allocateRandomIP(parent *metal.Network, ipamer ipam.IPAMer) (ipAddress, parentPrefixCidr string, err error) {
+	for _, prefix := range parent.Prefixes {
+		ipAddress, err = ipamer.AllocateIP(prefix)
+		if err != nil && errors.Is(err, goipam.ErrNoIPAvailable) {
+			continue
+		}
+		if err != nil {
+			return "", "", err
 		}
-		return "", "", errors.New("cannot allocate free ip in ipam")
+
+		return ipAddress, prefix.String(), nil
 	}
 
-	return ipAddress, parentPrefixCidr, nil
+	return "", "", metal.Internal("cannot allocate free ip in ipam, no ips left")
 }
diff --git a/cmd/metal-api/internal/service/ip-service_test.go b/cmd/metal-api/internal/service/ip-service_test.go
@@ -145,7 +145,7 @@ func TestDeleteIP(t *testing.T) {
 	for i := range tests {
 		tt := tests[i]
 		t.Run(tt.name, func(t *testing.T) {
-			req := httptest.NewRequest("POST", "/v1/ip/free/"+tt.ip, nil)
+			req := httptest.NewRequest("DELETE", "/v1/ip/free/"+tt.ip, nil)
 			container = injectEditor(logger, container, req)
 			req.Header.Add("Content-Type", "application/json")
 			w := httptest.NewRecorder()
@@ -244,7 +244,7 @@ func TestAllocateIP(t *testing.T) {
 			},
 			specificIP:   "11.0.0.5",
 			wantedStatus: http.StatusUnprocessableEntity,
-			wantErr:      errors.New("cannot allocate free ip in ipam"),
+			wantErr:      errors.New("specific ip not contained in any of the defined prefixes"),
 		},
 	}
 	for i := range tests {

diff --git a/cmd/metal-api/internal/service/machine-service.go b/cmd/metal-api/internal/service/machine-service.go
@@ -1626,7 +1626,7 @@ func gatherUnderlayNetwork(ds *datastore.RethinkStore, partition *metal.Partitio
 
 func makeMachineNetwork(ds *datastore.RethinkStore, ipamer ipam.IPAMer, allocationSpec *machineAllocationSpec, n *allocationNetwork) (*metal.MachineNetwork, error) {
 	if n.auto {
-		ipAddress, ipParentCidr, err := allocateIP(n.network, "", ipamer)
+		ipAddress, ipParentCidr, err := allocateRandomIP(n.network, ipamer)
 		if err != nil {
 			return nil, fmt.Errorf("unable to allocate an ip in network: %s %w", n.network.ID, err)
 		}

diff --git a/spec/metal-api.json b/spec/metal-api.json
@@ -6665,44 +6665,6 @@
         "tags": [
           "ip"
         ]
-      },
-      "post": {
-        "consumes": [
-          "*/*",
-          "application/json"
-        ],
-        "deprecated": true,
-        "operationId": "freeIPDeprecated",
-        "parameters": [
-          {
-            "description": "identifier of the ip",
-            "in": "path",
-            "name": "id",
-            "required": true,
-            "type": "string"
-          }
-        ],
-        "produces": [
-          "application/json"
-        ],
-        "responses": {
-          "200": {
-            "description": "OK",
-            "schema": {
-              "$ref": "#/definitions/v1.IPResponse"
-            }
-          },
-          "default": {
-            "description": "Error",
-            "schema": {
-              "$ref": "#/definitions/httperrors.HTTPErrorResponse"
-            }
-          }
-        },
-        "summary": "frees an ip",
-        "tags": [
-          "ip"
-        ]
       }
     },
     "/v1/ip/{id}": {