Handle NTPServers list per the specification
The Redfish specification for PATCH of arrays defines a number of
requirements.
- Setting a value to null, should remove it from the list.
- Setting a value to empty object "{}" should leave the value unmodified
- Values at indexes larger than whats included in the PATCH request
shall be removed.
This commit attempts to fix this behavior for NTPServers and make it
correct. It does this by first getting the list of NTP servers, then
walking the list in parallel with the list given in the patch, and
either modifying or changing the list as the spec requires before
setting the setting across the system.
It also turns out that the current behavior of unpacking nlohmann::json
objects requires an object to be an array, object, or null, which
doesn't allow unpacking the strings required in this case, so that check
is removed. A quick inspection shows that we don't unpack nlohmann
objects very often, and this should have no impact.
Tested:
Redfish-protocol-validator tests for NTPServers now pass
'''
curl -vvvv --insecure --user root:0penBmc https://192.168.7.2/redfish/v1/Managers/bmc/NetworkProtocol -X PATCH -d '{"NTP": {"NTPServers": []}}'
'''
Used to patch values succeeds with various "good" values;
["time-a-b.nist.gov", "time-b-b.nist.gov"]
[{}, {}]
["time-a-b.nist.gov", null]
[]
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I23a8febde34817bb0b934e46e2b77ff391b52a57
diff --git a/redfish-core/include/utils/json_utils.hpp b/redfish-core/include/utils/json_utils.hpp
index de608df..3ae57fb 100644
--- a/redfish-core/include/utils/json_utils.hpp
+++ b/redfish-core/include/utils/json_utils.hpp
@@ -164,14 +164,6 @@
else if constexpr (std::is_same_v<nlohmann::json, Type>)
{
- // Must be a complex type. Simple types (int string etc) should be
- // unpacked directly
- if (!jsonValue.is_object() && !jsonValue.is_array() &&
- !jsonValue.is_null())
- {
- return UnpackErrorCode::invalidType;
- }
-
value = std::move(jsonValue);
}
else
diff --git a/redfish-core/lib/network_protocol.hpp b/redfish-core/lib/network_protocol.hpp
index 1345677..dd24a40 100644
--- a/redfish-core/lib/network_protocol.hpp
+++ b/redfish-core/lib/network_protocol.hpp
@@ -241,22 +241,86 @@
inline void
handleNTPServersPatch(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
- std::vector<std::string>& ntpServers)
+ const std::vector<nlohmann::json>& ntpServerObjects,
+ std::vector<std::string> currentNtpServers)
{
- auto iter = stl_utils::firstDuplicate(ntpServers.begin(), ntpServers.end());
- if (iter != ntpServers.end())
+ std::vector<std::string>::iterator currentNtpServer =
+ currentNtpServers.begin();
+ for (size_t index = 0; index < ntpServerObjects.size(); index++)
{
- std::string pointer =
- "NTPServers/" +
- std::to_string(std::distance(ntpServers.begin(), iter));
- messages::propertyValueIncorrect(asyncResp->res, pointer, *iter);
- return;
+ const nlohmann::json& ntpServer = ntpServerObjects[index];
+ if (ntpServer.is_null())
+ {
+ // Can't delete an item that doesn't exist
+ if (currentNtpServer == currentNtpServers.end())
+ {
+ messages::propertyValueNotInList(asyncResp->res, "null",
+ "NTP/NTPServers/" +
+ std::to_string(index));
+
+ return;
+ }
+ currentNtpServer = currentNtpServers.erase(currentNtpServer);
+ continue;
+ }
+ const nlohmann::json::object_t* ntpServerObject =
+ ntpServer.get_ptr<const nlohmann::json::object_t*>();
+ if (ntpServerObject != nullptr)
+ {
+ if (!ntpServerObject->empty())
+ {
+ messages::propertyValueNotInList(
+ asyncResp->res,
+ ntpServer.dump(2, ' ', true,
+ nlohmann::json::error_handler_t::replace),
+ "NTP/NTPServers/" + std::to_string(index));
+ return;
+ }
+ // Can't retain an item that doesn't exist
+ if (currentNtpServer == currentNtpServers.end())
+ {
+ messages::propertyValueOutOfRange(
+ asyncResp->res,
+ ntpServer.dump(2, ' ', true,
+ nlohmann::json::error_handler_t::replace),
+ "NTP/NTPServers/" + std::to_string(index));
+
+ return;
+ }
+ // empty objects should leave the NtpServer unmodified
+ currentNtpServer++;
+ continue;
+ }
+
+ const std::string* ntpServerStr =
+ ntpServer.get_ptr<const std::string*>();
+ if (ntpServerStr == nullptr)
+ {
+ messages::propertyValueTypeError(
+ asyncResp->res,
+ ntpServer.dump(2, ' ', true,
+ nlohmann::json::error_handler_t::replace),
+ "NTP/NTPServers/" + std::to_string(index));
+ return;
+ }
+ if (currentNtpServer == currentNtpServers.end())
+ {
+ // if we're at the end of the list, append to the end
+ currentNtpServers.push_back(*ntpServerStr);
+ currentNtpServer = currentNtpServers.end();
+ continue;
+ }
+ *currentNtpServer = *ntpServerStr;
+ currentNtpServer++;
}
+ // Any remaining array elements should be removed
+ currentNtpServers.erase(currentNtpServer, currentNtpServers.end());
+
crow::connections::systemBus->async_method_call(
- [asyncResp,
- ntpServers](boost::system::error_code ec,
- const dbus::utility::MapperGetSubTreeResponse& subtree) {
+ [asyncResp, currentNtpServers](
+ boost::system::error_code ec,
+ const dbus::utility::MapperGetSubTreeResponse& subtree) {
if (ec)
{
BMCWEB_LOG_WARNING << "D-Bus error: " << ec << ", " << ec.message();
@@ -286,7 +350,7 @@
},
service, objectPath, "org.freedesktop.DBus.Properties",
"Set", interface, "NTPServers",
- dbus::utility::DbusVariantType{ntpServers});
+ dbus::utility::DbusVariantType{currentNtpServers});
}
}
}
@@ -413,7 +477,7 @@
return;
}
std::optional<std::string> newHostName;
- std::optional<std::vector<std::string>> ntpServers;
+ std::optional<std::vector<nlohmann::json>> ntpServerObjects;
std::optional<bool> ntpEnabled;
std::optional<bool> ipmiEnabled;
std::optional<bool> sshEnabled;
@@ -422,7 +486,7 @@
if (!json_util::readJsonPatch(
req, asyncResp->res,
"HostName", newHostName,
- "NTP/NTPServers", ntpServers,
+ "NTP/NTPServers", ntpServerObjects,
"NTP/ProtocolEnabled", ntpEnabled,
"IPMI/ProtocolEnabled", ipmiEnabled,
"SSH/ProtocolEnabled", sshEnabled))
@@ -442,10 +506,21 @@
{
handleNTPProtocolEnabled(*ntpEnabled, asyncResp);
}
- if (ntpServers)
+ if (ntpServerObjects)
{
- stl_utils::removeDuplicate(*ntpServers);
- handleNTPServersPatch(asyncResp, *ntpServers);
+ getEthernetIfaceData(
+ [asyncResp, ntpServerObjects](
+ const bool success,
+ std::vector<std::string>& currentNtpServers,
+ const std::vector<std::string>& /*domainNames*/) {
+ if (!success)
+ {
+ messages::internalError(asyncResp->res);
+ return;
+ }
+ handleNTPServersPatch(asyncResp, *ntpServerObjects,
+ std::move(currentNtpServers));
+ });
}
if (ipmiEnabled)