Merge pull request #58070 from weekface/weekface/aggregator-proxy-fix

Automatic merge from submit-queue (batch tested with PRs 57896, 58070). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Don't remove APIService from apiHandlerManager when its Available Conditions is not True

**What this PR does / why we need it**:

I use my own apiserver works together with `kube-apiserver`, i have a custom resource: `databases` and created a `database` named: `db-name-1`.

When this apiserver is down(for example: OOMKilled), `kubectl get databases db-name-1 -v 10` returns `404 NotFound`:

```
[{
  "metadata": {},
  "status": "Failure",
  "message": "the server could not find the requested resource (get databases.core.example.com db-name-1)”,
  "reason": "NotFound",
  "details": {
    "name": “db-name-1”,
    "group": "core.example.com",
    "kind": “databases”,
    "causes": [
      {
        "reason": "UnexpectedServerResponse",
        "message": "404 page not found"
      }
    ]
  },
  "code": 404
}]
```

But it is not really `NotFound`.

So if the APIService is not available, just return 503.

There was a PR related with this: #57943 

**Release note**:


```release-note
kube-apiserver: requests to endpoints handled by unavailable extension API servers (as indicated by an `Available` condition of `false` in the registered APIService) now return `503` errors instead of `404` errors.
```

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go

@@ -80,12 +80,6 @@ func (c *APIServiceRegistrationController) sync(key string) error {
 		return err
 	}
 
-	// remove registration handling for APIServices which are not available
-	if !apiregistration.IsAPIServiceConditionTrue(apiService, apiregistration.Available) {
-		c.apiHandlerManager.RemoveAPIService(key)
-		return nil
-	}
-
 	return c.apiHandlerManager.AddAPIService(apiService)
 }
 

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -74,6 +74,8 @@ type proxyHandlingInfo struct {
 	serviceName string
 	// namespace is the namespace the service lives in
 	serviceNamespace string
+	// serviceAvailable indicates this APIService is available or not
+	serviceAvailable bool
 }
 
 func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
@@ -92,6 +94,11 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	if !handlingInfo.serviceAvailable {
+		http.Error(w, "service unavailable", http.StatusServiceUnavailable)
+		return
+	}
+
 	if handlingInfo.transportBuildingError != nil {
 		http.Error(w, handlingInfo.transportBuildingError.Error(), http.StatusInternalServerError)
 		return
@@ -205,6 +212,7 @@ func (r *proxyHandler) updateAPIService(apiService *apiregistrationapi.APIServic
 		},
 		serviceName:      apiService.Spec.Service.Name,
 		serviceNamespace: apiService.Spec.Service.Namespace,
+		serviceAvailable: apiregistrationapi.IsAPIServiceConditionTrue(apiService, apiregistrationapi.Available),
 	}
 	newInfo.proxyRoundTripper, newInfo.transportBuildingError = restclient.TransportFor(newInfo.restConfig)
 	if newInfo.transportBuildingError == nil && r.proxyTransport != nil && r.proxyTransport.Dial != nil {

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

@@ -125,6 +125,11 @@ func TestProxyHandler(t *testing.T) {
 					Group:   "foo",
 					Version: "v1",
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			expectedStatusCode: http.StatusInternalServerError,
 			expectedBody:       "missing user",
@@ -143,6 +148,11 @@ func TestProxyHandler(t *testing.T) {
 					Version:               "v1",
 					InsecureSkipTLSVerify: true,
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			expectedStatusCode: http.StatusOK,
 			expectedCalled:     true,
@@ -170,6 +180,11 @@ func TestProxyHandler(t *testing.T) {
 					Version:  "v1",
 					CABundle: testCACrt,
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			expectedStatusCode: http.StatusOK,
 			expectedCalled:     true,
@@ -183,6 +198,28 @@ func TestProxyHandler(t *testing.T) {
 				"X-Remote-Group":    {"one", "two"},
 			},
 		},
+		"service unavailable": {
+			user: &user.DefaultInfo{
+				Name:   "username",
+				Groups: []string{"one", "two"},
+			},
+			path: "/request/path",
+			apiService: &apiregistration.APIService{
+				ObjectMeta: metav1.ObjectMeta{Name: "v1.foo"},
+				Spec: apiregistration.APIServiceSpec{
+					Service:  &apiregistration.ServiceReference{Name: "test-service", Namespace: "test-ns"},
+					Group:    "foo",
+					Version:  "v1",
+					CABundle: testCACrt,
+				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionFalse},
+					},
+				},
+			},
+			expectedStatusCode: http.StatusServiceUnavailable,
+		},
 		"fail on bad serving cert": {
 			user: &user.DefaultInfo{
 				Name:   "username",
@@ -196,6 +233,11 @@ func TestProxyHandler(t *testing.T) {
 					Group:   "foo",
 					Version: "v1",
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			expectedStatusCode: http.StatusServiceUnavailable,
 		},
@@ -269,6 +311,11 @@ func TestProxyUpgrade(t *testing.T) {
 					Version:  "v1",
 					Service:  &apiregistration.ServiceReference{Name: "test-service", Namespace: "test-ns"},
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			ExpectError:  false,
 			ExpectCalled: true,
@@ -281,6 +328,11 @@ func TestProxyUpgrade(t *testing.T) {
 					Version: "v1",
 					Service: &apiregistration.ServiceReference{Name: "invalid-service", Namespace: "invalid-ns"},
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			ExpectError:  false,
 			ExpectCalled: true,
@@ -293,6 +345,11 @@ func TestProxyUpgrade(t *testing.T) {
 					Version:  "v1",
 					Service:  &apiregistration.ServiceReference{Name: "invalid-service", Namespace: "invalid-ns"},
 				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
 			},
 			ExpectError:  true,
 			ExpectCalled: false,

test/e2e/apimachinery/aggregator.go

@@ -314,7 +314,6 @@ func TestSampleAPIServer(f *framework.Framework, image string) {
 	// is setting up a new namespace, we are just using that.
 	err = framework.WaitForDeploymentComplete(client, deployment)
 
-	// We seem to need to do additional waiting until the extension api service is actually up.
 	err = wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
 		request := restClient.Get().AbsPath("/apis/wardle.k8s.io/v1alpha1/namespaces/default/flunders")
 		request.SetHeader("Accept", "application/json")
@@ -324,6 +323,9 @@ func TestSampleAPIServer(f *framework.Framework, image string) {
 			if !ok {
 				return false, err
 			}
+			if status.Status().Code == 503 {
+				return false, nil
+			}
 			if status.Status().Code == 404 && strings.HasPrefix(err.Error(), "the server could not find the requested resource") {
 				return false, nil
 			}