Consider making pkg/conversion a Go sub-module #3130
Comments
|
I think we should find a way to implement conversion outside of API packages (just like mutating & validating webhooks today) |
|
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 |
So yes, the issue is the use of 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 |
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. |
|
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
} |
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) |
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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!
The text was updated successfully, but these errors were encountered: