prime_server
prime_server copied to clipboard
kevinkreiser
Shortcircuiters for CORS Preflight Support
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
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
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
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!
Hello @kevinkreiser!!
I requested another review to the requested changes! I suppose that i should have done it right after i commit the requested changes.
sorry @ldsmoreira ive just been a bit busy. this weekend i will not have much time but perhaps friday or something will work. apologies for the delay
Hello @kevinkreiser! I have a question!
When it comes to preflight requests, the server needs to know what it can accept or not. What can we do?
- Just not support preflight request handle inside prime_server;
- we can provide a config file to define what can be accepted;
- provide support just to the Access-Control-Allow-Methods since we define the allowed methods using the method mask.
What do you think ?
When it comes to preflight requests, the server needs to know what it can accept or not. What can we do?
I assumed we would update prime_serverd so that just like it does for the health check and other options it can take some kind of argument that tells it what methods are accepted, like a comma separated string eg. HEAD,GET,POST,OPTIONS. once it has that then it can reply just fine to the preflight requests right? i think early on we can be very permissive with cors, basically allowing any headers and any origin and just listing out the methods we support.
this is getting very close! there are still some formatting things and i didnt review that the responses for http were exactly to spec but we can get those last after the other structural changes. almost there!
this is getting very close! there are still some formatting things and i didnt review that the responses for http were exactly to spec but we can get those last after the other structural changes. almost there!
Hahaha, you caught me! I was having a bit too much fun experimenting and totally forgot to put everything back in order! I'm pretty happy to getting closer to our goal and looking foward to contribute to more projects that you are involved in (Valhalla, and so on).
I couldnt have had a better teacher in my introduction to open source! Thanks a lot!
no worries ill probably get to this tonight if i have time. sorry for it takign so long but also thank you for taking the time!
after finishing the feedback from above i think the only thing left for us to do is get unit test coverage for all of the changes. let me know when you are ready for that and i'll make some suggestions how to do it!
Hi @kevinkreiser,
I hope you're doing well! I've made changes to implement your suggestions and support preflight requests in the right manner. Although there's room for improvement, we're making progress. I'd appreciate your feedback on the current state.
Thank you!
@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
going to work on some unit tests for the new functionality so we can get this merged
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!
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: https://github.com/kevinkreiser/prime_server/blob/eaa396342417323d1a80fc649ddc66cc5a221dbc/test/http.cpp#L421-L443
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
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:
https://github.com/kevinkreiser/prime_server/blob/eaa396342417323d1a80fc649ddc66cc5a221dbc/test/http.cpp#L421-L443
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!
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 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!
https://github.com/kevinkreiser/prime_server/blob/eaa396342417323d1a80fc649ddc66cc5a221dbc/test/http.cpp#L514
https://github.com/kevinkreiser/prime_server/blob/eaa396342417323d1a80fc649ddc66cc5a221dbc/test/http.cpp#L300-L309
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
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?
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 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.
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
dequeuegot. 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?
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.
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!!