From 642e6168d191ec6daa6bae6c19f0a629d3eb1751 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 30 Apr 2021 22:55:37 -0400 Subject: [PATCH] Add metadata.generation support to all pod-spec-containing types --- pkg/registry/batch/cronjob/strategy.go | 9 ++ pkg/registry/batch/cronjob/strategy_test.go | 20 ++++- pkg/registry/batch/job/strategy.go | 9 ++ pkg/registry/batch/job/strategy_test.go | 15 ++++ pkg/registry/core/podtemplate/strategy.go | 10 ++- .../core/podtemplate/strategy_test.go | 90 +++++++++++++++++++ 6 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 pkg/registry/core/podtemplate/strategy_test.go diff --git a/pkg/registry/batch/cronjob/strategy.go b/pkg/registry/batch/cronjob/strategy.go index c434f703b87..cda5f13d678 100644 --- a/pkg/registry/batch/cronjob/strategy.go +++ b/pkg/registry/batch/cronjob/strategy.go @@ -20,6 +20,7 @@ import ( "context" batchv1beta1 "k8s.io/api/batch/v1beta1" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" @@ -83,6 +84,8 @@ func (cronJobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) cronJob := obj.(*batch.CronJob) cronJob.Status = batch.CronJobStatus{} + cronJob.Generation = 1 + pod.DropDisabledTemplateFields(&cronJob.Spec.JobTemplate.Spec.Template, nil) } @@ -93,6 +96,12 @@ func (cronJobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Ob newCronJob.Status = oldCronJob.Status pod.DropDisabledTemplateFields(&newCronJob.Spec.JobTemplate.Spec.Template, &oldCronJob.Spec.JobTemplate.Spec.Template) + + // Any changes to the spec increment the generation number. + // See metav1.ObjectMeta description for more information on Generation. + if !apiequality.Semantic.DeepEqual(newCronJob.Spec, oldCronJob.Spec) { + newCronJob.Generation = oldCronJob.Generation + 1 + } } // Validate validates a new scheduled job. diff --git a/pkg/registry/batch/cronjob/strategy_test.go b/pkg/registry/batch/cronjob/strategy_test.go index 5ba88b28e56..f33c5a2e429 100644 --- a/pkg/registry/batch/cronjob/strategy_test.go +++ b/pkg/registry/batch/cronjob/strategy_test.go @@ -44,8 +44,9 @@ func TestCronJobStrategy(t *testing.T) { } cronJob := &batch.CronJob{ ObjectMeta: metav1.ObjectMeta{ - Name: "mycronjob", - Namespace: metav1.NamespaceDefault, + Name: "mycronjob", + Namespace: metav1.NamespaceDefault, + Generation: 999, }, Spec: batch.CronJobSpec{ Schedule: "* * * * ?", @@ -62,11 +63,23 @@ func TestCronJobStrategy(t *testing.T) { if len(cronJob.Status.Active) != 0 { t.Errorf("CronJob does not allow setting status on create") } + if cronJob.Generation != 1 { + t.Errorf("expected Generation=1, got %d", cronJob.Generation) + } errs := Strategy.Validate(ctx, cronJob) if len(errs) != 0 { t.Errorf("Unexpected error validating %v", errs) } now := metav1.Now() + + // ensure we do not change generation for non-spec updates + updatedLabelCronJob := cronJob.DeepCopy() + updatedLabelCronJob.Labels = map[string]string{"a": "true"} + Strategy.PrepareForUpdate(ctx, updatedLabelCronJob, cronJob) + if updatedLabelCronJob.Generation != 1 { + t.Errorf("expected Generation=1, got %d", updatedLabelCronJob.Generation) + } + updatedCronJob := &batch.CronJob{ ObjectMeta: metav1.ObjectMeta{Name: "bar", ResourceVersion: "4"}, Spec: batch.CronJobSpec{ @@ -82,6 +95,9 @@ func TestCronJobStrategy(t *testing.T) { if updatedCronJob.Status.Active != nil { t.Errorf("PrepareForUpdate should have preserved prior version status") } + if updatedCronJob.Generation != 2 { + t.Errorf("expected Generation=2, got %d", updatedCronJob.Generation) + } errs = Strategy.ValidateUpdate(ctx, updatedCronJob, cronJob) if len(errs) == 0 { t.Errorf("Expected a validation error") diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 02d4a81e49a..8942262b0bd 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -22,6 +22,7 @@ import ( "strconv" batchv1 "k8s.io/api/batch/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" @@ -89,6 +90,8 @@ func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { job := obj.(*batch.Job) job.Status = batch.JobStatus{} + job.Generation = 1 + if !utilfeature.DefaultFeatureGate.Enabled(features.TTLAfterFinished) { job.Spec.TTLSecondsAfterFinished = nil } @@ -129,6 +132,12 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object } pod.DropDisabledTemplateFields(&newJob.Spec.Template, &oldJob.Spec.Template) + + // Any changes to the spec increment the generation number. + // See metav1.ObjectMeta description for more information on Generation. + if !apiequality.Semantic.DeepEqual(newJob.Spec, oldJob.Spec) { + newJob.Generation = oldJob.Generation + 1 + } } // Validate validates a new job. diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index 00f0082015d..d0852b55a1a 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -110,6 +110,9 @@ func testJobStrategy(t *testing.T) { if job.Status.Active != 0 { t.Errorf("Job does not allow setting status on create") } + if job.Generation != 1 { + t.Errorf("expected Generation=1, got %d", job.Generation) + } errs := Strategy.Validate(ctx, job) if len(errs) != 0 { t.Errorf("Unexpected error validating %v", errs) @@ -125,6 +128,15 @@ func testJobStrategy(t *testing.T) { } parallelism := int32(10) + + // ensure we do not change generation for non-spec updates + updatedLabelJob := job.DeepCopy() + updatedLabelJob.Labels = map[string]string{"a": "true"} + Strategy.PrepareForUpdate(ctx, updatedLabelJob, job) + if updatedLabelJob.Generation != 1 { + t.Errorf("expected Generation=1, got %d", updatedLabelJob.Generation) + } + updatedJob := &batch.Job{ ObjectMeta: metav1.ObjectMeta{Name: "bar", ResourceVersion: "4"}, Spec: batch.JobSpec{ @@ -144,6 +156,9 @@ func testJobStrategy(t *testing.T) { if updatedJob.Status.Active != 10 { t.Errorf("PrepareForUpdate should have preserved prior version status") } + if updatedJob.Generation != 2 { + t.Errorf("expected Generation=2, got %d", updatedJob.Generation) + } if ttlEnabled != (updatedJob.Spec.TTLSecondsAfterFinished != nil) { t.Errorf("Job should only allow updating .spec.ttlSecondsAfterFinished when %v feature is enabled", features.TTLAfterFinished) } diff --git a/pkg/registry/core/podtemplate/strategy.go b/pkg/registry/core/podtemplate/strategy.go index 5f42b391e81..fd077a66303 100644 --- a/pkg/registry/core/podtemplate/strategy.go +++ b/pkg/registry/core/podtemplate/strategy.go @@ -19,6 +19,7 @@ package podtemplate import ( "context" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" @@ -46,7 +47,7 @@ func (podTemplateStrategy) NamespaceScoped() bool { // PrepareForCreate clears fields that are not allowed to be set by end users on creation. func (podTemplateStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { template := obj.(*api.PodTemplate) - + template.Generation = 1 pod.DropDisabledTemplateFields(&template.Template, nil) } @@ -77,6 +78,13 @@ func (podTemplateStrategy) PrepareForUpdate(ctx context.Context, obj, old runtim oldTemplate := old.(*api.PodTemplate) pod.DropDisabledTemplateFields(&newTemplate.Template, &oldTemplate.Template) + + // Any changes to the template increment the generation number. + // See metav1.ObjectMeta description for more information on Generation. + if !apiequality.Semantic.DeepEqual(newTemplate.Template, oldTemplate.Template) { + newTemplate.Generation = oldTemplate.Generation + 1 + } + } // ValidateUpdate is the default update validation for an end user. diff --git a/pkg/registry/core/podtemplate/strategy_test.go b/pkg/registry/core/podtemplate/strategy_test.go new file mode 100644 index 00000000000..7ad0f5634d2 --- /dev/null +++ b/pkg/registry/core/podtemplate/strategy_test.go @@ -0,0 +1,90 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package podtemplate + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + api "k8s.io/kubernetes/pkg/apis/core" +) + +func TestStrategy(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + if !Strategy.NamespaceScoped() { + t.Errorf("must be namespace scoped") + } + if Strategy.AllowCreateOnUpdate() { + t.Errorf("should not allow create on update") + } + + podTemplate := &api.PodTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytemplate", + Namespace: metav1.NamespaceDefault, + Generation: 999, + }, + Template: api.PodTemplateSpec{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyOnFailure, + DNSPolicy: api.DNSClusterFirst, + Containers: []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}}, + }, + }, + } + + Strategy.PrepareForCreate(ctx, podTemplate) + if podTemplate.Generation != 1 { + t.Errorf("expected Generation=1, got %d", podTemplate.Generation) + } + errs := Strategy.Validate(ctx, podTemplate) + if len(errs) != 0 { + t.Errorf("Unexpected error validating %v", errs) + } + + // ensure we do not change generation for non-spec updates + updatedLabel := podTemplate.DeepCopy() + updatedLabel.Labels = map[string]string{"a": "true"} + Strategy.PrepareForUpdate(ctx, updatedLabel, podTemplate) + if updatedLabel.Generation != 1 { + t.Errorf("expected Generation=1, got %d", updatedLabel.Generation) + } + + updatedTemplate := podTemplate.DeepCopy() + updatedTemplate.ResourceVersion = "10" + updatedTemplate.Generation = 999 + updatedTemplate.Template.Spec.RestartPolicy = api.RestartPolicyNever + + // ensure generation is updated for spec changes + Strategy.PrepareForUpdate(ctx, updatedTemplate, podTemplate) + if updatedTemplate.Generation != 2 { + t.Errorf("expected Generation=2, got %d", updatedTemplate.Generation) + } + errs = Strategy.ValidateUpdate(ctx, updatedTemplate, podTemplate) + if len(errs) != 0 { + t.Errorf("Unexpected error validating %v", errs) + } + + invalidUpdatedTemplate := updatedTemplate.DeepCopy() + invalidUpdatedTemplate.Name = "changed" + Strategy.PrepareForUpdate(ctx, invalidUpdatedTemplate, podTemplate) + errs = Strategy.ValidateUpdate(ctx, invalidUpdatedTemplate, podTemplate) + if len(errs) == 0 { + t.Errorf("expected error validating, got none") + } +}