-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix json parsing bug with plugins that don't provide args #5489
Conversation
This fixes a bug with JSON agent configuration parsing where the AST for the plugin stanza had unnecessary flattening originating from hcl parsing library. The workaround fixes the AST by popping off the flattened element and wrapping it in a list. The workaround comes from similar code in terraform. There were no existing test cases for json parsing so I added a few.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this in few config files as well, and it is a good incremental fix - it fixes the case for new stanzas added for 0.9; but I think we'll need to increase our test coverage past 0.9.
@@ -0,0 +1,288 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest placing test content in a testdata
directory. testdata
directory is special:
The go tool will ignore a directory named "testdata", making it available to hold ancillary data needed by the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, noting that we use both testdata/ and other different names for test fixtures, should be made consistent in a future PR
@@ -0,0 +1,288 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe document how you generated this file? This is neat.
command/agent/config_parse.go
Outdated
// Deal with json->hcl AST parsing incorrectness when directly nested | ||
// items show up as additional keys. This currently only affects plugin | ||
// configuration because args is not necessary. All other fields in the config | ||
// have multiple keys and parse from json into the AST correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree with the outcome (this is the only place to add now), I disagree with the rationale.
It's true that other config stanzas require multiple entries, but a user may still set one field only triggering this condition and without unwrapLegacyHCLObjectKeysFromJSON
call, the error message might not be comprehensible. However, given this is only relevant for deeply nested entries, it affects few other keys at most.
Beyond 0.9, we want to increase our test coverage. I noticed that other parse*
functions have list = list.Elem()
call that's missing here, correctly so but not sure why. Also, I noticed that it's easy to get parser to crash with simple config files, like:
# basic client stanza
{"client": {"server_join": {"retry_max": 3}}}
# sentinel path that seems similar to this one
{"sentinel": {
"import": {"custom-plugin": {"path": "/custom-path"}}
}}
In both of these cases, list = list.Elem()
causes us to get an empty list that causes a crash when indexed. The pattern causing them affect many stanzes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, there are more lingering bugs in json parsing. Found this open ticket from 2016 :( #1290. Let's track this for the next minor release, +1 for more test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the wording of the comment
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This fixes a bug with JSON agent configuration parsing where the AST
for the plugin stanza had unnecessary flattening originating from hcl parsing.
The workaround fixes the AST by popping off the flattened element and wrapping
it in a list. The workaround comes from similar code in terraform
There were no existing test cases for json parsing so I added a few.
fixes #5484