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

OpenAPI spec (swagger.json) integer type valid ranges not specified #105533

Open
arobertn opened this issue Oct 7, 2021 · 19 comments
Open

OpenAPI spec (swagger.json) integer type valid ranges not specified #105533

arobertn opened this issue Oct 7, 2021 · 19 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@arobertn
Copy link

arobertn commented Oct 7, 2021

In the generated OpenAPI swagger.json, integer types such as ContainerPort have clearly documented valid ranges in the human-readable comments, but these are not actually specified in the API JSON using the minimum/maximum/exclusiveMinimum/exclusiveMaximum keywords. This creates problems for statically-typed languages used to generate for the API because they cannot perform this type checking. They can't even make a choice as to whether to represent a given integer quantity as a "natural" or "unsigned" type or a signed one.

I assume the range information is available in some form within the API server that generates the swagger, so would it be possible to include it?

@arobertn arobertn added the kind/bug Categorizes issue or PR as related to a bug. label Oct 7, 2021
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 7, 2021
@arobertn
Copy link
Author

arobertn commented Oct 7, 2021

For example, to:

    "io.k8s.api.core.v1.ContainerPort": {
      "description": "ContainerPort represents a network port in a single container.",
      "properties": {
        "containerPort": {
          "description": "Number of port to expose on the pod's IP address. This must be a valid port number, 0 < x < 65536.",
          "format": "int32",
          "type": "integer"
        },

should be added:

          "exclusiveMinimum": "0",
          "exclusiveMaximum": "65536"

@neolit123
Copy link
Member

neolit123 commented Oct 8, 2021

      "exclusiveMinimum": "0",
     "exclusiveMaximum": "65536"

the spec says that these fields are bools:
https://swagger.io/docs/specification/data-models/data-types/
aren't minimum and maximum the fields that should be set?

I assume the range information is available in some form within the API server that generates the swagger, so would it be possible to include it?

the Go struct -> OpenAPI generator could include the type limits from the type information (e.g. uint16 -> 0-65535), but that would be artificial in some cases, since some integer fields have actual limits.

for actual limits the generator would need a way to extract those from tags above a field. field limits are currently only documented loosely in the comments above a field in a written form.
e.g. see some of the int values in Probe:
https://pkg.go.dev/k8s.io/api/core/v1#Probe
this is a more complicated approach and probably needs a KEP:
https://github.com/kubernetes/enhancements/tree/master/keps

/sig api-machinery
/kind feature
/remove-kind bug

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/feature Categorizes issue or PR as related to a new feature. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. kind/bug Categorizes issue or PR as related to a bug. labels Oct 8, 2021
@arobertn
Copy link
Author

arobertn commented Oct 9, 2021

the spec says that these fields are bools:

Right. It must be that exclusiveMinimum/Maximum are meant to be used in conjunction with minimum/maximum to specify exclusive/inclusive.

Including the type limits from uint16 etc. would not be perfect, but it would be noticeably better than now since the signed/unsigned distinction and basic ranges can be used to determine appropriate static types. It could be made clear in the top-level OpenAPI doc that these are lower and upper bounds not limits.

@fedebongio
Copy link
Contributor

/assign @Jefftree @jpbetz
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 12, 2021
@Jefftree
Copy link
Member

Jefftree commented Oct 13, 2021

This is already supported in CRDs, and it doesn't seem too difficult to add into the OpenAPI generator for built-ins. Something like

// +minimum=X
// +maximum=Y
Foo int32

for the go structs of native types. Deriving the limits from some kind of NLP parsing of the comments feels quite error prone and I would heavily favor a more simplistic approach with the added comment tags.

@apelisse @jpbetz do you see any drawbacks of adding this? These tags are already supported by OpenAPI v2

@apelisse
Copy link
Member

for the go structs of native types. Deriving the limits from some kind of NLP parsing of the comments feels quite error prone and I would heavily favor a more simplistic approach with the added comment tags.

@apelisse @jpbetz do you see any drawbacks of adding this? These tags are already supported by OpenAPI v2

You're right, it's a great idea and it's part of the plan, it's just not that easy to implement. We've clarified a lot of things recently with defaults so I think we'll be able to do it now/next.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 11, 2022
@apelisse
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 19, 2022
@apelisse
Copy link
Member

Still definitely interested in doing that, but we don't have the bandwidth right now.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2022
@apelisse
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 18, 2022
@vaibhav2107
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 15, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 13, 2022
@apelisse
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 16, 2022
@apelisse
Copy link
Member

/lifecycle frozen

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jan 19, 2024
@Jefftree
Copy link
Member

fyi kubernetes/enhancements#4153 should resolve this issue
cc @alexzielenski

@seans3
Copy link
Contributor

seans3 commented Feb 9, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

10 participants