Special characters in env var used in config causes error "can not convert 'object' into 'string' accessing"

[carsonip]

If an env var is used in apm-server.yml, and that the env var contains certain special characters, it may cause an error during parsing, e.g. Error: error unpacking output.elasticsearch for fetching agent config: can not convert 'object' into 'string' accessing 'output.elasticsearch.password' (source:'apm-server.yml')

To reproduce, run test:

func TestPassword(t *testing.T) {
	t.Setenv("FOO", "abc,123")
	initCfgfile(t, `
apm-server:
  host: :8200
output.elasticsearch:
  username: user
  password: "${FOO}"
  `)
	cfg, rawConfig, keystore, err := LoadConfig()
	require.NoError(t, err)
	assert.NotNil(t, cfg)
	assert.NotNil(t, rawConfig)
	assert.NotNil(t, keystore)

	assertConfigEqual(t, map[string]interface{}{
		"apm-server": map[string]interface{}{
			"host": ":8200",
		},
		"path": map[string]interface{}{
			"config": paths.Paths.Config,
			"logs":   paths.Paths.Logs,
			"data":   paths.Paths.Data,
			"home":   paths.Paths.Home,
		},
		"output": map[string]interface{}{
			"elasticsearch": map[string]interface{}{
				"username": "user",
				"password": "abc,123",
			},
		},
	}, rawConfig)

	assertConfigEqual(t, map[string]interface{}{"host": ":8200"}, cfg.APMServer)
	assert.Equal(t, "elasticsearch", cfg.Output.Name())
	assertConfigEqual(t, map[string]interface{}{"username": "user", "password": "abc,123"}, cfg.Output.Config())
}

Test output:

=== RUN   TestPassword
    config_test.go:195: 
        	Error Trace:	/home/carson/projects/apm-server/internal/beatcmd/config_test.go:148
        	            				/home/carson/projects/apm-server/internal/beatcmd/config_test.go:195
        	Error:      	Not equal: 
        	            	expected: map[string]interface {}{"apm-server":map[string]interface {}{"host":":8200"}, "output":map[string]interface {}{"elasticsearch":map[string]interface {}{"password":"abc,123", "username":"user"}}, "path":map[string]interface {}{"config":"/tmp/TestPassword4154751502/001", "data":"/tmp/TestPassword4154751502/001/data", "home":"/tmp/TestPassword4154751502/001", "logs":"/tmp/TestPassword4154751502/001/logs"}}
        	            	actual  : map[string]interface {}{"apm-server":map[string]interface {}{"host":":8200"}, "output":map[string]interface {}{"elasticsearch":map[string]interface {}{"password":[]interface {}{"abc", 0x7b}, "username":"user"}}, "path":map[string]interface {}{"config":"/tmp/TestPassword4154751502/001", "data":"/tmp/TestPassword4154751502/001/data", "home":"/tmp/TestPassword4154751502/001", "logs":"/tmp/TestPassword4154751502/001/logs"}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -6,3 +6,6 @@
        	            	   (string) (len=13) "elasticsearch": (map[string]interface {}) (len=2) {
        	            	-   (string) (len=8) "password": (string) (len=7) "abc,123",
        	            	+   (string) (len=8) "password": ([]interface {}) (len=2) {
        	            	+    (string) (len=3) "abc",
        	            	+    (uint64) 123
        	            	+   },
        	            	    (string) (len=8) "username": (string) (len=4) "user"
        	Test:       	TestPassword
    config_test.go:215: 
        	Error Trace:	/home/carson/projects/apm-server/internal/beatcmd/config_test.go:148
        	            				/home/carson/projects/apm-server/internal/beatcmd/config_test.go:215
        	Error:      	Not equal: 
        	            	expected: map[string]interface {}{"password":"abc,123", "username":"user"}
        	            	actual  : map[string]interface {}{"password":[]interface {}{"abc", 0x7b}, "username":"user"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,6 @@
        	            	 (map[string]interface {}) (len=2) {
        	            	- (string) (len=8) "password": (string) (len=7) "abc,123",
        	            	+ (string) (len=8) "password": ([]interface {}) (len=2) {
        	            	+  (string) (len=3) "abc",
        	            	+  (uint64) 123
        	            	+ },
        	            	  (string) (len=8) "username": (string) (len=4) "user"
        	Test:       	TestPassword
--- FAIL: TestPassword (0.00s)

This is actually as a feature such that env var could provide lists, see docs, but it is surprising to users who expect this setup to work, until the env var contains certain characters, e.g. a comma in a randomly generated password to set output.elasticsearch.password.

go-ucfg added an IgnoreCommas config to opt-out of this behavior, and Elastic Agent already uses it. However, adopting this by default would be a breaking change for users who rely on this parsing behavior.

Related issue in beats: https://github.com/elastic/beats/issues/22460

[simitt]

If the IgnoreCommas were supported and enabled, would there still be an option to configure lists or would it be an either or configuration decision?

@carsonip I changed the label from bug to enhancement - while this might not be optimal, it currently works as designed.

[carsonip]

If the IgnoreCommas were supported and enabled, would there still be an option to configure lists or would it be an either or configuration decision?

I am not aware of other ways to configure a list from env var other than this.

Either FOO="['foo',1]" or FOO="'foo',1" will cause

apm-server:
  host: :8200
output.elasticsearch:
  username: user
  password: "${FOO}"

output.elasticsearch.password to be a list. The argument still stands - this complex object parsing behavior is going to catch unsuspecting users off guard, but setting IgnoreCommas by default would remove the ability to configure lists.

[simitt]

@axw do you have an opinion on whether this is worth a change in 9.0?

[axw]

I'm inclined to close it as WONTFIX:

• there's a (now documented) workaround
• it seems to be a very rarely encountered issue

[simitt]

++ on closing this as I don't think the fix would lead to global improvements.

@carsonip any objections or concerns?

[carsonip]

++, closing it as WONTFIX.