Warn on unknown environment variable; add feature gate to error out

CHANGELOG.md

@@ -8,6 +8,9 @@
 
 ### 💡 Enhancements 💡
 
+- Warn when expanding unknown environment variable (#5734)
+  - The `confmap.expandconverter.RaiseErrorOnUnknownEnvVar` feature gate will turn this into an error.
+
 ### 🧰 Bug fixes 🧰
 
 ## v0.56.0 Beta

confmap/converter/expandconverter/expand.go

@@ -16,58 +16,138 @@ package expandconverter // import "go.opentelemetry.io/collector/confmap/convert
 
 import (
 	"context"
+	"fmt"
 	"os"
+	"sort"
 
 	"go.opentelemetry.io/collector/confmap"
+	"go.opentelemetry.io/collector/internal/nonfatalerror"
+	"go.opentelemetry.io/collector/service/featuregate"
 )
 
-type converter struct{}
+var (
+	// raiseErrorOnUnknownEnvVarFeatureGateID controls whether to raise an error when an environment variable is used but not present.
+	raiseErrorOnUnknownEnvVarFeatureGateID = "confmap.expandconverter.RaiseErrorOnUnknownEnvVar"
+	raiseErrorOnUnknownEnvVarFeatureGate   = featuregate.Gate{
+		ID:          raiseErrorOnUnknownEnvVarFeatureGateID,
+		Description: "Raise an error when an environment variable is used but not present",
+		Enabled:     false,
+	}
+)
+
+func init() {
+	featuregate.GetRegistry().MustRegister(raiseErrorOnUnknownEnvVarFeatureGate)
+}
+
+type converter struct {
+	registry *featuregate.Registry
+}
+
+type stringSet map[string]struct{}
+
+func (s stringSet) Add(str string) { s[str] = struct{}{} }
+func (s stringSet) Merge(other stringSet) {
+	for str := range other {
+		s[str] = struct{}{}
+	}
+}
+func (s stringSet) Slice() []string {
+	var slice []string
+	for elem := range s {
+		slice = append(slice, elem)
+	}
+	// sort for reproducibility in unit tests.
+	// remove if ever in the hot path.
+	sort.Strings(slice)
+	return slice
+}
 
 // New returns a confmap.Converter, that expands all environment variables for a given confmap.Conf.
 //
 // Notice: This API is experimental.
 func New() confmap.Converter {
-	return converter{}
+	return newWithRegistry(featuregate.GetRegistry())
 }
 
-func (converter) Convert(_ context.Context, conf *confmap.Conf) error {
+// newWithRegistry is useful for unit tests.
+func newWithRegistry(registry *featuregate.Registry) confmap.Converter {
+	return &converter{
+		registry: registry,
+	}
+}
+
+func (c converter) Convert(_ context.Context, conf *confmap.Conf) error {
 	out := make(map[string]interface{})
+	unknownVars := make(stringSet)
 	for _, k := range conf.AllKeys() {
-		out[k] = expandStringValues(conf.Get(k))
+		expanded, unknownExpanded := expandStringValues(conf.Get(k))
+		out[k] = expanded
+		unknownVars.Merge(unknownExpanded)
+	}
+
+	unknownVarsSlice := unknownVars.Slice()
+	var unknownErr error
+	if len(unknownVarsSlice) > 0 {
+		unknownErr = fmt.Errorf("failed to expand unknown environment variable(s): %v", unknownVarsSlice)
+		if !c.registry.IsEnabled(raiseErrorOnUnknownEnvVarFeatureGateID) {
+			unknownErr = nonfatalerror.New(fmt.Errorf(
+				"%w. Use %q to turn this into a fatal error",
+				unknownErr, raiseErrorOnUnknownEnvVarFeatureGateID,
+			))
+		}
 	}
-	return conf.Merge(confmap.NewFromStringMap(out))
+
+	if err := conf.Merge(confmap.NewFromStringMap(out)); err != nil {
+		return err
+	}
+	return unknownErr
 }
 
-func expandStringValues(value interface{}) interface{} {
+func expandStringValues(value interface{}) (expanded interface{}, unknownVars stringSet) {
+	unknownVars = make(stringSet)
+
 	switch v := value.(type) {
 	case string:
 		return expandEnv(v)
 	case []interface{}:
 		nslice := make([]interface{}, 0, len(v))
 		for _, vint := range v {
-			nslice = append(nslice, expandStringValues(vint))
+			expandedVal, unknownExpanded := expandStringValues(vint)
+			nslice = append(nslice, expandedVal)
+			unknownVars.Merge(unknownExpanded)
 		}
-		return nslice
+		return nslice, unknownVars
 	case map[string]interface{}:
 		nmap := map[string]interface{}{}
 		for mk, mv := range v {
-			nmap[mk] = expandStringValues(mv)
+			expandedVal, unknownExpanded := expandStringValues(mv)
+			nmap[mk] = expandedVal
+			unknownVars.Merge(unknownExpanded)
 		}
-		return nmap
+		return nmap, unknownVars
 	default:
-		return v
+		return v, unknownVars
 	}
 }
 
-func expandEnv(s string) string {
-	return os.Expand(s, func(str string) string {
+func expandEnv(s string) (expanded string, unknownVars stringSet) {
+	unknownVars = make(stringSet)
+	expanded = os.Expand(s, func(str string) string {
 		// This allows escaping environment variable substitution via $$, e.g.
 		// - $FOO will be substituted with env var FOO
 		// - $$FOO will be replaced with $FOO
 		// - $$$FOO will be replaced with $ + substituted env var FOO
 		if str == "$" {
 			return "$"
 		}
-		return os.Getenv(str)
+
+		// Use LookupEnv to check if environment variable exists
+		val, ok := os.LookupEnv(str)
+		if !ok {
+			unknownVars.Add(str)
+		}
+		return val
 	})
+
+	return expanded, unknownVars
 }

confmap/converter/expandconverter/expand_test.go

@@ -24,6 +24,8 @@ import (
 
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
+	"go.opentelemetry.io/collector/internal/nonfatalerror"
+	"go.opentelemetry.io/collector/service/featuregate"
 )
 
 func TestNewExpandConverter(t *testing.T) {
@@ -115,3 +117,72 @@ func TestNewExpandConverter_EscapedEnvVars(t *testing.T) {
 	require.NoError(t, New().Convert(context.Background(), conf))
 	assert.Equal(t, expectedMap, conf.ToStringMap())
 }
+
+func TestMissingEnvVar(t *testing.T) {
+	const receiverExtraMapValue = "some map value"
+	t.Setenv("MAP_VALUE", receiverExtraMapValue)
+	sourceMap := map[string]interface{}{
+		"test_string_map": map[string]interface{}{
+			"recv":    []interface{}{map[string]interface{}{"field": "$MAP_VALUE", "unknown": "$UNKNOWN_1"}},
+			"unknown": "$UNKNOWN_1-$UNKNOWN_2-$UNKNOWN_3",
+		},
+	}
+
+	tests := []struct {
+		name            string
+		registryBuilder func() *featuregate.Registry
+		expectedMap     map[string]interface{}
+		expectedErr     string
+		isNonFatal      bool
+	}{
+		{
+			name: "no fail on unknown environment variable",
+			registryBuilder: func() *featuregate.Registry {
+				registry := featuregate.NewRegistry()
+				registry.MustRegister(raiseErrorOnUnknownEnvVarFeatureGate)
+				registry.MustApply(map[string]bool{
+					raiseErrorOnUnknownEnvVarFeatureGateID: false,
+				})
+				return registry
+			},
+			expectedMap: map[string]interface{}{
+				"test_string_map": map[string]interface{}{
+					"recv":    []interface{}{map[string]interface{}{"field": receiverExtraMapValue, "unknown": ""}},
+					"unknown": "--",
+				},
+			},
+			expectedErr: "Non fatal error: failed to expand unknown environment variable(s): [UNKNOWN_1 UNKNOWN_2 UNKNOWN_3]. " +
+				"Use \"confmap.expandconverter.RaiseErrorOnUnknownEnvVar\" to turn this into a fatal error",
+			isNonFatal: true,
+		},
+		{
+			name: "fail on environment variable",
+			registryBuilder: func() *featuregate.Registry {
+				registry := featuregate.NewRegistry()
+				registry.MustRegister(raiseErrorOnUnknownEnvVarFeatureGate)
+				registry.MustApply(map[string]bool{
+					raiseErrorOnUnknownEnvVarFeatureGateID: true,
+				})
+				return registry
+			},
+			expectedErr: "failed to expand unknown environment variable(s): [UNKNOWN_1 UNKNOWN_2 UNKNOWN_3]",
+			isNonFatal:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			registry := tt.registryBuilder()
+			converter := newWithRegistry(registry)
+			conf := confmap.NewFromStringMap(sourceMap)
+
+			err := converter.Convert(context.Background(), conf)
+			if err != nil || tt.expectedErr != "" {
+				assert.EqualError(t, err, tt.expectedErr)
+				assert.Equal(t, tt.isNonFatal, nonfatalerror.IsNonFatal(err))
+			} else {
+				assert.Equal(t, tt.expectedMap, conf.ToStringMap())
+			}
+		})
+	}
+}

confmap/resolver.go

@@ -23,6 +23,8 @@ import (
 	"sync"
 
 	"go.uber.org/multierr"
+
+	"go.opentelemetry.io/collector/internal/nonfatalerror"
 )
 
 // follows drive-letter specification:
@@ -140,13 +142,18 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) {
 	}
 
 	// Apply the converters in the given order.
+	var nonFatalErr error
 	for _, confConv := range mr.converters {
 		if err := confConv.Convert(ctx, retMap); err != nil {
-			return nil, fmt.Errorf("cannot convert the confmap.Conf: %w", err)
+			if nonfatalerror.IsNonFatal(err) {
+				nonFatalErr = multierr.Append(nonFatalErr, err)
+			} else {
+				return nil, fmt.Errorf("cannot convert the confmap.Conf: %w", err)
+			}
 		}
 	}
 
-	return retMap, nil
+	return retMap, nonFatalErr
 }
 
 // Watch blocks until any configuration change was detected or an unrecoverable error

internal/nonfatalerror/nonfatal.go

@@ -0,0 +1,48 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//       http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package nonfatalerror // import "go.opentelemetry.io/collector/internal/nonfatalerror"
+
+import "errors"
+
+// nonFatal is an error that will be always returned if its source
+// receives the same inputs.
+type nonFatal struct {
+	err error
+}
+
+// New wraps an error to indicate that it is a non fatal error, i.e. an
+// error that needs to be reported but will not make the Collector fail.
+func New(err error) error {
+	return nonFatal{err: err}
+}
+
+func (p nonFatal) Error() string {
+	return "Non fatal error: " + p.err.Error()
+}
+
+// Unwrap returns the wrapped error for functions Is and As in standard package errors.
+func (p nonFatal) Unwrap() error {
+	return p.err
+}
+
+// IsNonFatal checks if an error was wrapped with the New function, which
+// is used to indicate that a given error needs to be reported but will not make
+// the Collector fail.
+func IsNonFatal(err error) bool {
+	if err == nil {
+		return false
+	}
+	return errors.As(err, &nonFatal{})
+}

internal/nonfatalerror/nonfatal_test.go

@@ -0,0 +1,63 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//       http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package nonfatalerror
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type testErrorType struct {
+	s string
+}
+
+func (t testErrorType) Error() string {
+	return ""
+}
+
+func TestIsNonFatal(t *testing.T) {
+	var err error
+	assert.False(t, IsNonFatal(err))
+
+	err = errors.New("testError")
+	assert.False(t, IsNonFatal(err))
+
+	err = New(err)
+	assert.True(t, IsNonFatal(err))
+
+	err = fmt.Errorf("%w", err)
+	assert.True(t, IsNonFatal(err))
+}
+
+func TestNonFatal_Unwrap(t *testing.T) {
+	var err error = testErrorType{"testError"}
+	require.False(t, IsNonFatal(err))
+
+	// Wrapping testErrorType err with non fatal error.
+	nonFatalErr := New(err)
+	require.True(t, IsNonFatal(nonFatalErr))
+
+	target := testErrorType{}
+	require.NotEqual(t, err, target)
+
+	isTestErrorTypeWrapped := errors.As(nonFatalErr, &target)
+	require.True(t, isTestErrorTypeWrapped)
+
+	require.Equal(t, err, target)
+}