From fa045b13161c9722420290af7921d502c0b2f0e5 Mon Sep 17 00:00:00 2001 From: Marco Hofstetter Date: Fri, 20 Dec 2024 12:45:44 +0100 Subject: [PATCH 1/3] bpf_metadata: combine nested conditions in getPolicy for N/S L7LB case This commit combines the nested conditions in the method `getPolicy` that are handling the special case for N/S L7 Loadbalancing. (Allow all traffic if no explicit Ingress policy is set) Signed-off-by: Marco Hofstetter --- cilium/bpf_metadata.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index 16efbd402..76d9c77d0 100644 --- a/cilium/bpf_metadata.cc +++ b/cilium/bpf_metadata.cc @@ -314,15 +314,15 @@ const PolicyInstanceConstSharedPtr Config::getPolicy(const std::string& pod_ip) if (npmap_ != nullptr) policy = npmap_->GetPolicyInstance(pod_ip); - if (policy == nullptr) { - // Allow all traffic for egress without a policy when 'is_l7lb_' is true, - // or if configured without bpf. - // This is the case for L7 LB listeners only. This is needed to allow traffic forwarded by k8s - // Ingress (which is implemented as an egress listener!). - if (npmap_ == nullptr || (!enforce_policy_on_l7lb_ && !is_ingress_ && is_l7lb_)) { - return NetworkPolicyMap::AllowAllEgressPolicy; - } + // Allow all traffic for egress without a policy when 'is_l7lb_' is true, + // or if configured without bpf (npmap_ == nullptr). + // This is the case for L7 LB listeners only. This is needed to allow traffic forwarded by k8s + // Ingress (which is implemented as an egress listener!). + if (policy == nullptr && + (npmap_ == nullptr || (!enforce_policy_on_l7lb_ && !is_ingress_ && is_l7lb_))) { + return NetworkPolicyMap::AllowAllEgressPolicy; } + return policy; } From 7abc2b098abc924f905f375974387e4b8d26cffe Mon Sep 17 00:00:00 2001 From: Marco Hofstetter Date: Fri, 20 Dec 2024 12:59:26 +0100 Subject: [PATCH 2/3] bpf_metadata: simplify variable naming for N/S L7 LB case This commit simplifies the variable naming for the N/S L7 LB case in getMetadata. * Rename `ip` to `ingress_ip` * use `ingress_ip` instead of `pod_ip` to avoid confusion Signed-off-by: Marco Hofstetter --- cilium/bpf_metadata.cc | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index 76d9c77d0..d073d5f2c 100644 --- a/cilium/bpf_metadata.cc +++ b/cilium/bpf_metadata.cc @@ -406,27 +406,28 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc // North/south L7 LB, assume the source security identity of the configured source addresses, // if any and policy for this identity exists. - // Pick the local source address of the same family as the incoming connection - const Network::Address::Ip* ip = selectIPVersion(sip->version(), source_addresses); - - if (!ip) { - // IP family of the connection has no configured local source address - ENVOY_LOG(warn, - "cilium.bpf_metadata (north/south L7 LB): No local IP source address configured " - "for the family of {}", - pod_ip); + // Pick the local ingress source address of the same family as the incoming connection + const Network::Address::Ip* ingress_ip = selectIPVersion(sip->version(), source_addresses); + + if (!ingress_ip) { + // IP family of the connection has no configured local ingress source address + ENVOY_LOG( + warn, + "cilium.bpf_metadata (north/south L7 LB): No local Ingress IP source address configured " + "for the family of {}", + sip->addressAsString()); return nullptr; } - pod_ip = ip->addressAsString(); + auto& ingress_ip_str = ingress_ip->addressAsString(); - auto new_source_id = resolvePolicyId(ip); - if (new_source_id == Cilium::ID::WORLD) { + auto new_source_identity = resolvePolicyId(ingress_ip); + if (new_source_identity == Cilium::ID::WORLD) { // No security ID available for the configured source IP ENVOY_LOG(warn, - "cilium.bpf_metadata (north/south L7 LB): Unknown local IP source address " + "cilium.bpf_metadata (north/south L7 LB): Unknown local Ingress IP source address " "configured: {}", - pod_ip); + ingress_ip_str); return nullptr; } @@ -434,10 +435,14 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc if (enforce_policy_on_l7lb_) ingress_source_identity = source_identity; - source_identity = new_source_id; + source_identity = new_source_identity; // AllowAllEgressPolicy will be returned if no explicit Ingress policy exists - policy = getPolicy(pod_ip); + policy = getPolicy(ingress_ip_str); + + // Set Ingress source IP as pod_ip (In case of egress (how N/S L7 LB is implemented), the pod_ip + // is the source IP) + pod_ip = ingress_ip_str; // Original source address is never used for north/south LB // This means that a local host IP is used if no IP is configured to be used instead of it From 2c4e3c7113e241ccc98fe2757c85c90d9954502d Mon Sep 17 00:00:00 2001 From: Marco Hofstetter Date: Fri, 20 Dec 2024 13:01:31 +0100 Subject: [PATCH 3/3] bpf_metadata: remove outdated comment This commit removes an outdated comment about Ingress IP handling. Currently, the existence of an Ingress source IP with the right IP family is validated in the config. See ``` const Network::Address::Ip* ingress_ip = selectIPVersion(sip->version(), source_addresses); if (!ingress_ip) { // IP family of the connection has no configured local ingress source address ENVOY_LOG( warn, "cilium.bpf_metadata (north/south L7 LB): No local Ingress IP source address configured " "for the family of {}", sip->addressAsString()); return nullptr; } ``` Signed-off-by: Marco Hofstetter --- cilium/bpf_metadata.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index d073d5f2c..3120c1884 100644 --- a/cilium/bpf_metadata.cc +++ b/cilium/bpf_metadata.cc @@ -445,8 +445,6 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc pod_ip = ingress_ip_str; // Original source address is never used for north/south LB - // This means that a local host IP is used if no IP is configured to be used instead of it - // ('ip' above is null). src_address = nullptr; } else if (!use_original_source_address_ || (npmap_ != nullptr && npmap_->exists(other_ip))) { // Otherwise only use the original source address if permitted and the destination is not