metadata: Eliminate separate 'ingress_source_identity'

Both debug logging and access logging are more intelligible when the
original source identity is used, also in the case of the north/south L7
LB, where an "Ingress IP" is used as the source address in the upstream
connections. In that case SO_MARK encodes the identity of the Ingress IP
so that the source identity seen in the destination is the same when the
destination is in the same node (source identity derived from SO_MARK)
and when the destination is in a different node (source identity mapped
from the source (Ingress) IP).

Note that the (original) source identity is used for policy determination
only for ingress policy, for which the original source identity was
already used. Given this, the only visible change is the source identity
as seen on debug/trace logs and (hubble) access logs. Access logs already
show the original source address, so this change aligns the recorded
source identity with it, so that instead of:

Jun 18 12:37:20.940: default/ubuntu-deployment-6f7cc4b9fb-9gmnp:39430 (ingress) -> default/nginx-deployment-worker-7d99874b8b-dw4bt:80 (ID:53552) http-request FORWARDED (HTTP/1.1 GET http://10.96.154.80/)

Hubble will show this:

Signed-off-by: Jarno Rajahalme <[email protected]>

Author: jrajahalme

cilium/bpf_metadata.cc

@@ -403,16 +403,18 @@ Config::extractSocketMetadata(Network::ConnectionSocket& socket) {
   // Use a pointer as we may need to change the policy in the case of "North/South L7 LB" below.
   const auto* policy = &getPolicy(pod_ip);
 
-  // Resolve the source security ID from conntrack map, or from ip cache
-  uint32_t source_identity = resolveSourceIdentity(*policy, sip, dip, is_ingress_, is_l7lb_);
+  // original_source_identity is set to the original source identity and is used in policy logs and
+  // access logging. This is typically the same as 'source_identity', but for north/south L7 LB,
+  // where the upstream source address is set to an Ingress IP, the 'source_identity' will be that
+  // if the Ingress IP. This original source identity is still used in policy logs and access
+  // logging even in that case.
+  uint32_t original_source_identity =
+      resolveSourceIdentity(*policy, sip, dip, is_ingress_, is_l7lb_);
+  uint32_t source_identity = original_source_identity;
 
   // Resolve the destination security ID for egress traffic
   uint32_t destination_identity = is_ingress_ ? 0 : resolvePolicyId(dip);
 
-  // ingress_source_identity is non-zero when the egress path l7 LB should also enforce
-  // the ingress path policy using the original source identity.
-  uint32_t ingress_source_identity = 0;
-
   // Use the configured IPv4/IPv6 Ingress IPs as starting point for the sources addresses
   IpAddressPair source_addresses(ipv4_source_address_, ipv6_source_address_);
 
@@ -463,7 +465,6 @@ Config::extractSocketMetadata(Network::ConnectionSocket& socket) {
 
     // Enforce Ingress policy?
     if (enforce_policy_on_l7lb_) {
-      ingress_source_identity = source_identity;
       ingress_policy_name =
           l7lb_policy_name_.empty() ? ingress_ip->addressAsString() : l7lb_policy_name_;
     }
@@ -519,15 +520,15 @@ Config::extractSocketMetadata(Network::ConnectionSocket& socket) {
   }
 
   ENVOY_LOG(trace,
-            "cilium.bpf_metadata: mark {}, ingress_source_identity {}, source_identity {}, "
+            "cilium.bpf_metadata: mark {}, original_source_identity {}, source_identity {}, "
             "is_ingress {}, is_l7lb_ {}, ingress_policy_name {}, port {}, pod_ip {}",
-            mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_,
+            mark, original_source_identity, source_identity, is_ingress_, is_l7lb_,
             ingress_policy_name, dip->port(), pod_ip);
   return {Cilium::BpfMetadata::SocketMetadata(
-      mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
-      std::move(pod_ip), std::move(ingress_policy_name), std::move(src_address),
-      std::move(source_addresses.ipv4_), std::move(source_addresses.ipv6_), std::move(dst_address),
-      shared_from_this(), proxy_id_, std::move(proxylib_l7proto), sni)};
+      mark, original_source_identity, is_ingress_, is_l7lb_, dip->port(), std::move(pod_ip),
+      std::move(ingress_policy_name), std::move(src_address), std::move(source_addresses.ipv4_),
+      std::move(source_addresses.ipv6_), std::move(dst_address), shared_from_this(), proxy_id_,
+      std::move(proxylib_l7proto), sni)};
 }
 
 Network::FilterStatus Instance::onAccept(Network::ListenerFilterCallbacks& cb) {

cilium/bpf_metadata.h

@@ -35,19 +35,17 @@ namespace BpfMetadata {
 #define DEFAULT_CACHE_ENTRY_TTL_MS 3
 
 struct SocketMetadata : public Logger::Loggable<Logger::Id::filter> {
-  SocketMetadata(uint32_t mark, uint32_t ingress_source_identity, uint32_t source_identity,
-                 bool ingress, bool l7lb, uint16_t port, std::string&& pod_ip,
-                 std::string&& ingress_policy_name,
+  SocketMetadata(uint32_t mark, uint32_t source_identity, bool ingress, bool l7lb, uint16_t port,
+                 std::string&& pod_ip, std::string&& ingress_policy_name,
                  Network::Address::InstanceConstSharedPtr original_source_address,
                  Network::Address::InstanceConstSharedPtr source_address_ipv4,
                  Network::Address::InstanceConstSharedPtr source_address_ipv6,
                  Network::Address::InstanceConstSharedPtr original_dest_address,
                  const PolicyResolverSharedPtr& policy_resolver, uint32_t proxy_id,
                  std::string&& proxylib_l7_proto, absl::string_view sni)
-      : ingress_source_identity_(ingress_source_identity), source_identity_(source_identity),
-        ingress_(ingress), is_l7lb_(l7lb), port_(port), pod_ip_(std::move(pod_ip)),
-        ingress_policy_name_(std::move(ingress_policy_name)), proxy_id_(proxy_id),
-        proxylib_l7_proto_(std::move(proxylib_l7_proto)), sni_(sni),
+      : source_identity_(source_identity), ingress_(ingress), is_l7lb_(l7lb), port_(port),
+        pod_ip_(std::move(pod_ip)), ingress_policy_name_(std::move(ingress_policy_name)),
+        proxy_id_(proxy_id), proxylib_l7_proto_(std::move(proxylib_l7_proto)), sni_(sni),
         policy_resolver_(policy_resolver), mark_(mark),
         original_source_address_(std::move(original_source_address)),
         source_address_ipv4_(std::move(source_address_ipv4)),
@@ -56,7 +54,7 @@ struct SocketMetadata : public Logger::Loggable<Logger::Id::filter> {
 
   std::shared_ptr<Envoy::Cilium::CiliumPolicyFilterState> buildCiliumPolicyFilterState() {
     return std::make_shared<Envoy::Cilium::CiliumPolicyFilterState>(
-        ingress_source_identity_, source_identity_, ingress_, is_l7lb_, port_, std::move(pod_ip_),
+        source_identity_, ingress_, is_l7lb_, port_, std::move(pod_ip_),
         std::move(ingress_policy_name_), policy_resolver_, proxy_id_, sni_);
   };
 
@@ -111,7 +109,6 @@ struct SocketMetadata : public Logger::Loggable<Logger::Id::filter> {
     socket.connectionInfoProvider().restoreLocalAddress(original_dest_address_);
   }
 
-  uint32_t ingress_source_identity_;
   uint32_t source_identity_;
   bool ingress_;
   bool is_l7lb_;
@@ -165,8 +162,8 @@ class Config : public Cilium::PolicyResolver,
   bool enforce_policy_on_l7lb_;
   std::string l7lb_policy_name_;
   std::chrono::milliseconds ipcache_entry_ttl_;
-  Random::RandomGenerator& random_;
 
+  Random::RandomGenerator& random_;
   std::shared_ptr<const Cilium::NetworkPolicyMap> npmap_{};
   Cilium::CtMapSharedPtr ct_maps_{};
   Cilium::IpCacheSharedPtr ipcache_{};

cilium/filter_state_cilium_policy.cc

@@ -53,15 +53,13 @@ bool CiliumPolicyFilterState::enforceNetworkPolicy(const Network::Connection& co
     const auto& policy = policy_resolver_->getPolicy(ingress_policy_name_);
 
     // Enforce ingress policy for Ingress, on the original destination port
-    if (ingress_source_identity_ != 0) {
-      auto ingress_port_policy = policy.findPortPolicy(true, port_);
-      if (!ingress_port_policy.allowed(proxy_id_, ingress_source_identity_, sni)) {
-        ENVOY_CONN_LOG(debug,
-                       "Ingress network policy {} DROP for source identity and destination "
-                       "reserved ingress identity: {} proxy_id: {} port: {} sni: \"{}\"",
-                       conn, ingress_policy_name_, ingress_source_identity_, proxy_id_, port_, sni);
-        return false;
-      }
+    auto ingress_port_policy = policy.findPortPolicy(true, port_);
+    if (!ingress_port_policy.allowed(proxy_id_, source_identity_, sni)) {
+      ENVOY_CONN_LOG(debug,
+                     "Ingress network policy {} DROP for source identity and destination "
+                     "reserved ingress identity: {} proxy_id: {} port: {} sni: \"{}\"",
+                     conn, ingress_policy_name_, source_identity_, proxy_id_, port_, sni);
+      return false;
     }
 
     // Enforce egress policy for Ingress
@@ -122,36 +120,34 @@ bool CiliumPolicyFilterState::enforceIngressHTTPPolicy(
   const auto& policy = policy_resolver_->getPolicy(ingress_policy_name_);
 
   // Enforce ingress policy for Ingress, on the original destination port
-  if (ingress_source_identity_ != 0) {
-    const auto port_policy = policy.findPortPolicy(true, port_);
-    if (!port_policy.hasHttpRules()) {
-      ENVOY_CONN_LOG(debug,
-                     "cilium.l7policy: Ingress {} HTTP ingress policy enforcement skipped (no HTTP "
-                     "rules) on proxy_id: {} id: {} port: {}",
-                     conn, ingress_policy_name_, proxy_id_, ingress_source_identity_, port_);
-      return true;
-    }
+  const auto ingress_policy = policy.findPortPolicy(true, port_);
+  if (!ingress_policy.hasHttpRules()) {
+    ENVOY_CONN_LOG(debug,
+                   "cilium.l7policy: Ingress {} HTTP ingress policy enforcement skipped (no HTTP "
+                   "rules) on proxy_id: {} id: {} port: {}",
+                   conn, ingress_policy_name_, proxy_id_, source_identity_, port_);
+    return true;
+  }
 
-    if (!port_policy.allowed(proxy_id_, ingress_source_identity_, headers, log_entry)) {
-      ENVOY_CONN_LOG(
-          debug,
-          "cilium.l7policy: Ingress {} HTTP ingress policy DROP on proxy_id: {} id: {} port: {}",
-          conn, ingress_policy_name_, proxy_id_, ingress_source_identity_, port_);
-      return false;
-    }
+  if (!ingress_policy.allowed(proxy_id_, source_identity_, headers, log_entry)) {
+    ENVOY_CONN_LOG(
+        debug,
+        "cilium.l7policy: Ingress {} HTTP ingress policy DROP on proxy_id: {} id: {} port: {}",
+        conn, ingress_policy_name_, proxy_id_, source_identity_, port_);
+    return false;
   }
 
   // Enforce egress policy for Ingress on the upstream destination identity and port
-  const auto port_policy = policy.findPortPolicy(false, destination_port);
-  if (!port_policy.hasHttpRules()) {
+  const auto egress_policy = policy.findPortPolicy(false, destination_port);
+  if (!egress_policy.hasHttpRules()) {
     ENVOY_CONN_LOG(debug,
                    "cilium.l7policy: Ingress {} HTTP egress policy enforcement skipped (no HTTP "
                    "rules) on proxy_id: {} id: {} port: {}",
                    conn, ingress_policy_name_, proxy_id_, destination_identity, destination_port);
     return true;
   }
 
-  if (!port_policy.allowed(proxy_id_, destination_identity, headers, log_entry)) {
+  if (!egress_policy.allowed(proxy_id_, destination_identity, headers, log_entry)) {
     ENVOY_CONN_LOG(
         debug,
         "cilium.l7policy: Ingress {} HTTP egress policy DROP on proxy_id: {} id: {}  port: {}",

cilium/filter_state_cilium_policy.h

@@ -34,15 +34,13 @@ using PolicyResolverSharedPtr = std::shared_ptr<PolicyResolver>;
 class CiliumPolicyFilterState : public StreamInfo::FilterState::Object,
                                 public Logger::Loggable<Logger::Id::filter> {
 public:
-  CiliumPolicyFilterState(uint32_t ingress_source_identity, uint32_t source_identity, bool ingress,
-                          bool l7lb, uint16_t port, std::string&& pod_ip,
-                          std::string&& ingress_policy_name,
+  CiliumPolicyFilterState(uint32_t source_identity, bool ingress, bool l7lb, uint16_t port,
+                          std::string&& pod_ip, std::string&& ingress_policy_name,
                           const PolicyResolverSharedPtr& policy_resolver, uint32_t proxy_id,
                           absl::string_view sni)
-      : ingress_source_identity_(ingress_source_identity), source_identity_(source_identity),
-        ingress_(ingress), is_l7lb_(l7lb), port_(port), pod_ip_(std::move(pod_ip)),
-        ingress_policy_name_(std::move(ingress_policy_name)), proxy_id_(proxy_id), sni_(sni),
-        policy_resolver_(policy_resolver) {
+      : source_identity_(source_identity), ingress_(ingress), is_l7lb_(l7lb), port_(port),
+        pod_ip_(std::move(pod_ip)), ingress_policy_name_(std::move(ingress_policy_name)),
+        proxy_id_(proxy_id), sni_(sni), policy_resolver_(policy_resolver) {
     ENVOY_LOG(
         debug,
         "Cilium CiliumPolicyFilterState(): source_identity: {}, "
@@ -79,7 +77,6 @@ class CiliumPolicyFilterState : public StreamInfo::FilterState::Object,
   static const std::string& key();
 
   // Additional ingress policy enforcement is performed if ingress_source_identity is non-zero
-  uint32_t ingress_source_identity_;
   uint32_t source_identity_;
   bool ingress_;
   bool is_l7lb_;

cilium/l7policy.cc

@@ -269,7 +269,7 @@ Http::FilterHeadersStatus AccessFilter::decodeHeaders(Http::RequestHeaderMap& he
     }
 
     // Is there an Ingress policy?
-    if (policy_fs->ingress_policy_name_.length() > 0) {
+    if (!policy_fs->ingress_policy_name_.empty()) {
       allowed = policy_fs->enforceIngressHTTPPolicy(conn.ref(), destination_identity,
                                                     destination_port, headers, *log_entry_);
 

tests/bpf_metadata.cc

@@ -191,7 +191,7 @@ TestConfig::extractSocketMetadata(Network::ConnectionSocket& socket) {
                      port, l7proto);
 
   return {Cilium::BpfMetadata::SocketMetadata(
-      0, 0, source_identity, is_ingress_, is_l7lb_, port, std::move(pod_ip), "", nullptr, nullptr,
+      0, source_identity, is_ingress_, is_l7lb_, port, std::move(pod_ip), "", nullptr, nullptr,
       nullptr, original_dst_address, shared_from_this(), 0, std::move(l7proto), "")};
 }
 

tests/metadata_config_test.cc

@@ -300,13 +300,12 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbMetadata) {
   auto policy_fs = socket_metadata->buildCiliumPolicyFilterState();
   EXPECT_NE(nullptr, policy_fs);
 
-  EXPECT_EQ(8, policy_fs->source_identity_);
+  EXPECT_EQ(12345678, policy_fs->source_identity_);
   EXPECT_EQ(false, policy_fs->ingress_);
   EXPECT_EQ(true, policy_fs->is_l7lb_);
   EXPECT_EQ(80, policy_fs->port_);
   EXPECT_EQ("", policy_fs->pod_ip_);
   EXPECT_EQ("", policy_fs->ingress_policy_name_);
-  EXPECT_EQ(0, policy_fs->ingress_source_identity_);
 
   auto source_addresses_socket_option = socket_metadata->buildSourceAddressSocketOption(-1);
   EXPECT_NE(nullptr, source_addresses_socket_option);
@@ -341,13 +340,12 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedMetadata) {
   auto policy_fs = socket_metadata->buildCiliumPolicyFilterState();
   EXPECT_NE(nullptr, policy_fs);
 
-  EXPECT_EQ(8, policy_fs->source_identity_);
+  EXPECT_EQ(12345678, policy_fs->source_identity_);
   EXPECT_EQ(false, policy_fs->ingress_);
   EXPECT_EQ(true, policy_fs->is_l7lb_);
   EXPECT_EQ(80, policy_fs->port_);
   EXPECT_EQ("", policy_fs->pod_ip_);
   EXPECT_EQ("10.1.1.42", policy_fs->ingress_policy_name_);
-  EXPECT_EQ(12345678, policy_fs->ingress_source_identity_);
 
   AccessLog::Entry log_entry;
   log_entry.entry_.set_policy_name("pod");
@@ -411,13 +409,12 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbPodAndIngressEnforcedMetadata) {
   auto policy_fs = socket_metadata->buildCiliumPolicyFilterState();
   EXPECT_NE(nullptr, policy_fs);
 
-  EXPECT_EQ(8, policy_fs->source_identity_);
+  EXPECT_EQ(9999, policy_fs->source_identity_);
   EXPECT_EQ(false, policy_fs->ingress_);
   EXPECT_EQ(true, policy_fs->is_l7lb_);
   EXPECT_EQ(80, policy_fs->port_);
   EXPECT_EQ("192.168.1.1", policy_fs->pod_ip_);
   EXPECT_EQ("10.1.1.42", policy_fs->ingress_policy_name_);
-  EXPECT_EQ(9999, policy_fs->ingress_source_identity_);
 
   AccessLog::Entry log_entry;
   log_entry.entry_.set_policy_name("pod");
@@ -482,13 +479,12 @@ TEST_F(MetadataConfigTest, NorthSouthL7LbIngressEnforcedCIDRMetadata) {
   auto policy_fs = socket_metadata->buildCiliumPolicyFilterState();
   EXPECT_NE(nullptr, policy_fs);
 
-  EXPECT_EQ(8, policy_fs->source_identity_);
+  EXPECT_EQ(2, policy_fs->source_identity_);
   EXPECT_EQ(false, policy_fs->ingress_);
   EXPECT_EQ(true, policy_fs->is_l7lb_);
   EXPECT_EQ(80, policy_fs->port_);
   EXPECT_EQ("", policy_fs->pod_ip_);
   EXPECT_EQ("10.1.1.42", policy_fs->ingress_policy_name_);
-  EXPECT_EQ(2, policy_fs->ingress_source_identity_);
 
   AccessLog::Entry log_entry;
   log_entry.entry_.set_policy_name("pod");
@@ -590,13 +586,12 @@ TEST_F(MetadataConfigTest, EastWestL7LbMetadataNoOriginalSource) {
   auto policy_fs = socket_metadata->buildCiliumPolicyFilterState();
   EXPECT_NE(nullptr, policy_fs);
 
-  EXPECT_EQ(8, policy_fs->source_identity_);
+  EXPECT_EQ(111, policy_fs->source_identity_);
   EXPECT_EQ(false, policy_fs->ingress_);
   EXPECT_EQ(true, policy_fs->is_l7lb_);
   EXPECT_EQ(80, policy_fs->port_);
   EXPECT_EQ("10.1.1.1", policy_fs->pod_ip_);
   EXPECT_EQ("", policy_fs->ingress_policy_name_);
-  EXPECT_EQ(0, policy_fs->ingress_source_identity_);
 
   auto source_addresses_socket_option = socket_metadata->buildSourceAddressSocketOption(-1);
   EXPECT_NE(nullptr, source_addresses_socket_option);