Agent Actions: Part 1 of Osquerybeat with Agent actions #24507
Conversation
botelastic
bot
added
needs_team
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
botelastic
bot
removed
the
needs_team
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
|
odd: local run check 🤷 |
|
/test |
| } else { | ||
| action.StartedAt = readMapString(res, "started_at") | ||
| action.CompletedAt = readMapString(res, "completed_at") | ||
| action.Error = readMapString(res, "error") |
There was a problem hiding this comment.
probably ok for the error, can add some value for started/completed
| @@ -757,7 +757,7 @@ func (*SleepAction) Name() string { | |||
| return "sleep" | |||
| } | |||
|
|
|||
| func (*SleepAction) Execute(request map[string]interface{}) (map[string]interface{}, error) { | |||
| func (*SleepAction) Execute(ctx context.Context, request map[string]interface{}) (map[string]interface{}, error) { | |||
There was a problem hiding this comment.
Why is the context needed, and why is the sleep not cancellable. For cancellation consider timed.Wait.
There was a problem hiding this comment.
it implements the Action interface
https://github.com/elastic/elastic-agent-client/blob/7dd05ee2b5a5d7c6800117d7a49375d87294a716/pkg/client/client.go#L60
There was a problem hiding this comment.
Does the Action interface use context.Context for cancellation or is it require to pass values via Values?
See line 769: The sleep is not cancellable and should be changed to err := timed.Wait(ctx, time.Duration(sleep)) in order to make the action cancellable.
There was a problem hiding this comment.
ahh, misunderstood the previous comment, will update SleepAction implementation
| UnregisterAction(action client.Action) | ||
|
|
||
| // SetPayload sets the client payload | ||
| SetPayload(map[string]interface{}) |
There was a problem hiding this comment.
As this interface describes how input/Beats developers interact with the Agent: What exactly is the payload, what can/users do with it? Am I required to set it, or is it better to ignore SetPayload?
My understanding is that Endpoint uses the payload in order to pass some 'private monitoring' data via Agent to Kibana. I don't see why we need this on the Beats side as well, though.
There was a problem hiding this comment.
The payload was something that was not exposed for the beat to set.
In the case of osquerybeat work we need to leverage the payload to communicate to the agent the version of osqueryd.
Also using payload to communicate to the agent what inputs the application/beat is subscribed to.
This allows the agent to not care and not parse the config details of the particular app/beat.
Upon connection to the agent the beat subscribes to the actions on a receiving gRCP side and communicates back to the agent what "input types" are supported by the app.
More details are in the original PR: #24456
There was a problem hiding this comment.
Is this specific to osquerybeat?
Why not have a proper interface/type for the payload if it is bound to actual functionality in Agent?
If other Beats use a Payload, will it be used for similar or completely different functionality?
There was a problem hiding this comment.
It is generic just was not exposed, so the beats could not set their custom payload.
If you look at the current status updates, they pass nil with the status update payload
cm.client.Status(proto.StateObserved_STOPPING, "Stopping", nil)
@blakerouse mentioned before that there is already existing payload we could leverage to pass the custom payload over gRPC back to the agent. So I leveraged what we already have in place. Open for other suggestions.
There was a problem hiding this comment.
I guess I'm missing context here.
- Why not have a proper interface/type for the payload if it is bound to actual functionality in Agent?
- If other Beats use a Payload, will it be used for similar or completely different functionality?
There was a problem hiding this comment.
I guess I'm missing context here.
- Why not have a proper interface/type for the payload if it is bound to actual functionality in Agent?
We already have the payload that can be used. So I might be missing the bigger picture:
It's possible that I'm leveraging something the way it should not be used.
Not sure why we need another payload. What was the original purpose of this existing payload? and why it can't be
used for this particular purpose.
- If other Beats use a Payload, will it be used for similar or completely different functionality?
It's whatever payload they want to send back that gets passed with the checkin events back to the server.
So there are two things that I use the payload for:
- passing the osqueryd binary version with check in to the server
- passing the input_types array back to the elastic agent so it know where to dispatch the actions
Is this the right approach? @blakerouse
There was a problem hiding this comment.
@aleksmaus I think in this context you are miss using the payload. The payload has always been opaque to Elastic Agent. Using it for registering the input_types to the Elastic Agent feels more like a hack than the proper use of payload.
I don't think it is correct behavior that the input types are told to the Agent from the beat anyway. I think it would be better for this to be determined by the Agent already. Being that the specification file in Agent already has to code in the input type that is allowed to a beat, then that should be used instead.
Basically removing the need for payload to be used for the input_types. I do think you are using it correctly for the version of osquery.
|
I didn't check the CI errors. Jenkins itself was rather unstable the last days. But I noticed that quite a few (unrelated?) packages have been updated in go.mod. Some of these packages don't follow the semantics convetions and might also impact CI (e.g. updating |
|
I'll update the go.mod to roll back all unnecessary modules updates, will see if it helps. |
what's strange that the test that was failing in the unrelated beat was failing only on win8 and passing on all other versions of win. |
|
/test |
|
hmmm .... all arm packaging failed ... |
|
/test |
| } | ||
|
|
||
| if v, ok := m[key]; ok { | ||
| if s, ok := v.(string); ok { |
There was a problem hiding this comment.
how about if s, ok := v.(string); ok && s != "" { ... } ? In that case we will return def if the key exists, but the contents is empty.
|
@blakerouse Can you also review this? |
ph
added
Team:Elastic-Agent
|
Pinging @elastic/agent (Team:Agent) |
|
@aleksmaus please use "Team:Agent" instead of "Team:Ingest management". |
x-pack/elastic-agent/.gitignore
Outdated
| @@ -7,3 +7,7 @@ pkg/agent/operation/tests/scripts/configurable-1.0-darwin-x86/configurable | |||
| pkg/agent/operation/tests/scripts/servicable-1.0-darwin-x86/configurable | |||
| pkg/agent/transpiler/tests/exec-1.0-darwin-x86_64/exec | |||
| pkg/agent/application/fleet.yml | |||
| pkg/core/plugin/operation/tests/scripts/configurable/1.0/configurable | |||
There was a problem hiding this comment.
hmm, will remove. I certainly did add this by hand.
Based on conversation on PR made the change so now the input type are not passed back to the agent. Instead they are derived by the agent from the application spec. This is the additional property that was added to the spec to declare accepted action_input_types: action_input_types: - osquery
|
@blakerouse I made the change as we discussed. |
looks like the pesky botelastic adds it back :-) |
|
@aleksmaus I have missed that in the changes of teams see #24599 |
|
yesterday it failed on packed beat, now it's libbeat 🤷 |
|
|
packetbeat on this run |
|
/test |
|
* Agent Actions: Part 1 of Osquerybeat with Agent actions * Rollback some mods upgrade. Address some code review feedback * Address code review feedback * Add missing copyright header * Address more of the code review feedback * Change the way the inputs are tied to the applications Based on conversation on PR made the change so now the input type are not passed back to the agent. Instead they are derived by the agent from the application spec. This is the additional property that was added to the spec to declare accepted action_input_types: action_input_types: - osquery * Make action_input_type field optional in the spec. Fixes the existing test. (cherry picked from commit 0b4e053)
What does this PR do?
Agent Actions: Part 1 of Osquerybeat with Agent actions
This a first part of the original PR #24456
For more details on the whole picture and how it suppose to work refer the original PR details.
@urso I'll keep the original PR and will rebase it once these changes make it in.
Why is it important?
This allows the security assets management team to ship osquery integration in the upcoming release.
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.