search

kubernetes-retired / cluster-api-provider-nested

Cluster API Provider for Nested Clusters
Apache License 2.0
301 stars 67 forks source link

⚠️ Update VC controller-runtime #70

Closed christopherhein closed 3 years ago

christopherhein commented 3 years ago

This is a relatively large change unfortunately, cause controller runtime's APIs changed a fair amount, adding context support throughout. Also as of v1.20.x kubelet doesn't support redirects anymore only streaming configs which I have filed https://github.com/kubernetes-sigs/cluster-api-provider-nested/issues/69 to which needs to be followed up on.

Related

closes #27

christopherhein commented 3 years ago

/assign @Fei-Guo

christopherhein commented 3 years ago

/cc @weiling61

christopherhein commented 3 years ago

/kind cleanup /milestone v0.1.x

christopherhein commented 3 years ago

Awesome work. This change bumps up Kubernetes dependency to 1.20 as well.

We still don't support the one annoying thing about the ClusterVersion CR where if you regenerate it using the proper version of controller-gen for the version of controller runtime we use then you end up with a huge ClusterVersion CRD and run into issues, so for now in the make file I've disabled generating the CRD manifests so that we continue to use the current working ones.

christopherhein commented 3 years ago

LGTM. (Most changes are due to runtime package switch, no major logic modification)

@weiling61 yup, thanks for giving this a read-through. Something to take into consideration is how the new MCController is implemented note I've broken the implementation for List and Get and removed ListByObjectType and GetByObjectType. Instead using Get and List which take in a pointer to client.Object or client.ObjectList respectively which allows us to populate that object instead of having to return the object back and do type assertions. I've also made one other change to remove us having to have a custom scheme now all "core" types are supported out of the box without us having https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/70/files#diff-f9cddd7f54205f0c3f3483eb332fb22f9fa9c1184b76ec5223af230b5ab7c14b

Fei-Guo commented 3 years ago

“I've also made one other change to remove us having to have a custom scheme now all "core" types are supported out of the box without us having”

Will this make syncing CR resources (not CRD) harder or break existing CR synchronization?

christopherhein commented 3 years ago

Will this make syncing CR resources (not CRD) harder or break existing CR synchronization?

@Fei-Guo this should make it easier to manage these for folks cause we can leverage a lot of the tools that have been built already like controller-runtimes SchemeBuilder which is preconfigured for all kubebuilder/controller runtime projects. This would allow you to take a package like https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v2/api/v1 and all you'd need to do is:

import (
  "sigs.k8s.io/kubebuilder/testdata/project-v2/api/v1"
  "sigs.k8s.io/cluster-api-provider-nested/virtualcluster/pkg/syncer/util/scheme"
)

func init() {
  // custom scheme we want to add 
  v1.AddToScheme(scheme.Scheme)
}

This will use the SchemeBuilder from https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v2/api/v1/groupversion_info.go#L27-L36 and takes in a defined *runtime.Scheme to add those new types and GVK for you.

christopherhein commented 3 years ago

As I was reviewing that I did make one mistake which I've just cleaned up the var Scheme = runtime.NewScheme() was supposed to use https://github.com/kubernetes/client-go/blob/v0.21.1/kubernetes/scheme/register.go#L72 instead which is the client-go scheme that has everything defined already. So I've just updated that. Just waiting on tests to build.

zhuangqh commented 3 years ago

thanks for your job! controller runtime style client makes everything simple 😄

christopherhein commented 3 years ago

Awesome, thanks @zhuangqh. Looks like we just need someone to /lgtm and we'll get this merged.

Fei-Guo commented 3 years ago

/approve /lgtm

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, Fei-Guo, weiling61, zhuangqh

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: - ~~[virtualcluster/OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/OWNERS)~~ [Fei-Guo,christopherhein,zhuangqh] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment