From 7c887412c882afc130d78e4bb6efad056b0fe376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Mon, 6 Jan 2025 22:04:31 +0300 Subject: [PATCH] Add validation for revisionHistoryLimit in sts to prevent negative value (#129017) * Add validation for revisionHistoryLimit in sts to prevent negative value * Add unit tests to verify warning messages --- pkg/apis/apps/validation/validation.go | 13 +++---- pkg/registry/apps/statefulset/strategy.go | 7 ++++ .../apps/statefulset/strategy_test.go | 34 +++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go index 7f05624fb64..59d1782562d 100644 --- a/pkg/apis/apps/validation/validation.go +++ b/pkg/apis/apps/validation/validation.go @@ -182,15 +182,16 @@ func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *apps.StatefulSet, op // statefulset updates aren't super common and general updates are likely to be touching spec, so we'll do this // deep copy right away. This avoids mutating our inputs newStatefulSetClone := statefulSet.DeepCopy() - newStatefulSetClone.Spec.Replicas = oldStatefulSet.Spec.Replicas // +k8s:verify-mutation:reason=clone - newStatefulSetClone.Spec.Template = oldStatefulSet.Spec.Template // +k8s:verify-mutation:reason=clone - newStatefulSetClone.Spec.UpdateStrategy = oldStatefulSet.Spec.UpdateStrategy // +k8s:verify-mutation:reason=clone - newStatefulSetClone.Spec.MinReadySeconds = oldStatefulSet.Spec.MinReadySeconds // +k8s:verify-mutation:reason=clone - newStatefulSetClone.Spec.Ordinals = oldStatefulSet.Spec.Ordinals // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.Replicas = oldStatefulSet.Spec.Replicas // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.Template = oldStatefulSet.Spec.Template // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.UpdateStrategy = oldStatefulSet.Spec.UpdateStrategy // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.MinReadySeconds = oldStatefulSet.Spec.MinReadySeconds // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.Ordinals = oldStatefulSet.Spec.Ordinals // +k8s:verify-mutation:reason=clone + newStatefulSetClone.Spec.RevisionHistoryLimit = oldStatefulSet.Spec.RevisionHistoryLimit // +k8s:verify-mutation:reason=clone newStatefulSetClone.Spec.PersistentVolumeClaimRetentionPolicy = oldStatefulSet.Spec.PersistentVolumeClaimRetentionPolicy // +k8s:verify-mutation:reason=clone if !apiequality.Semantic.DeepEqual(newStatefulSetClone.Spec, oldStatefulSet.Spec) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden")) + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'revisionHistoryLimit', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden")) } return allErrs diff --git a/pkg/registry/apps/statefulset/strategy.go b/pkg/registry/apps/statefulset/strategy.go index e650001258d..57848a47945 100644 --- a/pkg/registry/apps/statefulset/strategy.go +++ b/pkg/registry/apps/statefulset/strategy.go @@ -150,6 +150,10 @@ func (statefulSetStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Obj for i, pvc := range newStatefulSet.Spec.VolumeClaimTemplates { warnings = append(warnings, pvcutil.GetWarningsForPersistentVolumeClaimSpec(field.NewPath("spec", "volumeClaimTemplates").Index(i), pvc.Spec)...) } + + if newStatefulSet.Spec.RevisionHistoryLimit != nil && *newStatefulSet.Spec.RevisionHistoryLimit < 0 { + warnings = append(warnings, "spec.revisionHistoryLimit: a negative value retains all historical revisions; a value >= 0 is recommended") + } return warnings } @@ -182,6 +186,9 @@ func (statefulSetStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtim for i, pvc := range newStatefulSet.Spec.VolumeClaimTemplates { warnings = append(warnings, pvcutil.GetWarningsForPersistentVolumeClaimSpec(field.NewPath("spec", "volumeClaimTemplates").Index(i).Child("Spec"), pvc.Spec)...) } + if newStatefulSet.Spec.RevisionHistoryLimit != nil && *newStatefulSet.Spec.RevisionHistoryLimit < 0 { + warnings = append(warnings, "spec.revisionHistoryLimit: a negative value retains all historical revisions; a value >= 0 is recommended") + } return warnings } diff --git a/pkg/registry/apps/statefulset/strategy_test.go b/pkg/registry/apps/statefulset/strategy_test.go index bdf64ec7b5a..16b9ccdd237 100644 --- a/pkg/registry/apps/statefulset/strategy_test.go +++ b/pkg/registry/apps/statefulset/strategy_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" @@ -28,6 +29,7 @@ import ( "k8s.io/kubernetes/pkg/apis/apps" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" + "k8s.io/utils/ptr" ) func TestStatefulSetStrategy(t *testing.T) { @@ -137,6 +139,38 @@ func TestStatefulSetStrategy(t *testing.T) { } }) + t.Run("StatefulSet revisionHistoryLimit field validations on creation and updation", func(t *testing.T) { + // Test creation + oldSts := &apps.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: apps.StatefulSetSpec{ + PodManagementPolicy: apps.OrderedReadyPodManagement, + Selector: &metav1.LabelSelector{MatchLabels: validSelector}, + Template: validPodTemplate.Template, + UpdateStrategy: apps.StatefulSetUpdateStrategy{Type: apps.RollingUpdateStatefulSetStrategyType}, + RevisionHistoryLimit: ptr.To(int32(-2)), + }, + } + + warnings := Strategy.WarningsOnCreate(ctx, oldSts) + if len(warnings) != 1 { + t.Errorf("expected one warning got %v", warnings) + } + if warnings[0] != "spec.revisionHistoryLimit: a negative value retains all historical revisions; a value >= 0 is recommended" { + t.Errorf("unexpected warning: %v", warnings) + } + oldSts.Spec.RevisionHistoryLimit = ptr.To(int32(2)) + newSts := oldSts.DeepCopy() + newSts.Spec.RevisionHistoryLimit = ptr.To(int32(-2)) + warnings = Strategy.WarningsOnUpdate(ctx, newSts, oldSts) + if len(warnings) != 1 { + t.Errorf("expected one warning got %v", warnings) + } + if warnings[0] != "spec.revisionHistoryLimit: a negative value retains all historical revisions; a value >= 0 is recommended" { + t.Errorf("unexpected warning: %v", warnings) + } + }) + validPs = &apps.StatefulSet{ ObjectMeta: metav1.ObjectMeta{Name: ps.Name, Namespace: ps.Namespace, ResourceVersion: "1", Generation: 1}, Spec: apps.StatefulSetSpec{