diff --git a/pkg/registry/discovery/endpointslice/strategy.go b/pkg/registry/discovery/endpointslice/strategy.go index 9e2c48bc246..92cb2e65e06 100644 --- a/pkg/registry/discovery/endpointslice/strategy.go +++ b/pkg/registry/discovery/endpointslice/strategy.go @@ -50,7 +50,7 @@ func (endpointSliceStrategy) PrepareForCreate(ctx context.Context, obj runtime.O endpointSlice := obj.(*discovery.EndpointSlice) endpointSlice.Generation = 1 - dropDisabledConditionsOnCreate(endpointSlice) + dropDisabledFieldsOnCreate(endpointSlice) } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. @@ -72,7 +72,7 @@ func (endpointSliceStrategy) PrepareForUpdate(ctx context.Context, obj, old runt newEPS.ObjectMeta = ogNewMeta oldEPS.ObjectMeta = ogOldMeta - dropDisabledConditionsOnUpdate(oldEPS, newEPS) + dropDisabledFieldsOnUpdate(oldEPS, newEPS) } // Validate validates a new EndpointSlice. @@ -103,41 +103,56 @@ 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 - } +// dropDisabledConditionsOnCreate will drop any fields that are disabled. +func dropDisabledFieldsOnCreate(endpointSlice *discovery.EndpointSlice) { + dropNodeName := !utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceNodeName) + dropTerminating := !utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceTerminatingCondition) - // Always drop the serving/terminating conditions on create when feature gate is disabled. - for i := range endpointSlice.Endpoints { - endpointSlice.Endpoints[i].Conditions.Serving = nil - endpointSlice.Endpoints[i].Conditions.Terminating = nil + if dropNodeName || dropTerminating { + for i := range endpointSlice.Endpoints { + if dropNodeName { + endpointSlice.Endpoints[i].NodeName = nil + } + if dropTerminating { + endpointSlice.Endpoints[i].Conditions.Serving = 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 serving/terminating condition if the existing EndpointSlice doesn't have it set. - dropConditions := true - for _, ep := range oldEPS.Endpoints { - if ep.Conditions.Serving != nil || ep.Conditions.Terminating != nil { - dropConditions = false - break +// dropDisabledFieldsOnUpdate will drop any disable fields that have not already +// been set on the EndpointSlice. +func dropDisabledFieldsOnUpdate(oldEPS, newEPS *discovery.EndpointSlice) { + dropNodeName := !utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceNodeName) + if dropNodeName { + for _, ep := range oldEPS.Endpoints { + if ep.NodeName != nil { + dropNodeName = false + break + } } } - if dropConditions { + dropTerminating := !utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceTerminatingCondition) + if dropTerminating { + for _, ep := range oldEPS.Endpoints { + if ep.Conditions.Serving != nil || ep.Conditions.Terminating != nil { + dropTerminating = false + break + } + } + } + + if dropNodeName || dropTerminating { for i := range newEPS.Endpoints { - newEPS.Endpoints[i].Conditions.Serving = nil - newEPS.Endpoints[i].Conditions.Terminating = nil + if dropNodeName { + newEPS.Endpoints[i].NodeName = nil + } + if dropTerminating { + newEPS.Endpoints[i].Conditions.Serving = 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 index 81bb687b8eb..fce0bf76231 100644 --- a/pkg/registry/discovery/endpointslice/strategy_test.go +++ b/pkg/registry/discovery/endpointslice/strategy_test.go @@ -27,15 +27,16 @@ import ( utilpointer "k8s.io/utils/pointer" ) -func Test_dropConditionsOnCreate(t *testing.T) { +func Test_dropDisabledFieldsOnCreate(t *testing.T) { testcases := []struct { name string terminatingGateEnabled bool + nodeNameGateEnabled bool eps *discovery.EndpointSlice expectedEPS *discovery.EndpointSlice }{ { - name: "gate enabled, field should be allowed", + name: "terminating gate enabled, field should be allowed", terminatingGateEnabled: true, eps: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -83,7 +84,7 @@ func Test_dropConditionsOnCreate(t *testing.T) { }, }, { - name: "gate disabled, field should be set to nil", + name: "terminating gate disabled, field should be set to nil", terminatingGateEnabled: false, eps: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -130,13 +131,62 @@ func Test_dropConditionsOnCreate(t *testing.T) { }, }, }, + { + name: "node name gate enabled, field should be allowed", + nodeNameGateEnabled: true, + eps: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + }, + { + name: "node name gate disabled, field should be allowed", + nodeNameGateEnabled: false, + eps: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: nil, + }, + { + NodeName: nil, + }, + }, + }, + }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceTerminatingCondition, testcase.terminatingGateEnabled)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceNodeName, testcase.nodeNameGateEnabled)() - dropDisabledConditionsOnCreate(testcase.eps) + dropDisabledFieldsOnCreate(testcase.eps) if !apiequality.Semantic.DeepEqual(testcase.eps, testcase.expectedEPS) { t.Logf("actual endpointslice: %v", testcase.eps) t.Logf("expected endpointslice: %v", testcase.expectedEPS) @@ -146,16 +196,17 @@ func Test_dropConditionsOnCreate(t *testing.T) { } } -func Test_dropTerminatingConditionOnUpdate(t *testing.T) { +func Test_dropDisabledFieldsOnUpdate(t *testing.T) { testcases := []struct { name string terminatingGateEnabled bool + nodeNameGateEnabled bool oldEPS *discovery.EndpointSlice newEPS *discovery.EndpointSlice expectedEPS *discovery.EndpointSlice }{ { - name: "gate enabled, field should be allowed", + name: "terminating gate enabled, field should be allowed", terminatingGateEnabled: true, oldEPS: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -225,7 +276,7 @@ func Test_dropTerminatingConditionOnUpdate(t *testing.T) { }, }, { - name: "gate disabled, and not set on existing EPS", + name: "terminating gate disabled, and not set on existing EPS", terminatingGateEnabled: false, oldEPS: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -295,7 +346,7 @@ func Test_dropTerminatingConditionOnUpdate(t *testing.T) { }, }, { - name: "gate disabled, and set on existing EPS", + name: "terminating gate disabled, and set on existing EPS", terminatingGateEnabled: false, oldEPS: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -365,7 +416,7 @@ func Test_dropTerminatingConditionOnUpdate(t *testing.T) { }, }, { - name: "gate disabled, and set on existing EPS with new values", + name: "terminating gate disabled, and set on existing EPS with new values", terminatingGateEnabled: false, oldEPS: &discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ @@ -431,13 +482,116 @@ func Test_dropTerminatingConditionOnUpdate(t *testing.T) { }, }, }, + { + name: "node name gate enabled, set on new EPS", + nodeNameGateEnabled: true, + oldEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: nil, + }, + { + NodeName: nil, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + }, + { + name: "node name gate disabled, set on new EPS", + nodeNameGateEnabled: false, + oldEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: nil, + }, + { + NodeName: nil, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: nil, + }, + { + NodeName: nil, + }, + }, + }, + }, + { + name: "node name gate disabled, set on old and updated EPS", + nodeNameGateEnabled: false, + oldEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1-old"), + }, + { + NodeName: utilpointer.StringPtr("node-2-old"), + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + NodeName: utilpointer.StringPtr("node-1"), + }, + { + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceTerminatingCondition, testcase.terminatingGateEnabled)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceNodeName, testcase.nodeNameGateEnabled)() - dropDisabledConditionsOnUpdate(testcase.oldEPS, testcase.newEPS) + dropDisabledFieldsOnUpdate(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)