Merge pull request #508 from mengqiy/fixIsConvetible

🐛 IsConvertible should not error on uninitialized struct.

pkg/webhook/conversion/conversion.go

@@ -174,7 +174,10 @@ 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) {
-	gvks := objectGVKs(wh.scheme, obj)
+	gvks, err := objectGVKs(wh.scheme, obj)
+	if err != nil {
+		return nil, err
+	}
 	if len(gvks) == 0 {
 		return nil, fmt.Errorf("error retrieving gvks for object : %v", obj)
 	}
@@ -223,7 +226,10 @@ func (wh *Webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, e
 func IsConvertible(scheme *runtime.Scheme, obj runtime.Object) (bool, error) {
 	var hubs, spokes, nonSpokes []runtime.Object
 
-	gvks := objectGVKs(scheme, obj)
+	gvks, err := objectGVKs(scheme, obj)
+	if err != nil {
+		return false, err
+	}
 	if len(gvks) == 0 {
 		return false, fmt.Errorf("error retrieving gvks for object : %v", obj)
 	}
@@ -273,18 +279,27 @@ func IsConvertible(scheme *runtime.Scheme, obj runtime.Object) (bool, error) {
 }
 
 // objectGVKs returns all (Group,Version,Kind) for the Group/Kind of given object.
-func objectGVKs(scheme *runtime.Scheme, obj runtime.Object) []schema.GroupVersionKind {
-	var gvks []schema.GroupVersionKind
-
-	objGVK := obj.GetObjectKind().GroupVersionKind()
+func objectGVKs(scheme *runtime.Scheme, obj runtime.Object) ([]schema.GroupVersionKind, error) {
+	// NB: we should not use `obj.GetObjectKind().GroupVersionKind()` to get the
+	// GVK here, since it is parsed from apiVersion and kind fields and it may
+	// return empty GVK if obj is an uninitialized object.
+	objGVKs, _, err := scheme.ObjectKinds(obj)
+	if err != nil {
+		return nil, err
+	}
+	if len(objGVKs) != 1 {
+		return nil, fmt.Errorf("expect to get only one GVK for %v", obj)
+	}
+	objGVK := objGVKs[0]
 	knownTypes := scheme.AllKnownTypes()
 
+	var gvks []schema.GroupVersionKind
 	for gvk := range knownTypes {
 		if objGVK.GroupKind() == gvk.GroupKind() {
 			gvks = append(gvks, gvk)
 		}
 	}
-	return gvks
+	return gvks, nil
 }
 
 // PartialImplementationError represents an error due to partial conversion

pkg/webhook/conversion/conversion_test.go

@@ -29,6 +29,7 @@ import (
 	appsv1beta1 "k8s.io/api/apps/v1beta1"
 	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
@@ -312,6 +313,27 @@ var _ = Describe("IsConvertible", func() {
 		Expect(jobsv3.AddToScheme(scheme)).To(Succeed())
 	})
 
+	It("should not error for uninitialized types", func() {
+		obj := &jobsv2.ExternalJob{}
+
+		ok, err := IsConvertible(scheme, obj)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(BeTrue())
+	})
+
+	It("should not error for unstructured types", func() {
+		obj := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"kind":       "ExternalJob",
+				"apiVersion": "jobs.testprojects.kb.io/v2",
+			},
+		}
+
+		ok, err := IsConvertible(scheme, obj)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(BeTrue())
+	})
+
 	It("should return true for convertible types", func() {
 		obj := &jobsv2.ExternalJob{
 			TypeMeta: metav1.TypeMeta{