From 526e809bd50eed4ff5c2211adc91126abb864530 Mon Sep 17 00:00:00 2001 From: fatedier Date: Thu, 16 Nov 2023 21:03:36 +0800 Subject: [PATCH] update for strict config (#3779) --- Release.md | 4 ++ client/admin_api.go | 9 +++- client/service.go | 17 +++----- cmd/frpc/sub/admin.go | 4 +- cmd/frpc/sub/nathole.go | 2 +- cmd/frpc/sub/proxy.go | 4 +- cmd/frpc/sub/root.go | 21 ++++------ cmd/frpc/sub/verify.go | 2 +- cmd/frps/root.go | 10 ++--- cmd/frps/verify.go | 2 +- pkg/config/load.go | 3 +- pkg/config/load_test.go | 74 +++++++++++++++++++++------------ pkg/sdk/client/client.go | 13 +++++- test/e2e/legacy/basic/client.go | 2 +- test/e2e/v1/basic/client.go | 2 +- 15 files changed, 99 insertions(+), 70 deletions(-) diff --git a/Release.md b/Release.md index a834392..b424518 100644 --- a/Release.md +++ b/Release.md @@ -1,3 +1,7 @@ +### Features + +* New command line parameter `--strict_config` is added to enable strict configuration validation mode. It will throw an error for non-existent fields instead of ignoring them. + ### Fixes * frpc: Return code 1 when the first login attempt fails and exits. diff --git a/client/admin_api.go b/client/admin_api.go index 84db31c..3a56a99 100644 --- a/client/admin_api.go +++ b/client/admin_api.go @@ -45,8 +45,13 @@ func (svr *Service) healthz(w http.ResponseWriter, _ *http.Request) { } // GET /api/reload -func (svr *Service) apiReload(w http.ResponseWriter, _ *http.Request) { +func (svr *Service) apiReload(w http.ResponseWriter, r *http.Request) { res := GeneralResponse{Code: 200} + strictConfigMode := false + strictStr := r.URL.Query().Get("strictConfig") + if strictStr != "" { + strictConfigMode, _ = strconv.ParseBool(strictStr) + } log.Info("api request [/api/reload]") defer func() { @@ -57,7 +62,7 @@ func (svr *Service) apiReload(w http.ResponseWriter, _ *http.Request) { } }() - cliCfg, pxyCfgs, visitorCfgs, _, err := config.LoadClientConfig(svr.cfgFile, svr.strictConfig) + cliCfg, pxyCfgs, visitorCfgs, _, err := config.LoadClientConfig(svr.cfgFile, strictConfigMode) if err != nil { res.Code = 400 res.Msg = err.Error() diff --git a/client/service.go b/client/service.go index 0a25ae0..66a642c 100644 --- a/client/service.go +++ b/client/service.go @@ -70,9 +70,6 @@ type Service struct { // string if no configuration file was used. cfgFile string - // Whether strict configuration parsing had been requested. - strictConfig bool - // service context ctx context.Context // call cancel to stop service @@ -85,16 +82,14 @@ func NewService( pxyCfgs []v1.ProxyConfigurer, visitorCfgs []v1.VisitorConfigurer, cfgFile string, - strictConfig bool, ) *Service { return &Service{ - authSetter: auth.NewAuthSetter(cfg.Auth), - cfg: cfg, - cfgFile: cfgFile, - strictConfig: strictConfig, - pxyCfgs: pxyCfgs, - visitorCfgs: visitorCfgs, - ctx: context.Background(), + authSetter: auth.NewAuthSetter(cfg.Auth), + cfg: cfg, + cfgFile: cfgFile, + pxyCfgs: pxyCfgs, + visitorCfgs: visitorCfgs, + ctx: context.Background(), } } diff --git a/cmd/frpc/sub/admin.go b/cmd/frpc/sub/admin.go index d98b4d3..5d478d4 100644 --- a/cmd/frpc/sub/admin.go +++ b/cmd/frpc/sub/admin.go @@ -52,7 +52,7 @@ func NewAdminCommand(name, short string, handler func(*v1.ClientCommonConfig) er Use: name, Short: short, Run: func(cmd *cobra.Command, args []string) { - cfg, _, _, _, err := config.LoadClientConfig(cfgFile, strictConfig) + cfg, _, _, _, err := config.LoadClientConfig(cfgFile, strictConfigMode) if err != nil { fmt.Println(err) os.Exit(1) @@ -73,7 +73,7 @@ func NewAdminCommand(name, short string, handler func(*v1.ClientCommonConfig) er func ReloadHandler(clientCfg *v1.ClientCommonConfig) error { client := clientsdk.New(clientCfg.WebServer.Addr, clientCfg.WebServer.Port) client.SetAuth(clientCfg.WebServer.User, clientCfg.WebServer.Password) - if err := client.Reload(); err != nil { + if err := client.Reload(strictConfigMode); err != nil { return err } fmt.Println("reload success") diff --git a/cmd/frpc/sub/nathole.go b/cmd/frpc/sub/nathole.go index eafea27..56fcf67 100644 --- a/cmd/frpc/sub/nathole.go +++ b/cmd/frpc/sub/nathole.go @@ -48,7 +48,7 @@ var natholeDiscoveryCmd = &cobra.Command{ Short: "Discover nathole information from stun server", RunE: func(cmd *cobra.Command, args []string) error { // ignore error here, because we can use command line pameters - cfg, _, _, _, err := config.LoadClientConfig(cfgFile, strictConfig) + cfg, _, _, _, err := config.LoadClientConfig(cfgFile, strictConfigMode) if err != nil { cfg = &v1.ClientCommonConfig{} } diff --git a/cmd/frpc/sub/proxy.go b/cmd/frpc/sub/proxy.go index 41c20bc..7ae8d35 100644 --- a/cmd/frpc/sub/proxy.go +++ b/cmd/frpc/sub/proxy.go @@ -84,7 +84,7 @@ func NewProxyCommand(name string, c v1.ProxyConfigurer, clientCfg *v1.ClientComm fmt.Println(err) os.Exit(1) } - err := startService(clientCfg, []v1.ProxyConfigurer{c}, nil, "", strictConfig) + err := startService(clientCfg, []v1.ProxyConfigurer{c}, nil, "") if err != nil { fmt.Println(err) os.Exit(1) @@ -110,7 +110,7 @@ func NewVisitorCommand(name string, c v1.VisitorConfigurer, clientCfg *v1.Client fmt.Println(err) os.Exit(1) } - err := startService(clientCfg, nil, []v1.VisitorConfigurer{c}, "", strictConfig) + err := startService(clientCfg, nil, []v1.VisitorConfigurer{c}, "") if err != nil { fmt.Println(err) os.Exit(1) diff --git a/cmd/frpc/sub/root.go b/cmd/frpc/sub/root.go index 855c7ab..c4a5acb 100644 --- a/cmd/frpc/sub/root.go +++ b/cmd/frpc/sub/root.go @@ -36,17 +36,17 @@ import ( ) var ( - cfgFile string - cfgDir string - showVersion bool - strictConfig bool + cfgFile string + cfgDir string + showVersion bool + strictConfigMode bool ) func init() { rootCmd.PersistentFlags().StringVarP(&cfgFile, "config", "c", "./frpc.ini", "config file of frpc") rootCmd.PersistentFlags().StringVarP(&cfgDir, "config_dir", "", "", "config directory, run one frpc service for each file in config directory") rootCmd.PersistentFlags().BoolVarP(&showVersion, "version", "v", false, "version of frpc") - rootCmd.PersistentFlags().BoolVarP(&strictConfig, "strict_config", "", false, "strict config parsing mode") + rootCmd.PersistentFlags().BoolVarP(&strictConfigMode, "strict_config", "", false, "strict config parsing mode, unknown fields will cause an error") } var rootCmd = &cobra.Command{ @@ -110,7 +110,7 @@ func handleTermSignal(svr *client.Service) { } func runClient(cfgFilePath string) error { - cfg, pxyCfgs, visitorCfgs, isLegacyFormat, err := config.LoadClientConfig(cfgFilePath, strictConfig) + cfg, pxyCfgs, visitorCfgs, isLegacyFormat, err := config.LoadClientConfig(cfgFilePath, strictConfigMode) if err != nil { return err } @@ -122,14 +122,11 @@ func runClient(cfgFilePath string) error { warning, err := validation.ValidateAllClientConfig(cfg, pxyCfgs, visitorCfgs) if warning != nil { fmt.Printf("WARNING: %v\n", warning) - if strictConfig { - return fmt.Errorf("warning: %v", warning) - } } if err != nil { return err } - return startService(cfg, pxyCfgs, visitorCfgs, cfgFilePath, strictConfig) + return startService(cfg, pxyCfgs, visitorCfgs, cfgFilePath) } func startService( @@ -137,7 +134,6 @@ func startService( pxyCfgs []v1.ProxyConfigurer, visitorCfgs []v1.VisitorConfigurer, cfgFile string, - strictConfig bool, ) error { log.InitLog(cfg.Log.To, cfg.Log.Level, cfg.Log.MaxDays, cfg.Log.DisablePrintColor) @@ -145,13 +141,12 @@ func startService( log.Info("start frpc service for config file [%s]", cfgFile) defer log.Info("frpc service for config file [%s] stopped", cfgFile) } - svr := client.NewService(cfg, pxyCfgs, visitorCfgs, cfgFile, strictConfig) + svr := client.NewService(cfg, pxyCfgs, visitorCfgs, cfgFile) shouldGracefulClose := cfg.Transport.Protocol == "kcp" || cfg.Transport.Protocol == "quic" // Capture the exit signal if we use kcp or quic. if shouldGracefulClose { go handleTermSignal(svr) } - return svr.Run(context.Background()) } diff --git a/cmd/frpc/sub/verify.go b/cmd/frpc/sub/verify.go index 0e6adca..1b6ac5a 100644 --- a/cmd/frpc/sub/verify.go +++ b/cmd/frpc/sub/verify.go @@ -37,7 +37,7 @@ var verifyCmd = &cobra.Command{ return nil } - cliCfg, pxyCfgs, visitorCfgs, _, err := config.LoadClientConfig(cfgFile, strictConfig) + cliCfg, pxyCfgs, visitorCfgs, _, err := config.LoadClientConfig(cfgFile, strictConfigMode) if err != nil { fmt.Println(err) os.Exit(1) diff --git a/cmd/frps/root.go b/cmd/frps/root.go index adb8852..1fa57d9 100644 --- a/cmd/frps/root.go +++ b/cmd/frps/root.go @@ -30,9 +30,9 @@ import ( ) var ( - cfgFile string - showVersion bool - strictConfig bool + cfgFile string + showVersion bool + strictConfigMode bool serverCfg v1.ServerConfig ) @@ -40,7 +40,7 @@ var ( func init() { rootCmd.PersistentFlags().StringVarP(&cfgFile, "config", "c", "", "config file of frps") rootCmd.PersistentFlags().BoolVarP(&showVersion, "version", "v", false, "version of frps") - rootCmd.PersistentFlags().BoolVarP(&strictConfig, "strict_config", "", false, "strict config parsing mode") + rootCmd.PersistentFlags().BoolVarP(&strictConfigMode, "strict_config", "", false, "strict config parsing mode, unknown fileds will cause error") RegisterServerConfigFlags(rootCmd, &serverCfg) } @@ -60,7 +60,7 @@ var rootCmd = &cobra.Command{ err error ) if cfgFile != "" { - svrCfg, isLegacyFormat, err = config.LoadServerConfig(cfgFile, strictConfig) + svrCfg, isLegacyFormat, err = config.LoadServerConfig(cfgFile, strictConfigMode) if err != nil { fmt.Println(err) os.Exit(1) diff --git a/cmd/frps/verify.go b/cmd/frps/verify.go index 838ac7b..33ad3f6 100644 --- a/cmd/frps/verify.go +++ b/cmd/frps/verify.go @@ -36,7 +36,7 @@ var verifyCmd = &cobra.Command{ fmt.Println("frps: the configuration file is not specified") return nil } - svrCfg, _, err := config.LoadServerConfig(cfgFile, strictConfig) + svrCfg, _, err := config.LoadServerConfig(cfgFile, strictConfigMode) if err != nil { fmt.Println(err) os.Exit(1) diff --git a/pkg/config/load.go b/pkg/config/load.go index a4013c3..41d1a23 100644 --- a/pkg/config/load.go +++ b/pkg/config/load.go @@ -27,7 +27,7 @@ import ( "github.com/samber/lo" "gopkg.in/ini.v1" "k8s.io/apimachinery/pkg/util/sets" - yaml "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/apimachinery/pkg/util/yaml" "github.com/fatedier/frp/pkg/config/legacy" v1 "github.com/fatedier/frp/pkg/config/v1" @@ -113,7 +113,6 @@ func LoadConfigureFromFile(path string, c any, strict bool) error { func LoadConfigure(b []byte, c any, strict bool) error { var tomlObj interface{} // Try to unmarshal as TOML first; swallow errors from that (assume it's not valid TOML). - // TODO: caller should probably be able to specify the format, so we don't need to swallow errors. if err := toml.Unmarshal(b, &tomlObj); err == nil { b, err = json.Marshal(&tomlObj) if err != nil { diff --git a/pkg/config/load_test.go b/pkg/config/load_test.go index 876d53e..9bf7dbb 100644 --- a/pkg/config/load_test.go +++ b/pkg/config/load_test.go @@ -15,6 +15,7 @@ package config import ( + "fmt" "strings" "testing" @@ -56,36 +57,57 @@ const jsonServerContent = ` ` func TestLoadServerConfig(t *testing.T) { - for _, content := range []string{tomlServerContent, yamlServerContent, jsonServerContent} { - svrCfg := v1.ServerConfig{} - err := LoadConfigure([]byte(content), &svrCfg, true) - require := require.New(t) - require.NoError(err) - require.EqualValues("127.0.0.1", svrCfg.BindAddr) - require.EqualValues(7000, svrCfg.KCPBindPort) - require.EqualValues(7001, svrCfg.QUICBindPort) - require.EqualValues(7005, svrCfg.TCPMuxHTTPConnectPort) - require.EqualValues("/abc.html", svrCfg.Custom404Page) - require.EqualValues(10, svrCfg.Transport.TCPKeepAlive) + tests := []struct { + name string + content string + }{ + {"toml", tomlServerContent}, + {"yaml", yamlServerContent}, + {"json", jsonServerContent}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + require := require.New(t) + svrCfg := v1.ServerConfig{} + err := LoadConfigure([]byte(test.content), &svrCfg, true) + require.NoError(err) + require.EqualValues("127.0.0.1", svrCfg.BindAddr) + require.EqualValues(7000, svrCfg.KCPBindPort) + require.EqualValues(7001, svrCfg.QUICBindPort) + require.EqualValues(7005, svrCfg.TCPMuxHTTPConnectPort) + require.EqualValues("/abc.html", svrCfg.Custom404Page) + require.EqualValues(10, svrCfg.Transport.TCPKeepAlive) + }) } } // Test that loading in strict mode fails when the config is invalid. -func TestLoadServerConfigErrorMode(t *testing.T) { - for strict := range []bool{false, true} { - for _, content := range []string{tomlServerContent, yamlServerContent, jsonServerContent} { - // Break the content with an innocent typo - brokenContent := strings.Replace(content, "bindAddr", "bindAdur", 1) - svrCfg := v1.ServerConfig{} - err := LoadConfigure([]byte(brokenContent), &svrCfg, strict == 1) - require := require.New(t) - if strict == 1 { - require.ErrorContains(err, "bindAdur") - } else { - require.NoError(err) - // BindAddr didn't get parsed because of the typo. - require.EqualValues("", svrCfg.BindAddr) - } +func TestLoadServerConfigStrictMode(t *testing.T) { + tests := []struct { + name string + content string + }{ + {"toml", tomlServerContent}, + {"yaml", yamlServerContent}, + {"json", jsonServerContent}, + } + + for _, strict := range []bool{false, true} { + for _, test := range tests { + t.Run(fmt.Sprintf("%s-strict-%t", test.name, strict), func(t *testing.T) { + require := require.New(t) + // Break the content with an innocent typo + brokenContent := strings.Replace(test.content, "bindAddr", "bindAdur", 1) + svrCfg := v1.ServerConfig{} + err := LoadConfigure([]byte(brokenContent), &svrCfg, strict) + if strict { + require.ErrorContains(err, "bindAdur") + } else { + require.NoError(err) + // BindAddr didn't get parsed because of the typo. + require.EqualValues("", svrCfg.BindAddr) + } + }) } } } diff --git a/pkg/sdk/client/client.go b/pkg/sdk/client/client.go index c965790..395063e 100644 --- a/pkg/sdk/client/client.go +++ b/pkg/sdk/client/client.go @@ -6,6 +6,7 @@ import ( "io" "net" "net/http" + "net/url" "strconv" "strings" @@ -69,8 +70,16 @@ func (c *Client) GetAllProxyStatus() (client.StatusResp, error) { return allStatus, nil } -func (c *Client) Reload() error { - req, err := http.NewRequest("GET", "http://"+c.address+"/api/reload", nil) +func (c *Client) Reload(strictMode bool) error { + v := url.Values{} + if strictMode { + v.Set("strictConfig", "true") + } + queryStr := "" + if len(v) > 0 { + queryStr = "?" + v.Encode() + } + req, err := http.NewRequest("GET", "http://"+c.address+"/api/reload"+queryStr, nil) if err != nil { return err } diff --git a/test/e2e/legacy/basic/client.go b/test/e2e/legacy/basic/client.go index da23db9..d4862e5 100644 --- a/test/e2e/legacy/basic/client.go +++ b/test/e2e/legacy/basic/client.go @@ -69,7 +69,7 @@ var _ = ginkgo.Describe("[Feature: ClientManage]", func() { err = client.UpdateConfig(newClientConf) framework.ExpectNoError(err) - err = client.Reload() + err = client.Reload(true) framework.ExpectNoError(err) time.Sleep(time.Second) diff --git a/test/e2e/v1/basic/client.go b/test/e2e/v1/basic/client.go index 25b9942..b0b258d 100644 --- a/test/e2e/v1/basic/client.go +++ b/test/e2e/v1/basic/client.go @@ -72,7 +72,7 @@ var _ = ginkgo.Describe("[Feature: ClientManage]", func() { err = client.UpdateConfig(newClientConf) framework.ExpectNoError(err) - err = client.Reload() + err = client.Reload(true) framework.ExpectNoError(err) time.Sleep(time.Second)