| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
There are cases in aggregation where an expand parameter might get
forwarded to a client. Because our previous expand algorithm assumed
that any endpoint within bmcweb would only produce "depth=1" responses,
it was reasonable to assume that the pre-response could not contain
expanded content. Aggregated resources can't make that assumption.
This commit attempts to pass through depth through the request, to
ensure that we only expand the level that the user requested, and not
any level returned by the request. This is done by using the existence
of the resource identifer "@odata.id" to indicate each level in an
expanded response. This should be fine since the Redfish spec requires
that property to exist.
Added unit tests to cover aggregation scenarios. Modified existing
$expand tests to comply with the resource identifier dependency.
Tested:
New unit tests pass
Queried '/redfish/v1/Systems?$expand=.($levels=2)' on an aggregated
system whose satellite BMC supported $expand. The overall response was
correctly expanded for both resources on the aggregating BMC as well as
on the satellite BMC. Expanding the satellite resources did not require
sending multiple queries to the satellite.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I20ba60ee39bac11ffb3fe1768cec6299cf9ee13e
Signed-off-by: Carson Labrado <clabrado@google.com>
|
|
Per cpp core guidelines, these should be methods.
Tested: on last patchset of the series.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib16479db9d2b68da68e7ad6e825c7e205c64f1de
|
|
This property was added in Redfish 2022.3 to allow clients to determine
which manager is hosting the ServiceRoot, such that they can find uptime
statistics, and other metrics from that resource, without needing to
attach them directly to serviceroot.
Tested:
Redfish service validator passes.
GET /redfish/v1/Managers/bmc returns the expected response.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: If2b78528d1499fbdae46120e1a1792ecf7ceb1d3
|
|
This change moves getMessage and getMessageFromRegistry to a .cpp file
so that they can be easily tested.
Tested: Unit test passes
Signed-off-by: Sui Chen <suichen@google.com>
Change-Id: Ia9fc91e5a47036198bf013ff3ea21ea9f6d5259a
|
|
string_view should always be passed by value; This commit is a sed
replace of the code to make all string_views pass by value, per general
coding guidelines[1].
[1] https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/
Tested: Code compiles.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I55b342a29a0fbfce0a4ed9ea63db6014d03b134c
|
|
Only id in event_service and account_service have not been updated due
to the risk of it breaking the username/id. It will require further
testing to verify.
Use urlFromPieces wherever that is needed to insert a variable in the
URI. Don't use urlFromPieces when it is hardcoded values. This allow us
to control all resource URIs that is dynamically added and to sync with
the current recommanded method for `@odata.id`. The goal is to have a
common place to manage the url created from dbus-paths in order to
manage/update it easily when needed.
Tested:
RedfishValidtor Passed for all resource including the sensors with the
fragments.
Change-Id: I95cdfaaee58fc7f21c95f5944e1e5c813b3215f2
Signed-off-by: Willy Tu <wltu@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Corrections style complaints in the aggregator code.
Tested:
Jenkins output did not show any style complaints
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I87426fcf2a48448a62152e0ad4a6c3aa54a7fc45
|
|
There are certain cases where we use this split function, and we expect
tokens to be read out. For example:
/xyz/openbmc_project/sensors/unit/name
Should split into a "" in the first position. This use case is not
common, and a quick grep shows only two places in the code expect this
behavior. Boost::split has this behavior already, which is what this
function is emulating. While we could fix these, in the end they should
be following the rules outlined in COMMON_ERRORS.md, which disallow
this kind of parsing completely.
Tested: New unit tests passing.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iec3dcbf2b495b2b3b4ed419172c4133b16f7c65d
|
|
For systems implementing to the OWASP security guidelines[1] (of which all
should ideally) we should be checking the content-type header all times
that we parse a request as JSON.
This commit adds an option for parsing content-type, and sets a default
of "must get content-type". Ideally this would not be a breaking
change, but given the number of guides and scripts that omit the content
type, it seems worthwhile to add a trapdoor, such that people can opt
into their own model on how they would like to see this checking work.
Tested:
```
curl --insecure -H "Content-Type: application/json" -X POST -D headers.txt https://${bmc}/redfish/v1/SessionService/Sessions -d '{"UserName":"root", "Password":"0penBmc"}'
```
Succeeds.
Removing Content-Type argument causes bmc to return
Base.1.13.0.UnrecognizedRequestBody.
[1] cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html
Change-Id: Iaa47dd563b40036ff2fc2cacb70d941fd8853038
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
boost::split has a documented false-positive in clang-tidy. While
normally we'd handle this with NOLINTNEXTLINE, this doesn't appear to
work in all cases. Unclear why, but seems to be due to some of our
lambda callback complexity.
Each of these uses is a case where we should be using a more specific
check, rather than split, but for the moment, this is the best we have.
Tested: clang-tidy passes.
[1] https://github.com/llvm/llvm-project/issues/40486
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I144c6610cb740287b7225e2be03b4142a64f9563
|
|
In the continual quest to get tidy passing when run in isolation, fix
some more includes.
This includes removing a circular #include to app.hpp. We don't use
app.hpp in these files, which is why our code compiles but having this
include it here causes a few circular dependencies
app.hpp -> http_server.hpp -> persistent_data.hpp -> app.hpp.
app.hpp -> http_server.hpp -> authentication.hpp -> app.hpp.
This confuses clang when run on header files directly.
Fix a couple more includes at the same time.
Tested: Code compiles
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib62d78b49c7e38ef7061c9fbbf6b3d463f11917d
|
|
The aggregator did not propagate header's fields from aggregated
responses. This change will take into account of response code other
than 200, which will modify a field called "Location". The Location
field in the response's header will point to where the response data
can be read from. This "Location" field in response Header will now
contain the correct URI with the prefix appended.
We will also copy over other Header Values to aggregated response. These
header values include "Content-Type", "Allow", "Retry-After", and also
the response's body
Added some test cases for the above fixes.
Tested:
Unit Tests pass.
Queries reponse that returns other result than 200 that has Location
field and the response received is as expected.
Signed-off-by: Khang Kieu <khangk@google.com>
Change-Id: I77c7dae32a103fbec3015fe14b51a3ed0022143e
|
|
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
|
|
This function is something that's easily unit tested. Do it.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8d664c77ec4b3a9886128597449c5f9c041b86b3
|
|
We only attempt prefix matching when we receive a 200 response. For
the retry policy we consider 2XX and 404 to be valid codes. Instead we
should forward all responses to the client and let them decide what
action they want to take. As part of that we should always attempt to
do prefix fixing on the response.
Also fixes an oversight where we attempt to do prefix fixing on
"OriginOfCondition" properties. That property is only a URI when it is
an Action parameter in a SubmitTestEvent request. It is an object when
it appears as a response property.
Adds test cases for the above fixes.
Tested:
Tests pass. Queries to top level collections and aggregated URIs still
return expected results with added prefixes.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ic76324ceab618160061be5f3c687af20a857fa25
|
|
Per the title, add unit tests for this function.
Tested: Unit tests pass
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ifdd9c314d2fa62ae3fa3b8f8150fcdd224a7eb03
|
|
Update the clearDump() implementation to call the DeleteAll D-Bus
method instead of iterating through D-Bus objects representing
individual log entries and calling the Delete D-Bus method on each one.
(It's more efficient for phosphor-debug-collector to iterate through
entries in its DeleteAll method handler than for bmcweb to iterate
through them.)
It seems like clearDump() wasn't originally implemented using DeleteAll
because dumps of various types were under the same D-Bus path namespace
at the time and there wasn't a way to selectively clear dumps of only a
specific type. The commit at [1] put different dump types under
different path namespaces (enabling us to now use DeleteAll).
Now clients should see a bit of performance improvement when running
the ClearLog action on dump LogServices, due to the reduced number of
D-Bus method calls needed to execute ClearLog.
Also updated getDumpServiceInfo() to populate the ClearLog action for
dump LogServices based on whether their dump manager object implements
xyz.openbmc_project.Collection.DeleteAll.
Tested:
Cleared the fault log containing 100 entries.
Ran with the time command several times before and after the change:
```
time curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Actions/LogService.ClearLog
```
Before the change, "real" time reported was ~1.2s.
After the change, "real" time reported was ~0.4s.
Forced creation of dump entries and then ran Redfish ClearLog action on
each dump type:
```
curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/Dump/Actions/LogService.ClearLog
curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Actions/LogService.ClearLog
curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Systems/system/LogServices/Dump/Actions/LogService.ClearLog
```
Then verified that there were no dump LogService entries afterwards:
```
curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Managers/bmc/LogServices/Dump/Entries
curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Entries
curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Systems/system/LogServices/Dump/Entries
```
Also verified that the corresponding D-Bus objects were gone from the
D-Bus tree after running ClearLog on each dump type:
Before ClearLog:
busctl tree xyz.openbmc_project.Dump.Manager
`-/xyz
`-/xyz/openbmc_project
`-/xyz/openbmc_project/dump
|-/xyz/openbmc_project/dump/bmc
| `-/xyz/openbmc_project/dump/bmc/entry
| `-/xyz/openbmc_project/dump/bmc/entry/101
|-/xyz/openbmc_project/dump/faultlog
| `-/xyz/openbmc_project/dump/faultlog/entry
| |-/xyz/openbmc_project/dump/faultlog/entry/11
| |-/xyz/openbmc_project/dump/faultlog/entry/12
| |-/xyz/openbmc_project/dump/faultlog/entry/13
| |-/xyz/openbmc_project/dump/faultlog/entry/14
| |-/xyz/openbmc_project/dump/faultlog/entry/15
| |-/xyz/openbmc_project/dump/faultlog/entry/16
| |-/xyz/openbmc_project/dump/faultlog/entry/17
| |-/xyz/openbmc_project/dump/faultlog/entry/18
| |-/xyz/openbmc_project/dump/faultlog/entry/19
| `-/xyz/openbmc_project/dump/faultlog/entry/20
|-/xyz/openbmc_project/dump/internal
| `-/xyz/openbmc_project/dump/internal/manager
`-/xyz/openbmc_project/dump/system
`-/xyz/openbmc_project/dump/system/entry
|-/xyz/openbmc_project/dump/system/entry/3
`-/xyz/openbmc_project/dump/system/entry/4
After ClearLog:
busctl tree xyz.openbmc_project.Dump.Manager
`-/xyz
`-/xyz/openbmc_project
`-/xyz/openbmc_project/dump
|-/xyz/openbmc_project/dump/bmc
|-/xyz/openbmc_project/dump/faultlog
|-/xyz/openbmc_project/dump/internal
| `-/xyz/openbmc_project/dump/internal/manager
`-/xyz/openbmc_project/dump/system
Confirmed that ClearLog action is listed for the following
LogServices:
/redfish/v1/Managers/bmc/LogServices/Dump
/redfish/v1/Managers/bmc/LogServices/FaultLog
/redfish/v1/Systems/system/LogServices/Dump
Then ran "systemctl stop xyz.openbmc_project.Dump.Manager" (which
removes dump manager objects including their
xyz.openbmc_project.Collection.DeleteAll interface) and saw that the
ClearLog action was no longer listed. Also locally built a version of
phosphor-debug-collecor with the interface
xyz.openbmc_project.Collection.DeleteAll removed from dump managers and
ran it and saw that the ClearLog action wasn't listed.
Redfish Service Validator passed on the following URIs
(with service xyz.openbmc_project.Dump.Manager running):
/redfish/v1/Managers/bmc/LogServices/Dump
/redfish/v1/Managers/bmc/LogServices/FaultLog
/redfish/v1/Systems/system/LogServices/Dump
Note: Most dump LogService unit tests were removed in this patchset
since this patchset adds a D-Bus call to getDumpServiceInfo(), and
we haven't decided how to mock D-Bus calls for unit testing yet.
[1] https://github.com/openbmc/phosphor-debug-collector/commit/fef66a951fe6fe283515480b2c493dfdc2275a95
Signed-off-by: Claire Weinan <cweinan@google.com>
Change-Id: Ic5f8f9e3528f521887766d8710bd77f969d8236a
|
|
Utilize the new array of top level collection URIs to determine if a
given URI in the response needs to have the aggregation prefix added.
This removes the need to check for specific collections like
/redfish/v1/UpdateService/FirmwareInventory which do not fit the
generic format of /redfish/v1/<collection>.
Future patches will use this same approach to improve the logic for
initially determining if and how a request should be aggregated.
This patch also adds a series of unit tests for the function
responsible for adding a prefix to a given URI. Cases covered include
valid URIs that involve a selection of aggregated resources, top level
collection URIs, other invalid URIs, and URIs with a trailing "/".
Tested:
Unit tests pass.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I676983d3c77ae3126c04e9f57ad8698c51df2675
|
|
c1d019a6056a2a0ef50e577b3139ab5a8dc49355 Sensor Optimization
Recently changed the way Ids were calculated in the sensor subsystem.
Unfortunately, it wasn't clear to the author that this would effect the
sensor override system, which relies on matching up a member ID with a
dbus path, and was broken by this change.
This commit breaks out the code to calculate the type and name from a
given URI segment into a helper method.
Tested: Inspection only. Very few systems support this feature. Code appears more correct than previously, which is known broken, so the lack of testing here seems reasonable.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I9aa8099a947a36b5ce914bc07ae60f1ebf0d209b
|
|
The new boost URL now interops properly with std::string_view, which is
great, and cleans up a bunch of mediocre code to convert one to another.
It has also been pulled into boost-proper, so we no longer need a
boost-url dependency that's separate.
Unfortunately, boost url makes these improvements by changing
boost::string_view for boost::urls::const_string, which causes us to
have some compile errors on the missing type.
The bulk of these changes fall into a couple categories, and have to be
executed in one commit.
string() is replaced with buffer() on the url and url_view types
boost::string_view is replaced by std::string_view for many times, in
many cases removing a temporary that we had in the code previously.
Tested: Code compiles with boost 1.81.0 beta.
Redfish service validator passes.
Pretty good unit test coverage for URL-specific use cases.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8d3dc89b53d1cc390887fe53605d4867f75f76fd
|
|
There are a number of properties of Type "string (uri)" for which we
do not currently support adding prefixes. This patch adds support
for all existing URI properties which are missed by the existing
implementation.
This change will be needed by future patches which will expand
aggregation support to all top level collections defined by the
schema. Those collections that are not currently supported include
properties whose URIs should be fixed, but would be missed by the
existing implementation.
Tested:
New unit test passes.
URI properties are still handled correctly.
```shell
curl localhost/redfish/v1/Chassis/5B247A_<chassisID>
{
"@odata.id": "/redfish/v1/Chassis/5B247A_<chassisID>",
"@odata.type": "#Chassis.v1_16_0.Chassis",
"Actions": {
"#Chassis.Reset": {
"@Redfish.ActionInfo": "/redfish/v1/Chassis/5B247A_<chassisID>/ResetActionInfo",
"target": "/redfish/v1/Chassis/5B247A_<chassisID>/Actions/Chassis.Reset"
}
},
...
}
```
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I3b3e06ee3191564d266598f7bc9f1641e6fcb333
|
|
As is, the router designates routes for every possible boost verb, of
which there are 31. In bmcweb, we only make use of 6 of those verbs, so
that ends up being quite a bit of wasted space and cache non-locality.
This commit invents a new enum class for declaring a subset of boost
verbs that we support, and a mapping between bmcweb verbs and boost
verbs.
Then it walks through and updates the router to support converting one
to another.
Tested:
Unit Tested
Redfish Service Validator performed on future commit
Signed-off-by: Ed Tanous <edtanous@google.com>
Signed-off-by: Edward Lee <edwarddl@google.com>
Change-Id: I3c89e896c632a5d4134dbd08a30b313c12a60de6
|
|
Testd: unit test only change
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I0a8d1e97d8f0be8a79b9c40a75eeb0659bba638b
|
|
This is a dummy commit to test owner plugin. It can be merged in as well
given that it adds a little bit readability.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ia4cc1866ebeb19e6d0f1d1ceece0ecc73fb4b468
|
|
Add helper function to append pieces to existing url to allow more
flexible control over the url. This allows us to avoid have each
resource append the pieces outside of the utility functions and help
maintain all url modifications in a central place for easy management.
Tested: Does not affect Redfish Tree. Unit Test passed.
Change-Id: I751f3c120cbadb465915b12aa253edd53ef32123
Signed-off-by: Willy Tu <wltu@google.com>
|
|
This commit implements the Redfish PowerSubsystem schema and collects
default property values.
PowerSupplies will be implemented in the next commit.
ref:
https://www.dmtf.org/sites/default/files/standards/documents/
DSP0268_2022.2.pdf (6.86 PowerSubsystem 1.1.0)
https://redfish.dmtf.org/schemas/v1/PowerSupply.v1_1_0.json
Tested: Validator and UT passes
1. curl -k -H "X-Auth-Token: $token" -X GET
https://${bmc}/redfish/v1/Chassis/chassis/PowerSubsystem
{
"@odata.id": "/redfish/v1/Chassis/chassis/PowerSubsystem",
"@odata.type": "#PowerSubsystem.v1_1_0.PowerSubsystem",
"Id": "PowerSubsystem",
"Name": "Power Subsystem",
"Status": {
"Health": "OK",
"State": "Enabled"
}
}
2. bad chassisID
curl -k -H "X-Auth-Token: $token" -X GET https://${bmc}
/redfish/v1/Chassis/badchassisID/PowerSubsystem/
PowerSupplies/powersupply0
{
"error": {
"@Message.ExtendedInfo": [
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The requested resource of type Chassis named
badchassisID was not found.",
"MessageArgs": [
"Chassis",
"badchassisID"
],
"MessageId": "Base.1.13.1.ResourceNotFound",
"MessageSeverity": "Critical",
"Resolution": "Provide a valid resource identifier
and resubmit the request."
}
],
"code": "Base.1.13.1.ResourceNotFound",
"message": "The requested resource of type Chassis named
badchassisID was not found."
}
}
Signed-off-by: Chicago Duan <duanzhijia01@inspur.com>
Change-Id: I6885b1777082538eceaf7ea85a8f69966459ee43
|
|
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
|
|
- Index was not checked against size before dereference. Which cased to
override memory.
- Header without colon could put parser into invalid state. Now it will
return with error.
- Content after boundary was not correctly discarded.
- Parser did not check body for final boudary. Now missing final
boundary will return with error.
Tested:
- Tested that payload with header without colon doesn't cause memory
corruption anymore.
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
Change-Id: I12f496ab5f53e6c088cdfdf2e96be636d66f7c7f
|
|
An HTTP header of Accepts: */* throws a big wrench into our
implementation for a couple reasons. First, because it's the default in
a lot of commonly-used libraries, and second, because clients use it
when they certainly don't mean what the specification says it should
mean "ie, I accept ANY type".
This commit tries to address some of that, by making an explicit option
for content-type="ANY" and pushes it to the individual callers to handle
explicitly as if it were yet another type. In most protocols, there's a
"most common" representation, so protocols are free to use that, or to
explicitly handle it, and require that the user be explicit.
Tested:
Redfish Protocol Validator no longer locks up. (TBD, getting bugs filed
with protocol validator for this missing Accepts header).
For ServiceRoot
GET /redfish/v1 Accepts: application/json - returns json
GET /redfish/v1 Accepts: */* - returns json
GET /redfish/v1 Accepts: text/html - returns html
GET /redfish/v1 no-accepts header - returns json
Redfish-service-validator passes.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iae6711ae587115d3e159a48a6fc46a903ed6c403
|
|
Like other C++ projects, unit tests normally are in a separate repo and
respect the folder structure of the file under test.
This commit deleted all "ut" folder and move tests to a "test" folder.
The test folder also has similar structure as the main folder.
This commit also made neccessary include changes to make codes compile.
Unused tests are untouched.
Tested: unit test passed.
Reference:
[1] https://github.com/grpc/grpc/tree/master/test
[2] https://github.com/boostorg/core/tree/414dfb466878af427d33b36e6ccf84d21c0e081b/test
[3] Many other OpenBMC repos: https://github.com/openbmc/entity-manager/tree/master/test
[4] https://stackoverflow.com/questions/2360734/whats-a-good-directory-structure-for-larger-c-projects-using-makefile
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I4521c7ef5fa03c47cca5c146d322bbb51365ee96
|
