From b8d46edce637ed32a55f0f46adb4af67d690e4dc Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Thu, 1 Nov 2018 10:19:18 -0400 Subject: [PATCH] 14262: Move the context deadline to the top of the handler stack Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- lib/controller/fed_collections.go | 18 +++--------------- lib/controller/fed_containers.go | 8 +------- lib/controller/fed_generic.go | 14 +++----------- lib/controller/federation.go | 7 +++---- lib/controller/federation_test.go | 9 +++++++++ lib/controller/handler.go | 18 ++++++++++-------- lib/controller/proxy.go | 17 ++++------------- 7 files changed, 33 insertions(+), 58 deletions(-) diff --git a/lib/controller/fed_collections.go b/lib/controller/fed_collections.go index 88b0f95a02..b9cd205829 100644 --- a/lib/controller/fed_collections.go +++ b/lib/controller/fed_collections.go @@ -178,10 +178,7 @@ func (h *collectionFederatedRequestHandler) ServeHTTP(w http.ResponseWriter, req if clusterId != "" && clusterId != h.handler.Cluster.ClusterID { // request for remote collection by uuid - resp, cancel, err := h.handler.remoteClusterRequest(clusterId, req) - if cancel != nil { - defer cancel() - } + resp, err := h.handler.remoteClusterRequest(clusterId, req) newResponse, err := rewriteSignatures(clusterId, "", resp, err) h.handler.proxy.ForwardResponse(w, newResponse, err) return @@ -196,10 +193,7 @@ func (h *collectionFederatedRequestHandler) ServeHTTP(w http.ResponseWriter, req // Request for collection by PDH. Search the federation. // First, query the local cluster. - resp, localClusterRequestCancel, err := h.handler.localClusterRequest(req) - if localClusterRequestCancel != nil { - defer localClusterRequestCancel() - } + resp, err := h.handler.localClusterRequest(req) newResp, err := filterLocalClusterResponse(resp, err) if newResp != nil || err != nil { h.handler.proxy.ForwardResponse(w, newResp, err) @@ -244,19 +238,13 @@ func (h *collectionFederatedRequestHandler) ServeHTTP(w http.ResponseWriter, req default: } - resp, _, err := h.handler.remoteClusterRequest(remote, req) + resp, err := h.handler.remoteClusterRequest(remote, req) wasSuccess := false defer func() { if resp != nil && !wasSuccess { resp.Body.Close() } }() - // Don't need to do anything with the cancel - // function returned by remoteClusterRequest - // because the context inherits from - // sharedContext, so when sharedContext is - // cancelled it should cancel that one as - // well. if err != nil { errorChan <- err return diff --git a/lib/controller/fed_containers.go b/lib/controller/fed_containers.go index fc627d3faf..e4c80a32cc 100644 --- a/lib/controller/fed_containers.go +++ b/lib/controller/fed_containers.go @@ -9,7 +9,6 @@ import ( "encoding/json" "fmt" "io/ioutil" - "log" "net/http" "git.curoverse.com/arvados.git/sdk/go/auth" @@ -64,8 +63,6 @@ func remoteContainerRequestCreate( // If runtime_token is not set, create a new token if _, ok := containerRequest["runtime_token"]; !ok { - log.Printf("ok %v", ok) - // First make sure supplied token is valid. creds := auth.NewCredentials() creds.LoadTokensFromHTTPRequest(req) @@ -98,10 +95,7 @@ func remoteContainerRequestCreate( req.ContentLength = int64(buf.Len()) req.Header.Set("Content-Length", fmt.Sprintf("%v", buf.Len())) - resp, cancel, err := h.handler.remoteClusterRequest(*clusterId, req) - if cancel != nil { - defer cancel() - } + resp, err := h.handler.remoteClusterRequest(*clusterId, req) h.handler.proxy.ForwardResponse(w, resp, err) return true } diff --git a/lib/controller/fed_generic.go b/lib/controller/fed_generic.go index 7d5b63d310..63e61e6908 100644 --- a/lib/controller/fed_generic.go +++ b/lib/controller/fed_generic.go @@ -6,7 +6,6 @@ package controller import ( "bytes" - "context" "encoding/json" "fmt" "io/ioutil" @@ -66,16 +65,12 @@ func (h *genericFederatedRequestHandler) remoteQueryUUIDs(w http.ResponseWriter, rc := multiClusterQueryResponseCollector{clusterID: clusterID} var resp *http.Response - var cancel context.CancelFunc if clusterID == h.handler.Cluster.ClusterID { - resp, cancel, err = h.handler.localClusterRequest(&remoteReq) + resp, err = h.handler.localClusterRequest(&remoteReq) } else { - resp, cancel, err = h.handler.remoteClusterRequest(clusterID, &remoteReq) + resp, err = h.handler.remoteClusterRequest(clusterID, &remoteReq) } rc.collectResponse(resp, err) - if cancel != nil { - cancel() - } if rc.error != nil { return nil, "", rc.error @@ -309,10 +304,7 @@ func (h *genericFederatedRequestHandler) ServeHTTP(w http.ResponseWriter, req *h if clusterId == "" || clusterId == h.handler.Cluster.ClusterID { h.next.ServeHTTP(w, req) } else { - resp, cancel, err := h.handler.remoteClusterRequest(clusterId, req) - if cancel != nil { - defer cancel() - } + resp, err := h.handler.remoteClusterRequest(clusterId, req) h.handler.proxy.ForwardResponse(w, resp, err) } } diff --git a/lib/controller/federation.go b/lib/controller/federation.go index 0e016f301d..e08a1c1674 100644 --- a/lib/controller/federation.go +++ b/lib/controller/federation.go @@ -6,7 +6,6 @@ package controller import ( "bytes" - "context" "database/sql" "encoding/json" "fmt" @@ -29,10 +28,10 @@ var containerRequestsRe = regexp.MustCompile(fmt.Sprintf(pathPattern, "container var collectionRe = regexp.MustCompile(fmt.Sprintf(pathPattern, "collections", "4zz18")) var collectionByPDHRe = regexp.MustCompile(`^/arvados/v1/collections/([0-9a-fA-F]{32}\+[0-9]+)+$`) -func (h *Handler) remoteClusterRequest(remoteID string, req *http.Request) (*http.Response, context.CancelFunc, error) { +func (h *Handler) remoteClusterRequest(remoteID string, req *http.Request) (*http.Response, error) { remote, ok := h.Cluster.RemoteClusters[remoteID] if !ok { - return nil, nil, HTTPError{fmt.Sprintf("no proxy available for cluster %v", remoteID), http.StatusNotFound} + return nil, HTTPError{fmt.Sprintf("no proxy available for cluster %v", remoteID), http.StatusNotFound} } scheme := remote.Scheme if scheme == "" { @@ -40,7 +39,7 @@ func (h *Handler) remoteClusterRequest(remoteID string, req *http.Request) (*htt } saltedReq, err := h.saltAuthToken(req, remoteID) if err != nil { - return nil, nil, err + return nil, err } urlOut := &url.URL{ Scheme: scheme, diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go index f6bfca3021..da640071c5 100644 --- a/lib/controller/federation_test.go +++ b/lib/controller/federation_test.go @@ -594,6 +594,15 @@ func (s *FederationSuite) TestCreateRemoteContainerRequestCheckRuntimeToken(c *c `)) req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveToken) req.Header.Set("Content-type", "application/json") + + np := arvados.NodeProfile{ + Controller: arvados.SystemServiceInstance{Listen: ":"}, + RailsAPI: arvados.SystemServiceInstance{Listen: os.Getenv("ARVADOS_TEST_API_HOST"), + TLS: true, Insecure: true}} + s.testHandler.Cluster.ClusterID = "zzzzz" + s.testHandler.Cluster.NodeProfiles["*"] = np + s.testHandler.NodeProfile = &np + resp := s.testRequest(req) c.Check(resp.StatusCode, check.Equals, http.StatusOK) var cr struct { diff --git a/lib/controller/handler.go b/lib/controller/handler.go index cbfaaddab4..295dde7ca4 100644 --- a/lib/controller/handler.go +++ b/lib/controller/handler.go @@ -50,6 +50,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { req.URL.Path = strings.Replace(req.URL.Path, "//", "/", -1) } } + if h.Cluster.HTTPRequestTimeout > 0 { + ctx, cancel := context.WithDeadline(req.Context(), time.Now().Add(time.Duration(h.Cluster.HTTPRequestTimeout))) + req = req.WithContext(ctx) + defer cancel() + } + h.handlerStack.ServeHTTP(w, req) } @@ -84,8 +90,7 @@ func (h *Handler) setup() { h.insecureClient = &ic h.proxy = &proxy{ - Name: "arvados-controller", - RequestTimeout: time.Duration(h.Cluster.HTTPRequestTimeout), + Name: "arvados-controller", } } @@ -122,10 +127,10 @@ func prepend(next http.Handler, middleware middlewareFunc) http.Handler { }) } -func (h *Handler) localClusterRequest(req *http.Request) (*http.Response, context.CancelFunc, error) { +func (h *Handler) localClusterRequest(req *http.Request) (*http.Response, error) { urlOut, insecure, err := findRailsAPI(h.Cluster, h.NodeProfile) if err != nil { - return nil, nil, err + return nil, err } urlOut = &url.URL{ Scheme: urlOut.Scheme, @@ -142,10 +147,7 @@ func (h *Handler) localClusterRequest(req *http.Request) (*http.Response, contex } func (h *Handler) proxyRailsAPI(w http.ResponseWriter, req *http.Request, next http.Handler) { - resp, cancel, err := h.localClusterRequest(req) - if cancel != nil { - defer cancel() - } + resp, err := h.localClusterRequest(req) n, err := h.proxy.ForwardResponse(w, resp, err) if err != nil { httpserver.Logger(req).WithError(err).WithField("bytesCopied", n).Error("error copying response body") diff --git a/lib/controller/proxy.go b/lib/controller/proxy.go index c89b9b36ae..c01c152352 100644 --- a/lib/controller/proxy.go +++ b/lib/controller/proxy.go @@ -5,18 +5,15 @@ package controller import ( - "context" "io" "net/http" "net/url" - "time" "git.curoverse.com/arvados.git/sdk/go/httpserver" ) type proxy struct { - Name string // to use in Via header - RequestTimeout time.Duration + Name string // to use in Via header } type HTTPError struct { @@ -49,7 +46,7 @@ type ResponseFilter func(*http.Response, error) (*http.Response, error) func (p *proxy) Do( reqIn *http.Request, urlOut *url.URL, - client *http.Client) (*http.Response, context.CancelFunc, error) { + client *http.Client) (*http.Response, error) { // Copy headers from incoming request, then add/replace proxy // headers like Via and X-Forwarded-For. @@ -69,22 +66,16 @@ func (p *proxy) Do( } hdrOut.Add("Via", reqIn.Proto+" arvados-controller") - ctx := reqIn.Context() - var cancel context.CancelFunc - if p.RequestTimeout > 0 { - ctx, cancel = context.WithDeadline(ctx, time.Now().Add(time.Duration(p.RequestTimeout))) - } - reqOut := (&http.Request{ Method: reqIn.Method, URL: urlOut, Host: reqIn.Host, Header: hdrOut, Body: reqIn.Body, - }).WithContext(ctx) + }).WithContext(reqIn.Context()) resp, err := client.Do(reqOut) - return resp, cancel, err + return resp, err } // Copy a response (or error) to the downstream client -- 2.39.5