Recent architecture breakages to bmcweb

Recent architecture breakages to bmcweb

[Ed Tanous]

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/30c58d581606b4484757e6ee9133c248de1514a6/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

Re: Recent architecture breakages to bmcweb

[Ed Tanous]

On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed@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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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

Re: Recent architecture breakages to bmcweb

[raviteja bailapudi]

[-- Attachment #1: Type: text/plain, Size: 5758 bytes --]

Ed,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.
I had few more doubts which I asked in the gerrit, Can you please take a
look?



-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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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
>

[-- Attachment #2: Type: text/html, Size: 8413 bytes --]

Re: Recent architecture breakages to bmcweb

[raviteja bailapudi]

[-- Attachment #1: Type: text/plain, Size: 7830 bytes --]

Hi Ed,
As requested in the gerrit commit(36038), continuing the discussion on
the mailing list.Once again, thanks for pointing out flaws in the code
and apologies for missing your initial mail on this. Trying to
summarize and also proposing my thoughts here to address the issues.
Following is my understanding of high level flow for connection which
requires streaming -
   1) Create StreamSocketRule and StreamSocket classes as like as websocket
   2) Add one more RuleParameterTraits for StreamSocketTwo ways to
invoke this new class -
    1) Create static bmcweb route for each logservice with dump uri
with specific rule trait for streamsocket
      eg:
        BMCWEB_ROUTE(app, "/system/LogServices/Dump/attachment/<str>",id)
        .privileges({"ConfigureComponents", "ConfigureManager"})
        .streamsocket()
    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 -
    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
Thanks,
-Raviteja


On Tue, Sep 1, 2020 at 3:26 PM raviteja bailapudi <
raviteja28031990@gmail.com> wrote:

> Ed,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.
> I had few more doubts which I asked in the gerrit, Can you please take a
> look?
>
>
>
> -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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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
>>
>

[-- Attachment #2: Type: text/html, Size: 12596 bytes --]

Re: Recent architecture breakages to bmcweb

[Ed Tanous]

On Wed, Sep 2, 2020 at 4:49 AM raviteja bailapudi
<raviteja28031990@gmail.com> wrote:
>
> Hi Ed,
>
> As requested in the gerrit commit(36038), continuing the discussion on the mailing list.Once again, thanks for pointing out flaws in the code and apologies for missing your initial mail on this. Trying to summarize and also proposing my thoughts here to address the issues. Following is my understanding of high level flow for connection which requires streaming -

Would you mind also commenting on the revert for this?  It would help
get consensus so we can fix the memory leaks that a couple of these
changesets injected.  Given that we've been pretty badly broken for
about a month, I think we need to get the revert merged as soon as
possible.

>    1) Create StreamSocketRule and StreamSocket classes as like as websocket
>    2) Add one more RuleParameterTraits for StreamSocketTwo ways to invoke this new class -
>     1) Create static bmcweb route for each logservice with dump uri  with specific rule trait for streamsocket
>       eg:
>         BMCWEB_ROUTE(app, "/system/LogServices/Dump/attachment/<str>",id)
>         .privileges({"ConfigureComponents", "ConfigureManager"})
>         .streamsocket()

I'd much rather call it "dyamicResponse" and have it take 3 callables,
one for onRequest, that accepts a Request and a Response object, like
a normal handler, one for onData, that accepts a buffer of the body
data, and one for onClose, that informs the calling code the socket
was closed.

>     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.

>
> Thanks,
> -Raviteja
>
>
> On Tue, Sep 1, 2020 at 3:26 PM raviteja bailapudi <raviteja28031990@gmail.com> wrote:
>>
>> Ed,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.
>> I had few more doubts which I asked in the gerrit, Can you please take a look?
>>
>>
>>
>> -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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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

Re: Recent architecture breakages to bmcweb

[raviteja bailapudi]

[-- Attachment #1: Type: text/plain, Size: 11142 bytes --]

Ed,

Thank you so much for quick response.

>    1) Create StreamSocketRule and StreamSocket classes as like as
> websocket
> >    2) Add one more RuleParameterTraits for StreamSocketTwo ways to
> invoke this new class -
> >     1) Create static bmcweb route for each logservice with dump uri
> with specific rule trait for streamsocket
> >       eg:
> >         BMCWEB_ROUTE(app, "/system/LogServices/Dump/attachment/<str>",id)
> >         .privileges({"ConfigureComponents", "ConfigureManager"})
> >         .streamsocket()
>
> I'd much rather call it "dyamicResponse" and have it take 3 callables,
> one for onRequest, that accepts a Request and a Response object, like
> a normal handler, one for onData, that accepts a buffer of the body
> data, and one for onClose, that informs the calling code the socket
> was closed.



Exactly, we thought to implement 4 callables in new rule class similar to
websocket
   OnOpen(),OnMessage(),OnClose(),OnError() .  We can use names as
suggested.

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.


>     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.

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?

What do you mean by dynamic request and dynamic response?
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?

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

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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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
>

[-- Attachment #2: Type: text/html, Size: 15279 bytes --]

Re: Recent architecture breakages to bmcweb

[Ed Tanous]

On Thu, Sep 3, 2020 at 5:20 AM raviteja bailapudi
<raviteja28031990@gmail.com> wrote:
>
>
> Ed,
>
> Thank you so much for quick response.
>
>> >    1) Create StreamSocketRule and StreamSocket classes as like as websocket
>> >    2) Add one more RuleParameterTraits for StreamSocketTwo ways to invoke this new class -
>> >     1) Create static bmcweb route for each logservice with dump uri  with specific rule trait for streamsocket
>> >       eg:
>> >         BMCWEB_ROUTE(app, "/system/LogServices/Dump/attachment/<str>",id)
>> >         .privileges({"ConfigureComponents", "ConfigureManager"})
>> >         .streamsocket()
>>
>> I'd much rather call it "dyamicResponse" and have it take 3 callables,
>> one for onRequest, that accepts a Request and a Response object, like
>> a normal handler, one for onData, that accepts a buffer of the body
>> data, and one for onClose, that informs the calling code the socket
>> was closed.
>
>
>
> 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?

>
>
>> >     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.

>
> 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.

>
> 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@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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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

Re: Recent architecture breakages to bmcweb

[raviteja bailapudi]

[-- Attachment #1: Type: text/plain, Size: 12906 bytes --]

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.



> >
> >
> >> >     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.

>
> > 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?

> >
> > 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@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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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
>

[-- Attachment #2: Type: text/html, Size: 18471 bytes --]

Re: Recent architecture breakages to bmcweb

[Ed Tanous]

On Thu, Sep 3, 2020 at 9:34 PM raviteja bailapudi
<raviteja28031990@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@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 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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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

RE: Recent architecture breakages to bmcweb

[Bruce Mitchell]

[-- Attachment #1: Type: text/plain, Size: 13081 bytes --]

Let’s stay focused on the now feature of HTTP.  Keep an eye towards HTTP/2, HTTP/3, and QUIK;
I am trying to keep the ADHD down for the OpenBMC project and especially security related areas.

From: openbmc [mailto:openbmc-bounces+bruce_mitchell=phoenix.com@lists.ozlabs.org] On Behalf Of raviteja bailapudi
Sent: Thursday, September 3, 2020 21:34
To: Ed Tanous
Cc: OpenBMC Maillist
Subject: Re: Recent architecture breakages to bmcweb


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.


>
>
>> >     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.

> 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?
>
> 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@tanous.net<mailto:ed@tanous.net>> wrote:
>> >>>
>> >>> On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed@tanous.net<mailto:ed@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/30c58d581606b4484757e6ee9133c248de1514a6/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/ebd459060ea4f42761402dd54acd0962c36136c2/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

[-- Attachment #2: Type: text/html, Size: 22284 bytes --]

RE: 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@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@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@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 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