diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e7fcacac6..c9750a5c909 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/confmap/converter/expandconverter/expand.go b/confmap/converter/expandconverter/expand.go index f98100df361..33559f827a5 100644 --- a/confmap/converter/expandconverter/expand.go +++ b/confmap/converter/expandconverter/expand.go @@ -16,51 +16,123 @@ 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 @@ -68,6 +140,14 @@ func expandEnv(s string) string { 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 } diff --git a/confmap/converter/expandconverter/expand_test.go b/confmap/converter/expandconverter/expand_test.go index 65fb24e653c..32440ce4839 100644 --- a/confmap/converter/expandconverter/expand_test.go +++ b/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()) + } + }) + } +} diff --git a/confmap/resolver.go b/confmap/resolver.go index 96a7f419fba..e511bd0e14a 100644 --- a/confmap/resolver.go +++ b/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 diff --git a/internal/nonfatalerror/nonfatal.go b/internal/nonfatalerror/nonfatal.go new file mode 100644 index 00000000000..50d459956c5 --- /dev/null +++ b/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{}) +} diff --git a/internal/nonfatalerror/nonfatal_test.go b/internal/nonfatalerror/nonfatal_test.go new file mode 100644 index 00000000000..fcfac2945a2 --- /dev/null +++ b/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) +} diff --git a/service/collector.go b/service/collector.go index 249f88c695a..0eba4606fbe 100644 --- a/service/collector.go +++ b/service/collector.go @@ -31,6 +31,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/extension/ballastextension" + "go.opentelemetry.io/collector/internal/nonfatalerror" "go.opentelemetry.io/collector/service/internal/telemetrylogs" ) @@ -177,8 +178,13 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error { col.setCollectorState(Starting) cfg, err := col.set.ConfigProvider.Get(ctx, col.set.Factories) + var nonFatalErr error if err != nil { - return fmt.Errorf("failed to get config: %w", err) + if nonfatalerror.IsNonFatal(err) { + nonFatalErr = err + } else { + return fmt.Errorf("failed to get config: %w", err) + } } col.service, err = newService(&settings{ @@ -193,6 +199,13 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error { return err } + if nonFatalErr != nil { + col.service.telemetrySettings.Logger.Warn( + "Got nonfatal error while getting configuration", + zap.Error(nonFatalErr), + ) + } + if !col.set.SkipSettingGRPCLogger { telemetrylogs.SetColGRPCLogger(col.service.telemetrySettings.Logger, cfg.Service.Telemetry.Logs.Level) } diff --git a/service/config_provider.go b/service/config_provider.go index 8f428c25190..9aba642f819 100644 --- a/service/config_provider.go +++ b/service/config_provider.go @@ -18,12 +18,15 @@ import ( "context" "fmt" + "go.uber.org/multierr" + "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/converter/expandconverter" "go.opentelemetry.io/collector/confmap/provider/envprovider" "go.opentelemetry.io/collector/confmap/provider/fileprovider" "go.opentelemetry.io/collector/confmap/provider/yamlprovider" + "go.opentelemetry.io/collector/internal/nonfatalerror" "go.opentelemetry.io/collector/service/internal/configunmarshaler" ) @@ -106,8 +109,13 @@ func NewConfigProvider(set ConfigProviderSettings) (ConfigProvider, error) { func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*Config, error) { retMap, err := cm.mapResolver.Resolve(ctx) + var nonFatalErr error if err != nil { - return nil, fmt.Errorf("cannot resolve the configuration: %w", err) + if nonfatalerror.IsNonFatal(err) { + nonFatalErr = multierr.Append(nonFatalErr, err) + } else { + return nil, fmt.Errorf("cannot resolve the configuration: %w", err) + } } var cfg *Config @@ -119,7 +127,7 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories return nil, fmt.Errorf("invalid configuration: %w", err) } - return cfg, nil + return cfg, nonFatalErr } func (cm *configProvider) Watch() <-chan error {