Skip to content

Commit

Permalink
cmd/node: refuse to start if viper shadows keys
Browse files Browse the repository at this point in the history
If a node is started with VOCDONI_VOCHAIN=foo,
then all flags or config keys under the vochain group are broken.
They don't bind properly to the config type,
and their defaults don't get applied properly.

This then leads to confusing failures, since values are left empty:

    $ VOCDONI_VOCHAIN=dev go run ./cmd/node
    [...]
    2023-12-25T00:05:54.798+01:00 FTL node/main.go:449 > invalid P2PListen "": missing port in address
    goroutine 1 [running]:
    runtime/debug.Stack()
    	/home/mvdan/tip/src/runtime/debug/stack.go:24 +0x5e
    go.vocdoni.io/dvote/log.Fatal({0xc0006bbf20, 0x1, 0x1})
    	/home/mvdan/src/voc/log/log.go:212 +0x65
    main.main()
    	/home/mvdan/src/voc/cmd/node/main.go:449 +0xc26

Don't let the node start if we know this would happen via an env var.
It's technically still possible for this to happen via the YAML config,
but it feels far less likely to be the case.

We can avoid repetition in the code by looping over the groups of
config key prefixes we use. It turns out that viper is always
case insensitive, so we don't have to worry about "tls" vs "TLS".

While here, add context to these SplitHostPort errors,
since the lone "missing port in address" error seemed confusing.
  • Loading branch information
mvdan authored and p4u committed Dec 28, 2023
1 parent ce59203 commit aad2c6b
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
28 changes: 17 additions & 11 deletions cmd/node/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,20 +89,15 @@ type pflagValue struct {
flag *flag.Flag
}

var viperGroups = []string{"vochain", "ipfs", "metrics", "tls"}

func (p pflagValue) Name() string {
name := p.flag.Name
// In some cases, vochainFoo becomes vochain.Foo to get YAML nesting
if after, ok := strings.CutPrefix(name, "vochain"); ok {
return "vochain." + after
}
if after, ok := strings.CutPrefix(name, "ipfs"); ok {
return "ipfs." + after
}
if after, ok := strings.CutPrefix(name, "metrics"); ok {
return "metrics." + after
}
if after, ok := strings.CutPrefix(name, "tls"); ok {
return "TLS." + after // note that it's all uppercase
for _, group := range viperGroups {
if after, ok := strings.CutPrefix(name, group); ok {
return group + "." + after
}
}
return name
}
Expand Down Expand Up @@ -224,6 +219,17 @@ func loadConfig() *config.Config {
viper.AutomaticEnv()
viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))

// If VOCDONI_VOCHAIN is set, then other values like VOCDONI_VOCHAIN_PEERS
// would be entirely ignored and left empty by viper as "shadowed".
// Since that's almost always a human error and would lead to confusing failures,
// refuse to continue any further.
for _, group := range viperGroups {
name := "VOCDONI_" + strings.ToUpper(group)
if val := os.Getenv(name); val != "" {
log.Fatalf("found %s=%s, which breaks our viper config", name, val)
}
}

viper.BindFlagValues(pflagValueSet{flag.CommandLine})

// use different datadirs for different networks
Expand Down
4 changes: 2 additions & 2 deletions service/vochain.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ func (vs *VocdoniService) Vochain() error {
} else {
_, port, err := net.SplitHostPort(vs.Config.P2PListen)
if err != nil {
return err
return fmt.Errorf("invalid P2PListen %q: %w", vs.Config.P2PListen, err)
}
vs.Config.PublicAddr = net.JoinHostPort(ip.String(), port)
}
} else {
host, port, err := net.SplitHostPort(vs.Config.PublicAddr)
if err != nil {
return err
return fmt.Errorf("invalid PublicAddr %q: %w", vs.Config.PublicAddr, err)
}
vs.Config.PublicAddr = net.JoinHostPort(host, port)
}
Expand Down

0 comments on commit aad2c6b

Please sign in to comment.