[WIP] ✨ Allow implementation of conversion outside of API packages

[sbueringer]

Goal is to fix the issue described in #3130

I.e. making it possible to implement conversion without introducing a dependency to controller-runtime in API packages.

Here is an example how this can be used to move ConvertTo/ConvertFrom funcs out of the API package: https://github.com/kubernetes-sigs/cluster-api/pull/12820/files#diff-f6619b6bca7abdc3fe36434e753669ddefe9cf8c9cf85278ff815d683a9cde45

Note: As conversion functions now don't have to import controller-runtime anymore they could also be kept in the API package.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

This confuses me. Shouldn't this take a slice of ConvertToHub(spoke runtime.Object)(hub runtime.object, _ error)? Right now, this can only be used if there are exactly to versions (and in that context, the concept of hub and spoke doesn't really make too much sense)

[sbueringer]

Some general context. Current state of conversion:

• Hub has to be implemented on the hup API type
• ConvertTo / ConvertFrom has to be implemented on all spoke API types (there's validation for that in CR)
• So overall the following is needed (example from CAPI):
  • v1beta2 API package:
    • func (*Cluster) Hub() {}
  • v1alpha3 / v1alpha4 / v1beta1 API packages:
    • func (src *Cluster) ConvertTo(dstRaw conversion.Hub) error
    • func (dst *Cluster) ConvertFrom(srcRaw conversion.Hub) error

My main goals are:

• Be able to implement ConvertTo/ConvertFrom outside of API packages
• Of course accordingly ConvertTo/ConvertFrom can't be methods anymore
• Because they are not methods anymore I need a new way to register the funcs (but I still want to be able to verify that all necessary conversions have been provided)
• Make the conversion funcs more type-safe:
  • Today:
    • func (src *Cluster) ConvertTo(dstRaw conversion.Hub) error
    • func (dst *Cluster) ConvertFrom(srcRaw conversion.Hub) error
  • With this PR: (func names don't matter, only the parameter)
    • func ConvertClusterV1Beta1ToHub(src *clusterv1beta1.Cluster, dst *clusterv1.Cluster) error
    • func ConvertClusterHubToV1Beta1(src *clusterv1.Cluster, dst *clusterv1beta1.Cluster) error
• Minimal migration effort for folks that already implemented ConvertTo/ConvertFrom methods and want to move to the new model

[sbueringer]

I think generics are a nice way to make the conversion funcs more type safe (similar to how this works with source.Kind, e.g.: https://github.com/kubernetes-sigs/cluster-api/blob/a9fbe115c8adf0de2c6a27f4f436ccdf657aa884/internal/controllers/clusterresourceset/clusterresourceset_controller.go#L100)

The webhook builder itself cannot become generic with type parameters for the hub type and an arbitrary amount of spoke types. So I thought I'll use an additioanl type that takes care of the generics and then implements an interface that allows to pass it into the builder and use it later.

A concrete example how this can be used based on Cluster API:

	return ctrl.NewWebhookManagedBy(mgr).
		For(&clusterv1.Cluster{}).
		WithDefaulter(webhook).
		WithValidator(webhook).
		WithConverters(
			conversion.NewConverter(&clusterv1.Cluster{}, &clusterv1beta1.Cluster{}, ConvertClusterHubToV1Beta1, ConvertClusterV1Beta1ToHub),
			conversion.NewConverter(&clusterv1.Cluster{}, &clusterv1alpha4.Cluster{}, ConvertClusterHubToV1Alpha4, ConvertClusterV1Alpha4ToHub),
			conversion.NewConverter(&clusterv1.Cluster{}, &clusterv1alpha3.Cluster{}, ConvertClusterHubToV1Alpha3, ConvertClusterV1Alpha3ToHub),
		).
		Complete()

• The generic NewConverter func provides a conversion between the hub and one spoke type
• The hubObject and spokeObject parameter ensure:
  • WithConverters can validate that all necessary conversions have been provided (not fully implemented yet in the PR, but it can check hub and spoke types against the type of the webhook + all GK's registered in the scheme)
  • that the passed in conversion functions have the right type parameters

[sbueringer]

Shouldn't this take a slice of ConvertToHub(spoke runtime.Object)(hub runtime.object, _ error)?

It could, but it would be less type-safe (side-note: I would prefer to keep hub an input parameter, so that CR stays responible for creating an instance of the hub type).

Right now, this can only be used if there are exactly two versions (and in that context, the concept of hub and spoke doesn't really make too much sense)

No, see example one comment above. NewConverter would be just for one hub-spoke conversion. Users have to pass in all the necessary conversions (similar to how they previously had to implement ConvertTo/ConvertFrom on all spoke types)

[alvaroaleman]

While this should be how it works internally, why do we need to have the concept of hub and spoke visible in the external interface and the webhook be aware of it? All it really wants is a convert(from, to runtime.Object) error.

Also without really having context on our current conversion machinery, why don't we use the scheme, it allows to register and call conversion funcs (which internally can be built on a hub and spoke system, but that is nothing the scheme or anything else that wants to convert really cares about)

[sbueringer]

While this should be how it works internally, why do we need to have the concept of hub and spoke visible in the external interface and the webhook be aware of it? All it really wants is a convert(from, to runtime.Object) error.

We could also say just take a convert(from, to runtime.Object) func, but then we have no validation on our side and users have to implement the logic in our convertObject func on there side. I would prefer if users still only have to implement ~ the ConvertTo/ConvertFrom funcs for all their hub-spoke combinations and we take care of the rest.

Also without really having context on our current conversion machinery, why don't we use the scheme, it allows to register and call conversion funcs (which internally can be built on a hub and spoke system, but that is nothing the scheme or anything else that wants to convert really cares about)

Today conversions with CR work the following way:

• auto-generated funcs: https://github.com/kubernetes-sigs/cluster-api/blob/1c45137e25328d8c7ccfd70ee9d3b24d04da1c1b/api/core/v1beta1/zz_generated.conversion.go
  • These funcs are automatically registered in the scheme
• ConvertTo/ConvertFrom funcs implemented on top of auto-generated funcs https://github.com/kubernetes-sigs/cluster-api/blob/42ca147923ba1f9367419f4bd1b8748cf872c1a7/api/core/v1beta1/conversion.go

I would prefer if we could avoid mixing these two layers by putting all of these funcs into the scheme.
I also just realized that we should start passing context.Context into the ConvertTo/ConvertFrom funcs, that would not be possible with the scheme (it only takes type ConversionFunc func(a, b interface{}, scope Scope) error funcs)

If I understand correctly if we would want to delegate the conversion entirely to the scheme we would have to register conversion funcs for all combinations in the scheme, e.g.

• v1beta2 <=> v1beta1, v1beta2 <=> v1alpha4, v1beta2 <=> v1alpha3
• v1beta1 <=> v1alpha4, v1beta1 <=> v1alpha3
• v1alpha4 <=> v1alpha3

Instead of just:

• v1beta2 <=> v1beta1, v1beta2 <=> v1alpha4, v1beta2 <=> v1alpha3

I believe that's why the hub-spoke model was implemented as it is today. I would really prefer if users writing conversion code only have to implement the hub-spoke conversions and not conversion funcs for all combinations.

[sbueringer]

TODO for myself: we should start passing in ctx here (e.g. for logging)