diff --git a/pkg/registry/discovery/endpointslice/strategy.go b/pkg/registry/discovery/endpointslice/strategy.go index 37ac9dd4663..1473563ce10 100644 --- a/pkg/registry/discovery/endpointslice/strategy.go +++ b/pkg/registry/discovery/endpointslice/strategy.go @@ -19,16 +19,19 @@ package endpointslice import ( "context" + corev1 "k8s.io/api/core/v1" discoveryv1beta1 "k8s.io/api/discovery/v1beta1" apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" + apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/discovery" "k8s.io/kubernetes/pkg/apis/discovery/validation" "k8s.io/kubernetes/pkg/features" @@ -186,11 +189,40 @@ func dropTopologyOnV1(ctx context.Context, oldEPS, newEPS *discovery.EndpointSli return } + // Only node names that exist in previous version of the EndpointSlice + // deprecatedTopology fields may be retained in new version of the + // EndpointSlice. + prevNodeNames := getDeprecatedTopologyNodeNames(oldEPS) + for i := range newEPS.Endpoints { ep := &newEPS.Endpoints[i] - //Silently clear out DeprecatedTopology + newTopologyNodeName, ok := ep.DeprecatedTopology[corev1.LabelHostname] + if ep.NodeName == nil && ok && prevNodeNames.Has(newTopologyNodeName) && len(apivalidation.ValidateNodeName(newTopologyNodeName, false)) == 0 { + // Copy the label previously used to store the node name into the nodeName field, + // in order to make partial updates preserve previous node info + ep.NodeName = &newTopologyNodeName + } + // Drop writes to this field via the v1 API as documented ep.DeprecatedTopology = nil } } } + +// getDeprecatedTopologyNodeNames returns a set of node names present in +// deprecatedTopology fields within the provided EndpointSlice. +func getDeprecatedTopologyNodeNames(eps *discovery.EndpointSlice) sets.String { + if eps == nil { + return nil + } + var names sets.String + for _, ep := range eps.Endpoints { + if nodeName, ok := ep.DeprecatedTopology[corev1.LabelHostname]; ok && len(nodeName) > 0 { + if names == nil { + names = sets.NewString() + } + names.Insert(nodeName) + } + } + return names +} diff --git a/pkg/registry/discovery/endpointslice/strategy_test.go b/pkg/registry/discovery/endpointslice/strategy_test.go index 6d80cea7e72..8dd6a6edb11 100644 --- a/pkg/registry/discovery/endpointslice/strategy_test.go +++ b/pkg/registry/discovery/endpointslice/strategy_test.go @@ -23,6 +23,7 @@ import ( corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -980,6 +981,312 @@ func Test_dropTopologyOnV1(t *testing.T) { }, }, }, + { + name: "v1 request, updated endpoints with topology node names + other topology fields", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1", "other": "value"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1", "foo": "bar"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1", "other": "value"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1", "foo": "bar"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + NodeName: utilpointer.StringPtr("node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-1"), + }, + }, + }, + }, + { + name: "v1 request, updated endpoints with topology node names", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + NodeName: utilpointer.StringPtr("node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-1"), + }, + }, + }, + }, + { + name: "v1 request, updated endpoints with topology node names swapped", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-2"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-2"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + NodeName: utilpointer.StringPtr("node-2"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-1"), + }, + }, + }, + }, + { + name: "v1 request, updated endpoints with new topology node name", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-2"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + // Invalid node name because it did not exist in previous version of EndpointSlice + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-3"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-1"), + }, + }, + }, + }, + { + name: "v1 request, updated endpoints with topology node names + 1 new node name", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-2"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + NodeName: utilpointer.StringPtr("node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + }, + { + name: "v1 request, updated endpoints with topology node names + new node names", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + NodeName: utilpointer.StringPtr("node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-2"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}, + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1a"), + NodeName: utilpointer.StringPtr("node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-1b"), + NodeName: utilpointer.StringPtr("node-2"), + }, + }, + }, + }, + { + name: "v1 request, invalid node name label", + v1Request: true, + originalEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "valid-node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-2"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "invalid node-2"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-3"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "valid-node-3"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-4"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "invalid node-4"}, + }, + }, + }, + newEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "valid-node-1"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-2"), + DeprecatedTopology: map[string]string{corev1.LabelHostname: "invalid node-2"}, + }, + { + Hostname: utilpointer.StringPtr("hostname-3"), + NodeName: utilpointer.StringPtr("node-3"), + }, + { + Hostname: utilpointer.StringPtr("hostname-4"), + NodeName: utilpointer.StringPtr("node-4"), + }, + }, + }, + expectedEPS: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + { + Hostname: utilpointer.StringPtr("hostname-1"), + NodeName: utilpointer.StringPtr("valid-node-1"), + }, + { + Hostname: utilpointer.StringPtr("hostname-2"), + }, + { + Hostname: utilpointer.StringPtr("hostname-3"), + NodeName: utilpointer.StringPtr("node-3"), + }, + { + Hostname: utilpointer.StringPtr("hostname-4"), + NodeName: utilpointer.StringPtr("node-4"), + }, + }, + }, + }, } for _, tc := range testcases { @@ -998,3 +1305,53 @@ func Test_dropTopologyOnV1(t *testing.T) { }) } } + +func Test_getDeprecatedTopologyNodeNames(t *testing.T) { + testcases := []struct { + name string + endpointSlice *discovery.EndpointSlice + expectedNodeNames sets.String + }{ + { + name: "2 nodes", + endpointSlice: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + {DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}}, + {DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-2"}}, + }, + }, + expectedNodeNames: sets.NewString("node-1", "node-2"), + }, + { + name: "duplicate values", + endpointSlice: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + {DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-1"}}, + {DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-3"}}, + {DeprecatedTopology: map[string]string{corev1.LabelHostname: "node-3"}}, + }, + }, + expectedNodeNames: sets.NewString("node-1", "node-3"), + }, + { + name: "unset", + endpointSlice: &discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{ + {DeprecatedTopology: map[string]string{"other": "value"}}, + {DeprecatedTopology: map[string]string{"foo": "bar"}}, + {DeprecatedTopology: nil}, + }, + }, + expectedNodeNames: sets.NewString(), + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + actualNames := getDeprecatedTopologyNodeNames(tc.endpointSlice) + if !tc.expectedNodeNames.Equal(actualNames) { + t.Errorf("Expected %+v node names, got %+v", tc.expectedNodeNames, actualNames) + } + }) + } +}