Merge pull request #2144 from vincepri/fix-conversion-remove-admissio…

…n-decoder

⚠️ Add constructor for conversion webhooks, rm admission.GetDecoder()

pkg/builder/webhook.go

@@ -213,7 +213,7 @@ func (blder *WebhookBuilder) registerConversionWebhook() error {
 	}
 	if ok {
 		if !blder.isAlreadyHandled("/convert") {
-			blder.mgr.GetWebhookServer().Register("/convert", &conversion.Webhook{})
+			blder.mgr.GetWebhookServer().Register("/convert", conversion.NewWebhookHandler(blder.mgr.GetScheme()))
 		}
 		log.Info("Conversion webhook enabled", "GVK", blder.gvk)
 	}

pkg/webhook/admission/webhook.go

@@ -135,9 +135,6 @@ type Webhook struct {
 	// outside the context of requests.
 	LogConstructor func(base logr.Logger, req *Request) logr.Logger
 
-	// decoder is constructed on receiving a scheme and passed down to then handler
-	decoder *Decoder
-
 	setupLogOnce sync.Once
 	log          logr.Logger
 }
@@ -204,12 +201,6 @@ func DefaultLogConstructor(base logr.Logger, req *Request) logr.Logger {
 	return base
 }
 
-// GetDecoder returns a decoder to decode the objects embedded in admission requests.
-// It may be nil if we haven't received a scheme to use to determine object types yet.
-func (wh *Webhook) GetDecoder() *Decoder {
-	return wh.decoder
-}
-
 // StandaloneOptions let you configure a StandaloneWebhook.
 type StandaloneOptions struct {
 	// Logger to be used by the webhook.

pkg/webhook/conversion/conversion.go

@@ -39,16 +39,20 @@ var (
 	log = logf.Log.WithName("conversion-webhook")
 )
 
-// Webhook implements a CRD conversion webhook HTTP handler.
-type Webhook struct {
+func NewWebhookHandler(scheme *runtime.Scheme) http.Handler {
+	return &webhook{scheme: scheme, decoder: NewDecoder(scheme)}
+}
+
+// webhook implements a CRD conversion webhook HTTP handler.
+type webhook struct {
 	scheme  *runtime.Scheme
 	decoder *Decoder
 }
 
 // ensure Webhook implements http.Handler
-var _ http.Handler = &Webhook{}
+var _ http.Handler = &webhook{}
 
-func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	convertReview := &apix.ConversionReview{}
 	err := json.NewDecoder(r.Body).Decode(convertReview)
 	if err != nil {
@@ -83,7 +87,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) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
 	if req == nil {
 		return nil, fmt.Errorf("conversion request is nil")
 	}
@@ -116,7 +120,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) convertObject(src, dst runtime.Object) error {
 	srcGVK := src.GetObjectKind().GroupVersionKind()
 	dstGVK := dst.GetObjectKind().GroupVersionKind()
 
@@ -143,7 +147,7 @@ func (wh *Webhook) convertObject(src, dst runtime.Object) error {
 	}
 }
 
-func (wh *Webhook) convertViaHub(src, dst conversion.Convertible) error {
+func (wh *webhook) convertViaHub(src, dst conversion.Convertible) error {
 	hub, err := wh.getHub(src)
 	if err != nil {
 		return err
@@ -167,7 +171,7 @@ 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) getHub(obj runtime.Object) (conversion.Hub, error) {
 	gvks, err := objectGVKs(wh.scheme, obj)
 	if err != nil {
 		return nil, err
@@ -195,7 +199,7 @@ func (wh *Webhook) getHub(obj runtime.Object) (conversion.Hub, error) {
 }
 
 // allocateDstObject returns an instance for a given GVK.
-func (wh *Webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
+func (wh *webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
 	gvk := schema.FromAPIVersionAndKind(apiVersion, kind)
 
 	obj, err := wh.scheme.New(gvk)

pkg/webhook/conversion/conversion_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package conversion
+package conversion_test
 
 import (
 	"bytes"
@@ -33,6 +33,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
+	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
 	jobsv1 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v1"
 	jobsv2 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v2"
 	jobsv3 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v3"
@@ -41,9 +42,9 @@ import (
 var _ = Describe("Conversion Webhook", func() {
 
 	var respRecorder *httptest.ResponseRecorder
-	var decoder *Decoder
+	var decoder *conversion.Decoder
 	var scheme *runtime.Scheme
-	var webhook *Webhook
+	var wh http.Handler
 
 	BeforeEach(func() {
 		respRecorder = &httptest.ResponseRecorder{
@@ -56,8 +57,8 @@ var _ = Describe("Conversion Webhook", func() {
 		Expect(jobsv2.AddToScheme(scheme)).To(Succeed())
 		Expect(jobsv3.AddToScheme(scheme)).To(Succeed())
 
-		decoder = NewDecoder(scheme)
-		webhook = &Webhook{scheme: scheme, decoder: decoder}
+		decoder = conversion.NewDecoder(scheme)
+		wh = conversion.NewWebhookHandler(scheme)
 	})
 
 	doRequest := func(convReq *apix.ConversionReview) *apix.ConversionReview {
@@ -69,7 +70,7 @@ var _ = Describe("Conversion Webhook", func() {
 		req := &http.Request{
 			Body: io.NopCloser(bytes.NewReader(payload.Bytes())),
 		}
-		webhook.ServeHTTP(respRecorder, req)
+		wh.ServeHTTP(respRecorder, req)
 		Expect(json.NewDecoder(respRecorder.Result().Body).Decode(convReview)).To(Succeed())
 		return convReview
 	}
@@ -313,7 +314,7 @@ var _ = Describe("IsConvertible", func() {
 	It("should not error for uninitialized types", func() {
 		obj := &jobsv2.ExternalJob{}
 
-		ok, err := IsConvertible(scheme, obj)
+		ok, err := conversion.IsConvertible(scheme, obj)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).To(BeTrue())
 	})
@@ -326,7 +327,7 @@ var _ = Describe("IsConvertible", func() {
 			},
 		}
 
-		ok, err := IsConvertible(scheme, obj)
+		ok, err := conversion.IsConvertible(scheme, obj)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).To(BeTrue())
 	})
@@ -339,7 +340,7 @@ var _ = Describe("IsConvertible", func() {
 			},
 		}
 
-		ok, err := IsConvertible(scheme, obj)
+		ok, err := conversion.IsConvertible(scheme, obj)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).To(BeTrue())
 	})
@@ -352,7 +353,7 @@ var _ = Describe("IsConvertible", func() {
 			},
 		}
 
-		ok, err := IsConvertible(scheme, obj)
+		ok, err := conversion.IsConvertible(scheme, obj)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).ToNot(BeTrue())
 	})