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

Fix CRD validation error on 'items' field #76124

Merged
merged 8 commits into from
Jun 27, 2019

Conversation

tossmilestone
Copy link
Member

Signed-off-by: He Xiaoxi tossmilestone@gmail.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Fix CRD validation errror on 'items' field

Which issue(s) this PR fixes:

Fixes #68466

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix CRD validation error on 'items' field.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Apr 4, 2019
@caesarxuchao
Copy link
Member

/assign @mbohlool
/cc @apelisse

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/dependency Issues or PRs related to dependency changes and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 8, 2019
@tossmilestone
Copy link
Member Author

/assign @liggitt

@k8s-ci-robot k8s-ci-robot added area/apiserver sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Apr 8, 2019
@tossmilestone tossmilestone force-pushed the fix-crd-validate-items branch 2 times, most recently from 8dfb907 to 1403ba2 Compare April 8, 2019 03:20
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/cloudprovider and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 8, 2019
@tossmilestone
Copy link
Member Author

@liggitt this PR is done now, PTAL.

@liggitt
Copy link
Member

liggitt commented Jun 26, 2019

thanks for the quick update. the dependencies you pulled in have transitive dependencies we need to also bump. hack/lint-dependencies.sh | grep -v golang will show the ones we need to bump. Go ahead and put all the openapi ones in a single commit, and the others in their own commits.

Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
Signed-off-by: He Xiaoxi <xxhe@alauda.io>
@tossmilestone
Copy link
Member Author

tossmilestone commented Jun 27, 2019

@liggitt all bumps are done, please check again.

@liggitt
Copy link
Member

liggitt commented Jun 27, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2019
@@ -349,9 +303,15 @@ func (s *Schema) AddType(tpe, format string) *Schema {
return s
}

// AsNullable flags this schema as nullable.
func (s *Schema) AsNullable() *Schema {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts, fyi that we picked up a version with this

@liggitt
Copy link
Member

liggitt commented Jun 27, 2019

@tossmilestone, thanks for all the work on this

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tossmilestone

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2019
@liggitt liggitt moved this from Required for GA, in progress to Complete in Custom Resource Definitions Jun 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit 1a80962 into kubernetes:master Jun 27, 2019
@ukclivecox
Copy link

Unfortunately this fix has not solved the core issue. See go-openapi/validate#108
This has gone into k8s 1.16 and causes the k8sapi-server to panic. I will try to raise a bug issue for this to fully document. @liggitt

@ukclivecox
Copy link

On further investigation I think this is due to x-kubernetes-int-or-string

See #83778

wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020
…te-items

Fix CRD validation error on 'items' field

Kubernetes-commit: 1a80962
aiyengar2 added a commit to aiyengar2/charts that referenced this pull request 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
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/custom-resources area/dependency Issues or PRs related to dependency changes area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

CRD cannot have a field named 'items'
6 participants