-
Notifications
You must be signed in to change notification settings - Fork 122
[ResponseOps] Maintenance window resource #1224
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
Conversation
- uses the plugin framework - client generating the openApi specification
458b699
to
882ab65
Compare
2647913
to
f059ff2
Compare
9be5db8
to
a1bb3bf
Compare
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.
Thank you for writing this resource. Great work! 🚀
I left some smaller nits and comments, but looks great in general.
if model.Scope != nil { | ||
// Yes, I hate it too | ||
body.Scope = &struct { | ||
Alerting struct { | ||
Query struct { | ||
Kql string `json:"kql"` | ||
} `json:"query"` | ||
} `json:"alerting"` | ||
}{ | ||
Alerting: struct { | ||
Query struct { | ||
Kql string `json:"kql"` | ||
} `json:"query"` | ||
}{ | ||
Query: struct { | ||
Kql string `json:"kql"` | ||
}{ | ||
Kql: model.Scope.Alerting.Kql.ValueString(), | ||
}, | ||
}, | ||
} | ||
} |
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.
(Just a comment/discussion, no feedback to address)
The reason behind this is that the openapi spec defines this model inline without a ref/name. So in Go these are also generated as anonymous types.
I can't think of any way to have types here, except changing the spec coming from Kibana.
@tobio @dimuon I was wondering if you had any ideas on how to improve the typing 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.
(We discussed this offline already, but I'll leave it here for everyone else.)
My team has an upcoming OnWeek, and one of the things I am planning to look into is this. Not all resources have this problem, so there is something in the way the schema variables are defined in Kibana that leads to better-organized, automatically generated types.
In this case, I suspect this happens because in Kibana we do not have a separate variable for the Scope
type used in the POST body type definition.
One of the things I suspect(not directly connected to the example you commented on) is that although the type is the same for the PATCH and POST MW responses, we use different variables. That may lead to different response objects here.
PS.: I left the comment here hoping for this discussion, I will remove it now ;)
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 guess Kibana specs is the first candidate for improvement. However, perhaps @jamietanna could share his thoughts on forcing oapi-codegen to generate types in this situation. (sorry for the direct ping Jamie).
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.
There's been some discussion around improving this as part of elastic/kibana#228077, I assume that won't happen in the timeframe we'd like to improve the automation here. I suspect the simplest short term path here might be to extract these anonymous types in a script prior to passing the spec to oapi-codegen.
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 suspect the simplest short term path here might be to extract these anonymous types in a script prior to passing the spec to oapi-codegen.
Correct - right now, oapi-codegen
will not generate these anonymous types.
One option we've got is that we may be able to use OpenAPI Overlay which may be a little more "scalable" as a solution to make the extraction a little more straightforward to maintain
(Aside: I'm leaving Elastic on 2025-09-05, so you'll be best raising things like this with me via the public oapi-codegen
issue tracker in the future - or DM me on Slack in the next few weeks to discuss alternatives)
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 mean in a transform script as we already have here:
func transformFleetPaths(schema *Schema) { |
My main concern is that then we have additional code to maintain there. (Looking at the existing code, I can't easily figure out what it is achieving. But probably it is also fixing some issues with the upstream spec). It seems more easily maintainable to have the ugly instantiation logic as in this PR.
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.
It seems more easily maintainable to have the ugly instantiation logic as in this PR.
There's ugly instantiation logic (like in this PR), but very similar spec code results in an unusable generated client (elastic/kibana#197153). There's oapi-codegen issues (e.g oapi-codegen/oapi-codegen#2018) around this, but we need a path forward now.
None of this should be a blocker for this PR, but there seems to be three options here (in order of preference):
- oapi-codegen generates a valid client for anonymous polymorphic types
- Kibana generates a spec using only named types within a polymorphic container
- The provider rewrites the Kibana spec to only use names types within a polymorphic container
Why does this relate to this anonymous type? Well if we're going to use only named types in some specific situations then it's not much more effort to use named types in all situations since it avoid this ugliness (it's also really hard to figure out compile time errors if the types don't match exactly so it's not just ugliness). IMO, ideally the Kibana spec would avoid these anonymous types altogether, even if that happens in the future though the provider likely needs to solve this problem sooner. Maybe it's worth looking into solving it within the Kibana repo (I haven't looked at their spec generator at all), but I suspect it's easier to do so within transform_schema
in the short term.
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.
One option we've got is that we may be able to use OpenAPI Overlay which may be a little more "scalable" as a solution to make the extraction a little more straightforward to maintain
@jamietanna FWIW the JSON path library used in oapi-codegen doesn't support at least some valid selectors (e.g $..properties[?(@.nullable==true)]
). I'd like to look at widening the support if I had some time, but at the moment it's a non-starter for us.
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.
Having the Kibana spec be generated with named refs is the way to go long-term. I think the overlay files could also be a good temporary solution, it seems more readable compared to the programmatic transformers.
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 think the overlay files could also be a good temporary solution
I mean in theory, sure. They don't appear to support the selectors we need to make use of though.
examples/resources/elasticstack_kibana_maintenance_window/resource.tf
Outdated
Show resolved
Hide resolved
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.
LGTM, some small nits.
Co-authored-by: Toby Brain <[email protected]>
c242820
to
5aad68a
Compare
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.
LGTM, thanks!
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 did QA the CRUD operations and wanted to share my findings. Overall looking good, but I did stumble across an unexpected issue and some nits that are worth addressing.
Here's what I found:
- It's the first time I'm trying
terraform import
, so there is a big chance I'm doing something wrong. I have a MW resource in my .tf file calledmy_maintenance_window
, and I have a MW created via UI.
terraform import elasticstack_kibana_maintenance_window.my_maintenance_window default/7f634a31-0895-41d0-ac2d-7d2b2de90643
runs succesfully.terraform apply
throws with:
elasticstack_kibana_maintenance_window.my_maintenance_window: Refreshing state... [id=/7f634a31-0895-41d0-ac2d-7d2b2de90643]
╷
│ Error: Unexpected status code from server: got HTTP 404
│
│ with elasticstack_kibana_maintenance_window.my_maintenance_window,
│ on maintenance-window.tf line 1, in resource "elasticstack_kibana_maintenance_window" "my_maintenance_window":
│ 1: resource "elasticstack_kibana_maintenance_window" "my_maintenance_window" {
│
│ {"statusCode":404,"error":"Not Found","message":"Not Found"}
╵
nits:
- I used
duration = 1d
and it throws, should/can we catch it sooner? After updating to "24h" it works.
│ Error: Provider produced inconsistent result after apply
│
│ When applying changes to elasticstack_kibana_maintenance_window.my_maintenance_window, provider "provider[\"registry.terraform.io/elastic/elasticstack\"]" produced an unexpected new value: .custom_schedule.duration: was
│ cty.StringVal("1d"), but now cty.StringVal("24h").
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
- Trying the example resource in the description, it shows
~timezone = "UTC" -> (known after apply)
when updating, even if I'm changing something else. Is this expected? - I used
duration = "1w"
and the error message doesn't seem to be formatted correctly (see %!s()) at the end
│ Error: expected value to be a valid alerting duration
│
│ with elasticstack_kibana_maintenance_window.my_maintenance_window,
│ on maintenance-window.tf line 1, in resource "elasticstack_kibana_maintenance_window" "my_maintenance_window":
│ 1: resource "elasticstack_kibana_maintenance_window" "my_maintenance_window" {
│
│ This value must be a valid alerting duration in seconds (s), minutes (m), hours (h), or days (d) %!s(<nil>)
Thanks for the review @jcger !
This was a bug in the read logic. The ID was being saved with the I fixed this in the last commit. Now the maintenance window ID and the space ID are saved correctly and shouldn't affect
This is an API problem. Since the final value is different than what was in the I created an issue for this: elastic/kibana#234019
This is expected. timezone, when not defined, is a computed value.
Fixed in the last commit, good catch! |
Closes elastic/kibana#197021
Summary
This PR introduces a new Maintenance Window resource.
Other changes:
transform_schema.go
to accept PATCH since the MW API does not supportPUT
..gitignore
.Running tests
(With Kibana no SSL running.)
Testing instructions
The fastest way is to have Kibana running without SSL and then start the provider in debug mode.
I follow the instructions here for VS Code with a slightly different
launch.json
file.This generates a
TF_REATTACH_PROVIDERS
that can then be used in the console to apply a resource, such as the one in the section below.Something like:
If the command runs successfully you can then go to Kibana and make sure the maintenance window that was just created has all the files defined in the
.tf
file.Example resource