diff --git a/pkg/api/meta/meta.go b/pkg/api/meta/meta.go index 6509fbabfd7..70b3f745832 100644 --- a/pkg/api/meta/meta.go +++ b/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) diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index 51167c9edd9..7275a742e41 100644 --- a/pkg/conversion/converter.go +++ b/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, diff --git a/pkg/conversion/meta.go b/pkg/conversion/meta.go index d82968bae8d..357f099351e 100644 --- a/pkg/conversion/meta.go +++ b/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 } diff --git a/pkg/conversion/meta_test.go b/pkg/conversion/meta_test.go index b4c6c0771b2..f7ede632123 100644 --- a/pkg/conversion/meta_test.go +++ b/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") + } +} diff --git a/pkg/resources/resources_test.go b/pkg/resources/resources_test.go index e4bce94f2e9..e7748754008 100644 --- a/pkg/resources/resources_test.go +++ b/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) } } } diff --git a/pkg/runtime/helper.go b/pkg/runtime/helper.go index dcede13cfa8..a94f8745a01 100644 --- a/pkg/runtime/helper.go +++ b/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) diff --git a/pkg/runtime/scheme_test.go b/pkg/runtime/scheme_test.go index 42b75311705..dd5a59418a1 100644 --- a/pkg/runtime/scheme_test.go +++ b/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(`{ }`) diff --git a/pkg/tools/etcd_tools.go b/pkg/tools/etcd_tools.go index fc40c19674f..0a5c0a31d27 100644 --- a/pkg/tools/etcd_tools.go +++ b/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