Be-very defensive when it comes to updating local_metadata. #5783
Conversation
blakerouse
added
Team:Elastic-Agent-Control-Plane
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
changelog/fragments/1761687609-fix-issue-prevent-checkin-local_metadata-from-being-updated.yaml
Outdated
Show resolved
Hide resolved
| var agentLocalMeta interface{} | ||
| if err := json.Unmarshal(agent.LocalMetadata, &agentLocalMeta); err != nil { | ||
| return nil, fmt.Errorf("parseMeta local: %w", err) | ||
| if agent.LocalMetadata != nil { |
There was a problem hiding this comment.
Just curious, why is this nil check necessary? json.Unmarshal(nil, &agentLocalMeta) should result in agentLocalMeta == nil, which is the zero value of agentLocalMeta anyway, right?
There was a problem hiding this comment.
Because this actually would fail if the local_metadata was missing. Seems like we don't actually hit this is the real world. Seems elasticsearch always returns it as local_metadata: {}. The unit test now tests both cases, so I added this here just to be more defensive.
This is because the checkin bulker has a default timeout of 10 seconds, meaning the original 10 seconds could result in it being missed by the check.
Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
There was a problem hiding this comment.
@ycombinator Thanks for the suggestions and review. I have applied them.
| var agentLocalMeta interface{} | ||
| if err := json.Unmarshal(agent.LocalMetadata, &agentLocalMeta); err != nil { | ||
| return nil, fmt.Errorf("parseMeta local: %w", err) | ||
| if agent.LocalMetadata != nil { |
There was a problem hiding this comment.
Because this actually would fail if the local_metadata was missing. Seems like we don't actually hit this is the real world. Seems elasticsearch always returns it as local_metadata: {}. The unit test now tests both cases, so I added this here just to be more defensive.
|
|
||
| # REQUIRED for all kinds | ||
| # Change summary; a 80ish characters long description of the change. | ||
| summary: fix issue prevent checkin local_metadata from being updated |
There was a problem hiding this comment.
sounds weird, should it be fix issue preventing?
4 participants
What is the problem this PR solves?
Adds extra checks for the local_metadata field when an Elastic Agent checks in. This ensures that an empty string doesn't result in it trying to update the document to an invalid object.
Adds other checks to ensure that the bulk update never tries to use an empty
[]byte()to update metadata or components.Fixes an issue in the checkin bulk that could result in a check-in previous values being lost if the check-in ticker has a very low interval (not real in the field).
How does this PR solve the problem?
It adds more defensive code checks and units tests to ensure that it remains that way.
How to test this PR locally
Unit tests are exercising the bad code paths, but reproduction in the field is unclear still.
Checklist
./changelog/fragmentsusing the changelog toolRelated issues