From ae32b56e0a55790c0b44c19683d0c8a749a8e77e Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 16:16:46 +0200 Subject: [PATCH 01/16] Use the working version of the proto defs --- collector/proto/third_party/stackrox | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/collector/proto/third_party/stackrox b/collector/proto/third_party/stackrox index 0e536c9e74..50ff7c402a 160000 --- a/collector/proto/third_party/stackrox +++ b/collector/proto/third_party/stackrox @@ -1 +1 @@ -Subproject commit 0e536c9e7423aac43e1f79d8245ba52b311eef91 +Subproject commit 50ff7c402ab239e1f2ba2c11e7a763eba2486925 From d4c351ced97b7f510ebb44e7ae266d181a6e71c0 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 16:27:25 +0200 Subject: [PATCH 02/16] External-IPs can be enabled just in one direction --- collector/lib/CollectorConfig.cpp | 37 ++++++++++++++++++++++-- collector/lib/CollectorConfig.h | 38 +++++++++++++++++++------ collector/lib/ConnTracker.cpp | 33 ++++++++++++--------- collector/lib/ConnTracker.h | 14 +++++---- collector/lib/NetworkStatusNotifier.cpp | 16 +++++++---- collector/test/CollectorConfigTest.cpp | 15 +++++++--- collector/test/ConfigLoaderTest.cpp | 21 ++++++-------- collector/test/ConnTrackerTest.cpp | 10 +++---- 8 files changed, 128 insertions(+), 56 deletions(-) diff --git a/collector/lib/CollectorConfig.cpp b/collector/lib/CollectorConfig.cpp index 54a434a5c1..986605c740 100644 --- a/collector/lib/CollectorConfig.cpp +++ b/collector/lib/CollectorConfig.cpp @@ -91,7 +91,6 @@ constexpr const char* CollectorConfig::kSyscalls[]; constexpr bool CollectorConfig::kEnableProcessesListeningOnPorts; const UnorderedSet CollectorConfig::kIgnoredL4ProtoPortPairs = {{L4Proto::UDP, 9}}; -; CollectorConfig::CollectorConfig() { // Set default configuration values @@ -439,7 +438,7 @@ std::ostream& operator<<(std::ostream& os, const CollectorConfig& c) { << ", set_import_users:" << c.ImportUsers() << ", collect_connection_status:" << c.CollectConnectionStatus() << ", enable_detailed_metrics:" << c.EnableDetailedMetrics() - << ", enable_external_ips:" << c.EnableExternalIPs() + << ", external_ips:" << c.ExternalIPsConf() << ", track_send_recv:" << c.TrackingSendRecv(); } @@ -545,4 +544,38 @@ std::pair CollectorConfig::CheckConfiguration(co return std::make_pair(ARG_OK, ""); } +CollectorConfig::ExternalIPsConfig::ExternalIPsConfig(std::optional runtime_config, bool default_enabled) + : direction_enabled_(Direction::NONE) { + if (runtime_config.has_value()) { + if (runtime_config.value().networking().external_ips().enabled() == sensor::ExternalIpsEnabled::ENABLED) { + switch (runtime_config.value().networking().external_ips().direction()) { + case sensor::ExternalIpsDirection::INGRESS: + direction_enabled_ = Direction::INGRESS; + break; + case sensor::ExternalIpsDirection::EGRESS: + direction_enabled_ = Direction::EGRESS; + break; + default: + direction_enabled_ = Direction::BOTH; + break; + } + } + } else if (default_enabled) { + direction_enabled_ = Direction::BOTH; + } +} + +std::ostream& operator<<(std::ostream& os, const collector::CollectorConfig::ExternalIPsConfig& config) { + static std::unordered_map mapping = { + {CollectorConfig::ExternalIPsConfig::Direction::NONE, "NONE"}, + {CollectorConfig::ExternalIPsConfig::Direction::INGRESS, "INGRESS"}, + {CollectorConfig::ExternalIPsConfig::Direction::EGRESS, "EGRESS"}, + {CollectorConfig::ExternalIPsConfig::Direction::BOTH, "BOTH"}, + }; + + auto str = mapping.find(config.GetDirection()); + + return os << "direction(" << ((str == mapping.end()) ? "invalid" : (*str).second) << ")"; +} + } // namespace collector diff --git a/collector/lib/CollectorConfig.h b/collector/lib/CollectorConfig.h index e62f658dad..55c74c38d1 100644 --- a/collector/lib/CollectorConfig.h +++ b/collector/lib/CollectorConfig.h @@ -94,18 +94,39 @@ class CollectorConfig { bool ImportUsers() const { return import_users_; } bool CollectConnectionStatus() const { return collect_connection_status_; } + // Encapsulates the configuration of the External-IPs feature + class ExternalIPsConfig { + public: + enum Direction { + NONE = 0, + INGRESS = 1 << 0, + EGRESS = 1 << 1, + BOTH = INGRESS | EGRESS, + }; + + // Are External-IPs enabled in the provided direction ? + bool IsEnabled(Direction direction) { return (direction & direction_enabled_) == direction; } + + // Direction in which External-IPs are enabled + Direction GetDirection() const { return direction_enabled_; } + + // Extract the External-IPs configuration from the provided runtime-conf. + // If the runtime-configuration is unset then 'default_enabled' is used + // as a fallback to enable in both directions. + // 'runtime_config' should be locked prior to calling. + ExternalIPsConfig(std::optional runtime_config, bool default_enabled); + + private: + Direction direction_enabled_; + }; + // EnableExternalIPs will check for the existence // of a runtime configuration, and defer to that value // otherwise, we rely on the feature flag (env var) - bool EnableExternalIPs() const { + ExternalIPsConfig ExternalIPsConf() const { auto lock = ReadLock(); - if (runtime_config_.has_value()) { - return runtime_config_.value() - .networking() - .external_ips() - .enabled() == sensor::ExternalIpsEnabled::ENABLED; - } - return enable_external_ips_; + + return ExternalIPsConfig(runtime_config_, enable_external_ips_); } void RuntimeConfigHeuristics() { @@ -266,5 +287,6 @@ class CollectorConfig { }; std::ostream& operator<<(std::ostream& os, const CollectorConfig& c); +std::ostream& operator<<(std::ostream& os, const collector::CollectorConfig::ExternalIPsConfig& config); } // end namespace collector diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index 9b21934417..0eb95d8dc7 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -112,7 +112,7 @@ IPNet ConnectionTracker::NormalizeAddressNoLock(const Address& address, bool ena } bool ConnectionTracker::ShouldNormalizeConnection(const Connection* conn) const { - Endpoint local, remote = conn->remote(); + Endpoint remote = conn->remote(); IPNet ipnet = NormalizeAddressNoLock(remote.address(), false); return Address::IsCanonicalExternalIp(ipnet.address()); @@ -138,28 +138,35 @@ void ConnectionTracker::CloseConnections(ConnMap* old_conn_state, ConnMap* delta /** * Closes connections that have the 255.255.255.255 external IP address + * Affects only connections with a matching is_server property */ -void ConnectionTracker::CloseNormalizedConnections(ConnMap* old_conn_state, ConnMap* delta_conn) { - CloseConnections(old_conn_state, delta_conn, [](const Connection* conn) { - return Address::IsCanonicalExternalIp(conn->remote().address()); +void ConnectionTracker::CloseNormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn) { + CloseConnections(old_conn_state, delta_conn, [is_server](const Connection* conn) { + return conn->is_server() == is_server && Address::IsCanonicalExternalIp(conn->remote().address()); }); } /** * Closes unnormalized connections that would be normalized to the canonical external * IP address if external IPs was enabled + * Affects only connections with a matching is_server property */ -void ConnectionTracker::CloseExternalUnnormalizedConnections(ConnMap* old_conn_state, ConnMap* delta_conn) { - CloseConnections(old_conn_state, delta_conn, [this](const Connection* conn) { - return ShouldNormalizeConnection(conn) && !Address::IsCanonicalExternalIp(conn->remote().address()); +void ConnectionTracker::CloseExternalUnnormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn) { + CloseConnections(old_conn_state, delta_conn, [this, is_server](const Connection* conn) { + return conn->is_server() == is_server && ShouldNormalizeConnection(conn) && !Address::IsCanonicalExternalIp(conn->remote().address()); }); } -void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn, bool enableExternalIPs) { - if (enableExternalIPs) { - CloseNormalizedConnections(old_conn_state, delta_conn); +void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn) { + if (enable_external_ips_egress_) { + CloseNormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); } else { - CloseExternalUnnormalizedConnections(old_conn_state, delta_conn); + CloseExternalUnnormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); + } + if (enable_external_ips_ingress_) { + CloseNormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); + } else { + CloseExternalUnnormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); } } @@ -175,11 +182,11 @@ Connection ConnectionTracker::NormalizeConnectionNoLock(const Connection& conn) if (is_server) { // If this is the server, only the local port is relevant, while the remote port does not matter. local = Endpoint(IPNet(Address()), conn.local().port()); - remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), enable_external_ips_), 0); + remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), enable_external_ips_ingress_), 0); } else { // If this is the client, the local port and address are not relevant. local = Endpoint(); - remote = Endpoint(NormalizeAddressNoLock(remote.address(), enable_external_ips_), remote.port()); + remote = Endpoint(NormalizeAddressNoLock(remote.address(), enable_external_ips_egress_), remote.port()); } return Connection(conn.container(), local, remote, conn.l4proto(), is_server); diff --git a/collector/lib/ConnTracker.h b/collector/lib/ConnTracker.h index d4fbe5fe1f..20f24174d5 100644 --- a/collector/lib/ConnTracker.h +++ b/collector/lib/ConnTracker.h @@ -101,9 +101,9 @@ class ConnectionTracker { static void UpdateOldState(UnorderedMap* old_state, const UnorderedMap& new_state, int64_t time_micros, int64_t afterglow_period_micros); void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function predicate); - void CloseNormalizedConnections(ConnMap* old_conn_state, ConnMap* delta_conn); - void CloseExternalUnnormalizedConnections(ConnMap* old_conn_state, ConnMap* delta_conn); - void CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn, bool enableExternalIPs); + void CloseNormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); + void CloseExternalUnnormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); + void CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn); // ComputeDelta computes a diff between new_state and old_state template @@ -131,7 +131,10 @@ class ConnectionTracker { void UpdateKnownPublicIPs(UnorderedSet
&& known_public_ips); void UpdateKnownIPNetworks(UnorderedMap>&& known_ip_networks); - void EnableExternalIPs(bool enable) { enable_external_ips_ = enable; } + void EnableExternalIPs(bool enable_ingress, bool enable_egress) { + enable_external_ips_ingress_ = enable_ingress; + enable_external_ips_egress_ = enable_egress; + } void UpdateIgnoredL4ProtoPortPairs(UnorderedSet&& ignored_l4proto_port_pairs); void UpdateIgnoredNetworks(const std::vector& network_list); void UpdateNonAggregatedNetworks(const std::vector& network_list); @@ -202,7 +205,8 @@ class ConnectionTracker { UnorderedSet
known_public_ips_; NRadixTree known_ip_networks_; - bool enable_external_ips_ = false; + bool enable_external_ips_ingress_ = false; + bool enable_external_ips_egress_ = false; UnorderedMap known_private_networks_exists_; UnorderedSet ignored_l4proto_port_pairs_; NRadixTree ignored_networks_; diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index 8d37baf09a..0f9fc773c1 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -16,6 +16,8 @@ namespace collector { namespace { +using Direction = CollectorConfig::ExternalIPsConfig::Direction; + storage::L4Protocol TranslateL4Protocol(L4Proto proto) { switch (proto) { case L4Proto::TCP: @@ -225,7 +227,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterSleep(next_scrape)) { CLOG(DEBUG) << "Starting network status notification"; @@ -242,17 +244,19 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterEnableExternalIPs(enableExternalIPs); + conn_tracker_->EnableExternalIPs( + externalIPsConfig.IsEnabled(Direction::INGRESS), + externalIPsConfig.IsEnabled(Direction::EGRESS)); new_conn_state = conn_tracker_->FetchConnState(true, true); if (config_.EnableAfterglow()) { ConnectionTracker::ComputeDeltaAfterglow(new_conn_state, old_conn_state, delta_conn, time_micros, time_at_last_scrape, config_.AfterglowPeriod()); - if (prevEnableExternalIPs != enableExternalIPs) { - conn_tracker_->CloseConnectionsOnRuntimeConfigChange(&old_conn_state, &delta_conn, enableExternalIPs); - prevEnableExternalIPs = enableExternalIPs; + if (prevEnableExternalIPs.GetDirection() != externalIPsConfig.GetDirection()) { + conn_tracker_->CloseConnectionsOnRuntimeConfigChange(&old_conn_state, &delta_conn); + prevEnableExternalIPs = externalIPsConfig; } } else { ConnectionTracker::ComputeDelta(new_conn_state, &old_conn_state); diff --git a/collector/test/CollectorConfigTest.cpp b/collector/test/CollectorConfigTest.cpp index 8b03bb9af2..5bb354ecca 100644 --- a/collector/test/CollectorConfigTest.cpp +++ b/collector/test/CollectorConfigTest.cpp @@ -8,6 +8,7 @@ #include "gtest/gtest.h" using namespace testing; +using Direction = collector::CollectorConfig::ExternalIPsConfig::Direction; namespace collector { @@ -115,11 +116,11 @@ TEST(CollectorConfigTest, TestEnableExternalIpsFeatureFlag) { config.MockSetEnableExternalIPs(false); - EXPECT_FALSE(config.EnableExternalIPs()); + EXPECT_EQ(Direction::NONE, config.ExternalIPsConf().GetDirection()); config.MockSetEnableExternalIPs(true); - EXPECT_TRUE(config.EnableExternalIPs()); + EXPECT_EQ(Direction::BOTH, config.ExternalIPsConf().GetDirection()); } TEST(CollectorConfigTest, TestEnableExternalIpsRuntimeConfig) { @@ -138,14 +139,20 @@ TEST(CollectorConfigTest, TestEnableExternalIpsRuntimeConfig) { config.SetRuntimeConfig(runtime_config); - EXPECT_FALSE(config.EnableExternalIPs()); + EXPECT_EQ(Direction::NONE, config.ExternalIPsConf().GetDirection()); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); config.MockSetEnableExternalIPs(false); external_ips_config->set_enabled(sensor::ExternalIpsEnabled::ENABLED); config.SetRuntimeConfig(runtime_config); - EXPECT_TRUE(config.EnableExternalIPs()); + EXPECT_EQ(CollectorConfig::ExternalIPsConfig::Direction::BOTH, config.ExternalIPsConf().GetDirection()); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); } } // namespace collector diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp index 0ddfe16768..34943c986e 100644 --- a/collector/test/ConfigLoaderTest.cpp +++ b/collector/test/ConfigLoaderTest.cpp @@ -11,6 +11,7 @@ namespace collector { using namespace google::protobuf::util; +using Direction = CollectorConfig::ExternalIPsConfig::Direction; std::string ErrorsToString(const std::vector& errors) { std::stringstream ss; @@ -308,29 +309,28 @@ TEST(TestParserYaml, ValidationMode) { */ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) { - std::vector> tests = { + std::vector> tests = { {R"( networking: externalIps: enabled: enabled )", - true}, + Direction::BOTH}, {R"( networking: externalIps: enabled: DISABLED )", - false}, + Direction::NONE}, {R"( networking: externalIps: )", - false}, - { - R"( + Direction::NONE}, + {R"( networking: )", - false}, + Direction::NONE}, }; for (const auto& [yamlStr, expected] : tests) { @@ -342,12 +342,7 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) { EXPECT_TRUE(runtime_config.has_value()); - bool enabled = runtime_config.value() - .networking() - .external_ips() - .enabled() == sensor::ExternalIpsEnabled::ENABLED; - EXPECT_EQ(enabled, expected); - EXPECT_EQ(config.EnableExternalIPs(), expected); + EXPECT_EQ(config.ExternalIPsConf().GetDirection(), expected); } } diff --git a/collector/test/ConnTrackerTest.cpp b/collector/test/ConnTrackerTest.cpp index aa6542c083..ead208b17b 100644 --- a/collector/test/ConnTrackerTest.cpp +++ b/collector/test/ConnTrackerTest.cpp @@ -415,7 +415,7 @@ TEST(ConnTrackerTest, TestNormalizedEnableExternalIPs) { int64_t time_micros = 1000; ConnectionTracker tracker; - tracker.EnableExternalIPs(true); + tracker.EnableExternalIPs(true, true); UnorderedMap> known_networks = {{Address::Family::IPV4, {IPNet(Address(35, 127, 1, 0), 24)}}}; tracker.UpdateKnownIPNetworks(std::move(known_networks)); @@ -1664,7 +1664,7 @@ TEST(ConnTrackerTest, TestCloseNormalizedConnections) { ConnMap delta; ConnMap expected_delta = {{conn, ConnStatus(connection_time, false)}}; - tracker.CloseNormalizedConnections(&old_state, &delta); + tracker.CloseNormalizedConnections(true, &old_state, &delta); EXPECT_THAT(old_state, IsEmpty()); EXPECT_THAT(delta, expected_delta); @@ -1683,7 +1683,7 @@ TEST(CloseNormalizedConnectionsTest, UnnormalizedConnectionsAreKept) { ConnMap delta; ConnMap expected_old_state = {{conn, ConnStatus(connection_time, true)}}; - tracker.CloseNormalizedConnections(&old_state, &delta); + tracker.CloseNormalizedConnections(true, &old_state, &delta); EXPECT_THAT(old_state, expected_old_state); EXPECT_THAT(delta, IsEmpty()); @@ -1702,7 +1702,7 @@ TEST(ConnTrackerTest, TestCloseExternalUnnormalizedConnections) { ConnMap delta; ConnMap expected_delta = {{conn, ConnStatus(connection_time, false)}}; - tracker.CloseExternalUnnormalizedConnections(&old_state, &delta); + tracker.CloseExternalUnnormalizedConnections(true, &old_state, &delta); EXPECT_THAT(old_state, IsEmpty()); EXPECT_THAT(delta, expected_delta); @@ -1721,7 +1721,7 @@ TEST(CloseExternalUnnormalizedConnectionsTest, InternalConnectionsAreKept) { ConnMap delta; ConnMap expected_old_state = {{conn, ConnStatus(connection_time, true)}}; - tracker.CloseExternalUnnormalizedConnections(&old_state, &delta); + tracker.CloseExternalUnnormalizedConnections(true, &old_state, &delta); EXPECT_THAT(old_state, expected_old_state); EXPECT_THAT(delta, IsEmpty()); From 7195315aadc2a22ffe17606ed24bbb5ede837d2b Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 16:54:09 +0200 Subject: [PATCH 03/16] Additional unit tests with ingres and egress directions --- collector/test/ConfigLoaderTest.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp index 34943c986e..96625d9039 100644 --- a/collector/test/ConfigLoaderTest.cpp +++ b/collector/test/ConfigLoaderTest.cpp @@ -316,6 +316,27 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) { enabled: enabled )", Direction::BOTH}, + {R"( + networking: + externalIps: + enabled: enabled + direction: ingress + )", + Direction::INGRESS}, + {R"( + networking: + externalIps: + enabled: enabled + direction: egress + )", + Direction::EGRESS}, + {R"( + networking: + externalIps: + enabled: enabled + direction: both + )", + Direction::BOTH}, {R"( networking: externalIps: From 13f2ef58624733e9d04b906e2fc0da537f38ca68 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 18:06:13 +0200 Subject: [PATCH 04/16] Rewrite to make the logic clearer --- collector/lib/CollectorConfig.cpp | 70 +++++++++++++++++++------------ 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/collector/lib/CollectorConfig.cpp b/collector/lib/CollectorConfig.cpp index 986605c740..f1278a5e73 100644 --- a/collector/lib/CollectorConfig.cpp +++ b/collector/lib/CollectorConfig.cpp @@ -544,38 +544,54 @@ std::pair CollectorConfig::CheckConfiguration(co return std::make_pair(ARG_OK, ""); } -CollectorConfig::ExternalIPsConfig::ExternalIPsConfig(std::optional runtime_config, bool default_enabled) - : direction_enabled_(Direction::NONE) { - if (runtime_config.has_value()) { - if (runtime_config.value().networking().external_ips().enabled() == sensor::ExternalIpsEnabled::ENABLED) { - switch (runtime_config.value().networking().external_ips().direction()) { - case sensor::ExternalIpsDirection::INGRESS: - direction_enabled_ = Direction::INGRESS; - break; - case sensor::ExternalIpsDirection::EGRESS: - direction_enabled_ = Direction::EGRESS; - break; - default: - direction_enabled_ = Direction::BOTH; - break; - } - } - } else if (default_enabled) { - direction_enabled_ = Direction::BOTH; +CollectorConfig::ExternalIPsConfig::ExternalIPsConfig(std::optional runtime_config, bool default_enabled) { + if (!runtime_config.has_value()) { + direction_enabled_ = default_enabled ? Direction::BOTH : Direction::NONE; + return; + } + + // At this point we know runtime_config has a value, we can access it directly + const auto& external_ips = runtime_config->networking().external_ips(); + if (external_ips.enabled() != sensor::ExternalIpsEnabled::ENABLED) { + direction_enabled_ = Direction::NONE; + return; + } + + switch (external_ips.direction()) { + case sensor::ExternalIpsDirection::INGRESS: + direction_enabled_ = Direction::INGRESS; + break; + case sensor::ExternalIpsDirection::EGRESS: + direction_enabled_ = Direction::EGRESS; + break; + default: + direction_enabled_ = Direction::BOTH; + break; } } std::ostream& operator<<(std::ostream& os, const collector::CollectorConfig::ExternalIPsConfig& config) { - static std::unordered_map mapping = { - {CollectorConfig::ExternalIPsConfig::Direction::NONE, "NONE"}, - {CollectorConfig::ExternalIPsConfig::Direction::INGRESS, "INGRESS"}, - {CollectorConfig::ExternalIPsConfig::Direction::EGRESS, "EGRESS"}, - {CollectorConfig::ExternalIPsConfig::Direction::BOTH, "BOTH"}, - }; - - auto str = mapping.find(config.GetDirection()); + os << "direction("; + + switch (config.GetDirection()) { + case CollectorConfig::ExternalIPsConfig::Direction::NONE: + os << "NONE"; + break; + case CollectorConfig::ExternalIPsConfig::Direction::INGRESS: + os << "INGRESS"; + break; + case CollectorConfig::ExternalIPsConfig::Direction::EGRESS: + os << "EGRESS"; + break; + case CollectorConfig::ExternalIPsConfig::Direction::BOTH: + os << "BOTH"; + break; + default: + os << "invalid"; + break; + } - return os << "direction(" << ((str == mapping.end()) ? "invalid" : (*str).second) << ")"; + return os << ")"; } } // namespace collector From 2e17cf498660662c0c2deacddf325bda6f5eddca Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 18:38:40 +0200 Subject: [PATCH 05/16] Move ExternalIPsConfig to its own file. --- collector/lib/CollectorConfig.cpp | 50 ---------------------- collector/lib/CollectorConfig.h | 28 +------------ collector/lib/ExternalIPsConfig.cpp | 55 +++++++++++++++++++++++++ collector/lib/ExternalIPsConfig.h | 40 ++++++++++++++++++ collector/lib/NetworkStatusNotifier.cpp | 10 ++--- collector/test/CollectorConfigTest.cpp | 4 +- collector/test/ConfigLoaderTest.cpp | 2 +- 7 files changed, 103 insertions(+), 86 deletions(-) create mode 100644 collector/lib/ExternalIPsConfig.cpp create mode 100644 collector/lib/ExternalIPsConfig.h diff --git a/collector/lib/CollectorConfig.cpp b/collector/lib/CollectorConfig.cpp index f1278a5e73..810eba62aa 100644 --- a/collector/lib/CollectorConfig.cpp +++ b/collector/lib/CollectorConfig.cpp @@ -544,54 +544,4 @@ std::pair CollectorConfig::CheckConfiguration(co return std::make_pair(ARG_OK, ""); } -CollectorConfig::ExternalIPsConfig::ExternalIPsConfig(std::optional runtime_config, bool default_enabled) { - if (!runtime_config.has_value()) { - direction_enabled_ = default_enabled ? Direction::BOTH : Direction::NONE; - return; - } - - // At this point we know runtime_config has a value, we can access it directly - const auto& external_ips = runtime_config->networking().external_ips(); - if (external_ips.enabled() != sensor::ExternalIpsEnabled::ENABLED) { - direction_enabled_ = Direction::NONE; - return; - } - - switch (external_ips.direction()) { - case sensor::ExternalIpsDirection::INGRESS: - direction_enabled_ = Direction::INGRESS; - break; - case sensor::ExternalIpsDirection::EGRESS: - direction_enabled_ = Direction::EGRESS; - break; - default: - direction_enabled_ = Direction::BOTH; - break; - } -} - -std::ostream& operator<<(std::ostream& os, const collector::CollectorConfig::ExternalIPsConfig& config) { - os << "direction("; - - switch (config.GetDirection()) { - case CollectorConfig::ExternalIPsConfig::Direction::NONE: - os << "NONE"; - break; - case CollectorConfig::ExternalIPsConfig::Direction::INGRESS: - os << "INGRESS"; - break; - case CollectorConfig::ExternalIPsConfig::Direction::EGRESS: - os << "EGRESS"; - break; - case CollectorConfig::ExternalIPsConfig::Direction::BOTH: - os << "BOTH"; - break; - default: - os << "invalid"; - break; - } - - return os << ")"; -} - } // namespace collector diff --git a/collector/lib/CollectorConfig.h b/collector/lib/CollectorConfig.h index 55c74c38d1..90af256654 100644 --- a/collector/lib/CollectorConfig.h +++ b/collector/lib/CollectorConfig.h @@ -14,6 +14,7 @@ #include #include "CollectionMethod.h" +#include "ExternalIPsConfig.h" #include "HostConfig.h" #include "Logging.h" #include "NetworkConnection.h" @@ -94,32 +95,6 @@ class CollectorConfig { bool ImportUsers() const { return import_users_; } bool CollectConnectionStatus() const { return collect_connection_status_; } - // Encapsulates the configuration of the External-IPs feature - class ExternalIPsConfig { - public: - enum Direction { - NONE = 0, - INGRESS = 1 << 0, - EGRESS = 1 << 1, - BOTH = INGRESS | EGRESS, - }; - - // Are External-IPs enabled in the provided direction ? - bool IsEnabled(Direction direction) { return (direction & direction_enabled_) == direction; } - - // Direction in which External-IPs are enabled - Direction GetDirection() const { return direction_enabled_; } - - // Extract the External-IPs configuration from the provided runtime-conf. - // If the runtime-configuration is unset then 'default_enabled' is used - // as a fallback to enable in both directions. - // 'runtime_config' should be locked prior to calling. - ExternalIPsConfig(std::optional runtime_config, bool default_enabled); - - private: - Direction direction_enabled_; - }; - // EnableExternalIPs will check for the existence // of a runtime configuration, and defer to that value // otherwise, we rely on the feature flag (env var) @@ -287,6 +262,5 @@ class CollectorConfig { }; std::ostream& operator<<(std::ostream& os, const CollectorConfig& c); -std::ostream& operator<<(std::ostream& os, const collector::CollectorConfig::ExternalIPsConfig& config); } // end namespace collector diff --git a/collector/lib/ExternalIPsConfig.cpp b/collector/lib/ExternalIPsConfig.cpp new file mode 100644 index 0000000000..1d57ecda65 --- /dev/null +++ b/collector/lib/ExternalIPsConfig.cpp @@ -0,0 +1,55 @@ +#include "ExternalIPsConfig.h" + +namespace collector { + +ExternalIPsConfig::ExternalIPsConfig(std::optional runtime_config, bool default_enabled) { + if (!runtime_config.has_value()) { + direction_enabled_ = default_enabled ? Direction::BOTH : Direction::NONE; + return; + } + + // At this point we know runtime_config has a value, we can access it directly + const auto& external_ips = runtime_config->networking().external_ips(); + if (external_ips.enabled() != sensor::ExternalIpsEnabled::ENABLED) { + direction_enabled_ = Direction::NONE; + return; + } + + switch (external_ips.direction()) { + case sensor::ExternalIpsDirection::INGRESS: + direction_enabled_ = Direction::INGRESS; + break; + case sensor::ExternalIpsDirection::EGRESS: + direction_enabled_ = Direction::EGRESS; + break; + default: + direction_enabled_ = Direction::BOTH; + break; + } +} + +std::ostream& operator<<(std::ostream& os, const ExternalIPsConfig& config) { + os << "direction("; + + switch (config.GetDirection()) { + case ExternalIPsConfig::Direction::NONE: + os << "NONE"; + break; + case ExternalIPsConfig::Direction::INGRESS: + os << "INGRESS"; + break; + case ExternalIPsConfig::Direction::EGRESS: + os << "EGRESS"; + break; + case ExternalIPsConfig::Direction::BOTH: + os << "BOTH"; + break; + default: + os << "invalid"; + break; + } + + return os << ")"; +} + +} // namespace collector \ No newline at end of file diff --git a/collector/lib/ExternalIPsConfig.h b/collector/lib/ExternalIPsConfig.h new file mode 100644 index 0000000000..ffe5976db8 --- /dev/null +++ b/collector/lib/ExternalIPsConfig.h @@ -0,0 +1,40 @@ +#pragma once + +#include +#include + +#include + +namespace collector { + +// Encapsulates the configuration of the External-IPs feature +class ExternalIPsConfig { + public: + enum Direction { + NONE = 0, + INGRESS = 1 << 0, + EGRESS = 1 << 1, + BOTH = INGRESS | EGRESS, + }; + + // Are External-IPs enabled in the provided direction ? + bool IsEnabled(Direction direction) const { return (direction & direction_enabled_) == direction; } + + // Direction in which External-IPs are enabled + Direction GetDirection() const { return direction_enabled_; } + + // Extract the External-IPs configuration from the provided runtime-conf. + // If the runtime-configuration is unset then 'default_enabled' is used + // as a fallback to enable in both directions. + // 'runtime_config' should be locked prior to calling. + ExternalIPsConfig(std::optional runtime_config, bool default_enabled); + + ExternalIPsConfig() : direction_enabled_(Direction::NONE) {} + + private: + Direction direction_enabled_; +}; + +std::ostream& operator<<(std::ostream& os, const ExternalIPsConfig& config); + +} // end namespace collector \ No newline at end of file diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index 0f9fc773c1..57a27e224c 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -16,8 +16,6 @@ namespace collector { namespace { -using Direction = CollectorConfig::ExternalIPsConfig::Direction; - storage::L4Protocol TranslateL4Protocol(L4Proto proto) { switch (proto) { case L4Proto::TCP: @@ -227,7 +225,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterSleep(next_scrape)) { CLOG(DEBUG) << "Starting network status notification"; @@ -244,12 +242,12 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterEnableExternalIPs( - externalIPsConfig.IsEnabled(Direction::INGRESS), - externalIPsConfig.IsEnabled(Direction::EGRESS)); + externalIPsConfig.IsEnabled(ExternalIPsConfig::Direction::INGRESS), + externalIPsConfig.IsEnabled(ExternalIPsConfig::Direction::EGRESS)); new_conn_state = conn_tracker_->FetchConnState(true, true); if (config_.EnableAfterglow()) { diff --git a/collector/test/CollectorConfigTest.cpp b/collector/test/CollectorConfigTest.cpp index 5bb354ecca..cdbad03994 100644 --- a/collector/test/CollectorConfigTest.cpp +++ b/collector/test/CollectorConfigTest.cpp @@ -8,7 +8,7 @@ #include "gtest/gtest.h" using namespace testing; -using Direction = collector::CollectorConfig::ExternalIPsConfig::Direction; +using Direction = collector::ExternalIPsConfig::Direction; namespace collector { @@ -149,7 +149,7 @@ TEST(CollectorConfigTest, TestEnableExternalIpsRuntimeConfig) { external_ips_config->set_enabled(sensor::ExternalIpsEnabled::ENABLED); config.SetRuntimeConfig(runtime_config); - EXPECT_EQ(CollectorConfig::ExternalIPsConfig::Direction::BOTH, config.ExternalIPsConf().GetDirection()); + EXPECT_EQ(Direction::BOTH, config.ExternalIPsConf().GetDirection()); EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); diff --git a/collector/test/ConfigLoaderTest.cpp b/collector/test/ConfigLoaderTest.cpp index 96625d9039..e4df3da05d 100644 --- a/collector/test/ConfigLoaderTest.cpp +++ b/collector/test/ConfigLoaderTest.cpp @@ -11,7 +11,7 @@ namespace collector { using namespace google::protobuf::util; -using Direction = CollectorConfig::ExternalIPsConfig::Direction; +using Direction = ExternalIPsConfig::Direction; std::string ErrorsToString(const std::vector& errors) { std::stringstream ss; From 41109ea8bf1e96420db5bcb5d38c474cf989960b Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 20 May 2025 19:01:24 +0200 Subject: [PATCH 06/16] Directly make use of the ExternalIPsConfig in ConnTracker --- collector/lib/ConnTracker.cpp | 8 ++++---- collector/lib/ConnTracker.h | 9 +++------ collector/lib/NetworkStatusNotifier.cpp | 4 +--- collector/test/ConnTrackerTest.cpp | 2 +- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index 0eb95d8dc7..a6d1cde41d 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -158,12 +158,12 @@ void ConnectionTracker::CloseExternalUnnormalizedConnections(bool is_server, Con } void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn) { - if (enable_external_ips_egress_) { + if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { CloseNormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); } else { CloseExternalUnnormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); } - if (enable_external_ips_ingress_) { + if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { CloseNormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); } else { CloseExternalUnnormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); @@ -182,11 +182,11 @@ Connection ConnectionTracker::NormalizeConnectionNoLock(const Connection& conn) if (is_server) { // If this is the server, only the local port is relevant, while the remote port does not matter. local = Endpoint(IPNet(Address()), conn.local().port()); - remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), enable_external_ips_ingress_), 0); + remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)), 0); } else { // If this is the client, the local port and address are not relevant. local = Endpoint(); - remote = Endpoint(NormalizeAddressNoLock(remote.address(), enable_external_ips_egress_), remote.port()); + remote = Endpoint(NormalizeAddressNoLock(remote.address(), external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)), remote.port()); } return Connection(conn.container(), local, remote, conn.l4proto(), is_server); diff --git a/collector/lib/ConnTracker.h b/collector/lib/ConnTracker.h index 20f24174d5..37a26a931e 100644 --- a/collector/lib/ConnTracker.h +++ b/collector/lib/ConnTracker.h @@ -4,6 +4,7 @@ #include #include "Containers.h" +#include "ExternalIPsConfig.h" #include "Hash.h" #include "NRadix.h" #include "NetworkConnection.h" @@ -131,10 +132,7 @@ class ConnectionTracker { void UpdateKnownPublicIPs(UnorderedSet
&& known_public_ips); void UpdateKnownIPNetworks(UnorderedMap>&& known_ip_networks); - void EnableExternalIPs(bool enable_ingress, bool enable_egress) { - enable_external_ips_ingress_ = enable_ingress; - enable_external_ips_egress_ = enable_egress; - } + void SetExternalIPsConfig(ExternalIPsConfig config) { external_IPs_config_ = config; } void UpdateIgnoredL4ProtoPortPairs(UnorderedSet&& ignored_l4proto_port_pairs); void UpdateIgnoredNetworks(const std::vector& network_list); void UpdateNonAggregatedNetworks(const std::vector& network_list); @@ -205,8 +203,7 @@ class ConnectionTracker { UnorderedSet
known_public_ips_; NRadixTree known_ip_networks_; - bool enable_external_ips_ingress_ = false; - bool enable_external_ips_egress_ = false; + ExternalIPsConfig external_IPs_config_; UnorderedMap known_private_networks_exists_; UnorderedSet ignored_l4proto_port_pairs_; NRadixTree ignored_networks_; diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index 57a27e224c..3f91007784 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -245,9 +245,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterEnableExternalIPs( - externalIPsConfig.IsEnabled(ExternalIPsConfig::Direction::INGRESS), - externalIPsConfig.IsEnabled(ExternalIPsConfig::Direction::EGRESS)); + conn_tracker_->SetExternalIPsConfig(externalIPsConfig); new_conn_state = conn_tracker_->FetchConnState(true, true); if (config_.EnableAfterglow()) { diff --git a/collector/test/ConnTrackerTest.cpp b/collector/test/ConnTrackerTest.cpp index ead208b17b..53aedef2e0 100644 --- a/collector/test/ConnTrackerTest.cpp +++ b/collector/test/ConnTrackerTest.cpp @@ -415,7 +415,7 @@ TEST(ConnTrackerTest, TestNormalizedEnableExternalIPs) { int64_t time_micros = 1000; ConnectionTracker tracker; - tracker.EnableExternalIPs(true, true); + tracker.SetExternalIPsConfig(ExternalIPsConfig(std::nullopt, true)); UnorderedMap> known_networks = {{Address::Family::IPV4, {IPNet(Address(35, 127, 1, 0), 24)}}}; tracker.UpdateKnownIPNetworks(std::move(known_networks)); From a4faa0504debdd506818e87945a472f12b514d18 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Mon, 26 May 2025 17:04:44 +0200 Subject: [PATCH 07/16] Trigger afterglow early connection close independently for each direction --- collector/lib/ConnTracker.cpp | 24 +++++++++++++++--------- collector/lib/ConnTracker.h | 2 +- collector/lib/NetworkStatusNotifier.cpp | 2 +- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index a6d1cde41d..7d4e2bfbda 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -157,16 +157,22 @@ void ConnectionTracker::CloseExternalUnnormalizedConnections(bool is_server, Con }); } -void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn) { - if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { - CloseNormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); - } else { - CloseExternalUnnormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); +void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn) { + if (prev_config.IsEnabled(ExternalIPsConfig::Direction::EGRESS) != + external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { + if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { + CloseNormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); + } else { + CloseExternalUnnormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); + } } - if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { - CloseNormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); - } else { - CloseExternalUnnormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); + if (prev_config.IsEnabled(ExternalIPsConfig::Direction::INGRESS) != + external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { + if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { + CloseNormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); + } else { + CloseExternalUnnormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); + } } } diff --git a/collector/lib/ConnTracker.h b/collector/lib/ConnTracker.h index 37a26a931e..092b1b98e1 100644 --- a/collector/lib/ConnTracker.h +++ b/collector/lib/ConnTracker.h @@ -104,7 +104,7 @@ class ConnectionTracker { void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function predicate); void CloseNormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); void CloseExternalUnnormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); - void CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn); + void CloseConnectionsOnRuntimeConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn); // ComputeDelta computes a diff between new_state and old_state template diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index 3f91007784..cb6d134271 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -251,7 +251,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterCloseConnectionsOnRuntimeConfigChange(&old_conn_state, &delta_conn); + conn_tracker_->CloseConnectionsOnRuntimeConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn); prevEnableExternalIPs = externalIPsConfig; } } else { From bb0afb6402517d32a8955c356de4f2e38469bfd9 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Mon, 26 May 2025 17:06:49 +0200 Subject: [PATCH 08/16] Move config change detection logic to Conntracker --- collector/lib/NetworkStatusNotifier.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index cb6d134271..e10e896a89 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -250,10 +250,10 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterFetchConnState(true, true); if (config_.EnableAfterglow()) { ConnectionTracker::ComputeDeltaAfterglow(new_conn_state, old_conn_state, delta_conn, time_micros, time_at_last_scrape, config_.AfterglowPeriod()); - if (prevEnableExternalIPs.GetDirection() != externalIPsConfig.GetDirection()) { - conn_tracker_->CloseConnectionsOnRuntimeConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn); - prevEnableExternalIPs = externalIPsConfig; - } + + conn_tracker_->CloseConnectionsOnRuntimeConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn); + prevEnableExternalIPs = externalIPsConfig; + } else { ConnectionTracker::ComputeDelta(new_conn_state, &old_conn_state); } From 36fb9f4f7dc5f5a37c6d7a3b71f8999d398b91d5 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 27 May 2025 11:55:00 +0200 Subject: [PATCH 09/16] Refactor: limit the number of public methods and improve readability --- collector/lib/ConnTracker.cpp | 68 ++++---- collector/lib/ConnTracker.h | 8 +- collector/lib/ExternalIPsConfig.h | 2 +- collector/lib/NetworkStatusNotifier.cpp | 2 +- collector/test/ConnTrackerTest.cpp | 201 ++++++++++++++++-------- 5 files changed, 171 insertions(+), 110 deletions(-) diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index 7d4e2bfbda..6addec99bc 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -136,43 +136,41 @@ void ConnectionTracker::CloseConnections(ConnMap* old_conn_state, ConnMap* delta } } -/** - * Closes connections that have the 255.255.255.255 external IP address - * Affects only connections with a matching is_server property - */ -void ConnectionTracker::CloseNormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn) { - CloseConnections(old_conn_state, delta_conn, [is_server](const Connection* conn) { - return conn->is_server() == is_server && Address::IsCanonicalExternalIp(conn->remote().address()); - }); -} - -/** - * Closes unnormalized connections that would be normalized to the canonical external - * IP address if external IPs was enabled - * Affects only connections with a matching is_server property - */ -void ConnectionTracker::CloseExternalUnnormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn) { - CloseConnections(old_conn_state, delta_conn, [this, is_server](const Connection* conn) { - return conn->is_server() == is_server && ShouldNormalizeConnection(conn) && !Address::IsCanonicalExternalIp(conn->remote().address()); - }); -} +void ConnectionTracker::CloseConnectionsOnExternalIPsConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn) const { + bool ingress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS); + bool egress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS); + + if (egress != prev_config.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { + CloseConnections(old_conn_state, delta_conn, [this, egress](const Connection* conn) -> bool { + /* egress is when we are not server */ + if (conn->is_server()) { + return false; + } -void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn) { - if (prev_config.IsEnabled(ExternalIPsConfig::Direction::EGRESS) != - external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { - if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { - CloseNormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); - } else { - CloseExternalUnnormalizedConnections(/* egress is when we are not server */ false, old_conn_state, delta_conn); - } + if (egress) { + // Enabling: Close connections previously normalized + return Address::IsCanonicalExternalIp(conn->remote().address()); + } else { + // Disabling: Close connections that should now be normalized + return !Address::IsCanonicalExternalIp(conn->remote().address()) && ShouldNormalizeConnection(conn); + } + }); } - if (prev_config.IsEnabled(ExternalIPsConfig::Direction::INGRESS) != - external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { - if (external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { - CloseNormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); - } else { - CloseExternalUnnormalizedConnections(/* ingress is when we are server */ true, old_conn_state, delta_conn); - } + if (ingress != prev_config.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { + CloseConnections(old_conn_state, delta_conn, [this, ingress](const Connection* conn) -> bool { + /* ingress is when we are server */ + if (!conn->is_server()) { + return false; + } + + if (ingress) { + // Enabling: Close connections previously normalized + return Address::IsCanonicalExternalIp(conn->remote().address()); + } else { + // Disabling: Close connections that should now be normalized + return !Address::IsCanonicalExternalIp(conn->remote().address()) && ShouldNormalizeConnection(conn); + } + }); } } diff --git a/collector/lib/ConnTracker.h b/collector/lib/ConnTracker.h index 092b1b98e1..c86fa89205 100644 --- a/collector/lib/ConnTracker.h +++ b/collector/lib/ConnTracker.h @@ -101,10 +101,10 @@ class ConnectionTracker { template static void UpdateOldState(UnorderedMap* old_state, const UnorderedMap& new_state, int64_t time_micros, int64_t afterglow_period_micros); - void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function predicate); - void CloseNormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); - void CloseExternalUnnormalizedConnections(bool is_server, ConnMap* old_conn_state, ConnMap* delta_conn); - void CloseConnectionsOnRuntimeConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn); + // Mark all matching connections as closed + static void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function predicate); + // Detect a change in the External-IPs config and report connections as closed if their representation is affected + void CloseConnectionsOnExternalIPsConfigChange(ExternalIPsConfig prev_config, ConnMap* old_conn_state, ConnMap* delta_conn) const; // ComputeDelta computes a diff between new_state and old_state template diff --git a/collector/lib/ExternalIPsConfig.h b/collector/lib/ExternalIPsConfig.h index ffe5976db8..a583c045c1 100644 --- a/collector/lib/ExternalIPsConfig.h +++ b/collector/lib/ExternalIPsConfig.h @@ -29,7 +29,7 @@ class ExternalIPsConfig { // 'runtime_config' should be locked prior to calling. ExternalIPsConfig(std::optional runtime_config, bool default_enabled); - ExternalIPsConfig() : direction_enabled_(Direction::NONE) {} + ExternalIPsConfig(Direction direction = Direction::NONE) : direction_enabled_(direction) {} private: Direction direction_enabled_; diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index e10e896a89..abb25e4b5e 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -251,7 +251,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriterCloseConnectionsOnRuntimeConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn); + conn_tracker_->CloseConnectionsOnExternalIPsConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn); prevEnableExternalIPs = externalIPsConfig; } else { diff --git a/collector/test/ConnTrackerTest.cpp b/collector/test/ConnTrackerTest.cpp index 53aedef2e0..575383c230 100644 --- a/collector/test/ConnTrackerTest.cpp +++ b/collector/test/ConnTrackerTest.cpp @@ -1651,80 +1651,143 @@ TEST(ConnTrackerTest, TestConnectionStats) { EXPECT_EQ(stats.outbound.public_, 4); } -TEST(ConnTrackerTest, TestCloseNormalizedConnections) { - ConnectionTracker tracker; - - Endpoint a(Address(192, 168, 0, 1), 80); - Endpoint b(Address(255, 255, 255, 255), 9999); - - Connection conn("xyz", a, b, L4Proto::TCP, true); - int64_t connection_time = 990; - - ConnMap old_state = {{conn, ConnStatus(connection_time, true)}}; - ConnMap delta; - ConnMap expected_delta = {{conn, ConnStatus(connection_time, false)}}; - - tracker.CloseNormalizedConnections(true, &old_state, &delta); - - EXPECT_THAT(old_state, IsEmpty()); - EXPECT_THAT(delta, expected_delta); -} +TEST(ConnTrackerTest, TestExternalIPsConfigChangeEnableEgress) { + Endpoint ingress_local(IPNet(Address()), 80); + Endpoint ingress_remote(Address(223, 42, 0, 1), 0); + Endpoint ingress_remote_normalized(Address(255, 255, 255, 255), 0); -TEST(CloseNormalizedConnectionsTest, UnnormalizedConnectionsAreKept) { - ConnectionTracker tracker; + Endpoint egress_local = Endpoint(); + Endpoint egress_remote(Address(223, 42, 0, 2), 443); + Endpoint egress_remote_normalized(Address(255, 255, 255, 255), 443); - Endpoint a(Address(192, 168, 0, 1), 80); - Endpoint b(Address(192, 168, 1, 10), 9999); + Connection conn_ingress("xyz", ingress_local, ingress_remote, L4Proto::TCP, true); + Connection conn_ingress_normalized("xyz", ingress_local, ingress_remote_normalized, L4Proto::TCP, true); + Connection conn_egress("xyz", egress_local, egress_remote, L4Proto::TCP, false); + Connection conn_egress_normalized("xyz", egress_local, egress_remote_normalized, L4Proto::TCP, false); - Connection conn("xyz", a, b, L4Proto::TCP, true); int64_t connection_time = 990; - ConnMap old_state = {{conn, ConnStatus(connection_time, true)}}; - ConnMap delta; - ConnMap expected_old_state = {{conn, ConnStatus(connection_time, true)}}; - - tracker.CloseNormalizedConnections(true, &old_state, &delta); - - EXPECT_THAT(old_state, expected_old_state); - EXPECT_THAT(delta, IsEmpty()); -} - -TEST(ConnTrackerTest, TestCloseExternalUnnormalizedConnections) { - ConnectionTracker tracker; - - Endpoint a(Address(192, 168, 0, 1), 80); - Endpoint b(Address(11, 168, 1, 10), 9999); - - Connection conn("xyz", a, b, L4Proto::TCP, true); - int64_t connection_time = 990; - - ConnMap old_state = {{conn, ConnStatus(connection_time, true)}}; - ConnMap delta; - ConnMap expected_delta = {{conn, ConnStatus(connection_time, false)}}; - - tracker.CloseExternalUnnormalizedConnections(true, &old_state, &delta); - - EXPECT_THAT(old_state, IsEmpty()); - EXPECT_THAT(delta, expected_delta); -} - -TEST(CloseExternalUnnormalizedConnectionsTest, InternalConnectionsAreKept) { - ConnectionTracker tracker; - - Endpoint a(Address(192, 168, 0, 1), 80); - Endpoint b(Address(192, 168, 1, 10), 9999); - - Connection conn("xyz", a, b, L4Proto::TCP, true); - int64_t connection_time = 990; - - ConnMap old_state = {{conn, ConnStatus(connection_time, true)}}; - ConnMap delta; - ConnMap expected_old_state = {{conn, ConnStatus(connection_time, true)}}; - - tracker.CloseExternalUnnormalizedConnections(true, &old_state, &delta); - - EXPECT_THAT(old_state, expected_old_state); - EXPECT_THAT(delta, IsEmpty()); + struct TestStep { + ExternalIPsConfig::Direction previous_direction; + ExternalIPsConfig::Direction new_direction; + ConnMap old_state; + ConnMap resulting_old_state; + ConnMap expected_delta; + } testSteps[] = { + {// No change (enabled) + .previous_direction = ExternalIPsConfig::Direction::BOTH, + .new_direction = ExternalIPsConfig::Direction::BOTH, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = {}}, + {// No change (disabled) + .previous_direction = ExternalIPsConfig::Direction::NONE, + .new_direction = ExternalIPsConfig::Direction::NONE, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = {}}, + {// Enable EGRESS + .previous_direction = ExternalIPsConfig::Direction::NONE, + .new_direction = ExternalIPsConfig::Direction::EGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)} /* closed */, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_egress_normalized, ConnStatus(connection_time, false)}, + }}, + {// Enable INGRESS + .previous_direction = ExternalIPsConfig::Direction::NONE, + .new_direction = ExternalIPsConfig::Direction::INGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)} /* closed */, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_ingress_normalized, ConnStatus(connection_time, false)}, + }}, + {// Disable EGRESS + .previous_direction = ExternalIPsConfig::Direction::BOTH, + .new_direction = ExternalIPsConfig::Direction::INGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)} /* closed */, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_egress, ConnStatus(connection_time, false)}, + }}, + {// Disable INGRESS + .previous_direction = ExternalIPsConfig::Direction::BOTH, + .new_direction = ExternalIPsConfig::Direction::EGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)} /* closed */, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_ingress, ConnStatus(connection_time, false)}, + }}}; + + for (auto step : testSteps) { + ConnectionTracker tracker; + ConnMap delta; + + tracker.SetExternalIPsConfig(step.new_direction); + tracker.CloseConnectionsOnExternalIPsConfigChange( + step.previous_direction, + &step.old_state, + &delta); + + EXPECT_THAT(step.old_state, step.resulting_old_state); + EXPECT_THAT(delta, step.expected_delta); + } } TEST(ConnTrackerTest, TestShouldNormalizeConnection) { From a42d526d2cf3963447c0afa412f68770b33cbc08 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 27 May 2025 12:24:35 +0200 Subject: [PATCH 10/16] Extract config to variable for more readability --- collector/lib/ConnTracker.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index 6addec99bc..dbf4cff518 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -182,15 +182,17 @@ Connection ConnectionTracker::NormalizeConnectionNoLock(const Connection& conn) } Endpoint local, remote = conn.remote(); + bool extIPs_ingress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS); + bool extIPs_egress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS); if (is_server) { // If this is the server, only the local port is relevant, while the remote port does not matter. local = Endpoint(IPNet(Address()), conn.local().port()); - remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS)), 0); + remote = Endpoint(NormalizeAddressNoLock(conn.remote().address(), extIPs_ingress), 0); } else { // If this is the client, the local port and address are not relevant. local = Endpoint(); - remote = Endpoint(NormalizeAddressNoLock(remote.address(), external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS)), remote.port()); + remote = Endpoint(NormalizeAddressNoLock(remote.address(), extIPs_egress), remote.port()); } return Connection(conn.container(), local, remote, conn.l4proto(), is_server); From 9b8ae8a782602c690eaf573e0196020188f6ec8f Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 27 May 2025 12:34:31 +0200 Subject: [PATCH 11/16] Factorize the lambda content --- collector/lib/ConnTracker.cpp | 38 +++++++++++++---------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/collector/lib/ConnTracker.cpp b/collector/lib/ConnTracker.cpp index dbf4cff518..0d3d469a7c 100644 --- a/collector/lib/ConnTracker.cpp +++ b/collector/lib/ConnTracker.cpp @@ -140,36 +140,26 @@ void ConnectionTracker::CloseConnectionsOnExternalIPsConfigChange(ExternalIPsCon bool ingress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::INGRESS); bool egress = external_IPs_config_.IsEnabled(ExternalIPsConfig::Direction::EGRESS); + auto should_close = [this](const Connection* conn, bool enabling_extIPs) { + if (enabling_extIPs) { + // Enabling: Close connections previously normalized + return Address::IsCanonicalExternalIp(conn->remote().address()); + } else { + // Disabling: Close connections that should now be normalized + return !Address::IsCanonicalExternalIp(conn->remote().address()) && ShouldNormalizeConnection(conn); + } + }; + if (egress != prev_config.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) { - CloseConnections(old_conn_state, delta_conn, [this, egress](const Connection* conn) -> bool { + CloseConnections(old_conn_state, delta_conn, [egress, should_close](const Connection* conn) -> bool { /* egress is when we are not server */ - if (conn->is_server()) { - return false; - } - - if (egress) { - // Enabling: Close connections previously normalized - return Address::IsCanonicalExternalIp(conn->remote().address()); - } else { - // Disabling: Close connections that should now be normalized - return !Address::IsCanonicalExternalIp(conn->remote().address()) && ShouldNormalizeConnection(conn); - } + return !conn->is_server() && should_close(conn, egress); }); } if (ingress != prev_config.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) { - CloseConnections(old_conn_state, delta_conn, [this, ingress](const Connection* conn) -> bool { + CloseConnections(old_conn_state, delta_conn, [ingress, should_close](const Connection* conn) -> bool { /* ingress is when we are server */ - if (!conn->is_server()) { - return false; - } - - if (ingress) { - // Enabling: Close connections previously normalized - return Address::IsCanonicalExternalIp(conn->remote().address()); - } else { - // Disabling: Close connections that should now be normalized - return !Address::IsCanonicalExternalIp(conn->remote().address()) && ShouldNormalizeConnection(conn); - } + return conn->is_server() && should_close(conn, ingress); }); } } From 75384fb4522601cfa714ad6a5e476f7a37c4f64e Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 27 May 2025 17:31:21 +0200 Subject: [PATCH 12/16] Use the finalized version of the proto defs --- collector/proto/third_party/stackrox | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/collector/proto/third_party/stackrox b/collector/proto/third_party/stackrox index 50ff7c402a..1d04bca326 160000 --- a/collector/proto/third_party/stackrox +++ b/collector/proto/third_party/stackrox @@ -1 +1 @@ -Subproject commit 50ff7c402ab239e1f2ba2c11e7a763eba2486925 +Subproject commit 1d04bca3268f5edd3837a0850284be0fbd4100c3 From 5ea1a23e3aee5a6121dedd8e2a1512fe46c3a0f9 Mon Sep 17 00:00:00 2001 From: Olivier Valentin Date: Tue, 27 May 2025 18:13:02 +0200 Subject: [PATCH 13/16] Document runtime-config and the `direction` attribute --- docs/references.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/docs/references.md b/docs/references.md index 110b2757df..19966cda25 100644 --- a/docs/references.md +++ b/docs/references.md @@ -151,6 +151,36 @@ Whenever the configuration file is updated or created, collector will update the configuration. If the configuration file is deleted the configuration will revert to the default. The configuration file can be created after collector start up. +### Runtime-configuration + +When Collector is provided with a runtime configuration file as described in the previous +section, it is able to reconfigure itself dynamically (without a need for restart) if the +content of the file changes. + +The following attributes can be set using the runtime-configuration file : + +* `networking.externalIPs.enabled: ENABLED | DISABLED`: enable/disable the external-IPs feature. +Default value: `DISABLED` + +* `networking.externalIPs.direction: INGRESS | EGRESS | BOTH`: when external-IPs are enabled, +this attribute can restrict the direction in which it is effective. For instance, using `EGRESS` +will aggregate all the incoming connections and give all details for the outgoing ones. This can +be particularly useful to limit the load resulting from enabling external-IPs. +Default value: `BOTH` + +* `networking.maxConnectionsPerMinute: `: set a limit on the number of connections that can +be reported per container and per minute. +Default value: 2048 + +Here is an example of runtime-configuration to enable external-IPs for `EGRESS` only: + +``` +networking: + externalIps: + enabled: ENABLED + direction: EGRESS +``` + ### Other arguments * `--collection-method`: Which technology to use for data gathering. Either From 26e1b997ad96b50fe539dd4a6596b7315fd0802f Mon Sep 17 00:00:00 2001 From: JoukoVirtanen Date: Tue, 27 May 2025 13:39:27 -0700 Subject: [PATCH 14/16] X-Smart-Branch-Parent: ovalenti/ROX-29399-directional-extIPs From a873d4ab08c68ec3eb975f2be9ee15423bb6d2e6 Mon Sep 17 00:00:00 2001 From: JoukoVirtanen Date: Tue, 27 May 2025 14:41:37 -0700 Subject: [PATCH 15/16] Fixed issue with NONE. Added tests that check for the problem --- collector/lib/ExternalIPsConfig.h | 4 +- collector/test/CollectorConfigTest.cpp | 92 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/collector/lib/ExternalIPsConfig.h b/collector/lib/ExternalIPsConfig.h index a583c045c1..36737c63ec 100644 --- a/collector/lib/ExternalIPsConfig.h +++ b/collector/lib/ExternalIPsConfig.h @@ -11,7 +11,7 @@ namespace collector { class ExternalIPsConfig { public: enum Direction { - NONE = 0, + NONE = 1 << 2, INGRESS = 1 << 0, EGRESS = 1 << 1, BOTH = INGRESS | EGRESS, @@ -37,4 +37,4 @@ class ExternalIPsConfig { std::ostream& operator<<(std::ostream& os, const ExternalIPsConfig& config); -} // end namespace collector \ No newline at end of file +} // end namespace collector diff --git a/collector/test/CollectorConfigTest.cpp b/collector/test/CollectorConfigTest.cpp index cdbad03994..d4f8962d5b 100644 --- a/collector/test/CollectorConfigTest.cpp +++ b/collector/test/CollectorConfigTest.cpp @@ -4,6 +4,7 @@ #include "CollectorArgs.h" #include "CollectorConfig.h" +#include "ConfigLoader.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -155,4 +156,95 @@ TEST(CollectorConfigTest, TestEnableExternalIpsRuntimeConfig) { EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); } +TEST(CollectorConfigTest, TestIsEnabledIngress) { + std::string yamlStr = R"( + networking: + externalIps: + enabled: enabled + direction: ingress + )"; + + YAML::Node yamlNode = YAML::Load(yamlStr); + CollectorConfig config; + ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::SUCCESS) << "Input: " << yamlStr; + + auto runtime_config = config.GetRuntimeConfig(); + + EXPECT_TRUE(runtime_config.has_value()); + + EXPECT_EQ(Direction::INGRESS, config.ExternalIPsConf().GetDirection()); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::NONE)); +} + +TEST(CollectorConfigTest, TestIsEnabledEgress) { + std::string yamlStr = R"( + networking: + externalIps: + enabled: enabled + direction: egress + )"; + + YAML::Node yamlNode = YAML::Load(yamlStr); + CollectorConfig config; + ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::SUCCESS) << "Input: " << yamlStr; + + auto runtime_config = config.GetRuntimeConfig(); + + EXPECT_TRUE(runtime_config.has_value()); + + EXPECT_EQ(Direction::EGRESS, config.ExternalIPsConf().GetDirection()); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::NONE)); +} + +TEST(CollectorConfigTest, TestIsEnabledBoth) { + std::string yamlStr = R"( + networking: + externalIps: + enabled: enabled + direction: both + )"; + + YAML::Node yamlNode = YAML::Load(yamlStr); + CollectorConfig config; + ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::SUCCESS) << "Input: " << yamlStr; + + auto runtime_config = config.GetRuntimeConfig(); + + EXPECT_TRUE(runtime_config.has_value()); + + EXPECT_EQ(Direction::BOTH, config.ExternalIPsConf().GetDirection()); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::NONE)); +} + +TEST(CollectorConfigTest, TestIsEnabledNone) { + std::string yamlStr = R"( + networking: + externalIps: + enabled: disabled + )"; + + YAML::Node yamlNode = YAML::Load(yamlStr); + CollectorConfig config; + ASSERT_EQ(ConfigLoader(config).LoadConfiguration(yamlNode), ConfigLoader::SUCCESS) << "Input: " << yamlStr; + + auto runtime_config = config.GetRuntimeConfig(); + + EXPECT_TRUE(runtime_config.has_value()); + + EXPECT_EQ(Direction::NONE, config.ExternalIPsConf().GetDirection()); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::INGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::EGRESS)); + EXPECT_FALSE(config.ExternalIPsConf().IsEnabled(Direction::BOTH)); + EXPECT_TRUE(config.ExternalIPsConf().IsEnabled(Direction::NONE)); +} + } // namespace collector From bf62f7259b53cffb496795a6bcba531b0ff2a742 Mon Sep 17 00:00:00 2001 From: JoukoVirtanen Date: Tue, 27 May 2025 21:05:10 -0700 Subject: [PATCH 16/16] Added a couple of unit tests --- collector/test/ConnTrackerTest.cpp | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/collector/test/ConnTrackerTest.cpp b/collector/test/ConnTrackerTest.cpp index 575383c230..a8457247cc 100644 --- a/collector/test/ConnTrackerTest.cpp +++ b/collector/test/ConnTrackerTest.cpp @@ -1740,6 +1740,40 @@ TEST(ConnTrackerTest, TestExternalIPsConfigChangeEnableEgress) { .expected_delta = { {conn_ingress_normalized, ConnStatus(connection_time, false)}, }}, + {// Disable EGRESS enable INGRESS + .previous_direction = ExternalIPsConfig::Direction::EGRESS, + .new_direction = ExternalIPsConfig::Direction::INGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, /* closed */ + {conn_egress, ConnStatus(connection_time, true)} /* closed */, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .resulting_old_state = { + {conn_ingress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_ingress_normalized, ConnStatus(connection_time, false)}, + {conn_egress, ConnStatus(connection_time, false)}, + }}, + {// Disable INGRESS enable EGRESS + .previous_direction = ExternalIPsConfig::Direction::INGRESS, + .new_direction = ExternalIPsConfig::Direction::EGRESS, + .old_state = { + {conn_ingress, ConnStatus(connection_time, true)} /* closed */, + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + {conn_egress_normalized, ConnStatus(connection_time, true)} /* closed */, + }, + .resulting_old_state = { + {conn_ingress_normalized, ConnStatus(connection_time, true)}, + {conn_egress, ConnStatus(connection_time, true)}, + }, + .expected_delta = { + {conn_ingress, ConnStatus(connection_time, false)}, + {conn_egress_normalized, ConnStatus(connection_time, false)}, + }}, {// Disable EGRESS .previous_direction = ExternalIPsConfig::Direction::BOTH, .new_direction = ExternalIPsConfig::Direction::INGRESS,