Consider making pkg/conversion a Go sub-module

[akutz]

Please consider making pkg/conversion a Go sub-module within this project. That way folks that need to implement conversion webhooks in their API modules can do so without pulling in the entire Controller-Runtime dependency graph. Thanks!

[sbueringer]

I think we should find a way to implement conversion outside of API packages (just like mutating & validating webhooks today)

[sbueringer]

Hm wondering if there is a pretty easy way to make this work

Currently we have the following interfaces

type Convertible interface {
	runtime.Object
	ConvertTo(dst Hub) error
	ConvertFrom(src Hub) error
}

type Hub interface {
	runtime.Object
	Hub()
}

I think the only reason why pkg/conversion needs to be imported is because otherwise it's not possible to implement the ConvertTo / ConvertFrom functions.

If we would additionally support implementing the following interface, no import to CR would be needed at all?

type Convertible interface { 
	runtime.Object
	ConvertTo(dst runtime.Object) error
	ConvertFrom(src runtime.Object) error
}

An alternative: I don't know if there is a way to define an interface with generics so that src & dst in the functions only have to be types that have the Hub method.

[akutz]

Hm wondering if there is a pretty easy way to make this work

Currently we have the following interfaces

type Convertible interface {
runtime.Object
ConvertTo(dst Hub) error
ConvertFrom(src Hub) error
}

type Hub interface {
runtime.Object
Hub()
}
I think the only reason why pkg/conversion needs to be imported is because otherwise it's not possible to implement the ConvertTo / ConvertFrom functions.

If we would additionally support implementing the following interface, no import to CR would be needed at all?

type Convertible interface {
runtime.Object
ConvertTo(dst runtime.Object) error
ConvertFrom(src runtime.Object) error
}
An alternative: I don't know if there is a way to define an interface with generics so that src & dst in the functions only have to be types that have the Hub method.

So yes, the issue is the use of dst Hub and src Hub in the function signatures. And yes, generics would help, but that would also mean everything in controller-runtime related to those interfaces would need to use type parameters as well.

I've already thought a lot about this, and the simplest solution is to make that package a separate Go module. There's nothing else in it, and it would be trivial to do. The only other solution that would approach the simplicity of this would be to do the following:

type Convertible interface {
	runtime.Object
	ConvertTo(dst runtime.Object) error
	ConvertFrom(src runtime.Object) error
}

type Hub interface {
	runtime.Object
	Hub()
}

This way people could just copy the interfaces locally and the actual implementation of code that expects a Convertible object could validate the object implements Hub() or returns an error.

[akutz]

I think we should find a way to implement conversion outside of API packages (just like mutating & validating webhooks today)

Yes, 1000 times yes, but what I'm recommending is far easier and does not add any real debt. In fact, my second recommendation is even simpler.

[akutz]

By the way, this is what it would look like to make this work with generics:

diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go
index c74742d6..bd80cb55 100644
--- a/pkg/builder/webhook.go
+++ b/pkg/builder/webhook.go
@@ -30,13 +30,17 @@ import (
 	"k8s.io/klog/v2"
 
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
+	pkgconv "sigs.k8s.io/controller-runtime/pkg/conversion"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
 )
 
 // WebhookBuilder builds a Webhook.
-type WebhookBuilder struct {
+type WebhookBuilder[
+	THub pkgconv.Hub,
+	TConvertible pkgconv.Convertible[THub],
+] struct {
 	apiType             runtime.Object
 	customDefaulter     admission.CustomDefaulter
 	customDefaulterOpts []admission.DefaulterOption
@@ -51,8 +55,15 @@ type WebhookBuilder struct {
 }
 
 // WebhookManagedBy returns a new webhook builder.
-func WebhookManagedBy(m manager.Manager) *WebhookBuilder {
-	return &WebhookBuilder{mgr: m}
+func WebhookManagedBy(m manager.Manager) *WebhookBuilder[pkgconv.Hub, pkgconv.Convertible[pkgconv.Hub]] {
+	return &WebhookBuilder[pkgconv.Hub, pkgconv.Convertible[pkgconv.Hub]]{mgr: m}
+}
+
+func WebhookManagedByTyped[
+	THub pkgconv.Hub,
+	TConvertible pkgconv.Convertible[THub],
+](m manager.Manager) *WebhookBuilder[THub, TConvertible] {
+	return &WebhookBuilder[THub, TConvertible]{mgr: m}
 }
 
 // TODO(droot): update the GoDoc for conversion.
@@ -60,7 +71,7 @@ func WebhookManagedBy(m manager.Manager) *WebhookBuilder {
 // For takes a runtime.Object which should be a CR.
 // If the given object implements the admission.Defaulter interface, a MutatingWebhook will be wired for this type.
 // If the given object implements the admission.Validator interface, a ValidatingWebhook will be wired for this type.
-func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) For(apiType runtime.Object) *WebhookBuilder[THub, TConvertible] {
 	if blder.apiType != nil {
 		blder.err = errors.New("For(...) should only be called once, could not assign multiple objects for webhook registration")
 	}
@@ -70,39 +81,39 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder {
 
 // WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook with the provided opts (admission.DefaulterOption)
 // will be wired for this type.
-func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) WithDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder[THub, TConvertible] {
 	blder.customDefaulter = defaulter
 	blder.customDefaulterOpts = opts
 	return blder
 }
 
 // WithValidator takes a admission.CustomValidator interface, a ValidatingWebhook will be wired for this type.
-func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) WithValidator(validator admission.CustomValidator) *WebhookBuilder[THub, TConvertible] {
 	blder.customValidator = validator
 	return blder
 }
 
 // WithLogConstructor overrides the webhook's LogConstructor.
-func (blder *WebhookBuilder) WithLogConstructor(logConstructor func(base logr.Logger, req *admission.Request) logr.Logger) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) WithLogConstructor(logConstructor func(base logr.Logger, req *admission.Request) logr.Logger) *WebhookBuilder[THub, TConvertible] {
 	blder.logConstructor = logConstructor
 	return blder
 }
 
 // RecoverPanic indicates whether panics caused by the webhook should be recovered.
 // Defaults to true.
-func (blder *WebhookBuilder) RecoverPanic(recoverPanic bool) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) RecoverPanic(recoverPanic bool) *WebhookBuilder[THub, TConvertible] {
 	blder.recoverPanic = &recoverPanic
 	return blder
 }
 
 // WithCustomPath overrides the webhook's default path by the customPath
-func (blder *WebhookBuilder) WithCustomPath(customPath string) *WebhookBuilder {
+func (blder *WebhookBuilder[THub, TConvertible]) WithCustomPath(customPath string) *WebhookBuilder[THub, TConvertible] {
 	blder.customPath = customPath
 	return blder
 }
 
 // Complete builds the webhook.
-func (blder *WebhookBuilder) Complete() error {
+func (blder *WebhookBuilder[THub, TConvertible]) Complete() error {
 	// Set the Config
 	blder.loadRestConfig()
 
@@ -113,13 +124,13 @@ func (blder *WebhookBuilder) Complete() error {
 	return blder.registerWebhooks()
 }
 
-func (blder *WebhookBuilder) loadRestConfig() {
+func (blder *WebhookBuilder[THub, TConvertible]) loadRestConfig() {
 	if blder.config == nil {
 		blder.config = blder.mgr.GetConfig()
 	}
 }
 
-func (blder *WebhookBuilder) setLogConstructor() {
+func (blder *WebhookBuilder[THub, TConvertible]) setLogConstructor() {
 	if blder.logConstructor == nil {
 		blder.logConstructor = func(base logr.Logger, req *admission.Request) logr.Logger {
 			log := base.WithValues(
@@ -139,7 +150,7 @@ func (blder *WebhookBuilder) setLogConstructor() {
 	}
 }
 
-func (blder *WebhookBuilder) registerWebhooks() error {
+func (blder *WebhookBuilder[THub, TConvertible]) registerWebhooks() error {
 	typ, err := blder.getType()
 	if err != nil {
 		return err
@@ -169,7 +180,7 @@ func (blder *WebhookBuilder) registerWebhooks() error {
 }
 
 // registerDefaultingWebhook registers a defaulting webhook if necessary.
-func (blder *WebhookBuilder) registerDefaultingWebhook() error {
+func (blder *WebhookBuilder[THub, TConvertible]) registerDefaultingWebhook() error {
 	mwh := blder.getDefaultingWebhook()
 	if mwh != nil {
 		mwh.LogConstructor = blder.logConstructor
@@ -195,7 +206,7 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
+func (blder *WebhookBuilder[THub, TConvertible]) getDefaultingWebhook() *admission.Webhook {
 	if defaulter := blder.customDefaulter; defaulter != nil {
 		w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter, blder.customDefaulterOpts...)
 		if blder.recoverPanic != nil {
@@ -207,7 +218,7 @@ func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
 }
 
 // registerValidatingWebhook registers a validating webhook if necessary.
-func (blder *WebhookBuilder) registerValidatingWebhook() error {
+func (blder *WebhookBuilder[THub, TConvertible]) registerValidatingWebhook() error {
 	vwh := blder.getValidatingWebhook()
 	if vwh != nil {
 		vwh.LogConstructor = blder.logConstructor
@@ -233,7 +244,7 @@ func (blder *WebhookBuilder) registerValidatingWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
+func (blder *WebhookBuilder[THub, TConvertible]) getValidatingWebhook() *admission.Webhook {
 	if validator := blder.customValidator; validator != nil {
 		w := admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, validator)
 		if blder.recoverPanic != nil {
@@ -244,15 +255,15 @@ func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
 	return nil
 }
 
-func (blder *WebhookBuilder) registerConversionWebhook() error {
-	ok, err := conversion.IsConvertible(blder.mgr.GetScheme(), blder.apiType)
+func (blder *WebhookBuilder[THub, TConvertible]) registerConversionWebhook() error {
+	ok, err := conversion.IsConvertibleTyped[THub, TConvertible](blder.mgr.GetScheme(), blder.apiType)
 	if err != nil {
 		log.Error(err, "conversion check failed", "GVK", blder.gvk)
 		return err
 	}
 	if ok {
 		if !blder.isAlreadyHandled("/convert") {
-			blder.mgr.GetWebhookServer().Register("/convert", conversion.NewWebhookHandler(blder.mgr.GetScheme()))
+			blder.mgr.GetWebhookServer().Register("/convert", conversion.NewWebhookHandlerTyped[THub, TConvertible](blder.mgr.GetScheme()))
 		}
 		log.Info("Conversion webhook enabled", "GVK", blder.gvk)
 	}
@@ -260,14 +271,14 @@ func (blder *WebhookBuilder) registerConversionWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getType() (runtime.Object, error) {
+func (blder *WebhookBuilder[THub, TConvertible]) getType() (runtime.Object, error) {
 	if blder.apiType != nil {
 		return blder.apiType, nil
 	}
 	return nil, errors.New("For() must be called with a valid object")
 }
 
-func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
+func (blder *WebhookBuilder[THub, TConvertible]) isAlreadyHandled(path string) bool {
 	if blder.mgr.GetWebhookServer().WebhookMux() == nil {
 		return false
 	}
diff --git a/pkg/conversion/conversion.go b/pkg/conversion/conversion.go
index df5973dd..21e2b99f 100644
--- a/pkg/conversion/conversion.go
+++ b/pkg/conversion/conversion.go
@@ -24,10 +24,10 @@ package conversion
 import "k8s.io/apimachinery/pkg/runtime"
 
 // Convertible defines capability of a type to convertible i.e. it can be converted to/from a hub type.
-type Convertible interface {
+type Convertible[THub Hub] interface {
 	runtime.Object
-	ConvertTo(dst runtime.Object) error
-	ConvertFrom(src runtime.Object) error
+	ConvertTo(dst THub) error
+	ConvertFrom(src THub) error
 }
 
 // Hub marks that a given type is the hub type for conversion. This means that
diff --git a/pkg/webhook/conversion/conversion.go b/pkg/webhook/conversion/conversion.go
index 249a364b..9f50ca5e 100644
--- a/pkg/webhook/conversion/conversion.go
+++ b/pkg/webhook/conversion/conversion.go
@@ -40,19 +40,30 @@ var (
 )
 
 func NewWebhookHandler(scheme *runtime.Scheme) http.Handler {
-	return &webhook{scheme: scheme, decoder: NewDecoder(scheme)}
+	return &webhook[conversion.Hub, conversion.Convertible[conversion.Hub]]{scheme: scheme, decoder: NewDecoder(scheme)}
+}
+
+func NewWebhookHandlerTyped[
+	THub conversion.Hub,
+	TConvertible conversion.Convertible[THub],
+](scheme *runtime.Scheme) http.Handler {
+
+	return &webhook[THub, TConvertible]{scheme: scheme, decoder: NewDecoder(scheme)}
 }
 
 // webhook implements a CRD conversion webhook HTTP handler.
-type webhook struct {
+type webhook[
+	THub conversion.Hub,
+	TConvertible conversion.Convertible[THub],
+] struct {
 	scheme  *runtime.Scheme
 	decoder *Decoder
 }
 
 // ensure Webhook implements http.Handler
-var _ http.Handler = &webhook{}
+var _ http.Handler = &webhook[conversion.Hub, conversion.Convertible[conversion.Hub]]{}
 
-func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+func (wh *webhook[THub, TConvertible]) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	convertReview := &apix.ConversionReview{}
 	err := json.NewDecoder(r.Body).Decode(convertReview)
 	if err != nil {
@@ -87,7 +98,7 @@ func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 // handles a version conversion request.
-func (wh *webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
+func (wh *webhook[THub, TConvertible]) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
 	if req == nil {
 		return nil, fmt.Errorf("conversion request is nil")
 	}
@@ -120,7 +131,7 @@ func (wh *webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.Conv
 // convertObject will convert given a src object to dst object.
 // Note(droot): couldn't find a way to reduce the cyclomatic complexity under 10
 // without compromising readability, so disabling gocyclo linter
-func (wh *webhook) convertObject(src, dst runtime.Object) error {
+func (wh *webhook[THub, TConvertible]) convertObject(src, dst runtime.Object) error {
 	srcGVK := src.GetObjectKind().GroupVersionKind()
 	dstGVK := dst.GetObjectKind().GroupVersionKind()
 
@@ -133,28 +144,28 @@ func (wh *webhook) convertObject(src, dst runtime.Object) error {
 	}
 
 	srcIsHub, dstIsHub := isHub(src), isHub(dst)
-	srcIsConvertible, dstIsConvertible := isConvertible(src), isConvertible(dst)
+	srcIsConvertible, dstIsConvertible := isConvertible[THub, TConvertible](src), isConvertible[THub, TConvertible](dst)
 
 	switch {
 	case srcIsHub && dstIsConvertible:
-		return dst.(conversion.Convertible).ConvertFrom(src.(conversion.Hub))
+		return dst.(TConvertible).ConvertFrom(src.(THub))
 	case dstIsHub && srcIsConvertible:
-		return src.(conversion.Convertible).ConvertTo(dst.(conversion.Hub))
+		return src.(TConvertible).ConvertTo(dst.(THub))
 	case srcIsConvertible && dstIsConvertible:
-		return wh.convertViaHub(src.(conversion.Convertible), dst.(conversion.Convertible))
+		return wh.convertViaHub(src.(TConvertible), dst.(TConvertible))
 	default:
 		return fmt.Errorf("%T is not convertible to %T", src, dst)
 	}
 }
 
-func (wh *webhook) convertViaHub(src, dst conversion.Convertible) error {
-	hub, err := wh.getHub(src)
+func (wh *webhook[THub, TConvertible]) convertViaHub(src, dst TConvertible) error {
+	hub, ok, err := wh.getHub(src)
 	if err != nil {
 		return err
 	}
 
-	if hub == nil {
-		return fmt.Errorf("%s does not have any Hub defined", src)
+	if !ok {
+		return fmt.Errorf("%T does not have any Hub defined", src)
 	}
 
 	err = src.ConvertTo(hub)
@@ -171,35 +182,36 @@ func (wh *webhook) convertViaHub(src, dst conversion.Convertible) error {
 }
 
 // getHub returns an instance of the Hub for passed-in object's group/kind.
-func (wh *webhook) getHub(obj runtime.Object) (conversion.Hub, error) {
+func (wh *webhook[THub, TConvertible]) getHub(obj runtime.Object) (THub, bool, error) {
+	var hub THub
+
 	gvks, err := objectGVKs(wh.scheme, obj)
 	if err != nil {
-		return nil, err
+		return hub, false, err
 	}
 	if len(gvks) == 0 {
-		return nil, fmt.Errorf("error retrieving gvks for object : %v", obj)
+		return hub, false, fmt.Errorf("error retrieving gvks for object : %v", obj)
 	}
 
-	var hub conversion.Hub
 	var hubFoundAlready bool
 	for _, gvk := range gvks {
 		instance, err := wh.scheme.New(gvk)
 		if err != nil {
-			return nil, fmt.Errorf("failed to allocate an instance for gvk %v: %w", gvk, err)
+			return hub, false, fmt.Errorf("failed to allocate an instance for gvk %v: %w", gvk, err)
 		}
-		if val, isHub := instance.(conversion.Hub); isHub {
+		if val, isHub := instance.(THub); isHub {
 			if hubFoundAlready {
-				return nil, fmt.Errorf("multiple hub version defined for %T", obj)
+				return hub, false, fmt.Errorf("multiple hub version defined for %T", obj)
 			}
 			hubFoundAlready = true
 			hub = val
 		}
 	}
-	return hub, nil
+	return hub, hubFoundAlready, nil
 }
 
 // allocateDstObject returns an instance for a given GVK.
-func (wh *webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
+func (wh *webhook[THub, TConvertible]) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
 	gvk := schema.FromAPIVersionAndKind(apiVersion, kind)
 
 	obj, err := wh.scheme.New(gvk)
@@ -222,6 +234,17 @@ func (wh *webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, e
 // to be convertible, the group-kind needs to have a Hub type defined and all
 // non-hub types must be able to convert to/from Hub.
 func IsConvertible(scheme *runtime.Scheme, obj runtime.Object) (bool, error) {
+	return IsConvertibleTyped[conversion.Hub, conversion.Convertible[conversion.Hub]](scheme, obj)
+}
+
+// IsConvertibleTyped determines if given type is convertible or not. For a type
+// to be convertible, the group-kind needs to have a Hub type defined and all
+// non-hub types must be able to convert to/from Hub.
+func IsConvertibleTyped[
+	THub conversion.Hub,
+	TConvertible conversion.Convertible[THub],
+](scheme *runtime.Scheme, obj runtime.Object) (bool, error) {
+
 	var hubs, spokes, nonSpokes []runtime.Object
 
 	gvks, err := objectGVKs(scheme, obj)
@@ -243,7 +266,7 @@ func IsConvertible(scheme *runtime.Scheme, obj runtime.Object) (bool, error) {
 			continue
 		}
 
-		if !isConvertible(instance) {
+		if !isConvertible[THub, TConvertible](instance) {
 			nonSpokes = append(nonSpokes, instance)
 			continue
 		}
@@ -327,8 +350,11 @@ func isHub(obj runtime.Object) bool {
 }
 
 // isConvertible determines if passed-in object is a convertible.
-func isConvertible(obj runtime.Object) bool {
-	_, yes := obj.(conversion.Convertible)
+func isConvertible[
+	THub conversion.Hub,
+	TConvertible conversion.Convertible[THub],
+](obj runtime.Object) bool {
+	_, yes := obj.(TConvertible)
 	return yes
 }

[sbueringer]

The only other solution that would approach the simplicity of this would be to do the following:

I would be fine with that one, either by modifying the existing interface or adding a new one if we don't want to modify the existing one. Taking Hub as an input parameter isn't worth the dependency this introduces to API packages in my opinion.

I"m not a big fan of a separate module as we either have to push additional tags for that one or it won't have proper versions.

@vincepri @alvaroaleman Opinions?

(I'll take a closer look at the generics option when I have some time)

[sbueringer]

Hm, yeah the generics option has a lot of impact.

I think we could avoid some / half of it if we don't change builder/webhook and require folks to call the following once directly instead:

			blder.mgr.GetWebhookServer().Register("/convert", conversion.NewWebhookHandlerTyped[THub, TConvertible](blder.mgr.GetScheme()))

But it's still a lot of changes only to ~ keep the Hub interface in the ConvertTo/ConvertFrom methods.

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 as fresh with /remove-lifecycle stale
• Close this issue 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.

This bot triages un-triaged issues 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 as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

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

/lifecycle rotten