Remove controller runtime dependency and client-go dependency on importing API types

[DheerajSShetty]

User Story

As a developer if we are importing CAPI api types, it would be good to not have hard dependency on controller runtime version.

Detailed Description

Importing v1alpha2 or v1alpha3 api type brings controller runtime version dependency, for example CAPI 0.3.8 depends on runtime 0.5.10.

Example if a service has been using 0.6.0 while importing CAPI 0.3.6, when the service imports CAPI 0.3.8, it would be forced to update to 0.6.1. At times updating controller-runtime can have dependency on specific version of k8s go client etc.

This feature is good to have as it allows services import CAPI and use controller runtime of their choice.
Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

[ncdc]

@DheerajSShetty please fill in the description completely from the template. Thanks.

[ncdc]

The api types import controller-runtime because they are implementing validating and defaulting webhooks, and the setup functions for them need access to the Manager. We can investigate to see if we can move the setup functions into a separate package. If that's feasible, we should also adjust kubebuilder to use this same pattern (as kubebuilder generated our webhook code).

[detiber]

This may be difficult unless we duplicate or embed the types. The defined interfaces in controller-runtime embed runtime.Object:
https://github.com/kubernetes-sigs/controller-runtime/blob/1095de6493596b5faa6ef9792651ed064b3efb4c/pkg/webhook/admission/validator.go#L27-L33
https://github.com/kubernetes-sigs/controller-runtime/blob/1095de6493596b5faa6ef9792651ed064b3efb4c/pkg/webhook/admission/defaulter.go#L27-L31
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/conversion/conversion.go#L26-L31

[ncdc]

I don't think that will be a problem, if we move the webhook SetupWithManager functions to another package? runtime.Object here is apimachinery, not c-r?

[vincepri]

/milestone Next
/priority backlog

[vincepri]

/milestone Next
/priority backlog

[vincepri]

I'm more inclined to actually close this issue because we're probably not going to be able to tackle it until much later

[DheerajSShetty]

I'm more inclined to actually close this issue because we're probably not going to be able to tackle it until much later

Can we keep it open and mark as low priority instead of closing it?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[JoelSpeed]

As we are starting to discuss a move to v1beta1, this should be kept in the discussion. Moving to v1beta1 could be an appropriate time to make a change like this.

My thoughts on the issue:

• When building a controller that needs Cluster API go types, it is frustrating to be tied to the exact controller-runtime (and therefore others) versions
  • It would be preferable if the API types didn't tie users to specific dependency versions, this should be flexible and up to the application developer importing CAPI types
• There is precedence within the Kubernetes community to create a separate package for APIs (eg K/api)
  • There is nothing to say this must be a separate repository, we could use a separate go module
• Currently the API folder imports controller runtime for two things
  • Setting up the webhooks with the manager (this could exist somewhere else)
  • To import the webhook Defaulter/Validator interfaces (we could either copy these locally or just not import them, it's not 100% necessary, it just adds type safety in the API packages, other)
• We will not be able to remove dependencies on K/apimachinery and K/api, though these repos are stable enough they shouldn't cause issues for developers importing our apis.
• This is becoming more and more important as we look into the future of using CAPI as building blocks with external provisioners for things like Cluster Infrastructure.

[enxebre]

I agree with @JoelSpeed here, I think solving this is critical for community adoption and UX for CAPI as a library that one can build on top of. In some scenarios we are currently having to mirror the API only in a "third party" folder to mitigate the hassle of transient dependencies.

[vincepri]

When building a controller that needs Cluster API go types, it is frustrating to be tied to the exact controller-runtime (and therefore others) versions

Would you want to limit this to only the API types? When one imports Cluster API and uses its utilities the Controller Runtime version must match to avoid any potential impact on behaviors.

There is nothing to say this must be a separate repository, we could use a separate go module

Maybe? The separate go module creates other sorts of issues, for example the release process is going to be more complicated, and we'd need to figure out ways to make sure we can still reference those types in the main CAPI repo by using relative paths (which also have other issues). Kuberentes staging repositories is much more complicated, and it's still using the vendor folder.

To import the webhook Defaulter/Validator interfaces (we could either copy these locally or just not import them, it's not 100% necessary, it just adds type safety in the API packages, other)

I wouldn't want to remove the type safety that comes at compile time. Copying the types has also drawbacks. There was some effort to have the conversion/defaulting/validating webhooks live somewhere other than the types folder.

We will not be able to remove dependencies on K/apimachinery and K/api, though these repos are stable enough they shouldn't cause issues for developers importing our apis.

Kind of? With 1.18 and the context addition, there were breaking changes that needed to be fixed.

---

Overall, I'd like to understand a bit more what's the benefit of having a different controller runtime version, rather than having it be on the same cadence. As we approach v1beta1, we could definitely think about approaching this problem, although we should try to find a solution that's non-invasive, and it's easy to manage.

[JoelSpeed]

Would you want to limit this to only the API types? When one imports Cluster API and uses its utilities the Controller Runtime version must match to avoid any potential impact on behaviors.

I think yes, limit this to the API types, and maybe even generate clientsets for them (possibly a different conversation). While I appreciate that currently all of the community uses controller-runtime clients, there are reasons that you may want to use a typed clientset instead, and I could see this being more prevalent with projects that are integrating their own existing deployment methods

Maybe? The separate go module creates other sorts of issues, for example the release process is going to be more complicated, and we'd need to figure out ways to make sure we can still reference those types in the main CAPI repo by using relative paths (which also have other issues). Kuberentes staging repositories is much more complicated, and it's still using the vendor folder.

Yeah I was just throwing one idea out. I think there's a few options and it's probably worth some discussion. This topic could probably be CAEP worthy

I wouldn't want to remove the type safety that comes at compile time. Copying the types has also drawbacks. There was some effort to have the conversion/defaulting/validating webhooks live somewhere other than the types folder.

I don't think we would lose it completely, because you would still have it in other packages, just not the API package. The var _ webhook.Defaulter = &MachineSet lines are pretty much the only bits in the API package that explicitly require that controller-runtime package right now. If we can avoid having that in there, then that would help with this effort. I'm not familiar with the existing investigation to moving the webhooks so can't comment on that right now.

Kind of? With 1.18 and the context addition, there were breaking changes that needed to be fixed.

Yes, that is definitely true! Though the core K8s repos are much more stable than controller-runtime is at the moment, and I think that's one of the real problems here. By having the API types linked to controller-runtime dependencies, you bring in what is still a pretty quickly changing project into everyone else's dependency trees. Eg right now controller-runtime is pinned to 0.9.0-alpha.1 in cluster api

As we approach v1beta1, we could definitely think about approaching this problem, although we should try to find a solution that's non-invasive, and it's easy to manage.

+1 Definitely think we need to determine the feasibility of this and evaluate effort vs reward

Overall, I'd like to understand a bit more what's the benefit of having a different controller runtime version, rather than having it be on the same cadence.

Perhaps @enxebre can expand on this as I know he was closer to an issue around this recently. I think one of the concerns for me is around integrating with existing projects. For example, if you already have some integration with some library that depends on controller-runtime, k/api, k/apimachinery, and those versions are different what CAPI is pinning, then importing the CAPI types forces you into an impossible dependency tree which cannot be resolved. When all you need from the project is the Go type definitions, and these don't actually use controller-runtime, that extra dependency becomes frustrating

[sbueringer]

I also think, if possible we should avoid forcing consumers of our API types to use the same controller runtime version. In our case it's not an issues as we are essentially pinning to the same controller-runtime / client-go / ... versions as ClusterAPI main branch.

But I think it's not a good idea to force consumers of our API types to the same version which potentially blocks them from up/downgrading controller-runtime independently from us. Concrete example:

• consumers of our api packages import our v1alpha3 API types from our latest v0.3.16 release
• release v0.3.16 uses controller-runtime v0.5.14 which in turn uses k8s.io/client-go, k8s.io/api v0.17.9

So essentially we force consumers to:

• stay on pretty old controller-runtime / client-go versions (in this case even before the client-go context breaking change)
• copy our API types and remove the controller-runtime dependencies

So the only really feasible option is copying the API types around which is in my opinion not desirable. I think it also won't help foster a ecosystem around our API types with hurdles like that (of course that's not a showstopper, but we also not making it exactly easy for consumers).

(The problem also exists in the other direction, if I want to import CAPI v1alpha4, I have to probably upgrade to controller-runtime version v0.9.x, which might be bigger refactorings in some cases)

Of course it's always a gamble which controller-runtime / client-go version differences are actually breaking.

I'm certainly not an expert on go modules, but I'm wondering if it wouldn't be enough to "just" ensure our api packages do not depend on controller-runtime (of course we have have to find a way to solve the webhook issue and I'm not sure how to do this). Wouldn't then api packages without controller-runtime dependencies already be enough to "break" the transitive dependency chains? (without a separate go module)

[enxebre]

Related discussion about controller-runtime own deps kubernetes-sigs/controller-runtime#1495

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fabriziopandini]

/remove-lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[JoelSpeed]

/remove-lifecycle rotten

[vincepri]

/close

We discussed this issue at the backlog grooming on Oct 29th 2021, the consensus was that for now we're not able to fully decouple our API types from the controller runtime dependencies due to conversion webhooks which are strictly tied to our types.

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

/close

We discussed this issue at the backlog grooming on Oct 29th 2021, the consensus was that for now we're not able to fully decouple our API types from the controller runtime dependencies due to conversion webhooks which are strictly tied to our types.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.