From 88b39cadf877402b6fe7bfba78dbf5743fae76ce Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 5 May 2016 17:28:54 -0400 Subject: [PATCH] Have the service account controller force retry Service account controller, when API token not found, now sends 500 with Retry-After: 1s. Also change the apiserver to actually write the error. --- pkg/apiserver/apiserver.go | 6 +++++ pkg/apiserver/apiserver_test.go | 25 ++++++++++++++++++- .../pkg/admission/serviceaccount/admission.go | 9 +++++-- .../serviceaccount/admission_test.go | 5 ++-- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 49db40d618d..e96369c2975 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -26,6 +26,7 @@ import ( "net/http" "path" rt "runtime" + "strconv" "strings" "time" @@ -449,6 +450,11 @@ func writeNegotiated(s runtime.NegotiatedSerializer, gv unversioned.GroupVersion func errorNegotiated(err error, s runtime.NegotiatedSerializer, gv unversioned.GroupVersion, w http.ResponseWriter, req *http.Request) int { status := errToAPIStatus(err) code := int(status.Code) + // when writing an error, check to see if the status indicates a retry after period + if status.Details != nil && status.Details.RetryAfterSeconds > 0 { + delay := strconv.Itoa(int(status.Details.RetryAfterSeconds)) + w.Header().Set("Retry-After", delay) + } writeNegotiated(s, gv, w, req, code, status) return code } diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index cc89ddc0555..4408c16124d 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -1562,6 +1562,7 @@ func TestGetNamespaceSelfLink(t *testing.T) { t.Errorf("Never set self link") } } + func TestGetMissing(t *testing.T) { storage := map[string]rest.Storage{} simpleStorage := SimpleRESTStorage{ @@ -1572,7 +1573,7 @@ func TestGetMissing(t *testing.T) { server := httptest.NewServer(handler) defer server.Close() - resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/simple/id") + resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/id") if err != nil { t.Errorf("unexpected error: %v", err) } @@ -1582,6 +1583,28 @@ func TestGetMissing(t *testing.T) { } } +func TestGetRetryAfter(t *testing.T) { + storage := map[string]rest.Storage{} + simpleStorage := SimpleRESTStorage{ + errors: map[string]error{"get": apierrs.NewServerTimeout(api.Resource("simples"), "id", 2)}, + } + storage["simple"] = &simpleStorage + handler := handle(storage) + server := httptest.NewServer(handler) + defer server.Close() + + resp, err := http.Get(server.URL + "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/simple/id") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusInternalServerError { + t.Errorf("Unexpected response %#v", resp) + } + if resp.Header.Get("Retry-After") != "2" { + t.Errorf("Unexpected Retry-After header: %v", resp.Header) + } +} + func TestConnect(t *testing.T) { responseText := "Hello World" itemID := "theID" diff --git a/plugin/pkg/admission/serviceaccount/admission.go b/plugin/pkg/admission/serviceaccount/admission.go index 55b7bfaa6fc..7be0dded2b2 100644 --- a/plugin/pkg/admission/serviceaccount/admission.go +++ b/plugin/pkg/admission/serviceaccount/admission.go @@ -28,6 +28,7 @@ import ( "k8s.io/kubernetes/pkg/admission" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/errors" + "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/client/cache" "k8s.io/kubernetes/pkg/fields" kubelet "k8s.io/kubernetes/pkg/kubelet/types" @@ -199,6 +200,9 @@ func (s *serviceAccount) Admit(a admission.Attributes) (err error) { if s.MountServiceAccountToken { if err := s.mountServiceAccountToken(serviceAccount, pod); err != nil { + if _, ok := err.(errors.APIStatus); ok { + return err + } return admission.NewForbidden(a, err) } } @@ -357,8 +361,9 @@ func (s *serviceAccount) mountServiceAccountToken(serviceAccount *api.ServiceAcc // We don't have an API token to mount, so return if s.RequireAPIToken { // If a token is required, this is considered an error - // TODO: convert to a ServerTimeout error (or other error that sends a Retry-After header) - return fmt.Errorf("no API token found for service account %s/%s, retry after the token is automatically created and added to the service account", serviceAccount.Namespace, serviceAccount.Name) + err := errors.NewServerTimeout(unversioned.GroupResource{Resource: "serviceaccounts"}, "create pod", 1) + err.ErrStatus.Message = fmt.Sprintf("No API token found for service account %q, retry after the token is automatically created and added to the service account", serviceAccount.Name) + return err } return nil } diff --git a/plugin/pkg/admission/serviceaccount/admission_test.go b/plugin/pkg/admission/serviceaccount/admission_test.go index f6d9addbbf1..9640b1fd789 100644 --- a/plugin/pkg/admission/serviceaccount/admission_test.go +++ b/plugin/pkg/admission/serviceaccount/admission_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/kubernetes/pkg/admission" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake" kubelet "k8s.io/kubernetes/pkg/kubelet/types" "k8s.io/kubernetes/pkg/types" @@ -168,8 +169,8 @@ func TestAssignsDefaultServiceAccountAndRejectsMissingAPIToken(t *testing.T) { pod := &api.Pod{} attrs := admission.NewAttributesRecord(pod, api.Kind("Pod").WithVersion("version"), ns, "myname", api.Resource("pods").WithVersion("version"), "", admission.Create, nil) err := admit.Admit(attrs) - if err == nil { - t.Errorf("Expected admission error for missing API token") + if err == nil || !errors.IsServerTimeout(err) { + t.Errorf("Expected server timeout error for missing API token: %v", err) } }