Merge pull request #78914 from liggitt/pod-spec-defaults

Add tests to detect default changes to podspec and podspectemplate defaults

pkg/apis/core/v1/defaults_test.go

@@ -17,14 +17,17 @@ limitations under the License.
 package v1_test
 
 import (
+	"encoding/json"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	corev1 "k8s.io/kubernetes/pkg/apis/core/v1"
@@ -34,6 +37,279 @@ import (
 	_ "k8s.io/kubernetes/pkg/api/testapi"
 )
 
+// TestWorkloadDefaults detects changes to defaults within PodTemplateSpec.
+// Defaulting changes within this type can cause spurious rollouts of workloads on API server update.
+func TestWorkloadDefaults(t *testing.T) {
+	rc := &v1.ReplicationController{Spec: v1.ReplicationControllerSpec{Template: &v1.PodTemplateSpec{}}}
+	template := rc.Spec.Template
+	// New defaults under PodTemplateSpec are only acceptable if they would not be applied when reading data from a previous release.
+	// Forbidden: adding a new field `MyField *bool` and defaulting it to a non-nil value
+	// Forbidden: defaulting an existing field `MyField *bool` when it was previously not defaulted
+	// Forbidden: changing an existing default value
+	// Allowed: adding a new field `MyContainer *MyType` and defaulting a child of that type (e.g. `MyContainer.MyChildField`) if and only if MyContainer is non-nil
+	expectedDefaults := map[string]string{
+		".Spec.Containers[0].Env[0].ValueFrom.FieldRef.APIVersion":  `"v1"`,
+		".Spec.Containers[0].ImagePullPolicy":                       `"IfNotPresent"`,
+		".Spec.Containers[0].Lifecycle.PostStart.HTTPGet.Path":      `"/"`,
+		".Spec.Containers[0].Lifecycle.PostStart.HTTPGet.Scheme":    `"HTTP"`,
+		".Spec.Containers[0].Lifecycle.PreStop.HTTPGet.Path":        `"/"`,
+		".Spec.Containers[0].Lifecycle.PreStop.HTTPGet.Scheme":      `"HTTP"`,
+		".Spec.Containers[0].LivenessProbe.FailureThreshold":        `3`,
+		".Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Path":    `"/"`,
+		".Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Scheme":  `"HTTP"`,
+		".Spec.Containers[0].LivenessProbe.PeriodSeconds":           `10`,
+		".Spec.Containers[0].LivenessProbe.SuccessThreshold":        `1`,
+		".Spec.Containers[0].LivenessProbe.TimeoutSeconds":          `1`,
+		".Spec.Containers[0].Ports[0].Protocol":                     `"TCP"`,
+		".Spec.Containers[0].ReadinessProbe.FailureThreshold":       `3`,
+		".Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Path":   `"/"`,
+		".Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Scheme": `"HTTP"`,
+		".Spec.Containers[0].ReadinessProbe.PeriodSeconds":          `10`,
+		".Spec.Containers[0].ReadinessProbe.SuccessThreshold":       `1`,
+		".Spec.Containers[0].ReadinessProbe.TimeoutSeconds":         `1`,
+		".Spec.Containers[0].TerminationMessagePath":                `"/dev/termination-log"`,
+		".Spec.Containers[0].TerminationMessagePolicy":              `"File"`,
+		".Spec.DNSPolicy": `"ClusterFirst"`,
+		".Spec.InitContainers[0].Env[0].ValueFrom.FieldRef.APIVersion":  `"v1"`,
+		".Spec.InitContainers[0].ImagePullPolicy":                       `"IfNotPresent"`,
+		".Spec.InitContainers[0].Lifecycle.PostStart.HTTPGet.Path":      `"/"`,
+		".Spec.InitContainers[0].Lifecycle.PostStart.HTTPGet.Scheme":    `"HTTP"`,
+		".Spec.InitContainers[0].Lifecycle.PreStop.HTTPGet.Path":        `"/"`,
+		".Spec.InitContainers[0].Lifecycle.PreStop.HTTPGet.Scheme":      `"HTTP"`,
+		".Spec.InitContainers[0].LivenessProbe.FailureThreshold":        `3`,
+		".Spec.InitContainers[0].LivenessProbe.Handler.HTTPGet.Path":    `"/"`,
+		".Spec.InitContainers[0].LivenessProbe.Handler.HTTPGet.Scheme":  `"HTTP"`,
+		".Spec.InitContainers[0].LivenessProbe.PeriodSeconds":           `10`,
+		".Spec.InitContainers[0].LivenessProbe.SuccessThreshold":        `1`,
+		".Spec.InitContainers[0].LivenessProbe.TimeoutSeconds":          `1`,
+		".Spec.InitContainers[0].Ports[0].Protocol":                     `"TCP"`,
+		".Spec.InitContainers[0].ReadinessProbe.FailureThreshold":       `3`,
+		".Spec.InitContainers[0].ReadinessProbe.Handler.HTTPGet.Path":   `"/"`,
+		".Spec.InitContainers[0].ReadinessProbe.Handler.HTTPGet.Scheme": `"HTTP"`,
+		".Spec.InitContainers[0].ReadinessProbe.PeriodSeconds":          `10`,
+		".Spec.InitContainers[0].ReadinessProbe.SuccessThreshold":       `1`,
+		".Spec.InitContainers[0].ReadinessProbe.TimeoutSeconds":         `1`,
+		".Spec.InitContainers[0].TerminationMessagePath":                `"/dev/termination-log"`,
+		".Spec.InitContainers[0].TerminationMessagePolicy":              `"File"`,
+		".Spec.RestartPolicy":                                                                         `"Always"`,
+		".Spec.SchedulerName":                                                                         `"default-scheduler"`,
+		".Spec.SecurityContext":                                                                       `{}`,
+		".Spec.TerminationGracePeriodSeconds":                                                         `30`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.CachingMode":                                         `"ReadWrite"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.FSType":                                              `"ext4"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.Kind":                                                `"Shared"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.ReadOnly":                                            `false`,
+		".Spec.Volumes[0].VolumeSource.ConfigMap.DefaultMode":                                         `420`,
+		".Spec.Volumes[0].VolumeSource.DownwardAPI.DefaultMode":                                       `420`,
+		".Spec.Volumes[0].VolumeSource.DownwardAPI.Items[0].FieldRef.APIVersion":                      `"v1"`,
+		".Spec.Volumes[0].VolumeSource.EmptyDir":                                                      `{}`,
+		".Spec.Volumes[0].VolumeSource.HostPath.Type":                                                 `""`,
+		".Spec.Volumes[0].VolumeSource.ISCSI.ISCSIInterface":                                          `"default"`,
+		".Spec.Volumes[0].VolumeSource.Projected.DefaultMode":                                         `420`,
+		".Spec.Volumes[0].VolumeSource.Projected.Sources[0].DownwardAPI.Items[0].FieldRef.APIVersion": `"v1"`,
+		".Spec.Volumes[0].VolumeSource.Projected.Sources[0].ServiceAccountToken.ExpirationSeconds":    `3600`,
+		".Spec.Volumes[0].VolumeSource.RBD.Keyring":                                                   `"/etc/ceph/keyring"`,
+		".Spec.Volumes[0].VolumeSource.RBD.RBDPool":                                                   `"rbd"`,
+		".Spec.Volumes[0].VolumeSource.RBD.RadosUser":                                                 `"admin"`,
+		".Spec.Volumes[0].VolumeSource.ScaleIO.FSType":                                                `"xfs"`,
+		".Spec.Volumes[0].VolumeSource.ScaleIO.StorageMode":                                           `"ThinProvisioned"`,
+		".Spec.Volumes[0].VolumeSource.Secret.DefaultMode":                                            `420`,
+	}
+	defaults := detectDefaults(t, rc, reflect.ValueOf(template))
+	if !reflect.DeepEqual(expectedDefaults, defaults) {
+		t.Errorf("Defaults for PodTemplateSpec changed. This can cause spurious rollouts of workloads on API server upgrade.")
+		t.Logf(diff.ObjectReflectDiff(expectedDefaults, defaults))
+	}
+}
+
+// TestPodDefaults detects changes to defaults within PodSpec.
+// Defaulting changes within this type (*especially* within containers) can cause kubelets to restart containers on API server update.
+func TestPodDefaults(t *testing.T) {
+	pod := &v1.Pod{}
+	// New defaults under PodSpec are only acceptable if they would not be applied when reading data from a previous release.
+	// Forbidden: adding a new field `MyField *bool` and defaulting it to a non-nil value
+	// Forbidden: defaulting an existing field `MyField *bool` when it was previously not defaulted
+	// Forbidden: changing an existing default value
+	// Allowed: adding a new field `MyContainer *MyType` and defaulting a child of that type (e.g. `MyContainer.MyChildField`) if and only if MyContainer is non-nil
+	expectedDefaults := map[string]string{
+		".Spec.Containers[0].Env[0].ValueFrom.FieldRef.APIVersion":  `"v1"`,
+		".Spec.Containers[0].ImagePullPolicy":                       `"IfNotPresent"`,
+		".Spec.Containers[0].Lifecycle.PostStart.HTTPGet.Path":      `"/"`,
+		".Spec.Containers[0].Lifecycle.PostStart.HTTPGet.Scheme":    `"HTTP"`,
+		".Spec.Containers[0].Lifecycle.PreStop.HTTPGet.Path":        `"/"`,
+		".Spec.Containers[0].Lifecycle.PreStop.HTTPGet.Scheme":      `"HTTP"`,
+		".Spec.Containers[0].LivenessProbe.FailureThreshold":        `3`,
+		".Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Path":    `"/"`,
+		".Spec.Containers[0].LivenessProbe.Handler.HTTPGet.Scheme":  `"HTTP"`,
+		".Spec.Containers[0].LivenessProbe.PeriodSeconds":           `10`,
+		".Spec.Containers[0].LivenessProbe.SuccessThreshold":        `1`,
+		".Spec.Containers[0].LivenessProbe.TimeoutSeconds":          `1`,
+		".Spec.Containers[0].Ports[0].Protocol":                     `"TCP"`,
+		".Spec.Containers[0].ReadinessProbe.FailureThreshold":       `3`,
+		".Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Path":   `"/"`,
+		".Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Scheme": `"HTTP"`,
+		".Spec.Containers[0].ReadinessProbe.PeriodSeconds":          `10`,
+		".Spec.Containers[0].ReadinessProbe.SuccessThreshold":       `1`,
+		".Spec.Containers[0].ReadinessProbe.TimeoutSeconds":         `1`,
+		".Spec.Containers[0].Resources.Requests":                    `{"":"0"}`, // this gets defaulted from the limits field
+		".Spec.Containers[0].TerminationMessagePath":                `"/dev/termination-log"`,
+		".Spec.Containers[0].TerminationMessagePolicy":              `"File"`,
+		".Spec.DNSPolicy":          `"ClusterFirst"`,
+		".Spec.EnableServiceLinks": `true`,
+		".Spec.InitContainers[0].Env[0].ValueFrom.FieldRef.APIVersion":  `"v1"`,
+		".Spec.InitContainers[0].ImagePullPolicy":                       `"IfNotPresent"`,
+		".Spec.InitContainers[0].Lifecycle.PostStart.HTTPGet.Path":      `"/"`,
+		".Spec.InitContainers[0].Lifecycle.PostStart.HTTPGet.Scheme":    `"HTTP"`,
+		".Spec.InitContainers[0].Lifecycle.PreStop.HTTPGet.Path":        `"/"`,
+		".Spec.InitContainers[0].Lifecycle.PreStop.HTTPGet.Scheme":      `"HTTP"`,
+		".Spec.InitContainers[0].LivenessProbe.FailureThreshold":        `3`,
+		".Spec.InitContainers[0].LivenessProbe.Handler.HTTPGet.Path":    `"/"`,
+		".Spec.InitContainers[0].LivenessProbe.Handler.HTTPGet.Scheme":  `"HTTP"`,
+		".Spec.InitContainers[0].LivenessProbe.PeriodSeconds":           `10`,
+		".Spec.InitContainers[0].LivenessProbe.SuccessThreshold":        `1`,
+		".Spec.InitContainers[0].LivenessProbe.TimeoutSeconds":          `1`,
+		".Spec.InitContainers[0].Ports[0].Protocol":                     `"TCP"`,
+		".Spec.InitContainers[0].ReadinessProbe.FailureThreshold":       `3`,
+		".Spec.InitContainers[0].ReadinessProbe.Handler.HTTPGet.Path":   `"/"`,
+		".Spec.InitContainers[0].ReadinessProbe.Handler.HTTPGet.Scheme": `"HTTP"`,
+		".Spec.InitContainers[0].ReadinessProbe.PeriodSeconds":          `10`,
+		".Spec.InitContainers[0].ReadinessProbe.SuccessThreshold":       `1`,
+		".Spec.InitContainers[0].ReadinessProbe.TimeoutSeconds":         `1`,
+		".Spec.InitContainers[0].Resources.Requests":                    `{"":"0"}`, // this gets defaulted from the limits field
+		".Spec.InitContainers[0].TerminationMessagePath":                `"/dev/termination-log"`,
+		".Spec.InitContainers[0].TerminationMessagePolicy":              `"File"`,
+		".Spec.RestartPolicy":                                                                         `"Always"`,
+		".Spec.SchedulerName":                                                                         `"default-scheduler"`,
+		".Spec.SecurityContext":                                                                       `{}`,
+		".Spec.TerminationGracePeriodSeconds":                                                         `30`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.CachingMode":                                         `"ReadWrite"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.FSType":                                              `"ext4"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.Kind":                                                `"Shared"`,
+		".Spec.Volumes[0].VolumeSource.AzureDisk.ReadOnly":                                            `false`,
+		".Spec.Volumes[0].VolumeSource.ConfigMap.DefaultMode":                                         `420`,
+		".Spec.Volumes[0].VolumeSource.DownwardAPI.DefaultMode":                                       `420`,
+		".Spec.Volumes[0].VolumeSource.DownwardAPI.Items[0].FieldRef.APIVersion":                      `"v1"`,
+		".Spec.Volumes[0].VolumeSource.EmptyDir":                                                      `{}`,
+		".Spec.Volumes[0].VolumeSource.HostPath.Type":                                                 `""`,
+		".Spec.Volumes[0].VolumeSource.ISCSI.ISCSIInterface":                                          `"default"`,
+		".Spec.Volumes[0].VolumeSource.Projected.DefaultMode":                                         `420`,
+		".Spec.Volumes[0].VolumeSource.Projected.Sources[0].DownwardAPI.Items[0].FieldRef.APIVersion": `"v1"`,
+		".Spec.Volumes[0].VolumeSource.Projected.Sources[0].ServiceAccountToken.ExpirationSeconds":    `3600`,
+		".Spec.Volumes[0].VolumeSource.RBD.Keyring":                                                   `"/etc/ceph/keyring"`,
+		".Spec.Volumes[0].VolumeSource.RBD.RBDPool":                                                   `"rbd"`,
+		".Spec.Volumes[0].VolumeSource.RBD.RadosUser":                                                 `"admin"`,
+		".Spec.Volumes[0].VolumeSource.ScaleIO.FSType":                                                `"xfs"`,
+		".Spec.Volumes[0].VolumeSource.ScaleIO.StorageMode":                                           `"ThinProvisioned"`,
+		".Spec.Volumes[0].VolumeSource.Secret.DefaultMode":                                            `420`,
+	}
+	defaults := detectDefaults(t, pod, reflect.ValueOf(pod))
+	if !reflect.DeepEqual(expectedDefaults, defaults) {
+		t.Errorf("Defaults for PodSpec changed. This can cause spurious restarts of containers on API server upgrade.")
+		t.Logf(diff.ObjectReflectDiff(expectedDefaults, defaults))
+	}
+}
+
+type testPath struct {
+	path  string
+	value reflect.Value
+}
+
+func detectDefaults(t *testing.T, obj runtime.Object, v reflect.Value) map[string]string {
+	defaults := map[string]string{}
+	toVisit := []testPath{{path: "", value: v}}
+
+	for len(toVisit) > 0 {
+		visit := toVisit[0]
+		toVisit = toVisit[1:]
+
+		legacyscheme.Scheme.Default(obj)
+		defaultedV := visit.value
+		zeroV := reflect.Zero(visit.value.Type())
+
+		switch {
+		case visit.value.Kind() == reflect.Struct:
+			for fi := 0; fi < visit.value.NumField(); fi++ {
+				structField := visit.value.Type().Field(fi)
+				valueField := visit.value.Field(fi)
+				if valueField.CanSet() {
+					toVisit = append(toVisit, testPath{path: visit.path + "." + structField.Name, value: valueField})
+				}
+			}
+
+		case visit.value.Kind() == reflect.Slice:
+			if !visit.value.IsNil() {
+				// if we already have a value, we got defaulted
+				marshaled, _ := json.Marshal(defaultedV.Interface())
+				defaults[visit.path] = string(marshaled)
+			} else if visit.value.Type().Elem().Kind() == reflect.Struct {
+				if strings.HasPrefix(visit.path, ".ObjectMeta.ManagedFields[") {
+					break
+				}
+				// if we don't already have a value, and contain structs, add an empty item so we can recurse
+				item := reflect.New(visit.value.Type().Elem()).Elem()
+				visit.value.Set(reflect.Append(visit.value, item))
+				toVisit = append(toVisit, testPath{path: visit.path + "[0]", value: visit.value.Index(0)})
+			} else if !isPrimitive(visit.value.Type().Elem().Kind()) {
+				t.Logf("unhandled non-primitive slice type %s: %s", visit.path, visit.value.Type().Elem())
+			}
+
+		case visit.value.Kind() == reflect.Map:
+			if !visit.value.IsNil() {
+				// if we already have a value, we got defaulted
+				marshaled, _ := json.Marshal(defaultedV.Interface())
+				defaults[visit.path] = string(marshaled)
+			} else if visit.value.Type().Key().Kind() == reflect.String && visit.value.Type().Elem().Kind() == reflect.Struct {
+				if strings.HasPrefix(visit.path, ".ObjectMeta.ManagedFields[") {
+					break
+				}
+				// if we don't already have a value, and contain structs, add an empty item so we can recurse
+				item := reflect.New(visit.value.Type().Elem()).Elem()
+				visit.value.Set(reflect.MakeMap(visit.value.Type()))
+				visit.value.SetMapIndex(reflect.New(visit.value.Type().Key()).Elem(), item)
+				toVisit = append(toVisit, testPath{path: visit.path + "[*]", value: item})
+			} else if !isPrimitive(visit.value.Type().Elem().Kind()) {
+				t.Logf("unhandled non-primitive map type %s: %s", visit.path, visit.value.Type().Elem())
+			}
+
+		case visit.value.Kind() == reflect.Ptr:
+			if visit.value.IsNil() {
+				if visit.value.Type().Elem().Kind() == reflect.Struct {
+					visit.value.Set(reflect.New(visit.value.Type().Elem()))
+					toVisit = append(toVisit, testPath{path: visit.path, value: visit.value.Elem()})
+				} else if !isPrimitive(visit.value.Type().Elem().Kind()) {
+					t.Errorf("unhandled non-primitive nil ptr: %s: %s", visit.path, visit.value.Type())
+				}
+			} else {
+				if visit.path != "" {
+					marshaled, _ := json.Marshal(defaultedV.Interface())
+					defaults[visit.path] = string(marshaled)
+				}
+				toVisit = append(toVisit, testPath{path: visit.path, value: visit.value.Elem()})
+			}
+
+		case isPrimitive(visit.value.Kind()):
+			if !reflect.DeepEqual(defaultedV.Interface(), zeroV.Interface()) {
+				marshaled, _ := json.Marshal(defaultedV.Interface())
+				defaults[visit.path] = string(marshaled)
+			}
+
+		default:
+			t.Errorf("unhandled kind: %s: %s", visit.path, visit.value.Type())
+		}
+
+	}
+	return defaults
+}
+
+func isPrimitive(k reflect.Kind) bool {
+	switch k {
+	case reflect.String, reflect.Bool, reflect.Int32, reflect.Int64, reflect.Int:
+		return true
+	default:
+		return false
+	}
+}
+
 func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
 	codec := legacyscheme.Codecs.LegacyCodec(corev1.SchemeGroupVersion)
 	data, err := runtime.Encode(codec, obj)