<div dir="auto"><span style="color:rgb(29,28,29);font-family:slack-lato,applelogo,sans-serif;font-size:15px">Ed,</span><span style="display:block;height:8px;color:rgb(29,28,29);font-family:slack-lato,applelogo,sans-serif;font-size:15px"></span><span style="color:rgb(29,28,29);font-family:slack-lato,applelogo,sans-serif;font-size:15px">Sorry for the late response, I forgot to reply here, Some how the first mail got missed but once I got a  reminder mail, I started working on this and the discussion is going on in the gerrit.</span><br style="color:rgb(29,28,29);font-family:slack-lato,applelogo,sans-serif;font-size:15px"><span style="color:rgb(29,28,29);font-family:slack-lato,applelogo,sans-serif;font-size:15px">I had few more doubts which I asked in the gerrit, Can you please take a look?</span><div style="color:rgb(136,136,136);font-family:sans-serif;font-size:12.8px" dir="auto"><span style="display:block;height:8px;color:rgb(29,28,29);font-family:'slack-lato','applelogo',sans-serif;font-size:15px"><br><br></span><span style="color:rgb(29,28,29);font-family:'slack-lato','applelogo',sans-serif;font-size:15px"><br>-RaviTeja</span> </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 28 Aug, 2020, 12:42 am Ed Tanous, <<a href="mailto:ed@tanous.net">ed@tanous.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <<a href="mailto:ed@tanous.net" target="_blank" rel="noreferrer">ed@tanous.net</a>> wrote:<br>
><br>
> I'm looking at a couple recent changes to bmcweb, and I'm finding a<br>
> significant architecture problem has been injected.  Namely, it's<br>
> these innocuous looking 4 lines here, which injects the socket adaptor<br>
> into the request object for use later.<br>
> <a href="https://github.com/openbmc/bmcweb/blob/30c58d581606b4484757e6ee9133c248de1514a6/http/http_request.h#L18" rel="noreferrer noreferrer" target="_blank">https://github.com/openbmc/bmcweb/blob/30c58d581606b4484757e6ee9133c248de1514a6/http/http_request.h#L18</a><br>
><br>
> The problem with this approach has a few roots:<br>
> 1. The Request class is meant to model a single request, single<br>
> response model.  Adding the stream semantics breaks this in pretty<br>
> significant ways, and forces a hard dependency between the streaming<br>
> adapter and the Request, which was not the intent.  We have<br>
> abstractions for "streaming" requests, but that was seemingly not<br>
> used.<br>
><br>
> 2. In the code that existed before this, Adaptor was a template on<br>
> purpose.  It is designed to implement the std::networking<br>
> AsyncReadStream and AsyncWriteStream concepts.  This is designed to<br>
> allow injection of Unit Tests at some point, as I've talked about<br>
> before.  Hardcoding it in request to 2 forced stream types, based on<br>
> the compiler flag is incorrect per asio standards, and removes the<br>
> ability to inject arbitrary adapters, like test adaptors.  Also, the<br>
> flag used is incorrect, as it's possible to inject a non-ssl socket<br>
> even if SSL is enabled.<br>
><br>
> 3. There is already a precedent and pattern for streaming interfaces<br>
> in bmcweb that we adopted from Crow.  If you look at the Websocket<br>
> request response type, it implements a way to request a route that<br>
> streams dynamically.  Frustratingly, part of what this was used for<br>
> was SSE, which I had already written a patch for that didn't have any<br>
> of the above issues, and only hadn't merged it because we didn't have<br>
> any SSE routes yet, and didn't want to check in dead code.<br>
> <a href="https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/13948" rel="noreferrer noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/13948</a><br>
><br>
> 4. It opens the possibility for lifetime and possible (at the very<br>
> least harder to audit) security issues, as now the "http server"<br>
> component is no longer the only thing that can own sockets.<br>
> Previously, the server owned the sockets until handed off, then there<br>
> was no shared ownership between the websocket class, and the<br>
> Connection class.  The Connection class could be completely destroyed<br>
> (and memory freed) while the websocket was still connected and<br>
> running.<br>
><br>
> Moving to another track, you may ask, how did I come across this and<br>
> why does it matter?  I'm trying to add 2 new features to bmcweb.  The<br>
> first allows opening multiple sockets, and dynamically detecting TLS<br>
> streams on them.  This allows bmcweb to handle both HTTPS redirects in<br>
> band, and handle the case where users type in something erroneous,<br>
> like "<a href="http://mybmc:443" rel="noreferrer noreferrer" target="_blank">http://mybmc:443</a>" and connect to an SSL socket with a non-ssl<br>
> protocol.  In those cases, we can simply do the right thing.  It also<br>
> allows bmcweb to host on multiple ports, which might be interesting<br>
> for aggregator types.  More importantly, it cleans up some of the<br>
> Adaptor abstraction to make way for unit testing, and being able to<br>
> inject a "test" socket, that we can control the semantics of.  I'm<br>
> hoping eventually to be able to mock dbus, and mock the TCP socket,<br>
> and run a full Redfish validator run in a unit test.  I think that<br>
> would save a lot of time overall for both committers and consumers.<br>
><br>
> The first of these patches is posted here, and simply comments out the<br>
> above problems for now.<br>
> <a href="https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35265" rel="noreferrer noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35265</a><br>
><br>
> If I look through the commit logs, it looks like Ravi and Appu built<br>
> the two small subsystems that rely on the above abstraction, one for<br>
> SSE, and one for some NBD streamer.<br>
> What do you two think about the above?  Was it something you<br>
> considered when you wrote your patches?  Would you consider fixing<br>
> them?<br>
><br>
>   My recommendation would be to move both of those two over to<br>
> something similar to the websocket abstraction we have, with, on<br>
> connect, on data, and on close handlers.  This means that handlers no<br>
> longer take a hard dependency on the transport, which will help for<br>
> both unit testing, and if we ever want to support redfish device<br>
> enablement (which relies on an i2c based transport). The SSE one can<br>
> probably be used more or less as-is from my old patch.  The NBD one<br>
> might need a "Dynamic body" type, which beast already has an<br>
> abstraction for that seems to have been discounted.<br>
><br>
> What do you guys think?<br>
><br>
> -Ed<br>
<br>
<br>
It's been 3 weeks and I haven't gotten any replies to this pretty<br>
major architecture break.  It also looks like it can also cause a<br>
memory leak in HttpConnection here (found by code inspection here).<br>
<a href="https://github.com/openbmc/bmcweb/blob/ebd459060ea4f42761402dd54acd0962c36136c2/http/http_connection.h#L351" rel="noreferrer noreferrer" target="_blank">https://github.com/openbmc/bmcweb/blob/ebd459060ea4f42761402dd54acd0962c36136c2/http/http_connection.h#L351</a><br>
<br>
I've pushed a revert to remove the features in question.  I would love<br>
some comments from the developers that caused these breakages, so I<br>
can make sure I'm doing the right thing here, and I'm not completely<br>
off base (or that you intend to fix them, and this patch is<br>
unnecessary).<br>
<a href="https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36038" rel="noreferrer noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36038</a><br>
<br>
Thanks,<br>
<br>
-Ed<br>
</blockquote></div>