From eba45142083477fbb2a2269ce92c199613ff4cc8 Mon Sep 17 00:00:00 2001 From: Zheng Xiangsheng Date: Mon, 8 Jun 2020 14:47:22 +0800 Subject: [PATCH 1/3] config: validate dashboard-address Signed-off-by: Zheng Xiangsheng --- pkg/dashboard/adapter/manager.go | 6 ------ pkg/dashboard/adapter/redirector.go | 2 +- server/cluster/cluster.go | 16 ++++++++++++++++ server/config/config.go | 14 ++++++++++++++ server/config/util.go | 12 ++++++++++++ server/server.go | 23 +++++++++++++++++++++++ 6 files changed, 66 insertions(+), 7 deletions(-) diff --git a/pkg/dashboard/adapter/manager.go b/pkg/dashboard/adapter/manager.go index 4ee2f281fd6..f469a1ae6f5 100644 --- a/pkg/dashboard/adapter/manager.go +++ b/pkg/dashboard/adapter/manager.go @@ -15,7 +15,6 @@ package adapter import ( "context" - "net/url" "sort" "sync" "time" @@ -137,11 +136,6 @@ func (m *Manager) checkAddress() { m.stopService() return default: - if _, err := url.Parse(dashboardAddress); err != nil { - log.Error("illegal dashboard address", zap.String("address", dashboardAddress)) - return - } - if m.isLeader && m.needResetAddress(dashboardAddress) { m.setNewAddress() return diff --git a/pkg/dashboard/adapter/redirector.go b/pkg/dashboard/adapter/redirector.go index 20d0350e21d..c24ddef04bd 100644 --- a/pkg/dashboard/adapter/redirector.go +++ b/pkg/dashboard/adapter/redirector.go @@ -69,7 +69,7 @@ func (h *Redirector) SetAddress(addr string) { h.status.Start() h.address = addr - target, _ := url.Parse(addr) // error has been handled in checkAddress + target, _ := url.Parse(addr) // error has been handled h.proxy = httputil.NewSingleHostReverseProxy(target) defaultDirector := h.proxy.Director diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index 8df117597ed..3e67eed2c02 100644 --- a/server/cluster/cluster.go +++ b/server/cluster/cluster.go @@ -1695,3 +1695,19 @@ func GetMembers(etcdClient *clientv3.Client) ([]*pdpb.Member, error) { return members, nil } + +// IsClientUrl returns whether addr is a ClientUrl of any member. +func IsClientUrl(addr string, etcdClient *clientv3.Client) bool { + members, err := GetMembers(etcdClient) + if err != nil { + return false + } + for _, member := range members { + for _, u := range member.GetClientUrls() { + if u == addr { + return true + } + } + } + return false +} diff --git a/server/config/config.go b/server/config/config.go index 6bc8f8a3b73..9cd384c8fe8 100644 --- a/server/config/config.go +++ b/server/config/config.go @@ -965,6 +965,20 @@ func (c *PDServerConfig) Clone() *PDServerConfig { } } +// Validate is used to validate if some pd-server configurations are right. +func (c *PDServerConfig) Validate() error { + switch c.DashboardAddress { + case "auto": + case "none": + default: + if err := ValidateURLWithScheme(c.DashboardAddress); err != nil { + return err + } + } + + return nil +} + // StoreLabel is the config item of LabelPropertyConfig. type StoreLabel struct { Key string `toml:"key" json:"key"` diff --git a/server/config/util.go b/server/config/util.go index bdcb4747427..d5e6045710b 100644 --- a/server/config/util.go +++ b/server/config/util.go @@ -14,6 +14,7 @@ package config import ( + "net/url" "regexp" "github.com/pingcap/kvproto/pkg/metapb" @@ -49,3 +50,14 @@ func ValidateLabels(labels []*metapb.StoreLabel) error { } return nil } + +func ValidateURLWithScheme(rawurl string) error { + u, err := url.ParseRequestURI(rawurl) + if err != nil { + return err + } + if u.Scheme == "" || u.Host == "" { + return errors.Errorf("%s has no scheme", rawurl) + } + return nil +} diff --git a/server/server.go b/server/server.go index aa369bb4bd3..a2d4cbbd779 100644 --- a/server/server.go +++ b/server/server.go @@ -595,6 +595,14 @@ func (s *Server) GetAddr() string { return s.cfg.AdvertiseClientUrls } +// GetClientScheme returns the client URL scheme +func (s *Server) GetClientScheme() string { + if len(s.cfg.Security.CertPath) == 0 && len(s.cfg.Security.KeyPath) == 0 { + return "http" + } + return "https" +} + // GetMemberInfo returns the server member information. func (s *Server) GetMemberInfo() *pdpb.Member { return proto.Clone(s.member.Member()).(*pdpb.Member) @@ -786,6 +794,21 @@ func (s *Server) GetPDServerConfig() *config.PDServerConfig { // SetPDServerConfig sets the server config. func (s *Server) SetPDServerConfig(cfg config.PDServerConfig) error { + switch cfg.DashboardAddress { + case "auto": + case "none": + default: + if !strings.HasPrefix(cfg.DashboardAddress, "http") { + cfg.DashboardAddress = fmt.Sprintf("%s://%s", s.GetClientScheme(), cfg.DashboardAddress) + } + if !cluster.IsClientUrl(cfg.DashboardAddress, s.client) { + return errors.Errorf("%s is not the client url of any member", cfg.DashboardAddress) + } + } + if err := cfg.Validate(); err != nil { + return err + } + old := s.persistOptions.GetPDServerConfig() s.persistOptions.SetPDServerConfig(&cfg) if err := s.persistOptions.Persist(s.storage); err != nil { From be948fd1e3ebfd77001d3e0fd74d4dbdcf7f62ce Mon Sep 17 00:00:00 2001 From: Zheng Xiangsheng Date: Mon, 8 Jun 2020 15:09:44 +0800 Subject: [PATCH 2/3] fix lint Signed-off-by: Zheng Xiangsheng --- server/cluster/cluster.go | 4 ++-- server/config/util.go | 1 + server/server.go | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index 3e67eed2c02..b614087defc 100644 --- a/server/cluster/cluster.go +++ b/server/cluster/cluster.go @@ -1696,8 +1696,8 @@ func GetMembers(etcdClient *clientv3.Client) ([]*pdpb.Member, error) { return members, nil } -// IsClientUrl returns whether addr is a ClientUrl of any member. -func IsClientUrl(addr string, etcdClient *clientv3.Client) bool { +// IsClientURL returns whether addr is a ClientUrl of any member. +func IsClientURL(addr string, etcdClient *clientv3.Client) bool { members, err := GetMembers(etcdClient) if err != nil { return false diff --git a/server/config/util.go b/server/config/util.go index d5e6045710b..498a475d807 100644 --- a/server/config/util.go +++ b/server/config/util.go @@ -51,6 +51,7 @@ func ValidateLabels(labels []*metapb.StoreLabel) error { return nil } +// ValidateURLWithScheme checks the format of the URL. func ValidateURLWithScheme(rawurl string) error { u, err := url.ParseRequestURI(rawurl) if err != nil { diff --git a/server/server.go b/server/server.go index a2d4cbbd779..488253f7a33 100644 --- a/server/server.go +++ b/server/server.go @@ -801,7 +801,7 @@ func (s *Server) SetPDServerConfig(cfg config.PDServerConfig) error { if !strings.HasPrefix(cfg.DashboardAddress, "http") { cfg.DashboardAddress = fmt.Sprintf("%s://%s", s.GetClientScheme(), cfg.DashboardAddress) } - if !cluster.IsClientUrl(cfg.DashboardAddress, s.client) { + if !cluster.IsClientURL(cfg.DashboardAddress, s.client) { return errors.Errorf("%s is not the client url of any member", cfg.DashboardAddress) } } From f1354ec93182b12da718b4544572e02c5e47420e Mon Sep 17 00:00:00 2001 From: Zheng Xiangsheng Date: Mon, 8 Jun 2020 15:49:42 +0800 Subject: [PATCH 3/3] add tests Signed-off-by: Zheng Xiangsheng --- server/config/config.go | 4 +-- server/config/config_test.go | 65 ++++++++++++++++++++++++++++++++++++ server/config/util_test.go | 24 ++++++++++++- 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/server/config/config.go b/server/config/config.go index 9cd384c8fe8..433dcfa25d9 100644 --- a/server/config/config.go +++ b/server/config/config.go @@ -948,10 +948,10 @@ func (c *PDServerConfig) adjust(meta *configMetaData) error { if !meta.IsDefined("dashboard-address") { c.DashboardAddress = defaultDashboardAddress } - return nil + return c.Validate() } -// Clone retruns a cloned PD server config. +// Clone returns a cloned PD server config. func (c *PDServerConfig) Clone() *PDServerConfig { runtimeServices := make(typeutil.StringSlice, len(c.RuntimeServices)) copy(runtimeServices, c.RuntimeServices) diff --git a/server/config/config_test.go b/server/config/config_test.go index c908672f5b6..e437bdcca0b 100644 --- a/server/config/config_test.go +++ b/server/config/config_test.go @@ -237,6 +237,71 @@ func newTestScheduleOption() (*PersistOptions, error) { return opt, nil } +func (s *testConfigSuite) TestPDServerConfig(c *C) { + tests := []struct { + cfgData string + hasErr bool + dashboardAddress string + }{ + { + ` +[pd-server] +dashboard-address = "http://127.0.0.1:2379" +`, + false, + "http://127.0.0.1:2379", + }, + { + ` +[pd-server] +dashboard-address = "auto" +`, + false, + "auto", + }, + { + ` +[pd-server] +dashboard-address = "none" +`, + false, + "none", + }, + { + "", + false, + "auto", + }, + { + ` +[pd-server] +dashboard-address = "127.0.0.1:2379" +`, + true, + "", + }, + { + ` +[pd-server] +dashboard-address = "foo" +`, + true, + "", + }, + } + + for _, t := range tests { + cfg := NewConfig() + meta, err := toml.Decode(t.cfgData, &cfg) + c.Assert(err, IsNil) + err = cfg.Adjust(&meta) + c.Assert(err != nil, Equals, t.hasErr) + if !t.hasErr { + c.Assert(cfg.PDServerCfg.DashboardAddress, Equals, t.dashboardAddress) + } + } +} + func (s *testConfigSuite) TestDashboardConfig(c *C) { cfgData := ` [dashboard] diff --git a/server/config/util_test.go b/server/config/util_test.go index 2bc0f1aa344..399de3aa748 100644 --- a/server/config/util_test.go +++ b/server/config/util_test.go @@ -22,7 +22,7 @@ var _ = Suite(&testUtilSuite{}) type testUtilSuite struct{} -func (s *testUtilSuite) TestVerifyLabels(c *C) { +func (s *testUtilSuite) TestValidateLabels(c *C) { tests := []struct { label string hasErr bool @@ -54,3 +54,25 @@ func (s *testUtilSuite) TestVerifyLabels(c *C) { c.Assert(ValidateLabels([]*metapb.StoreLabel{{Key: t.label}}) != nil, Equals, t.hasErr) } } + +func (s *testUtilSuite) TestValidateURLWithScheme(c *C) { + tests := []struct { + addr string + hasErr bool + }{ + {"", true}, + {"foo", true}, + {"/foo", true}, + {"http", true}, + {"http://", true}, + {"http://foo", false}, + {"https://foo", false}, + {"http://127.0.0.1", false}, + {"http://127.0.0.1/", false}, + {"https://foo.com/bar", false}, + {"https://foo.com/bar/", false}, + } + for _, t := range tests { + c.Assert(ValidateURLWithScheme(t.addr) != nil, Equals, t.hasErr) + } +}