update for strict config (#3779)

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.

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()

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(),
 	}
 }
 

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")

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{}
 		}

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)

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())
 }

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)

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)

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)

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 {

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)
+				}
+			})
 		}
 	}
 }

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
 	}

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)
 

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)