[confmap] log a warning when using $VAR in config (WIP)

confmap/converter/expandconverter/expand.go

@@ -5,51 +5,71 @@ package expandconverter // import "go.opentelemetry.io/collector/confmap/convert
 
 import (
 	"context"
+	"fmt"
 	"os"
+	"regexp"
+
+	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/confmap"
 )
 
-type converter struct{}
+type converter struct {
+	logger *zap.Logger
+
+	// Record of which env vars we have logged a warning for
+	loggedDeprecations map[string]struct{}
+}
 
 // New returns a confmap.Converter, that expands all environment variables for a given confmap.Conf.
 //
 // Notice: This API is experimental.
-func New(confmap.ConverterSettings) confmap.Converter {
-	return converter{}
+func New(_ confmap.ConverterSettings) confmap.Converter {
+	return converter{
+		loggedDeprecations: make(map[string]struct{}),
+		logger:             zap.NewNop(), // TODO: pass logger in ConverterSettings
+	}
 }
 
-func (converter) Convert(_ context.Context, conf *confmap.Conf) error {
+func (c converter) Convert(_ context.Context, conf *confmap.Conf) error {
 	out := make(map[string]any)
 	for _, k := range conf.AllKeys() {
-		out[k] = expandStringValues(conf.Get(k))
+		out[k] = c.expandStringValues(conf.Get(k))
 	}
 	return conf.Merge(confmap.NewFromStringMap(out))
 }
 
-func expandStringValues(value any) any {
+func (c converter) expandStringValues(value any) any {
 	switch v := value.(type) {
 	case string:
-		return expandEnv(v)
+		return c.expandEnv(v)
 	case []any:
 		nslice := make([]any, 0, len(v))
 		for _, vint := range v {
-			nslice = append(nslice, expandStringValues(vint))
+			nslice = append(nslice, c.expandStringValues(vint))
 		}
 		return nslice
 	case map[string]any:
 		nmap := map[string]any{}
 		for mk, mv := range v {
-			nmap[mk] = expandStringValues(mv)
+			nmap[mk] = c.expandStringValues(mv)
 		}
 		return nmap
 	default:
 		return v
 	}
 }
 
-func expandEnv(s string) string {
+func (c converter) expandEnv(s string) string {
 	return os.Expand(s, func(str string) string {
+		// Matches on $VAR style environment variables
+		// in order to make sure we don't log a warning for ${VAR}
+		var regex = regexp.MustCompile(fmt.Sprintf(`\$%s`, regexp.QuoteMeta(str)))
+		if _, exists := c.loggedDeprecations[str]; !exists && regex.MatchString(s) {
+			msg := fmt.Sprintf("Variable substitution using $VAR will be deprecated in favor of ${VAR} and ${env:VAR}, please update $%s", str)
+			c.logger.Warn(msg, zap.String("variable", str))
+			c.loggedDeprecations[str] = struct{}{}
+		}
 		// This allows escaping environment variable substitution via $$, e.g.
 		// - $FOO will be substituted with env var FOO
 		// - $$FOO will be replaced with $FOO

confmap/converter/expandconverter/expand_test.go

@@ -5,11 +5,15 @@ package expandconverter
 
 import (
 	"context"
+	"fmt"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
 
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
@@ -159,3 +163,92 @@ func TestNewExpandConverterHostPort(t *testing.T) {
 		})
 	}
 }
+
+func NewTestConverter() (confmap.Converter, *observer.ObservedLogs) {
+	core, logs := observer.New(zapcore.InfoLevel)
+	conv := converter{loggedDeprecations: make(map[string]struct{}), logger: zap.New(core)}
+	return conv, logs
+}
+
+func TestDeprecatedWarning(t *testing.T) {
+	msgTemplate := `Variable substitution using $VAR will be deprecated in favor of ${VAR} and ${env:VAR}, please update $%s`
+	t.Setenv("HOST", "127.0.0.1")
+	t.Setenv("PORT", "4317")
+
+	t.Setenv("HOST_NAME", "127.0.0.2")
+	t.Setenv("HOST.NAME", "127.0.0.3")
+
+	var testCases = []struct {
+		name             string
+		input            map[string]any
+		expectedOutput   map[string]any
+		expectedWarnings []string
+	}{
+		{
+			name: "no warning",
+			input: map[string]any{
+				"test": "${HOST}:${PORT}",
+			},
+			expectedOutput: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+			expectedWarnings: []string{},
+		},
+		{
+			name: "one deprecated var",
+			input: map[string]any{
+				"test": "${HOST}:$PORT",
+			},
+			expectedOutput: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+			expectedWarnings: []string{"PORT"},
+		},
+		{
+			name: "two deprecated vars",
+			input: map[string]any{
+				"test": "$HOST:$PORT",
+			},
+			expectedOutput: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+			expectedWarnings: []string{"HOST", "PORT"},
+		},
+		{
+			name: "one depracated serveral times",
+			input: map[string]any{
+				"test":  "$HOST,$HOST",
+				"test2": "$HOST",
+			},
+			expectedOutput: map[string]any{
+				"test":  "127.0.0.1,127.0.0.1",
+				"test2": "127.0.0.1",
+			},
+			expectedWarnings: []string{"HOST"},
+		},
+		{
+			name: "one warning",
+			input: map[string]any{
+				"test": "$HOST_NAME,${HOST.NAME}",
+			},
+			expectedOutput: map[string]any{
+				"test": "127.0.0.2,127.0.0.3",
+			},
+			expectedWarnings: []string{"HOST_NAME"},
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			conf := confmap.NewFromStringMap(tt.input)
+			conv, logs := NewTestConverter()
+			require.NoError(t, conv.Convert(context.Background(), conf))
+
+			assert.Equal(t, tt.expectedOutput, conf.ToStringMap())
+			assert.Equal(t, len(tt.expectedWarnings), len(logs.All()))
+			for i, variable := range tt.expectedWarnings {
+				errorMsg := fmt.Sprintf(msgTemplate, variable)
+				assert.Equal(t, errorMsg, logs.All()[i].Message)
+			}
+		})
+	}
+}

confmap/converter/expandconverter/go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/stretchr/testify v1.9.0
 	go.opentelemetry.io/collector/confmap v0.96.0
 	go.uber.org/goleak v1.3.0
+	go.uber.org/zap v1.26.0
 )
 
 require (