✨ address review comments

Author: droot

pkg/webhook/conversion/testData/pkg/apis/addtoscheme_jobs_v1.go renamed to examples/conversion/pkg/apis/addtoscheme_jobs_v1.go

@@ -16,7 +16,7 @@ limitations under the License.
 package apis
 
 import (
-	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis/jobs/v1"
+	"sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis/jobs/v1"
 )
 
 func init() {

pkg/webhook/conversion/testData/pkg/apis/addtoscheme_jobs_v2.go renamed to examples/conversion/pkg/apis/addtoscheme_jobs_v2.go

@@ -16,7 +16,7 @@ limitations under the License.
 package apis
 
 import (
-	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis/jobs/v2"
+	"sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis/jobs/v2"
 )
 
 func init() {

pkg/webhook/conversion/testData/pkg/apis/apis.go renamed to examples/conversion/pkg/apis/apis.go

@@ -19,8 +19,8 @@ import (
 	"fmt"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis/jobs/v2"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis/jobs/v2"
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!

pkg/webhook/conversion/testData/pkg/apis/jobs/group.go renamed to examples/conversion/pkg/apis/jobs/group.go

@@ -14,6 +14,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+/*
+Package conversion provides interface definitions that an API Type needs to
+implement in order to be supported by the generic conversion webhook handler
+defined under pkg/webhook/conversion.
+*/
 package conversion
 
 import "k8s.io/apimachinery/pkg/runtime"
@@ -25,9 +30,10 @@ type Convertible interface {
 	ConvertFrom(src Hub) error
 }
 
-// Hub defines capability to indicate whether a versioned type is a Hub or not.
-// Default conversion handler will use this interface to implement spoke to
-// spoke conversion.
+// Hub marks that a given type is the hub type for conversion. This means that
+// all conversions will first convert to the hub type, then convert from the hub
+// type to the destination type. All types besides the hub type should implement
+// Convertible.
 type Hub interface {
 	runtime.Object
 	Hub()

pkg/webhook/conversion/testData/pkg/apis/jobs/v1/doc.go renamed to examples/conversion/pkg/apis/jobs/v1/doc.go

@@ -14,24 +14,29 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+/*
+Package conversion provides implementation for CRD conversion webhook that implements handler for version conversion requests for types that are convertible.
+
+See pkg/conversion for interface definitions required to ensure a Type is convertible.
+*/
 package conversion
 
 import (
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"net/http"
 
 	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 var (
-	log = logf.Log.WithName("conversion_webhook")
+	log = logf.Log.WithName("conversion-webhook")
 )
 
 // Webhook implements a CRD conversion webhook HTTP handler.
@@ -49,22 +54,15 @@ func (wh *Webhook) InjectScheme(s *runtime.Scheme) error {
 		return err
 	}
 
-	// inject the decoder here too, just in case the order of calling this is not
-	// scheme first, then inject func
-	// if w.Handler != nil {
-	// 	if _, err := InjectDecoderInto(w.GetDecoder(), w.Handler); err != nil {
-	// 		return err
-	// 	}
-	// }
-
 	return nil
 }
 
 // ensure Webhook implements http.Handler
 var _ http.Handler = &Webhook{}
 
 func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	convertReview, err := wh.readRequest(r)
+	convertReview := &apix.ConversionReview{}
+	err := json.NewDecoder(r.Body).Decode(convertReview)
 	if err != nil {
 		log.Error(err, "failed to read conversion request")
 		w.WriteHeader(http.StatusBadRequest)
@@ -89,28 +87,6 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func (wh *Webhook) readRequest(r *http.Request) (*apix.ConversionReview, error) {
-
-	var body []byte
-	if r.Body == nil {
-		return nil, fmt.Errorf("nil request body")
-	}
-	data, err := ioutil.ReadAll(r.Body)
-	if err != nil {
-		return nil, err
-	}
-	body = data
-
-	convertReview := &apix.ConversionReview{}
-	// TODO(droot): figure out if we want to split decoder for conversion
-	// request from the objects contained in the request
-	err = wh.decoder.DecodeInto(body, convertReview)
-	if err != nil {
-		return nil, err
-	}
-	return convertReview, nil
-}
-
 // handles a version conversion request.
 func (wh *Webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
 	if req == nil {
@@ -142,58 +118,49 @@ 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
-// nolint: gocyclo
 func (wh *Webhook) convertObject(src, dst runtime.Object) error {
 	srcGVK := src.GetObjectKind().GroupVersionKind()
 	dstGVK := dst.GetObjectKind().GroupVersionKind()
 
-	if srcGVK.GroupKind().String() != dstGVK.GroupKind().String() {
+	if srcGVK.GroupKind() != dstGVK.GroupKind() {
 		return fmt.Errorf("src %T and dst %T does not belong to same API Group", src, dst)
 	}
 
-	if srcGVK.String() == dstGVK.String() {
+	if srcGVK == dstGVK {
 		return fmt.Errorf("conversion is not allowed between same type %T", src)
 	}
 
 	srcIsHub, dstIsHub := isHub(src), isHub(dst)
 	srcIsConvertible, dstIsConvertible := isConvertible(src), isConvertible(dst)
 
-	if srcIsHub {
-		if dstIsConvertible {
-			return dst.(conversion.Convertible).ConvertFrom(src.(conversion.Hub))
-		}
+	switch {
+	case srcIsHub && dstIsConvertible:
+		return dst.(conversion.Convertible).ConvertFrom(src.(conversion.Hub))
+	case dstIsHub && srcIsConvertible:
+		return src.(conversion.Convertible).ConvertTo(dst.(conversion.Hub))
+	case srcIsConvertible && dstIsConvertible:
+		return wh.convertViaHub(src.(conversion.Convertible), dst.(conversion.Convertible))
+	default:
 		return fmt.Errorf("%T is not convertible to %T", src, dst)
 	}
+}
 
-	if dstIsHub {
-		if srcIsConvertible {
-			return src.(conversion.Convertible).ConvertTo(dst.(conversion.Hub))
-		}
-		return fmt.Errorf("%T is not convertible %T", dst, src)
-	}
-
-	// neither src nor dst are Hub, means both of them are spoke, so lets get the hub
-	// version type.
+func (wh *Webhook) convertViaHub(src, dst conversion.Convertible) error {
 	hub, err := wh.getHub(src)
 	if err != nil {
 		return err
 	}
 
 	if hub == nil {
-		return fmt.Errorf("API Group %s does not have any Hub defined", srcGVK)
+		return fmt.Errorf("%s does not have any Hub defined", src)
 	}
 
-	// src and dst needs to be convertable for it to work
-	if !srcIsConvertible || !dstIsConvertible {
-		return fmt.Errorf("%T and %T needs to be convertable", src, dst)
-	}
-
-	err = src.(conversion.Convertible).ConvertTo(hub)
+	err = src.ConvertTo(hub)
 	if err != nil {
 		return fmt.Errorf("%T failed to convert to hub version %T : %v", src, hub, err)
 	}
 
-	err = dst.(conversion.Convertible).ConvertFrom(hub)
+	err = dst.ConvertFrom(hub)
 	if err != nil {
 		return fmt.Errorf("%T failed to convert from hub version %T : %v", dst, hub, err)
 	}
@@ -256,3 +223,13 @@ func isConvertible(obj runtime.Object) bool {
 	_, yes := obj.(conversion.Convertible)
 	return yes
 }
+
+// helper to construct error response.
+func errored(err error) *apix.ConversionResponse {
+	return &apix.ConversionResponse{
+		Result: metav1.Status{
+			Status:  metav1.StatusFailure,
+			Message: err.Error(),
+		},
+	}
+}

pkg/webhook/conversion/testData/pkg/apis/jobs/v1/externaljob_types.go renamed to examples/conversion/pkg/apis/jobs/v1/externaljob_types.go

@@ -35,4 +35,4 @@ var _ = BeforeSuite(func(done Done) {
 	logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))
 
 	close(done)
-}, 60)
+})

pkg/webhook/conversion/testData/pkg/apis/jobs/v1/register.go renamed to examples/conversion/pkg/apis/jobs/v1/register.go

@@ -32,9 +32,9 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
-	jobsapis "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis"
-	jobsv1 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis/jobs/v1"
-	jobsv2 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testData/pkg/apis/jobs/v2"
+	jobsapis "sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis"
+	jobsv1 "sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis/jobs/v1"
+	jobsv2 "sigs.k8s.io/controller-runtime/examples/conversion/pkg/apis/jobs/v2"
 )
 
 var _ = Describe("Conversion Webhook", func() {
@@ -74,9 +74,8 @@ var _ = Describe("Conversion Webhook", func() {
 		return convReview
 	}
 
-	It("should convert objects successfully", func() {
-
-		v1Obj := &jobsv1.ExternalJob{
+	makeV1Obj := func() *jobsv1.ExternalJob {
+		return &jobsv1.ExternalJob{
 			TypeMeta: metav1.TypeMeta{
 				Kind:       "ExternalJob",
 				APIVersion: "jobs.example.org/v1",
@@ -89,6 +88,11 @@ var _ = Describe("Conversion Webhook", func() {
 				RunAt: "every 2 seconds",
 			},
 		}
+	}
+
+	It("should convert objects successfully", func() {
+
+		v1Obj := makeV1Obj()
 
 		expected := &jobsv2.ExternalJob{
 			TypeMeta: metav1.TypeMeta{
@@ -118,26 +122,14 @@ var _ = Describe("Conversion Webhook", func() {
 
 		convReview := doRequest(convReq)
 
-		Expect(convReview.Response.ConvertedObjects).Should(HaveLen(1))
+		Expect(convReview.Response.ConvertedObjects).To(HaveLen(1))
 		got, _, err := decoder.Decode(convReview.Response.ConvertedObjects[0].Raw)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(got).To(Equal(expected))
 	})
 
 	It("should return error when dest/src objects belong to different API groups", func() {
-		v1Obj := &jobsv1.ExternalJob{
-			TypeMeta: metav1.TypeMeta{
-				Kind:       "ExternalJob",
-				APIVersion: "jobs.example.org/v1",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "obj-1",
-			},
-			Spec: jobsv1.ExternalJobSpec{
-				RunAt: "every 2 seconds",
-			},
-		}
+		v1Obj := makeV1Obj()
 
 		convReq := &apix.ConversionReview{
 			TypeMeta: metav1.TypeMeta{},
@@ -159,19 +151,8 @@ var _ = Describe("Conversion Webhook", func() {
 	})
 
 	It("should return error when dest/src objects are of same type", func() {
-		v1Obj := &jobsv1.ExternalJob{
-			TypeMeta: metav1.TypeMeta{
-				Kind:       "ExternalJob",
-				APIVersion: "jobs.example.org/v1",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "default",
-				Name:      "obj-1",
-			},
-			Spec: jobsv1.ExternalJobSpec{
-				RunAt: "every 2 seconds",
-			},
-		}
+
+		v1Obj := makeV1Obj()
 
 		convReq := &apix.ConversionReview{
 			TypeMeta: metav1.TypeMeta{},
@@ -220,7 +201,7 @@ var _ = Describe("Conversion Webhook", func() {
 		convReview := doRequest(convReq)
 
 		Expect(convReview.Response.Result.Message).To(
-			Equal("API Group apps/v1beta1, Kind=Deployment does not have any Hub defined"))
+			Equal("*v1beta1.Deployment is not convertible to *v1.Deployment"))
 		Expect(convReview.Response.ConvertedObjects).Should(BeEmpty())
 	})