Skip to content

Add required API calls to retrieve configuration from Fleet.#14871

Merged
ph merged 4 commits intoelastic:fleetfrom
ph:agent/fetch-configuration-from-fleet
Dec 4, 2019
Merged

Add required API calls to retrieve configuration from Fleet.#14871
ph merged 4 commits intoelastic:fleetfrom
ph:agent/fetch-configuration-from-fleet

Conversation

@ph
Copy link
Copy Markdown
Contributor

@ph ph commented Nov 29, 2019

Kibana implement is in elastic/kibana#51968

--

This PR implements the required structs and calls to retrieve the
configuration from Fleet. This is step one before refactoring the
Reporter to split the pushing event and fetching of configuration.

@ph ph added [zube]: In Progress in progress Pull request is currently in progress. labels Nov 29, 2019
@ph ph self-assigned this Nov 29, 2019
@ph ph requested a review from michalpristas December 2, 2019 14:22
@ph ph changed the title [WIP] Agent/fetch configuration from fleet Add API required to retrieve configuration from Fleet. Dec 3, 2019
@ph ph changed the title Add API required to retrieve configuration from Fleet. Add required API calls to retrieve configuration from Fleet. Dec 3, 2019
@ph ph marked this pull request as ready for review December 3, 2019 16:22
@ph ph added [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. labels Dec 3, 2019
This PR implements the required structs and calls to retrieve the
configuration from Fleet. This is step one before refactoring the
Reporter to split the pushing event and fetching of configuration.
}
}
],
"success": true
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nchaulet Did we remove success from the fleet checkin response? I though all endpoints response was returning that field? But looking at your comments in elastic/kibana#51968 this doens't appear to be the case?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not removed it's here, I just did not put it in the examples.

@ph ph requested a review from nchaulet December 3, 2019 16:32
@nchaulet
Copy link
Copy Markdown
Member

nchaulet commented Dec 3, 2019

@ph Currently every events need to have a type and a subtype it's why ACTION_ACKNOWLEDGE it implemented like event {type: 'ACTION', subtype: 'ACKNOWLEDGE'}

Do you think it's better to keep it like this or to make subtype optional and have {type: 'ACTION_ACKNOWLEDGE'} {type: 'ACTION_UNKNOWN'} ? I thinks the second option is less easier to understand.

Also I am going to document that when we have a decision

Copy link
Copy Markdown
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks PH

Comment thread x-pack/agent/pkg/reporter/fleet/reporter.go
Comment thread x-pack/agent/pkg/fleetapi/actions.go
Comment thread x-pack/agent/pkg/fleetapi/actions.go Outdated
for _, response := range responses {
switch response.ActionType {
case "POLICY_CHANGE":
action = &PolicyChangeAction{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if it's not possible keep it as []Action and make decisions based on ActionType. Lazy casting basically

Copy link
Copy Markdown
Contributor Author

@ph ph Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had mixed feeling went implementing this, I had two ideas:

  1. Make it generic, extract the minimal required fields for the action ( type / action ID) and puts everything else in a json.RawMessage field so it can be lazily extracted later one.
  2. Implement a base type + concrete type on top of it.

I've gone with the second option because I wanted to keep the serialization in a single place and theses are just data object no logic should be added to them. If new behavior is required, we either:

  1. Create a new type / type cast to a new type.
  2. Create a function that accepts the struct.

Now, I diverge on that with the Event, as you may have noticed I've created a SerializableEvent interface that implements a minimum public interface and expect the user to correctly serialize the type to the JSON. I did this because, an event from Agent doesn't have at the moment a finite number of possibilities.

@ph
Copy link
Copy Markdown
Contributor Author

ph commented Dec 3, 2019

@ph Currently every events need to have a type and a subtype it's why ACTION_ACKNOWLEDGE it implemented like event {type: 'ACTION', subtype: 'ACKNOWLEDGE'}

Do you think it's better to keep it like this or to make subtype optional and have {type: 'ACTION_ACKNOWLEDGE'} {type: 'ACTION_UNKNOWN'} ? I thinks the second option is less easier to understand.

@nchaulet I can do the type/subtype thing, I was following your example in the PR description in elastic/kibana#51968 (comment)

edit: the description was updated with the right return now.

@ph
Copy link
Copy Markdown
Contributor Author

ph commented Dec 3, 2019

I've made the changes @nchaulet and @michalpristas:

  • Fix the fields when acking an action.
  • Prefixing the Action with "Action" instead of using a suffix this more inline with Error in golang and other system types.

Copy link
Copy Markdown
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, it should worked correctly with fleet API 👍

@ph ph merged commit bdf28d9 into elastic:fleet Dec 4, 2019
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…#14871)

* Implements the require "Checkin" calls

This PR implements the required structs and calls to retrieve the
configuration from Fleet. This is step one before refactoring the
Reporter to split the pushing event and fetching of configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants