address comments

Author: Mengqi Yu

pkg/builder/build.go

@@ -19,10 +19,8 @@ package builder
 import (
 	"fmt"
 	"strings"
-	"sync"
 
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
@@ -32,7 +30,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
-	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
@@ -44,14 +41,13 @@ var getGvk = apiutil.GVKForObject
 
 // Builder builds a Controller.
 type Builder struct {
-	apiType                runtime.Object
-	mgr                    manager.Manager
-	predicates             []predicate.Predicate
-	managedObjects         []runtime.Object
-	watchRequest           []watchRequest
-	config                 *rest.Config
-	ctrl                   controller.Controller
-	addServerToManagerOnce sync.Once
+	apiType        runtime.Object
+	mgr            manager.Manager
+	predicates     []predicate.Predicate
+	managedObjects []runtime.Object
+	watchRequest   []watchRequest
+	config         *rest.Config
+	ctrl           controller.Controller
 }
 
 // SimpleController returns a new Builder.
@@ -242,29 +238,22 @@ func (blder *Builder) doController(r reconcile.Reconciler) error {
 }
 
 func (blder *Builder) doWebhook() error {
-	if blder.mgr.GetScheme() == nil {
-		return fmt.Errorf("scheme must be provided")
-	}
-
-	svr := blder.mgr.GetWebhookServer()
-	if svr == nil {
-		svr = &webhook.Server{}
-	}
-
 	// Create a webhook for each type
 	gvk, err := apiutil.GVKForObject(blder.apiType, blder.mgr.GetScheme())
 	if err != nil {
 		return err
 	}
 
-	partialPath := generatePath(gvk)
+	partialPath := strings.Replace(gvk.Group, ".", "-", -1) + "-" +
+		gvk.Version + "-" + strings.ToLower(gvk.Kind)
+
 	defaulter, isDefaulter := blder.apiType.(admission.Defaulter)
 	if isDefaulter {
 		mwh := admission.DefaultingWebhookFor(defaulter)
 		if mwh != nil {
 			path := "/mutate-" + partialPath
 			log.Info("registering a mutating webhook", "path", path)
-			svr.Register(path, mwh)
+			blder.mgr.GetWebhookServer().Register(path, mwh)
 		}
 	}
 
@@ -274,26 +263,9 @@ func (blder *Builder) doWebhook() error {
 		if vwh != nil {
 			path := "/validate-" + partialPath
 			log.Info("registering a validating webhook", "path", path)
-			svr.Register(path, vwh)
+			blder.mgr.GetWebhookServer().Register(path, vwh)
 		}
 	}
 
-	if isDefaulter || isValidator {
-		blder.addServerToManagerOnce.Do(func() {
-			err = blder.mgr.Add(svr)
-		})
-	}
 	return err
 }
-
-func generatePath(gvk schema.GroupVersionKind) string {
-	var pathItems []string
-	splittedGroup := strings.Split(gvk.Group, ".")
-	if len(splittedGroup) > 0 && len(splittedGroup[0]) > 0 {
-		pathItems = append(pathItems, splittedGroup[0])
-	}
-	if len(gvk.Kind) > 0 {
-		pathItems = append(pathItems, strings.ToLower(gvk.Kind))
-	}
-	return strings.Join(pathItems, "-")
-}

pkg/manager/internal.go

@@ -171,6 +171,12 @@ func (cm *controllerManager) GetRESTMapper() meta.RESTMapper {
 }
 
 func (cm *controllerManager) GetWebhookServer() *webhook.Server {
+	if cm.webhookServer == nil {
+		cm.webhookServer = &webhook.Server{}
+		if err := cm.Add(cm.webhookServer); err != nil {
+			panic("unable to add webhookServer to the controller manager")
+		}
+	}
 	return cm.webhookServer
 }
 

pkg/webhook/admission/decode.go

@@ -37,24 +37,7 @@ func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) {
 // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object.
 // If you want decode the OldObject in the AdmissionRequest, use DecodeRaw.
 func (d *Decoder) Decode(req Request, into runtime.Object) error {
-	// NB(directxman12): there's a bug/weird interaction between decoders and
-	// the API server where the API server doesn't send a GVK on the embedded
-	// objects, which means the unstructured decoder refuses to decode.  It
-	// also means we can't pass the unstructured directly in, since it'll try
-	// and call unstructured's special Unmarshal implementation, which calls
-	// back into that same decoder :-/
-	// See kubernetes/kubernetes#74373.
-	if unstructuredInto, isUnstructured := into.(*unstructured.Unstructured); isUnstructured {
-		// unmarshal into unstructured's underlying object to avoid calling the decoder
-		if err := json.Unmarshal(req.Object.Raw, &unstructuredInto.Object); err != nil {
-			return err
-		}
-
-		return nil
-	}
-
-	deserializer := d.codecs.UniversalDeserializer()
-	return runtime.DecodeInto(deserializer, req.AdmissionRequest.Object.Raw, into)
+	return d.DecodeRaw(req.Object, into)
 }
 
 // DecodeRaw decodes a RawExtension object into the passed-in runtime.Object.

pkg/webhook/admission/defaulter.go

@@ -22,7 +22,6 @@ import (
 	"net/http"
 
 	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
 )
 
 // Defaulter defines functions for setting defaults on resources
@@ -40,18 +39,9 @@ func DefaultingWebhookFor(defaulter Defaulter) *Webhook {
 
 type mutatingHandler struct {
 	defaulter Defaulter
-	scheme    *runtime.Scheme
 	decoder   *Decoder
 }
 
-var _ inject.Scheme = &mutatingHandler{}
-
-// InjectScheme injects the scheme into a mutatingHandler.
-func (h *mutatingHandler) InjectScheme(s *runtime.Scheme) error {
-	h.scheme = s
-	return nil
-}
-
 var _ DecoderInjector = &mutatingHandler{}
 
 // InjectDecoder injects the decoder into a mutatingHandler.
@@ -63,7 +53,7 @@ func (h *mutatingHandler) InjectDecoder(d *Decoder) error {
 // Handle handles admission requests.
 func (h *mutatingHandler) Handle(ctx context.Context, req Request) Response {
 	if h.defaulter == nil {
-		return Allowed("")
+		panic("defaulter should never be nil")
 	}
 
 	// Get the object in the request

pkg/webhook/admission/validator.go

@@ -22,7 +22,6 @@ import (
 
 	"k8s.io/api/admission/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
 )
 
 // Validator defines functions for validating an operation
@@ -41,18 +40,9 @@ func ValidatingWebhookFor(validator Validator) *Webhook {
 
 type validatingHandler struct {
 	validator Validator
-	scheme    *runtime.Scheme
 	decoder   *Decoder
 }
 
-var _ inject.Scheme = &validatingHandler{}
-
-// InjectScheme injects the scheme into a validatingHandler
-func (h *validatingHandler) InjectScheme(s *runtime.Scheme) error {
-	h.scheme = s
-	return nil
-}
-
 var _ DecoderInjector = &validatingHandler{}
 
 // InjectDecoder injects the decoder into a validatingHandler.
@@ -64,7 +54,7 @@ func (h *validatingHandler) InjectDecoder(d *Decoder) error {
 // Handle handles admission requests.
 func (h *validatingHandler) Handle(ctx context.Context, req Request) Response {
 	if h.validator == nil {
-		return Allowed("")
+		panic("validator should never be nil")
 	}
 
 	// Get the object in the request

pkg/webhook/alias.go

@@ -23,6 +23,12 @@ import (
 
 // define some aliases for common bits of the webhook functionality
 
+// Defaulter defines functions for setting defaults on resources
+type Defaulter = admission.Defaulter
+
+// Validator defines functions for validating an operation
+type Validator = admission.Validator
+
 // AdmissionRequest defines the input for an admission handler.
 // It contains information to identify the object in
 // question (group, version, kind, resource, subresource,

pkg/webhook/server.go

@@ -27,7 +27,6 @@ import (
 	"sync"
 	"time"
 
-	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
 )
@@ -58,9 +57,6 @@ type Server struct {
 	// the user is responsible to mount the secret to the this location for the server to consume.
 	CertDir string
 
-	// Scheme knows the mapping between go struct types and GVKs.
-	Scheme *runtime.Scheme
-
 	// TODO(directxman12): should we make the mux configurable?
 
 	// webhookMux is the multiplexer that handles different webhooks.