Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
373 stars 42 forks source link

i32 for port number #136

Closed sombralibre closed 1 year ago

sombralibre commented 1 year ago

Hi there, just a question about why does the struct that represent the ServiceBackendPort uses an i32 instead of an u16 for hold the port number; I've checked the definition for several versions of kubernetes and the definition is the same:

https://github.com/Arnavion/k8s-openapi/blob/0fdb6c66decf0a7794c77590b9078e854b24e917/src/v1_26/api/networking/v1/service_backend_port.rs#L10

Is there something that I missed?

I means whether the definition of the data wide for port number in the TCP/IP is 16 bits, It doesn't make sense that the port would be represented for an u16?

What would happens if someone don't pay attention to this, and ended passing a value over 65535 or a negative one? It will not be safe enough I assume.

Arnavion commented 1 year ago

Is there something that I missed?

Yes, you missed that this code is auto-generated from the OpenAPI spec. It's not like I made the conscious decision to represent port numbers as i32. :)

So, to that end, the OpenAPI spec says:

    "io.k8s.api.networking.v1.ServiceBackendPort": {
      "description": "ServiceBackendPort is the service port being referenced.",
      "properties": {
        "name": {
          "description": "name is the name of the port on the Service. This is a mutually exclusive setting with \"Number\".",
          "type": "string"
        },
        "number": {
          "description": "number is the numerical port number (e.g. 80) on the Service. This is a mutually exclusive setting with \"Name\".",
          "format": "int32",
          "type": "integer"
        }
      },
      "type": "object"
    }

"format": "int32"

At this point you might say it's a spec bug and would be resolved by specifying a better format. But even the API server code says:

https://github.com/kubernetes/kubernetes/blob/9b09d0600a69a2eb36b0d136465ccc3c179dacdb/pkg/apis/networking/types.go#L624-L636

Number int32

:shrug: Complain to upstream.

sombralibre commented 1 year ago

Yes, you're right, just after create this issue I went to the OpenApi specs and found the same.

I will scale the question to the openapi people.

Thanks a lot man.

Arnavion commented 1 year ago

Well, OpenAPI supports specifying integer ranges using minimum and maximum, but the Kubernetes OpenAPI spec is downstream of their golang code so they'd have to invent new annotations to express that. Besides, ServiceBackendPort::number is far from the only field that has this problem; searching for : Option<i32>, in this repo reveals all sorts of time durations and replica counts and percentages, and upstream would have to fix all of them for completeness.

Arnavion commented 1 year ago

Upstream issue: https://github.com/kubernetes/kubernetes/issues/105533

sombralibre commented 1 year ago

Upstream issue: kubernetes/kubernetes#105533

👍🏻