From e4aa2965609e4a637a1a9e6aa259a229755d7a44 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Thu, 19 Jun 2025 10:55:59 +0200 Subject: [PATCH 1/3] tidy fix Signed-off-by: Jarno Rajahalme --- cilium/network_policy.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/cilium/network_policy.cc b/cilium/network_policy.cc index 5321296f0..75500b64c 100644 --- a/cilium/network_policy.cc +++ b/cilium/network_policy.cc @@ -4,7 +4,6 @@ #include #include -#include #include #include #include From 1f74b636ff4d7f7950e173b5a10c6842d9cb2ac4 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 18 Jun 2025 13:33:41 +0200 Subject: [PATCH 2/3] accesslog: Add trace Envoy logging Signed-off-by: Jarno Rajahalme --- cilium/accesslog.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cilium/accesslog.cc b/cilium/accesslog.cc index 541bb417e..5f20b7b70 100644 --- a/cilium/accesslog.cc +++ b/cilium/accesslog.cc @@ -63,6 +63,8 @@ void AccessLog::log(AccessLog::Entry& log_entry, ::cilium::EntryType entry_type) log_entry.request_logged_ = true; } + ENVOY_LOG_MISC(trace, "Cilium Access log entry: {}", entry.DebugString()); + // encode protobuf std::string msg; entry.SerializeToString(&msg); From 9cea4685b4caab204d54174fdd1120d5224544dc Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 18 Jun 2025 15:29:42 +0200 Subject: [PATCH 3/3] 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: Jun 18 15:39:29.763: default/ubuntu-deployment-6f7cc4b9fb-9gmnp:57354 (ID:43964) -> default/nginx-deployment-worker-7d99874b8b-dw4bt:80 (ID:53552) http-request FORWARDED (HTTP/1.1 GET http://10.96.154.80/) where '43964' is the source security identity of 'default/ubuntu-deployment-6f7cc4b9fb-9gmnp' Similarly for north/south the original source identity is recorded in the hubble flow: Jun 18 15:41:15.186: 172.18.0.1:41684 (ID:16777217) -> default/nginx-deployment-worker-7d99874b8b-dw4bt:80 (ID:53552) http-request FORWARDED (HTTP/1.1 GET http://172.18.255.193/) where 16777217 is the node-local source identity if the CIDR 172.18.0.1. --- cilium/bpf_metadata.cc | 27 ++++++++------- cilium/bpf_metadata.h | 17 ++++----- cilium/filter_state_cilium_policy.cc | 52 +++++++++++++--------------- cilium/filter_state_cilium_policy.h | 13 +++---- cilium/l7policy.cc | 2 +- tests/bpf_metadata.cc | 2 +- tests/metadata_config_test.cc | 15 +++----- 7 files changed, 57 insertions(+), 71 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index 6c7746e9a..2f184f21c 100644 --- a/cilium/bpf_metadata.cc +++ b/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) { diff --git a/cilium/bpf_metadata.h b/cilium/bpf_metadata.h index 74d68c067..c5d055d6b 100644 --- a/cilium/bpf_metadata.h +++ b/cilium/bpf_metadata.h @@ -35,19 +35,17 @@ namespace BpfMetadata { #define DEFAULT_CACHE_ENTRY_TTL_MS 3 struct SocketMetadata : public Logger::Loggable { - 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 { std::shared_ptr buildCiliumPolicyFilterState() { return std::make_shared( - 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 { 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 npmap_{}; Cilium::CtMapSharedPtr ct_maps_{}; Cilium::IpCacheSharedPtr ipcache_{}; diff --git a/cilium/filter_state_cilium_policy.cc b/cilium/filter_state_cilium_policy.cc index ca292597c..43cc8f6e8 100644 --- a/cilium/filter_state_cilium_policy.cc +++ b/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,28 +120,26 @@ 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: {}", @@ -151,7 +147,7 @@ bool CiliumPolicyFilterState::enforceIngressHTTPPolicy( 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: {}", diff --git a/cilium/filter_state_cilium_policy.h b/cilium/filter_state_cilium_policy.h index f90632400..960a84db7 100644 --- a/cilium/filter_state_cilium_policy.h +++ b/cilium/filter_state_cilium_policy.h @@ -34,15 +34,13 @@ using PolicyResolverSharedPtr = std::shared_ptr; class CiliumPolicyFilterState : public StreamInfo::FilterState::Object, public Logger::Loggable { 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_; diff --git a/cilium/l7policy.cc b/cilium/l7policy.cc index d1161ea00..f61aab27b 100644 --- a/cilium/l7policy.cc +++ b/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_); diff --git a/tests/bpf_metadata.cc b/tests/bpf_metadata.cc index b10829bf5..d1f9a5f08 100644 --- a/tests/bpf_metadata.cc +++ b/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), "")}; } diff --git a/tests/metadata_config_test.cc b/tests/metadata_config_test.cc index e3a237198..f1969e702 100644 --- a/tests/metadata_config_test.cc +++ b/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);