apache / couchdb

Seamless multi-master syncing database with an intuitive HTTP/JSON API, designed for reliability
https://couchdb.apache.org/
Apache License 2.0
6.25k stars 1.03k forks source link

Allow for proxy provided `nonce` in chttpd #4993

Open chewbranca opened 8 months ago

chewbranca commented 8 months ago

Summary

As mentioned in https://github.com/apache/couchdb/pull/4990, when using a load balancer or proxy in front of CouchDB, the proxy will not receive the chosen nonce value from CouchDB until the response headers are sent, which, in the event of long running _find queries that return no data, can be a considerable amount of time longer than typical proxy timeouts.

The result is that the Mango reports are logged with the nonce value but the proxy timed out the request before getting any response headers so it was never able to retrieve the nonce for logging and connecting the proxy logs with the mango report. This is exacerbated by chttpd:maybe_log typically being set to false when using a proxy alongside a CouchDB cluster.

I was going to suggest modifying chttpd:maybe_log to be configurable based on error types, but @rnewson had a much simpler suggestion of allowing the proxy to provide the nonce, thereby establishing the connection from the get go.

I suggest we stick with the existing naming and allow for a request header named X-Couch-Request-ID, following https://github.com/apache/couchdb/blob/main/src/chttpd/src/chttpd.erl#L1368.

Desired Behaviour

A frontend proxy load balancer to a CouchDB cluster may supply an X-Couch-Request-ID to utilize as the nonce value instead of randomly generating one here: https://github.com/apache/couchdb/blob/main/src/chttpd/src/chttpd.erl#L297-L317

Possible Solution

diff --git a/src/chttpd/src/chttpd.erl b/src/chttpd/src/chttpd.erl
index ab8e1e9a3..6912115c1 100644
--- a/src/chttpd/src/chttpd.erl
+++ b/src/chttpd/src/chttpd.erl
@@ -294,7 +294,12 @@ handle_request_int(MochiReq) ->
             Other -> Other
         end,

-    Nonce = couch_util:to_hex(crypto:strong_rand_bytes(5)),
+    Nonce = case MochiReq:get_header_value("x-couch-request-id") of
+        undefined ->
+            couch_util:to_hex(crypto:strong_rand_bytes(5));
+        Nonce0 ->
+            Nonce0
+    end,

     HttpReq0 = #httpd{
         mochi_req = MochiReq,

Additional context

https://github.com/apache/couchdb/pull/4990

rnewson commented 8 months ago

good idea, but let's protect ourselves from client input. rejecting an x-couch-request-id that's above a certain length, say.

nickva commented 8 months ago

+1 to check the length and maybe also ensure it has only alphanumeric characters, no escapes, slashes, semicolons, etc.

rnewson commented 8 months ago

agreed on those extra checks, nick.

rnewson commented 8 months ago

and a 400 bad request if those checks aren't met, rather than silently ignoring the header or stripping it of invalid chars, etc.