From 7c169a876b622bd96d51d29abfef03a550680a71 Mon Sep 17 00:00:00 2001 From: Andy Pliszka Date: Thu, 19 Sep 2024 10:19:13 -0400 Subject: [PATCH] refactor: removes address flags for debug and metrics server * removes address flags so config file is the source of truth for addresses * adds tests for flags to enable debug and metrics servers * adds tests for config file settings for addresses for debug and metrics servers * adjusts log messages --- cmd/flags.go | 24 --- cmd/start.go | 139 +++++++------ cmd/start_test.go | 245 +++++++++++++++++++++++ internal/relaydebug/debugserver.go | 2 + internal/relayermetrics/metricsserver.go | 2 + internal/relayertest/system.go | 14 ++ 6 files changed, 331 insertions(+), 95 deletions(-) create mode 100644 cmd/start_test.go diff --git a/cmd/flags.go b/cmd/flags.go index f6ecd2d47..b52d9674e 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -39,9 +39,7 @@ const ( flagOrder = "order" flagVersion = "version" flagEnableDebugServer = "enable-debug-server" - flagDebugAddr = "debug-addr" flagEnableMetricsServer = "enable-metrics-server" - flagMetricsAddr = "metrics-addr" flagOverwriteConfig = "overwrite" flagLimit = "limit" flagHeight = "height" @@ -421,17 +419,6 @@ func dstPortFlag(v *viper.Viper, cmd *cobra.Command) *cobra.Command { } func debugServerFlags(v *viper.Viper, cmd *cobra.Command) *cobra.Command { - cmd.Flags().String( - flagDebugAddr, - "", - "address to use for debug and metrics server. By default, "+ - "will be the api-listen-addr parameter in the global config.", - ) - - if err := v.BindPFlag(flagDebugAddr, cmd.Flags().Lookup(flagDebugAddr)); err != nil { - panic(err) - } - cmd.Flags().Bool( flagEnableDebugServer, false, @@ -446,17 +433,6 @@ func debugServerFlags(v *viper.Viper, cmd *cobra.Command) *cobra.Command { } func metricsServerFlags(v *viper.Viper, cmd *cobra.Command) *cobra.Command { - cmd.Flags().String( - flagMetricsAddr, - "", - "address to use for metrics server. By default, "+ - "will be the metrics-listen-addr parameter in the global config.", - ) - - if err := v.BindPFlag(flagMetricsAddr, cmd.Flags().Lookup(flagMetricsAddr)); err != nil { - panic(err) - } - cmd.Flags().Bool( flagEnableMetricsServer, false, diff --git a/cmd/start.go b/cmd/start.go index b9f258f82..ff26dfacb 100644 --- a/cmd/start.go +++ b/cmd/start.go @@ -93,85 +93,16 @@ $ %s start demo-path2 --max-tx-size 10`, appName, appName, appName, appName)), return err } - debugAddr := a.config.Global.APIListenPort - - // debug server - debugAddrFlag, err := cmd.Flags().GetString(flagDebugAddr) + err = setupDebugServer(cmd, a, err) if err != nil { return err } - if debugAddrFlag != "" { - debugAddr = debugAddrFlag - } - - flagEnableDebugServer, err := cmd.Flags().GetBool(flagEnableDebugServer) + prometheusMetrics, err := setupMetricsServer(cmd, a, err, chains) if err != nil { return err } - if flagEnableDebugServer == false || debugAddr == "" { - a.log.Info("Skipping debug server due to empty debug address flag") - } else { - a.log.Warn("SECURITY WARNING! Debug server is enabled. It should only be used for non-production deployments.") - ln, err := net.Listen("tcp", debugAddr) - if err != nil { - a.log.Error( - "Failed to listen on debug address. If you have another relayer process open, use --" + - flagDebugAddr + - " to pick a different address.", - ) - - return fmt.Errorf("failed to listen on debug address %q: %w", debugAddr, err) - } - log := a.log.With(zap.String("sys", "debughttp")) - log.Info("Debug server listening", zap.String("addr", debugAddr)) - relaydebug.StartDebugServer(cmd.Context(), log, ln) - } - - // metrics server - var prometheusMetrics *processor.PrometheusMetrics - - metricsAddr := a.config.Global.MetricsListenPort - - metricsAddrFlag, err := cmd.Flags().GetString(flagMetricsAddr) - if err != nil { - return err - } - - if metricsAddrFlag != "" { - metricsAddr = metricsAddrFlag - } - - flagEnableMetricsServer, err := cmd.Flags().GetBool(flagEnableMetricsServer) - if err != nil { - return err - } - - if flagEnableMetricsServer == false || metricsAddr == "" { - a.log.Info("Skipping metrics server due to empty metrics address flag") - } else { - ln, err := net.Listen("tcp", metricsAddr) - if err != nil { - a.log.Error( - "Failed to listen on metrics address. If you have another relayer process open, use --" + - flagMetricsAddr + - " to pick a different address.", - ) - - return fmt.Errorf("failed to listen on metrics address %q: %w", debugAddr, err) - } - log := a.log.With(zap.String("sys", "metricshttp")) - log.Info("Metrics server listening", zap.String("addr", metricsAddr)) - prometheusMetrics = processor.NewPrometheusMetrics() - relayermetrics.StartMetricsServer(cmd.Context(), log, ln, prometheusMetrics.Registry) - for _, chain := range chains { - if ccp, ok := chain.ChainProvider.(*cosmos.CosmosProvider); ok { - ccp.SetMetrics(prometheusMetrics) - } - } - } - processorType, err := cmd.Flags().GetString(flagProcessor) if err != nil { return err @@ -240,3 +171,69 @@ $ %s start demo-path2 --max-tx-size 10`, appName, appName, appName, appName)), cmd = stuckPacketFlags(a.viper, cmd) return cmd } + +func setupMetricsServer(cmd *cobra.Command, a *appState, err error, chains map[string]*relayer.Chain) (*processor.PrometheusMetrics, error) { + var prometheusMetrics *processor.PrometheusMetrics + + metricsAddr := a.config.Global.MetricsListenPort + + flagEnableMetricsServer, err := cmd.Flags().GetBool(flagEnableMetricsServer) + if err != nil { + return nil, err + } + + if flagEnableMetricsServer == false || metricsAddr == "" { + a.log.Warn("Disabled metrics server due to missing metrics-listen-addr setting in config file.") + } else { + a.log.Info("Metrics server is enabled.") + ln, err := net.Listen("tcp", metricsAddr) + if err != nil { + a.log.Error( + "Failed to listen on metrics address. If you have another relayer process open, use --" + + metricsAddr + + " to pick a different address or port.", + ) + + return nil, fmt.Errorf("failed to listen on metrics address %q: %w", metricsAddr, err) + } + log := a.log.With(zap.String("sys", "metricshttp")) + log.Info("Metrics server listening", zap.String("addr", metricsAddr)) + prometheusMetrics = processor.NewPrometheusMetrics() + relayermetrics.StartMetricsServer(cmd.Context(), log, ln, prometheusMetrics.Registry) + for _, chain := range chains { + if ccp, ok := chain.ChainProvider.(*cosmos.CosmosProvider); ok { + ccp.SetMetrics(prometheusMetrics) + } + } + } + return prometheusMetrics, nil +} + +func setupDebugServer(cmd *cobra.Command, a *appState, err error) error { + debugAddr := a.config.Global.APIListenPort + + flagEnableDebugServer, err := cmd.Flags().GetBool(flagEnableDebugServer) + if err != nil { + return err + } + + if flagEnableDebugServer == false || debugAddr == "" { + a.log.Warn("Disabled debug server due to missing api-listen-addr setting in config file.") + } else { + a.log.Warn("SECURITY WARNING! Debug server is enabled. It should only be used with caution and proper security.") + ln, err := net.Listen("tcp", debugAddr) + if err != nil { + a.log.Error( + "Failed to listen on debug address. If you have another relayer process open, use --" + + debugAddr + + " to pick a different address or port.", + ) + + return fmt.Errorf("failed to listen on debug address %q: %w", debugAddr, err) + } + log := a.log.With(zap.String("sys", "debughttp")) + log.Info("Debug server listening", zap.String("addr", debugAddr)) + relaydebug.StartDebugServer(cmd.Context(), log, ln) + } + return nil +} diff --git a/cmd/start_test.go b/cmd/start_test.go new file mode 100644 index 000000000..df7f1c982 --- /dev/null +++ b/cmd/start_test.go @@ -0,0 +1,245 @@ +package cmd_test + +import ( + "fmt" + "net" + "net/http" + "os" + "strings" + "testing" + + "github.com/cosmos/relayer/v2/cmd" + "github.com/cosmos/relayer/v2/internal/relaydebug" + "github.com/cosmos/relayer/v2/internal/relayermetrics" + "github.com/cosmos/relayer/v2/internal/relayertest" + "github.com/cosmos/relayer/v2/relayer/chains/cosmos" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" +) + +func TestMetricsServerFlag(t *testing.T) { + t.Parallel() + + tests := []struct { + args []string + wantedRunning bool + }{ + { + []string{"start"}, + false, + }, + { + []string{"start", "--enable-metrics-server"}, + true, + }, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + sys := setupRelayer(t) + logs, logger := setupLogger() + + sys.MustRunWithLogger(t, logger, tt.args...) + + if tt.wantedRunning == true { + requireRunningMetricsServer(t, logs, relayermetrics.MetricsServerPort) + } else { + requireDisabledMetricsServer(t, logs, relayermetrics.MetricsServerPort) + } + }) + } +} + +func TestMetricsServerConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + args []string + newSetting string + wantedPort int + serverRunning bool + }{ + { + []string{"start"}, + "", + 0, + false, + }, + { + []string{"start", "--enable-metrics-server"}, + "metrics-listen-addr: 127.0.0.1:6184", + 6184, + true, + }, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + sys := setupRelayer(t) + + updateConfig(t, sys, "metrics-listen-addr: 127.0.0.1:5184", tt.newSetting) + + logs, logger := setupLogger() + + sys.MustRunWithLogger(t, logger, tt.args...) + + if tt.serverRunning == true { + requireRunningMetricsServer(t, logs, tt.wantedPort) + } else { + requireDisabledMetricsServer(t, logs, tt.wantedPort) + } + }) + } +} + +func TestDebugServerFlag(t *testing.T) { + t.Parallel() + + tests := []struct { + args []string + wantedRunning bool + }{ + { + []string{"start"}, + false, + }, + { + []string{"start", "--enable-debug-server"}, + true, + }, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + sys := setupRelayer(t) + logs, logger := setupLogger() + + sys.MustRunWithLogger(t, logger, tt.args...) + + if tt.wantedRunning == true { + requireRunningDebugServer(t, logs, relaydebug.DebugServerPort) + } else { + requireDisabledDebugServer(t, logs, relaydebug.DebugServerPort) + } + }) + } +} + +func TestDebugServerConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + args []string + newSetting string + wantedPort int + wantedRunning bool + }{ + { + []string{"start"}, + "", + 0, + false, + }, + { + []string{"start", "--enable-debug-server"}, + "api-listen-addr: 127.0.0.1:6183", + 6183, + true, + }, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + sys := setupRelayer(t) + + updateConfig(t, sys, "api-listen-addr: 127.0.0.1:5183", tt.newSetting) + + logs, logger := setupLogger() + + sys.MustRunWithLogger(t, logger, tt.args...) + + if tt.wantedRunning == true { + requireRunningDebugServer(t, logs, tt.wantedPort) + } else { + requireDisabledDebugServer(t, logs, tt.wantedPort) + } + }) + } +} + +func requireDisabledMetricsServer(t *testing.T, logs *observer.ObservedLogs, port int) { + conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + if conn != nil { + defer conn.Close() + } + require.Error(t, err, "Server should be disabled") + require.Len(t, logs.FilterMessage("Disabled debug server due to missing api-listen-addr setting in config file.").All(), 1) +} + +func requireRunningMetricsServer(t *testing.T, logs *observer.ObservedLogs, port int) { + conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + if conn != nil { + defer conn.Close() + } + require.NoError(t, err, "Metrics server should be running") + res, err := http.Get(fmt.Sprintf("http://127.0.0.1:%d/metrics", port)) + require.NoError(t, err) + require.Equal(t, res.StatusCode, 200) + require.Len(t, logs.FilterMessage("Metrics server listening").All(), 1) + require.Len(t, logs.FilterMessage("Metrics server is enabled.").All(), 1) +} + +func requireDisabledDebugServer(t *testing.T, logs *observer.ObservedLogs, port int) { + conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + if conn != nil { + defer conn.Close() + } + require.Error(t, err, "Server should be disabled") + require.Len(t, logs.FilterMessage("Disabled debug server due to missing api-listen-addr setting in config file.").All(), 1) +} + +func requireRunningDebugServer(t *testing.T, logs *observer.ObservedLogs, port int) { + conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + if conn != nil { + defer conn.Close() + } + require.NoError(t, err, "Server should be running") + res, err := http.Get(fmt.Sprintf("http://127.0.0.1:%d/debug/pprof/goroutine", port)) + require.NoError(t, err) + require.Equal(t, res.StatusCode, 200) + require.Len(t, logs.FilterMessage("Debug server listening").All(), 1) + require.Len(t, logs.FilterMessage("SECURITY WARNING! Debug server is enabled. It should only be used with caution and proper security.").All(), 1) +} + +func setupLogger() (*observer.ObservedLogs, *zap.Logger) { + observedZapCore, observedLogs := observer.New(zap.InfoLevel) + observedLogger := zap.New(observedZapCore) + return observedLogs, observedLogger +} + +func setupRelayer(t *testing.T) *relayertest.System { + sys := relayertest.NewSystem(t) + + _ = sys.MustRun(t, "config", "init") + + sys.MustAddChain(t, "testChain", cmd.ProviderConfigWrapper{ + Type: "cosmos", + Value: cosmos.CosmosProviderConfig{ + ChainID: "testcosmos", + KeyringBackend: "test", + Timeout: "10s", + }, + }) + return sys +} + +func updateConfig(t *testing.T, sys *relayertest.System, oldSetting string, newSetting string) { + configFile := fmt.Sprintf("%s/config/config.yaml", sys.HomeDir) + data, err := os.ReadFile(configFile) + require.NoError(t, err) + + newConfig := strings.Replace(string(data), oldSetting, newSetting, 1) + + os.WriteFile(configFile, []byte(newConfig), 0644) +} diff --git a/internal/relaydebug/debugserver.go b/internal/relaydebug/debugserver.go index 2ae8aae63..3eed17342 100644 --- a/internal/relaydebug/debugserver.go +++ b/internal/relaydebug/debugserver.go @@ -9,6 +9,8 @@ import ( "go.uber.org/zap" ) +const DebugServerPort = 5183 + // StartDebugServer starts a debug server in a background goroutine, // accepting connections on the given listener. // Any HTTP logging will be written at info level to the given logger. diff --git a/internal/relayermetrics/metricsserver.go b/internal/relayermetrics/metricsserver.go index 74873a039..fc2183fe4 100644 --- a/internal/relayermetrics/metricsserver.go +++ b/internal/relayermetrics/metricsserver.go @@ -10,6 +10,8 @@ import ( "go.uber.org/zap" ) +const MetricsServerPort = 5184 + // StartMetricsServer starts a metrics server in a background goroutine, // accepting connections on the given listener. // Any HTTP logging will be written at info level to the given logger. diff --git a/internal/relayertest/system.go b/internal/relayertest/system.go index 59daad624..11467cd9b 100644 --- a/internal/relayertest/system.go +++ b/internal/relayertest/system.go @@ -106,6 +106,20 @@ func (s *System) MustRunWithInput(t *testing.T, in io.Reader, args ...string) Ru return res } +func (s *System) MustRunWithLogger(t *testing.T, logger *zap.Logger, args ...string) RunResult { + t.Helper() + + res := s.RunWithInput(logger, bytes.NewReader(nil), args...) + if res.Err != nil { + t.Logf("Error executing %v: %v", args, res.Err) + t.Logf("Stdout: %q", res.Stdout.String()) + t.Logf("Stderr: %q", res.Stderr.String()) + t.FailNow() + } + + return res +} + // MustAddChain serializes pcw to disk and calls "chains add --file". func (s *System) MustAddChain(t *testing.T, chainName string, pcw cmd.ProviderConfigWrapper) { t.Helper()