From 11987af6e58a2d341c97f752e90c929e0a1189e5 Mon Sep 17 00:00:00 2001 From: Carson Labrado Date: Wed, 23 Nov 2022 20:55:18 +0000 Subject: Aggregation: Improve prefix fixup matching 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/. 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 Change-Id: I676983d3c77ae3126c04e9f57ad8698c51df2675 --- redfish-core/include/aggregation_utils.hpp | 90 ++++++++++---------- redfish-core/include/redfish_aggregator.hpp | 85 ++++++++++--------- scripts/generate_schema_collections.py | 6 +- .../include/redfish_aggregator_test.cpp | 98 ++++++++++++++++++++++ 4 files changed, 192 insertions(+), 87 deletions(-) diff --git a/redfish-core/include/aggregation_utils.hpp b/redfish-core/include/aggregation_utils.hpp index bb1782f1b0..673b56873c 100644 --- a/redfish-core/include/aggregation_utils.hpp +++ b/redfish-core/include/aggregation_utils.hpp @@ -15,52 +15,52 @@ namespace redfish { -// Note that each URI actually begins with "/redfish/v1/" +// Note that each URI actually begins with "/redfish/v1" // They've been omitted to save space and reduce search time constexpr std::array topCollections{ - "AggregationService/Aggregates", - "AggregationService/AggregationSources", - "AggregationService/ConnectionMethods", - "Cables", - "Chassis", - "ComponentIntegrity", - "CompositionService/ActivePool", - "CompositionService/CompositionReservations", - "CompositionService/FreePool", - "CompositionService/ResourceBlocks", - "CompositionService/ResourceZones", - "EventService/Subscriptions", - "Fabrics", - "Facilities", - "JobService/Jobs", - "JobService/Log/Entries", - "KeyService/NVMeoFKeyPolicies", - "KeyService/NVMeoFSecrets", - "LicenseService/Licenses", - "Managers", - "NVMeDomains", - "PowerEquipment/ElectricalBuses", - "PowerEquipment/FloorPDUs", - "PowerEquipment/PowerShelves", - "PowerEquipment/RackPDUs", - "PowerEquipment/Switchgear", - "PowerEquipment/TransferSwitches", - "RegisteredClients", - "Registries", - "ResourceBlocks", - "Storage", - "StorageServices", - "StorageSystems", - "Systems", - "TaskService/Tasks", - "TelemetryService/LogService/Entries", - "TelemetryService/MetricDefinitions", - "TelemetryService/MetricReportDefinitions", - "TelemetryService/MetricReports", - "TelemetryService/Triggers", - "UpdateService/ClientCertificates", - "UpdateService/FirmwareInventory", - "UpdateService/RemoteServerCertificates", - "UpdateService/SoftwareInventory", + "/AggregationService/Aggregates", + "/AggregationService/AggregationSources", + "/AggregationService/ConnectionMethods", + "/Cables", + "/Chassis", + "/ComponentIntegrity", + "/CompositionService/ActivePool", + "/CompositionService/CompositionReservations", + "/CompositionService/FreePool", + "/CompositionService/ResourceBlocks", + "/CompositionService/ResourceZones", + "/EventService/Subscriptions", + "/Fabrics", + "/Facilities", + "/JobService/Jobs", + "/JobService/Log/Entries", + "/KeyService/NVMeoFKeyPolicies", + "/KeyService/NVMeoFSecrets", + "/LicenseService/Licenses", + "/Managers", + "/NVMeDomains", + "/PowerEquipment/ElectricalBuses", + "/PowerEquipment/FloorPDUs", + "/PowerEquipment/PowerShelves", + "/PowerEquipment/RackPDUs", + "/PowerEquipment/Switchgear", + "/PowerEquipment/TransferSwitches", + "/RegisteredClients", + "/Registries", + "/ResourceBlocks", + "/Storage", + "/StorageServices", + "/StorageSystems", + "/Systems", + "/TaskService/Tasks", + "/TelemetryService/LogService/Entries", + "/TelemetryService/MetricDefinitions", + "/TelemetryService/MetricReportDefinitions", + "/TelemetryService/MetricReports", + "/TelemetryService/Triggers", + "/UpdateService/ClientCertificates", + "/UpdateService/FirmwareInventory", + "/UpdateService/RemoteServerCertificates", + "/UpdateService/SoftwareInventory", }; } // namespace redfish diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp index 293dad2491..bd56d680fd 100644 --- a/redfish-core/include/redfish_aggregator.hpp +++ b/redfish-core/include/redfish_aggregator.hpp @@ -72,53 +72,66 @@ static void addPrefixToItem(nlohmann::json& item, std::string_view prefix) // version mismatches between aggregator and satellite BMCs. For now // assume that the aggregator has all the schemas and versions that the // aggregated server has. - std::string collectionItem; if (crow::utility::readUrlSegments(thisUrl, "redfish", "v1", "JsonSchemas", - std::ref(collectionItem), crow::utility::OrMorePaths())) { BMCWEB_LOG_DEBUG << "Skipping JsonSchemas URI prefix fixing"; return; } - // We don't need to add prefixes to these URIs since - // /redfish/v1/UpdateService/ itself is not a collection: - // /redfish/v1/UpdateService/FirmwareInventory - // /redfish/v1/UpdateService/SoftwareInventory - if (crow::utility::readUrlSegments(thisUrl, "redfish", "v1", - "UpdateService", "FirmwareInventory") || - crow::utility::readUrlSegments(thisUrl, "redfish", "v1", - "UpdateService", "SoftwareInventory")) + // The first two segments should be "/redfish/v1". We need to check that + // before we can search topCollections + if (!crow::utility::readUrlSegments(thisUrl, "redfish", "v1", + crow::utility::OrMorePaths())) { - BMCWEB_LOG_DEBUG << "Skipping UpdateService URI prefix fixing"; return; } - // We need to add a prefix to FirmwareInventory and SoftwareInventory - // resources: - // /redfish/v1/UpdateService/FirmwareInventory/ - // /redfish/v1/UpdateService/SoftwareInventory/ - std::string collectionName; - if (crow::utility::readUrlSegments( - thisUrl, "redfish", "v1", "UpdateService", std::ref(collectionName), - std::ref(collectionItem), crow::utility::OrMorePaths())) + // Check array adding a segment each time until collection is identified + // Add prefix to segment after the collection + const boost::urls::segments_view urlSegments = thisUrl.segments(); + bool addedPrefix = false; + boost::urls::url url("/"); + boost::urls::segments_view::iterator it = urlSegments.begin(); + const boost::urls::segments_view::const_iterator end = urlSegments.end(); + + // Skip past the leading "/redfish/v1" + it++; + it++; + for (; it != end; it++) { - collectionItem.insert(0, "_"); - collectionItem.insert(0, prefix); - item = crow::utility::replaceUrlSegment(thisUrl, 4, collectionItem); - return; + // Trailing "/" will result in an empty segment. In that case we need + // to return so we don't apply a prefix to top level collections such + // as "/redfish/v1/Chassis/" + if ((*it).empty()) + { + return; + } + + if (std::binary_search(topCollections.begin(), topCollections.end(), + url.buffer())) + { + std::string collectionItem(prefix); + collectionItem += "_" + (*it); + url.segments().push_back(collectionItem); + it++; + addedPrefix = true; + break; + } + + url.segments().push_back(*it); + } + + // Finish constructing the URL here (if needed) to avoid additional checks + for (; it != end; it++) + { + url.segments().push_back(*it); } - // If we reach here then we need to add a prefix to resource IDs that take - // the general form of "/redfish/v1// such as: - // /redfish/v1/Chassis/foo - if (crow::utility::readUrlSegments( - thisUrl, "redfish", "v1", std::ref(collectionName), - std::ref(collectionItem), crow::utility::OrMorePaths())) + if (addedPrefix) { - collectionItem.insert(0, "_"); - collectionItem.insert(0, prefix); - item = crow::utility::replaceUrlSegment(thisUrl, 3, collectionItem); + url.segments().insert(url.segments().begin(), {"redfish", "v1"}); + item = url; } } @@ -164,12 +177,6 @@ class RedfishAggregator RedfishAggregator() { - // TODO: Remove in future patches, this is here to prevent compiler - // warnings and get an accurate idea of the increase in the size of the - // binary. - BMCWEB_LOG_DEBUG << "There are " << topCollections.size() - << " top level collections:"; - getSatelliteConfigs(constructorCallback); // Setup the retry policy to be used by Redfish Aggregation @@ -776,7 +783,7 @@ class RedfishAggregator { using crow::utility::OrMorePaths; using crow::utility::readUrlSegments; - const boost::urls::url_view& url = thisReq.urlView; + const boost::urls::url_view url = thisReq.urlView; // UpdateService is the only top level resource that is not a Collection if (readUrlSegments(url, "redfish", "v1", "UpdateService")) { diff --git a/scripts/generate_schema_collections.py b/scripts/generate_schema_collections.py index c9f8b5956d..d6efa0efe2 100755 --- a/scripts/generate_schema_collections.py +++ b/scripts/generate_schema_collections.py @@ -266,17 +266,17 @@ def generate_top_collections(): "\n" "namespace redfish\n" "{{\n" - '// Note that each URI actually begins with "/redfish/v1/"\n' + '// Note that each URI actually begins with "/redfish/v1"\n' "// They've been omitted to save space and reduce search time\n" "constexpr std::array " "topCollections{{\n".format(WARNING=WARNING, TOTAL=TOTAL) ) for collection in sorted(top_collections): - # All URIs start with "/redfish/v1/". We can omit that portion to + # All URIs start with "/redfish/v1". We can omit that portion to # save memory and reduce lookup time hpp_file.write( - ' "{}",\n'.format(collection.split("/redfish/v1/")[1]) + ' "{}",\n'.format(collection.split("/redfish/v1")[1]) ) hpp_file.write("};\n} // namespace redfish\n") diff --git a/test/redfish-core/include/redfish_aggregator_test.cpp b/test/redfish-core/include/redfish_aggregator_test.cpp index e5c718148b..1b3f9244be 100644 --- a/test/redfish-core/include/redfish_aggregator_test.cpp +++ b/test/redfish-core/include/redfish_aggregator_test.cpp @@ -38,4 +38,102 @@ TEST(IsPropertyUri, UnsupportedPropertyReturnsFalse) EXPECT_FALSE(isPropertyUri("Id")); } +TEST(addPrefixToItem, ValidURIs) +{ + nlohmann::json jsonRequest; + constexpr std::array validRoots{"Cables", + "Chassis", + "Fabrics", + "PowerEquipment/FloorPDUs", + "Systems", + "TaskService/Tasks", + "TelemetryService/LogService/Entries", + "UpdateService/SoftwareInventory"}; + + // We're only testing prefix fixing so it's alright that some of the + // resulting URIs will not actually be possible as defined by the schema + constexpr std::array validIDs{"1", + "1/", + "Test", + "Test/", + "Extra_Test", + "Extra_Test/", + "Extra_Test/Sensors", + "Extra_Test/Sensors/", + "Extra_Test/Sensors/power_sensor", + "Extra_Test/Sensors/power_sensor/"}; + + // Construct URIs which should have prefix fixing applied + for (const auto& root : validRoots) + { + for (const auto& id : validIDs) + { + std::string initial("/redfish/v1/" + std::string(root) + "/"); + std::string correct(initial + "asdfjkl_" + std::string(id)); + initial += id; + jsonRequest["@odata.id"] = initial; + addPrefixToItem(jsonRequest["@odata.id"], "asdfjkl"); + EXPECT_EQ(jsonRequest["@odata.id"], correct); + } + } +} + +TEST(addPrefixToItem, UnsupportedURIs) +{ + nlohmann::json jsonRequest; + constexpr std::array invalidRoots{ + "FakeCollection", "JsonSchemas", + "PowerEquipment", "TaskService", + "TelemetryService/Entries", "UpdateService"}; + + constexpr std::array validIDs{"1", + "1/", + "Test", + "Test/", + "Extra_Test", + "Extra_Test/", + "Extra_Test/Sensors", + "Extra_Test/Sensors/", + "Extra_Test/Sensors/power_sensor", + "Extra_Test/Sensors/power_sensor/"}; + + // Construct URIs which should NOT have prefix fixing applied + for (const auto& root : invalidRoots) + { + for (const auto& id : validIDs) + { + std::string initial("/redfish/v1/" + std::string(root) + "/"); + std::string correct(initial + "asdfjkl_" + std::string(id)); + initial += id; + jsonRequest["@odata.id"] = initial; + addPrefixToItem(jsonRequest["@odata.id"], "asdfjkl"); + EXPECT_EQ(jsonRequest["@odata.id"], initial); + } + } +} + +TEST(addPrefixToItem, TopLevelCollections) +{ + nlohmann::json jsonRequest; + constexpr std::array validRoots{"Cables", + "Chassis/", + "Fabrics", + "JsonSchemas", + "PowerEquipment/FloorPDUs", + "Systems", + "TaskService/Tasks", + "TelemetryService/LogService/Entries", + "TelemetryService/LogService/Entries/", + "UpdateService/SoftwareInventory/"}; + + // Construct URIs for top level collections. Prefixes should NOT be + // applied to any of the URIs + for (const auto& root : validRoots) + { + std::string initial("/redfish/v1/" + std::string(root)); + jsonRequest["@odata.id"] = initial; + addPrefixToItem(jsonRequest["@odata.id"], "perfix"); + EXPECT_EQ(jsonRequest["@odata.id"], initial); + } +} } // namespace redfish -- cgit v1.2.3