validation: Handle presence of MaxSurge on DaemonSet

When the maxsurge daemonset gate is disabled, the registry and validation
must properly handle stripping the field. In the special case where that
would leave the MaxUnavailable field set to 0, we must set it to 1 which
is the default value.

pkg/apis/apps/validation/BUILD

@@ -14,6 +14,7 @@ go_library(
         "//pkg/apis/apps: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/api/equality:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
@@ -22,6 +23,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/intstr: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",
     ],
 )
 

pkg/apis/apps/validation/validation.go

@@ -28,9 +28,11 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"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/apps"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // ValidateStatefulSetName can be used to check whether the given StatefulSet name is valid.
@@ -343,7 +345,27 @@ func ValidateDaemonSetSpec(spec *apps.DaemonSetSpec, fldPath *field.Path, opts a
 
 // ValidateRollingUpdateDaemonSet validates a given RollingUpdateDaemonSet.
 func ValidateRollingUpdateDaemonSet(rollingUpdate *apps.RollingUpdateDaemonSet, fldPath *field.Path) field.ErrorList {
-	allErrs := field.ErrorList{}
+	var allErrs field.ErrorList
+	if utilfeature.DefaultFeatureGate.Enabled(features.DaemonSetUpdateSurge) {
+		// Validate both fields are positive ints or have a percentage value
+		allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...)
+		allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxSurge, fldPath.Child("maxSurge"))...)
+
+		// Validate that MaxUnavailable and MaxSurge are not more than 100%.
+		allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...)
+		allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxSurge, fldPath.Child("maxSurge"))...)
+
+		// Validate exactly one of MaxSurge or MaxUnavailable is non-zero
+		hasUnavailable := getIntOrPercentValue(rollingUpdate.MaxUnavailable) != 0
+		hasSurge := getIntOrPercentValue(rollingUpdate.MaxSurge) != 0
+		switch {
+		case hasUnavailable && hasSurge:
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("maxSurge"), rollingUpdate.MaxSurge, "may not be set when maxUnavailable is non-zero"))
+		case !hasUnavailable && !hasSurge:
+			allErrs = append(allErrs, field.Required(fldPath.Child("maxUnavailable"), "cannot be 0 when maxSurge is 0"))
+		}
+
+	} else {
 		allErrs = append(allErrs, ValidatePositiveIntOrPercent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...)
 		if getIntOrPercentValue(rollingUpdate.MaxUnavailable) == 0 {
 			// MaxUnavailable cannot be 0.
@@ -351,6 +373,7 @@ func ValidateRollingUpdateDaemonSet(rollingUpdate *apps.RollingUpdateDaemonSet,
 		}
 		// Validate that MaxUnavailable is not more than 100%.
 		allErrs = append(allErrs, IsNotMoreThan100Percent(rollingUpdate.MaxUnavailable, fldPath.Child("maxUnavailable"))...)
+	}
 	return allErrs
 }
 

pkg/apis/apps/validation/validation_test.go

@@ -2971,3 +2971,182 @@ func TestValidateReplicaSet(t *testing.T) {
 		}
 	}
 }
+
+func TestDaemonSetUpdateMaxSurge(t *testing.T) {
+	testCases := map[string]struct {
+		ds          *apps.RollingUpdateDaemonSet
+		enableSurge bool
+		expectError bool
+	}{
+		"invalid: unset": {
+			ds:          &apps.RollingUpdateDaemonSet{},
+			expectError: true,
+		},
+		"invalid: zero percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("0%"),
+			},
+			expectError: true,
+		},
+		"invalid: zero": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(0),
+			},
+			expectError: true,
+		},
+		"valid: one": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(1),
+			},
+		},
+		"valid: one percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("1%"),
+			},
+		},
+		"valid: 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("100%"),
+			},
+		},
+		"invalid: greater than 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("101%"),
+			},
+			expectError: true,
+		},
+
+		"valid: surge and unavailable set": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("1%"),
+				MaxSurge:       intstr.FromString("1%"),
+			},
+		},
+
+		"invalid: surge enabled, unavailable zero percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("0%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"invalid: surge enabled, unavailable zero": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(0),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"valid: surge enabled, unavailable one": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(1),
+			},
+			enableSurge: true,
+		},
+		"valid: surge enabled, unavailable one percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("1%"),
+			},
+			enableSurge: true,
+		},
+		"valid: surge enabled, unavailable 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("100%"),
+			},
+			enableSurge: true,
+		},
+		"invalid: surge enabled, unavailable greater than 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("101%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+
+		"invalid: surge enabled, surge zero percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromString("0%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"invalid: surge enabled, surge zero": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromInt(0),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"valid: surge enabled, surge one": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromInt(1),
+			},
+			enableSurge: true,
+		},
+		"valid: surge enabled, surge one percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromString("1%"),
+			},
+			enableSurge: true,
+		},
+		"valid: surge enabled, surge 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromString("100%"),
+			},
+			enableSurge: true,
+		},
+		"invalid: surge enabled, surge greater than 100%": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxSurge: intstr.FromString("101%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+
+		"invalid: surge enabled, surge and unavailable set": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("1%"),
+				MaxSurge:       intstr.FromString("1%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+
+		"invalid: surge enabled, surge and unavailable zero percent": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromString("0%"),
+				MaxSurge:       intstr.FromString("0%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"invalid: surge enabled, surge and unavailable zero": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(0),
+				MaxSurge:       intstr.FromInt(0),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+		"invalid: surge enabled, surge and unavailable mixed zero": {
+			ds: &apps.RollingUpdateDaemonSet{
+				MaxUnavailable: intstr.FromInt(0),
+				MaxSurge:       intstr.FromString("0%"),
+			},
+			enableSurge: true,
+			expectError: true,
+		},
+	}
+	for tcName, tc := range testCases {
+		t.Run(tcName, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DaemonSetUpdateSurge, tc.enableSurge)()
+			errs := ValidateRollingUpdateDaemonSet(tc.ds, field.NewPath("spec", "updateStrategy", "rollingUpdate"))
+			if tc.expectError && len(errs) == 0 {
+				t.Errorf("Unexpected success")
+			}
+			if !tc.expectError && len(errs) != 0 {
+				t.Errorf("Unexpected error(s): %v", errs)
+			}
+		})
+	}
+}

pkg/registry/apps/daemonset/BUILD

@@ -18,16 +18,19 @@ go_library(
         "//pkg/api/pod:go_default_library",
         "//pkg/apis/apps:go_default_library",
         "//pkg/apis/apps/validation:go_default_library",
+        "//pkg/features:go_default_library",
         "//staging/src/k8s.io/api/apps/v1beta2:go_default_library",
         "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field: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/storage/names:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
     ],
 )
 
@@ -38,10 +41,15 @@ go_test(
     deps = [
         "//pkg/apis/apps: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/util/diff:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field: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",
+        "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
     ],
 )
 

pkg/registry/apps/daemonset/strategy.go

@@ -25,14 +25,17 @@ import (
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
 	"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/apps"
 	"k8s.io/kubernetes/pkg/apis/apps/validation"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // daemonSetStrategy implements verification logic for daemon sets.
@@ -75,6 +78,7 @@ func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Objec
 		daemonSet.Spec.TemplateGeneration = 1
 	}
 
+	dropDaemonSetDisabledFields(daemonSet, nil)
 	pod.DropDisabledTemplateFields(&daemonSet.Spec.Template, nil)
 }
 
@@ -83,6 +87,7 @@ func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
 	newDaemonSet := obj.(*apps.DaemonSet)
 	oldDaemonSet := old.(*apps.DaemonSet)
 
+	dropDaemonSetDisabledFields(newDaemonSet, oldDaemonSet)
 	pod.DropDisabledTemplateFields(&newDaemonSet.Spec.Template, &oldDaemonSet.Spec.Template)
 
 	// update is not allowed to set status
@@ -112,6 +117,35 @@ func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
 	}
 }
 
+// dropDaemonSetDisabledFields drops fields that are not used if their associated feature gates
+// are not enabled.  The typical pattern is:
+//     if !utilfeature.DefaultFeatureGate.Enabled(features.MyFeature) && !myFeatureInUse(oldSvc) {
+//         newSvc.Spec.MyFeature = nil
+//     }
+func dropDaemonSetDisabledFields(newDS *apps.DaemonSet, oldDS *apps.DaemonSet) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.DaemonSetUpdateSurge) {
+		if r := newDS.Spec.UpdateStrategy.RollingUpdate; r != nil {
+			if daemonSetSurgeFieldsInUse(oldDS) {
+				// we need to ensure that MaxUnavailable is non-zero to preserve previous behavior
+				if r.MaxUnavailable.IntVal == 0 && r.MaxUnavailable.StrVal == "0%" {
+					r.MaxUnavailable = intstr.FromInt(1)
+				}
+			} else {
+				// clear the MaxSurge field and let validation deal with MaxUnavailable
+				r.MaxSurge = intstr.IntOrString{}
+			}
+		}
+	}
+}
+
+// daemonSetSurgeFieldsInUse returns true if fields related to daemonset update surge are set
+func daemonSetSurgeFieldsInUse(ds *apps.DaemonSet) bool {
+	if ds == nil {
+		return false
+	}
+	return ds.Spec.UpdateStrategy.RollingUpdate != nil && (ds.Spec.UpdateStrategy.RollingUpdate.MaxSurge.IntVal != 0 || ds.Spec.UpdateStrategy.RollingUpdate.MaxSurge.StrVal != "")
+}
+
 // Validate validates a new daemon set.
 func (daemonSetStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	daemonSet := obj.(*apps.DaemonSet)

pkg/registry/apps/daemonset/strategy_test.go

@@ -21,11 +21,16 @@ import (
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/kubernetes/pkg/apis/apps"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 const (
@@ -189,3 +194,207 @@ func newDaemonSetWithSelectorLabels(selectorLabels map[string]string, templateGe
 		},
 	}
 }
+
+func makeDaemonSetWithSurge(unavailable intstr.IntOrString, surge intstr.IntOrString) *apps.DaemonSet {
+	return &apps.DaemonSet{
+		Spec: apps.DaemonSetSpec{
+			UpdateStrategy: apps.DaemonSetUpdateStrategy{
+				Type: apps.RollingUpdateDaemonSetStrategyType,
+				RollingUpdate: &apps.RollingUpdateDaemonSet{
+					MaxUnavailable: unavailable,
+					MaxSurge:       surge,
+				},
+			},
+		},
+	}
+}
+
+func TestDropDisabledField(t *testing.T) {
+	testCases := []struct {
+		name        string
+		enableSurge bool
+		ds          *apps.DaemonSet
+		old         *apps.DaemonSet
+		expect      *apps.DaemonSet
+	}{
+		{
+			name:        "not surge, no update",
+			enableSurge: false,
+			ds:          &apps.DaemonSet{},
+			old:         nil,
+			expect:      &apps.DaemonSet{},
+		},
+		{
+			name:        "not surge, field not used",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			old:         nil,
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, field not used in old and new",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, field used",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+		},
+		{
+			name:        "not surge, field used, percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+		},
+		{
+			name:        "not surge, field used and cleared",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, field used and cleared, percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+		},
+		{
+			name:        "surge, field not used",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			old:         nil,
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+		},
+		{
+			name:        "surge, field not used in old and new",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+		},
+		{
+			name:        "surge, field used",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			old:         nil,
+			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+		},
+		{
+			name:        "surge, field used, percent",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromString("1%")),
+		},
+		{
+			name:        "surge, field used in old and new",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+		},
+		{
+			name:        "surge, allows both fields (validation must catch)",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.FromInt(1)),
+		},
+		{
+			name:        "surge, allows change from unavailable to surge",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+		},
+		{
+			name:        "surge, allows change from surge to unvailable",
+			enableSurge: true,
+			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+		},
+		{
+			name:        "not surge, allows change from unavailable to surge",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, allows change from surge to unvailable",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromInt(1)),
+			old:         makeDaemonSetWithSurge(intstr.FromInt(2), intstr.IntOrString{}),
+			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, allows change from unavailable to surge, percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, allows change from surge to unvailable, percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.FromString("1%")),
+			old:         makeDaemonSetWithSurge(intstr.FromString("2%"), intstr.IntOrString{}),
+			expect:      makeDaemonSetWithSurge(intstr.IntOrString{}, intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, resets zero percent, one percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
+			old:         makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.FromString("1%")),
+		},
+		{
+			name:        "not surge, resets and clears when zero percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
+			old:         makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
+			expect:      makeDaemonSetWithSurge(intstr.FromInt(1), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, sets zero percent, one percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.FromString("1%")),
+			old:         nil,
+			expect:      makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
+		},
+		{
+			name:        "not surge, sets and clears zero percent",
+			enableSurge: false,
+			ds:          makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
+			old:         nil,
+			expect:      makeDaemonSetWithSurge(intstr.FromString("0%"), intstr.IntOrString{}),
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DaemonSetUpdateSurge, tc.enableSurge)()
+			old := tc.old.DeepCopy()
+
+			dropDaemonSetDisabledFields(tc.ds, tc.old)
+
+			// old obj should never be changed
+			if !reflect.DeepEqual(tc.old, old) {
+				t.Fatalf("old ds changed: %v", diff.ObjectReflectDiff(tc.old, old))
+			}
+
+			if !reflect.DeepEqual(tc.ds, tc.expect) {
+				t.Fatalf("unexpected ds spec: %v", diff.ObjectReflectDiff(tc.expect, tc.ds))
+			}
+		})
+	}
+
+}