Aggregation: Fix and forward all responses
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
diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index 8923fdd..b3252a3 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -35,7 +35,7 @@
// "HostName", // Isn't actually a Redfish URI
"Image",
"MetricProperty",
- "OriginOfCondition",
+ // "OriginOfCondition", // Is URI when in request, but is object in response
"TaskMonitor",
"target", // normal string, but target URI for POST to invoke an action
};
@@ -191,15 +191,10 @@
static inline boost::system::error_code
aggregationRetryHandler(unsigned int respCode)
{
- // As a default, assume 200X is alright.
- // We don't need to retry on a 404
- if ((respCode < 200) || ((respCode >= 300) && (respCode != 404)))
- {
- return boost::system::errc::make_error_code(
- boost::system::errc::result_out_of_range);
- }
-
- // Return 0 if the response code is valid
+ // Allow all response codes because we want to surface any satellite
+ // issue to the client
+ BMCWEB_LOG_DEBUG << "Received " << respCode
+ << " response from satellite";
return boost::system::errc::make_error_code(
boost::system::errc::success);
}
@@ -584,6 +579,19 @@
}
}
+ public:
+ RedfishAggregator(const RedfishAggregator&) = delete;
+ RedfishAggregator& operator=(const RedfishAggregator&) = delete;
+ RedfishAggregator(RedfishAggregator&&) = delete;
+ RedfishAggregator& operator=(RedfishAggregator&&) = delete;
+ ~RedfishAggregator() = default;
+
+ static RedfishAggregator& getInstance()
+ {
+ static RedfishAggregator handler;
+ return handler;
+ }
+
// Processes the response returned by a satellite BMC and loads its
// contents into asyncResp
static void
@@ -591,14 +599,7 @@
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
crow::Response& resp)
{
- // No processing needed if the request wasn't successful
- if (resp.resultInt() != 200)
- {
- BMCWEB_LOG_DEBUG << "No need to parse satellite response";
- asyncResp->res.stringResponse = std::move(resp.stringResponse);
- return;
- }
-
+ // We want to attempt prefix fixing regardless of response code
// The resp will not have a json component
// We need to create a json from resp's stringResponse
if (resp.getHeaderValue("Content-Type") == "application/json")
@@ -614,9 +615,6 @@
BMCWEB_LOG_DEBUG << "Successfully parsed satellite response";
- // TODO: For collections we want to add the satellite responses to
- // our response rather than just straight overwriting them if our
- // local handling was successful (i.e. would return a 200).
addPrefixes(jsonVal, prefix);
BMCWEB_LOG_DEBUG << "Added prefix to parsed satellite response";
@@ -630,8 +628,8 @@
{
if (!resp.body().empty())
{
- // We received a 200 response without the correct Content-Type
- // so return an Operation Failed error
+ // We received a valid response without the correct
+ // Content-Type so return an Operation Failed error
BMCWEB_LOG_ERROR
<< "Satellite response must be of type \"application/json\"";
messages::operationFailed(asyncResp->res);
@@ -762,19 +760,6 @@
}
} // End processCollectionResponse()
- public:
- RedfishAggregator(const RedfishAggregator&) = delete;
- RedfishAggregator& operator=(const RedfishAggregator&) = delete;
- RedfishAggregator(RedfishAggregator&&) = delete;
- RedfishAggregator& operator=(RedfishAggregator&&) = delete;
- ~RedfishAggregator() = default;
-
- static RedfishAggregator& getInstance()
- {
- static RedfishAggregator handler;
- return handler;
- }
-
// Entry point to Redfish Aggregation
// Returns Result stating whether or not we still need to locally handle the
// request
diff --git a/test/redfish-core/include/redfish_aggregator_test.cpp b/test/redfish-core/include/redfish_aggregator_test.cpp
index 1b3f924..665446c 100644
--- a/test/redfish-core/include/redfish_aggregator_test.cpp
+++ b/test/redfish-core/include/redfish_aggregator_test.cpp
@@ -1,9 +1,13 @@
#include "redfish_aggregator.hpp"
+#include <nlohmann/json.hpp>
+
#include <gtest/gtest.h> // IWYU pragma: keep
namespace redfish
{
+namespace
+{
TEST(IsPropertyUri, SupportedPropertyReturnsTrue)
{
@@ -11,7 +15,6 @@
EXPECT_TRUE(isPropertyUri("@odata.id"));
EXPECT_TRUE(isPropertyUri("Image"));
EXPECT_TRUE(isPropertyUri("MetricProperty"));
- EXPECT_TRUE(isPropertyUri("OriginOfCondition"));
EXPECT_TRUE(isPropertyUri("TaskMonitor"));
EXPECT_TRUE(isPropertyUri("target"));
}
@@ -29,6 +32,7 @@
EXPECT_FALSE(isPropertyUri("@odata.context"));
EXPECT_FALSE(isPropertyUri("Destination"));
EXPECT_FALSE(isPropertyUri("HostName"));
+ EXPECT_FALSE(isPropertyUri("OriginOfCondition"));
}
TEST(IsPropertyUri, UnsupportedPropertyReturnsFalse)
@@ -136,4 +140,105 @@
EXPECT_EQ(jsonRequest["@odata.id"], initial);
}
}
+
+TEST(addPrefixes, ParseJsonObject)
+{
+ nlohmann::json parameter;
+ parameter["Name"] = "/redfish/v1/Chassis/fakeName";
+ parameter["@odata.id"] = "/redfish/v1/Chassis/fakeChassis";
+
+ addPrefixes(parameter, "abcd");
+ EXPECT_EQ(parameter["Name"], "/redfish/v1/Chassis/fakeName");
+ EXPECT_EQ(parameter["@odata.id"], "/redfish/v1/Chassis/abcd_fakeChassis");
+}
+
+TEST(addPrefixes, ParseJsonArray)
+{
+ nlohmann::json array = nlohmann::json::parse(R"(
+ {
+ "Conditions": [
+ {
+ "Message": "This is a test",
+ "@odata.id": "/redfish/v1/Chassis/TestChassis"
+ },
+ {
+ "Message": "This is also a test",
+ "@odata.id": "/redfish/v1/Chassis/TestChassis2"
+ }
+ ]
+ }
+ )",
+ nullptr, false);
+
+ addPrefixes(array, "5B42");
+ EXPECT_EQ(array["Conditions"][0]["@odata.id"],
+ "/redfish/v1/Chassis/5B42_TestChassis");
+ EXPECT_EQ(array["Conditions"][1]["@odata.id"],
+ "/redfish/v1/Chassis/5B42_TestChassis2");
+}
+
+TEST(addPrefixes, ParseJsonObjectNestedArray)
+{
+ nlohmann::json objWithArray = nlohmann::json::parse(R"(
+ {
+ "Status": {
+ "Conditions": [
+ {
+ "Message": "This is a test",
+ "MessageId": "Test",
+ "OriginOfCondition": {
+ "@odata.id": "/redfish/v1/Chassis/TestChassis"
+ },
+ "Severity": "Critical"
+ }
+ ],
+ "Health": "Critical",
+ "State": "Enabled"
+ }
+ }
+ )",
+ nullptr, false);
+
+ addPrefixes(objWithArray, "5B42");
+ nlohmann::json& array = objWithArray["Status"]["Conditions"];
+ EXPECT_EQ(array[0]["OriginOfCondition"]["@odata.id"],
+ "/redfish/v1/Chassis/5B42_TestChassis");
+}
+
+// Attempts to perform prefix fixing on a response with response code "result".
+// Fixing should always occur
+void assertProcessResponse(unsigned result)
+{
+ nlohmann::json jsonResp;
+ jsonResp["@odata.id"] = "/redfish/v1/Chassis/TestChassis";
+ jsonResp["Name"] = "Test";
+
+ crow::Response resp;
+ resp.body() =
+ jsonResp.dump(2, ' ', true, nlohmann::json::error_handler_t::replace);
+ resp.addHeader("Content-Type", "application/json");
+ resp.result(result);
+
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ RedfishAggregator::processResponse("prefix", asyncResp, resp);
+
+ EXPECT_EQ(asyncResp->res.jsonValue["Name"], "Test");
+ EXPECT_EQ(asyncResp->res.jsonValue["@odata.id"],
+ "/redfish/v1/Chassis/prefix_TestChassis");
+ EXPECT_EQ(asyncResp->res.resultInt(), result);
+}
+
+TEST(processResponse, validResponseCodes)
+{
+ assertProcessResponse(100);
+ assertProcessResponse(200);
+ assertProcessResponse(204);
+ assertProcessResponse(300);
+ assertProcessResponse(404);
+ assertProcessResponse(405);
+ assertProcessResponse(500);
+ assertProcessResponse(507);
+}
+
+} // namespace
} // namespace redfish