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 Schema Fixes for CRD v1 #895

Merged
merged 4 commits into from
Oct 8, 2019
Merged

CRD Schema Fixes for CRD v1 #895

merged 4 commits into from
Oct 8, 2019

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented Oct 3, 2019

What this PR does / why we need it:
update to crds for require schema elements. required for kube ver 1.16 or crd v1

Fixes #

@gerred
Copy link
Member

gerred commented Oct 4, 2019

after stewing over this for the night, do we just want to go to CRD v1 now so we don't have to do this later and get it over with pinning to 1.16?

@gerred
Copy link
Member

gerred commented Oct 4, 2019

We'll still need the replace directive until controller-runtime lands kubernetes-sigs/controller-runtime#618 or kubernetes-sigs/controller-runtime#588

@kensipe
Copy link
Member Author

kensipe commented Oct 4, 2019

if we do... that will seal the deal on not supporting 1.14. If we are good with that... than yeah... I'm good with moving to CRD v1.

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

I don't understand the motivation and the implications of this (perhaps @gerred has more context since I was out on Friday?). Could we link an issue to this to see what problem is it solving, please?

go.mod Outdated
sigs.k8s.io/controller-runtime v0.2.0
sigs.k8s.io/controller-tools v0.2.0
sigs.k8s.io/kind v0.5.1
sigs.k8s.io/kustomize v2.0.3+incompatible
sigs.k8s.io/yaml v1.1.0
)

// Pinned to kubernetes-1.15.4
Copy link
Contributor

Choose a reason for hiding this comment

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

does that mean we're not compatible with lower versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

depends on what you mean:)

it has the compatible that is defined by what is in the dependencies... sorry for vagueness. It is likely that we will have kubernetes 1.16 as min supported version. let me add more context to this PR

@kensipe
Copy link
Member Author

kensipe commented Oct 7, 2019

The genesis of this PR was looking into kubectl explain which for our CRDS resulted in something less than useful, setting aside what this exactly means for a now, sufficient to say that explain didn't work as expected. As I look into what is needed to get the explain result we want, it comes down to using preserveUnknownFields: false as part of the spec definition. In the process of analyzing this I discovered that our openAPIV3Schema definition was not strictly correct which is resolved in this PR. During this period of investigation I was modifying the yaml output as it wasn't possible to set the preserveUnknownFields property in our current version of the code. After resolving the schema discrepancies, I focused on how to get preserveUnknownFields property and went down a rabbit hole. CRDs API breaks the commitment for semver AFAIK... or perhaps "they" didn't see it as breaking because it was additive instead of destructive. In the end, in order to have explain work on the v1beta1 apiVersion, it is required to have this field defined as false. That field wasn't defined in kubernetes 1.14 and it was introduced sometime in 1.15. It is also useful to know that this field for the v1 apiVersion isn't available AND the behavior is as that field is false, forcing CRDs in this direction. So in reality, the field was introduced in 1.15 and deprecated in that version.

To further challenge things... the controller-time libraries seemed to have jump 1.15 for it's dependencies and moved to 1.16. and to make life more fun.. there seem to be 2 competing PRs in that direction.
kubernetes-sigs/controller-runtime#588
kubernetes-sigs/controller-runtime#618

AND the a dependencies diamond created that makes it challenging to match a client-go to controller-runtime.

There is leaning towards requiring kubernetes 1.16 for kudo. It took some time to get the right combo of dependencies to be pinned to 1.15 which is apart of this PR. @gerred asked to do the same with 1.16 which has some challenges. I'm working under the impression that we are focused on 1.16 AND that we will move to CRD v1. Regardless... I think I will remove any dependency changes from this PR and focus only on the desired and required schema changes and work on the dependencies versions on a separate PR.

@gerred
Copy link
Member

gerred commented Oct 8, 2019

Capturing the resolution here:

We're going to bump to k8s 1.16 in a separate PR, remove preserveUnknownFields, and land the very clear schema changes in this PR.

@kensipe lmk if I missed anything but that covers it, and with that I'm 👍

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Thanks for the exhaustive comment, that was very helpful.

If I TLDR understand it correctly, we right now support k8s 1.15+ while in the future we'll be probably moving to 1.16+. Makes sense, thanks

@gerred
Copy link
Member

gerred commented Oct 8, 2019

That's correct. CRD v1 has Kubernetes SLAs/SLOs around it, will continue to get features and support, whereas v1beta1 will be deprecated and removed. I'd rather us not GA on v1beta1 for a lot of reasons, but that SLA/SLO/support expectation is a major one.

@kensipe kensipe merged commit 6b07663 into master Oct 8, 2019
@kensipe kensipe deleted the ken/crd-strict branch October 8, 2019 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants