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

feat: create CRDs as apiextensions.k8s.io/v1 #2025

Merged

Conversation

nabokihms
Copy link
Member

Signed-off-by: m.nabokikh maksim.nabokikh@flant.com

Overview

  • add Kubernetes version discovery mechanism
  • create crds as apiextensions.k8s.io/v1 for kubernetes clusters >= 1.16

What this PR does / why we need it

Fixes #1895

Special notes for your reviewer

There is no test that proves crds creation in kubernetes clusters >= 1.10 works (related to #1999).

Does this PR introduce a user-facing change?

It is not a breaking change.

kubernetes storage: create custom resource definitions as apiextensions.k8s.io/v1

@nabokihms nabokihms force-pushed the kubernetes-apiextensions-version branch from 7d8a7d1 to 1ffed6e Compare March 2, 2021 20:44
@nabokihms nabokihms force-pushed the kubernetes-apiextensions-version branch from 1ffed6e to f56c580 Compare March 2, 2021 20:46
@sagikazarmark
Copy link
Member

Needs rebase.

I think we don't have to keep supporting Kubernetes 1.10. Supporting the officially supported Kubernetes versions, minus a couple versions as we see fit should be more than enough in my opinion.

I would also consider removing the CRD installation from Dex (or at least make it optional). CRDs are better installed by the user or the Helm chart IMHO.

These aren't blockers of this PR, but I think it's a good conversation starter.

My only concern about this PR is that we need to make sure, that we have an upgrade path from older CRD versions. If that's a kubectl apply -f command, that's perfectly fine, but we need that documented somewhere.

@nabokihms
Copy link
Member Author

Actually, we do not need to upgrade previously created CRDs. Kubernetes does it for us. If CRDs were deployed earlier, it is fine. We will be able to access them using old and new API versions (one apiVersion is called storage version, the others are called served versions).
We will face the problem only in Kubernetes >=1.22 because the API version we use to create them will be removed.

I like the idea to add the flag or option to prevent CRDs creation by Dex, as it is in other Kubernetes operators/controllers. In Flant, we also want to manage CRDs ourselves.

@nabokihms nabokihms force-pushed the kubernetes-apiextensions-version branch from f56c580 to ddf3fcb Compare May 6, 2021 20:30
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@nabokihms nabokihms force-pushed the kubernetes-apiextensions-version branch from ddf3fcb to 7a24725 Compare June 10, 2021 16:01
@nabokihms
Copy link
Member Author

Kubernetes 1.22 is scheduled for Aug 4

Wednesday, August 4th: Week 15 - Kubernetes v1.22.0 released

@sagikazarmark
Copy link
Member

Let's schedule 2.30 for July. I'd like to get 2.29 released ASAP.

@nabokihms nabokihms added this to the v2.30.0 milestone Jul 1, 2021
@omesser
Copy link

omesser commented Jul 7, 2021

@sagikazarmark - Hey there,
Any estimated july release date for the 2.30 with this change?
The CRD incompatibility is of course a showstopper for using newer k8s versions (>=1.19)

@sagikazarmark
Copy link
Member

@omesser there is no scheduled date yet.

The CRD incompatibility is of course a showstopper for using newer k8s versions (>=1.19)

Is it? I thought it will only get removed in 1.22

@nabokihms I think this is good to go. A couple questions:

It is not a breaking change.

Are you sure? I think this will make Dex uninstallable on k8s <1.16. If that's the case, I think it would make sense to at least document that fact.

How are upgrades going to work?

@nabokihms
Copy link
Member Author

  1. I was choosing between making a breaking change or saving compatibility. Eventually, I took the second variant. For Kubernetes clusters <1.16, Dex will deploy CRDs as it does right now. For higher versions, Dex will use the stable API.

  2. About upgrade process:

  • Dex will detect the suitable API based on the Kubernetes version to work with CRDs.
  • Dex checks CRDs in the cluster with detected API version.
    NOTE: If CRDs were created through the old API and we want to get them through the new API, Kubernetes will convert them to the desired version on-flight and respond with converted objects.
  • If CRDs are available, Dex does nothing.
  • If there are no CRDs, Dex will create them.

Therefore no upgrade is required, and no breaking changes are provided!

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Perfect! I looked at the implementation and it looks good to me!

@omesser
Copy link

omesser commented Jul 7, 2021

@sagikazarmark - You're right, apologies, I've seen some contradicting info there, there's this from 1.16 stating it would be removed by 1.19, for example (and a lot of other products took it that way):
https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.16.md#deprecations-and-removals

The apiextensions.k8s.io/v1beta1 version of CustomResourceDefinition is deprecated and will no longer be served in v1.19. Use apiextensions.k8s.io/v1 instead. (#79604, @liggitt)

But later release notes support that v1beta is still served by 1.19 (while it's deprecated it should still be served), and will only be completely removed in 1.22:
https://kubernetes.io/docs/reference/using-api/deprecation-guide/#customresourcedefinition-v122
kubernetes/kubernetes#90673
so, yeah.
I'm gonna test to see if it's indeed ok on k8s 1.19, and this will make this not as immediate 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Kubernetes storage CRDs API version to apiextensions.k8s.io/v1
3 participants