From 617309cbce6fb543344493940253523c19eb3b83 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 11 Jun 2019 14:10:51 -0400 Subject: [PATCH] Add tests for podspec and podtemplatespec default changes --- pkg/apis/core/v1/defaults_test.go | 278 +++++++++++++++++++++++++++++- 1 file changed, 277 insertions(+), 1 deletion(-) diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index b3d9ad4316f..9a3f324fbb3 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/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)