From 3c94036c6f55621d5aad2689df437b76a2523449 Mon Sep 17 00:00:00 2001 From: Sameer Kumar Subudhi Date: Tue, 23 Jan 2024 08:30:50 +0530 Subject: [PATCH] :ok_hand: Apply review feedback --- cmd/main.go | 2 +- pkg/api/server.go | 14 +- pkg/config/config.go | 66 ++++--- pkg/config/config_test.go | 381 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 425 insertions(+), 38 deletions(-) create mode 100644 pkg/config/config_test.go diff --git a/cmd/main.go b/cmd/main.go index 4d1f9bd..0a1f530 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -19,7 +19,7 @@ import ( "github.com/spf13/viper" ) -var apiServer *api.HTTPServerWrapper +var apiServer *api.HTTPServer func main() { ctx, cancel := context.WithCancel(context.Background()) diff --git a/pkg/api/server.go b/pkg/api/server.go index d3095af..99ce7f3 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -15,8 +15,8 @@ import ( "github.com/gin-gonic/gin" ) -// HTTPServerWrapper embeds the http.Server along with the various other properties. -type HTTPServerWrapper struct { +// HTTPServer embeds the http.Server along with the various other properties. +type HTTPServer struct { server *http.Server ctx context.Context logger log.Logger @@ -25,7 +25,7 @@ type HTTPServerWrapper struct { } // Start starts the HTTP API server. -func (w *HTTPServerWrapper) Start() { +func (w *HTTPServer) Start() { defer w.wg.Done() w.logger.Infof("Starting the HTTP server on %s.", w.server.Addr) @@ -36,7 +36,7 @@ func (w *HTTPServerWrapper) Start() { } // Stop gracefully shuts down the HTTP API server. -func (w *HTTPServerWrapper) Stop() error { +func (w *HTTPServer) Stop() error { err := w.server.Shutdown(w.ctx) if err == nil { w.logger.Infof("Successfully stopped the HTTP server.") @@ -56,7 +56,7 @@ func getGinModeFromSysLogLevel(sysLogLevel string) string { } // NewHTTPServer creates a router instance and sets up the necessary routes/handlers. -func NewHTTPServer(ctx context.Context, logger log.Logger, wg *sync.WaitGroup, config *config.Config, errorChan chan error) *HTTPServerWrapper { +func NewHTTPServer(ctx context.Context, logger log.Logger, wg *sync.WaitGroup, config *config.Config, errorChan chan error) *HTTPServer { gin.SetMode(getGinModeFromSysLogLevel(config.System.LogLevel)) router := gin.Default() @@ -78,11 +78,11 @@ func NewHTTPServer(ctx context.Context, logger log.Logger, wg *sync.WaitGroup, c port := config.Api.Server.Port addr := fmt.Sprintf("%s:%d", host, port) - server := &HTTPServerWrapper{ + server := &HTTPServer{ &http.Server{ Addr: addr, Handler: router, - ReadHeaderTimeout: 10 * time.Second, // TODO: Check appropriate value + ReadHeaderTimeout: 10 * time.Second, }, ctx, logger, diff --git a/pkg/config/config.go b/pkg/config/config.go index 0bbb2e1..beac08c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -25,84 +25,90 @@ type System struct { } type Api struct { - Server struct { - Host string `mapstructure:"host"` - Port uint `mapstructure:"port"` - } `mapstructure:"server"` + Server *Server `mapstructure:"server"` BasePath string `mapstructure:"base_path"` RegisterVersions []string `mapstructure:"register_versions"` } +type Server struct { + Host string `mapstructure:"host"` + Port uint `mapstructure:"port"` +} + type FaultDetector struct { } +func formatError(validationErrors error) error { + if validationErrors == nil { + return nil + } + + // Beautify the error message + validationErrorSplits := strings.Split(validationErrors.Error(), ";") + formattedErrorStr := strings.Join(validationErrorSplits, "\n\t-") + + return fmt.Errorf("fix the following %d config validation fail(s) to continue:\n\t- %s", len(validationErrorSplits), formattedErrorStr) +} + func (c *Config) Validate() error { var validationErrors error - sysConfigError := validateSystemConfig(c) - apiConfigError := validateApiConfig(c) - fdConfigError := validateFaultDetectorConfig(c) + sysConfigError := c.System.Validate() + apiConfigError := c.Api.Validate() + fdConfigError := c.FaultDetector.Validate() validationErrors = multierr.Combine(sysConfigError, apiConfigError, fdConfigError) - if validationErrors != nil { - // Beautify the error message - validationErrorSplits := strings.Split(validationErrors.Error(), ";") - formattedErrorStr := strings.Join(validationErrorSplits, "\n\t- ") - - return fmt.Errorf("fix the following %d config validation fail(s) to continue:\n\t - %s", len(validationErrorSplits), formattedErrorStr) - } - - return validationErrors + return formatError(validationErrors) } -func validateSystemConfig(c *Config) error { +func (c *System) Validate() error { var validationErrors error allowedLogLevels := []string{log.LevelTrace, log.LevelDebug, log.LevelInfo, log.LevelWarn, log.LevelError, log.LevelFatal} - if !utils.Contains(allowedLogLevels, c.System.LogLevel) { - validationErrors = multierr.Append(validationErrors, fmt.Errorf("system.log_level expected one of %s, received: '%s'", allowedLogLevels, c.System.LogLevel)) + if !utils.Contains(allowedLogLevels, c.LogLevel) { + validationErrors = multierr.Append(validationErrors, fmt.Errorf("system.log_level expected one of %s, received: '%s'", allowedLogLevels, c.LogLevel)) } return validationErrors } -func validateApiConfig(c *Config) error { +func (c *Api) Validate() error { var validationErrors error - validationErrors = multierr.Append(validationErrors, validateApiServerConfig(c)) + validationErrors = multierr.Append(validationErrors, c.Server.Validate()) - basePath := c.Api.BasePath + basePath := c.BasePath basePathRegex := `^/?api$` basePathMatched, _ := regexp.MatchString(basePathRegex, basePath) if !basePathMatched { - validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.base_path expected to match regex: `%v`, received: '%s'", basePathRegex, basePath)) + validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.base_path expected to match regex: `%s`, received: '%s'", basePathRegex, basePath)) } - registerVersions := c.Api.RegisterVersions + registerVersions := c.RegisterVersions registerVersionRegex := `^v[1-9]\d*$` registerVersionRegexCompiled, _ := regexp.Compile(registerVersionRegex) for _, version := range registerVersions { registerVersionMatched := registerVersionRegexCompiled.MatchString(version) if !registerVersionMatched { - validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.register_versions entry expected to match regex: `%v`, received: '%s'", registerVersionRegex, version)) + validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", registerVersionRegex, version)) } } return validationErrors } -func validateApiServerConfig(c *Config) error { +func (c *Server) Validate() error { var validationErrors error - host := c.Api.Server.Host + host := c.Host hostRegex := `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$` hostMatched, _ := regexp.MatchString(hostRegex, host) if !hostMatched { - validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.server.host expected to match regex: `%v`, received: '%s'", hostRegex, host)) + validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.server.host expected to match regex: `%s`, received: '%s'", hostRegex, host)) } - port := c.Api.Server.Port + port := c.Port minPortNum := uint(0) maxPortNum := uint(math.Pow(2, 16) - 1) if port < minPortNum || port > maxPortNum { @@ -112,6 +118,6 @@ func validateApiServerConfig(c *Config) error { return validationErrors } -func validateFaultDetectorConfig(c *Config) error { +func (c *FaultDetector) Validate() error { return nil } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go new file mode 100644 index 0000000..078bd45 --- /dev/null +++ b/pkg/config/config_test.go @@ -0,0 +1,381 @@ +package config + +import ( + "fmt" + "math" + "testing" + + "github.com/LiskHQ/op-fault-detector/pkg/log" + "github.com/magiconair/properties/assert" + "go.uber.org/multierr" +) + +func TestValidate_System(t *testing.T) { + testCases := []struct { + name string + config *System + want error + }{ + { + name: "should return nil when correct system config specified", + config: &System{ + "info", + }, + want: nil, + }, + { + name: "should return error when incorrect log level specified in system config", + config: &System{ + "tracer", + }, + want: fmt.Errorf( + "system.log_level expected one of %s, received: '%s'", + []string{log.LevelTrace, log.LevelDebug, log.LevelInfo, log.LevelWarn, log.LevelError, log.LevelFatal}, + "tracer", + ), + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := tc.config.Validate() + assert.Equal(t, got, tc.want) + }) + } +} + +func TestValidate_Server(t *testing.T) { + testCases := []struct { + name string + config *Server + want error + }{ + { + name: "should return nil when correct server config specified", + config: &Server{ + "0.0.0.0", + 8080, + }, + want: nil, + }, + { + name: "should return error when incorrect host specified in system config", + config: &Server{ + "127.0.0.256", + 8080, + }, + want: fmt.Errorf( + "api.server.host expected to match regex: `%v`, received: '%v'", + `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`, + "127.0.0.256", + ), + }, + { + name: "should return error when incorrect port specified in system config", + config: &Server{ + "127.0.0.1", + 99999, + }, + want: fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 99999), + }, + { + name: "should return error when incorrect host and port specified in system config", + config: &Server{ + "127.0.0.256", + 99999, + }, + want: multierr.Append( + fmt.Errorf( + "api.server.host expected to match regex: `%v`, received: '%v'", + `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`, + "127.0.0.256", + ), + fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 99999), + ), + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := tc.config.Validate() + assert.Equal(t, got, tc.want) + }) + } +} + +func TestValidate_Api(t *testing.T) { + testCases := []struct { + name string + config *Api + want error + }{ + { + name: "should return nil when correct api config specified", + config: &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "/api", + []string{"v1"}, + }, + want: nil, + }, + { + name: "should return nil when correct base_path specified without leading slash in api config", + config: &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "api", + []string{"v1"}, + }, + want: nil, + }, + { + name: "should return error when incorrect base_path specified in api config", + config: &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "apis", + []string{"v1"}, + }, + want: fmt.Errorf("api.base_path expected to match regex: `%s`, received: '%s'", `^/?api$`, "apis"), + }, + { + name: "should return error when incorrect register_versions specified in api config", + config: &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "/api", + []string{"b1"}, + }, + want: fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", `^v[1-9]\d*$`, "b1"), + }, + { + name: "should return error when one of register_versions specified is incorrect in api config", + config: &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "api", + []string{"v1", "v2", "c1"}, + }, + want: fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", `^v[1-9]\d*$`, "c1"), + }, + { + name: "should return error when server host specified is incorrect in api config", + config: &Api{ + &Server{ + "256.255.0.8", + 8080, + }, + "api", + []string{"v1"}, + }, + want: fmt.Errorf( + "api.server.host expected to match regex: `%v`, received: '%v'", + `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`, + "256.255.0.8", + ), + }, + { + name: "should return error when server port specified is incorrect in api config", + config: &Api{ + &Server{ + "0.0.0.0", + 556677, + }, + "api", + []string{"v1"}, + }, + want: fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 556677), + }, + { + name: "should return error when multiple incorrect parameters specified in system config", + config: &Api{ + &Server{ + "127.0.0.256", + 99999, + }, + "api/", + []string{"v1", "version1"}, + }, + want: multierr.Combine( + fmt.Errorf( + "api.server.host expected to match regex: `%v`, received: '%v'", + `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`, + "127.0.0.256", + ), + fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 99999), + fmt.Errorf("api.base_path expected to match regex: `%s`, received: '%s'", `^/?api$`, "api/"), + fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", `^v[1-9]\d*$`, "version1"), + ), + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := tc.config.Validate() + assert.Equal(t, got, tc.want) + }) + } +} + +// TODO: Update test table when implementing the fault detector +func TestValidate_FaultDetector(t *testing.T) { + testCases := []struct { + name string + config *FaultDetector + want error + }{} + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := tc.config.Validate() + assert.Equal(t, got, tc.want) + }) + } +} + +func TestFormatError(t *testing.T) { + testCases := []struct { + name string + validationErrors error + want error + }{ + { + name: "should return nil when nil error is supplied", + validationErrors: nil, + want: nil, + }, + { + name: "should return properly formatted error when a single error is supplied", + validationErrors: fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 543210), + want: fmt.Errorf( + "fix the following 1 config validation fail(s) to continue:\n\t- api.server.port expected in range: %d - %d, received: %d", + uint(0), + uint(math.Pow(2, 16)-1), + 543210, + ), + }, + + { + name: "should return properly formatted error when multiple combined errors are supplied", + validationErrors: multierr.Combine( + fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 23232323), + fmt.Errorf("api.base_path expected to match regex: `%s`, received: '%s'", `^/?api$`, "api/"), + fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", `^v[1-9]\d*$`, "version1"), + ), + want: fmt.Errorf( + "fix the following 3 config validation fail(s) to continue:\n\t- api.server.port expected in range: %d - %d, received: %d\n\t- api.base_path expected to match regex: `%s`, received: '%s'\n\t- api.register_versions entry expected to match regex: `%s`, received: '%s'", + uint(0), uint(math.Pow(2, 16)-1), 23232323, + `^/?api$`, "api/", + `^v[1-9]\d*$`, "version1", + ), + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := formatError(tc.validationErrors) + assert.Equal(t, got, tc.want) + }) + } +} + +// TODO: Adjust the test table below after implementing the fault detector +func TestValidate_Config(t *testing.T) { + testCases := []struct { + name string + config *Config + want error + }{ + { + name: "should return nil when correct config specified", + config: &Config{ + &System{ + "info", + }, + &Api{ + &Server{ + "0.0.0.0", + 8080, + }, + "/api", + []string{"v1"}, + }, + &FaultDetector{}, + }, + want: nil, + }, + { + name: "should return error when incorrect port number specified in config", + config: &Config{ + &System{ + "info", + }, + &Api{ + &Server{ + "0.0.0.0", + 543210, + }, + "/api", + []string{"v1"}, + }, + &FaultDetector{}, + }, + want: formatError( + fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 543210), + ), + }, + { + name: "should return error when incorrect port number specified in config", + config: &Config{ + &System{ + "info", + }, + &Api{ + &Server{ + "127.0.0.256", + 99999, + }, + "api/", + []string{"v1", "version1"}, + }, + &FaultDetector{}, + }, + want: formatError( + multierr.Combine( + fmt.Errorf( + "api.server.host expected to match regex: `%v`, received: '%v'", + `^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`, + "127.0.0.256", + ), + fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", uint(0), uint(math.Pow(2, 16)-1), 99999), + fmt.Errorf("api.base_path expected to match regex: `%s`, received: '%s'", `^/?api$`, "api/"), + fmt.Errorf("api.register_versions entry expected to match regex: `%s`, received: '%s'", `^v[1-9]\d*$`, "version1"), + ), + ), + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := tc.config.Validate() + assert.Equal(t, got, tc.want) + }) + } +}