/retest

Request for an additional test. lgtm otherwise

/approve no-issue

Test added.

/approve no-issue

/lgtm

@smarterclayton you should look.

Another alternative is to say our "JSON Schema" differs slightly from the official spec, such as having to always use a string array in fields such as this, even if you only have one string (assuming there is no semantic difference between a string and an array with one entry). OpenAPI does this too; their docs explain the minor differences between OpenAPI Schema and the JSON Schema spec.

The ability to exert such control over our own API was after all one of the justifications for importing the structs. If we wanted to match the spec 100%, it may have made more sense to just store the JSON Schema as a string.

@sttts PR needs rebase

@enisoc I am still not convinced that something like StringArrayOrString is bad. What are the issues that everybody expects?

Short term in fact we could just fall back to "string only". This is what OpenAPI v3 does and we only support v3 now anyway. But IMO this is a very stupid restriction in the spec and I expect very much hat v3+ will support more than one type. But by that time we can reconsider.

@sttts I was mostly just trying to give a hypothetical answer to the question, "What's the alternative?" I'm not familiar enough with the pain experienced with IntOrString to have an opinion on whether StringArrayOrString will cause more pain.

@smarterclayton You mentioned pain with OpenAPI. Were you referring to this thread? It seems like that issue will go away eventually when we switch to OpenAPI v3.

It seems like that issue will go away eventually when we switch to OpenAPI v3.

We still don't have the type field as array in OpenAPI v3. We have dropped this now from our JSON Schema types. So the unmarshaler will complain if you try an array. If v4 finally adds arrays, we have to reconsider.

@sttts In the thread, @mbohlool suggested you could work around the specific issue raised there with oneOf. Would that not work?

@mbohlool suggested you could work around the specific issue raised there with oneOf. Would that not work?

We are on the other side of the problem: we offer the API, but our users won't be able to use an array for types. Actually, it's not oneOf, but anyOf that can replace the array, it's just ugly. Here we did a comparison of all those variants of JSON Schema: https://docs.google.com/spreadsheets/d/1Mkm9L7CXGvRorV0Cr4Vwfu0DH7XRi24YHPiDK1NZWo4/edit?usp=sharing

Sorry if I'm creating confusion. I didn't mean to suggest that oneOf/anyOf could fix the problem faced by CRD validation.

I was trying to say, if it's true that (1) the problem in that mailing list thread is the only known pain caused by IntOrString, and (2) the problem in that thread could be resolved by going to OpenAPI V3, then it would imply that there are actually no known, long-term problems with IntOrString. Therefore we would have no reason to suspect adding StringArrayOrString will cause long-term pain.

cc @mbohlool @jpbetz

@sttts We have absolutely no obligation to allow all of JSON / JSONSchema. The K8s API uses its own conventions, and even some of those we regret. Has anyone attempted to determine which features are exercised by the current K8s API surface?

Are there any examples of StringArrayOrString in the K8s core v1 API? IIRC, we explicitly decided not to support that for command, for instance.

We do have the problem that not all of the K8s API can be described by the current OpenAPI spec. At least that was true when I last checked.

cc @mbohlool

We have absolutely no obligation to allow all of JSON / JSONSchema. The K8s API uses its own conventions

That's a good second view point. We use OpenAPI v3 to decribe kube's API (lower bound), but here we want to define the JSON Schema subset to specify APIs following the kube API conventions. Wondering whether we can derive any upper bound from the conventions we envisions also for user-provided CRDs. Sounds like a nice topic for bachelor thesis (hint!). kubeapi-lint.

no known, long-term problems with IntOrString

At least not from the spec point of view: {anyOf: [{type: "string"}, {type: "integer"}]} should be equivalent to {type:["string", "integer"]} in v3, not possible in v2.

kubeapi-lint.

#44005

Let me see if I understand the problem correctly. User provides a validation schema that we will use to validate CRs. We want them to be able to provide multiple types. Currently we support OpenAPI v3 schema in CRD validation, so they can do things like '{anyOf: [{type: "string"}, {type: "integer"}]}' but they can't do {type:["string", "integer"]}.

If the above is correct, what if we do a post-processing on the provided spec and convert {type:["string", "integer"]} to {anyOf: [{type: "string"}, {type: "integer"}]} so it would be a valid OpenAPI v3 spec? I am not a fan of this solution but if we must compromise, it is the closest one to a good solution. I don't like it because it is just a nice to have feature and we are hacking around to let user do it. Also if we don't do it now, it would be always backward compatible to add it. OpenAPI folks decided not to do this as it make life harder for tools to consume it. The whole discussion can be found in this issue (and the referenced ones).

Regarding StringArrayOrString, if the only problem with IntOrString is OpenAPI v2 spec, we should be able to proceed with it (The number of open and closed issues with IntOrString, however, make me nervous). I think we are going to support v3 soon (not soon enough for 1.8, but soon). In v2, we should just represent it as Object. That means we don't do any validation on it. We are post-processing our spec to do that for some types here.

@mbohlool we could do that transformation. But as we restrict ourselves to OpenAPI v3 now in CRD validation, the problem is no problem anymore. I.e. we don't need StringArrayOrString. Our approach of validation was that we wanted to accept raw JSON Schema and then refuse certain constructs if they don't fall into OpenAPI v3. But this is essentially cosmetics and only a matter of another error message. So we switched to plain string for type, away from StringArrayOrString.

If we want common tooling, such as get, describe, schema validation, apply, etc., to work across built-in types and CRDs, then CRDs need to follow K8s API conventions, and the provided schema needs to support the same extensions we're adding for the built-in types.

cc @pwittrock

and the provided schema needs to support the same extensions we're adding for the built-in types.

This is on the roadmap, together with a common definition namespace such that you can reference e.g. a PodSpec from a CRD. Would be great to get a proposal done for that early in the 1.9 dev cycle.

Before looking in detail, let me say that IntOrString is terrible and we should not add more things like it :)

Why do we need to represent the jsonschema in go types at all? Do we have go code that operates on it? Why not store it in a RawExtension type (i.e., just pass-through the json).

On IntOrString: when we made it, we thought we were being nice to humans who were going to write yaml/json by hand directly. But in hindsight, I think it was not worth it. It confuses everyone who touches it. No type spec system represents it well, excepting OpenAPI3, which we don't use yet. Automation systems don't deal with it well, either, e.g. Google's Deployment Manager. It's especially confusing that "12" and 12 weren't the same thing for a while, but now I think they are, but "12%" still isn't.

There was really no good reason not to make a struct {name, number} for port identification. An extra level in a yaml file really isn't a big deal.

Basically, it violates the principle of least surprise for all the automation systems that have to deal with the API, without adding much benefit for users who are directly writing API objects.

this is no longer needed, correct?

Correct.