Skip to content

Commit

Permalink
[confmap] log a warning when using $VAR in config (WIP) (#9547)
Browse files Browse the repository at this point in the history
**Description:** As requested by @mx-psi , added a no-op log for when
variables using the deprecated $VAR style are used. The logger should be
replaced once it is clear how to pass it down (see #9443). Also, from my
testing, the function passed to os.Expand is in fact only run when we
have $VAR and not for ${env:VAR}, so I did not add additional checking.

**Link to tracking Issue:** #9162 

**Testing:** I am not sure how to go about testing it, since we are not
passing a logger in yet, there is no easy way to know what is being
logged or what the map looks like. Some ideas on this would be
appreciated

---------

Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
  • Loading branch information
tomasmota and mx-psi authored Mar 26, 2024
1 parent 5f9a7d7 commit 99b367c
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 10 deletions.
40 changes: 30 additions & 10 deletions confmap/converter/expandconverter/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
93 changes: 93 additions & 0 deletions confmap/converter/expandconverter/expand_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
})
}
}
1 change: 1 addition & 0 deletions confmap/converter/expandconverter/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/confmap v0.97.0
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.26.0
)

require (
Expand Down
2 changes: 2 additions & 0 deletions confmap/converter/expandconverter/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 99b367c

Please sign in to comment.