Hi Ed,

=C2=A0

> Exactly, we thought to implement 4 callables in new rule class similar= to=C2=A0 websocket
>=C2=A0 =C2=A0 OnOpen(),OnMessage(),OnClose(),OnError() .=C2=A0 We can u= se names as suggested.

This is a one way dynamic loader, there's no such thing as an http
server sending a message while a response is in progress, so OnMessage
isn't required.=C2=A0

>
> Dump dump offload happens using NBD protocol, it's like NBD over h= ttp.
> In this=C2=A0 streaming use-case, data will be bidirectional as there = will be nbd acknowledgement
> for each nbd packet transferred to client. so thought to use "Str= eamSocket" name.

HTTP (with the exception of websockets) is not bidirectional.=C2=A0 It'= s
request->response.=C2=A0 Please do not break the HTTP protocol in this case.=C2=A0 If acknowledgement is needed, that would be a separate URL
route, so you can track the state in the backend, or you can use
websockets, which give a bidirectional socket API.
I will try to go through the code and understand your use case, but it
sounds a little odd to me.=C2=A0 Given we already have a websocket based nbd, was that not a good fit to your use case?

I do understand that HTTP is not=C2=A0bidirection= al and works with request-response model.
but=C2=A0 seems=C2=A0 HTTP/2 s= upports bidirectional stream

https://tools.ietf.org/html/rfc7540
stream:=C2=A0 A bidirecti= onal flow of frames within the HTTP/2 connection.

=C2= =A0=C2=A0

=C2=A0

>
>
>> >=C2=A0 =C2=A0 =C2=A02) Create another redfish node class(as th= e existing node class is for dynamic route)
>>
>> which would call app.route with the streamSocket trait.Do you have= any preference here?handleUpgrade() of router class gets invoked in the fo= llowing conditions -
>>
>> The Redfish Node class is specifically for request->response ty= pe
>> operations, and in the long run, I'd really like to see it jus= t go
>> away.=C2=A0 Today, it doesn't do anything more than the underl= ying router
>> already does, and is missing a number of things, like typesafe
>> parameter parsing, as well as copy-less parameters.=C2=A0 If it we= re me, I
>> would simply model your dynamic responses around BMCWEB_ROUTE
>>
>>
>>
>> >=C2=A0 =C2=A0 =C2=A0a)request contains http header(upgrade:web= socket)
>> >=C2=A0 =C2=A0 =C2=A0b)request contains http header (accept: te= xt/event-stream) ---> yet to be upstreamedIn our use case for dump strea= m, 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.When we search in the trie for a specific URL, we will get the assoc= iated rule class object, which in our case would be StreamSocketRule and th= e handle function of this class should make sure that socket is not closed.=
>> > Is my understanding correct understanding or do you have more= suggestion on this?There is an active discussion going on in the following= commit for the same.
>> > https://gerrit.openbmc-proj= ect.xyz/c/openbmc/bmcweb/+/36038
>>
>> I think you've described what bmcweb does today.=C2=A0 In this= case, we
>> might want to just promote an "isUpgrade" method from th= e app, that we
>> can use to determine if a given route is dynamic or not, and call = into
>> the correct "handleStream" operator.=C2=A0 Keep in mind = that at some point
>> you need to transfer control back to httpConnection to handle
>> keepalive properly.
>
>
>>
>> Thinking about keepalive more gave me another thought: This isn= 9;t
>> really an "upgrade" in the normal sense.=C2=A0 What if w= e just added a
>> "write" method to the Response object that immediately w= rote the
>> payload to the dynamic body and sent it?=C2=A0 write() could retur= n an
>> error code in the cases where the client has disconnected, and we<= br> >> never have to transfer ownership or knowledge of the socket, and t= he
>> keepalive and end() mechanisms would continue to work like normal.=
>> HttpConnection would have to get some knowledge about whether this=
>> request was dynamic or not, but that seems pretty doable, and coul= d
>> just be based on the first call to write.=C2=A0 Also, this keeps a= ll the
>> keepalive code the same, which I think is good.
>>
> That's exactly why we thought to use handle() method, but there is= a gap how to transfer ownership
> of socket from connection class to rule class.
>
> In the existing implementation where we transferred ownership of socke= t from connection class to rule class
> based on=C2=A0 http header field "upgrade::websocket"
>
>=C2=A0 As I explained above, we need bi-directional communication, wher= e bmc sends certain payload and nbd on client-side
> acknowledges received payload.
>
> So we need websocket way of implementation, where we need to keep read= ing and writing constantly on the same socket.

Why not just use websockets?=C2=A0 That's what they're designed to = do.

We can't u= se websockets, Because how does the client knows that they need to make the= websocket request rather than HTTP.

Dump o= ffload url is given as url, we don't specify the protocol in the url.

=C2=A0

> What I am unable to connect is how to transfer ownership of socket con= nection to new rule class, as in this case
> we can't take the decision based on=C2=A0 request header/content. = can you provide your suggestion=C2=A0 please?

I think I know how to do it, but let's make sure it's the right thi= ng
to do before we commit to that.

=C2=A0=C2=A0

ok.= can you please explain your thoughts here?=C2=A0

>
> What do you mean by dynamic request and dynamic response?

It's a concept within Beast, for a body that is streaming the output.
> As per my understanding, dynamic response is based on http request con= tent, one of the header field "accept"
> where client specifies data format and depending on this format, respo= nse gets generated. Is it a dynamic response?
> if it's true, how is it applicable here?

No, dynamic requests and dynamic responses are where the http
framework doesn't wait for the request to be done sending before
reading it in.=C2=A0 It's generally used for large requests you don'= ;t want
to buffer before sending.

https://www.boost.org/doc/libs/1_74_0/libs/beast/doc/html/beast= /ref/boost__beast__http__basic_dynamic_body.html=C2=A0

=C2=A0 >
> Response object writing payload to dynamic body may not work in this c= ase.
> Response object does not=C2=A0 hold=C2=A0 socket, only connection clas= s which is having socket, except handleUpgrade case
> where we transfer socket ownership to connection to rule class which c= reates another connection

That's what I'm saying, don't transfer the ownership at all, ju= st
create a response API that allows you to send data to socket directly.

>
> Thanks
> -Raviteja
>
>>
>>
>>
>> >> On Fri, 28 Aug, 2020, 12:42 am Ed Tanous, <ed@tanous.net> wrote:
>> >>>
>> >>> On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed@tanous.net> wrote: >> >>> >
>> >>> > I'm looking at a couple recent changes to bm= cweb, and I'm finding a
>> >>> > significant architecture problem has been inject= ed.=C2=A0 Namely, it's
>> >>> > these innocuous looking 4 lines here, which inje= cts the socket adaptor
>> >>> > into the request object for use later.
>> >>> > https://github.com/openbmc/bmcweb/blob/30c58d= 581606b4484757e6ee9133c248de1514a6/http/http_request.h#L18
>> >>> >
>> >>> > The problem with this approach has a few roots:<= br> >> >>> > 1. The Request class is meant to model a single = request, single
>> >>> > response model.=C2=A0 Adding the stream semantic= s breaks this in pretty
>> >>> > significant ways, and forces a hard dependency b= etween the streaming
>> >>> > adapter and the Request, which was not the inten= t.=C2=A0 We have
>> >>> > abstractions for "streaming" requests,= but that was seemingly not
>> >>> > used.
>> >>> >
>> >>> > 2. In the code that existed before this, Adaptor= was a template on
>> >>> > purpose.=C2=A0 It is designed to implement the s= td::networking
>> >>> > AsyncReadStream and AsyncWriteStream concepts.= =C2=A0 This is designed to
>> >>> > allow injection of Unit Tests at some point, as = I've talked about
>> >>> > before.=C2=A0 Hardcoding it in request to 2 forc= ed stream types, based on
>> >>> > the compiler flag is incorrect per asio standard= s, and removes the
>> >>> > ability to inject arbitrary adapters, like test = adaptors.=C2=A0 Also, the
>> >>> > flag used is incorrect, as it's possible to = inject a non-ssl socket
>> >>> > even if SSL is enabled.
>> >>> >
>> >>> > 3. There is already a precedent and pattern for = streaming interfaces
>> >>> > in bmcweb that we adopted from Crow.=C2=A0 If yo= u look at the Websocket
>> >>> > request response type, it implements a way to re= quest a route that
>> >>> > streams dynamically.=C2=A0 Frustratingly, part o= f what this was used for
>> >>> > was SSE, which I had already written a patch for= that didn't have any
>> >>> > of the above issues, and only hadn't merged = it because we didn't have
>> >>> > any SSE routes yet, and didn't want to check= in dead code.
>> >>> > https://gerrit= .openbmc-project.xyz/c/openbmc/bmcweb/+/13948
>> >>> >
>> >>> > 4. It opens the possibility for lifetime and pos= sible (at the very
>> >>> > least harder to audit) security issues, as now t= he "http server"
>> >>> > component is no longer the only thing that can o= wn sockets.
>> >>> > Previously, the server owned the sockets until h= anded off, then there
>> >>> > was no shared ownership between the websocket cl= ass, and the
>> >>> > Connection class.=C2=A0 The Connection class cou= ld be completely destroyed
>> >>> > (and memory freed) while the websocket was still= connected and
>> >>> > running.
>> >>> >
>> >>> > Moving to another track, you may ask, how did I = come across this and
>> >>> > why does it matter?=C2=A0 I'm trying to add = 2 new features to bmcweb.=C2=A0 The
>> >>> > first allows opening multiple sockets, and dynam= ically detecting TLS
>> >>> > streams on them.=C2=A0 This allows bmcweb to han= dle both HTTPS redirects in
>> >>> > band, and handle the case where users type in so= mething erroneous,
>> >>> > like "http://mybmc:443" and connect to an S= SL socket with a non-ssl
>> >>> > protocol.=C2=A0 In those cases, we can simply do= the right thing.=C2=A0 It also
>> >>> > allows bmcweb to host on multiple ports, which m= ight be interesting
>> >>> > for aggregator types.=C2=A0 More importantly, it= cleans up some of the
>> >>> > Adaptor abstraction to make way for unit testing= , and being able to
>> >>> > inject a "test" socket, that we can co= ntrol the semantics of.=C2=A0 I'm
>> >>> > hoping eventually to be able to mock dbus, and m= ock the TCP socket,
>> >>> > and run a full Redfish validator run in a unit t= est.=C2=A0 I think that
>> >>> > would save a lot of time overall for both commit= ters and consumers.
>> >>> >
>> >>> > The first of these patches is posted here, and s= imply comments out the
>> >>> > above problems for now.
>> >>> > https://gerrit= .openbmc-project.xyz/c/openbmc/bmcweb/+/35265
>> >>> >
>> >>> > If I look through the commit logs, it looks like= Ravi and Appu built
>> >>> > the two small subsystems that rely on the above = abstraction, one for
>> >>> > SSE, and one for some NBD streamer.
>> >>> > What do you two think about the above?=C2=A0 Was= it something you
>> >>> > considered when you wrote your patches?=C2=A0 Wo= uld you consider fixing
>> >>> > them?
>> >>> >
>> >>> >=C2=A0 =C2=A0My recommendation would be to move b= oth of those two over to
>> >>> > something similar to the websocket abstraction w= e have, with, on
>> >>> > connect, on data, and on close handlers.=C2=A0 T= his means that handlers no
>> >>> > longer take a hard dependency on the transport, = which will help for
>> >>> > both unit testing, and if we ever want to suppor= t redfish device
>> >>> > enablement (which relies on an i2c based transpo= rt). The SSE one can
>> >>> > probably be used more or less as-is from my old = patch.=C2=A0 The NBD one
>> >>> > might need a "Dynamic body" type, whic= h beast already has an
>> >>> > abstraction for that seems to have been discount= ed.
>> >>> >
>> >>> > What do you guys think?
>> >>> >
>> >>> > -Ed
>> >>>
>> >>>
>> >>> It's been 3 weeks and I haven't gotten any re= plies to this pretty
>> >>> major architecture break.=C2=A0 It also looks like it= can also cause a
>> >>> memory leak in HttpConnection here (found by code ins= pection here).
>> >>> https://github.com/openbmc/bmcweb/blob/ebd4590= 60ea4f42761402dd54acd0962c36136c2/http/http_connection.h#L351
>> >>>
>> >>> I've pushed a revert to remove the features in qu= estion.=C2=A0 I would love
>> >>> some comments from the developers that caused these b= reakages, so I
>> >>> can make sure I'm doing the right thing here, and= I'm not completely
>> >>> off base (or that you intend to fix them, and this pa= tch is
>> >>> unnecessary).
>> >>> https://gerrit.open= bmc-project.xyz/c/openbmc/bmcweb/+/36038
>> >>>
>> >>> Thanks,
>> >>>
>> >>> -Ed

--000000000000ca723905ae756192--