Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 22, 2018
+68 −7
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,6 @@ func (c *APIServiceRegistrationController) sync(key string) error {
return err
}

// remove registration handling for APIServices which are not available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to return 502 or 503 instead. Is that the case? We need a test for that.

I remember we had aggregator issues like long-running not terminating requests when the apiserver was unavailable. If I remember right, this code block was added to remedy that. But I agree that a 404 is not a good response at all. @deads2k

Copy link
Contributor Author

@weekface weekface Jan 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 503 is a better response. If a 404, it means the object was deleted from etcd. My controller may begin to delete all the sub resources related to this object. It is very dangerous

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the garbage collector will do the same on 404. Not good.

if !apiregistration.IsAPIServiceConditionTrue(apiService, apiregistration.Available) {
c.apiHandlerManager.RemoveAPIService(key)
return nil
}

return c.apiHandlerManager.AddAPIService(apiService)
}

Expand Down
8 changes: 8 additions & 0 deletions staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
57 changes: 57 additions & 0 deletions staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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",
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion test/e2e/apimachinery/aggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
}
Expand Down