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

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

Open
carsonip opened this issue Aug 19, 2024 · 2 comments

Comments

@carsonip
Copy link
Member

carsonip commented Aug 19, 2024

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: elastic/beats#22460

@carsonip carsonip added the bug label Aug 19, 2024
@simitt
Copy link
Contributor

simitt commented Aug 20, 2024

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.

@simitt simitt added enhancement and removed bug labels Aug 20, 2024
@carsonip
Copy link
Member Author

carsonip commented Aug 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants