CustomResourceDefinitions should support maps in OpenAPI schema

[ash2k]

What happened:
Currently it is not possible to set additionalProperties in a CRD validation section of the spec to a schema. If set, validation rejects such CRD. That means OpenAPI schema for CustomResourceDefinitions does not support describing fields that are "maps" i.e. objects that do not have a pre-defined set of properties. Examples of such fields are labels and annotations in object metadata. For more context please see #58746.

What you expected to happen:
It should be possible to describe a schema for a field that is a map[string]string equivalent in a Go struct (or a map[string]<anything valid here>/etc where keys are not known in advance). For example to restrict the keys that can be used - see https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set.

/kind bug
/sig api-machinery
/area custom-resources
/cc @sttts @nikhita

[nikhita]

/cc @deads2k @liggitt @mbohlool @enisoc @jennybuckley

[nikhita]

There is a workaround to this:

JSONSchema has something called as patternProperties, which we allow now. patternProperties is just like properties but it allows you to specify a regex for the keys. Essentially, it means "if there is a key satisfying this regex, it's value should follow this schema".

It should be possible to describe a schema for a field that is a map[string]string equivalent in a Go struct (or a map[string]/etc where keys are not known in advance).

I understand that having a map means that you don't know about the keys in advance but you can modify the regex to support all keys.

Hijacking some comments from the previous thread #58746:

@sttts wrote:

We generally do not put constraints on unknown fields through-out the kube API.

Using patternProperties shows that the author is somewhat aware of the fields/keys. The line is kind of blurry here.

@ash2k wrote:

For CRDs the story is different - if a forward compatibility issue is discovered, the schema can be easily updated on the CRD. Nobody is affected - no downtime required, little and trivial amount of work is needed and the risk is minimal.

Like @ash2k mentioned, maybe we should make it the responsibility of the author to make sure that forward compatibility exists for patternProperties in this case? If they are using patternProperties, they are aware of what kind of fields can come in and if resources are invalidated later, it is the author's responsibility to fix the regex because they had specified the kind of fields that can be used.

In a gist,

• properties = If there is a key from these known keys, it's value should follow this schema.
• patternProperties = If there is a key satisfying this regex, it's value should follow this schema.
• additionalProperties = Any new key not specified in properties and not matching the regex in patternProperties should have a value following this schema.

[nikhita]

Also, in #59972, we can make sure that we strip only those fields which are not present in properties and which do not satisfy the regex in patternProperties - since the author had specified the regex for the keys and we should end up respecting it (and not stripping the fields/keys).

[anguslees]

I'm lost in the background discussion in the other bug, but I see a few mentions of workarounds .. could someone please summarise for me what I should put in my CRD schema now so a map[string]string is accepted by k8s 1.9+ validation? Or maybe this is currently impossible, and I need to remove the schema until a future fix is rolled out?

[ash2k]

@nikhita

There is a workaround to this:

JSONSchema has something called as patternProperties, which we allow now. patternProperties is just like properties but it allows you to specify a regex for the keys. Essentially, it means "if there is a key satisfying this regex, it's value should follow this schema".

It should be possible to describe a schema for a field that is a map[string]string equivalent in a Go struct (or a map[string]/etc where keys are not known in advance).

I understand that having a map means that you don't know about the keys in advance but you can modify the regex to support all keys.

I'm getting an error while trying to use patternProperties:

PatternProperties: map[string]apiext_v1b1.JSONSchemaProps{
	".*": {
		Type:      "string",
		MaxLength: int64ptr(63),
		Pattern:   "^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$",
	},
},

Error: Forbidden: patternProperties is not supported. I'm on kube 1.9.2.

https://github.com/kubernetes/kubernetes/blob/v1.9.2/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go#L309-L311

[nikhita]

Error: Forbidden: patternProperties is not supported. I'm on kube 1.9.2.

Er, yes, that's because OpenAPI v3 does not support patternProperties even though go-openapi does. 😕

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#schemaObject

https://docs.google.com/spreadsheets/d/1Mkm9L7CXGvRorV0Cr4Vwfu0DH7XRi24YHPiDK1NZWo4/edit#gid=0

[ayushpateria]

I might have a solution to this, the problem with using additionalProperties is that of forward-compatibility, we can't really use it with schemas that may be added with new fields later on. But to support maps what we can do is, we can allow it on the definitions that have only additionalProperties and no properties. When the author is specifying additionalProperties and not properties, we can assume that those fields are simply maps and wouldn't ever have a schema. Even for native types, I see in swagger.json wherever there is additionalProperties there is no properties.

[tamalsaha]

This is a common type when CRDs have nodeSelector or selector type fields. If this validation is not enforced, then any bad user input can break a controller in a cluster. Say, some CRD has a spec.nodeSlector field which is of map[string]string type. Now, user runs kubectl apply on a input like below:

spec:
  nodeSelector:
    ingress: true

This breaks controller for this CRD. Because now both List and Watch calls can't be unmarshaled . The only way to identify this error is from controller logs. Then user has to manually find the offending crd object and fix that to recover.

So, we would like to see this supported in CRD schema.

[sttts]

Added a milestone. IMO we have to solve this in 1.11. Who has capacity to work on this?

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Issue Needs Approval

@ash2k @kubernetes/sig-api-machinery-misc

Action required: This issue must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 7 days, the issue will be moved out of the v1.11 milestone.

Issue Labels

• sig/api-machinery: Issue will be escalated to these SIGs if needed.
• priority/important-soon: Escalate to the issue owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
• kind/bug: Fixes a bug discovered during the current release.

Help

• Additional instructions
• Commands for setting labels

[ayushpateria]

@sttts I'm interested. This is also in my gsoc proposal.

[yingqiaoshop]

There is a workaround to this:

JSONSchema has something called as patternProperties, which we allow now. patternProperties is just like properties but it allows you to specify a regex for the keys. Essentially, it means "if there is a key satisfying this regex, it's value should follow this schema".

It should be possible to describe a schema for a field that is a map[string]string equivalent in a Go struct (or a map[string]/etc where keys are not known in advance).

I understand that having a map means that you don't know about the keys in advance but you can modify the regex to support all keys.

Hijacking some comments from the previous thread #58746:

@sttts wrote:

We generally do not put constraints on unknown fields through-out the kube API.

Using patternProperties shows that the author is somewhat aware of the fields/keys. The line is kind of blurry here.

@ash2k wrote:

For CRDs the story is different - if a forward compatibility issue is discovered, the schema can be easily updated on the CRD. Nobody is affected - no downtime required, little and trivial amount of work is needed and the risk is minimal.

Like @ash2k mentioned, maybe we should make it the responsibility of the author to make sure that forward compatibility exists for patternProperties in this case? If they are using patternProperties, they are aware of what kind of fields can come in and if resources are invalidated later, it is the author's responsibility to fix the regex because they had specified the kind of fields that can be used.

In a gist,

• properties = If there is a key from these known keys, it's value should follow this schema.
• patternProperties = If there is a key satisfying this regex, it's value should follow this schema.
• additionalProperties = Any new key not specified in properties and not matching the regex in patternProperties should have a value following this schema.

Maybe, the OpenAPI specification could allow a pattern to be defined on the key of a dictionary.

[sttts]

Maybe, the OpenAPI specification could allow a pattern to be defined on the key of a dictionary.

There is no patternProperties in OpenAPI v3. It could be used to restricts keys.