From b1913a73233b7702064b2d454138fe82b9805c0d Mon Sep 17 00:00:00 2001 From: ramin Date: Tue, 25 Jun 2024 12:58:30 +0100 Subject: [PATCH] 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) + } + }) + } +}