Skip to content
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

Fixing schema definitions bugs #202

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

MbolotSuse
Copy link
Contributor

@MbolotSuse MbolotSuse commented Apr 26, 2024

Overview

This is related to rancher/rancher#45158, and resolves two of the issues identified in the issue summary. Specifically:

  • There are some resources (e.x. generateKubeconfigOutput) which are added as custom schemas - these don't represent real k8s resources. However, since they are still top-level schemas, they need definitions.
  • Some resources (e.x. monitoring.coreos.com.alertmanagerconfig from the rancher-monitoring chart) are not in the preferred version of the group, but are in some version of the group. monitoring.coreos.com.alertmanagerconfig, for example, appears in the v1alpha1 version, but not in the v1 version which is the preferred version of the group.

This does not address the third issue - this should be addressed by the work on rancher/rancher#45157, since these resources represent CRDs that have no typing information in kubernetes.

Solution Detail

  • We now add the baseSchema configured for the server to the SchemaDefinitionHandler. This contains the configured, built-in schemas (such as generateKubeconfigOutput).
  • When processing a request, if the schema is a baseSchema (as determined by trying to lookup the schema from the baseSchemas), then we extract the definition from the schema's resourceFields rather than from the openapi models.
    • In these cases, we still extract the definition from the schema on the request rather than the baseSchema (which is just used to determine if this resource is a base schema). This is done in case the schema has been transformed for this user - baseSchemas are setup-wide, where the schemas on the request are specific to the user.
    • This approach requires that the schema still has the resource fields set. For this reason, these resources still have resourceFileds in /v1/schemas. Since the impact of this is relatively low, this wasn't resolved in this PR.
    • This transformation is done at the time the schema is called, rather than at refresh time, so there's some overhead in processing these requests. However, that overhead is comparable (and probably lower) to what we do in recursively gathering the schemas on a proper k8s resource.
    • This approach requires that we parse the type field on the resource fields for cases of complex types like map[string]. This is done using an import from wrangler, which is used for similar purposes in rancher
  • The logic for preferred group/version was tweaked slightly.
    • In the current/master state, resources are only added when they are in the preferred version for a group.
    • In the new state, we add a resource if one of the two is true:
      • The resource is in the preferred version for a group
      • We don't have a model for that schema yet
    • This ensures that if we have a preferred version of a resource, that version is used (since it is always added, it will override any other non-preferred version). However, if none exists, we still have a version of the resource, so we can still get a legitimate definition.

Notes

Tests were added for the above cases to maintain coverage.

@MbolotSuse MbolotSuse requested a review from tomleb April 26, 2024 14:23
@MbolotSuse MbolotSuse requested a review from a team as a code owner April 26, 2024 14:23
@MbolotSuse MbolotSuse force-pushed the schema-fix-1 branch 2 times, most recently from 61744be to 2c2f18c Compare April 26, 2024 14:38
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, 1 question and 1 suggestion

pkg/schema/definitions/handler.go Outdated Show resolved Hide resolved
pkg/schema/definitions/handler.go Show resolved Hide resolved
Fixes two bugs with the schema definitions:
- Adds resources that are a part of the baseSchema (but aren't k8s resources).
- Adds logic to handle resources which aren't in a prefered version, but
  are still in some version.
Copy link
Contributor

@nflynt nflynt left a comment

Choose a reason for hiding this comment

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

👍

@MbolotSuse MbolotSuse merged commit ce206c9 into rancher:master May 15, 2024
1 check passed
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.

3 participants