From 340bf2f46f850302b5ad277fae720f88880ee2ca Mon Sep 17 00:00:00 2001 From: Shoma Kokuryo Date: Thu, 23 Aug 2018 06:19:29 +0000 Subject: [PATCH 1/4] remove restriction of BMC type --- e2e/sabactl_test.go | 4 ++-- machines.go | 7 ------- machines_test.go | 2 +- models/etcd/index_test.go | 12 ++++++------ web/machines.go | 4 ---- web/machines_test.go | 8 ++++---- 6 files changed, 13 insertions(+), 24 deletions(-) diff --git a/e2e/sabactl_test.go b/e2e/sabactl_test.go index fb31017d..58167b29 100644 --- a/e2e/sabactl_test.go +++ b/e2e/sabactl_test.go @@ -184,7 +184,7 @@ func testSabactlMachines(t *testing.T) { Datacenter: "ty3", Role: "worker", BMC: sabakan.MachineBMC{ - Type: sabakan.BmcIdrac9, + Type: "iDRAC-9", }, }, { @@ -193,7 +193,7 @@ func testSabactlMachines(t *testing.T) { Datacenter: "ty3", Role: "boot", BMC: sabakan.MachineBMC{ - Type: sabakan.BmcIpmi2, + Type: "IPMI-2.0", }, }, } diff --git a/machines.go b/machines.go index 901c9c65..83d6e485 100644 --- 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 diff --git a/machines_test.go b/machines_test.go index 3ff3db91..2ebc2dac 100644 --- a/machines_test.go +++ b/machines_test.go @@ -29,7 +29,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 index 94cd3add..2c00f4de 100644 --- 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", Product: "R630", Datacenter: "ty3", Role: "boot", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}), - sabakan.NewMachine(sabakan.MachineSpec{Serial: "2", Product: "R630", Datacenter: "ty3", Role: "worker", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}), - sabakan.NewMachine(sabakan.MachineSpec{Serial: "3", Product: "R730xd", Datacenter: "ty3", Role: "worker", BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}}), + sabakan.NewMachine(sabakan.MachineSpec{Serial: "1", Product: "R630", Datacenter: "ty3", Role: "boot", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}), + sabakan.NewMachine(sabakan.MachineSpec{Serial: "2", Product: "R630", Datacenter: "ty3", Role: "worker", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}), + sabakan.NewMachine(sabakan.MachineSpec{Serial: "3", Product: "R730xd", Datacenter: "ty3", Role: "worker", BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}}), } for _, m := range machines { @@ -36,7 +36,7 @@ func TestMachinesIndex(t *testing.T) { Product: "R630", Datacenter: "ty3", Role: "worker", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}, + BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}, }) current := sabakan.NewMachine( sabakan.MachineSpec{ @@ -44,7 +44,7 @@ func TestMachinesIndex(t *testing.T) { Product: "R730xd", Datacenter: "ty3", Role: "worker", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}, + BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}, }) current.Status.State = sabakan.StateRetiring @@ -70,7 +70,7 @@ func TestMachinesIndex(t *testing.T) { Product: "R730xd", Datacenter: "ty3", Role: "worker", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}, + BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}, })) serials = mi.query(sabakan.Query{"product": "R730xd"}) diff --git a/web/machines.go b/web/machines.go index 7ddc547f..a32e65e6 100644 --- a/web/machines.go +++ b/web/machines.go @@ -56,10 +56,6 @@ 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")) - return - } m.IPv4 = nil m.IPv6 = nil } diff --git a/web/machines_test.go b/web/machines_test.go index 5e6ffd60..1bfd9dcd 100644 --- a/web/machines_test.go +++ b/web/machines_test.go @@ -82,7 +82,7 @@ func testMachinesPost(t *testing.T) { "rack": 1, "role": "boot", "bmc": {"type": "unknown-BMC"} -}]`, http.StatusBadRequest}, +}]`, http.StatusCreated}, {`[{ "serial": "2222abcd", "product": "R630", @@ -125,7 +125,7 @@ func testMachinesGet(t *testing.T) { Datacenter: "ty3", Rack: 1, Role: "boot", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIdrac9}, + BMC: sabakan.MachineBMC{Type: "iDRAC-9"}, }), sabakan.NewMachine(sabakan.MachineSpec{ Serial: "5678abcd", @@ -133,7 +133,7 @@ func testMachinesGet(t *testing.T) { Datacenter: "ty3", Rack: 1, Role: "worker", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIdrac9}, + BMC: sabakan.MachineBMC{Type: "iDRAC-9"}, }), sabakan.NewMachine(sabakan.MachineSpec{ Serial: "1234efgh", @@ -141,7 +141,7 @@ func testMachinesGet(t *testing.T) { Datacenter: "ty3", Rack: 2, Role: "boot", - BMC: sabakan.MachineBMC{Type: sabakan.BmcIpmi2}, + BMC: sabakan.MachineBMC{Type: "IPMI-2.0"}, }), }) From 2b7b2207b32f680312a079c5bbd09dbd627a2921 Mon Sep 17 00:00:00 2001 From: Shoma Kokuryo Date: Thu, 23 Aug 2018 06:37:58 +0000 Subject: [PATCH 2/4] add validation --- web/machines.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/web/machines.go b/web/machines.go index a32e65e6..2ee260e4 100644 --- a/web/machines.go +++ b/web/machines.go @@ -3,11 +3,16 @@ package web import ( "encoding/json" "net/http" + "regexp" "strings" "github.com/cybozu-go/sabakan" ) +var ( + bmcTypeRegex = regexp.MustCompile(`^[a-z0-9A-Z-_/.]+$`) +) + func (s Server) handleMachines(w http.ResponseWriter, r *http.Request) { switch r.Method { case "GET": @@ -56,6 +61,10 @@ func (s Server) handleMachinesPost(w http.ResponseWriter, r *http.Request) { renderError(r.Context(), w, BadRequest("BMC type is empty")) return } + if !bmcTypeRegex.MatchString(m.BMC.Type) { + renderError(r.Context(), w, BadRequest("BMC type contains invalid character")) + return + } m.IPv4 = nil m.IPv6 = nil } From 3c609dd62239f9393449cc2ca427eb1a3c5fc715 Mon Sep 17 00:00:00 2001 From: Shoma Kokuryo Date: Thu, 23 Aug 2018 07:22:42 +0000 Subject: [PATCH 3/4] implement validate method --- machines.go | 8 +++++++- machines_test.go | 16 ++++++++++++++++ web/machines.go | 7 +------ web/machines_test.go | 4 ++-- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/machines.go b/machines.go index 83d6e485..58c79926 100644 --- a/machines.go +++ b/machines.go @@ -24,7 +24,8 @@ const ( ) var ( - reValidRole = regexp.MustCompile(`^[a-zA-Z][0-9a-zA-Z._-]*$`) + reValidRole = regexp.MustCompile(`^[a-zA-Z][0-9a-zA-Z._-]*$`) + reValidBmcType = regexp.MustCompile(`^[a-z0-9A-Z-_/.]+$`) ) // IsValidRole returns true if role is valid as machine role @@ -32,6 +33,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) +} + // MachineBMC is a bmc interface struct for Machine type MachineBMC struct { IPv4 string `json:"ipv4"` diff --git a/machines_test.go b/machines_test.go index 2ebc2dac..bfdef47f 100644 --- 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 TestMachine(t *testing.T) { t.Parallel() diff --git a/web/machines.go b/web/machines.go index 2ee260e4..7fd8520f 100644 --- a/web/machines.go +++ b/web/machines.go @@ -3,16 +3,11 @@ package web import ( "encoding/json" "net/http" - "regexp" "strings" "github.com/cybozu-go/sabakan" ) -var ( - bmcTypeRegex = regexp.MustCompile(`^[a-z0-9A-Z-_/.]+$`) -) - func (s Server) handleMachines(w http.ResponseWriter, r *http.Request) { switch r.Method { case "GET": @@ -61,7 +56,7 @@ func (s Server) handleMachinesPost(w http.ResponseWriter, r *http.Request) { renderError(r.Context(), w, BadRequest("BMC type is empty")) return } - if !bmcTypeRegex.MatchString(m.BMC.Type) { + if !sabakan.IsValidBmcType(m.BMC.Type) { renderError(r.Context(), w, BadRequest("BMC type contains invalid character")) return } diff --git a/web/machines_test.go b/web/machines_test.go index 1bfd9dcd..b990ed35 100644 --- a/web/machines_test.go +++ b/web/machines_test.go @@ -81,8 +81,8 @@ func testMachinesPost(t *testing.T) { "datacenter": "ty3", "rack": 1, "role": "boot", - "bmc": {"type": "unknown-BMC"} -}]`, http.StatusCreated}, + "bmc": {"type": "&invalid$ bmc#type+"} +}]`, http.StatusBadRequest}, {`[{ "serial": "2222abcd", "product": "R630", From 66a946190139a521ecca3399eba18da38298a325 Mon Sep 17 00:00:00 2001 From: Shoma Kokuryo Date: Thu, 23 Aug 2018 08:10:00 +0000 Subject: [PATCH 4/4] fix goimports error --- machines_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/machines_test.go b/machines_test.go index fcac88c4..be062464 100644 --- a/machines_test.go +++ b/machines_test.go @@ -34,8 +34,8 @@ func TestIsValidBmcType(t *testing.T) { for _, ty := range invalidTypes { if IsValidBmcType(ty) { t.Error("validator should return false:", ty) - } - } + } + } } func TestIsValidLabelName(t *testing.T) {