Merge pull request #99 from cybozu-go/feature/rm_bmc_restriction

Remove restriction of BMC type
cybozu-go · Aug 24, 2018 · d251066 · d251066
2 parents 8fd1e5b + 66a9461
commit d251066
Show file tree

Hide file tree

Showing 6 changed files with 37 additions and 22 deletions.
diff --git a/e2e/sabactl_test.go b/e2e/sabactl_test.go
@@ -186,7 +186,7 @@ func testSabactlMachines(t *testing.T) {
 			},
 			Role: "worker",
 			BMC: sabakan.MachineBMC{
-				Type: sabakan.BmcIdrac9,
+				Type: "iDRAC-9",
 			},
 		},
 		{
@@ -197,7 +197,7 @@ func testSabactlMachines(t *testing.T) {
 			},
 			Role: "boot",
 			BMC: sabakan.MachineBMC{
-				Type: sabakan.BmcIpmi2,
+				Type: "IPMI-2.0",
 			},
 		},
 	}

diff --git a/machines.go b/machines.go
@@ -6,13 +6,6 @@ import (
 	"time"
 )
 
-const (
-	// BmcIdrac9 is BMC type for iDRAC-9
-	BmcIdrac9 = "iDRAC-9"
-	// BmcIpmi2 is BMC type for IPMI-2.0
-	BmcIpmi2 = "IPMI-2.0"
-)
-
 // MachineState represents a machine's state.
 type MachineState string
 
@@ -32,6 +25,7 @@ const (
 
 var (
 	reValidRole      = regexp.MustCompile(`^[a-zA-Z][0-9a-zA-Z._-]*$`)
+	reValidBmcType   = regexp.MustCompile(`^[a-z0-9A-Z-_/.]+$`)
 	reValidLabelVal  = regexp.MustCompile(`^[[:print:]]+$`)
 	reValidLabelName = regexp.MustCompile(`^[a-z0-9A-Z-_/.]+$`)
 )
@@ -41,6 +35,11 @@ func IsValidRole(role string) bool {
 	return reValidRole.MatchString(role)
 }
 
+// IsValidBmcType returns true if role is valid as BMC type
+func IsValidBmcType(bmcType string) bool {
+	return reValidBmcType.MatchString(bmcType)
+}
+
 // IsValidLabelName returns true if label name is valid
 func IsValidLabelName(name string) bool {
 	return reValidLabelName.MatchString(name)

diff --git a/machines_test.go b/machines_test.go
@@ -22,6 +22,22 @@ func TestIsValidRole(t *testing.T) {
 	}
 }
 
+func TestIsValidBmcType(t *testing.T) {
+	t.Parallel()
+	validTypes := []string{"validtype1", "valid_type2", "valid/type-3"}
+	for _, ty := range validTypes {
+		if !IsValidBmcType(ty) {
+			t.Error("validator should return true:", ty)
+		}
+	}
+	invalidTypes := []string{"invalid\\type 1", "%invalid+type#2", "^invalid$type?"}
+	for _, ty := range invalidTypes {
+		if IsValidBmcType(ty) {
+			t.Error("validator should return false:", ty)
+		}
+	}
+}
+
 func TestIsValidLabelName(t *testing.T) {
 	t.Parallel()
 
@@ -63,7 +79,7 @@ func TestMachine(t *testing.T) {
 		Serial: "abc",
 		Rack:   3,
 		Role:   "boot",
-		BMC:    MachineBMC{Type: BmcIpmi2},
+		BMC:    MachineBMC{Type: "IPMI-2.0"},
 	}
 
 	m := NewMachine(spec)

diff --git a/models/etcd/index_test.go b/models/etcd/index_test.go
@@ -13,9 +13,9 @@ func TestMachinesIndex(t *testing.T) {
 	mi := newMachinesIndex()
 
 	machines := []*sabakan.Machine{
-		sabakan.NewMachine(sabakan.MachineSpec{Serial: "1", Labels: map[string]string{"product": "R630", "datacenter": "ty3"}, Role: "boot", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}),
-		sabakan.NewMachine(sabakan.MachineSpec{Serial: "2", Labels: map[string]string{"product": "R630", "datacenter": "ty3"}, Role: "worker", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}),
-		sabakan.NewMachine(sabakan.MachineSpec{Serial: "3", Labels: map[string]string{"product": "R730xd", "datacenter": "ty3"}, Role: "worker", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}),
+		sabakan.NewMachine(sabakan.MachineSpec{Serial: "1", Labels: map[string]string{"product": "R630", "datacenter": "ty3"}, Role: "boot", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}),
+		sabakan.NewMachine(sabakan.MachineSpec{Serial: "2", Labels: map[string]string{"product": "R630", "datacenter": "ty3"}, Role: "worker", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}),
+		sabakan.NewMachine(sabakan.MachineSpec{Serial: "3", Labels: map[string]string{"product": "R730xd", "datacenter": "ty3"}, Role: "worker", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}),
 	}
 
 	for _, m := range machines {
@@ -38,7 +38,7 @@ func TestMachinesIndex(t *testing.T) {
 				"datacenter": "ty3",
 			},
 			Role: "worker",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIpmi2},
+			BMC:  sabakan.MachineBMC{Type: "IPMI-2.0"},
 		})
 	current := sabakan.NewMachine(
 		sabakan.MachineSpec{
@@ -48,7 +48,7 @@ func TestMachinesIndex(t *testing.T) {
 				"datacenter": "ty3",
 			},
 			Role: "worker",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIpmi2},
+			BMC:  sabakan.MachineBMC{Type: "IPMI-2.0"},
 		})
 	current.Status.State = sabakan.StateRetiring
 
@@ -76,7 +76,7 @@ func TestMachinesIndex(t *testing.T) {
 				"datacenter": "ty3",
 			},
 			Role: "worker",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIpmi2},
+			BMC:  sabakan.MachineBMC{Type: "IPMI-2.0"},
 		}))
 
 	serials = mi.query(sabakan.Query{"labels": "product=R730xd"})

diff --git a/web/machines.go b/web/machines.go
@@ -56,8 +56,8 @@ func (s Server) handleMachinesPost(w http.ResponseWriter, r *http.Request) {
 			renderError(r.Context(), w, BadRequest("BMC type is empty"))
 			return
 		}
-		if (m.BMC.Type != sabakan.BmcIpmi2) && (m.BMC.Type != sabakan.BmcIdrac9) {
-			renderError(r.Context(), w, BadRequest("unknown BMC type"))
+		if !sabakan.IsValidBmcType(m.BMC.Type) {
+			renderError(r.Context(), w, BadRequest("BMC type contains invalid character"))
 			return
 		}
 		m.IPv4 = nil

diff --git a/web/machines_test.go b/web/machines_test.go
@@ -106,7 +106,7 @@ func testMachinesPost(t *testing.T) {
   },
   "rack": 1,
   "role": "boot",
-  "bmc": {"type": "unknown-BMC"}
+  "bmc": {"type": "&invalid$ bmc#type+"}
 }]`, http.StatusBadRequest},
 		{`[{
   "serial": "2222abcd",
@@ -185,7 +185,7 @@ func testMachinesGet(t *testing.T) {
 			},
 			Rack: 1,
 			Role: "boot",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIdrac9},
+			BMC:  sabakan.MachineBMC{Type: "iDRAC-9"},
 		}),
 		sabakan.NewMachine(sabakan.MachineSpec{
 			Serial: "5678abcd",
@@ -195,7 +195,7 @@ func testMachinesGet(t *testing.T) {
 			},
 			Rack: 1,
 			Role: "worker",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIdrac9},
+			BMC:  sabakan.MachineBMC{Type: "iDRAC-9"},
 		}),
 		sabakan.NewMachine(sabakan.MachineSpec{
 			Serial: "1234efgh",
@@ -205,7 +205,7 @@ func testMachinesGet(t *testing.T) {
 			},
 			Rack: 2,
 			Role: "boot",
-			BMC:  sabakan.MachineBMC{Type: sabakan.BmcIpmi2},
+			BMC:  sabakan.MachineBMC{Type: "IPMI-2.0"},
 		}),
 	})