Merge pull request #2001 from VojtechVitek/fix_scheme_panic

Fix reflect panic in runtime/conversion

pkg/api/meta/meta.go

@@ -224,11 +224,10 @@ func fieldPtr(v reflect.Value, fieldName string, dest interface{}) error {
 	if !field.IsValid() {
 		return fmt.Errorf("Couldn't find %v field in %#v", fieldName, v.Interface())
 	}
-	v = reflect.ValueOf(dest)
-	if v.Kind() != reflect.Ptr {
-		return fmt.Errorf("dest should be ptr")
+	v, err := conversion.EnforcePtr(dest)
+	if err != nil {
+		return err
 	}
-	v = v.Elem()
 	field = field.Addr()
 	if field.Type().AssignableTo(v.Type()) {
 		v.Set(field)

pkg/conversion/converter.go

@@ -213,18 +213,17 @@ func (f FieldMatchingFlags) IsSet(flag FieldMatchingFlags) bool {
 // it is not used by Convert() other than storing it in the scope.
 // Not safe for objects with cyclic references!
 func (c *Converter) Convert(src, dest interface{}, flags FieldMatchingFlags, meta *Meta) error {
-	dv, sv := reflect.ValueOf(dest), reflect.ValueOf(src)
-	if dv.Kind() != reflect.Ptr {
-		return fmt.Errorf("Need pointer, but got %#v", dest)
+	dv, err := EnforcePtr(dest)
+	if err != nil {
+		return err
 	}
-	if sv.Kind() != reflect.Ptr {
-		return fmt.Errorf("Need pointer, but got %#v", src)
-	}
-	dv = dv.Elem()
-	sv = sv.Elem()
 	if !dv.CanAddr() {
 		return fmt.Errorf("Can't write to dest")
 	}
+	sv, err := EnforcePtr(src)
+	if err != nil {
+		return err
+	}
 	s := &scope{
 		converter: c,
 		flags:     flags,

pkg/conversion/meta.go

@@ -113,7 +113,13 @@ func UpdateVersionAndKind(baseFields []string, versionField, version, kindField,
 func EnforcePtr(obj interface{}) (reflect.Value, error) {
 	v := reflect.ValueOf(obj)
 	if v.Kind() != reflect.Ptr {
-		return reflect.Value{}, fmt.Errorf("expected pointer, but got %v", v.Type().Name())
+		if v.Kind() == reflect.Invalid {
+			return reflect.Value{}, fmt.Errorf("expected pointer, but got invalid kind")
+		}
+		return reflect.Value{}, fmt.Errorf("expected pointer, but got %v type", v.Type().Name())
+	}
+	if v.IsNil() {
+		return reflect.Value{}, fmt.Errorf("expected pointer, but got nil")
 	}
 	return v.Elem(), nil
 }

pkg/conversion/meta_test.go

@@ -242,3 +242,22 @@ func TestMetaValuesUnregisteredConvert(t *testing.T) {
 		t.Errorf("Expected %v, got %v", e, a)
 	}
 }
+
+func TestInvalidPtrValueKind(t *testing.T) {
+	var simple interface{}
+	switch obj := simple.(type) {
+	default:
+		_, err := EnforcePtr(obj)
+		if err == nil {
+			t.Errorf("Expected error on invalid kind")
+		}
+	}
+}
+
+func TestEnforceNilPtr(t *testing.T) {
+	var nilPtr *struct{}
+	_, err := EnforcePtr(nilPtr)
+	if err == nil {
+		t.Errorf("Expected error on nil pointer")
+	}
+}

pkg/resources/resources_test.go

@@ -130,7 +130,7 @@ func TestGetFloat(t *testing.T) {
 	for _, test := range tests {
 		val := GetFloatResource(test.res, test.name, test.def)
 		if val != test.expected {
-			t.Errorf("%expected: %d found %d", test.expected, val)
+			t.Errorf("expected: %d found %d", test.expected, val)
 		}
 	}
 }

pkg/runtime/helper.go

@@ -19,6 +19,8 @@ package runtime
 import (
 	"fmt"
 	"reflect"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 )
 
 // GetItemsPtr returns a pointer to the list object's Items member.
@@ -26,11 +28,11 @@ import (
 // and an error will be returned.
 // This function will either return a pointer to a slice, or an error, but not both.
 func GetItemsPtr(list Object) (interface{}, error) {
-	v := reflect.ValueOf(list)
-	if !v.IsValid() {
-		return nil, fmt.Errorf("nil list object")
+	v, err := conversion.EnforcePtr(list)
+	if err != nil {
+		return nil, err
 	}
-	items := v.Elem().FieldByName("Items")
+	items := v.FieldByName("Items")
 	if !items.IsValid() {
 		return nil, fmt.Errorf("no Items field in %#v", list)
 	}
@@ -47,13 +49,16 @@ func ExtractList(obj Object) ([]Object, error) {
 	if err != nil {
 		return nil, err
 	}
-	items := reflect.ValueOf(itemsPtr).Elem()
+	items, err := conversion.EnforcePtr(itemsPtr)
+	if err != nil {
+		return nil, err
+	}
 	list := make([]Object, items.Len())
 	for i := range list {
 		raw := items.Index(i)
 		item, ok := raw.Addr().Interface().(Object)
 		if !ok {
-			return nil, fmt.Errorf("item in index %v isn't an object: %#v", i, raw.Interface())
+			return nil, fmt.Errorf("item[%v]: Expected object, got %#v", i, raw.Interface())
 		}
 		list[i] = item
 	}
@@ -69,21 +74,23 @@ func SetList(list Object, objects []Object) error {
 	if err != nil {
 		return err
 	}
-	items := reflect.ValueOf(itemsPtr).Elem()
+	items, err := conversion.EnforcePtr(itemsPtr)
+	if err != nil {
+		return err
+	}
 	slice := reflect.MakeSlice(items.Type(), len(objects), len(objects))
 	for i := range objects {
 		dest := slice.Index(i)
-		src := reflect.ValueOf(objects[i])
-		if !src.IsValid() || src.IsNil() {
-			return fmt.Errorf("an object was nil")
+		src, err := conversion.EnforcePtr(objects[i])
+		if err != nil {
+			return err
 		}
-		src = src.Elem() // Object is a pointer, but the items in slice are not.
 		if src.Type().AssignableTo(dest.Type()) {
 			dest.Set(src)
 		} else if src.Type().ConvertibleTo(dest.Type()) {
 			dest.Set(src.Convert(dest.Type()))
 		} else {
-			return fmt.Errorf("wrong type: need %v, got %v", dest.Type(), src.Type())
+			return fmt.Errorf("item[%v]: Type mismatch: Expected %v, got %v", dest.Type(), src.Type())
 		}
 	}
 	items.Set(slice)

pkg/runtime/scheme_test.go

@@ -123,6 +123,20 @@ func TestScheme(t *testing.T) {
 	}
 }
 
+func TestInvalidObjectValueKind(t *testing.T) {
+	scheme := runtime.NewScheme()
+	scheme.AddKnownTypeWithName("", "Simple", &InternalSimple{})
+
+	embedded := &runtime.EmbeddedObject{}
+	switch obj := embedded.Object.(type) {
+	default:
+		_, _, err := scheme.ObjectVersionAndKind(obj)
+		if err == nil {
+			t.Errorf("Expected error on invalid kind")
+		}
+	}
+}
+
 func TestBadJSONRejection(t *testing.T) {
 	scheme := runtime.NewScheme()
 	badJSONMissingKind := []byte(`{ }`)

pkg/tools/etcd_tools.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"strconv"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/coreos/go-etcd/etcd"
 )
@@ -169,12 +170,11 @@ func (h *EtcdHelper) ExtractList(key string, slicePtr interface{}, resourceVersi
 
 // decodeNodeList walks the tree of each node in the list and decodes into the specified object
 func (h *EtcdHelper) decodeNodeList(nodes []*etcd.Node, slicePtr interface{}) error {
-	pv := reflect.ValueOf(slicePtr)
-	if pv.Type().Kind() != reflect.Ptr || pv.Type().Elem().Kind() != reflect.Slice {
+	v, err := conversion.EnforcePtr(slicePtr)
+	if err != nil || v.Kind() != reflect.Slice {
 		// This should not happen at runtime.
 		panic("need ptr to slice")
 	}
-	v := pv.Elem()
 	for _, node := range nodes {
 		if node.Dir {
 			if err := h.decodeNodeList(node.Nodes, slicePtr); err != nil {
@@ -230,8 +230,11 @@ func (h *EtcdHelper) bodyAndExtractObj(key string, objPtr runtime.Object, ignore
 	}
 	if err != nil || response.Node == nil || len(response.Node.Value) == 0 {
 		if ignoreNotFound {
-			pv := reflect.ValueOf(objPtr)
-			pv.Elem().Set(reflect.Zero(pv.Type().Elem()))
+			v, err := conversion.EnforcePtr(objPtr)
+			if err != nil {
+				return "", 0, err
+			}
+			v.Set(reflect.Zero(v.Type()))
 			return "", 0, nil
 		} else if err != nil {
 			return "", 0, err
@@ -313,13 +316,13 @@ type EtcdUpdateFunc func(input runtime.Object) (output runtime.Object, err error
 // })
 //
 func (h *EtcdHelper) AtomicUpdate(key string, ptrToType runtime.Object, tryUpdate EtcdUpdateFunc) error {
-	pt := reflect.TypeOf(ptrToType)
-	if pt.Kind() != reflect.Ptr {
+	v, err := conversion.EnforcePtr(ptrToType)
+	if err != nil {
 		// Panic is appropriate, because this is a programming error.
 		panic("need ptr to type")
 	}
 	for {
-		obj := reflect.New(pt.Elem()).Interface().(runtime.Object)
+		obj := reflect.New(v.Type()).Interface().(runtime.Object)
 		origBody, index, err := h.bodyAndExtractObj(key, obj, true)
 		if err != nil {
 			return err