[WIP] ✨ Allow implementation of conversion outside of API packages #3335
+169
−15
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ import ( | |
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/apiutil" | ||
| "sigs.k8s.io/controller-runtime/pkg/conversion" | ||
| logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
| conversionmetrics "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/metrics" | ||
|
|
@@ -43,14 +45,110 @@ var ( | |
| log = logf.Log.WithName("conversion-webhook") | ||
| ) | ||
|
|
||
| type Registry struct { | ||
| scheme *runtime.Scheme | ||
| convertersByHubGK map[schema.GroupKind]convertersForHub | ||
| } | ||
|
|
||
| func NewRegistry(scheme *runtime.Scheme) Registry { | ||
| return Registry{ | ||
| scheme: scheme, | ||
| convertersByHubGK: map[schema.GroupKind]convertersForHub{}, | ||
| } | ||
| } | ||
|
|
||
| type convertersForHub struct { | ||
| hubGVK schema.GroupVersionKind | ||
| convertersBySpokeGVK map[schema.GroupVersionKind]Converter | ||
| } | ||
|
|
||
| func (r Registry) Register(hubGVK schema.GroupVersionKind, converters ...Converter) error { | ||
| if _, ok := r.convertersByHubGK[hubGVK.GroupKind()]; ok { | ||
| return fmt.Errorf("converter already registered for %s", hubGVK.GroupKind()) | ||
| } | ||
|
|
||
| // TODO: validate against schema that all converters have been registred for a type (similar to previous validation) | ||
|
|
||
| r.convertersByHubGK[hubGVK.GroupKind()] = convertersForHub{ | ||
| hubGVK: hubGVK, | ||
| convertersBySpokeGVK: map[schema.GroupVersionKind]Converter{}, | ||
| } | ||
| for _, converter := range converters { | ||
| converterHubGVK, err := apiutil.GVKForObject(converter.GetHub(), r.scheme) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if hubGVK != converterHubGVK { | ||
| return fmt.Errorf("converter GVK does not match builder gvk: FIXME") | ||
| } | ||
| converterSpokeGVK, err := apiutil.GVKForObject(converter.GetSpoke(), r.scheme) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if hubGVK.GroupKind() != converterSpokeGVK.GroupKind() { | ||
| return fmt.Errorf("converter GVK does not match builder gvk: FIXME") | ||
| } | ||
| r.convertersByHubGK[hubGVK.GroupKind()].convertersBySpokeGVK[converterSpokeGVK] = converter | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type Converter interface { | ||
| GetHub() client.Object | ||
| GetSpoke() client.Object | ||
| ConvertHubToSpoke(hub, spoke runtime.Object) error | ||
| ConvertSpokeToHub(hub, spoke runtime.Object) error | ||
| } | ||
|
|
||
| func NewConverter[hubObject, spokeObject client.Object]( | ||
|
There was a problem hiding this comment. This confuses me. Shouldn't this take a slice of There was a problem hiding this comment. Some general context. Current state of conversion:
My main goals are:
There was a problem hiding this comment. 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()
There was a problem hiding this comment.
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).
No, see example one comment above. |
||
| hub hubObject, | ||
| spoke spokeObject, | ||
| convertHubToSpokeFunc func(src hubObject, dst spokeObject) error, | ||
| convertSpokeToHubFunc func(src spokeObject, dst hubObject) error, | ||
|
Comment on lines
+107
to
+108
There was a problem hiding this comment. TODO for myself: we should start passing in ctx here (e.g. for logging) |
||
| ) Converter { | ||
| return &converter[hubObject, spokeObject]{ | ||
| hub: hub, | ||
| spoke: spoke, | ||
| convertSpokeToHubFunc: convertSpokeToHubFunc, | ||
| convertHubToSpokeFunc: convertHubToSpokeFunc, | ||
| } | ||
| } | ||
|
|
||
| var _ Converter = converter[client.Object, client.Object]{} | ||
|
|
||
| type converter[hubObject, spokeObject client.Object] struct { | ||
| hub hubObject | ||
| spoke spokeObject | ||
| convertHubToSpokeFunc func(src hubObject, dst spokeObject) error | ||
| convertSpokeToHubFunc func(src spokeObject, dst hubObject) error | ||
| } | ||
|
|
||
| func (c converter[hubObject, spokeObject]) GetHub() client.Object { | ||
| return c.hub | ||
| } | ||
|
|
||
| func (c converter[hubObject, spokeObject]) GetSpoke() client.Object { | ||
| return c.spoke | ||
| } | ||
|
|
||
| func (c converter[hubObject, spokeObject]) ConvertHubToSpoke(hub, spoke runtime.Object) error { | ||
| return c.convertHubToSpokeFunc(hub.(hubObject), spoke.(spokeObject)) | ||
| } | ||
|
|
||
| func (c converter[hubObject, spokeObject]) ConvertSpokeToHub(hub, spoke runtime.Object) error { | ||
| return c.convertSpokeToHubFunc(spoke.(spokeObject), hub.(hubObject)) | ||
| } | ||
|
|
||
| func NewWebhookHandler(scheme *runtime.Scheme, registry Registry) http.Handler { | ||
| return &webhook{scheme: scheme, decoder: NewDecoder(scheme), registry: registry} | ||
| } | ||
|
|
||
| // webhook implements a CRD conversion webhook HTTP handler. | ||
| type webhook struct { | ||
| scheme *runtime.Scheme | ||
| decoder *Decoder | ||
| registry Registry | ||
| } | ||
|
|
||
| // ensure Webhook implements http.Handler | ||
|
|
@@ -149,6 +247,34 @@ func (wh *webhook) convertObject(src, dst runtime.Object) error { | |
| return fmt.Errorf("conversion is not allowed between same type %T", src) | ||
| } | ||
|
|
||
| if converters, ok := wh.registry.convertersByHubGK[srcGVK.GroupKind()]; ok { | ||
| srcIsHub := converters.hubGVK == srcGVK | ||
| dstIsHub := converters.hubGVK == dstGVK | ||
| _, srcIsConvertible := converters.convertersBySpokeGVK[srcGVK] | ||
| _, dstIsConvertible := converters.convertersBySpokeGVK[dstGVK] | ||
|
|
||
| switch { | ||
| case srcIsHub && dstIsConvertible: | ||
| return converters.convertersBySpokeGVK[dstGVK].ConvertHubToSpoke(src, dst) | ||
| case dstIsHub && srcIsConvertible: | ||
| return converters.convertersBySpokeGVK[srcGVK].ConvertSpokeToHub(src, dst) | ||
| case srcIsConvertible && dstIsConvertible: | ||
| hubGVK := converters.hubGVK | ||
| hub, err := wh.scheme.New(hubGVK) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to allocate an instance for gvk %v: %w", hubGVK, err) | ||
| } | ||
| if err := converters.convertersBySpokeGVK[srcGVK].ConvertSpokeToHub(src, hub); err != nil { | ||
| return fmt.Errorf("%T failed to convert to hub version %T : %w", src, hub, err) | ||
| } | ||
| if err := converters.convertersBySpokeGVK[dstGVK].ConvertHubToSpoke(hub, dst); err != nil { | ||
| return fmt.Errorf("%T failed to convert from hub version %T : %w", dst, hub, err) | ||
| } | ||
| default: | ||
| return fmt.Errorf("%T is not convertible to %T", src, dst) | ||
| } | ||
| } | ||
|
|
||
| srcIsHub, dstIsHub := isHub(src), isHub(dst) | ||
| srcIsConvertible, dstIsConvertible := isConvertible(src), isConvertible(dst) | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ourconvertObjectfunc 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.Today conversions with CR work the following way:
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.Contextinto the ConvertTo/ConvertFrom funcs, that would not be possible with the scheme (it only takestype ConversionFunc func(a, b interface{}, scope Scope) errorfuncs)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.
Instead of just:
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.