From b1913a73233b7702064b2d454138fe82b9805c0d Mon Sep 17 00:00:00 2001 From: ramin Date: Tue, 25 Jun 2024 12:58:30 +0100 Subject: [PATCH 1/4] fix core config flag parsing where, in specific circumstances, the values from config.toml would be overwritten by defaults if any core.ip was provided as a cli flag --- cmd/flags_node.go | 1 + nodebuilder/core/config.go | 9 ++- nodebuilder/core/flags.go | 16 ++-- nodebuilder/core/flags_test.go | 143 +++++++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+), 8 deletions(-) create mode 100644 nodebuilder/core/flags_test.go diff --git a/cmd/flags_node.go b/cmd/flags_node.go index bc86cb4a27..510d428735 100644 --- a/cmd/flags_node.go +++ b/cmd/flags_node.go @@ -53,6 +53,7 @@ func ParseNodeFlags(ctx context.Context, cmd *cobra.Command, network p2p.Network if nodeConfig != "" { // try to load config from given path cfg, err := nodebuilder.LoadConfig(nodeConfig) + if err != nil { return ctx, fmt.Errorf("cmd: while parsing '%s': %w", nodeConfigFlag, err) } diff --git a/nodebuilder/core/config.go b/nodebuilder/core/config.go index bb5eea5b83..1690c61610 100644 --- a/nodebuilder/core/config.go +++ b/nodebuilder/core/config.go @@ -7,6 +7,11 @@ import ( "github.com/celestiaorg/celestia-node/libs/utils" ) +const ( + DefaultRPCPort = "26657" + DefaultGRPCPort = "9090" +) + var MetricsEnabled bool // Config combines all configuration fields for managing the relationship with a Core node. @@ -21,8 +26,8 @@ type Config struct { func DefaultConfig() Config { return Config{ IP: "", - RPCPort: "26657", - GRPCPort: "9090", + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, } } diff --git a/nodebuilder/core/flags.go b/nodebuilder/core/flags.go index 9cbed9b277..772a8a414e 100644 --- a/nodebuilder/core/flags.go +++ b/nodebuilder/core/flags.go @@ -26,12 +26,12 @@ func Flags() *flag.FlagSet { ) flags.String( coreRPCFlag, - "26657", + DefaultRPCPort, "Set a custom RPC port for the core node connection. The --core.ip flag must also be provided.", ) flags.String( coreGRPCFlag, - "9090", + DefaultGRPCPort, "Set a custom gRPC port for the core node connection. The --core.ip flag must also be provided.", ) return flags @@ -50,11 +50,15 @@ func ParseFlags( return nil } - rpc := cmd.Flag(coreRPCFlag).Value.String() - grpc := cmd.Flag(coreGRPCFlag).Value.String() + if cmd.Flag(coreRPCFlag).Changed { + rpc := cmd.Flag(coreRPCFlag).Value.String() + cfg.RPCPort = rpc + } + if cmd.Flag(coreGRPCFlag).Changed { + grpc := cmd.Flag(coreGRPCFlag).Value.String() + cfg.GRPCPort = grpc + } cfg.IP = coreIP - cfg.RPCPort = rpc - cfg.GRPCPort = grpc return cfg.Validate() } diff --git a/nodebuilder/core/flags_test.go b/nodebuilder/core/flags_test.go new file mode 100644 index 0000000000..8e58e6a03a --- /dev/null +++ b/nodebuilder/core/flags_test.go @@ -0,0 +1,143 @@ +package core + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +func TestParseFlags(t *testing.T) { + tests := []struct { + name string + args []string + inputCfg Config // config that could be read from ctx + expectedCfg Config + expectError bool + }{ + { + name: "no flags", + args: []string{}, + inputCfg: Config{}, + expectedCfg: Config{ + IP: "", + RPCPort: "", + GRPCPort: "", + }, + expectError: false, + }, + { + name: "only core.ip", + args: []string{"--core.ip=127.0.0.1"}, + inputCfg: Config{ + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectError: false, + }, + { + name: "no flags, values from input config.toml ", + args: []string{}, + inputCfg: Config{ + IP: "127.162.36.1", + RPCPort: "1234", + GRPCPort: "5678", + }, + expectedCfg: Config{ + IP: "127.162.36.1", + RPCPort: "1234", + GRPCPort: "5678", + }, + expectError: false, + }, + { + name: "only core.ip, with config.toml overridden defaults for ports", + args: []string{"--core.ip=127.0.0.1"}, + inputCfg: Config{ + RPCPort: "1234", + GRPCPort: "5678", + }, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: "1234", + GRPCPort: "5678", + }, + expectError: false, + }, + { + name: "core.ip and core.rpc.port", + args: []string{"--core.ip=127.0.0.1", "--core.rpc.port=12345"}, + inputCfg: Config{ + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: "12345", + GRPCPort: DefaultGRPCPort, + }, + expectError: false, + }, + { + name: "core.ip and core.grpc.port", + args: []string{"--core.ip=127.0.0.1", "--core.grpc.port=54321"}, + inputCfg: Config{ + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + GRPCPort: "54321", + }, + expectError: false, + }, + { + name: "core.ip, core.rpc.port, and core.grpc.port", + args: []string{"--core.ip=127.0.0.1", "--core.rpc.port=12345", "--core.grpc.port=54321"}, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: "12345", + GRPCPort: "54321", + }, + expectError: false, + }, + { + name: "core.rpc.port without core.ip", + args: []string{"--core.rpc.port=12345"}, + expectedCfg: Config{}, + expectError: true, + }, + { + name: "core.grpc.port without core.ip", + args: []string{"--core.grpc.port=54321"}, + expectedCfg: Config{}, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := &cobra.Command{} + flags := Flags() + cmd.Flags().AddFlagSet(flags) + cmd.SetArgs(tt.args) + + err := cmd.Execute() + require.NoError(t, err) + + err = ParseFlags(cmd, &tt.inputCfg) + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expectedCfg, tt.inputCfg) + } + }) + } +} From 4d39d57a663895f76a64146dfb55ae304d0146a2 Mon Sep 17 00:00:00 2001 From: ramin Date: Tue, 25 Jun 2024 14:38:47 +0100 Subject: [PATCH 2/4] catch a fringe case where we parse in a core.ip, but also an empty config with no defaults (likely never happen) --- nodebuilder/core/flags.go | 7 +++++-- nodebuilder/core/flags_test.go | 11 +++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/nodebuilder/core/flags.go b/nodebuilder/core/flags.go index 772a8a414e..0b58319007 100644 --- a/nodebuilder/core/flags.go +++ b/nodebuilder/core/flags.go @@ -50,11 +50,14 @@ func ParseFlags( return nil } - if cmd.Flag(coreRPCFlag).Changed { + // in the case that the RPC port is not set, use the default port + // or in the case a value has been set via flag, + // set that as highest precedence + if cfg.RPCPort == "" || cmd.Flag(coreRPCFlag).Changed { rpc := cmd.Flag(coreRPCFlag).Value.String() cfg.RPCPort = rpc } - if cmd.Flag(coreGRPCFlag).Changed { + if cfg.GRPCPort == "" || cmd.Flag(coreGRPCFlag).Changed { grpc := cmd.Flag(coreGRPCFlag).Value.String() cfg.GRPCPort = grpc } diff --git a/nodebuilder/core/flags_test.go b/nodebuilder/core/flags_test.go index 8e58e6a03a..41b18eec7f 100644 --- a/nodebuilder/core/flags_test.go +++ b/nodebuilder/core/flags_test.go @@ -40,6 +40,17 @@ func TestParseFlags(t *testing.T) { }, expectError: false, }, + { + name: "only core.ip, empty port values", + args: []string{"--core.ip=127.0.0.1"}, + inputCfg: Config{}, + expectedCfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectError: false, + }, { name: "no flags, values from input config.toml ", args: []string{}, From 0d20b12eac59c7defeaf6d8e6e6427bb2ab4f233 Mon Sep 17 00:00:00 2001 From: ramin Date: Wed, 26 Jun 2024 10:56:29 +0100 Subject: [PATCH 3/4] implement @Wondertan's suggestion to let empty pass all the way and prefer fail on validation, also fill out tests for Validate --- nodebuilder/core/config.go | 7 +++ nodebuilder/core/config_test.go | 84 +++++++++++++++++++++++++++++++++ nodebuilder/core/flags.go | 7 +-- nodebuilder/core/flags_test.go | 2 +- 4 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 nodebuilder/core/config_test.go diff --git a/nodebuilder/core/config.go b/nodebuilder/core/config.go index 1690c61610..a211fac231 100644 --- a/nodebuilder/core/config.go +++ b/nodebuilder/core/config.go @@ -37,6 +37,13 @@ func (cfg *Config) Validate() error { return nil } + if cfg.RPCPort == "" { + return fmt.Errorf("nodebuilder/core: rpc port is not set") + } + if cfg.GRPCPort == "" { + return fmt.Errorf("nodebuilder/core: grpc port is not set") + } + ip, err := utils.ValidateAddr(cfg.IP) if err != nil { return err diff --git a/nodebuilder/core/config_test.go b/nodebuilder/core/config_test.go new file mode 100644 index 0000000000..9b06d6918d --- /dev/null +++ b/nodebuilder/core/config_test.go @@ -0,0 +1,84 @@ +package core + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidate(t *testing.T) { + tests := []struct { + name string + cfg Config + expectErr bool + }{ + { + name: "valid config", + cfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectErr: false, + }, + { + name: "empty config, no endpoint", + cfg: Config{}, + expectErr: false, + }, + { + name: "missing RPC port", + cfg: Config{ + IP: "127.0.0.1", + GRPCPort: DefaultGRPCPort, + }, + expectErr: true, + }, + { + name: "missing GRPC port", + cfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + }, + expectErr: true, + }, + { + name: "invalid IP", + cfg: Config{ + IP: "invalid-ip", + RPCPort: DefaultRPCPort, + GRPCPort: DefaultGRPCPort, + }, + expectErr: true, + }, + { + name: "invalid RPC port", + cfg: Config{ + IP: "127.0.0.1", + RPCPort: "invalid-port", + GRPCPort: DefaultGRPCPort, + }, + expectErr: true, + }, + { + name: "invalid GRPC port", + cfg: Config{ + IP: "127.0.0.1", + RPCPort: DefaultRPCPort, + GRPCPort: "invalid-port", + }, + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.cfg.Validate() + if tt.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/nodebuilder/core/flags.go b/nodebuilder/core/flags.go index 0b58319007..772a8a414e 100644 --- a/nodebuilder/core/flags.go +++ b/nodebuilder/core/flags.go @@ -50,14 +50,11 @@ func ParseFlags( return nil } - // in the case that the RPC port is not set, use the default port - // or in the case a value has been set via flag, - // set that as highest precedence - if cfg.RPCPort == "" || cmd.Flag(coreRPCFlag).Changed { + if cmd.Flag(coreRPCFlag).Changed { rpc := cmd.Flag(coreRPCFlag).Value.String() cfg.RPCPort = rpc } - if cfg.GRPCPort == "" || cmd.Flag(coreGRPCFlag).Changed { + if cmd.Flag(coreGRPCFlag).Changed { grpc := cmd.Flag(coreGRPCFlag).Value.String() cfg.GRPCPort = grpc } diff --git a/nodebuilder/core/flags_test.go b/nodebuilder/core/flags_test.go index 41b18eec7f..ce906de037 100644 --- a/nodebuilder/core/flags_test.go +++ b/nodebuilder/core/flags_test.go @@ -49,7 +49,7 @@ func TestParseFlags(t *testing.T) { RPCPort: DefaultRPCPort, GRPCPort: DefaultGRPCPort, }, - expectError: false, + expectError: true, }, { name: "no flags, values from input config.toml ", From c09947d504150b271b27f082fdb5d3de86510096 Mon Sep 17 00:00:00 2001 From: ramin Date: Wed, 26 Jun 2024 11:05:36 +0100 Subject: [PATCH 4/4] gofumpt -extra --- cmd/flags_node.go | 1 - nodebuilder/core/flags.go | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/flags_node.go b/cmd/flags_node.go index 510d428735..bc86cb4a27 100644 --- a/cmd/flags_node.go +++ b/cmd/flags_node.go @@ -53,7 +53,6 @@ func ParseNodeFlags(ctx context.Context, cmd *cobra.Command, network p2p.Network if nodeConfig != "" { // try to load config from given path cfg, err := nodebuilder.LoadConfig(nodeConfig) - if err != nil { return ctx, fmt.Errorf("cmd: while parsing '%s': %w", nodeConfigFlag, err) } diff --git a/nodebuilder/core/flags.go b/nodebuilder/core/flags.go index 772a8a414e..127ee5ee60 100644 --- a/nodebuilder/core/flags.go +++ b/nodebuilder/core/flags.go @@ -54,6 +54,7 @@ func ParseFlags( rpc := cmd.Flag(coreRPCFlag).Value.String() cfg.RPCPort = rpc } + if cmd.Flag(coreGRPCFlag).Changed { grpc := cmd.Flag(coreGRPCFlag).Value.String() cfg.GRPCPort = grpc