Skip to content

Commit

Permalink
Fix incorrect snapshotting of MRP config by CASE client. (#32380)
Browse files Browse the repository at this point in the history
* Fix incorrect snapshotting of MRP config by CASE client.

Right now, we snapshot the MRP config as part of the CASEClientInitParams during stack startup.

After that, we will use that snapshotted config whenever we are the CASE initiator.

This config will not match the parameters we use as CASE responder or advertise
over DNS-SD if the local MRP configuration ever changes.  Which for an ICD it can.

The fix is to stop the incorrect snapshotting and get the information we need
from the right source of truth when we need it.

* Address review comments.

* Address issue with CASE session not assuming the right things when NullOptional is passed in.
  • Loading branch information
bzbarsky-apple authored Mar 1, 2024
1 parent a986606 commit ac1ebc5
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 4 deletions.
5 changes: 4 additions & 1 deletion src/app/CASEClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include <app/CASEClient.h>
#include <messaging/ReliableMessageProtocolConfig.h>

namespace chip {

Expand Down Expand Up @@ -49,10 +50,12 @@ CHIP_ERROR CASEClient::EstablishSession(const CASEClientInitParams & params, con
Messaging::ExchangeContext * exchange = params.exchangeMgr->NewContext(session.Value(), &mCASESession);
VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);

const Optional<ReliableMessageProtocolConfig> & mrpLocalConfig =
params.mrpLocalConfig.HasValue() ? params.mrpLocalConfig : GetLocalMRPConfig();
mCASESession.SetGroupDataProvider(params.groupDataProvider);
ReturnErrorOnFailure(mCASESession.EstablishSession(*params.sessionManager, params.fabricTable, peer, exchange,
params.sessionResumptionStorage, params.certificateValidityPolicy, delegate,
params.mrpLocalConfig));
mrpLocalConfig));

return CHIP_NO_ERROR;
}
Expand Down
6 changes: 5 additions & 1 deletion src/app/CASEClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ struct CASEClientInitParams
Messaging::ExchangeManager * exchangeMgr = nullptr;
FabricTable * fabricTable = nullptr;
Credentials::GroupDataProvider * groupDataProvider = nullptr;
Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();

// mrpLocalConfig should not generally be set to anything other than
// NullOptional. Doing that can lead to different parts of the system
// claiming different MRP parameters for the same node.
Optional<ReliableMessageProtocolConfig> mrpLocalConfig = NullOptional;

CHIP_ERROR Validate() const
{
Expand Down
4 changes: 3 additions & 1 deletion src/app/server/Server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,9 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
.exchangeMgr = &mExchangeMgr,
.fabricTable = &mFabrics,
.groupDataProvider = mGroupsProvider,
.mrpLocalConfig = GetLocalMRPConfig(),
// Don't provide an MRP local config, so each CASE initiation will use
// the then-current value.
.mrpLocalConfig = NullOptional,
},
.clientPool = &mCASEClientPool,
.sessionSetupPool = &mSessionSetupPool,
Expand Down
4 changes: 3 additions & 1 deletion src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,9 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
.exchangeMgr = stateParams.exchangeMgr,
.fabricTable = stateParams.fabricTable,
.groupDataProvider = stateParams.groupDataProvider,
.mrpLocalConfig = GetLocalMRPConfig(),
// Don't provide an MRP local config, so each CASE initiation will use
// the then-current value.
.mrpLocalConfig = NullOptional,
};

CASESessionManagerConfig sessionManagerConfig = {
Expand Down

0 comments on commit ac1ebc5

Please sign in to comment.