Recent architecture breakages to bmcweb

[Bruce Mitchell]

I agree with Ed; I too against breaking the HTTP RFC in bmcweb, we should avoid it in all cases.

> -----Original Message-----
> From: openbmc [mailto:openbmc-
> bounces+bruce_mitchell=phoenix.com at lists.ozlabs.org] On Behalf Of Ed
> Tanous
> Sent: Thursday, September 3, 2020 22:41
> To: raviteja bailapudi
> Cc: OpenBMC Maillist
> Subject: Re: Recent architecture breakages to bmcweb
> 
> On Thu, Sep 3, 2020 at 9:34 PM raviteja bailapudi
> <raviteja28031990 at gmail.com> wrote:
> >
> >
> > Hi Ed,
> >
> >>
> >>
> >> > Exactly, we thought to implement 4 callables in new rule class
> similar to  websocket
> >> >    OnOpen(),OnMessage(),OnClose(),OnError() .  We can use 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.
> >>
> >> >
> >> > Dump dump offload happens using NBD protocol, it's like NBD over
> http.
> >> > In this  streaming use-case, data will be bidirectional as there will be
> nbd acknowledgement
> >> > for each nbd packet transferred to client. so thought to use
> "StreamSocket" name.
> >>
> >>
> >> HTTP (with the exception of websockets) is not bidirectional.  It's
> >> request->response.  Please do not break the HTTP protocol in this
> >> case.  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.  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 bidirectional and works with request-
> response model.
> > but  seems  HTTP/2 supports bidirectional stream
> > https://tools.ietf.org/html/rfc7540
> > stream:  A bidirectional flow of frames within the HTTP/2 connection.
> 
> True.  If you're looking to implement HTTP/2 in bmcweb, that's a
> really big conversation, with a lot of implications.  The best place
> to start would probably be a design doc.
> 
> >
> >
> >>
> >> >
> >> >
> >> >> >     2) Create another redfish node class(as the 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 following conditions -
> >> >>
> >> >> The Redfish Node class is specifically for request->response type
> >> >> operations, and in the long run, I'd really like to see it just go
> >> >> away.  Today, it doesn't do anything more than the underlying
> router
> >> >> already does, and is missing a number of things, like typesafe
> >> >> parameter parsing, as well as copy-less parameters.  If it were me, I
> >> >> would simply model your dynamic responses around
> BMCWEB_ROUTE
> >> >>
> >> >>
> >> >>
> >> >> >     a)request contains http header(upgrade:websocket)
> >> >> >     b)request contains http header (accept: text/event-stream) --->
> yet to be upstreamedIn 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.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.
> >> >> > 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-project.xyz/c/openbmc/bmcweb/+/36038
> >> >>
> >> >> I think you've described what bmcweb does today.  In this case, we
> >> >> might want to just promote an "isUpgrade" method from the app,
> that we
> >> >> can use to determine if a given route is dynamic or not, and call into
> >> >> the correct "handleStream" operator.  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't
> >> >> really an "upgrade" in the normal sense.  What if we just added a
> >> >> "write" method to the Response object that immediately wrote the
> >> >> payload to the dynamic body and sent it?  write() could return an
> >> >> error code in the cases where the client has disconnected, and we
> >> >> never have to transfer ownership or knowledge of the socket, and
> the
> >> >> 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
> could
> >> >> just be based on the first call to write.  Also, this keeps all 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
> socket from connection class to rule class
> >> > based on  http header field "upgrade::websocket"
> >> >
> >> >  As I explained above, we need bi-directional communication,
> where bmc sends certain payload and nbd on client-side
> >> > acknowledges received payload.
> >> >
> >> > So we need websocket way of implementation, where we need to
> keep reading and writing constantly on the same socket.
> >>
> >> Why not just use websockets?  That's what they're designed to do.
> >>
> >
> > We can't use websockets, Because how does the client knows that they
> need to make the websocket request rather than HTTP.
> > Dump offload url is given as url, we don't specify the protocol in the url.
> 
> As a counter, in the model above, how does the client know it needs to
> speak NBD?  How does any client know it needs to do any protocol
> communication?  It has to have some prior knowledge of some port,
> host, and url, in question.  If you want a hypermedia API, you would
> provide a starter resource with a link that starts with wss://.
> Alternatively, you could just do what all the other websockets
> handlers do, and just have the client have prior knowledge that that
> URL leads to a websocket, and open it appropriately.
> 
> >
> >>
> >> > What I am unable to connect is how to transfer ownership of socket
> connection to new rule class, as in this case
> >> > we can't take the decision based on  request header/content. can
> you provide your suggestion  please?
> >>
> >> I think I know how to do it, but let's make sure it's the right thing
> >> to do before we commit to that.
> >
> >
> > ok. can you please explain your thoughts here?
> 
> If you want bidirectional communication, you should use websockets.
> If you want one way streaming communication, you should use regular
> HTTP (for "fast" data), SSE (For rare event data), or Long polling (if
> you want to implement something relatively well supported).  With that
> said, for what it looks like you're trying to do, I suspect you don't
> really need bidirectional communication for a file download.  With
> that said, I don't know your hypervisor, so I could be wrong.
> 
> To be clear, I'm against breaking the HTTP RFC in bmcweb, we should
> avoid it in all cases.
> 
> -Ed
> 
> >>
> >> >
> >> > 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 content, one of the header field "accept"
> >> > where client specifies data format and depending on this format,
> response 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.  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
> >>
> >>
> >>
> >> >
> >> > Response object writing payload to dynamic body may not work in
> this case.
> >> > Response object does not  hold  socket, only connection class which
> is having socket, except handleUpgrade case
> >> > where we transfer socket ownership to connection to rule class
> which creates another connection
> >>
> >> That's what I'm saying, don't transfer the ownership at all, just
> >> 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 at tanous.net>
> wrote:
> >> >> >>>
> >> >> >>> On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed at tanous.net>
> wrote:
> >> >> >>> >
> >> >> >>> > I'm looking at a couple recent changes to bmcweb, and I'm
> finding a
> >> >> >>> > significant architecture problem has been injected.  Namely,
> it's
> >> >> >>> > these innocuous looking 4 lines here, which injects the socket
> adaptor
> >> >> >>> > into the request object for use later.
> >> >> >>> >
> https://github.com/openbmc/bmcweb/blob/30c58d581606b4484757e6e
> e9133c248de1514a6/http/http_request.h#L18
> >> >> >>> >
> >> >> >>> > The problem with this approach has a few roots:
> >> >> >>> > 1. The Request class is meant to model a single request, single
> >> >> >>> > response model.  Adding the stream semantics breaks this in
> pretty
> >> >> >>> > significant ways, and forces a hard dependency between the
> streaming
> >> >> >>> > adapter and the Request, which was not the intent.  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.  It is designed to implement the std::networking
> >> >> >>> > AsyncReadStream and AsyncWriteStream concepts.  This is
> designed to
> >> >> >>> > allow injection of Unit Tests at some point, as I've talked
> about
> >> >> >>> > before.  Hardcoding it in request to 2 forced stream types,
> based on
> >> >> >>> > the compiler flag is incorrect per asio standards, and removes
> the
> >> >> >>> > ability to inject arbitrary adapters, like test adaptors.  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.  If you look at the
> Websocket
> >> >> >>> > request response type, it implements a way to request a
> route that
> >> >> >>> > streams dynamically.  Frustratingly, part of 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 possible (at the
> very
> >> >> >>> > least harder to audit) security issues, as now the "http server"
> >> >> >>> > component is no longer the only thing that can own sockets.
> >> >> >>> > Previously, the server owned the sockets until handed off,
> then there
> >> >> >>> > was no shared ownership between the websocket class, and
> the
> >> >> >>> > Connection class.  The Connection class could 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?  I'm trying to add 2 new features to
> bmcweb.  The
> >> >> >>> > first allows opening multiple sockets, and dynamically
> detecting TLS
> >> >> >>> > streams on them.  This allows bmcweb to handle both HTTPS
> redirects in
> >> >> >>> > band, and handle the case where users type in something
> erroneous,
> >> >> >>> > like "http://mybmc:443" and connect to an SSL socket with a
> non-ssl
> >> >> >>> > protocol.  In those cases, we can simply do the right thing.  It
> also
> >> >> >>> > allows bmcweb to host on multiple ports, which might be
> interesting
> >> >> >>> > for aggregator types.  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 control the semantics of.
> I'm
> >> >> >>> > hoping eventually to be able to mock dbus, and mock the TCP
> socket,
> >> >> >>> > and run a full Redfish validator run in a unit test.  I think that
> >> >> >>> > would save a lot of time overall for both committers and
> consumers.
> >> >> >>> >
> >> >> >>> > The first of these patches is posted here, and simply
> 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?  Was it something
> you
> >> >> >>> > considered when you wrote your patches?  Would you
> consider fixing
> >> >> >>> > them?
> >> >> >>> >
> >> >> >>> >   My recommendation would be to move both of those two
> over to
> >> >> >>> > something similar to the websocket abstraction we have, with,
> on
> >> >> >>> > connect, on data, and on close handlers.  This 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 support redfish
> device
> >> >> >>> > enablement (which relies on an i2c based transport). The SSE
> one can
> >> >> >>> > probably be used more or less as-is from my old patch.  The
> NBD one
> >> >> >>> > might need a "Dynamic body" type, which beast already has
> an
> >> >> >>> > abstraction for that seems to have been discounted.
> >> >> >>> >
> >> >> >>> > What do you guys think?
> >> >> >>> >
> >> >> >>> > -Ed
> >> >> >>>
> >> >> >>>
> >> >> >>> It's been 3 weeks and I haven't gotten any replies to this pretty
> >> >> >>> major architecture break.  It also looks like it can also cause a
> >> >> >>> memory leak in HttpConnection here (found by code inspection
> here).
> >> >> >>>
> https://github.com/openbmc/bmcweb/blob/ebd459060ea4f42761402dd
> 54acd0962c36136c2/http/http_connection.h#L351
> >> >> >>>
> >> >> >>> I've pushed a revert to remove the features in question.  I
> would love
> >> >> >>> some comments from the developers that caused these
> breakages, 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 patch is
> >> >> >>> unnecessary).
> >> >> >>> https://gerrit.openbmc-
> project.xyz/c/openbmc/bmcweb/+/36038
> >> >> >>>
> >> >> >>> Thanks,
> >> >> >>>
> >> >> >>> -Ed