config: add support for parsing env variables in configuration #6215
Conversation
| return nil | ||
| } | ||
|
|
||
| val, exists := os.LookupEnv(envVarName) |
There was a problem hiding this comment.
Should there be an allow list of env vars we can retrieve here, to avoid possible security problems?
For example, do we want to allow folks to inject PATH into the config?
There was a problem hiding this comment.
I suppose it would be up to the user if they set the configuration to include the whichever environment variable they choose to include.
It's true that someone could leak sensitive information through this mechanism, but it seems too restrictive to have an allow list. Either ways maybe this could be discussed further in the spec? Currently there are no such restrctions defined https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/data-model.md#environment-variable-substitution
| return nil | ||
| } | ||
|
|
||
| val, exists := os.LookupEnv(envVarName) |
There was a problem hiding this comment.
The SDK MUST interpret an empty value of an environment variable the same way as when the variable is unset.
Reasons are here: open-telemetry/opentelemetry-specification#2045. Maybe I should add the reasons to spec as well, but it was I think my first contribution to the specification.
| re := regexp.MustCompile(`\$\{([a-zA-Z_][a-zA-Z0-9_]*[-]?.*)\}`) | ||
|
|
||
| replaceEnvVars := func(input []byte) []byte { | ||
| return re.ReplaceAllFunc(input, func(s []byte) []byte { | ||
| match := re.FindSubmatch(s) | ||
| if len(match) < 2 { | ||
| return s | ||
| } | ||
| return replaceEnvVar(string(match[1])) | ||
| }) | ||
| } | ||
|
|
||
| file = replaceEnvVars(file) |
There was a problem hiding this comment.
The env var parsing is not compliant with the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/data-model.md#environment-variable-substitution
Environment variable substitution MUST only apply to scalar values. Mapping keys are not candidates for substitution.
and
It MUST NOT be possible to inject YAML structures by environment variables.
Are these handled anywhere? Can we add unit tests for unallowed substitutions?
There was a problem hiding this comment.
I have a feeling that implementing it in a safe manner would require implementing e.g. https://pkg.go.dev/gopkg.in/yaml.v3#Unmarshaler for each scalar type (and using these scalar types instead of the regular ones).
Such parsing would need to couple the package to some concrete YAML library (I am not against it).
There was a problem hiding this comment.
The alternative could be to implement https://pkg.go.dev/encoding/json#Unmarshaler and use https://github.com/kubernetes-sigs/yaml. Maybe it would be simpler. We could then also provide a ParseJSON function easier if needed.
There was a problem hiding this comment.
Would using something like this work here? https://github.com/open-telemetry/opentelemetry-collector/blob/28d0d57c358d850166c7a0d14eaad3d162e2ce56/confmap/provider.go#L234
There was a problem hiding this comment.
Would using something like this work here? https://github.com/open-telemetry/opentelemetry-collector/blob/28d0d57c358d850166c7a0d14eaad3d162e2ce56/confmap/provider.go#L234
Maybe. You would have to elaborate as I do not know how would like to use it (or maybe simply try). However, notice that probably we would not want to support []any and map[string]any.
This replaces the last bit of functionality that was opened in open-telemetry#4826 to support env variable replacement. Pulled the envprovider.go code from https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/provider/envprovider/provider.go Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
codeboten
force-pushed
the
codeboten/env-var-parsing
branch
from
e08dc29 to
7fc2745
Compare
This replaces the last bit of functionality that was opened in #4826 to support env variable replacement. Pulled the envprovider.go code from https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/provider/envprovider/provider.go
Fixes #4373