From f65dfc5f8588d55e77e32910e3a431c7b0c5bb56 Mon Sep 17 00:00:00 2001 From: Sameer Kumar Subudhi Date: Mon, 22 Jan 2024 21:12:09 +0530 Subject: [PATCH] :ok_hand: Apply review feedback --- .vscode/settings.json | 7 -- cmd/main.go | 92 +++++++++++++++++--- config.yaml | 16 ++-- go.mod | 2 +- pkg/api/middlewares/authorization.go | 18 ---- pkg/api/routes/registration.go | 5 +- pkg/api/server.go | 84 ++++++++++++------ pkg/api/server_test.go | 56 ++++++++++++ pkg/config/config.go | 122 ++++++++++++++++++++------- pkg/log/logger.go | 9 ++ pkg/utils/fs.go | 17 ---- pkg/utils/slice.go | 11 +++ pkg/utils/slice_test.go | 67 +++++++++++++++ 13 files changed, 390 insertions(+), 116 deletions(-) delete mode 100644 pkg/api/middlewares/authorization.go create mode 100644 pkg/api/server_test.go delete mode 100644 pkg/utils/fs.go create mode 100644 pkg/utils/slice.go create mode 100644 pkg/utils/slice_test.go diff --git a/.vscode/settings.json b/.vscode/settings.json index d0d78b0..c3d800f 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -3,12 +3,5 @@ "go.lintTool": "golangci-lint", "go.lintFlags": [ "--fast" - ], - "cSpell.words": [ - "Debugf", - "gonic", - "Infof", - "mapstructure", - "Warningf" ] } diff --git a/cmd/main.go b/cmd/main.go index 70bf063..4d1f9bd 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -3,17 +3,23 @@ package main import ( "context" + "errors" "flag" + "fmt" + "os" + "os/signal" "path" + "strings" "sync" + "syscall" "github.com/LiskHQ/op-fault-detector/pkg/api" "github.com/LiskHQ/op-fault-detector/pkg/config" "github.com/LiskHQ/op-fault-detector/pkg/log" - "github.com/LiskHQ/op-fault-detector/pkg/utils" + "github.com/spf13/viper" ) -var wg = sync.WaitGroup{} +var apiServer *api.HTTPServerWrapper func main() { ctx, cancel := context.WithCancel(context.Background()) @@ -24,24 +30,84 @@ func main() { panic(err) } - defaultConfigFilename := "config.yaml" - defaultConfigDir := path.Join(utils.GetCurrentDirectory(0), "..") - configFilepath := flag.String("config", path.Join(defaultConfigDir, defaultConfigFilename), "Path to the config file") + configFilepath := flag.String("config", "./config.yaml", "Path to the config file") flag.Parse() - config, err := config.GetAppConfig(ctx, logger, *configFilepath) + config, err := getAppConfig(logger, *configFilepath) if err != nil { panic(err) } + wg := sync.WaitGroup{} + + doneChan := make(chan struct{}) + signalChan := make(chan os.Signal, 1) + signal.Notify(signalChan, os.Interrupt, syscall.SIGTERM) + + // Start Fault Detector + + // Start API Server + serverChan := make(chan error, 1) + apiServer = api.NewHTTPServer(ctx, logger, &wg, config, serverChan) wg.Add(1) + go apiServer.Start() + go func() { - err := api.StartServer(ctx, &wg, logger, config) - if err != nil { - logger.Errorf("Failed to start the server due to: %v", err.Error()) - panic(err) - } + wg.Wait() + close(doneChan) }() - // Wait for all the goroutines to finish - wg.Wait() + for { + select { + case <-doneChan: + performCleanup(logger) + return + + case <-signalChan: + performCleanup(logger) + return + + case err := <-serverChan: + logger.Errorf("Received error of %v", err) + return + } + } +} + +// getAppConfig is the function that takes in the absolute path to the config file, parses the content and returns it. +func getAppConfig(logger log.Logger, configFilepath string) (*config.Config, error) { + configDir := path.Dir(configFilepath) + configFilenameWithExt := path.Base(configFilepath) + + splits := strings.FieldsFunc(configFilenameWithExt, func(r rune) bool { return r == '.' }) + configType := splits[len(splits)-1] // Config file extension + + viper.AddConfigPath(".") + viper.AddConfigPath("..") + viper.AddConfigPath("$HOME/.op-fault-detector") + viper.AddConfigPath(configDir) + viper.SetConfigName(configFilenameWithExt) + viper.SetConfigType(configType) + err := viper.ReadInConfig() + if err != nil { + return nil, fmt.Errorf("failed to load the config from disk: %w", err) + } + + var config config.Config + err = viper.Unmarshal(&config) + if err != nil { + return nil, errors.New("failed to unmarshal config. Verify the 'Config' struct definition in 'pkg/config/config.go'") + } + + if err := config.Validate(); err != nil { + return nil, err + } + + return &config, nil +} + +func performCleanup(logger log.Logger) { + err := apiServer.Stop() + if err != nil { + logger.Error("Server shutdown not successful: %w", err) + } } diff --git a/config.yaml b/config.yaml index 9965dd8..9dd18c2 100644 --- a/config.yaml +++ b/config.yaml @@ -1,11 +1,15 @@ -# Server configurations -server: - host: "127.0.0.1" - port: 8080 - gin_mode: "release" # Set to "debug" for development +# General system configurations +system: + log_level: "info" -# API configurations +# API related configurations api: + server: + host: "127.0.0.1" + port: 8080 base_path: "/api" register_versions: - v1 + +# Faultdetector configurations +fault_detector: diff --git a/go.mod b/go.mod index cbc6853..57218de 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/gin-gonic/gin v1.9.1 github.com/magiconair/properties v1.8.7 github.com/spf13/viper v1.18.2 + go.uber.org/multierr v1.10.0 go.uber.org/zap v1.26.0 ) @@ -57,7 +58,6 @@ require ( github.com/tklauser/numcpus v0.6.1 // indirect github.com/twitchyliquid64/golang-asm v0.15.1 // indirect github.com/ugorji/go/codec v1.2.12 // indirect - go.uber.org/multierr v1.10.0 // indirect golang.org/x/arch v0.7.0 // indirect golang.org/x/crypto v0.18.0 // indirect golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa // indirect diff --git a/pkg/api/middlewares/authorization.go b/pkg/api/middlewares/authorization.go deleted file mode 100644 index f9d4d6c..0000000 --- a/pkg/api/middlewares/authorization.go +++ /dev/null @@ -1,18 +0,0 @@ -package middlewares - -import "github.com/gin-gonic/gin" - -// Separate Authorization checks can be introduced per path or domain -// Ref: https://gin-gonic.com/zh-tw/docs/examples/using-middleware/ - -func Authorize() gin.HandlerFunc { - return func(c *gin.Context) { - // Run authorization checks - // Add logic to be executed BEFORE the request is processed - - // Do not forget to include Next inside all the middlewares to ensure execution of the pending handlers in the chain inside the calling handler - c.Next() - - // Add logic to be executed AFTER the request is processed - } -} diff --git a/pkg/api/routes/registration.go b/pkg/api/routes/registration.go index 981cb0d..ac590bc 100644 --- a/pkg/api/routes/registration.go +++ b/pkg/api/routes/registration.go @@ -7,16 +7,19 @@ import ( "github.com/gin-gonic/gin" ) +// RegisterHandlers is responsible to register all handlers for routes without any base path. func RegisterHandlers(logger log.Logger, router *gin.Engine) { router.GET("/ping", handlers.GetPing) } +// RegisterHandlersByGroup is responsible to register all the handlers for routes that are prefixed under a specified base path as a routerGroup. func RegisterHandlersByGroup(logger log.Logger, routerGroup *gin.RouterGroup, versions []string) { for _, version := range versions { RegisterHandlersForVersion(logger, routerGroup, version) } } +// RegisterHandlersForVersion is responsible to register API version specific route handlers. func RegisterHandlersForVersion(logger log.Logger, routerGroup *gin.RouterGroup, version string) { group := routerGroup.Group(version) @@ -25,6 +28,6 @@ func RegisterHandlersForVersion(logger log.Logger, routerGroup *gin.RouterGroup, group.GET("/status", v1.GetStatus) default: - logger.Warningf("No routes and handlers defined for version %v. Please verify the API config.", version) + logger.Warningf("No routes and handlers defined for version %s. Please verify the API config.", version) } } diff --git a/pkg/api/server.go b/pkg/api/server.go index 2fbecc4..d3095af 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -4,7 +4,9 @@ package api import ( "context" "fmt" + "net/http" "sync" + "time" "github.com/LiskHQ/op-fault-detector/pkg/api/middlewares" "github.com/LiskHQ/op-fault-detector/pkg/api/routes" @@ -13,26 +15,49 @@ import ( "github.com/gin-gonic/gin" ) -// StartServer starts the HTTP API server on invocation and stops it when the parent context is killed. -func StartServer(ctx context.Context, wg *sync.WaitGroup, logger log.Logger, config *config.Config) error { - defer wg.Done() - - for { - select { - case <-ctx.Done(): - return nil - - default: - err := registerHandlersAndStartAPIServer(ctx, logger, config) - if err != nil { - return err - } - } +// HTTPServerWrapper embeds the http.Server along with the various other properties. +type HTTPServerWrapper struct { + server *http.Server + ctx context.Context + logger log.Logger + wg *sync.WaitGroup + errorChan chan error +} + +// Start starts the HTTP API server. +func (w *HTTPServerWrapper) Start() { + defer w.wg.Done() + + w.logger.Infof("Starting the HTTP server on %s.", w.server.Addr) + err := w.server.ListenAndServe() + if err != nil { + w.errorChan <- err + } +} + +// Stop gracefully shuts down the HTTP API server. +func (w *HTTPServerWrapper) Stop() error { + err := w.server.Shutdown(w.ctx) + if err == nil { + w.logger.Infof("Successfully stopped the HTTP server.") } + + return err } -func registerHandlersAndStartAPIServer(ctx context.Context, logger log.Logger, config *config.Config) error { - gin.SetMode(config.Server.GinMode) +func getGinModeFromSysLogLevel(sysLogLevel string) string { + ginMode := gin.DebugMode // Default mode + + if sysLogLevel != "debug" && sysLogLevel != "trace" { + ginMode = gin.ReleaseMode + } + + return ginMode +} + +// 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 { + gin.SetMode(getGinModeFromSysLogLevel(config.System.LogLevel)) router := gin.Default() @@ -46,13 +71,24 @@ func registerHandlersAndStartAPIServer(ctx context.Context, logger log.Logger, c // Register handlers for routes following the base path basePath := config.Api.BasePath baseGroup := router.Group(basePath) - logger.Debugf("Registering handlers for endpoints under %v.", basePath) + logger.Debugf("Registering handlers for endpoints under path '%s'.", basePath) routes.RegisterHandlersByGroup(logger, baseGroup, config.Api.RegisterVersions) - // Start the HTTP server and serve API requests - host := config.Server.Host - port := config.Server.Port - addr := fmt.Sprintf("%v:%v", host, port) - logger.Infof("Starting the HTTP API server on %v:%v", host, port) - return router.Run(addr) + host := config.Api.Server.Host + port := config.Api.Server.Port + addr := fmt.Sprintf("%s:%d", host, port) + + server := &HTTPServerWrapper{ + &http.Server{ + Addr: addr, + Handler: router, + ReadHeaderTimeout: 10 * time.Second, // TODO: Check appropriate value + }, + ctx, + logger, + wg, + errorChan, + } + + return server } diff --git a/pkg/api/server_test.go b/pkg/api/server_test.go new file mode 100644 index 0000000..f3eab8a --- /dev/null +++ b/pkg/api/server_test.go @@ -0,0 +1,56 @@ +package api + +import ( + "testing" + + "github.com/LiskHQ/op-fault-detector/pkg/log" + "github.com/gin-gonic/gin" + "github.com/magiconair/properties/assert" +) + +func TestGetGinModeFromSysLogLevel(t *testing.T) { + testCases := []struct { + name string + logLevel string + wantGinMode string + }{ + { + name: "should return debug gin_mode with trace log_level", + logLevel: log.LevelTrace, + wantGinMode: gin.DebugMode, + }, + { + name: "should return debug gin_mode with debug log_level", + logLevel: log.LevelDebug, + wantGinMode: gin.DebugMode, + }, + { + name: "should return release gin_mode with info log_level", + logLevel: log.LevelInfo, + wantGinMode: gin.ReleaseMode, + }, + { + name: "should return release gin_mode with warn log_level", + logLevel: log.LevelWarn, + wantGinMode: gin.ReleaseMode, + }, + { + name: "should return release gin_mode with error log_level", + logLevel: log.LevelError, + wantGinMode: gin.ReleaseMode, + }, + { + name: "should return release gin_mode with fatal log_level", + logLevel: log.LevelFatal, + wantGinMode: gin.ReleaseMode, + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gotGinMode := getGinModeFromSysLogLevel(tc.logLevel) + assert.Equal(t, gotGinMode, tc.wantGinMode) + }) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 5a4aa65..c8cb2fc 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,52 +1,116 @@ package config import ( - "context" "fmt" - "path" + "math" + "regexp" "strings" "github.com/LiskHQ/op-fault-detector/pkg/log" "github.com/LiskHQ/op-fault-detector/pkg/utils" - "github.com/spf13/viper" + "go.uber.org/multierr" ) -// Config struct is used to store the contents of the parsed config file. The properties (sub-properties) should map on-to-one with the config file. +// Config struct is used to store the contents of the parsed config file. +// The properties (sub-properties) should map on-to-one with the config file. type Config struct { + System *System `mapstructure:"system"` + Api *Api `mapstructure:"api"` + FaultDetector *FaultDetector `mapstructure:"fault_detector"` +} + +type System struct { + LogLevel string `mapstructure:"log_level"` +} + +type Api struct { Server struct { - Host string `mapstructure:"host"` - Port string `mapstructure:"port"` - GinMode string `mapstructure:"gin_mode"` + Host string `mapstructure:"host"` + Port uint `mapstructure:"port"` } `mapstructure:"server"` + BasePath string `mapstructure:"base_path"` + RegisterVersions []string `mapstructure:"register_versions"` +} - Api struct { - BasePath string `mapstructure:"base_path"` - RegisterVersions []string `mapstructure:"register_versions"` - } `mapstructure:"api"` +type FaultDetector struct { } -// GetAppConfig is the function that takes in the absolute path to the config file, parses the content and returns it. -func GetAppConfig(ctx context.Context, logger log.Logger, configFilepath string) (*Config, error) { - configDir := path.Dir(configFilepath) - configFilenameWithExt := path.Base(configFilepath) +func (c *Config) Validate() error { + var validationErrors error + + sysConfigError := validateSystemConfig(c) + apiConfigError := validateApiConfig(c) + fdConfigError := validateFaultDetectorConfig(c) + + validationErrors = multierr.Combine(sysConfigError, apiConfigError, fdConfigError) - splits := strings.FieldsFunc(configFilenameWithExt, func(r rune) bool { return r == '.' }) - configName := splits[0] - configType := splits[1] + if validationErrors != nil { + // Beautify the error message + validationErrorSplits := strings.Split(validationErrors.Error(), ";") + formattedErrorStr := strings.Join(validationErrorSplits, "\n\t- ") - viper.AddConfigPath(configDir) - viper.SetConfigName(configName) - viper.SetConfigType(configType) - err := viper.ReadInConfig() - if err != nil { - return nil, fmt.Errorf("failed to load the config from disk: %v", err.Error()) + return fmt.Errorf("fix the following %d config validation fail(s) to continue:\n\t - %s", len(validationErrorSplits), formattedErrorStr) } - var config Config - err = viper.Unmarshal(&config) - if err != nil { - return nil, fmt.Errorf("failed to unmarshal config. Verify 'Config' struct definition in '%v'", utils.CurrentFilePath(0)) + return validationErrors +} + +func validateSystemConfig(c *Config) 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)) } - return &config, nil + return validationErrors +} + +func validateApiConfig(c *Config) error { + var validationErrors error + + validationErrors = multierr.Append(validationErrors, validateApiServerConfig(c)) + + basePath := c.Api.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)) + } + + registerVersions := c.Api.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)) + } + } + + return validationErrors +} + +func validateApiServerConfig(c *Config) error { + var validationErrors error + + host := c.Api.Server.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)) + } + + port := c.Api.Server.Port + minPortNum := uint(0) + maxPortNum := uint(math.Pow(2, 16) - 1) + if port < minPortNum || port > maxPortNum { + validationErrors = multierr.Append(validationErrors, fmt.Errorf("api.server.port expected in range: %d - %d, received: %d", minPortNum, maxPortNum, port)) + } + + return validationErrors +} + +func validateFaultDetectorConfig(c *Config) error { + return nil } diff --git a/pkg/log/logger.go b/pkg/log/logger.go index 328e407..a3aa340 100644 --- a/pkg/log/logger.go +++ b/pkg/log/logger.go @@ -6,6 +6,15 @@ import ( "go.uber.org/zap/zapcore" ) +const ( + LevelTrace = "trace" + LevelDebug = "debug" + LevelInfo = "info" + LevelWarn = "warn" + LevelError = "error" + LevelFatal = "fatal" +) + // DefaultLogger is a default setup logger. var DefaultLogger Logger diff --git a/pkg/utils/fs.go b/pkg/utils/fs.go deleted file mode 100644 index e69648a..0000000 --- a/pkg/utils/fs.go +++ /dev/null @@ -1,17 +0,0 @@ -package utils - -import ( - "path" - "runtime" -) - -func CurrentFilePath(frame int) string { - _, file, _, _ := runtime.Caller(frame + 1) // Ignore the current frame - return file -} - -func GetCurrentDirectory(frame int) string { - currentFile := CurrentFilePath(frame + 1) // Ignore the current frame - currentDir := path.Dir(currentFile) - return currentDir -} diff --git a/pkg/utils/slice.go b/pkg/utils/slice.go new file mode 100644 index 0000000..bc2e7cd --- /dev/null +++ b/pkg/utils/slice.go @@ -0,0 +1,11 @@ +package utils + +// Contains checks if the specified element exists in the given slice. The supplied element must be a comparable type. +func Contains[T comparable](s []T, e T) bool { + for _, a := range s { + if a == e { + return true + } + } + return false +} diff --git a/pkg/utils/slice_test.go b/pkg/utils/slice_test.go new file mode 100644 index 0000000..129c7d5 --- /dev/null +++ b/pkg/utils/slice_test.go @@ -0,0 +1,67 @@ +package utils + +import ( + "testing" + + "github.com/magiconair/properties/assert" +) + +func TestContains_String(t *testing.T) { + testCases := []struct { + name string + slice []string + elem string + want bool + }{ + { + name: "should return true when the specified string contained in the provided slice", + slice: []string{"a", "b", "c"}, + elem: "a", + want: true, + }, + { + name: "should return false when the specified string not contained in the provided slice", + slice: []string{"a", "b", "c"}, + elem: "z", + want: false, + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := Contains(tc.slice, tc.elem) + assert.Equal(t, got, tc.want) + }) + } +} + +func TestContains_Int(t *testing.T) { + testCases := []struct { + name string + slice []int + elem int + want bool + }{ + { + name: "should return true when the specified int contained in the provided slice", + slice: []int{1, 2, 3}, + elem: 3, + want: true, + }, + { + name: "should return false when the specified int not contained in the provided slice", + slice: []int{1, 2, 3}, + elem: 99, + want: false, + }, + } + + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := Contains(tc.slice, tc.elem) + assert.Equal(t, got, tc.want) + }) + } +}