Make defaulting part of versioning codec

Most normal codec use should perform defaulting. DirectCodecs should not
perform defaulting. Update the defaulting_test to fuzz the list of known
defaulters. Use the new versioning.NewDefaultingCodec() method.

pkg/api/defaulting_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2015 The Kubernetes Authors.
+Copyright 2016 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import (
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	apiv1 "k8s.io/kubernetes/pkg/api/v1"
+	batchv2alpha1 "k8s.io/kubernetes/pkg/apis/batch/v2alpha1"
 	extensionsv1beta1 "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/diff"
@@ -40,14 +41,73 @@ func (o orderedGroupVersionKinds) Less(i, j int) bool {
 	return o[i].String() < o[j].String()
 }
 
-// TODO: add a reflexive test that verifies that all SetDefaults functions are registered
+func TestVerifyDefaulting(t *testing.T) {
+	job := &batchv2alpha1.JobTemplate{}
+	batchv2alpha1.SetObjectDefaults_JobTemplate(job)
+	if job.Template.Spec.Template.Spec.DNSPolicy != apiv1.DNSClusterFirst {
+		t.Errorf("unexpected defaulting: %#v", job)
+	}
+}
 
-// TODO: once we remove defaulting from conversion, convert this test to ensuring
+// TODO: add a reflexive test that verifies that all SetDefaults functions are registered
-// that all objects that say they have defaulting are verified to mutate the originating
-// object.
 func TestDefaulting(t *testing.T) {
-	f := fuzz.New().NilChance(.5).NumElements(1, 1)
+	// these are the known types with defaulters - you must add to this list if you add a top level defaulter
-	f.RandSource(rand.NewSource(1))
+	typesWithDefaulting := map[unversioned.GroupVersionKind]struct{}{
+		{Group: "", Version: "v1", Kind: "ConfigMap"}:                                             {},
+		{Group: "", Version: "v1", Kind: "ConfigMapList"}:                                         {},
+		{Group: "", Version: "v1", Kind: "Endpoints"}:                                             {},
+		{Group: "", Version: "v1", Kind: "EndpointsList"}:                                         {},
+		{Group: "", Version: "v1", Kind: "Namespace"}:                                             {},
+		{Group: "", Version: "v1", Kind: "NamespaceList"}:                                         {},
+		{Group: "", Version: "v1", Kind: "Node"}:                                                  {},
+		{Group: "", Version: "v1", Kind: "NodeList"}:                                              {},
+		{Group: "", Version: "v1", Kind: "PersistentVolume"}:                                      {},
+		{Group: "", Version: "v1", Kind: "PersistentVolumeList"}:                                  {},
+		{Group: "", Version: "v1", Kind: "PersistentVolumeClaim"}:                                 {},
+		{Group: "", Version: "v1", Kind: "PersistentVolumeClaimList"}:                             {},
+		{Group: "", Version: "v1", Kind: "PodAttachOptions"}:                                      {},
+		{Group: "", Version: "v1", Kind: "PodExecOptions"}:                                        {},
+		{Group: "", Version: "v1", Kind: "Pod"}:                                                   {},
+		{Group: "", Version: "v1", Kind: "PodList"}:                                               {},
+		{Group: "", Version: "v1", Kind: "PodTemplate"}:                                           {},
+		{Group: "", Version: "v1", Kind: "PodTemplateList"}:                                       {},
+		{Group: "", Version: "v1", Kind: "ReplicationController"}:                                 {},
+		{Group: "", Version: "v1", Kind: "ReplicationControllerList"}:                             {},
+		{Group: "", Version: "v1", Kind: "Secret"}:                                                {},
+		{Group: "", Version: "v1", Kind: "SecretList"}:                                            {},
+		{Group: "", Version: "v1", Kind: "Service"}:                                               {},
+		{Group: "", Version: "v1", Kind: "ServiceList"}:                                           {},
+		{Group: "apps", Version: "v1alpha1", Kind: "PetSet"}:                                      {},
+		{Group: "apps", Version: "v1alpha1", Kind: "PetSetList"}:                                  {},
+		{Group: "autoscaling", Version: "v1", Kind: "HorizontalPodAutoscaler"}:                    {},
+		{Group: "autoscaling", Version: "v1", Kind: "HorizontalPodAutoscalerList"}:                {},
+		{Group: "batch", Version: "v1", Kind: "Job"}:                                              {},
+		{Group: "batch", Version: "v1", Kind: "JobList"}:                                          {},
+		{Group: "batch", Version: "v2alpha1", Kind: "Job"}:                                        {},
+		{Group: "batch", Version: "v2alpha1", Kind: "JobList"}:                                    {},
+		{Group: "batch", Version: "v2alpha1", Kind: "JobTemplate"}:                                {},
+		{Group: "batch", Version: "v2alpha1", Kind: "ScheduledJob"}:                               {},
+		{Group: "batch", Version: "v2alpha1", Kind: "ScheduledJobList"}:                           {},
+		{Group: "componentconfig", Version: "v1alpha1", Kind: "KubeProxyConfiguration"}:           {},
+		{Group: "componentconfig", Version: "v1alpha1", Kind: "KubeSchedulerConfiguration"}:       {},
+		{Group: "componentconfig", Version: "v1alpha1", Kind: "KubeletConfiguration"}:             {},
+		{Group: "extensions", Version: "v1beta1", Kind: "DaemonSet"}:                              {},
+		{Group: "extensions", Version: "v1beta1", Kind: "DaemonSetList"}:                          {},
+		{Group: "extensions", Version: "v1beta1", Kind: "Deployment"}:                             {},
+		{Group: "extensions", Version: "v1beta1", Kind: "DeploymentList"}:                         {},
+		{Group: "extensions", Version: "v1beta1", Kind: "HorizontalPodAutoscaler"}:                {},
+		{Group: "extensions", Version: "v1beta1", Kind: "HorizontalPodAutoscalerList"}:            {},
+		{Group: "extensions", Version: "v1beta1", Kind: "Job"}:                                    {},
+		{Group: "extensions", Version: "v1beta1", Kind: "JobList"}:                                {},
+		{Group: "extensions", Version: "v1beta1", Kind: "ReplicaSet"}:                             {},
+		{Group: "extensions", Version: "v1beta1", Kind: "ReplicaSetList"}:                         {},
+		{Group: "rbac.authorization.k8s.io", Version: "v1alpha1", Kind: "ClusterRoleBinding"}:     {},
+		{Group: "rbac.authorization.k8s.io", Version: "v1alpha1", Kind: "ClusterRoleBindingList"}: {},
+		{Group: "rbac.authorization.k8s.io", Version: "v1alpha1", Kind: "RoleBinding"}:            {},
+		{Group: "rbac.authorization.k8s.io", Version: "v1alpha1", Kind: "RoleBindingList"}:        {},
+	}
+
+	f := fuzz.New().NilChance(.5).NumElements(1, 1).RandSource(rand.NewSource(1))
 	f.Funcs(
 		func(s *runtime.RawExtension, c fuzz.Continue) {},
 		func(s *unversioned.LabelSelector, c fuzz.Continue) {
@@ -59,16 +119,6 @@ func TestDefaulting(t *testing.T) {
 			s.LabelSelector = "" // need to fuzz requirement strings specially
 			s.FieldSelector = "" // need to fuzz requirement strings specially
 		},
-		// No longer necessary when we remove defaulting from conversion
-		func(s *apiv1.Secret, c fuzz.Continue) {
-			c.FuzzNoCustom(s)
-			s.StringData = nil // is mapped into Data, which cannot easily be tested
-		},
-		func(s *extensionsv1beta1.ListOptions, c fuzz.Continue) {
-			c.FuzzNoCustom(s)
-			s.LabelSelector = "" // need to fuzz requirement strings specially
-			s.FieldSelector = "" // need to fuzz requirement strings specially
-		},
 		func(s *extensionsv1beta1.ScaleStatus, c fuzz.Continue) {
 			c.FuzzNoCustom(s)
 			s.TargetSelector = "" // need to fuzz requirement strings specially
@@ -86,7 +136,23 @@ func TestDefaulting(t *testing.T) {
 	sort.Sort(testTypes)
 
 	for _, gvk := range testTypes {
-		for i := 0; i < *fuzzIters; i++ {
+		_, expectedChanged := typesWithDefaulting[gvk]
+		iter := 0
+		changedOnce := false
+		for {
+			if iter > *fuzzIters {
+				if !expectedChanged || changedOnce {
+					break
+				}
+				if iter > 200 {
+					t.Errorf("expected %s to trigger defaulting due to fuzzing", gvk)
+					break
+				}
+				// if we expected defaulting, continue looping until the fuzzer gives us one
+				// at worst, we will timeout
+			}
+			iter++
+
 			src, err := scheme.New(gvk)
 			if err != nil {
 				t.Fatal(err)
@@ -95,30 +161,38 @@ func TestDefaulting(t *testing.T) {
 
 			src.GetObjectKind().SetGroupVersionKind(unversioned.GroupVersionKind{})
 
-			original, _ := scheme.DeepCopy(src)
+			original, err := scheme.DeepCopy(src)
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			// get internal
-			copied, _ := scheme.DeepCopy(src)
+			withDefaults, _ := scheme.DeepCopy(src)
-			scheme.Default(copied.(runtime.Object))
+			scheme.Default(withDefaults.(runtime.Object))
 
-			// get expected
+			if !reflect.DeepEqual(original, withDefaults) {
-			// TODO: this relies on the side effect behavior of defaulters applying to the external
+				changedOnce = true
-			// object
+				if !expectedChanged {
-			if _, err = scheme.UnsafeConvertToVersion(src, runtime.InternalGroupVersioner); err != nil {
+					t.Errorf("{Group: \"%s\", Version: \"%s\", Kind: \"%s\"} did not expect defaults to be set - update expected or check defaulter registering: %s", gvk.Group, gvk.Version, gvk.Kind, diff.ObjectReflectDiff(original, withDefaults))
-				t.Errorf("[%v] unable to convert: %v", gvk, err)
+				}
-				continue
+			}
+		}
 	}
-			src.GetObjectKind().SetGroupVersionKind(unversioned.GroupVersionKind{})
-
-			existingChanged := !reflect.DeepEqual(original, src)
-			newChanged := !reflect.DeepEqual(original, copied)
-			if existingChanged != newChanged {
-				t.Errorf("[%v] mismatched changes: old=%t new=%t", gvk, existingChanged, newChanged)
 }
 
-			if !reflect.DeepEqual(src, copied) {
+func BenchmarkPodDefaulting(b *testing.B) {
-				t.Errorf("[%v] changed: %s", gvk, diff.ObjectReflectDiff(src, copied))
+	f := fuzz.New().NilChance(.5).NumElements(1, 1).RandSource(rand.NewSource(1))
-			}
+	items := make([]apiv1.Pod, 100)
+	for i := range items {
+		f.Fuzz(&items[i])
 	}
+
+	scheme := api.Scheme
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		pod := &items[i%len(items)]
+
+		scheme.Default(pod)
 	}
+	b.StopTimer()
 }

pkg/client/unversioned/clientcmd/api/latest/latest.go

@@ -56,7 +56,7 @@ func init() {
 		panic(err)
 	}
 	yamlSerializer := json.NewYAMLSerializer(json.DefaultMetaFactory, Scheme, Scheme)
-	Codec = versioning.NewCodecForScheme(
+	Codec = versioning.NewDefaultingCodecForScheme(
 		Scheme,
 		yamlSerializer,
 		yamlSerializer,

pkg/runtime/codec.go

@@ -76,6 +76,24 @@ func EncodeOrDie(e Encoder, obj Object) string {
 	return string(bytes)
 }
 
+// DefaultingSerializer invokes defaulting after decoding.
+type DefaultingSerializer struct {
+	Defaulter ObjectDefaulter
+	Decoder   Decoder
+	// Encoder is optional to allow this type to be used as both a Decoder and an Encoder
+	Encoder
+}
+
+// Decode performs a decode and then allows the defaulter to act on the provided object.
+func (d DefaultingSerializer) Decode(data []byte, defaultGVK *unversioned.GroupVersionKind, into Object) (Object, *unversioned.GroupVersionKind, error) {
+	obj, gvk, err := d.Decoder.Decode(data, defaultGVK, into)
+	if err != nil {
+		return obj, gvk, err
+	}
+	d.Defaulter.Default(obj)
+	return obj, gvk, nil
+}
+
 // UseOrCreateObject returns obj if the canonical ObjectKind returned by the provided typer matches gvk, or
 // invokes the ObjectCreator to instantiate a new gvk. Returns an error if the typer cannot find the object.
 func UseOrCreateObject(t ObjectTyper, c ObjectCreater, gvk unversioned.GroupVersionKind, obj Object) (Object, error) {

pkg/runtime/interfaces.go

@@ -169,6 +169,12 @@ type NestedObjectDecoder interface {
 ///////////////////////////////////////////////////////////////////////////////
 // Non-codec interfaces
 
+type ObjectDefaulter interface {
+	// Default takes an object (must be a pointer) and applies any default values.
+	// Defaulters may not error.
+	Default(in Object)
+}
+
 type ObjectVersioner interface {
 	ConvertToVersion(in Object, gv GroupVersioner) (out Object, err error)
 }

pkg/runtime/serializer/codec_factory.go

@@ -196,7 +196,7 @@ func (f CodecFactory) SupportedStreamingMediaTypes() []string {
 // TODO: make this call exist only in pkg/api, and initialize it with the set of default versions.
 //   All other callers will be forced to request a Codec directly.
 func (f CodecFactory) LegacyCodec(version ...unversioned.GroupVersion) runtime.Codec {
-	return versioning.NewCodecForScheme(f.scheme, f.legacySerializer, f.universal, unversioned.GroupVersions(version), runtime.InternalGroupVersioner)
+	return versioning.NewDefaultingCodecForScheme(f.scheme, f.legacySerializer, f.universal, unversioned.GroupVersions(version), runtime.InternalGroupVersioner)
 }
 
 // UniversalDeserializer can convert any stored data recognized by this factory into a Go object that satisfies
@@ -235,7 +235,7 @@ func (f CodecFactory) CodecForVersions(encoder runtime.Encoder, decoder runtime.
 	if decode == nil {
 		decode = runtime.InternalGroupVersioner
 	}
-	return versioning.NewCodecForScheme(f.scheme, encoder, decoder, encode, decode)
+	return versioning.NewDefaultingCodecForScheme(f.scheme, encoder, decoder, encode, decode)
 }
 
 // DecoderToVersion returns a decoder that targets the provided group version.

pkg/runtime/serializer/versioning/versioning.go

@@ -21,6 +21,7 @@ import (
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/runtime"
+	utilruntime "k8s.io/kubernetes/pkg/util/runtime"
 )
 
 // NewCodecForScheme is a convenience method for callers that are using a scheme.
@@ -32,7 +33,19 @@ func NewCodecForScheme(
 	encodeVersion runtime.GroupVersioner,
 	decodeVersion runtime.GroupVersioner,
 ) runtime.Codec {
-	return NewCodec(encoder, decoder, runtime.UnsafeObjectConvertor(scheme), scheme, scheme, scheme, encodeVersion, decodeVersion)
+	return NewCodec(encoder, decoder, runtime.UnsafeObjectConvertor(scheme), scheme, scheme, scheme, nil, encodeVersion, decodeVersion)
+}
+
+// NewDefaultingCodecForScheme is a convenience method for callers that are using a scheme.
+func NewDefaultingCodecForScheme(
+	// TODO: I should be a scheme interface?
+	scheme *runtime.Scheme,
+	encoder runtime.Encoder,
+	decoder runtime.Decoder,
+	encodeVersion runtime.GroupVersioner,
+	decodeVersion runtime.GroupVersioner,
+) runtime.Codec {
+	return NewCodec(encoder, decoder, runtime.UnsafeObjectConvertor(scheme), scheme, scheme, scheme, scheme, encodeVersion, decodeVersion)
 }
 
 // NewCodec takes objects in their internal versions and converts them to external versions before
@@ -45,6 +58,7 @@ func NewCodec(
 	creater runtime.ObjectCreater,
 	copier runtime.ObjectCopier,
 	typer runtime.ObjectTyper,
+	defaulter runtime.ObjectDefaulter,
 	encodeVersion runtime.GroupVersioner,
 	decodeVersion runtime.GroupVersioner,
 ) runtime.Codec {
@@ -55,6 +69,7 @@ func NewCodec(
 		creater:   creater,
 		copier:    copier,
 		typer:     typer,
+		defaulter: defaulter,
 
 		encodeVersion: encodeVersion,
 		decodeVersion: decodeVersion,
@@ -69,6 +84,7 @@ type codec struct {
 	creater   runtime.ObjectCreater
 	copier    runtime.ObjectCopier
 	typer     runtime.ObjectTyper
+	defaulter runtime.ObjectDefaulter
 
 	encodeVersion runtime.GroupVersioner
 	decodeVersion runtime.GroupVersioner
@@ -102,11 +118,31 @@ func (c *codec) Decode(data []byte, defaultGVK *unversioned.GroupVersionKind, in
 			}
 			return into, gvk, nil
 		}
+
+		// perform defaulting if requested
+		if c.defaulter != nil {
+			// create a copy to ensure defaulting is not applied to the original versioned objects
+			if isVersioned {
+				copied, err := c.copier.Copy(obj)
+				if err != nil {
+					utilruntime.HandleError(err)
+					copied = obj
+				}
+				versioned.Objects = []runtime.Object{copied}
+			}
+			c.defaulter.Default(obj)
+		} else {
+			if isVersioned {
+				versioned.Objects = []runtime.Object{obj}
+			}
+		}
+
 		if err := c.convertor.Convert(obj, into, c.decodeVersion); err != nil {
 			return nil, gvk, err
 		}
+
 		if isVersioned {
-			versioned.Objects = []runtime.Object{obj, into}
+			versioned.Objects = append(versioned.Objects, into)
 			return versioned, gvk, nil
 		}
 		return into, gvk, nil
@@ -117,10 +153,17 @@ func (c *codec) Decode(data []byte, defaultGVK *unversioned.GroupVersionKind, in
 		// create a copy, because ConvertToVersion does not guarantee non-mutation of objects
 		copied, err := c.copier.Copy(obj)
 		if err != nil {
+			utilruntime.HandleError(err)
 			copied = obj
 		}
 		versioned.Objects = []runtime.Object{copied}
 	}
+
+	// perform defaulting if requested
+	if c.defaulter != nil {
+		c.defaulter.Default(obj)
+	}
+
 	out, err := c.convertor.ConvertToVersion(obj, c.decodeVersion)
 	if err != nil {
 		return nil, gvk, err

pkg/runtime/serializer/versioning/versioning_test.go

@@ -64,7 +64,7 @@ func (d *testNestedDecodable) DecodeNestedObjects(_ runtime.Decoder) error {
 func TestNestedDecode(t *testing.T) {
 	n := &testNestedDecodable{nestedErr: fmt.Errorf("unable to decode")}
 	decoder := &mockSerializer{obj: n}
-	codec := NewCodec(nil, decoder, nil, nil, nil, nil, nil, nil)
+	codec := NewCodec(nil, decoder, nil, nil, nil, nil, nil, nil, nil)
 	if _, _, err := codec.Decode([]byte(`{}`), nil, n); err != n.nestedErr {
 		t.Errorf("unexpected error: %v", err)
 	}
@@ -82,6 +82,7 @@ func TestNestedEncode(t *testing.T) {
 		&checkConvertor{obj: n2, groupVersion: unversioned.GroupVersion{Group: "other"}},
 		nil, nil,
 		&mockTyper{gvks: []unversioned.GroupVersionKind{{Kind: "test"}}},
+		nil,
 		unversioned.GroupVersion{Group: "other"}, nil,
 	)
 	if err := codec.Encode(n, ioutil.Discard); err != n2.nestedErr {
@@ -105,6 +106,7 @@ func TestDecode(t *testing.T) {
 		creater    runtime.ObjectCreater
 		copier     runtime.ObjectCopier
 		typer      runtime.ObjectTyper
+		defaulter  runtime.ObjectDefaulter
 		yaml       bool
 		pretty     bool
 
@@ -235,7 +237,7 @@ func TestDecode(t *testing.T) {
 
 	for i, test := range testCases {
 		t.Logf("%d", i)
-		s := NewCodec(test.serializer, test.serializer, test.convertor, test.creater, test.copier, test.typer, test.encodes, test.decodes)
+		s := NewCodec(test.serializer, test.serializer, test.convertor, test.creater, test.copier, test.typer, test.defaulter, test.encodes, test.decodes)
 		obj, gvk, err := s.Decode([]byte(`{}`), test.defaultGVK, test.into)
 
 		if !reflect.DeepEqual(test.expectedGVK, gvk) {

plugin/pkg/scheduler/api/latest/latest.go

@@ -43,7 +43,7 @@ var Codec runtime.Codec
 
 func init() {
 	jsonSerializer := json.NewSerializer(json.DefaultMetaFactory, api.Scheme, api.Scheme, true)
-	Codec = versioning.NewCodecForScheme(
+	Codec = versioning.NewDefaultingCodecForScheme(
 		api.Scheme,
 		jsonSerializer,
 		jsonSerializer,

test/integration/framework/serializer.go

@@ -58,9 +58,9 @@ func (s *wrappedSerializer) UniversalDeserializer() runtime.Decoder {
 }
 
 func (s *wrappedSerializer) EncoderForVersion(encoder runtime.Encoder, gv runtime.GroupVersioner) runtime.Encoder {
-	return versioning.NewCodec(encoder, nil, s.scheme, s.scheme, s.scheme, s.scheme, gv, nil)
+	return versioning.NewCodec(encoder, nil, s.scheme, s.scheme, s.scheme, s.scheme, s.scheme, gv, nil)
 }
 
 func (s *wrappedSerializer) DecoderToVersion(decoder runtime.Decoder, gv runtime.GroupVersioner) runtime.Decoder {
-	return versioning.NewCodec(nil, decoder, s.scheme, s.scheme, s.scheme, s.scheme, nil, gv)
+	return versioning.NewCodec(nil, decoder, s.scheme, s.scheme, s.scheme, s.scheme, s.scheme, nil, gv)
 }