From 91ae0709fd2b13ae4105d84a59d62f9bf0402c30 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 17 Aug 2024 15:52:24 -0400 Subject: [PATCH 1/6] Support default values and erorrs for env var expansion Signed-off-by: Yuri Shkuro --- .chloggen/envprovider-default-value.yaml | 25 +++++++ confmap/provider/envprovider/provider.go | 41 ++++++++++- confmap/provider/envprovider/provider_test.go | 68 +++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 .chloggen/envprovider-default-value.yaml diff --git a/.chloggen/envprovider-default-value.yaml b/.chloggen/envprovider-default-value.yaml new file mode 100644 index 00000000000..9528a2a41d0 --- /dev/null +++ b/.chloggen/envprovider-default-value.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap/provider/envprovider + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Support default values when env var is empty + +# One or more tracking issues or pull requests related to the change +issues: [5228] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: ['user'] diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index 4db1cb7601a..e4509c7a0cf 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -27,6 +27,12 @@ type provider struct { // // This Provider supports "env" scheme, and can be called with a selector: // `env:NAME_OF_ENVIRONMENT_VARIABLE` +// +// A default value for unset variable can be provided after :- suffix, for example: +// `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value` +// +// An error message for unset variable can be provided after :? suffix, for example: +// `env:NAME_OF_ENVIRONMENT_VARIABLE:?error_message` func NewFactory() confmap.ProviderFactory { return confmap.NewProviderFactory(newProvider) } @@ -41,14 +47,24 @@ func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFu if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) } - envVarName := uri[len(schemeName)+1:] + envVarName, defaultValuePtr, errorMessagePtr, err := parseEnvVarURI(uri[len(schemeName)+1:]) + if err != nil { + return nil, err + } if !envvar.ValidationRegexp.MatchString(envVarName) { return nil, fmt.Errorf("environment variable %q has invalid name: must match regex %s", envVarName, envvar.ValidationPattern) - } + val, exists := os.LookupEnv(envVarName) if !exists { - emp.logger.Warn("Configuration references unset environment variable", zap.String("name", envVarName)) + if errorMessagePtr != nil { + return nil, fmt.Errorf("environment variable %q is not set: %s", envVarName, *errorMessagePtr) + } + if defaultValuePtr != nil { + val = *defaultValuePtr + } else { + emp.logger.Warn("Configuration references unset environment variable", zap.String("name", envVarName)) + } } else if len(val) == 0 { emp.logger.Info("Configuration references empty environment variable", zap.String("name", envVarName)) } @@ -63,3 +79,22 @@ func (*provider) Scheme() string { func (*provider) Shutdown(context.Context) error { return nil } + +// returns (var name, default value, error message, parse error) +func parseEnvVarURI(uri string) (string, *string, *string, error) { + const defaultSuffix = ":-" + const errorSuffix = ":?" + if strings.Contains(uri, defaultSuffix) { + parts := strings.SplitN(uri, defaultSuffix, 2) + return parts[0], &parts[1], nil, nil + } + if strings.Contains(uri, errorSuffix) { + parts := strings.SplitN(uri, errorSuffix, 2) + errMsg := parts[1] + if errMsg == "" { + return "", nil, nil, fmt.Errorf("empty error message for unset environment variable: %q", uri) + } + return parts[0], nil, &errMsg, nil + } + return uri, nil, nil, nil +} diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index 1f4b99c8318..e836aa7e9ef 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -154,6 +154,74 @@ func TestEmptyEnvWithLoggerWarn(t *testing.T) { assert.Equal(t, envName, logLine.Context[0].String) } +func TestEnvWithDefaultValue(t *testing.T) { + env := createProvider() + tests := []struct { + name string + unset bool + varValue string + uri string + expected string + }{ + {name: "unset", unset: true, uri: "env:MY_VAR:-default % value", expected: "default % value"}, + {name: "unset2", unset: true, uri: "env:MY_VAR:-", expected: ""}, // empty default still applies + {name: "empty", varValue: "", uri: "env:MY_VAR:-foo", expected: ""}, + {name: "not empty", varValue: "value", uri: "env:MY_VAR:-", expected: "value"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if !test.unset { + t.Setenv("MY_VAR", test.varValue) + } + ret, err := env.Retrieve(context.Background(), test.uri, nil) + require.NoError(t, err) + str, err := ret.AsString() + require.NoError(t, err) + assert.Equal(t, test.expected, str) + }) + } + assert.NoError(t, env.Shutdown(context.Background())) +} + +func TestEnvWithErrorMessage(t *testing.T) { + env := createProvider() + tests := []struct { + name string + unset bool + varValue string + uri string + expectedErr string + }{ + {name: "unset", unset: true, uri: "env:MY_VAR:?foobar", expectedErr: "foobar"}, + {name: "unset2", unset: true, uri: "env:MY_VAR:?", expectedErr: "empty error message"}, + {name: "empty", varValue: "", uri: "env:MY_VAR:?foo", expectedErr: ""}, // empty value does not cause error + {name: "not empty", varValue: "value", uri: "env:MY_VAR:?foo", expectedErr: ""}, + } + for _, test := range tests { + t.Run(test.varValue, func(t *testing.T) { + if !test.unset { + t.Setenv("MY_VAR", test.varValue) + } + ret, err := env.Retrieve(context.Background(), test.uri, nil) + if test.expectedErr == "" { + require.NoError(t, err) + str, err := ret.AsString() + require.NoError(t, err) + assert.Equal(t, test.varValue, str) + } else { + assert.ErrorContains(t, err, test.expectedErr) + } + }) + } + assert.NoError(t, env.Shutdown(context.Background())) + // const envName = "default_config" + // const errorMessage = "environment variable is not set" + // env := createProvider() + // _, err := env.Retrieve(context.Background(), envSchemePrefix+envName+":?"+errorMessage, nil) + // assert.EqualError(t, err, fmt.Sprintf("environment variable %q is not set: %s", envName, errorMessage)) + // assert.NoError(t, env.Shutdown(context.Background())) +} + func createProvider() confmap.Provider { return NewFactory().Create(confmaptest.NewNopProviderSettings()) } From 03409251c47646c6ea5ea9e4bdc861270dd78136 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 17 Aug 2024 15:58:52 -0400 Subject: [PATCH 2/6] clean-up Signed-off-by: Yuri Shkuro --- confmap/provider/envprovider/provider_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index e836aa7e9ef..a73161d377b 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -205,8 +205,8 @@ func TestEnvWithErrorMessage(t *testing.T) { ret, err := env.Retrieve(context.Background(), test.uri, nil) if test.expectedErr == "" { require.NoError(t, err) - str, err := ret.AsString() - require.NoError(t, err) + str, err2 := ret.AsString() + require.NoError(t, err2) assert.Equal(t, test.varValue, str) } else { assert.ErrorContains(t, err, test.expectedErr) @@ -214,12 +214,6 @@ func TestEnvWithErrorMessage(t *testing.T) { }) } assert.NoError(t, env.Shutdown(context.Background())) - // const envName = "default_config" - // const errorMessage = "environment variable is not set" - // env := createProvider() - // _, err := env.Retrieve(context.Background(), envSchemePrefix+envName+":?"+errorMessage, nil) - // assert.EqualError(t, err, fmt.Sprintf("environment variable %q is not set: %s", envName, errorMessage)) - // assert.NoError(t, env.Shutdown(context.Background())) } func createProvider() confmap.Provider { From 55eb82a516bf09123dce15c3cd792c96b033f955 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Mon, 19 Aug 2024 12:17:43 -0400 Subject: [PATCH 3/6] add more tests Signed-off-by: Yuri Shkuro --- confmap/provider/envprovider/provider.go | 2 + confmap/provider/envprovider/provider_test.go | 43 +++++++++++-------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index e4509c7a0cf..38b25e29261 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -33,6 +33,8 @@ type provider struct { // // An error message for unset variable can be provided after :? suffix, for example: // `env:NAME_OF_ENVIRONMENT_VARIABLE:?error_message` +// +// See also: https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution func NewFactory() confmap.ProviderFactory { return confmap.NewProviderFactory(newProvider) } diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index a73161d377b..6fbff82e927 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -157,27 +157,34 @@ func TestEmptyEnvWithLoggerWarn(t *testing.T) { func TestEnvWithDefaultValue(t *testing.T) { env := createProvider() tests := []struct { - name string - unset bool - varValue string - uri string - expected string + name string + unset bool + value string + uri string + expectedVal string + expectedErr string }{ - {name: "unset", unset: true, uri: "env:MY_VAR:-default % value", expected: "default % value"}, - {name: "unset2", unset: true, uri: "env:MY_VAR:-", expected: ""}, // empty default still applies - {name: "empty", varValue: "", uri: "env:MY_VAR:-foo", expected: ""}, - {name: "not empty", varValue: "value", uri: "env:MY_VAR:-", expected: "value"}, + {name: "unset", unset: true, uri: "env:MY_VAR:-default % value", expectedVal: "default % value"}, + {name: "unset2", unset: true, uri: "env:MY_VAR:-", expectedVal: ""}, // empty default still applies + {name: "empty", value: "", uri: "env:MY_VAR:-foo", expectedVal: ""}, + {name: "not empty", value: "value", uri: "env:MY_VAR:-", expectedVal: "value"}, + {name: "syntax1", unset: true, uri: "env:-MY_VAR", expectedErr: "invalid name"}, + {name: "syntax2", unset: true, uri: "env:MY_VAR:-test:-test", expectedVal: "test:-test"}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { if !test.unset { - t.Setenv("MY_VAR", test.varValue) + t.Setenv("MY_VAR", test.value) } ret, err := env.Retrieve(context.Background(), test.uri, nil) + if test.expectedErr != "" { + require.ErrorContains(t, err, test.expectedErr) + return + } require.NoError(t, err) str, err := ret.AsString() require.NoError(t, err) - assert.Equal(t, test.expected, str) + assert.Equal(t, test.expectedVal, str) }) } assert.NoError(t, env.Shutdown(context.Background())) @@ -188,26 +195,28 @@ func TestEnvWithErrorMessage(t *testing.T) { tests := []struct { name string unset bool - varValue string + value string uri string expectedErr string }{ {name: "unset", unset: true, uri: "env:MY_VAR:?foobar", expectedErr: "foobar"}, {name: "unset2", unset: true, uri: "env:MY_VAR:?", expectedErr: "empty error message"}, - {name: "empty", varValue: "", uri: "env:MY_VAR:?foo", expectedErr: ""}, // empty value does not cause error - {name: "not empty", varValue: "value", uri: "env:MY_VAR:?foo", expectedErr: ""}, + {name: "empty", value: "", uri: "env:MY_VAR:?foo", expectedErr: ""}, // empty value does not cause error + {name: "not empty", value: "value", uri: "env:MY_VAR:?foo", expectedErr: ""}, + {name: "syntax1", unset: true, uri: "env:?MY_VAR", expectedErr: "invalid name"}, + {name: "syntax2", unset: true, uri: "env:MY_VAR:?test:?test", expectedErr: "test:?test"}, } for _, test := range tests { - t.Run(test.varValue, func(t *testing.T) { + t.Run(test.name, func(t *testing.T) { if !test.unset { - t.Setenv("MY_VAR", test.varValue) + t.Setenv("MY_VAR", test.value) } ret, err := env.Retrieve(context.Background(), test.uri, nil) if test.expectedErr == "" { require.NoError(t, err) str, err2 := ret.AsString() require.NoError(t, err2) - assert.Equal(t, test.varValue, str) + assert.Equal(t, test.value, str) } else { assert.ErrorContains(t, err, test.expectedErr) } From 3a3f9390339b6b8d59d53533fefbe418e9291d63 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 25 Aug 2024 17:35:18 -0400 Subject: [PATCH 4/6] remove error support Signed-off-by: Yuri Shkuro --- confmap/provider/envprovider/provider.go | 24 +++---------- confmap/provider/envprovider/provider_test.go | 35 ------------------- 2 files changed, 5 insertions(+), 54 deletions(-) diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index 38b25e29261..0b1c849b06e 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -31,9 +31,6 @@ type provider struct { // A default value for unset variable can be provided after :- suffix, for example: // `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value` // -// An error message for unset variable can be provided after :? suffix, for example: -// `env:NAME_OF_ENVIRONMENT_VARIABLE:?error_message` -// // See also: https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution func NewFactory() confmap.ProviderFactory { return confmap.NewProviderFactory(newProvider) @@ -49,7 +46,7 @@ func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFu if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) } - envVarName, defaultValuePtr, errorMessagePtr, err := parseEnvVarURI(uri[len(schemeName)+1:]) + envVarName, defaultValuePtr, err := parseEnvVarURI(uri[len(schemeName)+1:]) if err != nil { return nil, err } @@ -59,9 +56,6 @@ func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFu val, exists := os.LookupEnv(envVarName) if !exists { - if errorMessagePtr != nil { - return nil, fmt.Errorf("environment variable %q is not set: %s", envVarName, *errorMessagePtr) - } if defaultValuePtr != nil { val = *defaultValuePtr } else { @@ -82,21 +76,13 @@ func (*provider) Shutdown(context.Context) error { return nil } -// returns (var name, default value, error message, parse error) -func parseEnvVarURI(uri string) (string, *string, *string, error) { +// returns (var name, default value, parse error) +func parseEnvVarURI(uri string) (string, *string, error) { const defaultSuffix = ":-" const errorSuffix = ":?" if strings.Contains(uri, defaultSuffix) { parts := strings.SplitN(uri, defaultSuffix, 2) - return parts[0], &parts[1], nil, nil - } - if strings.Contains(uri, errorSuffix) { - parts := strings.SplitN(uri, errorSuffix, 2) - errMsg := parts[1] - if errMsg == "" { - return "", nil, nil, fmt.Errorf("empty error message for unset environment variable: %q", uri) - } - return parts[0], nil, &errMsg, nil + return parts[0], &parts[1], nil } - return uri, nil, nil, nil + return uri, nil, nil } diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index 6fbff82e927..482c7c03431 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -190,41 +190,6 @@ func TestEnvWithDefaultValue(t *testing.T) { assert.NoError(t, env.Shutdown(context.Background())) } -func TestEnvWithErrorMessage(t *testing.T) { - env := createProvider() - tests := []struct { - name string - unset bool - value string - uri string - expectedErr string - }{ - {name: "unset", unset: true, uri: "env:MY_VAR:?foobar", expectedErr: "foobar"}, - {name: "unset2", unset: true, uri: "env:MY_VAR:?", expectedErr: "empty error message"}, - {name: "empty", value: "", uri: "env:MY_VAR:?foo", expectedErr: ""}, // empty value does not cause error - {name: "not empty", value: "value", uri: "env:MY_VAR:?foo", expectedErr: ""}, - {name: "syntax1", unset: true, uri: "env:?MY_VAR", expectedErr: "invalid name"}, - {name: "syntax2", unset: true, uri: "env:MY_VAR:?test:?test", expectedErr: "test:?test"}, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - if !test.unset { - t.Setenv("MY_VAR", test.value) - } - ret, err := env.Retrieve(context.Background(), test.uri, nil) - if test.expectedErr == "" { - require.NoError(t, err) - str, err2 := ret.AsString() - require.NoError(t, err2) - assert.Equal(t, test.value, str) - } else { - assert.ErrorContains(t, err, test.expectedErr) - } - }) - } - assert.NoError(t, env.Shutdown(context.Background())) -} - func createProvider() confmap.Provider { return NewFactory().Create(confmaptest.NewNopProviderSettings()) } From 549bdd6116d9ebccdb9849564c9637c777caf86b Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 25 Aug 2024 17:36:42 -0400 Subject: [PATCH 5/6] remove unused const Signed-off-by: Yuri Shkuro --- confmap/provider/envprovider/provider.go | 1 - 1 file changed, 1 deletion(-) diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index 0b1c849b06e..1c4cca75128 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -79,7 +79,6 @@ func (*provider) Shutdown(context.Context) error { // returns (var name, default value, parse error) func parseEnvVarURI(uri string) (string, *string, error) { const defaultSuffix = ":-" - const errorSuffix = ":?" if strings.Contains(uri, defaultSuffix) { parts := strings.SplitN(uri, defaultSuffix, 2) return parts[0], &parts[1], nil From 4d741e76213c4eb4e05214a9648a6985b561e1f8 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 25 Aug 2024 18:01:38 -0400 Subject: [PATCH 6/6] remove unused error return value Signed-off-by: Yuri Shkuro --- confmap/provider/envprovider/provider.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/confmap/provider/envprovider/provider.go b/confmap/provider/envprovider/provider.go index 1c4cca75128..8a0b87a9174 100644 --- a/confmap/provider/envprovider/provider.go +++ b/confmap/provider/envprovider/provider.go @@ -46,10 +46,7 @@ func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFu if !strings.HasPrefix(uri, schemeName+":") { return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName) } - envVarName, defaultValuePtr, err := parseEnvVarURI(uri[len(schemeName)+1:]) - if err != nil { - return nil, err - } + envVarName, defaultValuePtr := parseEnvVarURI(uri[len(schemeName)+1:]) if !envvar.ValidationRegexp.MatchString(envVarName) { return nil, fmt.Errorf("environment variable %q has invalid name: must match regex %s", envVarName, envvar.ValidationPattern) } @@ -76,12 +73,12 @@ func (*provider) Shutdown(context.Context) error { return nil } -// returns (var name, default value, parse error) -func parseEnvVarURI(uri string) (string, *string, error) { +// returns (var name, default value) +func parseEnvVarURI(uri string) (string, *string) { const defaultSuffix = ":-" if strings.Contains(uri, defaultSuffix) { parts := strings.SplitN(uri, defaultSuffix, 2) - return parts[0], &parts[1], nil + return parts[0], &parts[1] } - return uri, nil, nil + return uri, nil }