Merge pull request #3531 from lavalamp/fix2

make conversion do deep copies

pkg/api/conversion.go

@@ -17,8 +17,10 @@ limitations under the License.
 package api
 
 import (
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 // Codec is the identity codec for this package - it can only convert itself
@@ -27,6 +29,16 @@ var Codec = runtime.CodecFor(Scheme, "")
 
 func init() {
 	Scheme.AddConversionFuncs(
+		func(in *util.Time, out *util.Time, s conversion.Scope) error {
+			// Cannot deep copy these, because time.Time has unexported fields.
+			*out = *in
+			return nil
+		},
+		func(in *resource.Quantity, out *resource.Quantity, s conversion.Scope) error {
+			// Cannot deep copy these, because inf.Dec has unexported fields.
+			*out = *in.Copy()
+			return nil
+		},
 		// Convert ContainerManifest to BoundPod
 		func(in *ContainerManifest, out *BoundPod, s conversion.Scope) error {
 			out.Spec.Containers = in.Containers

pkg/conversion/converter.go

@@ -394,20 +394,29 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error {
 func (c *Converter) defaultConvert(sv, dv reflect.Value, scope *scope) error {
 	dt, st := dv.Type(), sv.Type()
 
+	if !dv.CanSet() {
+		return scope.error("Cannot set dest. (Tried to deep copy something with unexported fields?)")
+	}
+
 	if !scope.flags.IsSet(AllowDifferentFieldTypeNames) && c.NameFunc(dt) != c.NameFunc(st) {
 		return scope.error(
 			"type names don't match (%v, %v), and no conversion 'func (%v, %v) error' registered.",
 			c.NameFunc(st), c.NameFunc(dt), st, dt)
 	}
 
-	// This should handle all simple types.
-	if st.AssignableTo(dt) {
-		dv.Set(sv)
-		return nil
-	}
-	if st.ConvertibleTo(dt) {
-		dv.Set(sv.Convert(dt))
-		return nil
+	switch st.Kind() {
+	case reflect.Map, reflect.Ptr, reflect.Slice, reflect.Interface, reflect.Struct:
+		// Don't copy these via assignment/conversion!
+	default:
+		// This should handle all simple types.
+		if st.AssignableTo(dt) {
+			dv.Set(sv)
+			return nil
+		}
+		if st.ConvertibleTo(dt) {
+			dv.Set(sv.Convert(dt))
+			return nil
+		}
 	}
 
 	if c.Debug != nil {
@@ -466,6 +475,18 @@ func (c *Converter) defaultConvert(sv, dv reflect.Value, scope *scope) error {
 			}
 			dv.SetMapIndex(dk, dkv)
 		}
+	case reflect.Interface:
+		if sv.IsNil() {
+			// Don't copy a nil interface!
+			dv.Set(reflect.Zero(dt))
+			return nil
+		}
+		tmpdv := reflect.New(sv.Elem().Type()).Elem()
+		if err := c.convert(sv.Elem(), tmpdv, scope); err != nil {
+			return err
+		}
+		dv.Set(reflect.ValueOf(tmpdv.Interface()))
+		return nil
 	default:
 		return scope.error("couldn't copy '%v' into '%v'; didn't understand types", st, dt)
 	}

pkg/conversion/converter_test.go

@@ -66,6 +66,46 @@ func TestConverter_DefaultConvert(t *testing.T) {
 	}
 }
 
+func TestConverter_DeepCopy(t *testing.T) {
+	type A struct {
+		Foo *string
+		Bar []string
+		Baz interface{}
+		Qux map[string]string
+	}
+	c := NewConverter()
+	c.Debug = t
+
+	foo, baz := "foo", "baz"
+	x := A{
+		Foo: &foo,
+		Bar: []string{"bar"},
+		Baz: &baz,
+		Qux: map[string]string{"qux": "qux"},
+	}
+	y := A{}
+
+	if err := c.Convert(&x, &y, 0, nil); err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	*x.Foo = "foo2"
+	x.Bar[0] = "bar2"
+	*x.Baz.(*string) = "baz2"
+	x.Qux["qux"] = "qux2"
+	if e, a := *x.Foo, *y.Foo; e == a {
+		t.Errorf("expected difference between %v and %v", e, a)
+	}
+	if e, a := x.Bar, y.Bar; reflect.DeepEqual(e, a) {
+		t.Errorf("expected difference between %v and %v", e, a)
+	}
+	if e, a := *x.Baz.(*string), *y.Baz.(*string); e == a {
+		t.Errorf("expected difference between %v and %v", e, a)
+	}
+	if e, a := x.Qux, y.Qux; reflect.DeepEqual(e, a) {
+		t.Errorf("expected difference between %v and %v", e, a)
+	}
+}
+
 func TestConverter_CallsRegisteredFunctions(t *testing.T) {
 	type A struct {
 		Foo string