From 9f5b249507bfebfd9a725d31462f5d877dde63e3 Mon Sep 17 00:00:00 2001
From: Mengqi Yu <mengqiy@google.com>
Date: Fri, 28 Jun 2019 14:10:30 -0700
Subject: [PATCH] :bug: IsConvertible should not error on uninitialized struct.

IsConvertible used to use obj.GetObjectKind().GroupVersionKind() to get
GVK which will not work if apiVersion and kind fields are not set. Now
it uses scheme.ObjectKinds(obj).
---
 pkg/webhook/conversion/conversion.go      | 29 +++++++++++++++++------
 pkg/webhook/conversion/conversion_test.go | 22 +++++++++++++++++
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/pkg/webhook/conversion/conversion.go b/pkg/webhook/conversion/conversion.go
index f9bd5cb2db..05760cbbc4 100644
--- a/pkg/webhook/conversion/conversion.go
+++ b/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
diff --git a/pkg/webhook/conversion/conversion_test.go b/pkg/webhook/conversion/conversion_test.go
index 1f2e1dc0e3..64ca7b575d 100644
--- a/pkg/webhook/conversion/conversion_test.go
+++ b/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{