Skip to content
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

CRD cannot have a field named 'items' #68466

Closed
mfranczy opened this issue Sep 10, 2018 · 25 comments · Fixed by #76124
Closed

CRD cannot have a field named 'items' #68466

mfranczy opened this issue Sep 10, 2018 · 25 comments · Fixed by #76124
Assignees
Labels
area/custom-resources kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Milestone

Comments

@mfranczy
Copy link

Is this a BUG REPORT or FEATURE REQUEST?:
/kind bug

What happened:
I cannot have a resource which has a field named items it doesn't pass validation.
As an example I will modify sample-controller (https://github.com/kubernetes/sample-controller) and add items to artifacts/examples/example-foo.yaml:

apiVersion: samplecontroller.k8s.io/v1alpha1
kind: Foo
metadata:
  name: example-foo
spec:
  deploymentName: example-foo
  replicas: 1
  items: # <-- cannot be set
  - "foo"
  - "bar"

after executing:

kubectl create -f artifacts/examples/example-foo.yaml

I receive an error:

The Foo "example-foo" is invalid: []: Invalid value: map[string]interface {}{"apiVersion":"samplecontroller.k8s.io/v1alpha1", "kind":"Foo", "metadata":map[string]interface {}{"name":"example-foo", "namespace":"default", "generation":1, "creationTimestamp":"2018-09-10T10:31:05Z", "uid":"9f41ebca-b4e4-11e8-ab9b-525500d15501", "selfLink":"", "clusterName":""}, "spec":map[string]interface {}{"deploymentName":"example-foo", "items":[]interface {}{"foo", "bar"}, "replicas":1}}: validation failure list:
type in spec is required

Validation expects that I will set a type field as well, spec below doesn't return the error:

apiVersion: samplecontroller.k8s.io/v1alpha1
kind: Foo
metadata:
  name: example-foo
spec:
  deploymentName: example-foo
  replicas: 1
  items:
  - "foo"
  - "bar"
  type: array  # <-- with 'type' it works

After all, in my opinion, it is a bug. I think this connected to OpenApi schema validation, that items keyword should always be in pair with type.

What you expected to happen:
I would expect that I am able to use a field named items in CRD spec and it passes validation.
Similar to ConfigMap, it has an items field, https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#add-configmap-data-to-a-specific-path-in-the-volume.

How to reproduce it (as minimally and precisely as possible):
The easiest way is to use https://github.com/kubernetes/sample-controller,
change artifacts/examples/example-foo.yaml
from:

apiVersion: samplecontroller.k8s.io/v1alpha1
kind: Foo
metadata:
  name: example-foo
spec:
  deploymentName: example-foo
  replicas: 1

to:

apiVersion: samplecontroller.k8s.io/v1alpha1
kind: Foo
metadata:
  name: example-foo
spec:
  deploymentName: example-foo
  replicas: 1
  items:
  - "foo"
  - "bar"

then execute:

kubectl -f create artifacts/examples/crd-validation.yaml (it doesn't matter if you will change something here)

kubectl -f create artifacts/examples/example-foo.yaml

After mentioned steps, you should receive the error (btw you don't have to change the Foo structure to reproduce this)

Anything else we need to know?:
No.

Environment:

  • Kubernetes version (use kubectl version):
    Client Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.4", GitCommit:"5ca598b4ba5abb89bb773071ce452e33fb66339d", GitTreeState:"clean", BuildDate:"2018-06-06T08:13:03Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}
    Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.4", GitCommit:"5ca598b4ba5abb89bb773071ce452e33fb66339d", GitTreeState:"clean", BuildDate:"2018-06-06T08:00:59Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}
    Tested as well with k8s-1.11.0, the same problem occurred.
  • OS (e.g. from /etc/os-release): Fedora27
  • Kernel (e.g. uname -a) :4.17.19-100.fc27.x86_64
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 10, 2018
@mfranczy
Copy link
Author

@kubernetes/sig-api-machinery

@mfranczy
Copy link
Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 10, 2018
@roycaihw
Copy link
Member

cc @roycaihw @mbohlool

@liggitt
Copy link
Member

liggitt commented Sep 11, 2018

I suspect it might be related to this:

func (obj *Unstructured) IsList() bool {
field, ok := obj.Object["items"]
if !ok {
return false
}
_, ok = field.([]interface{})
return ok
}

edit: nevermind... that is nested under spec, so shouldn't trigger IsList detection

@liggitt
Copy link
Member

liggitt commented Sep 11, 2018

cc @sttts

@sttts
Copy link
Contributor

sttts commented Sep 11, 2018

The error comes from here:

res.AddErrors(errors.Required("type", o.Path))

Is this some kind of JavaScript speciality that slices are represented as objects with items and type?

@liggitt
Copy link
Member

liggitt commented Sep 11, 2018

Is this some kind of JavaScript speciality that slices are represented as objects with items and type?

none I've ever encountered

@sttts
Copy link
Contributor

sttts commented Sep 11, 2018

Haven't found it in JSON Schema (yet) https://json-schema.org/specification.html

@sttts
Copy link
Contributor

sttts commented Sep 11, 2018

Found this example https://json-schema.org/latest/json-schema-core.html#rfc.section.8.3.2, which looks concerningly similar to what we see.

@sttts
Copy link
Contributor

sttts commented Sep 11, 2018

Comes from this PR, without any reasoning: go-openapi/validate#35

@liggitt
Copy link
Member

liggitt commented Sep 11, 2018

it looks like something that either should be done when validating a schema, or should pull type info from the validator when validating an object, but not arbitrarily pull type info from attributes of the object being validated

@sttts
Copy link
Contributor

sttts commented Sep 11, 2018

Bug in go-openapi I believe, misunderstanding of the OpenAPI spec: go-openapi/validate#35 (comment)

@nikhita
Copy link
Member

nikhita commented Nov 14, 2018

/assign

will fix it upstream and report back here after that's done

@marun
Copy link
Contributor

marun commented Nov 30, 2018

This problem has also been encountered in federation with the move to generate validation schema for federated template crds: kubernetes-retired/kubefed#469

@marun
Copy link
Contributor

marun commented Nov 30, 2018

Will it be possible for this fix to land in 1.13 (whether initially or a patch release)?

@tamalsaha
Copy link
Member

@nikhita / @sttts , is there an workaround for this issue? And am I right to assume that this will need a server side fix?

@tossmilestone
Copy link
Member

/assign
I will take this issue.

@tossmilestone
Copy link
Member

@nikhita Hi, do you have any plan to work on this?

@nikhita
Copy link
Member

nikhita commented Mar 20, 2019

@nikhita Hi, do you have any plan to work on this?

I noticed that you've created the PR upstream, so if you'd like, feel free to take it up! :)

@tossmilestone
Copy link
Member

Hi, @nikhita, after fix the bug in go-openapi, I can't find where the k8s validate the openapischema? Could you help to point out where is the validation code?
Thanks very much! 😄

@nikhita
Copy link
Member

nikhita commented Mar 29, 2019 via email

@sttts
Copy link
Contributor

sttts commented Apr 1, 2019

Where is the PR?

@tossmilestone
Copy link
Member

@sttts This is the PR go-openapi/validate#106, and I will submit a PR later in k8s.

davidcurrie pushed a commit to jenkins-x/jx that referenced this issue Apr 2, 2019
@liggitt liggitt moved this from Triage to Required for GA, in progress in Custom Resource Definitions Apr 18, 2019
@liggitt liggitt added this to the v1.16 milestone Jun 12, 2019
@liggitt liggitt added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Jun 12, 2019
@liggitt liggitt moved this from Required for GA, in progress to Complete in Custom Resource Definitions Jun 27, 2019
@shivasuri
Copy link

Hi, I just ran into this issue on on k8s 1.14. What version is this fixed in, if any?

@liggitt
Copy link
Member

liggitt commented Jul 1, 2020

Fixed in #76124 in 1.16

aiyengar2 added a commit to aiyengar2/charts that referenced this issue Oct 13, 2020
Root cause of the issue is that k8s 1.15 / 1.16 clusters seem to have known issues related to CRD fields that invoke `items` in it, such as the `volumes` we introduced as part of the nginx proxy that sits in front of Prometheus in Monitoring V2. This only seems to have been fixed in k8s 1.17+.

More context:

kubernetes/kubernetes#68466 is an issue that tracks a problem with CRDs that use fields named `items` introduced sometime before k8s 1.16.

In k8s 1.16's release notes, this issue seemed to have been resolved as it shows up under `Other Notable Changes` [here](https://v1-16.docs.kubernetes.io/docs/setup/release/notes/#other-notable-changes-7):
```
Fix CRD validation error on ‘items’ field. (#76124, @tossmilestone)
```

However, when looking into the actual [PR](kubernetes/kubernetes#76124) that was executed to fix this issue, it seems like this issue had a followup in kubernetes/kubernetes#85223 that was only resolved in k8s 1.17+.

This shows up in the k8s 1.17 release notes under `API Machinery` [here](https://v1-17.docs.kubernetes.io/docs/setup/release/notes/#api-machinery):
```
CRDs can have fields named type with value array and nested array with items fields without validation to fall over this. (#85223, @sttts)
```

Therefore, the quick fix was to just modify the contents provided to the `volumes` field of the `prometheusSpec`; once modified, Monitoring V2 successfully deployed in a k8s 1.16 cluster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/custom-resources kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Development

Successfully merging a pull request may close this issue.

10 participants