From 193f7f0d47e8abe2a75f6a9084770a4316690ee0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Mon, 25 Dec 2023 00:06:10 +0100
Subject: [PATCH] cmd/node: refuse to start if viper shadows keys

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.
---
 cmd/node/main.go   | 28 +++++++++++++++++-----------
 service/vochain.go |  4 ++--
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/cmd/node/main.go b/cmd/node/main.go
index d2c178c8e..7a121ff51 100644
--- a/cmd/node/main.go
+++ b/cmd/node/main.go
@@ -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
 }
@@ -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
diff --git a/service/vochain.go b/service/vochain.go
index 7b0b7eceb..f9e0cc3b6 100644
--- a/service/vochain.go
+++ b/service/vochain.go
@@ -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)
 	}