Fix support for concatenating envvars with colon (open-telemetry#6593)

* Fix support for concatenating envvars with colon Signed-off-by: Bogdan Drutu <[email protected]> * Update resolver.go Signed-off-by: Bogdan Drutu <[email protected]>
lightstep · Dec 14, 2022 · e58f118 · e58f118
1 parent 713a752
commit e58f118
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 3 deletions.
diff --git a/.chloggen/fixconfmap.yaml b/.chloggen/fixconfmap.yaml
@@ -0,0 +1,11 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: confmap
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix support for concatenating envvars with colon
+
+# One or more tracking issues or pull requests related to the change
+issues: [6580]
diff --git a/confmap/converter/expandconverter/expand_test.go b/confmap/converter/expandconverter/expand_test.go
@@ -115,3 +115,58 @@ func TestNewExpandConverter_EscapedEnvVars(t *testing.T) {
 	require.NoError(t, New().Convert(context.Background(), conf))
 	assert.Equal(t, expectedMap, conf.ToStringMap())
 }
+
+func TestNewExpandConverterHostPort(t *testing.T) {
+	t.Setenv("HOST", "127.0.0.1")
+	t.Setenv("PORT", "4317")
+
+	var testCases = []struct {
+		name     string
+		input    map[string]any
+		expected map[string]any
+	}{
+		{
+			name: "brackets",
+			input: map[string]any{
+				"test": "${HOST}:${PORT}",
+			},
+			expected: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+		},
+		{
+			name: "no brackets",
+			input: map[string]any{
+				"test": "$HOST:$PORT",
+			},
+			expected: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+		},
+		{
+			name: "mix",
+			input: map[string]any{
+				"test": "${HOST}:$PORT",
+			},
+			expected: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+		},
+		{
+			name: "reverse mix",
+			input: map[string]any{
+				"test": "$HOST:${PORT}",
+			},
+			expected: map[string]any{
+				"test": "127.0.0.1:4317",
+			},
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			conf := confmap.NewFromStringMap(tt.input)
+			require.NoError(t, New().Convert(context.Background(), conf))
+			assert.Equal(t, tt.expected, conf.ToStringMap())
+		})
+	}
+}
diff --git a/confmap/resolver.go b/confmap/resolver.go
@@ -245,7 +245,9 @@ func (mr *Resolver) expandValue(ctx context.Context, value interface{}) (interfa
 		}
 		lURI, err := newLocation(v[2 : len(v)-1])
 		if err != nil {
-			return nil, false, err
+			// Cannot return error, since a case like "${HOST}:${PORT}" is invalid location,
+			// but is supported in the legacy implementation.
+			return value, false, nil
 		}
 		if strings.Contains(lURI.opaqueValue, "$") {
 			return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString())

diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go
@@ -380,7 +380,7 @@ func TestResolverExpandEnvVars(t *testing.T) {
 }
 
 func TestResolverDoneNotExpandOldEnvVars(t *testing.T) {
-	expectedCfgMap := map[string]interface{}{"test.1": "${EXTRA}", "test.2": "$EXTRA"}
+	expectedCfgMap := map[string]interface{}{"test.1": "${EXTRA}", "test.2": "$EXTRA", "test.3": "${EXTRA}:${EXTRA}"}
 	fileProvider := newFakeProvider("test", func(context.Context, string, WatcherFunc) (*Retrieved, error) {
 		return NewRetrieved(expectedCfgMap)
 	})
@@ -486,7 +486,9 @@ func TestResolverExpandInvalidScheme(t *testing.T) {
 	require.NoError(t, err)
 
 	_, err = resolver.Resolve(context.Background())
-	assert.EqualError(t, err, `invalid uri: "g_c_s:VALUE"`)
+	// TODO: Investigate how to return an error like `invalid uri: "g_c_s:VALUE"` and keep the legacy behavior
+	//  for cases like "${HOST}:${PORT}"
+	assert.NoError(t, err)
 }
 
 func TestResolverExpandInvalidOpaqueValue(t *testing.T) {