Shortcircuiters for CORS Preflight Support #118

ldsmoreira · 2023-02-20T22:03:25Z

Hello everyone!

I hope you are doing well. I would like to open this pull request to show you the progress I have made so far. I named it "Work In Progress" since there are still unit tests to be added.

I would appreciate it if you could take a look and let me know if I am going in the right direction. I am open to feedback on how to improve the code to match your standards.

Thank you for your time and consideration.

PS.: There is some files related to my development environment.
PS2.: This pull request is related to the valhalla issue #117

kevinkreiser · 2023-02-20T23:08:38Z

.vscode/settings.json

@@ -0,0 +1,72 @@
+{


please remove this directory. to avoid committing it in the future you could consider adding it to .gitignore

you have to actually remove this directory from git though. git rm -rf .vscode and then commit that. this will also delete the contents of the directory so you should back it up first if you dont want to regenerate it with your IDE

kevinkreiser · 2023-02-20T23:08:47Z

ar-lib

@@ -0,0 +1,271 @@
+#! /bin/sh


this should also be removed

I put it on .gitignore to avoid futher commits involving it. Is it ok ?!

kevinkreiser · 2023-02-20T23:09:20Z

please also remove prime_testd

kevinkreiser · 2023-02-20T23:09:47Z

rebuild.sh

@@ -0,0 +1,12 @@
+# dont forget submodules


please remove

you must git rm this file and commit to actually remove it

also delete that binary file prime_testd

src/prime_testd.cpp

kevinkreiser · 2023-02-20T23:12:40Z

ive made some small comments so far and i am traveling a lot this week but by the end of the week i should be able to get you some good review comments

src/http_protocol.cpp

prime_server/http_protocol.hpp

kevinkreiser · 2023-02-20T23:31:32Z

prime_server/prime_server.hpp

@@ -59,8 +60,27 @@ constexpr uint32_t DEFAULT_REQUEST_TIMEOUT = -1;              // infinity second
 // TODO: bundle both request_containter_t (req, rep) and request_info_t into
 // a single session_t that implements all the guts of the protocol

+// Struct that stores the a vector of shortcircuiter functions and its priorities
+template <class request_container_t>
+struct shortcircuiters_t {


i dont think we need a struct to encapsulate this idea. at least not in the public (header) interface. remember that in public interfaces we want to expose as little as possible. the more we put here the more we have to maintain.

all we need here is a single type alias for the functor:

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

then its up to any protocol to provide that optionally. so over in each protocols header you'd make a free function to return the shortcircuiter_t object. each protocol can have different arguments to this shortcircuiter generator function. http will want healthcheck path and options mask (with defaults) whereas netstring will just want healthcheck.

then you can hide the actual implementation of this inside of http_protocol.cpp and netstring_protocol.cpp respectively.

no need for keeping structs around or a complex interface. you just call a function from each protocol that returns a lambda that does the work. you dont even need vectors of shortcircuiters. each protocol just has their priority list hardcoded i suppose (probably makes sense since protocols have a protocol hahaha).

this might be a bit of an advance c++ pattern and im not sure how familiar you are with that so please let me know if any of this is making sense. if not i can write some sample code to show what i mean.

I think with my last few commits i'm closer to that idea!!

kevinkreiser · 2023-02-20T23:32:18Z

by and large, you are in the right direction for sure, we'll get it cleaned up and get it working. once we like the design we can add unit tests

ldsmoreira · 2023-02-20T23:45:14Z

Thank you very much for the enlightening answer as always!
This week I'm going to go through your comments point by point!
I confess that I'm not so familiar with this C++ pattern, even C++ itself I don't have that much experience, but I don't lack the desire to learn and contribute!

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

src/http_protocol.cpp

prime_server/prime_server.hpp

src/http_protocol.cpp

kevinkreiser · 2023-04-17T13:44:49Z

@ldsmoreira i will finish the review tonight. hopefully just a few suggestions to the cors handling and then we are done. i think we can forget about the configuration stuff for now its not hugely important except for how we collect that info on the command line. i'll make a suggestion about that hopefully in the review too. anyway i have to do it later tonight

kevinkreiser · 2023-04-18T11:22:26Z

going to work on some unit tests for the new functionality so we can get this merged

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

ldsmoreira · 2023-04-26T22:52:13Z

going to work on some unit tests for the new functionality so we can get this merged

@kevinkreiser Next week I will be able to focus an entire day to adjust some details and make the unit tests :) I can't wait for this merge!

kevinkreiser · 2023-04-27T01:54:37Z

if you are looking for examples, i think you could make a unit test for each scenario of the cors preflight shortcutting logic. each test case should look similar to this:

prime_server/test/http.cpp

Lines 421 to 443 in eaa3963

    
           void test_health_check() { 
        
             zmq::context_t context; 
        
             auto request = http_request_t{GET, "/health_check"}.to_string(); 
        
             http_client_t client( 
        
                 context, "ipc:///tmp/test_http_server", 
        
                 [&request]() { 
        
                   return std::make_pair(static_cast<const void*>(request.c_str()), request.size()); 
        
                 }, 
        
                 [](const void* data, size_t size) { 
        
                   auto response = http_response_t::from_string(static_cast<const char*>(data), size); 
        
                   if (response.code != 200) 
        
                     throw std::runtime_error("Expected 200 response code!"); 
        
                   if (response.message != "OK") 
        
                     throw std::runtime_error("Expected OK message!"); 
        
                   if (response.body != "foo_bar_baz") 
        
                     throw std::runtime_error("Expected foo_bar_baz body!"); 
        
                   return false; 
        
                 }, 
        
                 1); 
        
             client.batch(); 
        
             // TODO: check that you're disconnected 
        
           }

where you set up a request and then you have the response handler assert the correct response. i expect you should have at least 3 new tests there for 304 and for different header configurations

ldsmoreira · 2023-05-01T18:09:40Z

if you are looking for examples, i think you could make a unit test for each scenario of the cors preflight shortcutting logic. each test case should look similar to this:

prime_server/test/http.cpp

Lines 421 to 443 in eaa3963

void test_health_check() {

zmq::context_t context;

auto request = http_request_t{GET, "/health_check"}.to_string();

http_client_t client(

context, "ipc:///tmp/test_http_server",

[&request]() {

return std::make_pair(static_cast<const void*>(request.c_str()), request.size());

},

[](const void* data, size_t size) {

auto response = http_response_t::from_string(static_cast<const char*>(data), size);

if (response.code != 200)

throw std::runtime_error("Expected 200 response code!");

if (response.message != "OK")

throw std::runtime_error("Expected OK message!");

if (response.body != "foo_bar_baz")

throw std::runtime_error("Expected foo_bar_baz body!");

return false;

},

1);

client.batch();

// TODO: check that you're disconnected

}

where you set up a request and then you have the response handler assert the correct response. i expect you should have at least 3 new tests there for 304 and for different header configurations

Hello @kevinkreiser,

I'm trying to understand the unit testing logic in the context of prime_server, as I'm relatively new to unit testing in C++. I have a few questions that I hope you can help clarify:

1 - In the example you provided earlier, it seems that a client is connecting to the ipc:///tmp/test_http_server endpoint and receiving responses. Is there a server running specifically for testing purposes in this case, or is the communication being simulated in some other way? If so, could you please explain how that's achieved?

2 - Regarding the shortcircuiter behavior, which is coupled with a server instance, would I need to start multiple servers with different configurations to test all possible scenarios?

If you could share any resources or insights about the testing architecture being used in prime_server, I would greatly appreciate it! I apologize in advance for my limited knowledge in this area, and I'm looking forward to learning more.

Thank you!

kevinkreiser · 2023-05-01T21:42:12Z

yep happy to help @ldsmoreira

regarding question number 1 above, yep exactly, we start an http server at the beginning of the test just for this purpose of testing

regarding question number 2 above, yep you will need to. i think you you might need to do about 4 different test cases: no cors allowed, all cors allowed, specific cors allowed, wrong cors not allowed. the already running server could be configured for testing maybe the last two since they are both supported by the same server config but that would still mean you need to run two more servers. to test the all and no cors variants.

it is possible to test this stuff in a weaker way (not end to end) by calling methods on the server class directly. you see at the top of the test file we make a testable http server with an exposed enqueue method. you could call this directly but you'd need to be able to see what dequeue got. to do this, you could similarly override that method and have it save some state about what it was called with that your test can assert on.

the former is more of a pain in the butt but it does an end to end test while the latter is quite a lot less annoying to deal with. let me know what you think!

ldsmoreira · 2023-05-01T22:05:50Z

@kevinkreiser I initially thought that the line below was responsible for instantiating the server, but interestingly, the tests still passed even after I commented it out hahah. I believe I just need some guidance to locate where the server configuration and startup are defined so that I can better understand the entire process!

prime_server/test/http.cpp

Line 514 in eaa3963

suite.test(TEST_CASE(test_streaming_server));

kevinkreiser · 2023-05-02T01:18:20Z

prime_server/test/http.cpp

Lines 300 to 309 in eaa3963

    
           // server 
        
           std::thread server( 
        
               std::bind(&http_server_t::serve, 
        
                         http_server_t( 
        
                             context, "ipc:///tmp/test_http_server", "ipc:///tmp/test_http_proxy_upstream", 
        
                             "ipc:///tmp/test_http_results", "ipc:///tmp/test_http_interrupt", false, 
        
                             MAX_REQUEST_SIZE, -1, 
        
                             [](const http_request_t& r) -> bool { return r.path == "/health_check"; }, 
        
                             http_response_t{200, "OK", "foo_bar_baz"}.to_string()))); 
        
           server.detach();

that is where the server is started, you can see at the bottom the tests are broken into two groups, ones that dont rely on an actual running server and ones that do, the first one of the ones that do starts the server. given the annoyance of testing this way it might be better to go with the other option of expanding the testable http server and capturing what comes out of dequeue

ldsmoreira · 2023-05-14T17:50:39Z

Hello @kevinkreiser,

I'm currently examining a piece of code that you've merged into the master branch and I've come across an unusual behavior that I hope you could help me understand.

https://github.com/ldsmoreira/prime_server/blob/3f2c7413b5d3e710a52de144f51988cbf8764ddb/test/http.cpp#L255-L284

What's intriguing is that when I input the shortcircuit parameter at server, the application throws a "host unreachable" error and the test fails. However, when I run the application such as prime_serverd, it is working as expected!

Would you have any insights into why this might be happening?

kevinkreiser · 2023-05-19T04:37:27Z

i think its happening because in this type of testing we dont actually connect a client that the server can send the shortcircuit reply to. maybe the right thing for me to do in my previous pr was to catch the exception in my testing dequeue function, assert that its the right kind of expected exception and swallow it. if i catch somethign that isnt the right exception then i throw it again. i think that sounds right.. i can pr that

ldsmoreira · 2023-05-20T01:37:05Z

i think its happening because in this type of testing we dont actually connect a client that the server can send the shortcircuit reply to. maybe the right thing for me to do in my previous pr was to catch the exception in my testing dequeue function, assert that its the right kind of expected exception and swallow it. if i catch somethign that isnt the right exception then i throw it again. i think that sounds right.. i can pr that

I will take another look later to completely understand this flow.

ldsmoreira · 2023-05-21T20:53:27Z

yep happy to help @ldsmoreira

regarding question number 1 above, yep exactly, we start an http server at the beginning of the test just for this purpose of testing

regarding question number 2 above, yep you will need to. i think you you might need to do about 4 different test cases: no cors allowed, all cors allowed, specific cors allowed, wrong cors not allowed. the already running server could be configured for testing maybe the last two since they are both supported by the same server config but that would still mean you need to run two more servers. to test the all and no cors variants.

it is possible to test this stuff in a weaker way (not end to end) by calling methods on the server class directly. you see at the top of the test file we make a testable http server with an exposed enqueue method. you could call this directly but you'd need to be able to see what dequeue got. to do this, you could similarly override that method and have it save some state about what it was called with that your test can assert on.

the former is more of a pain in the butt but it does an end to end test while the latter is quite a lot less annoying to deal with. let me know what you think!

@kevinkreiser, I have just committed some tests. I believe I'm heading in the right direction.

P.S. Should we restrict CORS outside of the preflight request context? Specifically, should we block all requests originating from sources not on our whitelist and using methods other than OPTIONS?

kevinkreiser · 2023-05-23T12:59:21Z

yep i think for me the only thing we need to worry about at this point is that the behavior is as it was before so that people who are using this will not have to make a non obvious code change to upgrade. i think we should default to allowing all origins (meaning we dont even set the shortcircuiter, which i think is what we have by default).

to your specific question, the way i understood the spec, if the preflight origin is not in the whitelist we should return a 403 and none of the access-control headers.

ldsmoreira · 2023-06-16T09:51:44Z

Hello @kevinkreiser , I apologize for not being able to work here for the past couple of weeks. I'm looking forward to finishing it this weekend!!

Ubuntu and others added 11 commits February 9, 2023 23:51

trying to understaand enqueue behavior

f23fed1

Reproduce the same behavior of health_check to shortcircuit

53c7af5

Implementing logic suggested by kk

e36b8d5

make shortcircuit return a pair

c21f12d

Turning shortcircuiting logic less verbose

26c6e1d

Shortcircuit generalization

92db76f

Implementing struct that hold the shortcircuiters to rule them all

3eb4415

Adding alias to the big def std:function<...>

4c0b566

http_healthcheck implementation

155b418

verb_mask and semi dynamics OPTIONS response implemented

f6e19cd

Suport to netsrig shortcircuits added

34a54eb

kevinkreiser reviewed Feb 20, 2023

View reviewed changes

src/prime_testd.cpp Outdated Show resolved Hide resolved

kevinkreiser reviewed Feb 20, 2023

View reviewed changes

src/http_protocol.cpp Outdated Show resolved Hide resolved

kevinkreiser reviewed Feb 20, 2023

View reviewed changes

prime_server/http_protocol.hpp Outdated Show resolved Hide resolved

kevinkreiser reviewed Feb 20, 2023

View reviewed changes

prime_server/http_protocol.hpp Outdated Show resolved Hide resolved

kevinkreiser requested changes Feb 20, 2023

View reviewed changes

ldsmoreira and others added 7 commits February 22, 2023 13:02

Ignore .vscode/

2120828

Removing unnecessary files

3da756b

Change enum to enum class to map verb_mask to verb

1c1ebcf

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

Removing hard coded http verb map

837771f

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

Make is_method_allowed simpler

4e48810

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

Refactorig to use a functor as server_t parameter

4d7eed0

Adding support to netstring shortcircuiting

7d954ea

ldsmoreira requested a review from kevinkreiser April 7, 2023 13:51

kevinkreiser reviewed Apr 17, 2023

View reviewed changes

src/http_protocol.cpp Outdated Show resolved Hide resolved

kevinkreiser reviewed Apr 17, 2023

View reviewed changes

prime_server/prime_server.hpp Outdated Show resolved Hide resolved

kevinkreiser reviewed Apr 17, 2023

View reviewed changes

src/http_protocol.cpp Outdated Show resolved Hide resolved

ldsmoreira and others added 3 commits April 26, 2023 19:49

Update src/http_protocol.cpp

fef67f2

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

Update src/http_protocol.cpp

fd1f149

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

Update prime_server/prime_server.hpp

41b4527

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

kevinkreiser mentioned this pull request May 8, 2023

add sample cors test #123

Merged

ldsmoreira added 2 commits May 14, 2023 15:23

Merge branch 'master' into shortcircuiters

79194ea

Host unreachable error

3f2c741

kevinkreiser mentioned this pull request May 19, 2023

http unit test, allow server test without connected client #126

Merged

ldsmoreira added 2 commits May 21, 2023 20:42

Merge branch 'master' into shortcircuiters

ef894a0

Adding tests for shortcircuiters

6be4e3d

ldsmoreira requested a review from kevinkreiser June 16, 2023 09:49

kevinkreiser changed the title ~~Shortcircuiters~~ Shortcircuiters for CORS Preflight Support Oct 12, 2023

kevinkreiser mentioned this pull request Oct 12, 2023

Can Valhalla service supports the OPTIONS requests (pre-flight requests)? valhalla/valhalla#4328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shortcircuiters for CORS Preflight Support #118

Shortcircuiters for CORS Preflight Support #118

ldsmoreira commented Feb 20, 2023 •

edited

Loading

kevinkreiser Feb 20, 2023

ldsmoreira Feb 22, 2023

kevinkreiser Mar 1, 2023

kevinkreiser Feb 20, 2023

ldsmoreira Feb 22, 2023

kevinkreiser commented Feb 20, 2023

kevinkreiser Feb 20, 2023

ldsmoreira Feb 22, 2023

kevinkreiser Mar 1, 2023

kevinkreiser Mar 1, 2023

kevinkreiser commented Feb 20, 2023

kevinkreiser Feb 20, 2023

ldsmoreira Feb 22, 2023

kevinkreiser commented Feb 20, 2023

ldsmoreira commented Feb 20, 2023

kevinkreiser commented Apr 17, 2023

kevinkreiser commented Apr 18, 2023

ldsmoreira commented Apr 26, 2023 •

edited

Loading

kevinkreiser commented Apr 27, 2023

ldsmoreira commented May 1, 2023

kevinkreiser commented May 1, 2023

ldsmoreira commented May 1, 2023 •

edited

Loading

kevinkreiser commented May 2, 2023

ldsmoreira commented May 14, 2023 •

edited

Loading

kevinkreiser commented May 19, 2023

ldsmoreira commented May 20, 2023

ldsmoreira commented May 21, 2023

kevinkreiser commented May 23, 2023

ldsmoreira commented Jun 16, 2023

Shortcircuiters for CORS Preflight Support #118

Are you sure you want to change the base?

Shortcircuiters for CORS Preflight Support #118

Conversation

ldsmoreira commented Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Feb 20, 2023

ldsmoreira commented Feb 20, 2023

kevinkreiser commented Apr 17, 2023

kevinkreiser commented Apr 18, 2023

ldsmoreira commented Apr 26, 2023 • edited Loading

kevinkreiser commented Apr 27, 2023

ldsmoreira commented May 1, 2023

kevinkreiser commented May 1, 2023

ldsmoreira commented May 1, 2023 • edited Loading

kevinkreiser commented May 2, 2023

ldsmoreira commented May 14, 2023 • edited Loading

kevinkreiser commented May 19, 2023

ldsmoreira commented May 20, 2023

ldsmoreira commented May 21, 2023

kevinkreiser commented May 23, 2023

ldsmoreira commented Jun 16, 2023

ldsmoreira commented Feb 20, 2023 •

edited

Loading

ldsmoreira commented Apr 26, 2023 •

edited

Loading

ldsmoreira commented May 1, 2023 •

edited

Loading

ldsmoreira commented May 14, 2023 •

edited

Loading