diff --git a/pkg/registry/discovery/endpointslice/BUILD b/pkg/registry/discovery/endpointslice/BUILD index 9646eb83d7b..53821cb6d34 100644 --- a/pkg/registry/discovery/endpointslice/BUILD +++ b/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", + ], +) diff --git a/pkg/registry/discovery/endpointslice/strategy.go b/pkg/registry/discovery/endpointslice/strategy.go index ee50a67063a..949eab9c984 100644 --- a/pkg/registry/discovery/endpointslice/strategy.go +++ b/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 + } + } +} diff --git a/pkg/registry/discovery/endpointslice/strategy_test.go b/pkg/registry/discovery/endpointslice/strategy_test.go new file mode 100644 index 00000000000..ff2e628fcba --- /dev/null +++ b/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") + } + }) + } +}