Merge pull request #108198 from liggitt/endpointslice-topology-strategy-fix

Make EndpointSlice strategy move node name from topology map to field instead of discarding when updating via v1

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
+}

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)
+			}
+		})
+	}
+}