Factor out API defaulting from validation logic

Currently, the validation logic validates fields in an object and supply default values wherever applies. This change factors out defaulting to a set of defaulting callback functions for decoding (see kubernetes#1502 for more discussion). * This change is based on pull request 2587. * Most defaulting has been migrated to defaults.go where the defaulting functions are added. * validation_test.go and converter_test.go have been adapted to not testing the default values. * Fixed all tests with that create invalid objects with the absence of defaulting logic.
duda · Feb 3, 2015 · 4a72add · 4a72add
1 parent 1ddb68d
commit 4a72add
Show file tree

Hide file tree

Showing 40 changed files with 1,058 additions and 383 deletions.
diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go
@@ -319,6 +319,8 @@ func runSelfLinkTest(c *client.Client) {
 				Selector: map[string]string{
 					"foo": "bar",
 				},
+				Protocol:        "TCP",
+				SessionAffinity: "None",
 			},
 		},
 	).Do().Into(&svc)
@@ -381,6 +383,8 @@ func runAtomicPutTest(c *client.Client) {
 				Selector: map[string]string{
 					"foo": "bar",
 				},
+				Protocol:        "TCP",
+				SessionAffinity: "None",
 			},
 		},
 	).Do().Into(&svc)
@@ -521,8 +525,11 @@ func runServiceTest(client *client.Client) {
 					Ports: []api.Port{
 						{ContainerPort: 1234},
 					},
+					ImagePullPolicy: "PullIfNotPresent",
 				},
 			},
+			RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}},
+			DNSPolicy:     api.DNSClusterFirst,
 		},
 		Status: api.PodStatus{
 			PodIP: "1.2.3.4",
@@ -541,7 +548,9 @@ func runServiceTest(client *client.Client) {
 			Selector: map[string]string{
 				"name": "thisisalonglabel",
 			},
-			Port: 8080,
+			Port:            8080,
+			Protocol:        "TCP",
+			SessionAffinity: "None",
 		},
 	}
 	_, err = client.Services(api.NamespaceDefault).Create(&svc1)
@@ -558,7 +567,9 @@ func runServiceTest(client *client.Client) {
 			Selector: map[string]string{
 				"name": "thisisalonglabel",
 			},
-			Port: 8080,
+			Port:            8080,
+			Protocol:        "TCP",
+			SessionAffinity: "None",
 		},
 	}
 	_, err = client.Services(api.NamespaceDefault).Create(&svc2)

diff --git a/pkg/api/conversion.go b/pkg/api/conversion.go
@@ -47,14 +47,6 @@ func init() {
 			out.Spec.DNSPolicy = in.DNSPolicy
 			out.Name = in.ID
 			out.UID = in.UUID
-			// TODO(dchen1107): Move this conversion to pkg/api/v1beta[123]/conversion.go
-			// along with fixing #1502
-			for i := range out.Spec.Containers {
-				ctr := &out.Spec.Containers[i]
-				if len(ctr.TerminationMessagePath) == 0 {
-					ctr.TerminationMessagePath = TerminationMessagePathDefault
-				}
-			}
 			return nil
 		},
 		func(in *BoundPod, out *ContainerManifest, s conversion.Scope) error {
@@ -65,12 +57,6 @@ func init() {
 			out.Version = "v1beta2"
 			out.ID = in.Name
 			out.UUID = in.UID
-			for i := range out.Containers {
-				ctr := &out.Containers[i]
-				if len(ctr.TerminationMessagePath) == 0 {
-					ctr.TerminationMessagePath = TerminationMessagePathDefault
-				}
-			}
 			return nil
 		},
 

diff --git a/pkg/api/serialization_test.go b/pkg/api/serialization_test.go
@@ -158,6 +158,10 @@ func TestEncode_Ptr(t *testing.T) {
 		ObjectMeta: api.ObjectMeta{
 			Labels: map[string]string{"name": "foo"},
 		},
+		Spec: api.PodSpec{
+			RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}},
+			DNSPolicy:     api.DNSClusterFirst,
+		},
 	}
 	obj := runtime.Object(pod)
 	data, err := latest.Codec.Encode(obj)
@@ -169,7 +173,7 @@ func TestEncode_Ptr(t *testing.T) {
 		t.Fatalf("Got wrong type")
 	}
 	if !api.Semantic.DeepEqual(obj2, pod) {
-		t.Errorf("Expected:\n %#v,\n Got:\n %#v", &pod, obj2)
+		t.Errorf("Expected:\n %#v,\n Got:\n %#v", pod, obj2)
 	}
 }
 

diff --git a/pkg/api/testing/fuzzer.go b/pkg/api/testing/fuzzer.go
@@ -32,6 +32,14 @@ import (
 	"speter.net/go/exp/math/dec/inf"
 )
 
+func fuzzOneOf(c fuzz.Continue, objs ...interface{}) {
+	// Use a new fuzzer which cannot populate nil to ensure one obj will be set.
+	// FIXME: would be nicer to use FuzzOnePtr() and reflect.
+	f := fuzz.New().NilChance(0).NumElements(1, 1)
+	i := c.RandUint64() % uint64(len(objs))
+	f.Fuzz(objs[i])
+}
+
 // FuzzerFor can randomly populate api objects that are destined for version.
 func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
 	f := fuzz.New().NilChance(.5).NumElements(1, 1)
@@ -84,15 +92,18 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
 			statuses := []api.PodPhase{api.PodPending, api.PodRunning, api.PodFailed, api.PodUnknown}
 			*j = statuses[c.Rand.Intn(len(statuses))]
 		},
+		func(j *api.PodTemplateSpec, c fuzz.Continue) {
+			// TODO: v1beta1/2 can't round trip a nil template correctly, fix by having v1beta1/2
+			// conversion compare converted object to nil via DeepEqual
+			j.ObjectMeta = api.ObjectMeta{}
+			c.Fuzz(&j.ObjectMeta)
+			j.ObjectMeta = api.ObjectMeta{Labels: j.ObjectMeta.Labels}
+			j.Spec = api.PodSpec{}
+			c.Fuzz(&j.Spec)
+		},
 		func(j *api.ReplicationControllerSpec, c fuzz.Continue) {
-			// TemplateRef must be nil for round trip
+			// TemplateRef is set to nil by omission; this is required for round trip
 			c.Fuzz(&j.Template)
-			if j.Template == nil {
-				// TODO: v1beta1/2 can't round trip a nil template correctly, fix by having v1beta1/2
-				// conversion compare converted object to nil via DeepEqual
-				j.Template = &api.PodTemplateSpec{}
-			}
-			j.Template.ObjectMeta = api.ObjectMeta{Labels: j.Template.ObjectMeta.Labels}
 			c.Fuzz(&j.Selector)
 			j.Replicas = int(c.RandUint64())
 		},
@@ -160,6 +171,46 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
 			policies := []api.PullPolicy{api.PullAlways, api.PullNever, api.PullIfNotPresent}
 			*p = policies[c.Rand.Intn(len(policies))]
 		},
+		func(rp *api.RestartPolicy, c fuzz.Continue) {
+			// Exactly one of the fields should be set.
+			fuzzOneOf(c, &rp.Always, &rp.OnFailure, &rp.Never)
+		},
+		func(vs *api.VolumeSource, c fuzz.Continue) {
+			// Exactly one of the fields should be set.
+			//FIXME: the fuzz can still end up nil.  What if fuzz allowed me to say that?
+			fuzzOneOf(c, &vs.HostPath, &vs.EmptyDir, &vs.GCEPersistentDisk, &vs.GitRepo)
+		},
+		func(d *api.DNSPolicy, c fuzz.Continue) {
+			policies := []api.DNSPolicy{api.DNSClusterFirst, api.DNSDefault}
+			*d = policies[c.Rand.Intn(len(policies))]
+		},
+		func(p *api.Protocol, c fuzz.Continue) {
+			protocols := []api.Protocol{api.ProtocolTCP, api.ProtocolUDP}
+			*p = protocols[c.Rand.Intn(len(protocols))]
+		},
+		func(p *api.AffinityType, c fuzz.Continue) {
+			types := []api.AffinityType{api.AffinityTypeClientIP, api.AffinityTypeNone}
+			*p = types[c.Rand.Intn(len(types))]
+		},
+		func(ct *api.Container, c fuzz.Continue) {
+			// This function exists soley to set TerminationMessagePath to a
+			// non-empty string. TODO: consider making TerminationMessagePath a
+			// new type to simplify fuzzing.
+			ct.TerminationMessagePath = api.TerminationMessagePathDefault
+			// Let fuzzer handle the rest of the fileds.
+			c.Fuzz(&ct.Name)
+			c.Fuzz(&ct.Image)
+			c.Fuzz(&ct.Command)
+			c.Fuzz(&ct.Ports)
+			c.Fuzz(&ct.WorkingDir)
+			c.Fuzz(&ct.Env)
+			c.Fuzz(&ct.VolumeMounts)
+			c.Fuzz(&ct.LivenessProbe)
+			c.Fuzz(&ct.Lifecycle)
+			c.Fuzz(&ct.ImagePullPolicy)
+			c.Fuzz(&ct.Privileged)
+			c.Fuzz(&ct.Capabilities)
+		},
 	)
 	return f
 }
diff --git a/pkg/api/v1beta1/defaults.go b/pkg/api/v1beta1/defaults.go
@@ -17,15 +17,17 @@ limitations under the License.
 package v1beta1
 
 import (
+	"strings"
+
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 func init() {
 	api.Scheme.AddDefaultingFuncs(
 		func(obj *Volume) {
-			if obj.Source == nil || util.AllPtrFieldsNil(obj.Source) {
-				obj.Source = &VolumeSource{
+			if util.AllPtrFieldsNil(&obj.Source) {
+				obj.Source = VolumeSource{
 					EmptyDir: &EmptyDir{},
 				}
 			}
@@ -36,13 +38,18 @@ func init() {
 			}
 		},
 		func(obj *Container) {
-			// TODO: delete helper functions that touch this
 			if obj.ImagePullPolicy == "" {
-				obj.ImagePullPolicy = PullIfNotPresent
+				// TODO(dchen1107): Move ParseImageName code to pkg/util
+				parts := strings.Split(obj.Image, ":")
+				// Check image tag
+				if parts[len(parts)-1] == "latest" {
+					obj.ImagePullPolicy = PullAlways
+				} else {
+					obj.ImagePullPolicy = PullIfNotPresent
+				}
 			}
 			if obj.TerminationMessagePath == "" {
-				// TODO: fix other code that sets this
-				obj.TerminationMessagePath = api.TerminationMessagePathDefault
+				obj.TerminationMessagePath = TerminationMessagePathDefault
 			}
 		},
 		func(obj *RestartPolicy) {
@@ -54,8 +61,19 @@ func init() {
 			if obj.Protocol == "" {
 				obj.Protocol = ProtocolTCP
 			}
+			if obj.SessionAffinity == "" {
+				obj.SessionAffinity = AffinityTypeNone
+			}
+		},
+		func(obj *PodSpec) {
+			if obj.DNSPolicy == "" {
+				obj.DNSPolicy = DNSClusterFirst
+			}
+		},
+		func(obj *ContainerManifest) {
+			if obj.DNSPolicy == "" {
+				obj.DNSPolicy = DNSClusterFirst
+			}
 		},
 	)
 }
-
-// TODO: remove redundant code in validation and conversion
diff --git a/pkg/api/v1beta1/defaults_test.go b/pkg/api/v1beta1/defaults_test.go
@@ -0,0 +1,65 @@
+/*
+Copyright 2015 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta1_test
+
+import (
+	"reflect"
+	"testing"
+
+	current "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+)
+
+func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
+	data, err := current.Codec.Encode(obj)
+	if err != nil {
+		t.Errorf("%v\n %#v", err, obj)
+		return nil
+	}
+	obj2 := reflect.New(reflect.TypeOf(obj).Elem()).Interface().(runtime.Object)
+	err = current.Codec.DecodeInto(data, obj2)
+	if err != nil {
+		t.Errorf("%v\nData: %s\nSource: %#v", err, string(data), obj)
+		return nil
+	}
+	return obj2
+}
+
+func TestSetDefaultService(t *testing.T) {
+	svc := &current.Service{}
+	obj2 := roundTrip(t, runtime.Object(svc))
+	svc2 := obj2.(*current.Service)
+	if svc2.Protocol != current.ProtocolTCP {
+		t.Errorf("Expected default protocol :%s, got: %s", current.ProtocolTCP, svc2.Protocol)
+	}
+	if svc2.SessionAffinity != current.AffinityTypeNone {
+		t.Errorf("Expected default sesseion affinity type:%s, got: %s", current.AffinityTypeNone, svc2.SessionAffinity)
+	}
+}
+
+func TestSetDefaulPodSpec(t *testing.T) {
+	bp := &current.BoundPod{}
+	obj2 := roundTrip(t, runtime.Object(bp))
+	bp2 := obj2.(*current.BoundPod)
+	if bp2.Spec.DNSPolicy != current.DNSClusterFirst {
+		t.Errorf("Expected default dns policy :%s, got: %s", current.DNSClusterFirst, bp2.Spec.DNSPolicy)
+	}
+	policy := bp2.Spec.RestartPolicy
+	if policy.Never != nil || policy.OnFailure != nil || policy.Always == nil {
+		t.Errorf("Expected only policy.Aways is set, got: %s", policy)
+	}
+}
diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go
@@ -71,6 +71,11 @@ type ContainerManifestList struct {
 	Items    []ContainerManifest `json:"items" description:"list of pod container manifests"`
 }
 
+const (
+	// TerminationMessagePathDefault means the default path to capture the application termination message running in a container
+	TerminationMessagePathDefault string = "/dev/termination-log"
+)
+
 // Volume represents a named volume in a pod that may be accessed by any containers in the pod.
 type Volume struct {
 	// Required: This must be a DNS_LABEL.  Each volume in a pod must have