Spec of API machinery's Quantity is wrong (incomplete)

[sttts]

controller-tools/pkg/crd/known_types.go

Line 58 in 538d4b2

Type: "string",

specifies quantities as string, while they allow ints and numbers https://github.com/kubernetes/kubernetes/blob/5713c22eecff4610026643fbd3d37c33a43c168d/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity_test.go#L219. We cannot express int-or-number-or-string in structural schemas, but we have x-kubernetes-int-or-string. That would make the spec more complete than today.

[marun]

/assign

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[marun]

/remove-lifecycle stale

[DirectXMan12]

huh?

The code in k8s.io/apimachinery says otherwise: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L393

[sttts]

Isn't https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L646 the relevant line? Quotes are optional. The OpenAPI spec you link to is incomplete (OpenAPI v2 ...).

[DirectXMan12]

Interesting... was that intended? Generally our OpenAPI is less restrictive that what we accept (i.e. we validate more than what our OpenAPI says), but in this case the OpenAPI is more restrictive that what we accept.

[DirectXMan12]

Technically, neither is right (it should be int or (string that matches the quantity format)), but I guess since we accept raw integers in the parsing logic we can move forward with that.

[sttts]

IMO the v2 spec is just wrong. A published spec should never be too restrictive. It may be incomplete server side validation will do the final check anyway.

I agree, we could add a regex to specify string that matches the quantity format if we want to. Maybe we actually should to avoid unmarshalling errors in Go code.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[dthomson25]

@sttts and @DirectXMan12, isn't it true that a quantity can be float (i.e. 1.2 not "1.2")? If that's the case, then isn't a stringOrInt is too restrictive?

[DirectXMan12]

I think when you specify a decimal, it needs to be in quotes, otherwise other systems will parse it as a float, voiding the entire purpose of resource.Quantity. UnmarshalJSON technically accepts unqualified floats (like it accepts unqualified ints), but I'd guess that's probably a bug.

@sttts thoughts?

[jessesuen]

I think when you specify a decimal, it needs to be in quotes

You can specify decimal without quotes. The following valid:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx-deploy
spec:
  template:
    spec:
      containers:
      - name: nginx
        image: nginx:latest
        resources:
          requests:
            cpu: 0.5
...

The normalized version of 0.5 cpu eventually becomes "500m".

In our CRD, when generating the OpenAPI validation spec for a field which is of resource.Quantity, controller-tools incorrectly generates:

properties:	
limits:	
  additionalProperties:	
    anyOf:	
    - type: integer	
    - type: string	
    pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$	
    x-kubernetes-int-or-string: true	
  type: object	
requests:	
  additionalProperties:	
    anyOf:	
    - type: integer	
    - type: string	
    pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$	
    x-kubernetes-int-or-string: true	
  type: object	

Which does not allow decimals. To work around this, we currently have to surgically remove the resource.Quantity validation.

[sttts]

Technically float is allowed by quantities. The problem here is that CRDs don't do the conversion of 0.5 to "500m" on the server-side, but probably should. For that x-kubernetes-quantity: true might make sense.

[DirectXMan12]

In our CRD, when generating the OpenAPI validation spec for a field which is of resource.Quantity, controller-tools incorrectly generates:

Yeah, I'm aware that it's incorrect, but I wanted to check to see what the story was here, because, as noted by @sttts, since CRDs don't get normalized, you're gonna read back 0.5, which doesn't get round-tripped through many systems properly (because floats don't round-trip).

For that x-kubernetes-quantity: true might make sense.

or format: quantity

We probably need to drop the int-or-string designation here, but I'm not really certain how to proceed -- if we can't say "intorstring", the quantity declaration becomes non-structural (i.e. according to kubernetes CRD rules we're not allowed to say "int or string or float").

it should currently accept "0.5" (i.e. float in quotes), which is a workaround.

[DirectXMan12]

@jessesuen I'm curious as to what your desired behavior would be here -- non-structural means no server-side defaulting, etc, which seems like a big trade-off for accepting unquoted floats.

[DirectXMan12]

(no intention of sounding passive-agressive here -- I'm just curious about your usecase)

[dthomson25]

@jessesuen and I are maintainors of a project called Argo Rollouts, and it has a CRD called a Rollout that replaces the Deployment object with other deployment strategies like blue-green or canary. For a user to start using a Rollout, they only need to change the apiVersion and kind and add a new deployment strategy. If a user has a float value for their container's limits or requests, then the validation rejects their rollout. To work around this, we removed the validation for the request and limit fields so we can have the same feature parity as the Deployment object.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[DirectXMan12]

@sttts is there any way here then for us to have a valid structural deployment object embedded in a CRD? It kinda seems like the answer is "no".

[sttts]

@sttts is there any way here then for us to have a valid structural deployment object embedded in a CRD? It kinda seems like the answer is "no".

No, nobody tried to solve the type reference problem in schemas.

[DirectXMan12]

Ah, slightly different question:

the schema for anything containing resource.Quantity is non-structural, correct?

[sttts]

the schema for anything containing resource.Quantity is non-structural, correct?

If you use x-kubernetes-int-or-string: true, it is structural. If you try to add floats with some anyOf construct it is non-structural.

[DirectXMan12]

yeah, so you can't represent what vanilla kubernetes accepts as a structural OpenAPI schema, since kubernetes accepts floats

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jessesuen]

/reopen

[k8s-ci-robot]

@jessesuen: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.