Fix setting gateways
There's a number of conditions in setting gateways that don't work
properly. Specifically, one of the issues is setting a gateway on an
address that already exists. It returns a PropertyValueConflict error
on Ipv4Addresses/1/Gateway with Ipv4Addresses/1/Gateway
Obviously an address can't conflict with itself, so this is wrong.
To address this, move the gateway setting and selection code into a
routine outside of the main loop, after all the gateways are accounted
for, and so we can treat them separately.
Tested;
PATCH to an existing ip address works, and no longer returns the error.
More test cases likely needed.
Change-Id: I0339e02fc27164337416637153d0b0f744b64ad8
Signed-off-by: Ed Tanous <etanous@nvidia.com>
diff --git a/meson.build b/meson.build
index d3833bc..1e413cb 100644
--- a/meson.build
+++ b/meson.build
@@ -441,17 +441,18 @@
'test/redfish-core/include/filter_expr_parser_test.cpp',
'test/redfish-core/include/privileges_test.cpp',
'test/redfish-core/include/redfish_aggregator_test.cpp',
+ 'test/redfish-core/include/redfish_test.cpp',
'test/redfish-core/include/registries_test.cpp',
'test/redfish-core/include/utils/dbus_utils.cpp',
'test/redfish-core/include/utils/hex_utils_test.cpp',
'test/redfish-core/include/utils/ip_utils_test.cpp',
'test/redfish-core/include/utils/json_utils_test.cpp',
- 'test/redfish-core/include/redfish_test.cpp',
'test/redfish-core/include/utils/query_param_test.cpp',
'test/redfish-core/include/utils/sensor_utils_test.cpp',
'test/redfish-core/include/utils/stl_utils_test.cpp',
'test/redfish-core/include/utils/time_utils_test.cpp',
'test/redfish-core/lib/chassis_test.cpp',
+ 'test/redfish-core/lib/ethernet_test.cpp',
'test/redfish-core/lib/log_services_dump_test.cpp',
'test/redfish-core/lib/manager_diagnostic_data_test.cpp',
'test/redfish-core/lib/metadata_test.cpp',
diff --git a/redfish-core/lib/ethernet.hpp b/redfish-core/lib/ethernet.hpp
index a0dcd54..c6bdfe0 100644
--- a/redfish-core/lib/ethernet.hpp
+++ b/redfish-core/lib/ethernet.hpp
@@ -1482,85 +1482,74 @@
});
}
-inline void handleIPv4StaticPatch(
- const std::string& ifaceId,
- std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>>& input,
- const EthernetInterfaceData& ethData,
- const std::vector<IPv4AddressData>& ipv4Data,
- const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
+enum class AddrChange
{
- unsigned entryIdx = 1;
- // Find the first static IP address currently active on the NIC and
- // match it to the first JSON element in the IPv4StaticAddresses array.
- // Match each subsequent JSON element to the next static IP programmed
- // into the NIC.
+ Noop,
+ Delete,
+ Update,
+};
+
+// Struct representing a dbus change
+struct AddressPatch
+{
+ std::string address;
+ std::string gateway;
+ uint8_t prefixLength = 0;
+ std::string existingDbusId;
+ AddrChange operation = AddrChange::Noop;
+};
+
+inline bool parseAddresses(
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>>& input,
+ const std::vector<IPv4AddressData>& ipv4Data, crow::Response& res,
+ std::vector<AddressPatch>& addressesOut, std::string& gatewayOut)
+{
std::vector<IPv4AddressData>::const_iterator nicIpEntry =
getNextStaticIpEntry(ipv4Data.cbegin(), ipv4Data.cend());
- bool gatewayValueAssigned{};
- bool preserveGateway{};
- std::string activePath{};
- std::string activeGateway{};
- if (!ethData.defaultGateway.empty() && ethData.defaultGateway != "0.0.0.0")
- {
- // The NIC is already configured with a default gateway. Use this if
- // the leading entry in the PATCH is '{}', which is preserving an active
- // static address.
- activeGateway = ethData.defaultGateway;
- activePath = "IPv4StaticAddresses/1";
- gatewayValueAssigned = true;
- }
-
+ std::string lastGatewayPath;
+ size_t entryIdx = 0;
for (std::variant<nlohmann::json::object_t, std::nullptr_t>& thisJson :
input)
{
std::string pathString =
- "IPv4StaticAddresses/" + std::to_string(entryIdx);
+ std::format("IPv4StaticAddresses/{}", entryIdx);
+ AddressPatch& thisAddress = addressesOut.emplace_back();
nlohmann::json::object_t* obj =
std::get_if<nlohmann::json::object_t>(&thisJson);
- if (obj == nullptr)
+ if (nicIpEntry != ipv4Data.cend())
{
- if (nicIpEntry != ipv4Data.cend())
- {
- deleteIPAddress(ifaceId, nicIpEntry->id, asyncResp);
- nicIpEntry =
- getNextStaticIpEntry(++nicIpEntry, ipv4Data.cend());
- if (!preserveGateway && (nicIpEntry == ipv4Data.cend()))
- {
- // All entries have been processed, and this last has
- // requested the IP address be deleted. No prior entry
- // performed an action that created or modified a
- // gateway. Deleting this IP address means the default
- // gateway entry has to be removed as well.
- updateIPv4DefaultGateway(ifaceId, "", asyncResp);
- }
- entryIdx++;
- continue;
- }
- // Received a DELETE action on an entry not assigned to the NIC
- messages::resourceCannotBeDeleted(asyncResp->res);
- return;
+ thisAddress.existingDbusId = nicIpEntry->id;
}
- // An Add/Modify action is requested
- if (!obj->empty())
+ if (obj == nullptr)
+ {
+ if (thisAddress.existingDbusId.empty())
+ {
+ // Received a DELETE action on an entry not assigned to the NIC
+ messages::resourceCannotBeDeleted(res);
+ return false;
+ }
+ thisAddress.operation = AddrChange::Delete;
+ }
+ else
{
std::optional<std::string> address;
- std::optional<std::string> subnetMask;
std::optional<std::string> gateway;
-
- if (!json_util::readJsonObject( //
- *obj, asyncResp->res, //
- "Address", address, //
- "Gateway", gateway, //
- "SubnetMask", subnetMask //
- ))
+ std::optional<std::string> subnetMask;
+ if (!obj->empty())
{
- messages::propertyValueFormatError(asyncResp->res, *obj,
- pathString);
- return;
+ if (!json_util::readJsonObject( //
+ *obj, res, //
+ "Address", address, //
+ "Gateway", gateway, //
+ "SubnetMask", subnetMask //
+ ))
+ {
+ messages::propertyValueFormatError(res, *obj, pathString);
+ return false;
+ }
}
-
// Find the address/subnet/gateway values. Any values that are
// not explicitly provided are assumed to be unmodified from the
// current state of the interface. Merge existing state into the
@@ -1569,131 +1558,178 @@
{
if (!ip_util::ipv4VerifyIpAndGetBitcount(*address))
{
- messages::propertyValueFormatError(asyncResp->res, *address,
+ messages::propertyValueFormatError(res, *address,
pathString + "/Address");
- return;
+ return false;
}
+ thisAddress.operation = AddrChange::Update;
+ thisAddress.address = *address;
}
- else if (nicIpEntry != ipv4Data.cend())
+ else if (thisAddress.existingDbusId.empty())
{
- address = (nicIpEntry->address);
+ messages::propertyMissing(res, pathString + "/Address");
+ return false;
}
else
{
- messages::propertyMissing(asyncResp->res,
- pathString + "/Address");
- return;
+ thisAddress.address = nicIpEntry->address;
}
- uint8_t prefixLength = 0;
if (subnetMask)
{
+ uint8_t prefixLength = 0;
if (!ip_util::ipv4VerifyIpAndGetBitcount(*subnetMask,
&prefixLength))
{
messages::propertyValueFormatError(
- asyncResp->res, *subnetMask,
- pathString + "/SubnetMask");
- return;
+ res, *subnetMask, pathString + "/SubnetMask");
+ return false;
}
+ thisAddress.prefixLength = prefixLength;
+ thisAddress.operation = AddrChange::Update;
}
- else if (nicIpEntry != ipv4Data.cend())
+ else if (thisAddress.existingDbusId.empty())
{
- if (!ip_util::ipv4VerifyIpAndGetBitcount(nicIpEntry->netmask,
- &prefixLength))
- {
- messages::propertyValueFormatError(
- asyncResp->res, nicIpEntry->netmask,
- pathString + "/SubnetMask");
- return;
- }
+ messages::propertyMissing(res, pathString + "/SubnetMask");
+ return false;
}
else
{
- messages::propertyMissing(asyncResp->res,
- pathString + "/SubnetMask");
- return;
- }
+ uint8_t prefixLength = 0;
+ // Ignore return code. It came from internal, it's it's invalid
+ // nothing we can do
+ ip_util::ipv4VerifyIpAndGetBitcount(nicIpEntry->netmask,
+ &prefixLength);
+ thisAddress.prefixLength = prefixLength;
+ }
if (gateway)
{
if (!ip_util::ipv4VerifyIpAndGetBitcount(*gateway))
{
- messages::propertyValueFormatError(asyncResp->res, *gateway,
+ messages::propertyValueFormatError(res, *gateway,
pathString + "/Gateway");
- return;
+ return false;
}
+ thisAddress.operation = AddrChange::Update;
+ thisAddress.gateway = *gateway;
}
- else if (nicIpEntry != ipv4Data.cend())
+ else if (thisAddress.existingDbusId.empty())
{
- gateway = nicIpEntry->gateway;
+ // Default to null gateway
+ gateway = "";
}
else
{
- messages::propertyMissing(asyncResp->res,
- pathString + "/Gateway");
- return;
+ thisAddress.gateway = nicIpEntry->gateway;
}
- if (gatewayValueAssigned)
+ // Changing gateway from existing
+ if (!thisAddress.gateway.empty() &&
+ thisAddress.gateway != "0.0.0.0")
{
- if (activeGateway != gateway)
+ if (!gatewayOut.empty() && gatewayOut != thisAddress.gateway)
{
// A NIC can only have a single active gateway value.
// If any gateway in the array of static addresses
// mismatch the PATCH is in error.
std::string arg1 = pathString + "/Gateway";
- std::string arg2 = activePath + "/Gateway";
- messages::propertyValueConflict(asyncResp->res, arg1, arg2);
- return;
+ std::string arg2 = lastGatewayPath + "/Gateway";
+ messages::propertyValueConflict(res, arg1, arg2);
+ return false;
+ }
+ gatewayOut = thisAddress.gateway;
+ lastGatewayPath = pathString;
+ }
+ }
+ nicIpEntry++;
+ nicIpEntry = getNextStaticIpEntry(nicIpEntry, ipv4Data.cend());
+ entryIdx++;
+ }
+
+ // Delete the remaining IPs
+ while (nicIpEntry != ipv4Data.cend())
+ {
+ AddressPatch& thisAddress = addressesOut.emplace_back();
+ thisAddress.operation = AddrChange::Delete;
+ thisAddress.existingDbusId = nicIpEntry->id;
+ nicIpEntry++;
+ nicIpEntry = getNextStaticIpEntry(nicIpEntry, ipv4Data.cend());
+ }
+
+ return true;
+}
+
+inline void handleIPv4StaticPatch(
+ const std::string& ifaceId,
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>>& input,
+ const EthernetInterfaceData& ethData,
+ const std::vector<IPv4AddressData>& ipv4Data,
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
+{
+ std::vector<AddressPatch> addresses;
+ std::string gatewayOut;
+ if (!parseAddresses(input, ipv4Data, asyncResp->res, addresses, gatewayOut))
+ {
+ return;
+ }
+
+ // If we're setting the gateway to something new, delete the
+ // existing so we won't conflict
+ if (!ethData.defaultGateway.empty() && ethData.defaultGateway != gatewayOut)
+ {
+ updateIPv4DefaultGateway(ifaceId, "", asyncResp);
+ }
+
+ for (const AddressPatch& address : addresses)
+ {
+ switch (address.operation)
+ {
+ case AddrChange::Delete:
+ {
+ BMCWEB_LOG_ERROR("Deleting id {} on interface {}",
+ address.existingDbusId, ifaceId);
+ deleteIPAddress(ifaceId, address.existingDbusId, asyncResp);
+ }
+ break;
+ case AddrChange::Update:
+ {
+ // Update is a delete then a recreate
+ // Only need to update if there is an existing ip at this index
+ if (!address.existingDbusId.empty())
+ {
+ BMCWEB_LOG_ERROR("Deleting id {} on interface {}",
+ address.existingDbusId, ifaceId);
+ deleteAndCreateIPAddress(
+ IpVersion::IpV4, ifaceId, address.existingDbusId,
+ address.prefixLength, address.address, address.gateway,
+ asyncResp);
+ }
+ else
+ {
+ // Otherwise, just create a new one
+ BMCWEB_LOG_ERROR(
+ "creating ip {} prefix {} gateway {} on interface {}",
+ address.address, address.prefixLength, address.gateway,
+ ifaceId);
+ createIPv4(ifaceId, address.prefixLength, address.gateway,
+ address.address, asyncResp);
}
}
- else
+ break;
+ default:
{
- // Capture the very first gateway value from the incoming
- // JSON record and use it at the default gateway.
- updateIPv4DefaultGateway(ifaceId, *gateway, asyncResp);
- activeGateway = *gateway;
- activePath = pathString;
- gatewayValueAssigned = true;
+ // Leave alone
}
+ break;
+ }
+ }
- if (nicIpEntry != ipv4Data.cend())
- {
- deleteAndCreateIPAddress(IpVersion::IpV4, ifaceId,
- nicIpEntry->id, prefixLength, *address,
- *gateway, asyncResp);
- nicIpEntry =
- getNextStaticIpEntry(++nicIpEntry, ipv4Data.cend());
- preserveGateway = true;
- }
- else
- {
- createIPv4(ifaceId, prefixLength, *gateway, *address,
- asyncResp);
- preserveGateway = true;
- }
- entryIdx++;
- }
- else
- {
- // Received {}, do not modify this address
- if (nicIpEntry != ipv4Data.cend())
- {
- nicIpEntry =
- getNextStaticIpEntry(++nicIpEntry, ipv4Data.cend());
- preserveGateway = true;
- entryIdx++;
- }
- else
- {
- // Requested a DO NOT MODIFY action on an entry not assigned
- // to the NIC
- messages::propertyValueFormatError(asyncResp->res, *obj,
- pathString);
- return;
- }
- }
+ // now update to the new gateway.
+ // Default gateway is already empty, so no need to update if we're clearing
+ if (!gatewayOut.empty() && ethData.defaultGateway != gatewayOut)
+ {
+ updateIPv4DefaultGateway(ifaceId, gatewayOut, asyncResp);
}
}
diff --git a/test/redfish-core/lib/ethernet_test.cpp b/test/redfish-core/lib/ethernet_test.cpp
new file mode 100644
index 0000000..ac4481a
--- /dev/null
+++ b/test/redfish-core/lib/ethernet_test.cpp
@@ -0,0 +1,212 @@
+#include "ethernet.hpp"
+#include "http_response.hpp"
+
+#include <nlohmann/json.hpp>
+
+#include <cstddef>
+#include <string>
+#include <variant>
+#include <vector>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace redfish
+{
+namespace
+{
+
+using ::testing::IsEmpty;
+
+TEST(Ethernet, parseAddressesEmpty)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_TRUE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+ EXPECT_THAT(addrOut, IsEmpty());
+ EXPECT_THAT(gatewayOut, IsEmpty());
+}
+
+// Create full entry with all fields
+TEST(Ethernet, parseAddressesCreateOne)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ eth["Address"] = "1.1.1.2";
+ eth["Gateway"] = "1.1.1.1";
+ eth["SubnetMask"] = "255.255.255.0";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_TRUE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+ EXPECT_EQ(addrOut.size(), 1);
+ EXPECT_EQ(addrOut[0].address, "1.1.1.2");
+ EXPECT_EQ(addrOut[0].gateway, "1.1.1.1");
+ EXPECT_EQ(addrOut[0].prefixLength, 24);
+ EXPECT_EQ(addrOut[0].existingDbusId, "");
+ EXPECT_EQ(addrOut[0].operation, AddrChange::Update);
+ EXPECT_EQ(gatewayOut, "1.1.1.1");
+}
+
+// Missing gateway should default to no gateway
+TEST(Ethernet, parseAddressesCreateOneNoGateway)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ eth["Address"] = "1.1.1.2";
+ eth["SubnetMask"] = "255.255.255.0";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_TRUE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+ EXPECT_EQ(addrOut.size(), 1);
+ EXPECT_EQ(addrOut[0].address, "1.1.1.2");
+ EXPECT_EQ(addrOut[0].gateway, "");
+ EXPECT_EQ(addrOut[0].prefixLength, 24);
+ EXPECT_EQ(addrOut[0].existingDbusId, "");
+ EXPECT_EQ(addrOut[0].operation, AddrChange::Update);
+ EXPECT_EQ(gatewayOut, "");
+}
+
+// Create two entries with conflicting gateways.
+TEST(Ethernet, conflictingGatewaysNew)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ eth["Address"] = "1.1.1.2";
+ eth["Gateway"] = "1.1.1.1";
+ eth["SubnetMask"] = "255.255.255.0";
+ addr.emplace_back(eth);
+ eth["Gateway"] = "1.1.1.5";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_FALSE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+}
+
+// Create full entry with all fields
+TEST(Ethernet, conflictingGatewaysExisting)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ addr.emplace_back(eth);
+ eth["Address"] = "1.1.1.2";
+ eth["Gateway"] = "1.1.1.1";
+ eth["SubnetMask"] = "255.255.255.0";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+ IPv4AddressData& existing = existingAddr.emplace_back();
+ existing.id = "my_ip_id";
+ existing.origin = "Static";
+ existing.gateway = "192.168.1.1";
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_FALSE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+}
+
+// Missing address should fail
+TEST(Ethernet, parseMissingAddress)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ eth["SubnetMask"] = "255.255.255.0";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_FALSE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+}
+
+// Missing subnet should fail
+TEST(Ethernet, parseAddressesMissingSubnet)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ nlohmann::json::object_t eth;
+ eth["Address"] = "1.1.1.2";
+ addr.emplace_back(eth);
+ std::vector<IPv4AddressData> existingAddr;
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_FALSE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+}
+
+// With one existing address, and a null, it should request deletion
+// and clear gateway
+TEST(Ethernet, parseAddressesDeleteExistingOnNull)
+{
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ addr.emplace_back(nullptr);
+ std::vector<IPv4AddressData> existingAddr;
+ IPv4AddressData& existing = existingAddr.emplace_back();
+ existing.id = "my_ip_id";
+ existing.origin = "Static";
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_TRUE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+ EXPECT_EQ(addrOut.size(), 1);
+ EXPECT_EQ(addrOut[0].existingDbusId, "my_ip_id");
+ EXPECT_EQ(addrOut[0].operation, AddrChange::Delete);
+ EXPECT_EQ(gatewayOut, "");
+}
+
+TEST(Ethernet, parseAddressesDeleteExistingOnShortLength)
+{
+ // With one existing address, and an input of len(0) it should request
+ // deletion and clear gateway
+ std::vector<std::variant<nlohmann::json::object_t, std::nullptr_t>> addr;
+ std::vector<IPv4AddressData> existingAddr;
+ IPv4AddressData& existing = existingAddr.emplace_back();
+ existing.id = "my_ip_id";
+ existing.origin = "Static";
+
+ std::vector<AddressPatch> addrOut;
+ std::string gatewayOut;
+
+ crow::Response res;
+
+ EXPECT_TRUE(parseAddresses(addr, existingAddr, res, addrOut, gatewayOut));
+ EXPECT_EQ(addrOut.size(), 1);
+ EXPECT_EQ(addrOut[0].existingDbusId, "my_ip_id");
+ EXPECT_EQ(addrOut[0].operation, AddrChange::Delete);
+ EXPECT_EQ(gatewayOut, "");
+}
+
+} // namespace
+} // namespace redfish