Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support default values for env var expansion #10907

Merged
merged 7 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/envprovider-default-value.yaml
Original file line number Diff line number Diff line change
@@ -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']
43 changes: 40 additions & 3 deletions confmap/provider/envprovider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ 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`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of a definition for setting an error message, is there an open issue somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still have it. I don't understand the approach "let's be like Shell but just different enough so that common things don't work".

Copy link
Contributor

@codeboten codeboten Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not disagreeing, this was me asking if there is already a discussion about this somewhere I haven't found yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the approach "let's be like Shell but just different enough so that common things don't work".

This doesn't mean that we need to add full support for Bash/POSIX-like syntax today. Your PR does not add full support for all the features that Bash supports and that's okay. As I said above, I am not opposed to adding this, but since alignment with the Configuration WG is an explicit goal we should bring it up on their repo first.

//
// See also: https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution
func NewFactory() confmap.ProviderFactory {
return confmap.NewProviderFactory(newProvider)
}
Expand All @@ -41,14 +49,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))
}
Expand All @@ -63,3 +81,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
}
71 changes: 71 additions & 0 deletions confmap/provider/envprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,77 @@ 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
value string
uri string
expectedVal string
expectedErr string
}{
{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.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.expectedVal, str)
})
}
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())
}
Loading