diff --git a/pkg/apis/batch/validation/BUILD b/pkg/apis/batch/validation/BUILD index b71dec02963..d371966a354 100644 --- a/pkg/apis/batch/validation/BUILD +++ b/pkg/apis/batch/validation/BUILD @@ -14,11 +14,13 @@ go_library( "//pkg/apis/batch:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/validation:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/github.com/robfig/cron:go_default_library", ], ) @@ -30,8 +32,10 @@ go_test( deps = [ "//pkg/apis/batch:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index a31bf6416e4..1e04e9779e5 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -24,9 +24,11 @@ import ( "k8s.io/apimachinery/pkg/labels" apimachineryvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/batch" api "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" ) // TODO: generalize for other controller objects that will follow the same pattern, such as ReplicaSet and DaemonSet, and @@ -117,6 +119,14 @@ func validateJobSpec(spec *batch.JobSpec, fldPath *field.Path) field.ErrorList { if spec.BackoffLimit != nil { allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.BackoffLimit), fldPath.Child("backoffLimit"))...) } + if utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { + // normal validation for TTLSecondsAfterFinished + if spec.TTLSecondsAfterFinished != nil { + allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.TTLSecondsAfterFinished), fldPath.Child("ttlSecondsAfterFinished"))...) + } + } else if spec.TTLSecondsAfterFinished != nil { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("ttlSecondsAfterFinished"), "disabled by feature-gate")) + } allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(&spec.Template, fldPath.Child("template"))...) if spec.Template.Spec.RestartPolicy != api.RestartPolicyOnFailure && diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 13135e2ea53..7ff9e3f73ea 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/pkg/apis/batch/validation/validation_test.go @@ -17,13 +17,16 @@ limitations under the License. package validation import ( + "fmt" "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/batch" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" ) func getValidManualSelector() *metav1.LabelSelector { @@ -64,7 +67,21 @@ func getValidPodTemplateSpecForGenerated(selector *metav1.LabelSelector) api.Pod } } +func featureToggle(feature utilfeature.Feature) []string { + enabled := fmt.Sprintf("%s=%t", feature, true) + disabled := fmt.Sprintf("%s=%t", feature, false) + return []string{enabled, disabled} +} + func TestValidateJob(t *testing.T) { + ttlEnabled := utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) + defer func() { + err := utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%s=%t", features.TTLAfterFinished, ttlEnabled)) + if err != nil { + t.Fatalf("Failed to set feature gate for %s: %v", features.TTLAfterFinished, err) + } + }() + validManualSelector := getValidManualSelector() validPodTemplateSpecForManual := getValidPodTemplateSpecForManual(validManualSelector) validGeneratedSelector := getValidGeneratedSelector() @@ -214,15 +231,39 @@ func TestValidateJob(t *testing.T) { }, } - for k, v := range errorCases { - errs := ValidateJob(&v) - if len(errs) == 0 { - t.Errorf("expected failure for %s", k) + for _, setFeature := range featureToggle(features.TTLAfterFinished) { + // Set error cases based on if TTLAfterFinished feature is enabled or not + if err := utilfeature.DefaultFeatureGate.Set(setFeature); err != nil { + t.Fatalf("Failed to set feature gate for %s: %v", features.TTLAfterFinished, err) + } + ttlCase := "spec.ttlSecondsAfterFinished:must be greater than or equal to 0" + if utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { + errorCases[ttlCase] = batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myjob", + Namespace: metav1.NamespaceDefault, + UID: types.UID("1a2b3c"), + }, + Spec: batch.JobSpec{ + TTLSecondsAfterFinished: &negative, + Selector: validGeneratedSelector, + Template: validPodTemplateSpecForGenerated, + }, + } } else { - s := strings.Split(k, ":") - err := errs[0] - if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { - t.Errorf("unexpected error: %v, expected: %s", err, k) + delete(errorCases, ttlCase) + } + + for k, v := range errorCases { + errs := ValidateJob(&v) + if len(errs) == 0 { + t.Errorf("expected failure for %s", k) + } else { + s := strings.Split(k, ":") + err := errs[0] + if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) { + t.Errorf("unexpected error: %v, expected: %s", err, k) + } } } } @@ -584,6 +625,25 @@ func TestValidateCronJob(t *testing.T) { }, }, } + if utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { + errorCases["spec.jobTemplate.spec.ttlSecondsAfterFinished:must be greater than or equal to 0"] = batch.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mycronjob", + Namespace: metav1.NamespaceDefault, + UID: types.UID("1a2b3c"), + }, + Spec: batch.CronJobSpec{ + Schedule: "* * * * ?", + ConcurrencyPolicy: batch.AllowConcurrent, + JobTemplate: batch.JobTemplateSpec{ + Spec: batch.JobSpec{ + TTLSecondsAfterFinished: &negative, + Template: validPodTemplateSpec, + }, + }, + }, + } + } for k, v := range errorCases { errs := ValidateCronJob(&v) diff --git a/pkg/registry/batch/job/BUILD b/pkg/registry/batch/job/BUILD index 5f2ccc0ab99..30474db1f3c 100644 --- a/pkg/registry/batch/job/BUILD +++ b/pkg/registry/batch/job/BUILD @@ -18,6 +18,7 @@ go_library( "//pkg/api/pod:go_default_library", "//pkg/apis/batch:go_default_library", "//pkg/apis/batch/validation:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", @@ -27,6 +28,7 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) @@ -39,10 +41,12 @@ go_test( "//pkg/api/testing:go_default_library", "//pkg/apis/batch:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index c0eb2be5f53..4e46ddc4b37 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -30,10 +30,12 @@ import ( "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/api/pod" "k8s.io/kubernetes/pkg/apis/batch" "k8s.io/kubernetes/pkg/apis/batch/validation" + "k8s.io/kubernetes/pkg/features" ) // jobStrategy implements verification logic for Replication Controllers. @@ -61,6 +63,10 @@ func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { job := obj.(*batch.Job) job.Status = batch.JobStatus{} + if !utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { + job.Spec.TTLSecondsAfterFinished = nil + } + pod.DropDisabledAlphaFields(&job.Spec.Template.Spec) } @@ -70,6 +76,11 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object oldJob := old.(*batch.Job) newJob.Status = oldJob.Status + if !utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { + newJob.Spec.TTLSecondsAfterFinished = nil + oldJob.Spec.TTLSecondsAfterFinished = nil + } + pod.DropDisabledAlphaFields(&newJob.Spec.Template.Spec) pod.DropDisabledAlphaFields(&oldJob.Spec.Template.Spec) } diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index f8cfc609196..e3b51678c15 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -24,19 +24,24 @@ import ( "k8s.io/apimachinery/pkg/types" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/testapi" apitesting "k8s.io/kubernetes/pkg/api/testing" "k8s.io/kubernetes/pkg/apis/batch" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" ) func newBool(a bool) *bool { - r := new(bool) - *r = a - return r + return &a +} + +func newInt32(i int32) *int32 { + return &i } func TestJobStrategy(t *testing.T) { + ttlEnabled := utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) ctx := genericapirequest.NewDefaultContext() if !Strategy.NamespaceScoped() { t.Errorf("Job must be namespace scoped") @@ -64,9 +69,10 @@ func TestJobStrategy(t *testing.T) { Namespace: metav1.NamespaceDefault, }, Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, - ManualSelector: newBool(true), + Selector: validSelector, + Template: validPodTemplateSpec, + TTLSecondsAfterFinished: newInt32(0), // Set TTL + ManualSelector: newBool(true), }, Status: batch.JobStatus{ Active: 11, @@ -81,11 +87,21 @@ func TestJobStrategy(t *testing.T) { if len(errs) != 0 { t.Errorf("Unexpected error validating %v", errs) } + if ttlEnabled && job.Spec.TTLSecondsAfterFinished == nil { + // When the TTL feature is enabled, the TTL field can be set + t.Errorf("Job should allow setting .spec.ttlSecondsAfterFinished when %v feature is enabled", features.TTLAfterFinished) + } + if !ttlEnabled && job.Spec.TTLSecondsAfterFinished != nil { + // When the TTL feature is disabled, the TTL field cannot be set + t.Errorf("Job should not allow setting .spec.ttlSecondsAfterFinished when %v feature is disabled", features.TTLAfterFinished) + } + parallelism := int32(10) updatedJob := &batch.Job{ ObjectMeta: metav1.ObjectMeta{Name: "bar", ResourceVersion: "4"}, Spec: batch.JobSpec{ - Parallelism: ¶llelism, + Parallelism: ¶llelism, + TTLSecondsAfterFinished: newInt32(1), // Update TTL }, Status: batch.JobStatus{ Active: 11, @@ -101,6 +117,9 @@ func TestJobStrategy(t *testing.T) { if len(errs) == 0 { t.Errorf("Expected a validation error") } + if ttlEnabled != (job.Spec.TTLSecondsAfterFinished != nil || updatedJob.Spec.TTLSecondsAfterFinished != nil) { + t.Errorf("Job should only allow updating .spec.ttlSecondsAfterFinished when %v feature is enabled", features.TTLAfterFinished) + } // Make sure we correctly implement the interface. // Otherwise a typo could silently change the default.