Fixing switch force delete. (#547)

metal-stack · Jul 8, 2024 · cead5f8 · cead5f8
1 parent d0c28f4
commit cead5f8
Show file tree

Hide file tree

Showing 4 changed files with 143 additions and 12 deletions.
diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go
@@ -81,11 +81,10 @@ func (r *switchResource) webService() *restful.WebService {
 		Operation("deleteSwitch").
 		Doc("deletes an switch and returns the deleted entity").
 		Param(ws.PathParameter("id", "identifier of the switch").DataType("string")).
-		Param(ws.PathParameter("force", "if true switch is deleted with no validation").DataType("boolean").DefaultValue("false")).
+		Param(ws.QueryParameter("force", "if true switch is deleted with no validation").DataType("boolean").DefaultValue("false")).
 		Metadata(restfulspec.KeyOpenAPITags, tags).
 		Writes(v1.SwitchResponse{}).
 		Returns(http.StatusOK, "OK", v1.SwitchResponse{}).
-		Returns(http.StatusBadRequest, "Bad input data", httperrors.HTTPErrorResponse{}).
 		DefaultReturns("Error", httperrors.HTTPErrorResponse{}))
 
 	ws.Route(ws.POST("/register").
@@ -194,9 +193,15 @@ func (r *switchResource) findSwitches(request *restful.Request, response *restfu
 
 func (r *switchResource) deleteSwitch(request *restful.Request, response *restful.Response) {
 	id := request.PathParameter("id")
-	force, err := strconv.ParseBool(request.PathParameter("force"))
+	forceParam := request.QueryParameter("force")
+	if forceParam == "" {
+		forceParam = "false"
+	}
+
+	force, err := strconv.ParseBool(forceParam)
 	if err != nil {
 		r.sendError(request, response, httperrors.BadRequest(err))
+		return
 	}
 
 	s, err := r.ds.FindSwitch(id)

diff --git a/cmd/metal-api/internal/service/switch-service_test.go b/cmd/metal-api/internal/service/switch-service_test.go
@@ -13,14 +13,17 @@ import (
 
 	restful "github.com/emicklei/go-restful/v3"
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/rethinkdb/rethinkdb-go.v6"
 	r "gopkg.in/rethinkdb/rethinkdb-go.v6"
 
 	"github.com/metal-stack/metal-api/cmd/metal-api/internal/datastore"
 	"github.com/metal-stack/metal-api/cmd/metal-api/internal/metal"
 	v1 "github.com/metal-stack/metal-api/cmd/metal-api/internal/service/v1"
 	"github.com/metal-stack/metal-api/cmd/metal-api/internal/testdata"
 	"github.com/metal-stack/metal-lib/httperrors"
+	"github.com/metal-stack/metal-lib/pkg/pointer"
 )
 
 func TestRegisterSwitch(t *testing.T) {
@@ -1418,3 +1421,128 @@ func TestToggleSwitchNicWithoutMachine(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, result.Message, fmt.Sprintf("switch %q does not have a connected machine at port %q", testdata.Switch1.ID, testdata.Switch1.Nics[1].Name))
 }
+
+func Test_SwitchDelete(t *testing.T) {
+	tests := []struct {
+		name       string
+		mockFn     func(mock *rethinkdb.Mock)
+		want       *v1.SwitchResponse
+		wantErr    error
+		wantStatus int
+		force      bool
+	}{
+		{
+			name: "delete switch",
+			mockFn: func(mock *rethinkdb.Mock) {
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
+					Base: metal.Base{
+						ID: "switch-1",
+					},
+				}, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil)
+			},
+			want: &v1.SwitchResponse{
+				Common: v1.Common{
+					Identifiable: v1.Identifiable{
+						ID: "switch-1",
+					},
+					Describable: v1.Describable{
+						Name:        pointer.Pointer(""),
+						Description: pointer.Pointer(""),
+					},
+				},
+				Nics:        v1.SwitchNics{},
+				Connections: []v1.SwitchConnection{},
+			},
+			wantStatus: http.StatusOK,
+		},
+		{
+			name: "delete switch does not work due to machine connections",
+			mockFn: func(mock *rethinkdb.Mock) {
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
+					Base: metal.Base{
+						ID: "switch-1",
+					},
+					MachineConnections: metal.ConnectionMap{
+						"port-a": metal.Connections{},
+					},
+				}, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
+			},
+			wantErr: &httperrors.HTTPErrorResponse{
+				StatusCode: http.StatusBadRequest,
+				Message:    "cannot delete switch switch-1 while it still has machines connected to it",
+			},
+			wantStatus: http.StatusBadRequest,
+		},
+		{
+			name: "delete switch with force",
+			mockFn: func(mock *rethinkdb.Mock) {
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
+					Base: metal.Base{
+						ID: "switch-1",
+					},
+					MachineConnections: metal.ConnectionMap{
+						"port-a": metal.Connections{},
+					},
+				}, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
+				mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil)
+			},
+			force: true,
+			want: &v1.SwitchResponse{
+				Common: v1.Common{
+					Identifiable: v1.Identifiable{
+						ID: "switch-1",
+					},
+					Describable: v1.Describable{
+						Name:        pointer.Pointer(""),
+						Description: pointer.Pointer(""),
+					},
+				},
+				Nics:        v1.SwitchNics{},
+				Connections: []v1.SwitchConnection{},
+			},
+			wantStatus: http.StatusOK,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			var (
+				ds, mock = datastore.InitMockDB(t)
+				ws       = NewSwitch(slog.Default(), ds)
+			)
+
+			if tt.mockFn != nil {
+				tt.mockFn(mock)
+			}
+
+			if tt.wantErr != nil {
+				code, got := genericWebRequest[*httperrors.HTTPErrorResponse](t, ws, testAdminUser, nil, "DELETE", "/v1/switch/switch-1")
+				assert.Equal(t, tt.wantStatus, code)
+
+				if diff := cmp.Diff(tt.wantErr, got); diff != "" {
+					t.Errorf("diff (-want +got):\n%s", diff)
+				}
+
+				return
+			}
+
+			force := ""
+			if tt.force {
+				force = "?force=true"
+			}
+
+			code, got := genericWebRequest[*v1.SwitchResponse](t, ws, testAdminUser, nil, "DELETE", "/v1/switch/switch-1"+force)
+			assert.Equal(t, tt.wantStatus, code)
+
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("diff (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
diff --git a/cmd/metal-api/internal/service/v1/switch.go b/cmd/metal-api/internal/service/v1/switch.go
@@ -138,6 +138,11 @@ func NewSwitchResponse(s *metal.Switch, ss *metal.SwitchStatus, p *metal.Partiti
 		}
 	}
 
+	var partition PartitionResponse
+	if partitionResp := NewPartitionResponse(p); partitionResp != nil {
+		partition = *partitionResp
+	}
+
 	return &SwitchResponse{
 		Common: Common{
 			Identifiable: Identifiable{
@@ -157,7 +162,7 @@ func NewSwitchResponse(s *metal.Switch, ss *metal.SwitchStatus, p *metal.Partiti
 			ConsoleCommand: s.ConsoleCommand,
 		},
 		Nics:          nics,
-		Partition:     *NewPartitionResponse(p),
+		Partition:     partition,
 		Connections:   cons,
 		LastSync:      snr.LastSync,
 		LastSyncError: snr.LastSyncError,

diff --git a/spec/metal-api.json b/spec/metal-api.json
@@ -9371,9 +9371,8 @@
           {
             "default": false,
             "description": "if true switch is deleted with no validation",
-            "in": "path",
+            "in": "query",
             "name": "force",
-            "required": true,
             "type": "boolean"
           }
         ],
@@ -9387,12 +9386,6 @@
               "$ref": "#/definitions/v1.SwitchResponse"
             }
           },
-          "400": {
-            "description": "Bad input data",
-            "schema": {
-              "$ref": "#/definitions/httperrors.HTTPErrorResponse"
-            }
-          },
           "default": {
             "description": "Error",
             "schema": {