Skip to content

Commit b337f04

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#127094 from sreeram-venkitesh/4818-allow-zero-for-prestop-hook
KEP-4818: Relaxed validation for allowing zero in PreStop hook sleep action
2 parents d34c181 + f1f9e7b commit b337f04

File tree

8 files changed

+197
-47
lines changed

8 files changed

+197
-47
lines changed

‎pkg/api/pod/util.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,7 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
385385
AllowInvalidTopologySpreadConstraintLabelSelector: false,
386386
AllowNamespacedSysctlsForHostNetAndHostIPC: false,
387387
AllowNonLocalProjectedTokenPath: false,
388+
AllowPodLifecycleSleepActionZeroValue: utilfeature.DefaultFeatureGate.Enabled(features.PodLifecycleSleepActionAllowZero),
388389
}
389390

390391
// If old spec uses relaxed validation or enabled the RelaxedEnvironmentVariableValidation feature gate,
@@ -415,6 +416,8 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
415416
}
416417
}
417418
}
419+
420+
opts.AllowPodLifecycleSleepActionZeroValue = opts.AllowPodLifecycleSleepActionZeroValue || podLifecycleSleepActionZeroValueInUse(podSpec)
418421
}
419422
if oldPodMeta != nil && !opts.AllowInvalidPodDeletionCost {
420423
// This is an update, so validate only if the existing object was valid.
@@ -795,6 +798,28 @@ func podLifecycleSleepActionInUse(podSpec *api.PodSpec) bool {
795798
return inUse
796799
}
797800

801+
func podLifecycleSleepActionZeroValueInUse(podSpec *api.PodSpec) bool {
802+
if podSpec == nil {
803+
return false
804+
}
805+
var inUse bool
806+
VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool {
807+
if c.Lifecycle == nil {
808+
return true
809+
}
810+
if c.Lifecycle.PreStop != nil && c.Lifecycle.PreStop.Sleep != nil && c.Lifecycle.PreStop.Sleep.Seconds == 0 {
811+
inUse = true
812+
return false
813+
}
814+
if c.Lifecycle.PostStart != nil && c.Lifecycle.PostStart.Sleep != nil && c.Lifecycle.PreStop.Sleep.Seconds == 0 {
815+
inUse = true
816+
return false
817+
}
818+
return true
819+
})
820+
return inUse
821+
}
822+
798823
// dropDisabledPodStatusFields removes disabled fields from the pod status
799824
func dropDisabledPodStatusFields(podStatus, oldPodStatus *api.PodStatus, podSpec, oldPodSpec *api.PodSpec) {
800825
// the new status is always be non-nil

‎pkg/apis/core/validation/validation.go‎

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3068,52 +3068,52 @@ func validatePodResourceClaim(podMeta *metav1.ObjectMeta, claim core.PodResource
30683068
return allErrs
30693069
}
30703070

3071-
func validateLivenessProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3071+
func validateLivenessProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
30723072
allErrs := field.ErrorList{}
30733073

30743074
if probe == nil {
30753075
return allErrs
30763076
}
3077-
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath)...)
3077+
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath, opts)...)
30783078
if probe.SuccessThreshold != 1 {
30793079
allErrs = append(allErrs, field.Invalid(fldPath.Child("successThreshold"), probe.SuccessThreshold, "must be 1"))
30803080
}
30813081
return allErrs
30823082
}
30833083

3084-
func validateReadinessProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3084+
func validateReadinessProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
30853085
allErrs := field.ErrorList{}
30863086

30873087
if probe == nil {
30883088
return allErrs
30893089
}
3090-
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath)...)
3090+
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath, opts)...)
30913091
if probe.TerminationGracePeriodSeconds != nil {
30923092
allErrs = append(allErrs, field.Invalid(fldPath.Child("terminationGracePeriodSeconds"), probe.TerminationGracePeriodSeconds, "must not be set for readinessProbes"))
30933093
}
30943094
return allErrs
30953095
}
30963096

3097-
func validateStartupProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3097+
func validateStartupProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
30983098
allErrs := field.ErrorList{}
30993099

31003100
if probe == nil {
31013101
return allErrs
31023102
}
3103-
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath)...)
3103+
allErrs = append(allErrs, validateProbe(probe, gracePeriod, fldPath, opts)...)
31043104
if probe.SuccessThreshold != 1 {
31053105
allErrs = append(allErrs, field.Invalid(fldPath.Child("successThreshold"), probe.SuccessThreshold, "must be 1"))
31063106
}
31073107
return allErrs
31083108
}
31093109

3110-
func validateProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3110+
func validateProbe(probe *core.Probe, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
31113111
allErrs := field.ErrorList{}
31123112

31133113
if probe == nil {
31143114
return allErrs
31153115
}
3116-
allErrs = append(allErrs, validateHandler(handlerFromProbe(&probe.ProbeHandler), gracePeriod, fldPath)...)
3116+
allErrs = append(allErrs, validateHandler(handlerFromProbe(&probe.ProbeHandler), gracePeriod, fldPath, opts)...)
31173117

31183118
allErrs = append(allErrs, ValidateNonnegativeField(int64(probe.InitialDelaySeconds), fldPath.Child("initialDelaySeconds"))...)
31193119
allErrs = append(allErrs, ValidateNonnegativeField(int64(probe.TimeoutSeconds), fldPath.Child("timeoutSeconds"))...)
@@ -3169,14 +3169,21 @@ func handlerFromLifecycle(lh *core.LifecycleHandler) commonHandler {
31693169
}
31703170
}
31713171

3172-
func validateSleepAction(sleep *core.SleepAction, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3172+
func validateSleepAction(sleep *core.SleepAction, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
31733173
allErrors := field.ErrorList{}
31743174
// We allow gracePeriod to be nil here because the pod in which this SleepAction
31753175
// is defined might have an invalid grace period defined, and we don't want to
31763176
// flag another error here when the real problem will already be flagged.
3177-
if gracePeriod != nil && sleep.Seconds <= 0 || sleep.Seconds > *gracePeriod {
3178-
invalidStr := fmt.Sprintf("must be greater than 0 and less than terminationGracePeriodSeconds (%d)", *gracePeriod)
3179-
allErrors = append(allErrors, field.Invalid(fldPath, sleep.Seconds, invalidStr))
3177+
if opts.AllowPodLifecycleSleepActionZeroValue {
3178+
if gracePeriod != nil && (sleep.Seconds < 0 || sleep.Seconds > *gracePeriod) {
3179+
invalidStr := fmt.Sprintf("must be non-negative and less than terminationGracePeriodSeconds (%d)", *gracePeriod)
3180+
allErrors = append(allErrors, field.Invalid(fldPath, sleep.Seconds, invalidStr))
3181+
}
3182+
} else {
3183+
if gracePeriod != nil && (sleep.Seconds <= 0 || sleep.Seconds > *gracePeriod) {
3184+
invalidStr := fmt.Sprintf("must be greater than 0 and less than terminationGracePeriodSeconds (%d). Enable AllowPodLifecycleSleepActionZeroValue feature gate for zero sleep.", *gracePeriod)
3185+
allErrors = append(allErrors, field.Invalid(fldPath, sleep.Seconds, invalidStr))
3186+
}
31803187
}
31813188
return allErrors
31823189
}
@@ -3289,7 +3296,7 @@ func validateTCPSocketAction(tcp *core.TCPSocketAction, fldPath *field.Path) fie
32893296
func validateGRPCAction(grpc *core.GRPCAction, fldPath *field.Path) field.ErrorList {
32903297
return ValidatePortNumOrName(intstr.FromInt32(grpc.Port), fldPath.Child("port"))
32913298
}
3292-
func validateHandler(handler commonHandler, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3299+
func validateHandler(handler commonHandler, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
32933300
numHandlers := 0
32943301
allErrors := field.ErrorList{}
32953302
if handler.Exec != nil {
@@ -3329,7 +3336,7 @@ func validateHandler(handler commonHandler, gracePeriod *int64, fldPath *field.P
33293336
allErrors = append(allErrors, field.Forbidden(fldPath.Child("sleep"), "may not specify more than 1 handler type"))
33303337
} else {
33313338
numHandlers++
3332-
allErrors = append(allErrors, validateSleepAction(handler.Sleep, gracePeriod, fldPath.Child("sleep"))...)
3339+
allErrors = append(allErrors, validateSleepAction(handler.Sleep, gracePeriod, fldPath.Child("sleep"), opts)...)
33333340
}
33343341
}
33353342
if numHandlers == 0 {
@@ -3338,13 +3345,13 @@ func validateHandler(handler commonHandler, gracePeriod *int64, fldPath *field.P
33383345
return allErrors
33393346
}
33403347

3341-
func validateLifecycle(lifecycle *core.Lifecycle, gracePeriod *int64, fldPath *field.Path) field.ErrorList {
3348+
func validateLifecycle(lifecycle *core.Lifecycle, gracePeriod *int64, fldPath *field.Path, opts PodValidationOptions) field.ErrorList {
33423349
allErrs := field.ErrorList{}
33433350
if lifecycle.PostStart != nil {
3344-
allErrs = append(allErrs, validateHandler(handlerFromLifecycle(lifecycle.PostStart), gracePeriod, fldPath.Child("postStart"))...)
3351+
allErrs = append(allErrs, validateHandler(handlerFromLifecycle(lifecycle.PostStart), gracePeriod, fldPath.Child("postStart"), opts)...)
33453352
}
33463353
if lifecycle.PreStop != nil {
3347-
allErrs = append(allErrs, validateHandler(handlerFromLifecycle(lifecycle.PreStop), gracePeriod, fldPath.Child("preStop"))...)
3354+
allErrs = append(allErrs, validateHandler(handlerFromLifecycle(lifecycle.PreStop), gracePeriod, fldPath.Child("preStop"), opts)...)
33483355
}
33493356
return allErrs
33503357
}
@@ -3523,11 +3530,11 @@ func validateInitContainers(containers []core.Container, regularContainers []cor
35233530
switch {
35243531
case restartAlways:
35253532
if ctr.Lifecycle != nil {
3526-
allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, gracePeriod, idxPath.Child("lifecycle"))...)
3533+
allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, gracePeriod, idxPath.Child("lifecycle"), opts)...)
35273534
}
3528-
allErrs = append(allErrs, validateLivenessProbe(ctr.LivenessProbe, gracePeriod, idxPath.Child("livenessProbe"))...)
3529-
allErrs = append(allErrs, validateReadinessProbe(ctr.ReadinessProbe, gracePeriod, idxPath.Child("readinessProbe"))...)
3530-
allErrs = append(allErrs, validateStartupProbe(ctr.StartupProbe, gracePeriod, idxPath.Child("startupProbe"))...)
3535+
allErrs = append(allErrs, validateLivenessProbe(ctr.LivenessProbe, gracePeriod, idxPath.Child("livenessProbe"), opts)...)
3536+
allErrs = append(allErrs, validateReadinessProbe(ctr.ReadinessProbe, gracePeriod, idxPath.Child("readinessProbe"), opts)...)
3537+
allErrs = append(allErrs, validateStartupProbe(ctr.StartupProbe, gracePeriod, idxPath.Child("startupProbe"), opts)...)
35313538

35323539
default:
35333540
// These fields are disallowed for init containers.
@@ -3655,11 +3662,11 @@ func validateContainers(containers []core.Container, volumes map[string]core.Vol
36553662
// Regular init container and ephemeral container validation will return
36563663
// field.Forbidden() for these paths.
36573664
if ctr.Lifecycle != nil {
3658-
allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, gracePeriod, path.Child("lifecycle"))...)
3665+
allErrs = append(allErrs, validateLifecycle(ctr.Lifecycle, gracePeriod, path.Child("lifecycle"), opts)...)
36593666
}
3660-
allErrs = append(allErrs, validateLivenessProbe(ctr.LivenessProbe, gracePeriod, path.Child("livenessProbe"))...)
3661-
allErrs = append(allErrs, validateReadinessProbe(ctr.ReadinessProbe, gracePeriod, path.Child("readinessProbe"))...)
3662-
allErrs = append(allErrs, validateStartupProbe(ctr.StartupProbe, gracePeriod, path.Child("startupProbe"))...)
3667+
allErrs = append(allErrs, validateLivenessProbe(ctr.LivenessProbe, gracePeriod, path.Child("livenessProbe"), opts)...)
3668+
allErrs = append(allErrs, validateReadinessProbe(ctr.ReadinessProbe, gracePeriod, path.Child("readinessProbe"), opts)...)
3669+
allErrs = append(allErrs, validateStartupProbe(ctr.StartupProbe, gracePeriod, path.Child("startupProbe"), opts)...)
36633670

36643671
// These fields are disallowed for regular containers
36653672
if ctr.RestartPolicy != nil {
@@ -4049,6 +4056,8 @@ type PodValidationOptions struct {
40494056
AllowRelaxedEnvironmentVariableValidation bool
40504057
// Allow the use of a relaxed DNS search
40514058
AllowRelaxedDNSSearchValidation bool
4059+
// Allows zero value for Pod Lifecycle Sleep Action
4060+
AllowPodLifecycleSleepActionZeroValue bool
40524061
}
40534062

40544063
// validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set,

‎pkg/apis/core/validation/validation_test.go‎

Lines changed: 82 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7445,7 +7445,7 @@ func TestValidateProbe(t *testing.T) {
74457445
}
74467446

74477447
for _, p := range successCases {
7448-
if errs := validateProbe(p, defaultGracePeriod, field.NewPath("field")); len(errs) != 0 {
7448+
if errs := validateProbe(p, defaultGracePeriod, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 {
74497449
t.Errorf("expected success: %v", errs)
74507450
}
74517451
}
@@ -7457,7 +7457,7 @@ func TestValidateProbe(t *testing.T) {
74577457
errorCases = append(errorCases, probe)
74587458
}
74597459
for _, p := range errorCases {
7460-
if errs := validateProbe(p, defaultGracePeriod, field.NewPath("field")); len(errs) == 0 {
7460+
if errs := validateProbe(p, defaultGracePeriod, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 {
74617461
t.Errorf("expected failure for %v", p)
74627462
}
74637463
}
@@ -7563,7 +7563,7 @@ func Test_validateProbe(t *testing.T) {
75637563
}
75647564
for _, tt := range tests {
75657565
t.Run(tt.name, func(t *testing.T) {
7566-
got := validateProbe(tt.args.probe, defaultGracePeriod, tt.args.fldPath)
7566+
got := validateProbe(tt.args.probe, defaultGracePeriod, tt.args.fldPath, PodValidationOptions{})
75677567
if len(got) != len(tt.want) {
75687568
t.Errorf("validateProbe() = %v, want %v", got, tt.want)
75697569
return
@@ -7588,7 +7588,7 @@ func TestValidateHandler(t *testing.T) {
75887588
{HTTPGet: &core.HTTPGetAction{Path: "/", Port: intstr.FromString("port"), Host: "", Scheme: "HTTP", HTTPHeaders: []core.HTTPHeader{{Name: "X-Forwarded-For", Value: "1.2.3.4"}, {Name: "X-Forwarded-For", Value: "5.6.7.8"}}}},
75897589
}
75907590
for _, h := range successCases {
7591-
if errs := validateHandler(handlerFromProbe(&h), defaultGracePeriod, field.NewPath("field")); len(errs) != 0 {
7591+
if errs := validateHandler(handlerFromProbe(&h), defaultGracePeriod, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 {
75927592
t.Errorf("expected success: %v", errs)
75937593
}
75947594
}
@@ -7603,7 +7603,7 @@ func TestValidateHandler(t *testing.T) {
76037603
{HTTPGet: &core.HTTPGetAction{Path: "/", Port: intstr.FromString("port"), Host: "", Scheme: "HTTP", HTTPHeaders: []core.HTTPHeader{{Name: "X_Forwarded_For", Value: "foo.example.com"}}}},
76047604
}
76057605
for _, h := range errorCases {
7606-
if errs := validateHandler(handlerFromProbe(&h), defaultGracePeriod, field.NewPath("field")); len(errs) == 0 {
7606+
if errs := validateHandler(handlerFromProbe(&h), defaultGracePeriod, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 {
76077607
t.Errorf("expected failure for %#v", h)
76087608
}
76097609
}
@@ -24162,43 +24162,109 @@ func TestValidateLoadBalancerStatus(t *testing.T) {
2416224162
func TestValidateSleepAction(t *testing.T) {
2416324163
fldPath := field.NewPath("root")
2416424164
getInvalidStr := func(gracePeriod int64) string {
24165-
return fmt.Sprintf("must be greater than 0 and less than terminationGracePeriodSeconds (%d)", gracePeriod)
24165+
return fmt.Sprintf("must be greater than 0 and less than terminationGracePeriodSeconds (%d). Enable AllowPodLifecycleSleepActionZeroValue feature gate for zero sleep.", gracePeriod)
24166+
}
24167+
24168+
getInvalidStrWithZeroValueEnabled := func(gracePeriod int64) string {
24169+
return fmt.Sprintf("must be non-negative and less than terminationGracePeriodSeconds (%d)", gracePeriod)
2416624170
}
2416724171

2416824172
testCases := []struct {
24169-
name string
24170-
action *core.SleepAction
24171-
gracePeriod int64
24172-
expectErr field.ErrorList
24173+
name string
24174+
action *core.SleepAction
24175+
gracePeriod int64
24176+
zeroValueEnabled bool
24177+
expectErr field.ErrorList
2417324178
}{
2417424179
{
2417524180
name: "valid setting",
2417624181
action: &core.SleepAction{
2417724182
Seconds: 5,
2417824183
},
24179-
gracePeriod: 30,
24184+
gracePeriod: 30,
24185+
zeroValueEnabled: false,
2418024186
},
2418124187
{
2418224188
name: "negative seconds",
2418324189
action: &core.SleepAction{
2418424190
Seconds: -1,
2418524191
},
24186-
gracePeriod: 30,
24187-
expectErr: field.ErrorList{field.Invalid(fldPath, -1, getInvalidStr(30))},
24192+
gracePeriod: 30,
24193+
zeroValueEnabled: false,
24194+
expectErr: field.ErrorList{field.Invalid(fldPath, -1, getInvalidStr(30))},
2418824195
},
2418924196
{
2419024197
name: "longer than gracePeriod",
2419124198
action: &core.SleepAction{
2419224199
Seconds: 5,
2419324200
},
24194-
gracePeriod: 3,
24195-
expectErr: field.ErrorList{field.Invalid(fldPath, 5, getInvalidStr(3))},
24201+
gracePeriod: 3,
24202+
zeroValueEnabled: false,
24203+
expectErr: field.ErrorList{field.Invalid(fldPath, 5, getInvalidStr(3))},
24204+
},
24205+
{
24206+
name: "sleep duration of zero with zero value feature gate disabled",
24207+
action: &core.SleepAction{
24208+
Seconds: 0,
24209+
},
24210+
gracePeriod: 30,
24211+
zeroValueEnabled: false,
24212+
expectErr: field.ErrorList{field.Invalid(fldPath, 0, getInvalidStr(30))},
24213+
},
24214+
{
24215+
name: "sleep duration of zero with zero value feature gate enabled",
24216+
action: &core.SleepAction{
24217+
Seconds: 0,
24218+
},
24219+
gracePeriod: 30,
24220+
zeroValueEnabled: true,
24221+
},
24222+
{
24223+
name: "invalid sleep duration (negative value) with zero value disabled",
24224+
action: &core.SleepAction{
24225+
Seconds: -1,
24226+
},
24227+
gracePeriod: 30,
24228+
zeroValueEnabled: false,
24229+
expectErr: field.ErrorList{field.Invalid(fldPath, -1, getInvalidStr(30))},
24230+
},
24231+
{
24232+
name: "invalid sleep duration (negative value) with zero value enabled",
24233+
action: &core.SleepAction{
24234+
Seconds: -1,
24235+
},
24236+
gracePeriod: 30,
24237+
zeroValueEnabled: true,
24238+
expectErr: field.ErrorList{field.Invalid(fldPath, -1, getInvalidStrWithZeroValueEnabled(30))},
24239+
},
24240+
{
24241+
name: "zero grace period duration with zero value enabled",
24242+
action: &core.SleepAction{
24243+
Seconds: 0,
24244+
},
24245+
gracePeriod: 0,
24246+
zeroValueEnabled: true,
24247+
},
24248+
{
24249+
name: "nil grace period with zero value disabled",
24250+
action: &core.SleepAction{
24251+
Seconds: 5,
24252+
},
24253+
zeroValueEnabled: false,
24254+
expectErr: field.ErrorList{field.Invalid(fldPath, 5, getInvalidStr(0))},
24255+
},
24256+
{
24257+
name: "nil grace period with zero value enabled",
24258+
action: &core.SleepAction{
24259+
Seconds: 0,
24260+
},
24261+
zeroValueEnabled: true,
2419624262
},
2419724263
}
2419824264

2419924265
for _, tc := range testCases {
2420024266
t.Run(tc.name, func(t *testing.T) {
24201-
errs := validateSleepAction(tc.action, &tc.gracePeriod, fldPath)
24267+
errs := validateSleepAction(tc.action, &tc.gracePeriod, fldPath, PodValidationOptions{AllowPodLifecycleSleepActionZeroValue: tc.zeroValueEnabled})
2420224268

2420324269
if len(tc.expectErr) > 0 && len(errs) == 0 {
2420424270
t.Errorf("Unexpected success")

‎pkg/features/kube_features.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,12 @@ const (
477477
// Enables SleepAction in container lifecycle hooks
478478
PodLifecycleSleepAction featuregate.Feature = "PodLifecycleSleepAction"
479479

480+
// owner: @sreeram-venkitesh
481+
// kep: http://kep.k8s.io/4818
482+
//
483+
// Allows zero value for sleep duration in SleepAction in container lifecycle hooks
484+
PodLifecycleSleepActionAllowZero featuregate.Feature = "PodLifecycleSleepActionAllowZero"
485+
480486
// owner: @Huang-Wei
481487
// kep: https://kep.k8s.io/3521
482488
//

0 commit comments

Comments
 (0)