Remove support to expand env variables in default configs (open-telem…

…etry#4366) Proven in open-telemetry#6122 that this can be done in the component itself, and no need for this complicated logic. No other usage in core/contrib. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
hex1848 · Nov 10, 2021 · 4bb261b · 4bb261b
1 parent 51e84c2
commit 4bb261b
Show file tree

Hide file tree

Showing 3 changed files with 1 addition and 269 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,7 @@
 - Remove `config.Pipeline.InputDataType` (#4343)
 - otlpexporter: Do not retry on PermissionDenied and Unauthenticated (#4349)
 - Remove deprecated funcs `consumererror.As[Traces|Metrics|Logs]` (#4364)
+- Remove support to expand env variables in default configs (#4366)
 
 ## 💡 Enhancements 💡
 - Supports more compression methods(`snappy` and `zstd`) for configgrpc, in addition to current `gzip` (#4088)

diff --git a/config/configunmarshaler/defaultunmarshaler.go b/config/configunmarshaler/defaultunmarshaler.go
@@ -16,8 +16,6 @@ package configunmarshaler // import "go.opentelemetry.io/collector/config/config
 
 import (
 	"fmt"
-	"os"
-	"reflect"
 
 	"go.uber.org/zap/zapcore"
 
@@ -151,7 +149,6 @@ func unmarshalExtensions(exts map[config.ComponentID]map[string]interface{}, fac
 		// Create the default config for this extension.
 		extensionCfg := factory.CreateDefaultConfig()
 		extensionCfg.SetIDName(id.Name())
-		expandEnvLoadedConfig(extensionCfg)
 
 		// Now that the default config struct is created we can Unmarshal into it,
 		// and it will apply user-defined config on top of the default.
@@ -195,7 +192,6 @@ func LoadReceiver(componentConfig *config.Map, id config.ComponentID, factory co
 	// Create the default config for this receiver.
 	receiverCfg := factory.CreateDefaultConfig()
 	receiverCfg.SetIDName(id.Name())
-	expandEnvLoadedConfig(receiverCfg)
 
 	// Now that the default config struct is created we can Unmarshal into it,
 	// and it will apply user-defined config on top of the default.
@@ -245,7 +241,6 @@ func unmarshalExporters(exps map[config.ComponentID]map[string]interface{}, fact
 		// Create the default config for this exporter.
 		exporterCfg := factory.CreateDefaultConfig()
 		exporterCfg.SetIDName(id.Name())
-		expandEnvLoadedConfig(exporterCfg)
 
 		// Now that the default config struct is created we can Unmarshal into it,
 		// and it will apply user-defined config on top of the default.
@@ -274,7 +269,6 @@ func unmarshalProcessors(procs map[config.ComponentID]map[string]interface{}, fa
 		// Create the default config for this processor.
 		processorCfg := factory.CreateDefaultConfig()
 		processorCfg.SetIDName(id.Name())
-		expandEnvLoadedConfig(processorCfg)
 
 		// Now that the default config struct is created we can Unmarshal into it,
 		// and it will apply user-defined config on top of the default.
@@ -288,64 +282,6 @@ func unmarshalProcessors(procs map[config.ComponentID]map[string]interface{}, fa
 	return processors, nil
 }
 
-// expandEnvLoadedConfig is a utility function that goes recursively through a config object
-// and tries to expand environment variables in its string fields.
-func expandEnvLoadedConfig(s interface{}) {
-	expandEnvLoadedConfigPointer(s)
-}
-
-func expandEnvLoadedConfigPointer(s interface{}) {
-	// Check that the value given is indeed a pointer, otherwise safely stop the search here
-	value := reflect.ValueOf(s)
-	if value.Kind() != reflect.Ptr {
-		return
-	}
-	// Run expandLoadedConfigValue on the value behind the pointer.
-	expandEnvLoadedConfigValue(value.Elem())
-}
-
-func expandEnvLoadedConfigValue(value reflect.Value) {
-	// The value given is a string, we expand it (if allowed).
-	if value.Kind() == reflect.String && value.CanSet() {
-		value.SetString(expandEnv(value.String()))
-	}
-	// The value given is a struct, we go through its fields.
-	if value.Kind() == reflect.Struct {
-		for i := 0; i < value.NumField(); i++ {
-			// Returns the content of the field.
-			field := value.Field(i)
-
-			// Only try to modify a field if it can be modified (eg. skip unexported private fields).
-			if field.CanSet() {
-				switch field.Kind() {
-				case reflect.String:
-					// The current field is a string, expand env variables in the string.
-					field.SetString(expandEnv(field.String()))
-				case reflect.Ptr:
-					// The current field is a pointer, run the expansion function on the pointer.
-					expandEnvLoadedConfigPointer(field.Interface())
-				case reflect.Struct:
-					// The current field is a nested struct, go through the nested struct
-					expandEnvLoadedConfigValue(field)
-				}
-			}
-		}
-	}
-}
-
-func expandEnv(s string) string {
-	return 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)
-	})
-}
-
 func unmarshal(componentSection *config.Map, intoCfg interface{}) error {
 	if cu, ok := intoCfg.(config.Unmarshallable); ok {
 		return cu.Unmarshal(componentSection)

diff --git a/config/configunmarshaler/defaultunmarshaler_test.go b/config/configunmarshaler/defaultunmarshaler_test.go
@@ -15,7 +15,6 @@
 package configunmarshaler
 
 import (
-	"os"
 	"path"
 	"testing"
 
@@ -213,207 +212,3 @@ func loadConfigFile(t *testing.T, fileName string, factories component.Factories
 	// Unmarshal the config from the config.Map using the given factories.
 	return NewDefault().Unmarshal(v, factories)
 }
-
-type nestedConfig struct {
-	NestedStringValue string
-	NestedIntValue    int
-}
-
-type testConfig struct {
-	config.ExporterSettings
-
-	NestedConfigPtr   *nestedConfig
-	NestedConfigValue nestedConfig
-	StringValue       string
-	StringPtrValue    *string
-	IntValue          int
-}
-
-func TestExpandEnvLoadedConfig(t *testing.T) {
-	assert.NoError(t, os.Setenv("NESTED_VALUE", "replaced_nested_value"))
-	assert.NoError(t, os.Setenv("VALUE", "replaced_value"))
-	assert.NoError(t, os.Setenv("PTR_VALUE", "replaced_ptr_value"))
-
-	defer func() {
-		assert.NoError(t, os.Unsetenv("NESTED_VALUE"))
-		assert.NoError(t, os.Unsetenv("VALUE"))
-		assert.NoError(t, os.Unsetenv("PTR_VALUE"))
-	}()
-
-	testString := "$PTR_VALUE"
-
-	cfg := &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    2,
-		},
-		StringValue:    "$VALUE",
-		StringPtrValue: &testString,
-		IntValue:       3,
-	}
-
-	expandEnvLoadedConfig(cfg)
-
-	replacedTestString := "replaced_ptr_value"
-
-	assert.Equal(t, &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    2,
-		},
-		StringValue:    "replaced_value",
-		StringPtrValue: &replacedTestString,
-		IntValue:       3,
-	}, cfg)
-}
-
-func TestExpandEnvLoadedConfigEscapedEnv(t *testing.T) {
-	assert.NoError(t, os.Setenv("NESTED_VALUE", "replaced_nested_value"))
-	assert.NoError(t, os.Setenv("ESCAPED_VALUE", "replaced_escaped_value"))
-	assert.NoError(t, os.Setenv("ESCAPED_PTR_VALUE", "replaced_escaped_pointer_value"))
-
-	defer func() {
-		assert.NoError(t, os.Unsetenv("NESTED_VALUE"))
-		assert.NoError(t, os.Unsetenv("ESCAPED_VALUE"))
-		assert.NoError(t, os.Unsetenv("ESCAPED_PTR_VALUE"))
-	}()
-
-	testString := "$$ESCAPED_PTR_VALUE"
-
-	cfg := &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    2,
-		},
-		StringValue:    "$$ESCAPED_VALUE",
-		StringPtrValue: &testString,
-		IntValue:       3,
-	}
-
-	expandEnvLoadedConfig(cfg)
-
-	replacedTestString := "$ESCAPED_PTR_VALUE"
-
-	assert.Equal(t, &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    2,
-		},
-		StringValue:    "$ESCAPED_VALUE",
-		StringPtrValue: &replacedTestString,
-		IntValue:       3,
-	}, cfg)
-}
-
-func TestExpandEnvLoadedConfigMissingEnv(t *testing.T) {
-	assert.NoError(t, os.Setenv("NESTED_VALUE", "replaced_nested_value"))
-
-	defer func() {
-		assert.NoError(t, os.Unsetenv("NESTED_VALUE"))
-	}()
-
-	testString := "$PTR_VALUE"
-
-	cfg := &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "$NESTED_VALUE",
-			NestedIntValue:    2,
-		},
-		StringValue:    "$VALUE",
-		StringPtrValue: &testString,
-		IntValue:       3,
-	}
-
-	expandEnvLoadedConfig(cfg)
-
-	replacedTestString := ""
-
-	assert.Equal(t, &testConfig{
-		ExporterSettings: config.NewExporterSettings(config.NewComponentID("test")),
-		NestedConfigPtr: &nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    1,
-		},
-		NestedConfigValue: nestedConfig{
-			NestedStringValue: "replaced_nested_value",
-			NestedIntValue:    2,
-		},
-		StringValue:    "",
-		StringPtrValue: &replacedTestString,
-		IntValue:       3,
-	}, cfg)
-}
-
-func TestExpandEnvLoadedConfigNil(t *testing.T) {
-	var cfg *testConfig
-
-	// This should safely do nothing
-	expandEnvLoadedConfig(cfg)
-
-	assert.Equal(t, (*testConfig)(nil), cfg)
-}
-
-func TestExpandEnvLoadedConfigNoPointer(t *testing.T) {
-	assert.NoError(t, os.Setenv("VALUE", "replaced_value"))
-
-	cfg := testConfig{
-		StringValue: "$VALUE",
-	}
-
-	// This should do nothing as cfg is not a pointer
-	expandEnvLoadedConfig(cfg)
-
-	assert.Equal(t, testConfig{StringValue: "$VALUE"}, cfg)
-}
-
-type testUnexportedConfig struct {
-	config.ExporterSettings
-
-	unexportedStringValue string
-	ExportedStringValue   string
-}
-
-func TestExpandEnvLoadedConfigUnexportedField(t *testing.T) {
-	assert.NoError(t, os.Setenv("VALUE", "replaced_value"))
-
-	defer func() {
-		assert.NoError(t, os.Unsetenv("VALUE"))
-	}()
-
-	cfg := &testUnexportedConfig{
-		unexportedStringValue: "$VALUE",
-		ExportedStringValue:   "$VALUE",
-	}
-
-	expandEnvLoadedConfig(cfg)
-
-	assert.Equal(t, &testUnexportedConfig{
-		unexportedStringValue: "$VALUE",
-		ExportedStringValue:   "replaced_value",
-	}, cfg)
-}