Adding --config.expand-environment-variables config as per review

Signed-off-by: Harshavardhan Musanalli <[email protected]>

README.md

@@ -123,6 +123,8 @@ using SNMP v2 GETBULK.
 The `--config.file` parameter can be used multiple times to load more than one file.
 It also supports [glob filename matching](https://pkg.go.dev/path/filepath#Glob), e.g. `snmp*.yml`.
 
+When `--config.expand-environment-variables` parameter is set to `true`, `username`, `password` & `priv_password` could be resolved from the environment variables. Defaults to false.
+
 Duplicate `module` or `auth` entries are treated as invalid and can not be loaded.
 
 ## Prometheus Configuration

config/config.go

@@ -14,18 +14,18 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
-	"strings"
 	"time"
 
 	"github.com/gosnmp/gosnmp"
 	"gopkg.in/yaml.v2"
 )
 
-func LoadFile(paths []string) (*Config, error) {
+func LoadFile(paths []string, expandEnvVars bool) (*Config, error) {
 	cfg := &Config{}
 	for _, p := range paths {
 		files, err := filepath.Glob(p)
@@ -44,8 +44,24 @@ func LoadFile(paths []string) (*Config, error) {
 		}
 	}
 
-	for _, auth := range cfg.Auths {
-		auth.Username = ResolveEnvVariables(string(auth.Username))
+	if expandEnvVars {
+		var err error
+		for i, auth := range cfg.Auths {
+			cfg.Auths[i].Username, err = substituteEnvVariables(auth.Username)
+			if err != nil {
+				return nil, err
+			}
+			password, err := substituteEnvVariables(string(auth.Password))
+			if err != nil {
+				return nil, err
+			}
+			cfg.Auths[i].Password.Set(password)
+			privPassword, err := substituteEnvVariables(string(auth.PrivPassword))
+			if err != nil {
+				return nil, err
+			}
+			cfg.Auths[i].PrivPassword.Set(privPassword)
+		}
 	}
 
 	return cfg, nil
@@ -137,7 +153,6 @@ func (c Auth) ConfigureSNMP(g *gosnmp.GoSNMP) {
 		priv = true
 	}
 	if auth {
-		usm.AuthenticationPassphrase = ResolveEnvVariables(string(c.Password))
 		switch c.AuthProtocol {
 		case "SHA":
 			usm.AuthenticationProtocol = gosnmp.SHA
@@ -154,7 +169,6 @@ func (c Auth) ConfigureSNMP(g *gosnmp.GoSNMP) {
 		}
 	}
 	if priv {
-		usm.PrivacyPassphrase = ResolveEnvVariables(string(c.PrivPassword))
 		switch c.PrivProtocol {
 		case "DES":
 			usm.PrivacyProtocol = gosnmp.DES
@@ -219,6 +233,10 @@ type Lookup struct {
 // Secret is a string that must not be revealed on marshaling.
 type Secret string
 
+func (s *Secret) Set(value string) {
+	*s = Secret(value)
+}
+
 // Hack for creating snmp.yml with the secret.
 var (
 	DoNotHideSecrets = false
@@ -324,27 +342,12 @@ func (re *Regexp) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return nil
 }
 
-func ResolveEnvVariables(input string) string {
-	// Search for "${env.ENV_VAR}" pattern in the input string
-	startIndex := strings.Index(input, "${env.")
-	endIndex := strings.Index(input, "}")
-
-	// If the pattern is not found, return the string as-is
-	if startIndex == -1 || endIndex == -1 || endIndex <= startIndex {
-		return input
+func substituteEnvVariables(value string) (string, error) {
+	result := os.Expand(value, func(s string) string {
+		return os.Getenv(s)
+	})
+	if result == "" {
+		return "", errors.New(value + " environment variable not found")
 	}
-
-	// Extract the environment variable name
-	envVarName := input[startIndex+len("${env.") : endIndex]
-
-	// Retrieve the value of the environment variable
-	envVarValue, exists := os.LookupEnv(envVarName)
-
-	// If the environment variable exists, replace the pattern with its value
-	if exists {
-		return strings.Replace(input, fmt.Sprintf("${env.%s}", envVarName), envVarValue, 1)
-	}
-
-	// If the environment variable doesn't exist, return the string as-is
-	return input
+	return result, nil
 }

config_test.go

@@ -18,13 +18,12 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/prometheus/snmp_exporter/config"
 	yaml "gopkg.in/yaml.v2"
 )
 
 func TestHideConfigSecrets(t *testing.T) {
 	sc := &SafeConfig{}
-	err := sc.ReloadConfig([]string{"testdata/snmp-auth.yml"})
+	err := sc.ReloadConfig([]string{"testdata/snmp-auth.yml"}, false)
 	if err != nil {
 		t.Errorf("Error loading config %v: %v", "testdata/snmp-auth.yml", err)
 	}
@@ -43,7 +42,7 @@ func TestHideConfigSecrets(t *testing.T) {
 
 func TestLoadConfigWithOverrides(t *testing.T) {
 	sc := &SafeConfig{}
-	err := sc.ReloadConfig([]string{"testdata/snmp-with-overrides.yml"})
+	err := sc.ReloadConfig([]string{"testdata/snmp-with-overrides.yml"}, false)
 	if err != nil {
 		t.Errorf("Error loading config %v: %v", "testdata/snmp-with-overrides.yml", err)
 	}
@@ -58,7 +57,7 @@ func TestLoadConfigWithOverrides(t *testing.T) {
 func TestLoadMultipleConfigs(t *testing.T) {
 	sc := &SafeConfig{}
 	configs := []string{"testdata/snmp-auth.yml", "testdata/snmp-with-overrides.yml"}
-	err := sc.ReloadConfig(configs)
+	err := sc.ReloadConfig(configs, false)
 	if err != nil {
 		t.Errorf("Error loading configs %v: %v", configs, err)
 	}
@@ -70,47 +69,17 @@ func TestLoadMultipleConfigs(t *testing.T) {
 	}
 }
 
-func TestResolveEnvVariables(t *testing.T) {
-	// Test case 1: When the environment variable is present
-	os.Setenv("ENV_VAR", "resolved_value")
-	defer os.Unsetenv("ENV_VAR")
-
-	input := "This is a sample configuration string with ${env.ENV_VAR} and without."
-	expected := "This is a sample configuration string with resolved_value and without."
-
-	result := config.ResolveEnvVariables(input)
-
-	if result != expected {
-		t.Errorf("Expected: %s, Got: %s", expected, result)
-	}
-
-	// Test case 2: When the environment variable is not present
-	os.Unsetenv("ENV_VAR")
-
-	input = "This is another configuration string without ${env.ENV_VAR}."
-	expected = "This is another configuration string without ${env.ENV_VAR}."
-
-	result = config.ResolveEnvVariables(input)
-
-	if result != expected {
-		t.Errorf("Expected: %s, Got: %s", expected, result)
-	}
-
-	// Test case 3: When the pattern is not present
-	input = "No pattern here."
-	expected = "No pattern here."
-
-	result = config.ResolveEnvVariables(input)
-
-	if result != expected {
-		t.Errorf("Expected: %s, Got: %s", expected, result)
-	}
-}
-
-// not so well written test case
+// When all environment variables are present
 func TestEnvSecrets(t *testing.T) {
+	os.Setenv("ENV_USERNAME", "snmp_username")
+	os.Setenv("ENV_PASSWORD", "snmp_password")
+	os.Setenv("ENV_PRIV_PASSWORD", "snmp_priv_password")
+	defer os.Unsetenv("ENV_USERNAME")
+	defer os.Unsetenv("ENV_PASSWORD")
+	defer os.Unsetenv("ENV_PRIV_PASSWORD")
+
 	sc := &SafeConfig{}
-	err := sc.ReloadConfig([]string{"testdata/snmp-auth-envvars.yml"})
+	err := sc.ReloadConfig([]string{"testdata/snmp-auth-envvars.yml"}, true)
 	if err != nil {
 		t.Errorf("Error loading config %v: %v", "testdata/snmp-auth-envvars.yml", err)
 	}
@@ -123,7 +92,33 @@ func TestEnvSecrets(t *testing.T) {
 		t.Errorf("Error marshaling config: %v", err)
 	}
 
-	if !strings.Contains(string(c), "/") {
-		t.Fatal("config's Username does not contain / substring")
+	if strings.Contains(string(c), "mysecret") {
+		t.Fatal("config's String method reveals authentication credentials.")
+	}
+
+	// we check whether vars we set are resolved correctly in config
+	for i := range sc.C.Auths {
+		if sc.C.Auths[i].Username != "snmp_username" || sc.C.Auths[i].Password != "snmp_password" || sc.C.Auths[i].PrivPassword != "snmp_priv_password" {
+			t.Fatal("failed to resolve secrets from env vars")
+		}
+	}
+}
+
+// When environment variable(s) are absent
+func TestEnvSecretsMissing(t *testing.T) {
+	os.Setenv("ENV_PASSWORD", "snmp_password")
+	os.Setenv("ENV_PRIV_PASSWORD", "snmp_priv_password")
+	defer os.Unsetenv("ENV_PASSWORD")
+	defer os.Unsetenv("ENV_PRIV_PASSWORD")
+
+	sc := &SafeConfig{}
+	err := sc.ReloadConfig([]string{"testdata/snmp-auth-envvars.yml"}, true)
+	if err != nil {
+		// we check the error message pattern to determine the error
+		if strings.Contains(err.Error(), "environment variable not found") {
+			t.Logf("Error loading config as env var is not set/missing %v: %v", "testdata/snmp-auth-envvars.yml", err)
+		} else {
+			t.Errorf("Error loading config %v: %v", "testdata/snmp-auth-envvars.yml", err)
+		}
 	}
 }

main.go

@@ -45,10 +45,11 @@ const (
 )
 
 var (
-	configFile  = kingpin.Flag("config.file", "Path to configuration file.").Default("snmp.yml").Strings()
-	dryRun      = kingpin.Flag("dry-run", "Only verify configuration is valid and exit.").Default("false").Bool()
-	concurrency = kingpin.Flag("snmp.module-concurrency", "The number of modules to fetch concurrently per scrape").Default("1").Int()
-	metricsPath = kingpin.Flag(
+	configFile    = kingpin.Flag("config.file", "Path to configuration file.").Default("snmp.yml").Strings()
+	dryRun        = kingpin.Flag("dry-run", "Only verify configuration is valid and exit.").Default("false").Bool()
+	concurrency   = kingpin.Flag("snmp.module-concurrency", "The number of modules to fetch concurrently per scrape").Default("1").Int()
+	expandEnvVars = kingpin.Flag("config.expand-environment-variables", "Expand environment variables to source secrets").Default("false").Bool()
+	metricsPath   = kingpin.Flag(
 		"web.telemetry-path",
 		"Path under which to expose metrics.",
 	).Default("/metrics").String()
@@ -165,8 +166,8 @@ type SafeConfig struct {
 	C *config.Config
 }
 
-func (sc *SafeConfig) ReloadConfig(configFile []string) (err error) {
-	conf, err := config.LoadFile(configFile)
+func (sc *SafeConfig) ReloadConfig(configFile []string, expandEnvVars bool) (err error) {
+	conf, err := config.LoadFile(configFile, expandEnvVars)
 	if err != nil {
 		return err
 	}
@@ -197,7 +198,7 @@ func main() {
 	prometheus.MustRegister(version.NewCollector("snmp_exporter"))
 
 	// Bail early if the config is bad.
-	err := sc.ReloadConfig(*configFile)
+	err := sc.ReloadConfig(*configFile, *expandEnvVars)
 	if err != nil {
 		level.Error(logger).Log("msg", "Error parsing config file", "err", err)
 		level.Error(logger).Log("msg", "Possible old config file, see https://github.com/prometheus/snmp_exporter/blob/main/auth-split-migration.md")
@@ -217,13 +218,13 @@ func main() {
 		for {
 			select {
 			case <-hup:
-				if err := sc.ReloadConfig(*configFile); err != nil {
+				if err := sc.ReloadConfig(*configFile, *expandEnvVars); err != nil {
 					level.Error(logger).Log("msg", "Error reloading config", "err", err)
 				} else {
 					level.Info(logger).Log("msg", "Loaded config file")
 				}
 			case rc := <-reloadCh:
-				if err := sc.ReloadConfig(*configFile); err != nil {
+				if err := sc.ReloadConfig(*configFile, *expandEnvVars); err != nil {
 					level.Error(logger).Log("msg", "Error reloading config", "err", err)
 					rc <- err
 				} else {

testdata/snmp-auth-envvars.yml

@@ -2,8 +2,8 @@ auths:
   with_secret:
     community: mysecret
     security_level: SomethingReadOnly
-    username: ${env.PWD}
-    password: mysecret
+    username: ${ENV_USERNAME}
+    password: ${ENV_PASSWORD}
     auth_protocol: SHA256
     priv_protocol: AES
-    priv_password: mysecret
+    priv_password: ${ENV_PRIV_PASSWORD}