Skip to content

Commit

Permalink
🐛 IsConvertible should not error on uninitialized struct.
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
Mengqi Yu committed Jun 28, 2019
1 parent 4c81828 commit 9f5b249
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 7 deletions.
29 changes: 22 additions & 7 deletions pkg/webhook/conversion/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions pkg/webhook/conversion/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit 9f5b249

Please sign in to comment.