| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
After a close call httpclient is not starting with fresh socket to
restart the connection. Send failure is observed in cases where new
connection is started from doResolve.Calling restartConnection instead
of doResolve did fix the issue.
Tested By:
Running developer test on use cases such as redfish aggregation where
number of retries are smaller.
Change-Id: I12f6a73fbafd14f482807f34ffa1e02fad944fc1
Signed-off-by: Abhilash Raju <abhilash.kollam@gmail.com>
|
|
Args captured by logging functions should be captured by rvalue, and use
std::forward to get perfect forwarding. In addition, separate out the
various std::out lines.
While we're here, also try to optimize a little. We should ideally be
writing each log line to the output once, and ideally not use iostreams,
which induce a lot of overhead.
Similar to spdlog[1] (which at one point this codebase used), construct
the string, then call fwrite and fflush once, rather than calling
std::cout repeatedly.
Now that we don't have a dependency on iostreams anymore, we can remove
it from the places where it has snuck in.
Tested:
Logging still functions as before. Logs present.
[1] https://github.com/gabime/spdlog/blob/27cb4c76708608465c413f6d0e6b8d99a4d84302/include/spdlog/sinks/stdout_sinks-inl.h#L70C7-L70C13
Change-Id: I1dd4739e06eb506d68989a066d122109b71b92cd
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
The configuration options that exist in bmcweb are an amalgimation of
CROW options, CMAKE options using #define, pre-bmcweb ifdef mechanisms
and meson options using a config file. This history has led to a lot of
different ways to configure code in the codebase itself, which has led
to problems, and issues in consistency.
ifdef options do no compile time checking of code not within the branch.
This is good when you have optional dependencies, but not great when
you're trying to ensure both options compile.
This commit moves all internal configuration options to:
1. A namespace called bmcweb
2. A naming scheme matching the meson option. hyphens are replaced with
underscores, and the option is uppercased. This consistent transform
allows matching up option keys with their code counterparts, without
naming changes.
3. All options are bool true = enabled, and any options with _ENABLED or
_DISABLED postfixes have those postfixes removed. (note, there are
still some options with disable in the name, those are left as-is)
4. All options are now constexpr booleans, without an explicit compare.
To accomplish this, unfortunately an option list in config/meson.build
is required, given that meson doesn't provide a way to dump all options,
as is a manual entry in bmcweb_config.h.in, in addition to the
meson_options. This obsoletes the map in the main meson.build, which
helps some of the complexity.
Now that we've done this, we have some rules that will be documented.
1. Runtime behavior changes should be added as a constexpr bool to
bmcweb_config.h
2. Options that require optionally pulling in a dependency shall use an
ifdef, defined in the primary meson.build. (note, there are no
options that currently meet this class, but it's included for
completeness.)
Note, that this consolidation means that at configure time, all options
are printed. This is a good thing and allows direct comparison of
configs in log files.
Tested: Code compiles
Server boots, and shows options configured in the default build. (HTTPS,
log level, etc)
Change-Id: I94e79a56bcdc01755036e4e7278c7e69e25809ce
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Boost beast ssl_stream is just a wrapper around asio ssl_stream, and
aims to optimize the case where we're writing small payloads (one or two
bytes.) which needs to be optimized in SSL.
bmcweb never writes one or two bytes, we almost always write the full
payload of what we received, so there's no reason to take the binary
size overhead, and additional boost headers that this implementation
requires.
Tested:
This drops the on-target binary size by 2.6%
Redfish service validator passes.
Change-Id: Ie1ae6f197f8e5ed70cf4abc6be9b1b382c42d64d
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Most of these were found by breaking every redfish class handler into
its own compile unit:
When that's done, these missing headers become compile errors. We
should just fix them.
In addition, this allows us to enable automatic header checking in
clang-tidy using misc-header-cleaner. Because the compiler can now
"see" all the defines, it no longer tries to remove headers that it
thinks are unused.
[1] https://github.com/openbmc/bmcweb/commit/4fdee9e39e9f03122ee16a6fb251a380681f56ac
Tested: Code compiles.
Change-Id: Ifa27ac4a512362b7ded7cc3068648dc4aea6ad7b
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Beast 331 added the message_generator class, which allows deduplicating
some templated code for the HTTP parser. When we use it, we can drop
our binary size, and ensure that we have code reuse.
This saves 2.2% on the compressed binary size.
Tested: Redfish service validator passes.
Change-Id: I5540d52dc256adfb62507c67ea642a9ea86d27ee
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Despite these objects being called "view" they are still relatively
large, as clang-tidy correctly flags, and we ignore.
Change all function uses to capture by:
const boost::urls::url_view_base&
Which is the base class of all boost URL types, and any class (url,
url_view, etc) is convertible to that base.
Change-Id: I8ee2ea3f4cfba38331303a7e4eb520a2b6f8ba92
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
The logger changes to move to std::format incidentally caused format
errors to no longer be flagged at compile time.
The example here[1] shows that the latest gcc/c++ gave us a way to solve
this, using std::format_string.
This incidentally shows two places where we got the format arguments
wrong, so fix them.
[1] https://stackoverflow.com/questions/72795189/how-can-i-wrap-stdformat-with-my-own-template-function
Change-Id: Id884200e2c98eeaf5ef8db6c1d6362ede2ffb858
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Now that our custom body type does things more than files, it makes
sense to rename it. This commit renames the header itself, then all
instances of the class.
Tested: Basic GET requests succeed.
Change-Id: If4361ac8992fc7c268f48a336707f96e68d3576c
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Now that we have a custom boost http body class, we can use it in more
cases. There's some significant overhead and code when switching to a
file body, namely removing all the headers. Making the body class
support strings would allow us to completely avoid that inefficiency.
At the same time, it would mean that we can now use that class for all
cases, including HttpClient, and http::Request. This leads to some code
reduction overall, and means we're reliant on fewer beast structures.
As an added benefit, we no longer have to take a dependency on
boost::variant2.
Tested: Redfish service validator passes, with the exception of
badNamespaceInclude, which is showing warnings prior to this commit.
Change-Id: I061883a73230d6085d951c15891465c2c8445969
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
These were found with:
codespell -w $(git ls-files | grep "\.[hc]\(pp\)\?$")
At some point in the future, we might want to get this enabled in CI.
Change-Id: Iccb57b2adfd06a2e177e99db2923fe4e8e329118
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
http_client is not handling connection termination by server due to
keep alive timeout. At present client is not aware of connection
termination from server. So whenever next redfish is event ready to be
sent, the client will try send/receives data over broken connection.
After failed operation the client will try to restart the connection
by closing the current connection.
Problems:
1) Restart is not attempted on all failure paths.
Eg: stream_truncated error was ignored, which usually happens when try
to read from broken connection, due to which retry is never performed.
2) Ssl shutdown over broken connection often fails to call the shutdown
callback
3) ssl session was reused for new connection attempt. Which is wrong
Solution:
This patch will try to reattempt the connection in all failure cases.
It uses new socket object and new ssl session for the retries
Tested by:
Test normal event flow between redfish-event clients and the BMC
Test failure event flow between redfish-event clients and the BMC
Tested the bad path by keeping the setup idle for 3 hours on the above
two setups. Verified the events flow after this idle time
Change-Id: I3d725b9d77bea22e2e8860e01ee0dfc971789008
Signed-off-by: Abhilash Raju <abhilash.kollam@gmail.com>
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
As is, it reads the whole file into memory before sending it. While
fairly fast for the user, this wastes ram, and makes bmcweb less useful
on less capable systems.
This patch enables using the boost::beast::http::file_body type, which
has more efficient serialization semantics than using a std::string. To
do this, it adds a openFile() handler to http::Response, which can be
used to properly open a file. Once the file is opened, the existing
string body is ignored, and the file payload is sent instead.
openFile() also returns success or failure, to allow users to properly
handle 404s and other errors.
To prove that it works, I moved over every instance of direct use of the
body() method over to using this, including the webasset handler. The
webasset handler specifically should help with system load when doing an
initial page load of the webui.
Tested:
Redfish service validator passes.
Change-Id: Ic7ea9ffefdbc81eb985de7edc0fac114822994ad
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
ssl_handshake fails while establishing connection to IPv6 destination
address, as IPv6 addresses considered as invalid value for SNI hostname
due to special characters.
SNI allows valid HostName which allows characters are only {alphabetic
characters (A-Z), numeric characters (0-9), the minus sign
This commit adds check to avoid setting SNI hostname if its an IP
address
Tested By: Verified redfish events 1. Subscribing Destination with IPv6
address. 2. Subscribing Destination with IPv4 address.
Change-Id: I32d30292bbc29c753f1c1815c66fcc93e8074eaa
Signed-off-by: Ravi Teja <raviteja28031990@gmail.com>
|
|
Type safety is a good thing. In:
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/65606
It was found that splitting out the URI into encoded pieces in the early
phase removed some information we needed, namely whether or not a URI
was ipv6. This commit changes http client such that it passes all the
information through, with the correct type, rather than passing in
hostname, port, path, and ssl separately.
Opportunistically, because a number of log lines are changing, this uses
the opportunity to remove a number of calls to std::to_string, and rely
on std::format instead.
Now that we no longer use custom URI splitting code, the
ValidateAndSplitUrl() method can be removed, given that our validation
now happens in the URI class.
Tested: Aggregation works properly when satellite URIs are queried.
Change-Id: I9f605863179af54c5af2719bc5ce9d29cbfffab7
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
There are instances of ERROR logs that would work better as WARNING or
DEBUG since they do not actually result in bailing early and returning
an error response.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I1e7bca0bb38487b26a4642ab72ce475170bb53c6
|
|
Clang-tidy has the aforementioned check, which shows a few places in the
core where we ignored the required optional checks. Fix all uses.
Note, we cannot enable the check that this time because of some weird
code in health.hpp that crashes tidy[1]. That will need to be a future
improvement.
There are tests that call something like
ASSERT(optional)
EXPECT(optional->foo())
While this isn't an actual violation, clang-tidy doesn't seem to be
smart enough to deal with it, so add some explicit checks.
[1] https://github.com/llvm/llvm-project/issues/55530
Tested: Redfish service validator passes.
Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
std::format is a much more modern logging solution, and gives us a lot
more flexibility, and better compile times when doing logging.
Unfortunately, given its level of compile time checks, it needs to be a
method, instead of the stream style logging we had before. This
requires a pretty substantial change. Fortunately, this change can be
largely automated, via the script included in this commit under
scripts/replace_logs.py. This is to aid people in moving their
patchsets over to the new form in the short period where old patches
will be based on the old logging. The intention is that this script
eventually goes away.
The old style logging (stream based) looked like.
BMCWEB_LOG_DEBUG << "Foo " << foo;
The new equivalent of the above would be:
BMCWEB_LOG_DEBUG("Foo {}", foo);
In the course of doing this, this also cleans up several ignored linter
errors, including macro usage, and array to pointer deconstruction.
Note, This patchset does remove the timestamp from the log message. In
practice, this was duplicated between journald and bmcweb, and there's
no need for both to exist.
One design decision of note is the addition of logPtr. Because the
compiler can't disambiguate between const char* and const MyThing*, it's
necessary to add an explicit cast to void*. This is identical to how
fmt handled it.
Tested: compiled with logging meson_option enabled, and launched bmcweb
Saw the usual logging, similar to what was present before:
```
[Error include/webassets.hpp:60] Unable to find or open /usr/share/www/ static file hosting disabled
[Debug include/persistent_data.hpp:133] Restored Session Timeout: 1800
[Debug redfish-core/include/event_service_manager.hpp:671] Old eventService config not exist
[Info src/webserver_main.cpp:59] Starting webserver on port 18080
[Error redfish-core/include/event_service_manager.hpp:1301] inotify_add_watch failed for redfish log file.
[Info src/webserver_main.cpp:137] Start Hostname Monitor Service...
```
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I86a46aa2454be7fe80df608cb7e5573ca4029ec8
|
|
The Async DBus resolver really has nothing to do with crow, which is our
core http library namespace and has some opportunistic cleanups that can
be done.
This commit moves it into the bmcweb namespace (unimportantly) and
breaks out one of the larger functions such that it can be unit tested,
and unit tests it.
Tested: Unit tests pass.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ie3cfbb0ef81a027a1ad42358c04967a517471117
|
|
This commit adds the host and port data to the existing error messages
that are hit during redfish event
Tested by:
Checked the traces on BMC when there is a bad subscriber
Change-Id: I3f18bc3b999c136c42c4c0021c5fcadddb9e4bff
Signed-off-by: Sunitha Harish <sunithaharish04@gmail.com>
|
|
This commit adds the SSE style eventservice subscription style event
Using this, end user can subscribe for Redfish event logs using GET
on SSE uris from browser.
Tested:
- From Browser did GET on above SSE URI and
generated some Redfish event logs(power cycle)
and saw redfish event logs streaming on browser.
- After SSE registration, Check Subscription collections
and GET on individual subscription and saw desired
response.
- Ran RedfishValidation and its passed.
Change-Id: I7f4b7a34974080739c4ba968ed570489af0474de
Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com>
Signed-off-by: P Dheeraj Srujan Kumar <p.dheeraj.srujan.kumar@intel.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
This commit adds a meson option to allow selecting which dns resolver
bmcweb uses. There are use cases, like Open Compute Project Inband
Management Agent, that would require not using dbus, which would require
us to fall back to the asio resolver. This commit makes the existing
asio resolver constructor, and async_resolve methods match the
equivalents in asio (which we intended to do anyway), then adds a macro
and configure option for being able to select which resolver backend to
rely on.
Tested: Code can now compile without sdbusplus.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I3220214367179f131a60082bdfaf7e725d35c125
|
|
Capturing these possibly overloaded values by reference can avoid a copy
in some cases, and it's good to be consistent.
This change was made automatically by grep/sed.
Tested: Code compiles.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iafeaca2a5dc52f39753b5a3880419d6bc943f81b
|
|
clang-format-16 has some backwards incompatible changes that require
additional settings for best compatibility and re-running the formatter.
Copy the latest .clang-format from the docs repository and reformat the
repository.
Change-Id: I75f89d2959b0f1338c20d72ad669fbdc1d720835
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
|
|
With Redfish aggregation, responses from satellite BMCs can be on the
order of MBs due to use cases like logging or binary payloads.
Offloading $expand could similar result in responses that exceed the
current read limit of 128 KB.
Splits the connection pools used for aggregation and EventService so
that the response read limit is 50MB for responses associated with
aggregation. Pools used by EventService keep the current limit of 2^17
bytes or 128 KB. It also propogates a ConnectionPolicy object that gets
instantiated within HttpClient, which allows per-client policies for
retry/byte limits. This allows EventService and aggregation to have
different policies.
Tested:
With aggregation enabled I was able to return a response from a
satellite BMC which was than 2MB. Ran the Redfish Mockup Creator and it
was able to successfully query all aggregated resources as part of
walking the tree. Also verified that HTTP push events still work with
EventListener.
Change-Id: I91de6f82aadf8ad6f7bc3f58dfa0d14c0759dd47
Signed-off-by: Carson Labrado <clabrado@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
By convention, we should be following boost here, and passing error_code
by reference, not by value. This makes our code consistent, and removes
the need for a copy in some cases.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id42ea4a90b6685a84818b87d1506c11256b3b9ae
|
|
With Redfish Aggregation there is a need for a large amount of
requests to be sent to a satellite BMC. Our current throughput of
4 connections with a 50 request buffer can be easily overwhelmed due
to the use of $expand. Also, BMCWeb itself can support 100 active
connections so multiple clients could be attempting to query a
satellite BMC that is being aggregated.
Increase the maximum number of connections to a destination to 20 and
increase the buffer request size to 500. These figures should be fine
since the requests themselves have a very small memory footprint and
the number of active connections is only a fifth of what the BMCWeb
in the satellite BMC should be able to support.
Note that these figures are an improvement over the current values.
They allowed making multi-level $expand requests without dropping any
subrequests due to the buffer becoming full. Further tuning will be
done in a future patch if it is determined that optimal performance can
be obtained by choosing different values.
Tested:
Debug logs after sending a multi-level $expand query showed the entire
connection pool being filled as well as well over 100 Requests being
queued. The additional connections provided enough throughput to
handle repeated simultaneous requests.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I96f165b5fbc76086e55b65faaaa49eb2753f8ef6
|
|
It's possible for HTTP client's request buffer to become full
(especially when $expand is used). Instead of ignoring the requests
we should provide a 429 Too Many Requests response for the provided
callback to process.
The aggregator's response handling also needs to account for this
possibility so that it only completely overwrites the asyncResp
object when it receives a response from a satellite.
Also added more test cases for the response processing functions.
Tested:
Unit tests passed
Flooded aggregator with requests for satellite resources. Requests
began returning 429 Too Many Requests errors after the request buffer
became full.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ib052dc0454d759de7fae761977ca26d6b8d208e5
|
|
Most of these missing includes were found by running clang-tidy on all
files, including headers. The existing scripts just run clang-tidy on
source files, which doesn't catch most of these.
Tested: Code compiles
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ic741fbb2cc9e5e92955fd5a1b778a482830e80e8
|
|
Now that we are using timer.async_wait() with the async http
operations we need to account for the scenario where the timer fails
before the operation fails. When that occurs we need to abort the
operation once its callback gets called.
Currently we proceed as if the timer doesn't exist. This causes a
fault if one of the operations times out. This patch adds a check
to the start of each async operation so that we do not continue with
the normal message sending flow when an operation times out.
Tested:
In order to create a connection timeout I created a dummy interface
and set the IP of my satellite BMC to route to the interface:
ip link add dummy0 type dummy
ip link set dev dummy0 up
ip route add 120.60.30.15 dev dummy0
All packets sent to 120.60.30.15 will get dropped and thus connection
attempts will timeout. This does not cause bmcweb to crash.
To make the satellite reachable again I used
this command to delete the routing:
ip route del 120.60.31.15 dev dummy0
After doing so messages were once again able to be forwarded correctly
to the satellite.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ie8d022c2195838e383eefcd0e12ae8cfab76e3e1
|
|
This commit fixed several places (but not all) where wrong include
directory is specified and prevent the clean up in the chidren changes.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ibbba62e2c0cfe3583a65f1befa1b233bd3eebf19
|
|
There's a few last calls that are inline lambdas. As we've been moving
away from inline lambdas due to the problems with debug logging and
usability, this file is the next cleanup.
Tested:
Ran Redfish-Event-Listener, and set an event into /var/log/redfish with
```
echo "2022-08-04T21:37:49.156798+00:00 OpenBMC.0.1.ServiceFailure,com.intel.DomainMapper.service" > /var/log/redfish
```
Simulating a crash. Saw the event get sent to Redfish-Event-Listener.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I61361eef03d1301dc126a28c695fdd74e504f891
|
|
tcp_stream has several problems in its current implementation. First,
it takes up a significant amount of binary size, for features that we
don't use. Next, it has some implied guarantees about timeouts that we
erronously rely on, namely that async_connect will timeout
appropriately given the tcp_stream timeout (it doesn't).
We already have a timer present in the ConnectionInfo class anyway, this
commit just makes use of it for ALL timeout operations, rather than just
the connect timeout operations. This flow is roughly analogous to what
we do in http_connection.hpp already, so the pattern is well troden.
This saves 2.8% of the binary size of bmcweb, and solves several race
conditions.
Tested:
Relying on Carson: Aggregated a sub-bmc, and ensured that top level
collections returned correctly under GET /redfish/v1/Chassis
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I72e8e97b667826f272bb4921afc2b16907f3b271
|
|
This commit adds the initial SSL support for http_client which can be
used for sending asynchronous Events/MetricReports to subscribed Event
Listener servers over secure channel.
Current implementation of http client only works for http protocol.
With current implementation, http client can be configured to work
with secure http (HTTPS). As part of implementation it adds the SSL
handshake mechanism and enforces the peer ceritificate verification.
The http-client uses the cipher suites which are supported by mozilla
browser and as recommended by OWASP. For better security enforcement
its disables the SSLv2, SSLv3, TLSv1, TLSv1.1 as described in below
OWASP cheetsheet.
It is validated with RootCA certificate(PEM) for now. Adding support
for different certificates can be looked in future as need arises.
[1]: https://cheatsheetseries.owasp.org/cheatsheets/TLS_Cipher_String_Cheat_Sheet.html
Tested:
- Created new subscription with SSL destination(https) and confirmed
that events are seen on EventListener side.
URI: /redfish/v1/EventService/Subscriptions
Method: POST
Body:
{
"Context": "CustomText",
"Destination": "https://<IP>:4000/service/collector/event_logs",
"EventFormatType": "Event",
"DeliveryRetryPolicy": "RetryForever",
"Protocol": "Redfish"
}
- Unit tested the non-SSL connection by disabling the check in code
(Note: EventService blocks all Non-SSL destinations). Verified that
all events are properly shown on EventListener.
URI: /redfish/v1/EventService/Subscriptions
Method: POST
Body:
{
"Context": "CustomText",
"Destination": "http://<IP>:4001/service/collector/event_logs",
"EventFormatType": "Event",
"Protocol": "Redfish"
}
- Combined above two tests and verified both SSL & Non-SSL work fine in
congention.
- Created subscription with different URI paths on same IP, Port and
protocol and verified that events sent as expected.
Change-Id: I13b2fc942c9ce6c55cd7348aae1e088a3f3d7fd9
Signed-off-by: AppaRao Puli <apparao.puli@intel.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Adds ability to route requests to either native resources or
resources that belong to a satellite BMC as part of Redfish
Aggregation. A prefix in the URI denotes if the resource is actually
from a satellite BMC. Prefixes are only used to denote satellite
resources. The URI of resources on the local/aggregating BMC will
remain unchanged.
Prefixes are separated from the resource ID by an underscore. This
means that underscores cannot be used in the prefix name itself.
The prefixes used by satellite BMCs are revealed via D-Bus as well as
the config information needed to connect to that BMC.
Requests for satellite resources will not be handled locally.
Care should be taken to not name any local resources in a way that
could cause a collision (e.g. having a Chassis object named
"aggregated0_1U" on the aggregating BMC).
The patch only covers routing requests. Requests to collection URIs
like /redfish/v1/Chassis will only return resources on the local BMC.
A future patch will cover adding satellite resources to collections.
Also note that URIs returned in the responses will not have the proper
prefix included. Fixing these URIs will be addressed in future
patches.
A number of TODO comments are included in the code to indicate that
this functionality (collections and URI fixup) still needs to be
implemented.
Example URIs w/o Redfish Aggregation:
/redfish/v1/Chassis/1U/
/redfish/v1/Systems/system/
/redfish/v1/Managers/bmc/
Example URIs after enabling Redfish Aggregation if the associated
resources are located on the local/aggregating BMC:
/redfish/v1/Chassis/1U/
/redfish/v1/Systems/system/
/redfish/v1/Managers/bmc/
Example URIs if resources are instead located on a satellite BMC
named "aggregated0":
/redfish/v1/Chassis/aggregated0_1U/
/redfish/v1/Systems/aggregated0_system/
/redfish/v1/Managers/aggregated0_bmc/
Tested:
I was able to query supported resources located on the local BMC
as well as on a satellite BMC. Requests with unknown prefixes return
a 404. Requests to resource collections only return the resources
that are located on the aggregating BMC.
Signed-off-by: Carson Labrado <clabrado@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I87a3deb730bda95e72ecd3144ea40b0e5ee7d491
|
|
A continuation of 4d94272fe54c147974f86788092bbbdd950406aa,
increases the size of httpReadBodyLimit to 2^17 bytes (about 128 KB).
This is because with Redfish Aggregation we need to be able to handle
larger response sizes such as json schema files. The ComputerSystem
schema in particular is over 120 KB.
Going forward we will not be able to keep increasing the limit.
We need a better solution for handling larger response that are
larger than httpReadBodyLimit.
Tested:
Used https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53310/68 to
successfully retrieve
/redfish/v1/JsonSchemas/ComputerSystem/ComputerSystem.json from a
satellite BMC.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ida80b8fddbd1df1310d18a77ecfb44b7fdf292ef
|
|
If a destination is not reachable, then the connection will timeout
in doConnect(). This causes issues later when the client attempts to
resend the message.
The error check in doClose() should not exit early since that will
result in the connection's status not being marked as closed and
thus it will never get reused.
Similarly, doCloseAndRetry() should not exit early since that will
cause the retry flow to hang and the connection's callback function
will not get deleted.
Tested:
Used Redfish Aggregation patches in the chain through
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/54896 to verify that
requests to collections such as /redfish/v1/Chassis no longer hang
when the specified Satellite BMC does not exist
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ic8369b0de8efb00ff168bc1ed43f1d7fd6c7366a
|
|
cppcheck correctly notes that a lot of variables in the new code can be
const. Make most of them const.
Tested: WIP
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8f37b6353fd707923f533e1d61c5b5419282bf23
|
|
HttpClient these days is intended to be a generic feature. It should
not be relying directly on Redfish messages. In this case, we only ever
rely on the return code internally, so replace messages with a simple
bad_gateway.
Tested: Code compiles. Unit tests pass.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ie4bd65dc0b90b75f61ab0e31ca535eefbf0f4ebb
|
|
cppcheck correctly notes this parameter can be const.
Unfortunately we've making a copy here, but looking at the ownership,
that can't be avoided simply without a shared_ptr, and that's far more
invasive.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib70c89a7a1bc7210219052ba2a44bc7a1c20c7f2
|
|
This variable was clearly for the print statement below, but then got
abandoned at some point. Clean it up.
cppcheck found this as well.
Tested: Unused code
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I2d143f00dc6956c87ba77d5cbe1b96be631ca795
|
|
cppcheck correctly finds that these branches will always be hit, so the
branch is unneeded. Let's start by getting the code cleaned up.
Tested: CI only, cpp check.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I7b89337a81e676915243d5bc9d26c24a89c74aef
|
|
Include what you use.
Tested: Code compiles
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I23c89fee6f3e39d2f2a7dc1c3ba2db8819e8adc7
|
|
This patchset is the conclusion of a multi-year effort to try to fix
shadowed variable names. Variables seem to be shadowed all over, and in
most places they exist, there's a "code smell" of things that aren't
doing what the author intended.
This commit attempts to clean up these in several ways by:
1. Renaming variables where appropriate.
2. Preferring to refer to member variables directly when operating
within a class
3. Rearranging code so that pass through variables are handled in the
calling scope, rather than passing them through.
These patterns are applied throughout the codebase, to the point where
-Wshadow can be enabled in meson.build.
Tested: Code compiles, unit tests pass. Still need to run redfish
service validator.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: If703398c2282f9e096ca2694fd94515de36a098b
|
|
Testing of https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53310 revealed
that recvMessage() was failing because the body limit was being
exceeded. httpReadBodyLimit was being used to set both the body limit
as well as the buffer size. Add a different variable to set the buffer
size so that the body limit can be doubled to 16K while the fixed
buffer can be set as a smaller size of 4K.
Tested:
Queries using the mentioned patch stopped failing after applying this
change.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I33b62dfc12ff078fba9fb92b2f95a05f41acce3d
|
|
Allows individual retry policies to specify what HTTP response codes
are considered valid. Sets functions for the EventService and
Redfish Aggregation retry policies. Those functions expect a
response code and return an error code based on what the response
code is.
This change is needed because EventService only considers 2XX codes
to be valid. Any code outside of that range would trigger a retry
attempt. Redfish Aggregation by design will need to return
errors outside of that range such as 404. It should not retry to
send a message when it receives a 404 from a satellite BMC.
Right now 404 is the only error code that is handled differently
between the services. Going forward, Redfish Aggregation will
likely want to allow other error codes as its functionality is
expanded.
Tested:
Used Redfish-Event-Listener with ssh port forwarding to create 3
subscriptions. I then closed the ssh connection and sent a test
event. Bmcweb made 3 retry attempts for each subscription. At
that point the max retry amount (as defined by EventService) was
reached and bmcweb stop attempting to resend the messages.
There were no errors when the Redfish-Event-Listener was correctly
connected. Test events resulted in messages being sent for each
subscription.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ifdfaf638d28982ed18998f3ca05280a288e0020a
|
|
The usual updates to make code compile on clang again. Extra semicolons
that have snuck in, missing inline and static definitions.
Tested: Code compiles on clang.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id7f889de98cafaa89471d75ed3e3bb97ab3855cd
|
|
clang-tidy has a setting, LambdaBodyIndentation, which it says:
"For callback-heavy code, it may improve readability to have the
signature indented two levels and to use OuterScope."
bmcweb is very callback heavy code. Try to enable it and see if that
improves things. There are many cases where the length of a lambda call
will change, and reindent the entire lambda function. This is really
bad for code reviews, as it's difficult to see the lines changed. This
commit should resolve it. This does have the downside of reindenting a
lot of functions, which is unfortunate, but probably worth it in the
long run.
All changes except for the .clang-format file were made by the robot.
Tested: Code compiles, whitespace changes only.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib4aa2f1391fada981febd25b67dcdb9143827f43
|
|
Modifies HttpClient so that the HTTP verb and headers can be set
for each individual message sent. Right now those fields are set
when a connection is first created and then reused by each
message sent using that connection.
Tested:
Launched two Event Listener servers that created 6 and 2
subscriptions. Sending a test event resulted in the servers
receiving 6 requests and 2 requests, respectively.
Change-Id: I8d7e2d54385bc2c403498293820adb584bff8b57
Signed-off-by: Carson Labrado <clabrado@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Adds sendDataWithCallback() which allows the caller to include a
callback specifying how to handle the response to that
request. This will be utilized for Redfish Aggregation
including returning the responses received when forwarding
requests to satellite BMCs.
Change-Id: I93826c8b254a5f28a982295d4145453352a90fae
Signed-off-by: Carson Labrado <clabrado@google.com>
|
