-
Notifications
You must be signed in to change notification settings - Fork 123
Add Fleet API Client #311
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
Add Fleet API Client #311
Conversation
- Add resource for Fleet Agent Policies, Server Hosts, and Outputs - Add data source for Fleet Enrollment Tokens - Add API client for Fleet - Add code generator for Fleet API types, uses OpenAPI spec - Add related docs and tests
…rit kibana values
|
This is awesome! Thanks for the work here. Are we able to split this one up a little to simplify the review process. It feels like it should be relatively easy to have a PR per resource/data source, and maybe an initial one adding the fleet client? |
I think that should be doable! Yeah the codegen adds a bit of weight to the review, along with the new fleet client. The I'll go ahead and remove all of the resources from this PR, but I'll maybe leave the data source in (enrollment tokens). I may need at least one thing in there for codegen to work properly, but I'll see about that. |
- Will be re-added in later PRs, this PR will only be for the new fleet client
|
Okay, removed all of the data source/resources from this PR, so it will just be about the fleet client. I'll add those back in later PRs.
|
tobio
left a comment
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.
Couple of comments inline, generally lgtm. Modifying the openapi spec during generation feels pretty dangerous, is there a reason we need that here, shouldn't we be able to publish a working spec?
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.
This file terrifies me, why do we need to be editing the openapi spec?
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.
This was originally written by @andrewkroh and I continued to use it as it seemed to work fine.
Andrew, did we edit it to work around some issues with the API, and is it even necessary anymore?
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, I'm looking at using openapitools/openapi-generator-cli to see if that works well with the Fleet API. If it works, I'll use that instead, as I'd rather be consistent with everything else.
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.
Okay, so OpenAPI Generator cannot generate valid code with the Fleet spec:
$ make build-ci
go build -o terraform-provider-elasticstack
# github.com/elastic/terraform-provider-elasticstack/generated/fleet
generated/fleet/model_package_policy.go:39:6: NewPackagePolicy redeclared in this block
generated/fleet/model_new_package_policy.go:21:6: other declaration of NewPackagePolicy
make: *** [build-ci] Error 1Also, it added some serious bloat to this change:
411 files changed, 86208 insertions(+), 541 deletions(-)
If we could limit what models are generated, that may help. We are waiting on some changes to be made in the API for package policies, so we don't want them being generated now anyway.
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.
@andrewkroh are you able to help me understand the need for this file?
Andrew, did we edit it to work around some issues with the API, and is it even necessary anymore?
Maybe we can try generating the client without this file and see what the result is?
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 wrote this when I was hacking a PoC. In it's current state I never intended it for production. It does a few things. Some of which should be incorporated into the upstream schema.
- It filters the generated code to only include the parts that are used. This removes a bunch of unused and deprecated models.
- It fixes some issues with the generated code to avoid anonymous nested structs. This makes the generated code easier to use.
- It adds some parameters that are missing from the openapi spec, but are used by the real API as observed by inspecting Kibana's requests to the API.
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 cleaned this code up, so hopefully less "hacks". It's driven mostly by structs, so a bit safer to deal with now instead of blind maps. That said, I left in the filtering of paths and methods, since that is actually very useful to us and cuts down on the code bloat. I also kept in the filtering of the kbn-xsrf parameters. If I understand it correctly, we don't need this in generated code as we set the header in our Fleet client (correct me if I'm wrong, @andrewkroh).
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.
@tobio, any other comments on this?
I think given the current state of the Fleet OpenAPI spec, the filtering that we are currently doing (especially with my latest round of changes), is fair. To recap, all I am doing now is:
- Filtering out unneeded/deprecated API endpoints and methods. As Andrew mentioned, this dramatically cuts down on the amount of code generated.
- Removed the kbn-xsrf parameter from generated code. As mentioned, we already set the header in the API client. I have not noticed any issues when removing this code.
Since this is all still considered experimental code, I'd rather get this in and start iterating on it sooner than later. We plan to use this extensively on the Security External Integrations team for our demo environment to continuously test our integrations.
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.
Yep, they're all good points. 👍 on getting this in and making progress.
…nt, fix empty fleet client being generated
tobio
left a comment
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.
🎉 thanks for all the effort here, looking forward to getting the resources added too.
andrewkroh
left a comment
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.
Got a chance to look it over today. Nice work.
| @@ -0,0 +1,5 @@ | |||
| package fleetapi | |||
|
|
|||
| //go:generate go run generate.go -o fleet-filtered.json | |||
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.
Do you think we should use -v v8.7.1 or some release tag instead of generating from main (from default)?
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.
Yeah I think we should once the API goes GA (or whenever we get the package policy changes?). I could quickly test it right now and if there's not any significant differences, we could lock to v8.7.1 or something equivalent
Uh oh!
There was an error while loading. Please reload this page.