fix(nodebuilder/core): non default values from config.toml are writte…

…n over by flag defaults in specific circumstances (#3526) Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
celestia-org · Jun 27, 2024 · 6e39a07 · 6e39a07
1 parent b9f4c7e
commit 6e39a07
Show file tree

Hide file tree

Showing 4 changed files with 263 additions and 8 deletions.
diff --git 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,
 	}
 }
 
@@ -32,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
@@ -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
@@ -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,16 @@ 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
@@ -0,0 +1,154 @@
+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:     "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: true,
+		},
+		{
+			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)
+			}
+		})
+	}
+}