Validate Job .spec.ttlSecondsAfterFinished; clear it when feature disabled

1. If TTLAfterFinished feature is enabled, the value should be non-negative.
2. If TTLAfterFinished feature is disabled, the field value should not
be kept.

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",
     ],
 )
 

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 &&

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 {
+	for _, setFeature := range featureToggle(features.TTLAfterFinished) {
-		errs := ValidateJob(&v)
+		// Set error cases based on if TTLAfterFinished feature is enabled or not
-		if len(errs) == 0 {
+		if err := utilfeature.DefaultFeatureGate.Set(setFeature); err != nil {
-			t.Errorf("expected failure for %s", k)
+			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, ":")
+			delete(errorCases, ttlCase)
-			err := errs[0]
+		}
-			if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) {
+
-				t.Errorf("unexpected error: %v, expected: %s", err, k)
+		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)

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",
     ],
 )
 

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)
 }

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)
+	return &a
-	*r = a
+}
-	return r
+
+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,
+			Selector:                validSelector,
-			Template:       validPodTemplateSpec,
+			Template:                validPodTemplateSpec,
-			ManualSelector: newBool(true),
+			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: &parallelism,
+			Parallelism:             &parallelism,
+			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.