[ARVADOS] created: 2.1.0-1097-g5ae9c613a
Git user
git at public.arvados.org
Thu Jul 22 18:32:42 UTC 2021
at 5ae9c613aadadb918c5cfb1c22e37e16a3a3c7fc (commit)
commit 5ae9c613aadadb918c5cfb1c22e37e16a3a3c7fc
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Thu Jul 22 15:31:11 2021 -0300
17830: Copies request's X-Request-Id header to response. Moves tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index 44c99bf30..bd910aab4 100644
--- a/lib/controller/integration_test.go
+++ b/lib/controller/integration_test.go
@@ -432,6 +432,42 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithBadToken(c *check.C) {
}
}
+func (s *IntegrationSuite) TestRequestIDHeader(c *check.C) {
+ conn1 := s.testClusters["z1111"].Conn()
+ rootctx1, _, _ := s.testClusters["z1111"].RootClients()
+ _, ac1, _, _ := s.testClusters["z1111"].UserClients(rootctx1, c, conn1, "user at example.com", true)
+
+ tests := []struct {
+ path string
+ reqIdProvided bool
+ }{
+ {"/arvados/v1/container_requests", false},
+ {"/arvados/v1/container_requests", true},
+ {"/arvados/v1/links", false},
+ {"/arvados/v1/links", true},
+ }
+
+ for _, tt := range tests {
+ c.Log(c.TestName() + " " + tt.path)
+ req, err := http.NewRequest("GET", "https://"+ac1.APIHost+tt.path, nil)
+ c.Assert(err, check.IsNil)
+ customReqId := "abcdeG"
+ if !tt.reqIdProvided {
+ c.Assert(req.Header.Get("X-Request-Id"), check.Equals, "")
+ } else {
+ req.Header.Set("X-Request-Id", customReqId)
+ }
+ resp, err := ac1.Do(req)
+ c.Assert(err, check.IsNil)
+ c.Check(resp.StatusCode, check.Equals, http.StatusOK)
+ if !tt.reqIdProvided {
+ c.Check(resp.Header.Get("X-Request-Id"), check.Matches, "^req-[0-9a-zA-Z]{20}$")
+ } else {
+ c.Check(resp.Header.Get("X-Request-Id"), check.Equals, customReqId)
+ }
+ }
+}
+
// We test the direct access to the database
// normally an integration test would not have a database access, but in this case we need
// to test tokens that are secret, so there is no API response that will give them back
diff --git a/lib/controller/router/response.go b/lib/controller/router/response.go
index 500fb3071..35a4e5f1b 100644
--- a/lib/controller/router/response.go
+++ b/lib/controller/router/response.go
@@ -57,7 +57,7 @@ func applySelectParam(selectParam []string, orig map[string]interface{}) map[str
return selected
}
-func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp interface{}, opts responseOptions, reqId string) {
+func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp interface{}, opts responseOptions) {
var tmp map[string]interface{}
if resp, ok := resp.(http.Handler); ok {
@@ -67,7 +67,7 @@ func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp i
return
}
- w.Header().Set("X-Request-Id", reqId)
+ w.Header().Set("X-Request-Id", req.Header.Get("X-Request-Id"))
err := rtr.transcode(resp, &tmp)
if err != nil {
rtr.sendError(w, err)
diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go
index 82e81d089..5ceabbfb1 100644
--- a/lib/controller/router/router.go
+++ b/lib/controller/router/router.go
@@ -505,12 +505,7 @@ func (rtr *router) addRoute(endpoint arvados.APIEndpoint, defaultOpts func() int
}
}
ctx := auth.NewContext(req.Context(), creds)
- var reqId string
- if reqId = req.Header.Get("X-Request-Id"); reqId == "" {
- reqIDGen := httpserver.IDGenerator{Prefix: "req-"}
- reqId = reqIDGen.Next()
- }
- ctx = arvados.ContextWithRequestID(ctx, reqId)
+ ctx = arvados.ContextWithRequestID(ctx, req.Header.Get("X-Request-Id"))
logger.WithFields(logrus.Fields{
"apiEndpoint": endpoint,
"apiOptsType": fmt.Sprintf("%T", opts),
@@ -522,7 +517,7 @@ func (rtr *router) addRoute(endpoint arvados.APIEndpoint, defaultOpts func() int
rtr.sendError(w, err)
return
}
- rtr.sendResponse(w, req, resp, respOpts, reqId)
+ rtr.sendResponse(w, req, resp, respOpts)
})
}
diff --git a/lib/controller/router/router_test.go b/lib/controller/router/router_test.go
index 0e0edad31..0330ec425 100644
--- a/lib/controller/router/router_test.go
+++ b/lib/controller/router/router_test.go
@@ -370,35 +370,6 @@ func (s *RouterIntegrationSuite) TestHEAD(c *check.C) {
c.Check(rr.Code, check.Equals, http.StatusOK)
}
-func (s *RouterIntegrationSuite) TestRequestIDHeader(c *check.C) {
- token := arvadostest.ActiveTokenV2
- req := (&testReq{
- method: "GET",
- path: "arvados/v1/collections/" + arvadostest.FooCollection,
- token: token,
- }).Request()
- rr := httptest.NewRecorder()
- s.rtr.ServeHTTP(rr, req)
- c.Check(rr.Code, check.Equals, http.StatusOK)
- c.Check(rr.Result().Header.Get("X-Request-Id"), check.Matches, "^req-[0-9a-zA-Z]{20}$")
-}
-
-func (s *RouterIntegrationSuite) TestRequestIDHeaderProvidedByClient(c *check.C) {
- token := arvadostest.ActiveTokenV2
- req := (&testReq{
- method: "GET",
- path: "arvados/v1/collections/" + arvadostest.FooCollection,
- token: token,
- header: http.Header{
- "X-Request-Id": []string{"abcdeG"},
- },
- }).Request()
- rr := httptest.NewRecorder()
- s.rtr.ServeHTTP(rr, req)
- c.Check(rr.Code, check.Equals, http.StatusOK)
- c.Check(rr.Result().Header.Get("X-Request-Id"), check.Equals, "abcdeG")
-}
-
func (s *RouterIntegrationSuite) TestRouteNotFound(c *check.C) {
token := arvadostest.ActiveTokenV2
req := (&testReq{
commit e940be406403ddad4b60ee91fdf60bf6bbfb1664
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Wed Jul 21 20:51:10 2021 -0300
17830: Sets up X-Request-Id if not provided by client. Adds it to the response.
Uses the IDGenerator to explicitly set one request id if needed, so that it
can return it to the client without the need to dig it out from the railsAPI
response.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/lib/controller/router/response.go b/lib/controller/router/response.go
index 03cdcf18d..500fb3071 100644
--- a/lib/controller/router/response.go
+++ b/lib/controller/router/response.go
@@ -57,7 +57,7 @@ func applySelectParam(selectParam []string, orig map[string]interface{}) map[str
return selected
}
-func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp interface{}, opts responseOptions) {
+func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp interface{}, opts responseOptions, reqId string) {
var tmp map[string]interface{}
if resp, ok := resp.(http.Handler); ok {
@@ -67,6 +67,7 @@ func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp i
return
}
+ w.Header().Set("X-Request-Id", reqId)
err := rtr.transcode(resp, &tmp)
if err != nil {
rtr.sendError(w, err)
diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go
index 5ceabbfb1..82e81d089 100644
--- a/lib/controller/router/router.go
+++ b/lib/controller/router/router.go
@@ -505,7 +505,12 @@ func (rtr *router) addRoute(endpoint arvados.APIEndpoint, defaultOpts func() int
}
}
ctx := auth.NewContext(req.Context(), creds)
- ctx = arvados.ContextWithRequestID(ctx, req.Header.Get("X-Request-Id"))
+ var reqId string
+ if reqId = req.Header.Get("X-Request-Id"); reqId == "" {
+ reqIDGen := httpserver.IDGenerator{Prefix: "req-"}
+ reqId = reqIDGen.Next()
+ }
+ ctx = arvados.ContextWithRequestID(ctx, reqId)
logger.WithFields(logrus.Fields{
"apiEndpoint": endpoint,
"apiOptsType": fmt.Sprintf("%T", opts),
@@ -517,7 +522,7 @@ func (rtr *router) addRoute(endpoint arvados.APIEndpoint, defaultOpts func() int
rtr.sendError(w, err)
return
}
- rtr.sendResponse(w, req, resp, respOpts)
+ rtr.sendResponse(w, req, resp, respOpts, reqId)
})
}
commit 36ddebdcf0ab91bf9b03e7bf18a6d63a2ab90d14
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Jul 20 17:59:58 2021 -0300
17830: Adds controller tests exposing the X-Request-Id header propagation bug.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/lib/controller/router/router_test.go b/lib/controller/router/router_test.go
index 0330ec425..0e0edad31 100644
--- a/lib/controller/router/router_test.go
+++ b/lib/controller/router/router_test.go
@@ -370,6 +370,35 @@ func (s *RouterIntegrationSuite) TestHEAD(c *check.C) {
c.Check(rr.Code, check.Equals, http.StatusOK)
}
+func (s *RouterIntegrationSuite) TestRequestIDHeader(c *check.C) {
+ token := arvadostest.ActiveTokenV2
+ req := (&testReq{
+ method: "GET",
+ path: "arvados/v1/collections/" + arvadostest.FooCollection,
+ token: token,
+ }).Request()
+ rr := httptest.NewRecorder()
+ s.rtr.ServeHTTP(rr, req)
+ c.Check(rr.Code, check.Equals, http.StatusOK)
+ c.Check(rr.Result().Header.Get("X-Request-Id"), check.Matches, "^req-[0-9a-zA-Z]{20}$")
+}
+
+func (s *RouterIntegrationSuite) TestRequestIDHeaderProvidedByClient(c *check.C) {
+ token := arvadostest.ActiveTokenV2
+ req := (&testReq{
+ method: "GET",
+ path: "arvados/v1/collections/" + arvadostest.FooCollection,
+ token: token,
+ header: http.Header{
+ "X-Request-Id": []string{"abcdeG"},
+ },
+ }).Request()
+ rr := httptest.NewRecorder()
+ s.rtr.ServeHTTP(rr, req)
+ c.Check(rr.Code, check.Equals, http.StatusOK)
+ c.Check(rr.Result().Header.Get("X-Request-Id"), check.Equals, "abcdeG")
+}
+
func (s *RouterIntegrationSuite) TestRouteNotFound(c *check.C) {
token := arvadostest.ActiveTokenV2
req := (&testReq{
commit 44f89a6814bc6b2f5bd96e16e5e12d1a0e0aadda
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Jul 20 16:23:36 2021 -0300
17830: Customizes ActionDispatch::RequestId middleware to our needs.
Moves all the request id code to the proper middleware, and updates tests.
As functional tests are "unit tests for controllers", things that happen inside
middlewares need to be tested from integration tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index fc33dde44..c39bdde4b 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -196,7 +196,7 @@ class ApplicationController < ActionController::Base
end
err[:errors] ||= args
err[:errors].map! do |err|
- err += " (" + Thread.current[:request_id] + ")"
+ err += " (#{request.request_id})"
end
err[:error_token] = [Time.now.utc.to_i, "%08x" % rand(16 ** 8)].join("+")
status = err.delete(:status) || 422
@@ -419,17 +419,9 @@ class ApplicationController < ActionController::Base
end
def set_current_request_id
- req_id = request.headers['X-Request-Id']
- if !req_id || req_id.length < 1 || req_id.length > 1024
- # Client-supplied ID is either missing or too long to be
- # considered friendly.
- req_id = "req-" + Random::DEFAULT.rand(2**128).to_s(36)[0..19]
- end
- response.headers['X-Request-Id'] = Thread.current[:request_id] = req_id
- Rails.logger.tagged(req_id) do
+ Rails.logger.tagged(request.request_id) do
yield
end
- Thread.current[:request_id] = nil
end
def append_info_to_payload(payload)
diff --git a/services/api/config/initializers/request_id_middleware.rb b/services/api/config/initializers/request_id_middleware.rb
new file mode 100644
index 000000000..e2158801e
--- /dev/null
+++ b/services/api/config/initializers/request_id_middleware.rb
@@ -0,0 +1,25 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+module CustomRequestId
+ def make_request_id(req_id)
+ if !req_id || req_id.length < 1 || req_id.length > 1024
+ # Client-supplied ID is either missing or too long to be
+ # considered friendly.
+ internal_request_id
+ else
+ req_id
+ end
+ end
+
+ def internal_request_id
+ "req-" + Random::DEFAULT.rand(2**128).to_s(36)[0..19]
+ end
+end
+
+class ActionDispatch::RequestId
+ # Instead of using the default UUID-like format for X-Request-Id headers,
+ # use our own.
+ prepend CustomRequestId
+end
\ No newline at end of file
diff --git a/services/api/test/functional/application_controller_test.rb b/services/api/test/functional/application_controller_test.rb
index 2cfa05444..af7882141 100644
--- a/services/api/test/functional/application_controller_test.rb
+++ b/services/api/test/functional/application_controller_test.rb
@@ -24,9 +24,6 @@ class ApplicationControllerTest < ActionController::TestCase
token_time = token.split('+', 2).first.to_i
assert_operator(token_time, :>=, @start_stamp, "error token too old")
assert_operator(token_time, :<=, now_timestamp, "error token too new")
- json_response['errors'].each do |err|
- assert_match(/req-[a-z0-9]{20}/, err, "X-Request-Id value missing on error message")
- end
end
def check_404(errmsg="Path not found")
@@ -56,28 +53,6 @@ class ApplicationControllerTest < ActionController::TestCase
check_error_token
end
- test "X-Request-Id header" do
- authorize_with :spectator
- get(:index)
- assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
- end
-
- # The response header is the one that gets logged, so this test also
- # ensures we log the ID supplied in the request, if any.
- test "X-Request-Id given by client" do
- authorize_with :spectator
- @request.headers['X-Request-Id'] = 'abcdefG'
- get(:index)
- assert_equal 'abcdefG', response.headers['X-Request-Id']
- end
-
- test "X-Request-Id given by client is ignored if too long" do
- authorize_with :spectator
- @request.headers['X-Request-Id'] = 'abcdefG' * 1000
- get(:index)
- assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
- end
-
['foo', '', 'FALSE', 'TRUE', nil, [true], {a:true}, '"true"'].each do |bogus|
test "bogus boolean parameter #{bogus.inspect} returns error" do
@controller = Arvados::V1::GroupsController.new
diff --git a/services/api/test/integration/errors_test.rb b/services/api/test/integration/errors_test.rb
index 04843adcf..e3224f491 100644
--- a/services/api/test/integration/errors_test.rb
+++ b/services/api/test/integration/errors_test.rb
@@ -30,10 +30,29 @@ class ErrorsTest < ActionDispatch::IntegrationTest
end
end
- test "X-Request-Id header format on non-existant object URL" do
+ test "X-Request-Id header" do
+ get "/", headers: auth(:spectator)
+ assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
+ end
+
+ test "X-Request-Id header on non-existant object URL" do
get "/arvados/v1/container_requests/invalid",
params: {:format => :json}, headers: auth(:active)
assert_response 404
assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
end
+
+ # The response header is the one that gets logged, so this test also
+ # ensures we log the ID supplied in the request, if any.
+ test "X-Request-Id given by client" do
+ get "/", headers: auth(:spectator).merge({'X-Request-Id': 'abcdefG'})
+ assert_equal 'abcdefG', response.headers['X-Request-Id']
+ end
+
+ test "X-Request-Id given by client is ignored if too long" do
+ authorize_with :spectator
+ long_reqId = 'abcdefG' * 1000
+ get "/", headers: auth(:spectator).merge({'X-Request-Id': long_reqId})
+ assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
+ end
end
commit b64a7596b2fb456de13aa885dbe14a817fd8d5b2
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Jul 20 11:26:21 2021 -0300
17830: Adds test exposing incorrect X-Request-Id format on railsAPI.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/services/api/test/integration/errors_test.rb b/services/api/test/integration/errors_test.rb
index d04e38383..04843adcf 100644
--- a/services/api/test/integration/errors_test.rb
+++ b/services/api/test/integration/errors_test.rb
@@ -14,6 +14,7 @@ class ErrorsTest < ActionDispatch::IntegrationTest
assert_nil assigns(:object)
assert_not_nil json_response['errors']
assert_response 404
+ assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
end
end
@@ -28,4 +29,11 @@ class ErrorsTest < ActionDispatch::IntegrationTest
"Unexpected new route: #{route.path.spec}")
end
end
+
+ test "X-Request-Id header format on non-existant object URL" do
+ get "/arvados/v1/container_requests/invalid",
+ params: {:format => :json}, headers: auth(:active)
+ assert_response 404
+ assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list