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

Default value not allowed for in x-kubernetes-list-map-keys #444

Closed
tamalsaha opened this issue May 24, 2020 · 14 comments · Fixed by #480
Closed

Default value not allowed for in x-kubernetes-list-map-keys #444

tamalsaha opened this issue May 24, 2020 · 14 comments · Fixed by #480
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@tamalsaha
Copy link
Contributor

I filed the issue in k/k: kubernetes/kubernetes#91395

I am cross posting here because I don't know if this is an issue in the upstream CRD validation or the YAML generated by controller-tools.

@tamalsaha
Copy link
Contributor Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 24, 2020
@liggitt
Copy link
Contributor

liggitt commented May 24, 2020

In a v1beta1 CRD, a property specified as a list-map item key must be a required property.

In a v1 CRD it can be a required property or be given a default value

@tamalsaha
Copy link
Contributor Author

The example CronJob yaml in controller-tools project also does not work in 1.18

https://github.com/kubernetes-sigs/controller-tools/blob/v0.3.0/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml

$ kubectl version --short
Client Version: v1.18.3
Server Version: v1.18.2

$ kubectl create -f https://github.com/kubernetes-sigs/controller-tools/raw/v0.3.0/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
The CustomResourceDefinition "cronjobs.testdata.kubebuilder.io" is invalid: 
* spec.validation.openAPIV3Schema.properties[spec].properties[jobTemplate].properties[spec].properties[template].properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[spec].properties[jobTemplate].properties[spec].properties[template].properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[spec].properties[embeddedResource].properties: Required value: must not be empty if x-kubernetes-embedded-resource is true without x-kubernetes-preserve-unknown-fields

@liggitt , how does CRD authors add default value for core/v1 types used in their CRD like done here?

https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/testdata/cronjob_types.go#L77-L78

@tamalsaha
Copy link
Contributor Author

@liggitt ,
Do you think we can add a pr like
kmodules/api@c8bc107

to solve this issue?

@kensipe
Copy link

kensipe commented Jun 15, 2020

@tamalsaha your suggested fix will help the kubebuilder community (and may be worth it)... but we should look to have a solution in controller-tools for all projects that use controller-runtime (in lieu of a core fix that defines how defaults are expressed)

@kwiesmueller
Copy link
Member

/cc @jennybuckley

@kwiesmueller
Copy link
Member

/cc @jpbetz

@tamalsaha
Copy link
Contributor Author

tamalsaha commented Jun 23, 2020

The proper solution is to write a KEP and update the k/api types with the new annotations. See here: kubernetes/kubernetes#91395

My personal hack has been just use my forked api repo with kubebuilder:default annotation. It unblocked me for now. kmodules/api@c8bc107

@kensipe
Copy link

kensipe commented Jun 24, 2020

@tamalsaha The solution discussed in the WG API Expression meeting today (which is a WG for api-machinery) is that @jennybuckley is working on a solution (I think for controller-gen). I specifically asked about the need for a KEP with answers being redirected to Jenny is working on it. Here is the meeting agenda: https://docs.google.com/document/d/1CSpNaicbEqKJoW306qaQEBIkwC1mGIcKl3yiB_C0HZk/edit
unfortunately there were no minutes / notes taken for the discussion.

the /cc: ^^ were to ensure that the solution being worked on covers these cases.

@estroz
Copy link
Contributor

estroz commented Aug 11, 2020

#440

@munnerz
Copy link
Member

munnerz commented Aug 25, 2020

In order to reduce the confusion that this causes, after a discussion with @pwittrock, we were thinking of adding a couple of extra code areas into controller-gen to help here whilst we wait for the eventual upstream solution:

  1. adding in a 'validation' step to controller-gen to actually inform the user that the CRD manifest generated/the input API types are not going to be valid in Kubernetes 1.18+
  2. adding in explicit mappings from field names->default values for some of these commonly affected types in the core API

This will at least allow users to move forward today with installing their resources into Kubernetes 1.18 clusters without having to fork kubernetes/api

amuraru added a commit to adobe/zookeeper-operator that referenced this issue Oct 7, 2020
This is an workaround for k8s 1.18 crd to remove
the required field protocol


See:
- datastax/cass-operator#132
- kubernetes-sigs/controller-tools#444
- operator-framework/operator-sdk#3235
cqbqdd11519 added a commit to tmax-cloud/cicd-operator that referenced this issue Nov 24, 2020
cqbqdd11519 added a commit to tmax-cloud/cicd-operator that referenced this issue Nov 24, 2020
@dvaldivia
Copy link
Contributor

@munnerz do you think there's a work around meanwhile?

@sudhakso
Copy link

Move to controller-gen@0.2.0 has at least got me past the issue. I am trying these CRDs and controllers on k8s@v1.18

@knrt10
Copy link

knrt10 commented Jan 6, 2021

Current hack, moving to controller-gen@0.2.0 fixed the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants