Skip to content

Commit

Permalink
Add ability to keep several IP addresses per PeerAddress (#14968)
Browse files Browse the repository at this point in the history
* Add ability for PeerAddress to maintain a list of valid IP addresses instead of a single one

* Switch IP destinations to contain both ip address and interface - they seem to be required in pairs

* SetInterface method is not used. remove

* Change ToPeerAddress to actually fill in all known peer addresses from DNSSD

* CHIPDeviceController::ToPeerAddress does not seem used, so removed it

* Restyle

* Add some udp printout unit tests for tostring. Found 1 minor bug

* A few more minor unit  tests additions for tostring (TCP and BLE)

* slight fix in the comment

* Slight fix for printing empty address lists and added unit test

* One more minor comment update

* We seem to keep a LOT of peer addresses and BSS increased too much. Lower down the number of IP addresses kept by peeraddress. This is very concerning - peer address should have been only one per session/exchange and not duplicated that much

* Update python code - set single IP address is used for UDP

* More changes to make python compile

* Fix unit test: make sure we work with even the significantly lower BSS usage

* off by one error fix in peer address append address

* off by one error fix in peer address append address

* Fix comparison order, should fix unit tests

* Attempt to work around unused variable error (assuming logging disabled)
  • Loading branch information
andy31415 authored and pull[bot] committed Sep 1, 2023
1 parent 105ffe9 commit e5a1fd0
Show file tree
Hide file tree
Showing 10 changed files with 366 additions and 111 deletions.
2 changes: 1 addition & 1 deletion src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void CASESessionManager::OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeDa
VerifyOrReturn(session != nullptr,
ChipLogDetail(Controller, "OnNodeIdResolved was called for a device with no active sessions, ignoring it."));

LogErrorOnFailure(session->UpdateDeviceData(session->ToPeerAddress(nodeData), nodeData.GetMRPConfig()));
LogErrorOnFailure(session->UpdateDeviceData(OperationalDeviceProxy::ToPeerAddress(nodeData), nodeData.GetMRPConfig()));
}

void CASESessionManager::OnNodeIdResolutionFailed(const PeerId & peer, CHIP_ERROR error)
Expand Down
32 changes: 22 additions & 10 deletions src/app/OperationalDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,31 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, SessionReleaseDele

static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData)
{
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())
Transport::PeerAddress address = Transport::PeerAddress(Transport::Type::kUdp);
address.SetPort(nodeData.mPort);

for (unsigned i = 0; i < nodeData.mNumIPs; i++)
{
interfaceId = nodeData.mInterfaceId;
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());
}
}

return Transport::PeerAddress::UDP(nodeData.mAddress[0], nodeData.mPort, interfaceId);
return address;
}

private:
Expand Down
17 changes: 0 additions & 17 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,23 +554,6 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowInternal()
}

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Transport::PeerAddress DeviceController::ToPeerAddress(const chip::Dnssd::ResolvedNodeData & nodeData) const
{
Inet::InterfaceId interfaceId;

// 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.
if (nodeData.mAddress[0].IsIPv6LinkLocal())
{
interfaceId = nodeData.mInterfaceId;
}

return Transport::PeerAddress::UDP(nodeData.mAddress[0], nodeData.mPort, interfaceId);
}

void DeviceController::OnNodeIdResolved(const chip::Dnssd::ResolvedNodeData & nodeData)
{
Expand Down
2 changes: 0 additions & 2 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate

PersistentStorageDelegate * mStorageDelegate = nullptr;
#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Transport::PeerAddress ToPeerAddress(const chip::Dnssd::ResolvedNodeData & nodeData) const;

DeviceAddressUpdateDelegate * mDeviceAddressUpdateDelegate = nullptr;
// TODO(cecille): Make this configuarable.
static constexpr int kMaxCommissionableNodes = 10;
Expand Down
2 changes: 1 addition & 1 deletion src/controller/CommissioneeDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat

Messaging::ExchangeManager * GetExchangeManager() const override { return mExchangeMgr; }

void SetAddress(const Inet::IPAddress & deviceAddr) { mDeviceAddress.SetIPAddress(deviceAddr); }
void SetAddress(const Inet::IPAddress & deviceAddr) { mDeviceAddress.SetSingleIPAddress(deviceAddr); }

PASESession & GetPairing() { return mPairing; }

Expand Down
4 changes: 2 additions & 2 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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).SetIPAddress(peerAddr);
addr.SetTransportType(chip::Transport::Type::kUdp).SetSingleIPAddress(peerAddr);
params.SetPeerAddress(addr).SetDiscriminator(0);

devCtrl->ReleaseOperationalDevice(nodeid);
Expand Down Expand Up @@ -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).SetIPAddress(peerAddr);
addr.SetTransportType(chip::Transport::Type::kUdp).SetSingleIPAddress(peerAddr);
params.SetPeerAddress(addr).SetDiscriminator(0);
return devCtrl->EstablishPASEConnection(nodeid, params).AsInteger();
}
Expand Down
1 change: 1 addition & 0 deletions src/transport/raw/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ static_library("raw") {
"Base.h",
"MessageHeader.cpp",
"MessageHeader.h",
"PeerAddress.cpp",
"PeerAddress.h",
"TCP.cpp",
"TCP.h",
Expand Down
124 changes: 124 additions & 0 deletions src/transport/raw/PeerAddress.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/*
*
* 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 <lib/support/StringBuilder.h>

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
/// <ip>%<iface>:port
void AddIpInfo(StringBuilderBase & dest, uint16_t port, Span<const PeerAddress::IPDestination> 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<int>(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<const IPDestination>(mDestinations, mNumValidDestinations));
break;
case Type::kTcp:
out.Add("TCP");
AddIpInfo(out, mPort, Span<const IPDestination>(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
Loading

0 comments on commit e5a1fd0

Please sign in to comment.