endpointslice API strategy: drop disabled fields 'accepting' and 'terminating'

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

pkg/registry/discovery/endpointslice/BUILD

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -12,11 +12,13 @@ go_library(
         "//pkg/api/legacyscheme:go_default_library",
         "//pkg/apis/discovery:go_default_library",
         "//pkg/apis/discovery/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/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field: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",
     ],
 )
 
@@ -36,3 +38,17 @@ filegroup(
     tags = ["automanaged"],
     visibility = ["//visibility:public"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["strategy_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/apis/discovery:go_default_library",
+        "//pkg/features:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/equality: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",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
+    ],
+)

pkg/registry/discovery/endpointslice/strategy.go

@@ -24,9 +24,11 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/storage/names"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/apis/discovery"
 	"k8s.io/kubernetes/pkg/apis/discovery/validation"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // endpointSliceStrategy implements verification logic for Replication.
@@ -47,6 +49,8 @@ func (endpointSliceStrategy) NamespaceScoped() bool {
 func (endpointSliceStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 	endpointSlice := obj.(*discovery.EndpointSlice)
 	endpointSlice.Generation = 1
+
+	dropDisabledConditionsOnCreate(endpointSlice)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -67,6 +71,8 @@ func (endpointSliceStrategy) PrepareForUpdate(ctx context.Context, obj, old runt
 
 	newEPS.ObjectMeta = ogNewMeta
 	oldEPS.ObjectMeta = ogOldMeta
+
+	dropDisabledConditionsOnUpdate(oldEPS, newEPS)
 }
 
 // Validate validates a new EndpointSlice.
@@ -96,3 +102,42 @@ func (endpointSliceStrategy) ValidateUpdate(ctx context.Context, new, old runtim
 func (endpointSliceStrategy) AllowUnconditionalUpdate() bool {
 	return true
 }
+
+// dropDisabledConditionsOnCreate will drop the terminating condition if the
+// EndpointSliceTerminatingCondition is disabled. Otherwise the field is left untouched.
+func dropDisabledConditionsOnCreate(endpointSlice *discovery.EndpointSlice) {
+	if utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceTerminatingCondition) {
+		return
+	}
+
+	// Always drop the accepting / terminating conditions on create when feature gate is disabled.
+	for i := range endpointSlice.Endpoints {
+		endpointSlice.Endpoints[i].Conditions.Accepting = nil
+		endpointSlice.Endpoints[i].Conditions.Terminating = nil
+	}
+}
+
+// dropDisabledConditionsOnUpdate will drop the terminating condition field if the EndpointSliceTerminatingCondition
+// feature gate is disabled unless an existing EndpointSlice object has the field already set. This ensures
+// the field is not dropped on rollback.
+func dropDisabledConditionsOnUpdate(oldEPS, newEPS *discovery.EndpointSlice) {
+	if utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceTerminatingCondition) {
+		return
+	}
+
+	// Only drop the accepting/terminating condition if the existing EndpointSlice doesn't have it set.
+	dropConditions := true
+	for _, ep := range oldEPS.Endpoints {
+		if ep.Conditions.Accepting != nil || ep.Conditions.Terminating != nil {
+			dropConditions = false
+			break
+		}
+	}
+
+	if dropConditions {
+		for i := range newEPS.Endpoints {
+			newEPS.Endpoints[i].Conditions.Accepting = nil
+			newEPS.Endpoints[i].Conditions.Terminating = nil
+		}
+	}
+}

pkg/registry/discovery/endpointslice/strategy_test.go

@@ -0,0 +1,448 @@
+/*
+Copyright 2020 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 endpointslice
+
+import (
+	"testing"
+
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
+	"k8s.io/kubernetes/pkg/apis/discovery"
+	"k8s.io/kubernetes/pkg/features"
+	utilpointer "k8s.io/utils/pointer"
+)
+
+func Test_dropConditionsOnCreate(t *testing.T) {
+	testcases := []struct {
+		name                   string
+		terminatingGateEnabled bool
+		eps                    *discovery.EndpointSlice
+		expectedEPS            *discovery.EndpointSlice
+	}{
+		{
+			name:                   "gate enabled, field should be allowed",
+			terminatingGateEnabled: true,
+			eps: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+		},
+		{
+			name:                   "gate disabled, field should be set to nil",
+			terminatingGateEnabled: false,
+			eps: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceTerminatingCondition, testcase.terminatingGateEnabled)()
+
+			dropDisabledConditionsOnCreate(testcase.eps)
+			if !apiequality.Semantic.DeepEqual(testcase.eps, testcase.expectedEPS) {
+				t.Logf("actual endpointslice: %v", testcase.eps)
+				t.Logf("expected endpointslice: %v", testcase.expectedEPS)
+				t.Errorf("unexpected EndpointSlice on create API strategy")
+			}
+		})
+	}
+}
+
+func Test_dropTerminatingConditionOnUpdate(t *testing.T) {
+	testcases := []struct {
+		name                   string
+		terminatingGateEnabled bool
+		oldEPS                 *discovery.EndpointSlice
+		newEPS                 *discovery.EndpointSlice
+		expectedEPS            *discovery.EndpointSlice
+	}{
+		{
+			name:                   "gate enabled, field should be allowed",
+			terminatingGateEnabled: true,
+			oldEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			newEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+		},
+		{
+			name:                   "gate disabled, and not set on existing EPS",
+			terminatingGateEnabled: false,
+			oldEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			newEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+		},
+		{
+			name:                   "gate disabled, and set on existing EPS",
+			terminatingGateEnabled: false,
+			oldEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			newEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   nil,
+							Terminating: nil,
+						},
+					},
+				},
+			},
+		},
+		{
+			name:                   "gate disabled, and set on existing EPS with new values",
+			terminatingGateEnabled: false,
+			oldEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(false),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Terminating: nil,
+						},
+					},
+				},
+			},
+			newEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(false),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+				},
+			},
+			expectedEPS: &discovery.EndpointSlice{
+				Endpoints: []discovery.Endpoint{
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(true),
+							Terminating: utilpointer.BoolPtr(true),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Accepting:   utilpointer.BoolPtr(false),
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+					{
+						Conditions: discovery.EndpointConditions{
+							Terminating: utilpointer.BoolPtr(false),
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceTerminatingCondition, testcase.terminatingGateEnabled)()
+
+			dropDisabledConditionsOnUpdate(testcase.oldEPS, testcase.newEPS)
+			if !apiequality.Semantic.DeepEqual(testcase.newEPS, testcase.expectedEPS) {
+				t.Logf("actual endpointslice: %v", testcase.newEPS)
+				t.Logf("expected endpointslice: %v", testcase.expectedEPS)
+				t.Errorf("unexpected EndpointSlice from update API strategy")
+			}
+		})
+	}
+}