[Suggestion] New configuration option to whitelist some or all (default) http verbs

[ldsmoreira]

As we are discussing here, i am pretty much interested in start contributing to those open source projects (prime_server and valhalla).

As mentioned by @kevinkreiser the points to cover would be:

• Implement a new configuration option to whitelist some or all (default) http verbs;
• Add that configuration to all binaries;
• Add unity tests;
• Make a release.

To start the first task, what would be the files that i need to put more focus to understand?

Should that configuration be passed to the server constructor like as follows ?

http_server_t server(context, server_endpoint, proxy_endpoint, server_result_loopback, server_request_interrupt, log, max_request_size_bytes, request_timeout_seconds, health_check_matcher, health_check_response, <NEW_CONFIG>);

[kevinkreiser]

ok its a bit more complicated unfortunately because the thing you want to add is specific to the http protocol.

you see prime_server was not only designed to be a general purpose api for you to create a service, its also meant to be protocol agnostic. what that means is that anyone can make their own protocol and still use the api to write their application. you'll notice that the way this work is with templates. the http server for example doesnt really exist as an object, its just the server_t object with the templates filled in by http specific protocol handling structs, eg. https://github.com/kevinkreiser/prime_server/blob/master/prime_server/http_protocol.hpp#L191

anyway, all you need to implement your protocol is to define a means by which you can parse requests from bytes that come over the socket. at the moment prime_server provides both http 1.1 (partial) and netstring protocol implementations.

you'll see if you look at server_t that its got all these generic server options, that you listed above:

https://github.com/kevinkreiser/prime_server/blob/master/prime_server/prime_server.hpp#L75-L79

most of these are generic enough to be relevant for all protocols and so they are implemented in server_t itself. the second to last works by dependency injection where you pass a functor to inject functionality into the struct and the last one is just a canned response which again is protocol agnostic.

it seems what we need for this to work is more of a new concept.. in this case we need a way of injecting protocol specific logic, similar to the dependency injection approach for the health_check_matcher. so i think we need to go that route.

what im thinking is that we can add a function that does "shortcircuiting". what i mean by that is that this function would be called on each incoming request and if it deems the request shortcircuitable, ie it shouldnt be forwarded on to be worked on but rather responded to right away, then it will return a response and indeed return the response. if the request is to be forwarded on and worked on by downstream custom logic (workers) then it should return nothing. we could use unique_ptr for this, or my obvious, move to c++17 and use std::optional. at any rate the additional parameter would be like:

  using shortcircuiter_t = std::function<std::unique_ptr<zmq::message_t>(const request_container_t&)>;

  server_t(zmq::context_t& context,
           const std::string& client_endpoint,
           const std::string& proxy_endpoint,
           const std::string& result_endpoint,
           const std::string& interrupt_endpoint,
           bool log = false,
           size_t max_request_size = DEFAULT_MAX_REQUEST_SIZE,
           uint32_t request_timeout = DEFAULT_REQUEST_TIMEOUT,
           const health_check_matcher_t& health_check_matcher = {},
           const std::string& health_check_response = {},
           const shortcircuiter_t& shortcircuiter = {});

then over in http_protocol.hpp we'd make something like:

  struct http_options_shortcircuiter_t {
    http_options_shortcircuiter_t(uint8_t verb_mask = std::numeric_limits<uint8_t>::max());
    std::unique_ptr<zmq::message_t> operator()(const request_container_t&)const;
  };

and finally over in prime_httpd.cc we can instantiate one of these and pass it in to be used by the server

  http_options_shortcuircuiter_t shortcircuiter(/*command line options determine this mask*/);
  auto shortcircuit = std::bind(&http_options_shortcuircuiter_t::operator(), shortcircuiter, _1);
  ...
  http_server_t server(context, server_endpoint, proxy_endpoint, server_result_loopback,
                       server_request_interrupt, log, max_request_size_bytes, request_timeout_seconds,
                       health_check_matcher, health_check_response, shortcircuit);

looking at this again, it seems unfortunate now that we didnt design the health check stuff this way. seems like we should make that refactor too and use this proposed design, then both can work that way. basically we have one shortcircuiter (health check is one as well) and what you can have is a priority order of shortcircuiters or a single one that implements them all...

of course i also didnt cover at all the implementation of the actual method, nor the changes to server_t that would call into it. but before we get further first let me know if what im saying at all makes sense.

i could make a branch with an outline of the stuff im explaining, so that its a bit more concrete and then perhaps you could fork and make a pr to the branch to fill in the actual work? let me know on how you'd like to best start or if we should try to clear it up and design it a bit more first

[ldsmoreira]

Yeh, i think i got it!! To be complete honest i will need to study all the tihings you mentioned.

I think it is a very good ideia to discuss my implementations here and get your thoughts on it too (I think its not necessary for now the branch creation! [I don't want to annoy you and abuse your willingness to help.])

Let me show what i was doing!

1. Implement a method called foo in each protocol, but the only one that actually have some logic is the http one, as follows:

http_request_t http_request_t::foo() const{
  headers_t headers = this->headers;
  headers.emplace("Access-Control-Allow-Methods", "GET, POST, OPTIONS");
  if(method == method_t::OPTIONS)
    return http_request_t(method_t::OPTIONS, path, body, query, headers, version);
  else{
    http_request_t copy = *this;
    return copy;
  }
}

2. Added a call to that method in bool server_t<request_container_t, request_info_t>::enqueue as follows:

request_container_t foozed_parsed_request = parsed_request.foo();
    // send on the request if its not a health check
    if (!health_check &&
        (!proxy.send(static_cast<const void*>(&info), sizeof(info), ZMQ_DONTWAIT | ZMQ_SNDMORE) ||
         !proxy.send(foozed_parsed_request.to_string(), ZMQ_DONTWAIT))) {
      logging::ERROR("Server failed to enqueue request");
      return false;
    }

I think what i was doing is some kind like a shortcircuit, let me know if i'm in the right way! Yesterday i managed to put it to work as expected!

If that logic is kind of right, this week i will go deeper into your thoughts and try to implement something like you are expecting!

Last, but the most important I want to thank you for your support and kindness.

[ldsmoreira]

Forget my last comment, I was confused about some points.

Now i'm implementing the same logic as health_check to make it works, and then i will advance in the suggested design.

For example:

server_t(zmq::context_t& context,
           const std::string& client_endpoint,
           const std::string& proxy_endpoint,
           const std::string& result_endpoint,
           const std::string& interrupt_endpoint,
           bool log = false,
           size_t max_request_size = DEFAULT_MAX_REQUEST_SIZE,
           uint32_t request_timeout = DEFAULT_REQUEST_TIMEOUT,
           const health_check_matcher_t& health_check_matcher = {},
           const std::string& health_check_response = {},
           const shortcircuit_matcher_t& shortcircuit_matcher = {},
           const std::string& shortcircuit_response = {});

And at enqueue something like that:

    if (health_check)
      dequeue(request_history.back(), health_check_response);
    if (shortcircuit)
      dequeue(request_history.back(), shortcircuit_response);

[kevinkreiser]

yep that will work, you are on the right track now.

however there is a problem with doing it "exactly" this way. it might be hard to understand the way i was explaining it above, apologies for that, but i was suggesting refactoring the way i did the health check. that refactor is, im fairly certain, necessary for us to properly implement a response to an OPTIONS request.

so for me i think server should endup looking like:

// server just takes a shortcutter interface and it can do both health check and OPTIONS
server_t(zmq::context_t& context,
           const std::string& client_endpoint,
           const std::string& proxy_endpoint,
           const std::string& result_endpoint,
           const std::string& interrupt_endpoint,
           bool log = false,
           size_t max_request_size = DEFAULT_MAX_REQUEST_SIZE,
           uint32_t request_timeout = DEFAULT_REQUEST_TIMEOUT,
           const shortcircuiter_t& shortcircuiter = {});

then the shortcircuiter can take a request and optionally return a response, so its interface would look like this:

// shortcircuiter will return a response if the request is one it should shortcircuit otherwise it returns nullptr
using shortcircuiter_t = std::function<std::unique_ptr<zmq::message_t> (const request_container_t&)>;

then over in the server implementation we would change it to something like:

<     // if its enabled, see if its a health check
<     bool health_check = health_check_matcher && health_check_matcher(parsed_request);
---
>     // if its enabled, see if its a request we can shortcircuit
>     auto shortcircuited = shortcircuiter ? shortcircuiter(parsed_request) : nullptr;
32,33c32,33
<     // send on the request if its not a health check
<     if (!health_check &&
---
>     // send on the request if its not shortcircuited
>     if (!shortcircuited &&
47,49c47,49
<     // if it was a health check we reply immediately
<     if (health_check)
<       dequeue(request_history.back(), health_check_response);
---
>     // if it was a request for which we can reply immediately
>     if (shortcircuited)
>       dequeue(request_history.back(), *shortcircuited);

then all we have to do is add a "shortcircuiter" interface to the http and netstring protocols which will do any shortcircuiting we need, in the case of netstring its just the health check, but in the case of http its OPTIONS and health check

let me know if this makes sense. oh and i almost forgot! above when i was saying we have to do this refactor, the reason we have to do that is because the OPTIONS response cannot be static content. have a look here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS you'll see that to properly handle cors preflight requests you need to actually look at the headers the request has and then echo them back in the response headers. so basically we have to generate the response every time we can't "pre-render" the response bytes and just send them (which is how the health check works)

[ldsmoreira]

For sure it makes sense, i forked the project and im working on the branch shortcircuiters.

I was doing something similar

    std::pair<std::unique_ptr<zmq::message_t>, bool> shortcircuited_request;

    if(shortcircuiter){
      shortcircuited_request = shortcircuiter(parsed_request);
    }else{
      shortcircuited_request = std::make_pair(nullptr, false);
    }

    bool need_shortcircuit = shortcircuited_request.second;

And something like that in the implementation of the http_protocol.cpp

std::pair<std::unique_ptr<zmq::message_t>, bool>http_options_shortcircuiter_t::operator() (const http_request_t& request) const{
  if(request.method == method_t::OPTIONS) {
    http_response_t response(200, "OK", "", headers_t{{"Access-Control-Allow-Origin", "*"},
                                                      {"Access-Control-Allow-Methods", "GET, POST, OPTIONS"},
                                                      {"Access-Control-Allow-Headers", "Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization"},
                                                      {"Access-Control-Allow-Credentials", "true"}});
                                                      
    std::unique_ptr<zmq::message_t> response_msg(std::make_unique<zmq::message_t>(response.to_string().length(), response.to_string().c_str()));
    return std::make_pair(std::move(response_msg), true);
  }
  std::unique_ptr<zmq::message_t> response_msg;
  return std::make_pair(std::move(response_msg), false);
}

Dont pay attention on how im making the response with the fixed headers, i will turn into dynamic as a next step kkk. I will follow your past suggestions, the way that i did was working as "expected" and wasnt breaking the other binary but the way that you suggested is less verbose!

After changing it, i will refactor the health_check logic too as suggested!

I have no words to thank you for such kind approach and attention given to me as a begginer in open source contribution!

Edit1: I didnt forget about the logic of verb_mask, i will address it in the future

Edit2: I thought that a good way to generalize the shortcircuiters is to pass them to a vector and iterate over that vector, the order of importance would be given by the position of the shortcircuiter in the vector.