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

Support v1 CRD API Version #325

Merged
merged 12 commits into from
Sep 27, 2023
Merged

Support v1 CRD API Version #325

merged 12 commits into from
Sep 27, 2023

Conversation

awh21
Copy link
Contributor

@awh21 awh21 commented Sep 22, 2023

Relates to #155

The apiextensions.k8s.io/v1beta1 API version for CRDs is deprecated in v1.22+ of Kubernetes, and a new v1 API version is supported instead. The changes are listed here: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#customresourcedefinition-v122

This PR adds v1 CRD support and moves the existing v1beta1 CRD implementations under a v1beta1 package. It is mostly porting changes introduced to doriordan/skuber in this commit: doriordan/skuber@dbbd839#diff-04a4c27859310f1477c9c3419c74682573418993db9b51912b0b5f84167ec427

My team at work are using Skuber as part of some work to implement deployment mechanisms involving K8s operators - and the K8s clusters we're deploying into are on v1.24 - which only supports v1 CRDs. We would appreciate a review on this (and hopefully an approval and release!) whenever you can please! Worst case, if you don't want to add this capability for whatever reason let me know, we can publish a fork of this internally to use

@hagay3 hagay3 self-requested a review September 22, 2023 14:19
@hagay3
Copy link
Owner

hagay3 commented Sep 22, 2023

Hi @awh21 ,
Thank you for taking the time and contributing to skuber :)
Just so you know, you can also use Dynamic Kubernetes Client for custom resources that are not yet defined in skuber.
https://skuber.co/#/?id=dynamic-kubernetes-client

You are also welcome to skuber discord channel
https://discord.gg/byEh56vFJR

@awh21
Copy link
Contributor Author

awh21 commented Sep 22, 2023

@hagay3 Thanks! I wasn't expecting you to spot this so soon whilst it's still in draft 😅 Like I said in the description we're using this lib at work for some K8s operators and support for v1 CRDs would make our lives much easier. I don't think the dynamic client supports the watch operations we want to use in our K8s operators fwiw, though I could be missing something there

Is v1 CRD support something you'd be up for adding to the library? If not, give me a shout and my team will figure out a way around this internally so no big deal

Also, is there a way to run all these PR checks locally with the various versions of K8s required within Minikube?

Anyways, looks like I need to spend some more time getting the tests to work for all these Scala/K8s versions so let me do that in the mean time

@hagay3
Copy link
Owner

hagay3 commented Sep 22, 2023

Incorporating the v1 CRD into skuber is an excellent idea! I'm eager to contribute and support this effort. Please feel free to reach out and let me know what additional assistance or resources are required to make this integration a success. I'm here to help in any way I can.

In order to run integration tests on your laptop you need to install docker and minikube.
After its installed you can Run the tests from the IDEA, the tests will connect automatically to your local k8s cluster,

You can use this small script to install minikube: https://github.com/hagay3/skuber/blob/master/infra/ci/minikube-commands.sh

Change the kubernetes-version in minikube command to test different versions of k8s cluster

minikube start --kubernetes-version=v1.19.6 --driver=docker

v1.19.6
v1.20.11
v1.21.5
v1.22.9
v1.23.6
v1.24.1

for deleting a cluster

minikube delete

@awh21 awh21 marked this pull request as ready for review September 25, 2023 08:18
@awh21
Copy link
Contributor Author

awh21 commented Sep 25, 2023

Hey @hagay3, hope you had a good weekend! The tests are passing locally so if you wouldn't mind kicking off the workflow for this PR please that'd be great 🙏 I'm hoping we're good to go with this change now, pending review of course!

@hagay3
Copy link
Owner

hagay3 commented Sep 25, 2023

@awh21 I have disabled the workflow approval

also, please fix the docs for custom resources in case needed
https://github.com/hagay3/skuber/tree/master/docs#custom-resource

thanks :)

versions: List[Version]
): CustomResourceDefinition = {
if (versions.exists(v => v.schema.isEmpty))
throw new Exception("Schema must be specified in CRD v1 version")
Copy link
Owner

Choose a reason for hiding this comment

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

so why schema is not mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah because the ResourceSpecification.Version class is used by both v1beta1 and v1 CRDs, but the Schema is only used in v1 (where it is mandatory). I agree it's a little clunky, but I wasn't sure of the best way around it

Maybe splitting it into a base Version trait and then specific classes like CRDv1Version (which would include the Schema as a mandatory field) would be better, I'm not sure. Happy with whatever approach you think is best!

@hagay3
Copy link
Owner

hagay3 commented Sep 27, 2023

migration notes for CustomResourceDefinition (v1beta1):
import skuber.apiextensions.CustomResourceDefinition
->
import skuber.apiextensions.v1beta1.CustomResourceDefinition

@hagay3
Copy link
Owner

hagay3 commented Sep 27, 2023

LGTM
Your work and contribution is much appreciated!
I'll release a new version, you will be able to fetch it from Sonatype in the coming 1-2 days

@awh21
Copy link
Contributor Author

awh21 commented Sep 27, 2023

LGTM
Your work and contribution is much appreciated!
I'll release a new version, you will be able to fetch it from Sonatype in the coming 1-2 days

Awesome stuff! Thank you for the assistance and speedy replies/action :)

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.

2 participants