From d6df1bbdca95c198e98911af24bf243cd3bba8fb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 11 Feb 2022 11:29:49 -0500 Subject: [PATCH] Revert "Add multi-address support for PeerAddress" (#15082) * Revert "Add ability to keep several IP addresses per PeerAddress (#14968)" This reverts commit 14eb3f5b5b1da23310e85ca1cbcb2c07fc740db6. * Add back unit tests for peer address to string formatting. More tests are good * Maintain the cleanup of ToPeerAddress from the previous patch - that was good * Restyle --- src/app/OperationalDeviceProxy.h | 32 +-- src/controller/CommissioneeDeviceProxy.h | 2 +- .../ChipDeviceController-ScriptBinding.cpp | 4 +- src/transport/raw/BUILD.gn | 1 - src/transport/raw/PeerAddress.cpp | 124 ----------- src/transport/raw/PeerAddress.h | 207 +++++++----------- src/transport/raw/tests/TestPeerAddress.cpp | 33 +-- 7 files changed, 88 insertions(+), 315 deletions(-) delete mode 100644 src/transport/raw/PeerAddress.cpp diff --git a/src/app/OperationalDeviceProxy.h b/src/app/OperationalDeviceProxy.h index 92f82c320ab4d1..95cca4a8825455 100644 --- a/src/app/OperationalDeviceProxy.h +++ b/src/app/OperationalDeviceProxy.h @@ -178,31 +178,19 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, SessionReleaseDele static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData) { - Transport::PeerAddress address = Transport::PeerAddress(Transport::Type::kUdp); - address.SetPort(nodeData.mPort); - - for (unsigned i = 0; i < nodeData.mNumIPs; i++) + Inet::InterfaceId interfaceId = Inet::InterfaceId::Null(); + + // TODO - Revisit usage of InterfaceID only for addresses that are IPv6 LLA + // Only use the DNS-SD resolution's InterfaceID for addresses that are IPv6 LLA. + // For all other addresses, we should rely on the device's routing table to route messages sent. + // Forcing messages down an InterfaceId might fail. For example, in bridged networks like Thread, + // mDNS advertisements are not usually received on the same interface the peer is reachable on. + if (nodeData.mAddress[0].IsIPv6LinkLocal()) { - const auto addr = nodeData.mAddress[i]; - // Only use the mDNS resolution's InterfaceID for addresses that are IPv6 LLA. - // For all other addresses, we should rely on the device's routing table to route messages sent. - // Forcing messages down an InterfaceId might fail. For example, in bridged networks like Thread, - // mDNS advertisements are not usually received on the same interface the peer is reachable on. - // TODO: Right now, just use addr0, but we should really push all the addresses and interfaces to - // the device and allow it to make a proper decision about which addresses are preferred and reachable. - CHIP_ERROR err = address.AppendDestination(nodeData.mAddress[i], - addr.IsIPv6LinkLocal() ? nodeData.mInterfaceId : Inet::InterfaceId::Null()); - - if (err != CHIP_NO_ERROR) - { - char addr_str[Inet::IPAddress::kMaxStringLength]; - addr.ToString(addr_str); - - ChipLogError(Controller, "Could not append IP address %s: %s", addr_str, err.AsString()); - } + interfaceId = nodeData.mInterfaceId; } - return address; + return Transport::PeerAddress::UDP(nodeData.mAddress[0], nodeData.mPort, interfaceId); } private: diff --git a/src/controller/CommissioneeDeviceProxy.h b/src/controller/CommissioneeDeviceProxy.h index 3352186cd8d0ae..14208d1f49da0e 100644 --- a/src/controller/CommissioneeDeviceProxy.h +++ b/src/controller/CommissioneeDeviceProxy.h @@ -214,7 +214,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat Messaging::ExchangeManager * GetExchangeManager() const override { return mExchangeMgr; } - void SetAddress(const Inet::IPAddress & deviceAddr) { mDeviceAddress.SetSingleIPAddress(deviceAddr); } + void SetAddress(const Inet::IPAddress & deviceAddr) { mDeviceAddress.SetIPAddress(deviceAddr); } PASESession & GetPairing() { return mPairing; } diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 7a6300522a90d0..72d579b1fdb72e 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -324,7 +324,7 @@ ChipError::StorageType pychip_DeviceController_ConnectIP(chip::Controller::Devic VerifyOrReturnError(chip::Inet::IPAddress::FromString(peerAddrStr, peerAddr), CHIP_ERROR_INVALID_ARGUMENT.AsInteger()); // TODO: IP rendezvous should use TCP connection. - addr.SetTransportType(chip::Transport::Type::kUdp).SetSingleIPAddress(peerAddr); + addr.SetTransportType(chip::Transport::Type::kUdp).SetIPAddress(peerAddr); params.SetPeerAddress(addr).SetDiscriminator(0); devCtrl->ReleaseOperationalDevice(nodeid); @@ -380,7 +380,7 @@ ChipError::StorageType pychip_DeviceController_EstablishPASESessionIP(chip::Cont chip::Transport::PeerAddress addr; RendezvousParameters params = chip::RendezvousParameters().SetSetupPINCode(setupPINCode); VerifyOrReturnError(chip::Inet::IPAddress::FromString(peerAddrStr, peerAddr), CHIP_ERROR_INVALID_ARGUMENT.AsInteger()); - addr.SetTransportType(chip::Transport::Type::kUdp).SetSingleIPAddress(peerAddr); + addr.SetTransportType(chip::Transport::Type::kUdp).SetIPAddress(peerAddr); params.SetPeerAddress(addr).SetDiscriminator(0); return devCtrl->EstablishPASEConnection(nodeid, params).AsInteger(); } diff --git a/src/transport/raw/BUILD.gn b/src/transport/raw/BUILD.gn index d13c3adb5e3861..ae3aeb58f8e3e9 100644 --- a/src/transport/raw/BUILD.gn +++ b/src/transport/raw/BUILD.gn @@ -22,7 +22,6 @@ static_library("raw") { "Base.h", "MessageHeader.cpp", "MessageHeader.h", - "PeerAddress.cpp", "PeerAddress.h", "TCP.cpp", "TCP.h", diff --git a/src/transport/raw/PeerAddress.cpp b/src/transport/raw/PeerAddress.cpp deleted file mode 100644 index 1bfb3c9f703560..00000000000000 --- a/src/transport/raw/PeerAddress.cpp +++ /dev/null @@ -1,124 +0,0 @@ -/* - * - * Copyright (c) 2022 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "PeerAddress.h" - -#include - -namespace chip { -namespace Transport { - -namespace { - -/// Formats IP addresses into a format suitable for PeerAddress::ToString -/// Specifically it will add the first IP address, interface used and port -/// and a marker if more than one IP address is contained in the input list -/// -/// Examples: -/// NONE:123 // when addresses are empty -/// 10.20.30.40:5040 // exactly one IP address in the destination list with null interface -/// 10.0.0.10%wlan0:123 (+3 more) // 4 input addresses in the list -/// %:port -void AddIpInfo(StringBuilderBase & dest, uint16_t port, Span destinations) -{ - if (destinations.empty()) - { - dest.Add(":NONE:"); - } - else - { - // First address information. - // - // In time, peer addresses should have an 'active address' to determine which IP is - // used to communicate with peers, in which case the output here should be the current - // active address rather than just the first in the list. - - dest.Add(":"); - auto first = destinations.begin(); - - { - char ip_addr[Inet::IPAddress::kMaxStringLength]; - first->ipAddress.ToString(ip_addr); - if (first->ipAddress.IsIPv6()) - { - dest.Add("["); - } - dest.Add(ip_addr); - } - - if (first->interface.IsPresent()) - { - dest.Add("%"); - - char interface[Inet::InterfaceId::kMaxIfNameLength] = {}; // +1 to prepend '%' - - if (first->interface.GetInterfaceName(interface, sizeof(interface)) != CHIP_NO_ERROR) - { - dest.Add("(err)"); - } - else - { - dest.Add(interface); - } - } - if (first->ipAddress.IsIPv6()) - { - dest.Add("]"); - } - dest.Add(":"); - } - dest.Add(port); - - if (destinations.size() > 1) - { - dest.Add(" (+"); - dest.Add(static_cast(destinations.size() - 1)); - dest.Add(" more)"); - } -} - -} // namespace - -void PeerAddress::ToString(char * buf, size_t bufSize) const -{ - StringBuilderBase out(buf, bufSize); - - switch (mTransportType) - { - case Type::kUndefined: - out.Add("UNDEFINED"); - break; - case Type::kUdp: - out.Add("UDP"); - AddIpInfo(out, mPort, Span(mDestinations, mNumValidDestinations)); - break; - case Type::kTcp: - out.Add("TCP"); - AddIpInfo(out, mPort, Span(mDestinations, mNumValidDestinations)); - break; - case Type::kBle: - // Note that BLE does not currently use any specific address. - out.Add("BLE"); - break; - default: - out.Add("ERROR"); - break; - } -} - -} // namespace Transport -} // namespace chip diff --git a/src/transport/raw/PeerAddress.h b/src/transport/raw/PeerAddress.h index c0338420dd755b..69318d027da747 100644 --- a/src/transport/raw/PeerAddress.h +++ b/src/transport/raw/PeerAddress.h @@ -28,7 +28,6 @@ #include #include #include -#include #include namespace chip { @@ -63,35 +62,8 @@ enum class Type : uint8_t class PeerAddress { public: - /// Target destinations for Peer addresses - struct IPDestination - { - Inet::IPAddress ipAddress; - Inet::InterfaceId interface = Inet::InterfaceId::Null(); - - void Set(const Inet::IPAddress & ip, const Inet::InterfaceId & iface) - { - ipAddress = ip; - interface = iface; - } - - bool operator==(const IPDestination & other) const - { - return (ipAddress == other.ipAddress) && (interface == other.interface); - } - - bool operator!=(const IPDestination & other) const { return !(*this == other); } - }; - - // When using DNSSD, a peer may be discovered at multiple addresses. - // This controls how many addresses can be kept. - static constexpr unsigned kMaxPeerDestinations = 3; - - PeerAddress() : mTransportType(Type::kUndefined) {} - PeerAddress(const Inet::IPAddress & addr, Type type) : mTransportType(type) - { - AppendDestination(addr, Inet::InterfaceId::Null()); - } + PeerAddress() : mIPAddress(Inet::IPAddress::Any), mTransportType(Type::kUndefined) {} + PeerAddress(const Inet::IPAddress & addr, Type type) : mIPAddress(addr), mTransportType(type) {} PeerAddress(Type type) : mTransportType(type) {} PeerAddress(PeerAddress &&) = default; @@ -99,31 +71,11 @@ class PeerAddress PeerAddress & operator=(const PeerAddress &) = default; PeerAddress & operator=(PeerAddress &&) = default; - const Inet::IPAddress & GetIPAddress() const - { - if (mNumValidDestinations > 0) - { - return mDestinations[0].ipAddress; - } - else - { - return Inet::IPAddress::Any; // not initialized really - } - } - - /// Add another IP address to the list of internal IP addresses that - /// are considred valid. - /// - /// Returns failure if insufficient memory available to append more - /// addresses - CHIP_ERROR AppendDestination(const Inet::IPAddress & addr, const Inet::InterfaceId & interface) + const Inet::IPAddress & GetIPAddress() const { return mIPAddress; } + PeerAddress & SetIPAddress(const Inet::IPAddress & addr) { - if (mNumValidDestinations >= kMaxPeerDestinations) - { - return CHIP_ERROR_NO_MEMORY; - } - mDestinations[mNumValidDestinations++].Set(addr, interface); - return CHIP_NO_ERROR; + mIPAddress = addr; + return *this; } Type GetTransportType() const { return mTransportType; } @@ -140,75 +92,27 @@ class PeerAddress return *this; } - Inet::InterfaceId GetInterface() const + Inet::InterfaceId GetInterface() const { return mInterface; } + PeerAddress & SetInterface(Inet::InterfaceId interface) { - if (mNumValidDestinations > 0) - { - return mDestinations[0].interface; - } - else - { - return Inet::InterfaceId::Null(); - } - } - - /// DEPRECATED: Setting this has several drawbacks: - /// - Interface is not set and LL ip addresses will not work without a - /// set interface - /// - Nodes are generally expected to support multiple IP addresses when - /// discovered using DNSSD. - PeerAddress & SetSingleIPAddress(const Inet::IPAddress & addr) - { - // NOTE: mDestinations[0].interface is NOT changed in any way - mDestinations[0].ipAddress = addr; - mNumValidDestinations = 1; + mInterface = interface; return *this; } bool IsInitialized() const { return mTransportType != Type::kUndefined; } - bool IsMulticast() - { - if (Type::kUdp != mTransportType) - { - return false; - } - - if (mNumValidDestinations == 0) - { - return false; - } - - // Assumption here is that if any IP address is multicast, the entire object should - // be multicast - return mDestinations[0].ipAddress.IsIPv6Multicast(); - } + bool IsMulticast() { return Type::kUdp == mTransportType && mIPAddress.IsIPv6Multicast(); } bool operator==(const PeerAddress & other) const { - if ((mTransportType != other.mTransportType) || (mPort != other.mPort) || - mNumValidDestinations != other.mNumValidDestinations) - { - return false; - } - - // NOTE: we do NOT try to order/treat as a set all destinations here. - // Check that the valid destinations are identical only, including order. - for (unsigned i = 0; i < mNumValidDestinations; i++) - { - if (mDestinations[i] != other.mDestinations[i]) - { - return false; - } - } - - return true; + return (mTransportType == other.mTransportType) && (mIPAddress == other.mIPAddress) && (mPort == other.mPort) && + (mInterface == other.mInterface); } bool operator!=(const PeerAddress & other) const { return !(*this == other); } /// Maximum size of the string outputes by ToString. Format is of the form: - /// "UDP:: (+N more)" + /// "UDP::" static constexpr size_t kMaxToStringSize = 3 // type: UDP/TCP/BLE + 1 // splitter : + 2 // brackets around address @@ -217,41 +121,79 @@ class PeerAddress + Inet::InterfaceId::kMaxIfNameLength // interface + 1 // splitter : + 5 // port: 16 bit interger - + 3 // " (+" - + 2 // integer for "more", low digits expected - + 6 // " more)" + 1; // NullTerminator - static_assert(kMaxPeerDestinations < 100, "for maxStringSize to be correct for the 'more' part"); - template inline void ToString(char (&buf)[N]) const { ToString(buf, N); } - void ToString(char * buf, size_t bufSize) const; + void ToString(char * buf, size_t bufSize) const + { + char ip_addr[Inet::IPAddress::kMaxStringLength]; + + char interface[Inet::InterfaceId::kMaxIfNameLength + 1] = {}; // +1 to prepend '%' + if (mInterface.IsPresent()) + { + interface[0] = '%'; + interface[1] = 0; + CHIP_ERROR err = mInterface.GetInterfaceName(interface + 1, sizeof(interface) - 1); + if (err != CHIP_NO_ERROR) + { + Platform::CopyString(interface, sizeof(interface), "%(err)"); + } + } + + switch (mTransportType) + { + case Type::kUndefined: + snprintf(buf, bufSize, "UNDEFINED"); + break; + case Type::kUdp: + mIPAddress.ToString(ip_addr); +#if INET_CONFIG_ENABLE_IPV4 + if (mIPAddress.IsIPv4()) + snprintf(buf, bufSize, "UDP:%s%s:%d", ip_addr, interface, mPort); + else +#endif + snprintf(buf, bufSize, "UDP:[%s%s]:%d", ip_addr, interface, mPort); + break; + case Type::kTcp: + mIPAddress.ToString(ip_addr); +#if INET_CONFIG_ENABLE_IPV4 + if (mIPAddress.IsIPv4()) + snprintf(buf, bufSize, "TCP:%s%s:%d", ip_addr, interface, mPort); + else +#endif + snprintf(buf, bufSize, "TCP:[%s%s]:%d", ip_addr, interface, mPort); + break; + case Type::kBle: + // Note that BLE does not currently use any specific address. + snprintf(buf, bufSize, "BLE"); + break; + default: + snprintf(buf, bufSize, "ERROR"); + break; + } + } /****** Factory methods for convenience ******/ - static PeerAddress Uninitialized() { return PeerAddress(Type::kUndefined); } + static PeerAddress Uninitialized() { return PeerAddress(Inet::IPAddress::Any, Type::kUndefined); } static PeerAddress BLE() { return PeerAddress(Type::kBle); } - - static PeerAddress UDP(const Inet::IPAddress & addr, uint16_t port = CHIP_PORT, - Inet::InterfaceId interface = Inet::InterfaceId::Null()) + static PeerAddress UDP(const Inet::IPAddress & addr) { return PeerAddress(addr, Type::kUdp); } + static PeerAddress UDP(const Inet::IPAddress & addr, uint16_t port) { return UDP(addr).SetPort(port); } + static PeerAddress UDP(const Inet::IPAddress & addr, uint16_t port, Inet::InterfaceId interface) { - PeerAddress result = PeerAddress(Type::kUdp); - result.SetPort(port).AppendDestination(addr, interface); - return result; + return UDP(addr).SetPort(port).SetInterface(interface); } - - static PeerAddress TCP(const Inet::IPAddress & addr, uint16_t port = CHIP_PORT, - Inet::InterfaceId interface = Inet::InterfaceId::Null()) + static PeerAddress TCP(const Inet::IPAddress & addr) { return PeerAddress(addr, Type::kTcp); } + static PeerAddress TCP(const Inet::IPAddress & addr, uint16_t port) { return TCP(addr).SetPort(port); } + static PeerAddress TCP(const Inet::IPAddress & addr, uint16_t port, Inet::InterfaceId interface) { - PeerAddress result = PeerAddress(Type::kTcp); - result.SetPort(port).AppendDestination(addr, interface); - return result; + return TCP(addr).SetPort(port).SetInterface(interface); } static PeerAddress Multicast(chip::FabricId fabric, chip::GroupId group) @@ -267,15 +209,14 @@ class PeerAddress // * 0x00 // * The 16-bits Group Identifier in big-endian order uint32_t groupId = static_cast((fabric << 24) & 0xff000000) | group; - return UDP(Inet::IPAddress::MakeIPv6PrefixMulticast(scope, prefixLength, prefix, groupId)); } private: - IPDestination mDestinations[kMaxPeerDestinations] = {}; - unsigned mNumValidDestinations = 0; - Type mTransportType = Type::kUndefined; - uint16_t mPort = CHIP_PORT; ///< Relevant for UDP data sending. + Inet::IPAddress mIPAddress = {}; + Type mTransportType = Type::kUndefined; + uint16_t mPort = CHIP_PORT; ///< Relevant for UDP data sending. + Inet::InterfaceId mInterface = Inet::InterfaceId::Null(); }; } // namespace Transport diff --git a/src/transport/raw/tests/TestPeerAddress.cpp b/src/transport/raw/tests/TestPeerAddress.cpp index 3ac323120b10d4..2ba57cdd03e70f 100644 --- a/src/transport/raw/tests/TestPeerAddress.cpp +++ b/src/transport/raw/tests/TestPeerAddress.cpp @@ -97,43 +97,13 @@ void TestToString(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, !strcmp(buff, "UDP:[1223::3456:789a]:8080")); } - { - - PeerAddress udp = PeerAddress(Transport::Type::kUdp); - udp.SetPort(5840); - - IPAddress::FromString("::1", ip); - udp.AppendDestination(ip, InterfaceId::Null()); - IPAddress::FromString("1223::3456:789a", ip); - udp.AppendDestination(ip, InterfaceId::Null()); - - udp.ToString(buff); - NL_TEST_ASSERT(inSuite, !strcmp(buff, "UDP:[::1]:5840 (+1 more)")); - } - { PeerAddress udp = PeerAddress(Transport::Type::kUdp); udp.SetPort(5840); udp.ToString(buff); - NL_TEST_ASSERT(inSuite, !strcmp(buff, "UDP:NONE:5840")); - } -} - -void TestAppendDestinationLimit(nlTestSuite * inSuite, void * inContext) -{ - IPAddress ip; - IPAddress::FromString("::1", ip); - - PeerAddress udp = PeerAddress(Transport::Type::kUdp); - udp.SetPort(123); - - for (unsigned i = 0; i < PeerAddress::kMaxPeerDestinations; i++) - { - NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == udp.AppendDestination(ip, InterfaceId::Null())); + NL_TEST_ASSERT(inSuite, !strcmp(buff, "UDP:[::]:5840")); } - - NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR != udp.AppendDestination(ip, InterfaceId::Null())); } /** @@ -145,7 +115,6 @@ const nlTest sTests[] = { NL_TEST_DEF("PeerAddress Multicast", TestPeerAddressMulticast), NL_TEST_DEF("ToString", TestToString), - NL_TEST_DEF("AppendDestination", TestAppendDestinationLimit), NL_TEST_SENTINEL() }; // clang-format on