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

CustomResourceDefinitions should support maps in OpenAPI schema #59485

Closed
ash2k opened this Issue Feb 7, 2018 · 11 comments

Comments

Projects
None yet
8 participants
@ash2k
Copy link
Member

ash2k commented Feb 7, 2018

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

This comment has been minimized.

Copy link
Member

nikhita commented Feb 7, 2018

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

nikhita commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

anguslees commented Feb 23, 2018

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

This comment has been minimized.

Copy link
Member Author

ash2k commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
Member

nikhita commented Feb 26, 2018

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

sullerandras added a commit to sullerandras/sealed-secrets that referenced this issue Mar 5, 2018

Remove validation from crd
because it seems like it's not supported by kubernetes 1.9: kubernetes/kubernetes#59485 (comment)
@ayushpateria

This comment has been minimized.

Copy link
Contributor

ayushpateria commented Mar 23, 2018

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 comment has been minimized.

Copy link
Member

tamalsaha commented Apr 10, 2018

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

This comment has been minimized.

Copy link
Contributor

sttts commented Apr 10, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 10, 2018

[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
@ayushpateria

This comment has been minimized.

Copy link
Contributor

ayushpateria commented Apr 10, 2018

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

k8s-github-robot pushed a commit that referenced this issue Apr 11, 2018

Kubernetes Submit Queue
Merge pull request #62333 from sttts/sttts-crd-validation-string-maps
Automatic merge from submit-queue (batch tested with PRs 59027, 62333, 57661, 62086, 61584). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

CustomResources: in OpenAPI spec allow additionalProperties without properties

This implements @ayushpateria's idea #59485 (comment).

With this PR it becomes possible to specify `map[string]Interface{}` non-object types, e.g. `map[string]string` for selectors. On the other hands, "normal" objects use `properties`, mutually exclusively to `additionalProperties`. This way we avoid a conflict with Kubernetes API conventions that unknown objects fields are dropped.

Fixes #59485

```release-note
Allow additionalProperties in CRD OpenAPI v3 specification for validation, mutually exclusive to properties.
```

k8s-publishing-bot added a commit to kubernetes/apiextensions-apiserver that referenced this issue Apr 11, 2018

Merge pull request #62333 from sttts/sttts-crd-validation-string-maps
Automatic merge from submit-queue (batch tested with PRs 59027, 62333, 57661, 62086, 61584). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

CustomResources: in OpenAPI spec allow additionalProperties without properties

This implements @ayushpateria's idea kubernetes/kubernetes#59485 (comment).

With this PR it becomes possible to specify `map[string]Interface{}` non-object types, e.g. `map[string]string` for selectors. On the other hands, "normal" objects use `properties`, mutually exclusively to `additionalProperties`. This way we avoid a conflict with Kubernetes API conventions that unknown objects fields are dropped.

Fixes #59485

```release-note
Allow additionalProperties in CRD OpenAPI v3 specification for validation, mutually exclusive to properties.
```

Kubernetes-commit: f3cad465d53a77c7b6b1f073b79755d4644d7011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment