Fix b3 header

[iilyak]

Overview

The B3 header parsing introduced in #2309 is broken. When b3 header is set the chttpd request is crashing with the following message:

[error] 2020-02-03T21:25:39.049590Z dbcore@172.30.138.101 <0.23004.0> -------- CRASH REPORT Process  (<0.23004.0>) with 0 neighbors crashed with reason: bad argument in call to binary:split("C10B5B9C9F06385D04BD6B3A57971D8C-1AEA26A4FE496E13-0000000000000000", <<"-">>, [global]) at 
   chttpd:get_trace_headers/1(line:1305) 
   <= chttpd:root_span_options/1(line:1271) 
   <= chttpd:start_span/1(line:1253) 
   <= chttpd:before_request/1(line:270) 
   <= chttpd:handle_request_int/1(line:243) 
   <= mochiweb_http:headers/6(line:128) 
   <= proc_lib:init_p_do_apply/3(line:247); 
initial_call: {mochiweb_acceptor,init,['Argument__1','Argument__2',...]}, 
ancestors: [chttpd,chttpd_sup,<0.452.0>], 
message_queue_len: 0, 
messages: [], 
links: [<0.455.0>,#Port<0.16978>], 
dictionary: [{dont_log_request,true},{ctrace_is_enabled,true},{nonce,"7f8a471..."},...], 
trap_exit: false, 
status: running, 
heap_size: 2586, 
stack_size: 27, 
reductions: 4211 

The reason for the crash is obvious. The binary:split/3 is used in the place where we have string as an argument. According to the code in mochiweb_headers.erl the headers are always strings. This PR switches from binary:split/3 to string:split/3. In order to make sure the change is working as expected the test is written. There was a need to adjust some testing plumbing to make test pass. This adjustment was done in the same PR since it is the very first test of this kind and testing adjustment in separate PR would be quite tricky. The adjustment to test plumbing consists of two changes:

• Update httpotion to 3.1.3 --- cherry pick from master
• Support setting base_url in Couch test helper --- this is needed in the context of unit tests when http port is dynamic

Testing recommendations

1. checkout PR
2. run test make exunit apps=chttpd - the test should pass
3. revert 3b9f04db97 locally and run test again - the test should timeout

Related Issues or Pull Requests

• Opentracing pd #2309

Checklist

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

[chewbranca]

It seems like the create_admin function along with some of the other function below would be useful to have in the shared Couch library. Any particular reason to not put these functions into the couch.ex module?

[iilyak]

Few concerns:

• I tried that before [1] and [2]
  • the complain was: we are developing our own framework which people need to learn.
• Adding create_admin to Couch exibit a chicken and egg problem. Couch implements HTTP interface. We cannot use HTTP interface without adding admin first. In context of eunit tests we always cheated and created admin via config manipulation.
• We could consider create_admin an exception and still add it to Couch inspite of it not being HTTP based interface. However it will introduce a confusion. People use Couch module from elixir integration tests and the function wouldn't work in the context of an integration test.

When I was writing this test I considered using Step.User.new(:admin, roles: [:server_admin]). However since pluguble adapters [1] didn't make it to the framework and [2] doesn't have fabric2 support, so I had to do it differently. I followed the advise of the committers who believe that DRY is bad for tests and they should be as self contained as possible.

---

[1]: #2036 - plugable adaptors based approach
[2]: #2039 - fabric only interface

[chewbranca]

Ahhhh ok, thanks for the detailed explanation @iilyak 👍. It's outside the scope of this PR, but I wonder if it would make sense to add a tag like @tag :with_admin like we've done with dbs and what not [1], to inject an admin user context into the test and then we can abstract away whether the admin user was created through config or the http interface. As for framework vs anti DRY, initially with the Elixir suite we tried to avoid making a heavy framework, which I think is worthwhile, but at the same time there's common activities which should be easy to take care of, like creating dbs/docs/users/ddocs/etc.

[1] https://github.com/apache/couchdb/blob/master/test/elixir/test/security_validation_test.exs#L151-L153

[jaydoane]

$ make exunit apps=chttpd
...
  * test Open Tracing should return success with combined b3 header (276.6ms)

Finished in 3.6 seconds
1 test, 0 failures

[ksnavely]

Thanks for this fix Ilya