From 1459034069d6aae09e69941f5e38407fb2f53137 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Wed, 24 Jul 2024 10:34:49 -0400 Subject: [PATCH] Add injectable ICD Check-In BackOff strategy --- src/app/icd/server/BUILD.gn | 17 +++++ .../server/DefaultICDCheckInBackOffStrategy.h | 63 +++++++++++++++++++ .../icd/server/ICDCheckInBackOffStrategy.h | 62 ++++++++++++++++++ src/app/icd/server/ICDManager.cpp | 34 +++++----- src/app/icd/server/ICDManager.h | 21 ++++--- src/app/icd/server/tests/BUILD.gn | 2 + .../TestDefaultICDCheckInBackOffStrategy.cpp | 57 +++++++++++++++++ src/app/icd/server/tests/TestICDManager.cpp | 8 ++- src/app/server/BUILD.gn | 7 +++ src/app/server/Server.cpp | 5 +- src/app/server/Server.h | 18 ++++++ 11 files changed, 267 insertions(+), 27 deletions(-) create mode 100644 src/app/icd/server/DefaultICDCheckInBackOffStrategy.h create mode 100644 src/app/icd/server/ICDCheckInBackOffStrategy.h create mode 100644 src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index 89c39c203a7c16..b30b79d3d4a1d0 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -66,6 +66,22 @@ source_set("notifier") { ] } +source_set("check-in-back-off") { + sources = [ "ICDCheckInBackOffStrategy.h" ] + + public_deps = [ + ":monitoring-table", + "${chip_root}/src/lib/core", + "${chip_root}/src/lib/support", + ] +} + +source_set("default-check-in-back-off") { + sources = [ "DefaultICDCheckInBackOffStrategy.h" ] + + public_deps = [ ":check-in-back-off" ] +} + # ICD Manager source-set is broken out of the main source-set to enable unit tests # All sources and configurations used by the ICDManager need to go in this source-set source_set("manager") { @@ -89,6 +105,7 @@ source_set("manager") { if (chip_enable_icd_checkin) { public_deps += [ + ":check-in-back-off", ":monitoring-table", ":sender", "${chip_root}/src/app:app_config", diff --git a/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h new file mode 100644 index 00000000000000..5043d15185f81d --- /dev/null +++ b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h @@ -0,0 +1,63 @@ +/* + * + * Copyright (c) 2024 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. + */ + +#pragma once + +#include +#include + +namespace chip { +namespace app { + +/** + * @brief Default ICD Check-In BackOff Strategy. + * The default strategy is based on the two types of controllers + * - kPermanent : Always send a Check-In message + * - kEphemeral : Never send a Check-In message + * + * This implementation represents a no back off strategy. + */ +class DefaultICDCheckInBackOffStrategy : public ICDCheckInBackOffStrategy +{ +public: + DefaultICDCheckInBackOffStrategy() = default; + ~DefaultICDCheckInBackOffStrategy() = default; + + /** + * @brief Function checks if the entry is a permanent or ephemeral client. + * If the client is permanent, we should send a Check-In message. + * If the cliet is ephemerakl, we should not send a Check-In message. + * + * @param entry Entry for which we are deciding if we need to send a Check-In message or not. + * @return true If the client is permanent, return true. + * @return false If the client is not permanent, ephemeral or invalid, return false. + */ + bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) + { + return (entry.clientType == Clusters::IcdManagement::ClientTypeEnum::kPermanent); + } + + /** + * @brief The default Check-In BackOff fundamentally implements a no back off strategy. + * As such, we don't need to execute anything to force the maximum Check-In BackOff. + * + */ + CHIP_ERROR ForceMaximumCheckInBackoff() { return CHIP_NO_ERROR; } +}; + +} // namespace app +} // namespace chip diff --git a/src/app/icd/server/ICDCheckInBackOffStrategy.h b/src/app/icd/server/ICDCheckInBackOffStrategy.h new file mode 100644 index 00000000000000..97ca7c9a4d3418 --- /dev/null +++ b/src/app/icd/server/ICDCheckInBackOffStrategy.h @@ -0,0 +1,62 @@ +/* + * + * Copyright (c) 2024 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. + */ +#pragma once + +#include +#include + +namespace chip { +namespace app { + +/** + * @brief This class defines the necessary interface a ICD Check-In BackOff strategy needs to implment to be consummed by the + * ICDManager class. The strategy is injected with the init server params when initializing the device Server class. + */ +class ICDCheckInBackOffStrategy +{ +public: + virtual ~ICDCheckInBackOffStrategy() = default; + + /** + * @brief Function is used by the ICDManager to determine if a Check-In message should be sent to the given entry based on the + * Check-In BackOff strategy. + * + * There are no requirements on how the Check-In BackOff strategy should behave. + * The only specified requirement is the maximum time between to Check-In message, MaximumCheckInBackOff. + * All strategies must respect this requirement. + * + * @param entry ICDMonitoringEntry for which we are about to send a Check-In message to. + * + * @return true ICDCheckInBackOffStrategy determines that we SHOULD send a Check-In message to the given entry + * @return falseI CDCheckInBackOffStrategy determines that we SHOULD NOT send a Check-In message to the given entry + */ + virtual bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) = 0; + + /** + * @brief Function is used within the test event trigger to force the maximum BackOff state of the ICD Check-In BackOff + * strategy. This enables to validate the strategy and to certify it respects the MaximumCheckInBackOff interval during + * certification. + * + * Function sets the maxmimum BackOff state for all clients registered with the ICD + * + * @return CHIP_ERROR Any error returned during the forcing of the maximum BackOff state + */ + virtual CHIP_ERROR ForceMaximumCheckInBackoff() = 0; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index ee55b0b0f9b23b..07632a059b2f1c 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -31,10 +31,11 @@ namespace { enum class ICDTestEventTriggerEvent : uint64_t { - kAddActiveModeReq = 0x0046'0000'00000001, - kRemoveActiveModeReq = 0x0046'0000'00000002, - kInvalidateHalfCounterValues = 0x0046'0000'00000003, - kInvalidateAllCounterValues = 0x0046'0000'00000004, + kAddActiveModeReq = 0x0046'0000'00000001, + kRemoveActiveModeReq = 0x0046'0000'00000002, + kInvalidateHalfCounterValues = 0x0046'0000'00000003, + kInvalidateAllCounterValues = 0x0046'0000'00000004, + kForceMaximumCheckInBackOffState = 0x0046'0000'00000005, }; } // namespace @@ -52,7 +53,8 @@ static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count"); void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider) + Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, + ICDCheckInBackOffStrategy * strategy) { #if CHIP_CONFIG_ENABLE_ICD_CIP VerifyOrDie(storage != nullptr); @@ -60,6 +62,7 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDie(symmetricKeystore != nullptr); VerifyOrDie(exchangeManager != nullptr); VerifyOrDie(subInfoProvider != nullptr); + VerifyOrDie(strategy != nullptr); #endif // CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_ENABLE_ICD_LIT @@ -82,11 +85,12 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDie(ICDNotifier::GetInstance().Subscribe(this) == CHIP_NO_ERROR); #if CHIP_CONFIG_ENABLE_ICD_CIP - mStorage = storage; - mFabricTable = fabricTable; - mSymmetricKeystore = symmetricKeystore; - mExchangeManager = exchangeManager; - mSubInfoProvider = subInfoProvider; + mStorage = storage; + mFabricTable = fabricTable; + mSymmetricKeystore = symmetricKeystore; + mExchangeManager = exchangeManager; + mSubInfoProvider = subInfoProvider; + mICDCheckInBackOffStrategy = strategy; VerifyOrDie(ICDConfigurationData::GetInstance().GetICDCounter().Init(mStorage, DefaultStorageKeyAllocator::ICDCheckInCounter(), ICDConfigurationData::kICDCounterPersistenceIncrement) == @@ -188,15 +192,15 @@ void ICDManager::SendCheckInMsgs() continue; } - if (entry.clientType == ClientTypeEnum::kEphemeral) + if (!ShouldCheckInMsgsBeSentAtActiveModeFunction(entry.fabricIndex, entry.monitoredSubject)) { - // If the registered client is ephemeral, do not send a Check-In message - // continue to next entry continue; } - if (!ShouldCheckInMsgsBeSentAtActiveModeFunction(entry.fabricIndex, entry.monitoredSubject)) + // Validate Check-In BackOff strategy if we should send a Check-In message. + if (!mICDCheckInBackOffStrategy->ShouldSendCheckInMessage(entry)) { + // continue to next entry continue; } @@ -689,6 +693,8 @@ CHIP_ERROR ICDManager::HandleEventTrigger(uint64_t eventTrigger) case ICDTestEventTriggerEvent::kInvalidateAllCounterValues: err = ICDConfigurationData::GetInstance().GetICDCounter().InvalidateAllCheckInCounterValues(); break; + case ICDTestEventTriggerEvent::kForceMaximumCheckInBackOffState: + err = mICDCheckInBackOffStrategy->ForceMaximumCheckInBackoff(); #endif // CHIP_CONFIG_ENABLE_ICD_CIP default: err = CHIP_ERROR_INVALID_ARGUMENT; diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 4b996e6dba5258..57aa5069a5904f 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -33,9 +33,10 @@ #include #if CHIP_CONFIG_ENABLE_ICD_CIP -#include // nogncheck -#include // nogncheck -#endif // CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck +#include // nogncheck +#include // nogncheck +#endif // CHIP_CONFIG_ENABLE_ICD_CIP namespace chip { namespace Crypto { @@ -115,7 +116,8 @@ class ICDManager : public ICDListener, public TestEventTriggerHandler ~ICDManager() = default; void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider); + Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, + ICDCheckInBackOffStrategy * strategy); void Shutdown(); /** @@ -318,11 +320,12 @@ class ICDManager : public ICDListener, public TestEventTriggerHandler bool mIsBootUpResumeSubscriptionExecuted = false; #endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - PersistentStorageDelegate * mStorage = nullptr; - FabricTable * mFabricTable = nullptr; - Messaging::ExchangeManager * mExchangeManager = nullptr; - Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; - SubscriptionsInfoProvider * mSubInfoProvider = nullptr; + PersistentStorageDelegate * mStorage = nullptr; + FabricTable * mFabricTable = nullptr; + Messaging::ExchangeManager * mExchangeManager = nullptr; + Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; + SubscriptionsInfoProvider * mSubInfoProvider = nullptr; + ICDCheckInBackOffStrategy * mICDCheckInBackOffStrategy = nullptr; ObjectPool mICDSenderPool; #endif // CHIP_CONFIG_ENABLE_ICD_CIP diff --git a/src/app/icd/server/tests/BUILD.gn b/src/app/icd/server/tests/BUILD.gn index 95ece1d28ae4df..9b08c9e17893ca 100644 --- a/src/app/icd/server/tests/BUILD.gn +++ b/src/app/icd/server/tests/BUILD.gn @@ -22,6 +22,7 @@ chip_test_suite("tests") { output_name = "libICDServerTests" test_sources = [ + "TestDefaultICDCheckInBackOffStrategy.cpp", "TestICDManager.cpp", "TestICDMonitoringTable.cpp", ] @@ -29,6 +30,7 @@ chip_test_suite("tests") { sources = [ "ICDConfigurationDataTestAccess.h" ] public_deps = [ + "${chip_root}/src/app/icd/server:default-check-in-back-off", "${chip_root}/src/app/icd/server:manager", "${chip_root}/src/app/icd/server:monitoring-table", "${chip_root}/src/lib/core:string-builder-adapters", diff --git a/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp b/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp new file mode 100644 index 00000000000000..b873379826d468 --- /dev/null +++ b/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp @@ -0,0 +1,57 @@ +/* + * + * Copyright (c) 2024 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 + +#include +#include +#include +#include +#include +#include + +using namespace chip; +using namespace chip::app; +using namespace chip::app::Clusters::IcdManagement; + +using TestSessionKeystoreImpl = Crypto::DefaultSessionKeystore; + +namespace { + +TEST(TestDefaultICDCheckInBackOffStrategy, TestShouldSendCheckInMessagePermanentClient) +{ + TestSessionKeystoreImpl keystore; + ICDMonitoringEntry entry(&keystore); + + entry.clientType = ClientTypeEnum::kPermanent; + + DefaultICDCheckInBackOffStrategy strategy; + EXPECT_TRUE(strategy.ShouldSendCheckInMessage(entry)); +} + +TEST(TestDefaultICDCheckInBackOffStrategy, TestShouldSendCheckInMessageEphemeralClient) +{ + TestSessionKeystoreImpl keystore; + ICDMonitoringEntry entry(&keystore); + + entry.clientType = ClientTypeEnum::kEphemeral; + + DefaultICDCheckInBackOffStrategy strategy; + EXPECT_FALSE(strategy.ShouldSendCheckInMessage(entry)); +} + +} // namespace diff --git a/src/app/icd/server/tests/TestICDManager.cpp b/src/app/icd/server/tests/TestICDManager.cpp index 3672955bdf4b26..f4b5db2eb42c2a 100644 --- a/src/app/icd/server/tests/TestICDManager.cpp +++ b/src/app/icd/server/tests/TestICDManager.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -197,7 +198,7 @@ class TestICDManager : public Test::LoopbackMessagingContext mICDStateObserver.ResetAll(); mICDManager.RegisterObserver(&mICDStateObserver); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider); + mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); } // Performs teardown for each individual test in the test suite @@ -212,6 +213,7 @@ class TestICDManager : public Test::LoopbackMessagingContext TestSubscriptionsInfoProvider mSubInfoProvider; TestPersistentStorageDelegate testStorage; TestICDStateObserver mICDStateObserver; + DefaultICDCheckInBackOffStrategy mStrategy; }; TEST_F(TestICDManager, TestICDModeDurations) @@ -568,7 +570,7 @@ TEST_F(TestICDManager, TestICDCounter) // Shut down and reinit ICDManager to increment counter mICDManager.Shutdown(); - mICDManager.Init(&(testStorage), &GetFabricTable(), &(mKeystore), &GetExchangeManager(), &(mSubInfoProvider)); + mICDManager.Init(&(testStorage), &GetFabricTable(), &(mKeystore), &GetExchangeManager(), &(mSubInfoProvider), &mStrategy); mICDManager.RegisterObserver(&(mICDStateObserver)); EXPECT_EQ(counter + ICDConfigurationData::kICDCounterPersistenceIncrement, @@ -973,7 +975,7 @@ TEST_F(TestICDManager, TestICDStateObserverOnICDModeChangeOnInit) // Shut down and reinit ICDManager - We should go to LIT mode since we have a registration mICDManager.Shutdown(); mICDManager.RegisterObserver(&(mICDStateObserver)); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider); + mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); // We have a registration, transition to LIT mode EXPECT_TRUE(mICDStateObserver.mOnICDModeChangeCalled); diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn index 51a259c86d2552..264e7e2e6d9daf 100644 --- a/src/app/server/BUILD.gn +++ b/src/app/server/BUILD.gn @@ -72,5 +72,12 @@ static_library("server") { if (chip_enable_icd_server) { public_deps += [ "${chip_root}/src/app/icd/server:notifier" ] + + if (chip_enable_icd_checkin) { + public_deps += [ + "${chip_root}/src/app/icd/server:check-in-back-off", + "${chip_root}/src/app/icd/server:default-check-in-back-off", + ] + } } } diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 5be815f0864c95..23d3054179edaa 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -360,7 +360,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) mICDManager.RegisterObserver(&app::DnssdServer::Instance()); mICDManager.Init(mDeviceStorage, &GetFabricTable(), mSessionKeystore, &mExchangeMgr, - chip::app::InteractionModelEngine::GetInstance()); + chip::app::InteractionModelEngine::GetInstance(), initParams.icdCheckInBackOffStrategy); // Register Test Event Trigger Handler if (mTestEventTriggerDelegate != nullptr) @@ -769,5 +769,8 @@ app::SimpleSubscriptionResumptionStorage CommonCaseDeviceServerInitParams::sSubs #endif app::DefaultAclStorage CommonCaseDeviceServerInitParams::sAclStorage; Crypto::DefaultSessionKeystore CommonCaseDeviceServerInitParams::sSessionKeystore; +#if CHIP_CONFIG_ENABLE_ICD_CIP +app::DefaultICDCheckInBackOffStrategy CommonCaseDeviceServerInitParams::sDefaultICDCheckInBackOffStrategy; +#endif } // namespace chip diff --git a/src/app/server/Server.h b/src/app/server/Server.h index d649e0fc923896..933b3438d3488a 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -69,8 +69,13 @@ #include #include +#include #if CHIP_CONFIG_ENABLE_ICD_SERVER #include // nogncheck + +#if CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck +#endif #endif namespace chip { @@ -164,6 +169,9 @@ struct ServerInitParams Credentials::OperationalCertificateStore * opCertStore = nullptr; // Required, if not provided, the Server::Init() WILL fail. app::reporting::ReportScheduler * reportScheduler = nullptr; + // Optionnal. Support for the ICD Check-In BackOff strategy. Must be initialized before being provided. + // If the ICD Check-In protocol use-case is supported and no strategy is prprovided, server will use the default strategy. + app::ICDCheckInBackOffStrategy * icdCheckInBackOffStrategy = nullptr; }; /** @@ -278,6 +286,13 @@ struct CommonCaseDeviceServerInitParams : public ServerInitParams ChipLogProgress(AppServer, "Subscription persistence not supported"); #endif +#if CHIP_CONFIG_ENABLE_ICD_CIP + if (this->icdCheckInBackOffStrategy == nullptr) + { + this->icdCheckInBackOffStrategy = &sDefaultICDCheckInBackOffStrategy; + } +#endif + return CHIP_NO_ERROR; } @@ -297,6 +312,9 @@ struct CommonCaseDeviceServerInitParams : public ServerInitParams #endif static app::DefaultAclStorage sAclStorage; static Crypto::DefaultSessionKeystore sSessionKeystore; +#if CHIP_CONFIG_ENABLE_ICD_CIP + static app::DefaultICDCheckInBackOffStrategy sDefaultICDCheckInBackOffStrategy; +#endif }; /**