Skip to content

Pull newer fleet schema #834

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

Merged
merged 9 commits into from
Oct 25, 2024
Merged

Pull newer fleet schema #834

merged 9 commits into from
Oct 25, 2024

Conversation

daemitus
Copy link
Contributor

@daemitus daemitus commented Oct 8, 2024

So the Fleet schema is getting pulled into Kibana's oas_docs folder now. Notable changes include:

  • all the refs are flattened and gone.
  • discriminators are gone, and I'm pretty sure not supported by the kbn-schema package yet
  • operationIds now look like /path/to/some/method#0 which not only break the generated code, are horrible to read.
  • application/json is now application/json; Elastic-Api-Version=2023-10-31

So far, I've updated the outputs paths, but tldr, all the paths need nested structs broken out and rewritten operationIDs.

I'm also hoping to eventually pull this generator down a folder and use it to spit out the other generated paths as they come available. SLOs, dataviews, spaces, and some of alerting are already there iirc.

@tobio
Copy link
Member

tobio commented Oct 8, 2024

operationIds now look like /path/to/some/method#0

This is largely unworkable in the published spec. There's an internal discussion happening already about enforcing actual operation ids.

Are any of the other changes causing issues with code generation? In general we should be aiming to fix these upstream as much as possible since ideally we're not the only consumer of the API spec.

I'm not sure how much you can ignore the operationID issue at the moment, but I certainly wouldn't put effort into rewriting those since we'll be fixing those in the source.

@daemitus
Copy link
Contributor Author

daemitus commented Oct 8, 2024

Regarding opIDs, nothing major went into it. just a big list with some hardcoded values roughly similar to what was there before. See here

I added a discriminator to the output union here but we can definitely omit it and just deserialize a struct{ Type string } instead to get the value.

Various nested structs have refs made as we use them, otherwise the only real "issue" is some of the complicated anyOfs that we really don't care about in the implementation. For example: the kafka output properties compression_level, connection_type, password, and username look roughly like this:

  password: schema.nullable(
    schema.conditional(
      schema.siblingRef('secrets.password'),
      secretRefSchema,
      schema.never(),
      schema.conditional(
        schema.siblingRef('username'),
        schema.string(),
        schema.string(),
        schema.never()
      )
    )
  ),
        password:
          anyOf:
            - items: {}
              type: array
            - type: boolean
            - type: number
            - type: object
            - type: string
          nullable: true
          oneOf:
            - not: {}
            - anyOf:
                - items: {}
                  type: array
                - type: boolean
                - type: number
                - type: object
                - type: string
              nullable: true
              oneOf:
                - type: string
                - not: {}

This definitely feels like the verification refs are ending up in the final schema, at a minimum having both anyOf and oneOf is likely wrong. For our purposes, we really don't care just give the raw value and see if it matches the user's input (once kafka is implemented).

The same is somewhat happening to the various package_policy vars fields. They are correctly oneOf bool, number, string, arrays, or secretRef. We could handle it appropriately, and check each of the 6 different possibilities, or just change it into a map[string]any and do how we were doing before.

@daemitus
Copy link
Contributor Author

daemitus commented Oct 9, 2024

Two errors that actually do look like they might be wrong upstream:

CreateAgentPolicy:
{"message":"[request body.supports_agentless]: expected value of type [boolean] but got [null]"}
{"message":"[request body.keep_monitoring_alive]: expected value of type [boolean] but got [null]"}

These are both listed as <field>?: boolean | null, when they should likely be just <field>?: boolean instead.

--- edit
Same from the output api:
request body.0.ca_sha256]: expected value of type [string] but got [null]

Might be a more prevalent problem that I previously thought.

@daemitus
Copy link
Contributor Author

daemitus commented Oct 10, 2024

Ok. We're passing tests as they were previously written.

Here's a detailed example of one of the nullable issues:

In the typescript spec:
  keep_monitoring_alive: schema.maybe(
    schema.oneOf([
      schema.literal(null),
      schema.boolean({
        defaultValue: false,
      }),
    ])
  ),

Becomes in the oas schema:
  keep_monitoring_alive:
    default: false
    nullable: true
    type: boolean

Causing the following error:
  [request body.keep_monitoring_alive]: expected value of type [boolean] but got [null]

Because it rendered to:
  KeepMonitoringAlive *bool `json:"keep_monitoring_alive"`

The fix:
  KeepMonitoringAlive *bool `json:"keep_monitoring_alive,omitempty"`

Now we could use the oapi nullable types to get around this potentially, output-options: { nullable-type: true }, which would entail more helpers basically; e.g. checking for known/unknown flavors of null.

Here is the list of changes necessary to get past all the tests in the workflow.

-- edit
Of course, the nullable types won't actually help for some of these errors, as ^ per the specific error message, its not actually nullable.

@daemitus daemitus mentioned this pull request Oct 17, 2024
@daemitus daemitus marked this pull request as ready for review October 18, 2024 19:37
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Are you able to open issues in the Kibana repo for the spec issues? The nullable behaviour at least sounds like a server bug?

It'd be nice to link the various transforms we're applying here back to an upstream issue so we can ideally track whether it continues to be required more easily.

@daemitus
Copy link
Contributor Author

Sure, let me go through the transforms again and see what I can open in Kibana.

@daemitus
Copy link
Contributor Author

I think I'm about done here. I gave the nullable types a test-run, and didn't seem to get much in the way of usefulness. At best, all we would gain is the ability to differentiate tf.Unknown to api.Unset. So long as the fields are properly nullable, not sending an unset null vs always sending null is effectively the same thing.

tobio
tobio previously approved these changes Oct 25, 2024
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM. Honestly not really actually, I intensely dislike that we have this code. The solution though, that looks good.

There's a lot of work here, it's really appreciated. It not only helps us keep track with the Fleet API changes but will be helpful with the wider Kibana spec in the future.

👍

@tobio tobio merged commit 78faa31 into elastic:main Oct 25, 2024
20 checks passed
@daemitus daemitus deleted the fleet-oas-oapi branch October 25, 2024 09:38
kjwardy pushed a commit to kjwardy/terraform-provider-elasticstack that referenced this pull request Nov 19, 2024
* wip

* lint

* fix downgrade

* fresh pull and documentation

* changelog

---------

Co-authored-by: Toby Brain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants