Ovalenti/rox 29399 directional ext i ps jv 2

collector/lib/CollectorConfig.cpp

@@ -91,7 +91,6 @@ constexpr const char* CollectorConfig::kSyscalls[];
 constexpr bool CollectorConfig::kEnableProcessesListeningOnPorts;
 
 const UnorderedSet<L4ProtoPortPair> 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();
 }
 

collector/lib/CollectorConfig.h

@@ -14,6 +14,7 @@
 #include <internalapi/sensor/collector.pb.h>
 
 #include "CollectionMethod.h"
+#include "ExternalIPsConfig.h"
 #include "HostConfig.h"
 #include "Logging.h"
 #include "NetworkConnection.h"
@@ -97,15 +98,10 @@ class CollectorConfig {
   // 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() {

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());
@@ -136,30 +136,31 @@ void ConnectionTracker::CloseConnections(ConnMap* old_conn_state, ConnMap* delta
   }
 }
 
-/**
- * Closes connections that have the 255.255.255.255 external IP address
- */
-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::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);
 
-/**
- * Closes unnormalized connections that would be normalized to the canonical external
- * IP address if external IPs was enabled
- */
-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());
-  });
-}
+  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);
+    }
+  };
 
-void ConnectionTracker::CloseConnectionsOnRuntimeConfigChange(ConnMap* old_conn_state, ConnMap* delta_conn, bool enableExternalIPs) {
-  if (enableExternalIPs) {
-    CloseNormalizedConnections(old_conn_state, delta_conn);
-  } else {
-    CloseExternalUnnormalizedConnections(old_conn_state, delta_conn);
+  if (egress != prev_config.IsEnabled(ExternalIPsConfig::Direction::EGRESS)) {
+    CloseConnections(old_conn_state, delta_conn, [egress, should_close](const Connection* conn) -> bool {
+      /* egress is when we are not server */
+      return !conn->is_server() && should_close(conn, egress);
+    });
+  }
+  if (ingress != prev_config.IsEnabled(ExternalIPsConfig::Direction::INGRESS)) {
+    CloseConnections(old_conn_state, delta_conn, [ingress, should_close](const Connection* conn) -> bool {
+      /* ingress is when we are server */
+      return conn->is_server() && should_close(conn, ingress);
+    });
   }
 }
 
@@ -171,15 +172,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(), enable_external_ips_), 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(), enable_external_ips_), remote.port());
+    remote = Endpoint(NormalizeAddressNoLock(remote.address(), extIPs_egress), remote.port());
   }
 
   return Connection(conn.container(), local, remote, conn.l4proto(), is_server);

collector/lib/ConnTracker.h

@@ -4,6 +4,7 @@
 #include <vector>
 
 #include "Containers.h"
+#include "ExternalIPsConfig.h"
 #include "Hash.h"
 #include "NRadix.h"
 #include "NetworkConnection.h"
@@ -100,10 +101,10 @@ class ConnectionTracker {
   template <typename T>
   static void UpdateOldState(UnorderedMap<T, ConnStatus>* old_state, const UnorderedMap<T, ConnStatus>& new_state, int64_t time_micros, int64_t afterglow_period_micros);
 
-  void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function<bool(const Connection*)> 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);
+  // Mark all matching connections as closed
+  static void CloseConnections(ConnMap* old_conn_state, ConnMap* delta_conn, std::function<bool(const Connection*)> 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 <typename T>
@@ -131,7 +132,7 @@ class ConnectionTracker {
 
   void UpdateKnownPublicIPs(UnorderedSet<Address>&& known_public_ips);
   void UpdateKnownIPNetworks(UnorderedMap<Address::Family, std::vector<IPNet>>&& known_ip_networks);
-  void EnableExternalIPs(bool enable) { enable_external_ips_ = enable; }
+  void SetExternalIPsConfig(ExternalIPsConfig config) { external_IPs_config_ = config; }
   void UpdateIgnoredL4ProtoPortPairs(UnorderedSet<L4ProtoPortPair>&& ignored_l4proto_port_pairs);
   void UpdateIgnoredNetworks(const std::vector<IPNet>& network_list);
   void UpdateNonAggregatedNetworks(const std::vector<IPNet>& network_list);
@@ -202,7 +203,7 @@ class ConnectionTracker {
 
   UnorderedSet<Address> known_public_ips_;
   NRadixTree known_ip_networks_;
-  bool enable_external_ips_ = false;
+  ExternalIPsConfig external_IPs_config_;
   UnorderedMap<Address::Family, bool> known_private_networks_exists_;
   UnorderedSet<L4ProtoPortPair> ignored_l4proto_port_pairs_;
   NRadixTree ignored_networks_;

collector/lib/ExternalIPsConfig.cpp

@@ -0,0 +1,55 @@
+#include "ExternalIPsConfig.h"
+
+namespace collector {
+
+ExternalIPsConfig::ExternalIPsConfig(std::optional<sensor::CollectorConfig> 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

collector/lib/ExternalIPsConfig.h

@@ -0,0 +1,40 @@
+#pragma once
+
+#include <optional>
+#include <ostream>
+
+#include <internalapi/sensor/collector.pb.h>
+
+namespace collector {
+
+// Encapsulates the configuration of the External-IPs feature
+class ExternalIPsConfig {
+ public:
+  enum Direction {
+    NONE = 1 << 2,
+    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<sensor::CollectorConfig> runtime_config, bool default_enabled);
+
+  ExternalIPsConfig(Direction direction = Direction::NONE) : direction_enabled_(direction) {}
+
+ private:
+  Direction direction_enabled_;
+};
+
+std::ostream& operator<<(std::ostream& os, const ExternalIPsConfig& config);
+
+}  // end namespace collector

collector/lib/NetworkStatusNotifier.cpp

@@ -225,7 +225,7 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriter<sensor::NetworkConnect
   auto next_scrape = std::chrono::system_clock::now();
   int64_t time_at_last_scrape = NowMicros();
 
-  bool prevEnableExternalIPs = config_.EnableExternalIPs();
+  ExternalIPsConfig prevEnableExternalIPs = config_.ExternalIPsConf();
 
   while (writer->Sleep(next_scrape)) {
     CLOG(DEBUG) << "Starting network status notification";
@@ -242,18 +242,18 @@ void NetworkStatusNotifier::RunSingle(IDuplexClientWriter<sensor::NetworkConnect
     const sensor::NetworkConnectionInfoMessage* msg;
     ConnMap new_conn_state, delta_conn;
     AdvertisedEndpointMap new_cep_state;
-    bool enableExternalIPs = config_.EnableExternalIPs();
+    ExternalIPsConfig externalIPsConfig = config_.ExternalIPsConf();
 
     WITH_TIMER(CollectorStats::net_fetch_state) {
-      conn_tracker_->EnableExternalIPs(enableExternalIPs);
+      conn_tracker_->SetExternalIPsConfig(externalIPsConfig);
 
       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;
-        }
+
+        conn_tracker_->CloseConnectionsOnExternalIPsConfigChange(prevEnableExternalIPs, &old_conn_state, &delta_conn);
+        prevEnableExternalIPs = externalIPsConfig;
+
       } else {
         ConnectionTracker::ComputeDelta(new_conn_state, &old_conn_state);
       }