UVIP: fix incorrect handling of a routable request

Author: richabanker

staging/src/k8s.io/apiserver/pkg/util/peerproxy/peerproxy_handler.go

@@ -162,20 +162,24 @@ func (h *peerProxyHandler) WrapHandler(handler http.Handler) http.Handler {
 			return
 		}
 
-		gv := schema.GroupVersion{Group: gvr.Group, Version: gvr.Version}
-
-		if serviceableByResp.errorFetchingAddressFromLease {
-			klog.ErrorS(err, "error fetching ip and port of remote server while proxying")
-			responsewriters.ErrorNegotiated(apierrors.NewServiceUnavailable("Error getting ip and port info of the remote server while proxying"), h.serializer, gv, w, r)
-			return
-		}
-
-		// no apiservers were found that could serve the request, pass request to
-		// next handler, that should eventually serve 404
+		// no peer endpoints found could mean one of the following things:
+		// 1. apiservers were found in storage version informer but either they had no endpoint
+		//    lease or their endpoint lease had invalid hostport information. Serve 503 in this case.
+		// 2. no apiservers were found in storage version informer cache meaning
+		//    this resource is not served by anything in this cluster. Pass request to
+		//    next handler, that should eventually serve 404.
 
 		// TODO: maintain locally serviceable GVRs somewhere so that we dont have to
 		// consult the storageversion-informed map for those
 		if len(serviceableByResp.peerEndpoints) == 0 {
+			gv := schema.GroupVersion{Group: gvr.Group, Version: gvr.Version}
+
+			if serviceableByResp.errorFetchingAddressFromLease {
+				klog.ErrorS(err, "error fetching ip and port of remote server while proxying")
+				responsewriters.ErrorNegotiated(apierrors.NewServiceUnavailable("Error getting ip and port info of the remote server while proxying"), h.serializer, gv, w, r)
+				return
+			}
+
 			klog.Errorf(fmt.Sprintf("GVR %v is not served by anything in this cluster", gvr))
 			handler.ServeHTTP(w, r)
 			return
@@ -203,7 +207,6 @@ func (h *peerProxyHandler) findServiceableByServers(gvr schema.GroupVersionResou
 	apiservers.Range(func(key, value interface{}) bool {
 		apiserverKey := key.(string)
 		if apiserverKey == localAPIServerId {
-			response.errorFetchingAddressFromLease = true
 			response.locallyServiceable = true
 			// stop iteration
 			return false
@@ -229,7 +232,10 @@ func (h *peerProxyHandler) findServiceableByServers(gvr schema.GroupVersionResou
 		return true
 	})
 
-	response.peerEndpoints = peerServerEndpoints
+	if len(peerServerEndpoints) > 0 {
+		response.errorFetchingAddressFromLease = false
+		response.peerEndpoints = peerServerEndpoints
+	}
 	return response, nil
 }
 

staging/src/k8s.io/apiserver/pkg/util/peerproxy/peerproxy_handler_test.go

@@ -59,8 +59,8 @@ const (
 )
 
 type FakeSVMapData struct {
-	gvr      schema.GroupVersionResource
-	serverId string
+	gvr       schema.GroupVersionResource
+	serverIds []string
 }
 
 type reconciler struct {
@@ -116,7 +116,7 @@ func TestPeerProxy(t *testing.T) {
 					Group:    "core",
 					Version:  "bar",
 					Resource: "baz"},
-				serverId: ""},
+				serverIds: []string{}},
 		},
 		{
 			desc:                 "503 if no endpoint fetched from lease",
@@ -128,7 +128,7 @@ func TestPeerProxy(t *testing.T) {
 					Group:    "core",
 					Version:  "foo",
 					Resource: "bar"},
-				serverId: remoteServerId},
+				serverIds: []string{remoteServerId}},
 		},
 		{
 			desc:                 "200 if locally serviceable",
@@ -140,7 +140,7 @@ func TestPeerProxy(t *testing.T) {
 					Group:    "core",
 					Version:  "foo",
 					Resource: "bar"},
-				serverId: localServerId},
+				serverIds: []string{localServerId}},
 		},
 		{
 			desc:                 "503 unreachable peer bind address",
@@ -152,7 +152,7 @@ func TestPeerProxy(t *testing.T) {
 					Group:    "core",
 					Version:  "foo",
 					Resource: "bar"},
-				serverId: remoteServerId},
+				serverIds: []string{remoteServerId}},
 			reconcilerConfig: reconciler{
 				do:       true,
 				publicIP: "1.2.3.4",
@@ -177,7 +177,7 @@ func TestPeerProxy(t *testing.T) {
 					Group:    "core",
 					Version:  "foo",
 					Resource: "bar"},
-				serverId: remoteServerId},
+				serverIds: []string{remoteServerId}},
 			reconcilerConfig: reconciler{
 				do:       true,
 				publicIP: "1.2.3.4",
@@ -192,6 +192,23 @@ func TestPeerProxy(t *testing.T) {
 			apiserver_rerouted_request_total{code="503"} 2
 			`,
 		},
+		{
+			desc:                 "503 if one apiserver's endpoint lease wasnt found but another valid (unreachable) apiserver was found",
+			requestPath:          "/api/foo/bar",
+			expectedStatus:       http.StatusServiceUnavailable,
+			informerFinishedSync: true,
+			svdata: FakeSVMapData{
+				gvr: schema.GroupVersionResource{
+					Group:    "core",
+					Version:  "foo",
+					Resource: "bar"},
+				serverIds: []string{"aggregated-apiserver", remoteServerId}},
+			reconcilerConfig: reconciler{
+				do:       true,
+				publicIP: "1.2.3.4",
+				serverId: remoteServerId,
+			},
+		},
 	}
 
 	metrics.Register()
@@ -290,16 +307,18 @@ func newFakePeerProxyHandler(informerFinishedSync bool, reconciler reconcilers.P
 	}
 	ppI := NewPeerProxyHandler(informerFactory, storageversion.NewDefaultManager(), proxyRoundTripper, id, reconciler, s)
 	if testDataExists(svdata.gvr) {
-		ppI.addToStorageVersionMap(svdata.gvr, svdata.serverId)
+		ppI.addToStorageVersionMap(svdata.gvr, svdata.serverIds)
 	}
 	return ppI, nil
 }
 
-func (h *peerProxyHandler) addToStorageVersionMap(gvr schema.GroupVersionResource, serverId string) {
+func (h *peerProxyHandler) addToStorageVersionMap(gvr schema.GroupVersionResource, serverIds []string) {
 	apiserversi, _ := h.svMap.LoadOrStore(gvr, &sync.Map{})
 	apiservers := apiserversi.(*sync.Map)
-	if serverId != "" {
-		apiservers.Store(serverId, true)
+	for _, serverID := range serverIds {
+		if serverID != "" {
+			apiservers.Store(serverID, true)
+		}
 	}
 }