<div dir="ltr"><div dir="ltr"><pre class="gmail-c-mrkdwn__pre" style="box-sizing:inherit;margin-top:4px;margin-bottom:4px;padding:8px;font-size:12px;line-height:1.50001;font-variant-ligatures:none;white-space:pre-wrap;word-break:normal;border-radius:4px;color:rgb(29,28,29);font-family:Monaco,Menlo,Consolas,"Courier New",monospace">Hi Ed,
<br style="box-sizing:inherit">As requested in the gerrit commit(36038), continuing the discussion on the mailing list.<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>Once again, thanks for pointing out flaws in the code and apologies for missing your initial mail on this. <span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>Trying to summarize and also proposing my thoughts here to address the issues. <span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>Following is my understanding of high level flow for connection which requires streaming -<br style="box-sizing:inherit"> 1) Create StreamSocketRule and StreamSocket classes as like as websocket<br style="box-sizing:inherit"> 2) Add one more RuleParameterTraits for StreamSocket<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>Two ways to invoke this new class -<br style="box-sizing:inherit"> 1) Create static bmcweb route for each logservice with dump uri with specific rule trait for streamsocket<br style="box-sizing:inherit"> eg: <br style="box-sizing:inherit"> BMCWEB_ROUTE(app, "/system/LogServices/Dump/attachment/<str>",id)<br style="box-sizing:inherit"> .privileges({"ConfigureComponents", "ConfigureManager"})<br style="box-sizing:inherit"> .streamsocket()<br style="box-sizing:inherit"> 2) Create another redfish node class(as the existing node class is for dynamic route) which would call app.route with the streamSocket trait.<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>Do you have any preference here?<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>handleUpgrade() of router class gets invoked in the following conditions -<br style="box-sizing:inherit"> a)request contains http header(upgrade:websocket) <br style="box-sizing:inherit"> b)request contains http header (accept: text/event-stream) ---> yet to be upstreamed<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>In our use case for dump stream, we do not need to take this decision by looking at the request header as the URL is already registered with their associated rule(StreamSocketRule) class.<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>When we search in the trie for a specific URL, we will get the associated rule class object, which in our case would be StreamSocketRule and the handle function of this class should make sure that socket is not closed.<br style="box-sizing:inherit">Is my understanding correct understanding or do you have more suggestion on this?<span class="gmail-c-mrkdwn__br" style="box-sizing:inherit;display:block;height:unset"></span>There is an active discussion going on in the following commit for the same.<br style="box-sizing:inherit"><a target="_blank" class="gmail-c-link" href="https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36038" rel="noopener noreferrer" style="box-sizing:inherit;color:inherit;text-decoration-line:none">https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36038</a>
<span style="font-family:Arial,Helvetica,sans-serif;font-size:small;font-variant-ligatures:normal;color:rgb(34,34,34)">Thanks,
-Raviteja</span>
</pre></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 1, 2020 at 3:26 PM raviteja bailapudi <<a href="mailto:raviteja28031990@gmail.com">raviteja28031990@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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" target="_blank">ed@tanous.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <<a href="mailto:ed@tanous.net" rel="noreferrer" target="_blank">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>
</blockquote></div></div>