[ARVADOS] created: 2.1.0-1061-ge57008566
Git user
git at public.arvados.org
Wed Jul 21 23:54:29 UTC 2021
at e57008566eac596ce410af361ed80060e7cbf58f (commit)
commit e57008566eac596ce410af361ed80060e7cbf58f
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 b543f54c8c38fd7ab3ba69650012554b8f003c4f
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 705bb88912403fb9af9484744be213b355a3b9a2
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/application.rb b/services/api/config/application.rb
index 316873f11..b28ae0e07 100644
--- a/services/api/config/application.rb
+++ b/services/api/config/application.rb
@@ -107,8 +107,5 @@ module Server
require Rails.root.join('lib/safer_file_store')
config.cache_store = ::SaferFileStore.new(default_cache_path)
end
-
- # We already implement our own X-Request-Id header (See: #17830)
- config.middleware.delete ActionDispatch::RequestId
end
end
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 df2ba5fa425a9d8013ad7f26c52a7f44a2269541
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Tue Jul 20 12:22:27 2021 -0300
17830: Disables standard rails middleware to avoid conflict with our code.
We've implemented the X-Request-Id header in a different way, the middleware
ActionDispatch::RequestId from Raild 5.0 onwards doesn't seem to support
customization.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/services/api/config/application.rb b/services/api/config/application.rb
index b28ae0e07..316873f11 100644
--- a/services/api/config/application.rb
+++ b/services/api/config/application.rb
@@ -107,5 +107,8 @@ module Server
require Rails.root.join('lib/safer_file_store')
config.cache_store = ::SaferFileStore.new(default_cache_path)
end
+
+ # We already implement our own X-Request-Id header (See: #17830)
+ config.middleware.delete ActionDispatch::RequestId
end
end
commit 8265d2eb51fdd720b5a8dd14b752008d780b8325
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