[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