Merge pull request #58697 from liggitt/aggregator-e2e-fix

Automatic merge from submit-queue (batch tested with PRs 58697, 58658, 58676, 58674). 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>.

Wait for healthy extension server before registering APIService, handle ServiceUnavailable errors

fixes #58642 
followup to #58070

* Because a registered APIService appears in discovery immediately, we should wait until the backing deployment is healthy before exposing it
* In e2e hasRemainingContent(), add ServiceUnavailable to the types of errors we tolerate when looking for remaining content.
* In proxy handler, return a ServiceUnavailable error if the referenced service cannot be resolved
```release-note
NONE
```

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

@@ -120,7 +120,8 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	location.Scheme = "https"
 	rloc, err := r.serviceResolver.ResolveEndpoint(handlingInfo.serviceNamespace, handlingInfo.serviceName)
 	if err != nil {
-		http.Error(w, fmt.Sprintf("missing route (%s)", err.Error()), http.StatusInternalServerError)
+		glog.Errorf("error resolving %s/%s: %v", handlingInfo.serviceNamespace, handlingInfo.serviceName, err)
+		http.Error(w, "service unavailable", http.StatusServiceUnavailable)
 		return
 	}
 	location.Host = rloc.Host

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

@@ -18,6 +18,7 @@ package apiserver
 
 import (
 	"crypto/tls"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -84,13 +85,11 @@ func (*fakeRequestContextMapper) Update(req *http.Request, context genericapireq
 
 type mockedRouter struct {
 	destinationHost string
+	err             error
 }
 
 func (r *mockedRouter) ResolveEndpoint(namespace, name string) (*url.URL, error) {
-	return &url.URL{
+	return &url.URL{Scheme: "https", Host: r.destinationHost}, r.err
-		Scheme: "https",
-		Host:   r.destinationHost,
-	}, nil
 }
 
 func TestProxyHandler(t *testing.T) {
@@ -109,6 +108,8 @@ func TestProxyHandler(t *testing.T) {
 		path       string
 		apiService *apiregistration.APIService
 
+		serviceResolver ServiceResolver
+
 		expectedStatusCode int
 		expectedBody       string
 		expectedCalled     bool
@@ -220,6 +221,29 @@ func TestProxyHandler(t *testing.T) {
 			},
 			expectedStatusCode: http.StatusServiceUnavailable,
 		},
+		"service unresolveable": {
+			user: &user.DefaultInfo{
+				Name:   "username",
+				Groups: []string{"one", "two"},
+			},
+			path:            "/request/path",
+			serviceResolver: &mockedRouter{err: fmt.Errorf("unresolveable")},
+			apiService: &apiregistration.APIService{
+				ObjectMeta: metav1.ObjectMeta{Name: "v1.foo"},
+				Spec: apiregistration.APIServiceSpec{
+					Service:  &apiregistration.ServiceReference{Name: "bad-service", Namespace: "test-ns"},
+					Group:    "foo",
+					Version:  "v1",
+					CABundle: testCACrt,
+				},
+				Status: apiregistration.APIServiceStatus{
+					Conditions: []apiregistration.APIServiceCondition{
+						{Type: apiregistration.Available, Status: apiregistration.ConditionTrue},
+					},
+				},
+			},
+			expectedStatusCode: http.StatusServiceUnavailable,
+		},
 		"fail on bad serving cert": {
 			user: &user.DefaultInfo{
 				Name:   "username",
@@ -247,9 +271,13 @@ func TestProxyHandler(t *testing.T) {
 		target.Reset()
 
 		func() {
+			serviceResolver := tc.serviceResolver
+			if serviceResolver == nil {
+				serviceResolver = &mockedRouter{destinationHost: targetServer.Listener.Addr().String()}
+			}
 			handler := &proxyHandler{
 				localDelegate:   http.NewServeMux(),
-				serviceResolver: &mockedRouter{destinationHost: targetServer.Listener.Addr().String()},
+				serviceResolver: serviceResolver,
 				proxyTransport:  &http.Transport{},
 			}
 			handler.contextMapper = &fakeRequestContextMapper{user: tc.user}

test/e2e/apimachinery/aggregator.go

@@ -291,6 +291,13 @@ func TestSampleAPIServer(f *framework.Framework, image string) {
 	})
 	framework.ExpectNoError(err, "creating role binding %s:sample-apiserver to access configMap", namespace)
 
+	// Wait for the extension apiserver to be up and healthy
+	// kubectl get deployments -n <aggregated-api-namespace> && status == Running
+	// NOTE: aggregated apis should generally be set up in there own namespace (<aggregated-api-namespace>). As the test framework
+	// is setting up a new namespace, we are just using that.
+	err = framework.WaitForDeploymentComplete(client, deployment)
+	framework.ExpectNoError(err, "deploying extension apiserver in namespace %s", namespace)
+
 	// kubectl create -f apiservice.yaml
 	_, err = aggrclient.ApiregistrationV1beta1().APIServices().Create(&apiregistrationv1beta1.APIService{
 		ObjectMeta: metav1.ObjectMeta{Name: "v1alpha1.wardle.k8s.io"},
@@ -308,12 +315,6 @@ func TestSampleAPIServer(f *framework.Framework, image string) {
 	})
 	framework.ExpectNoError(err, "creating apiservice %s with namespace %s", "v1alpha1.wardle.k8s.io", namespace)
 
-	// Wait for the extension apiserver to be up and healthy
-	// kubectl get deployments -n <aggregated-api-namespace> && status == Running
-	// NOTE: aggregated apis should generally be set up in there own namespace (<aggregated-api-namespace>). As the test framework
-	// is setting up a new namespace, we are just using that.
-	err = framework.WaitForDeploymentComplete(client, deployment)
-
 	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")

test/e2e/framework/util.go

@@ -1200,6 +1200,10 @@ func hasRemainingContent(c clientset.Interface, clientPool dynamic.ClientPool, n
 			if apierrs.IsMethodNotSupported(err) || apierrs.IsNotFound(err) || apierrs.IsForbidden(err) {
 				continue
 			}
+			// skip unavailable servers
+			if apierrs.IsServiceUnavailable(err) {
+				continue
+			}
 			return false, err
 		}
 		unstructuredList, ok := obj.(*unstructured.UnstructuredList)