From 9e2a75d360a1f87bacb72c584d062af6dba766ad Mon Sep 17 00:00:00 2001 From: Marco Hofstetter Date: Tue, 21 Jan 2025 13:35:16 +0100 Subject: [PATCH 1/2] bpf_metadata: extract restoreLocalAddress Currently, `extractSocketMetadata` also restores the local address for the use by the `OriginalDestCluster`. To keep the function free of side-effects, the logic that restores the local address gets extracted into a function within the `SocketMetadata` that gets called from the BPF metadata listener after extracting the BPF metadata from the socket. Signed-off-by: Marco Hofstetter --- cilium/bpf_metadata.cc | 10 ++++------ cilium/bpf_metadata.h | 18 +++++++++++++++++- tests/bpf_metadata.cc | 7 +------ tests/metadata_config_test.cc | 18 ++++++++++++++++++ 4 files changed, 40 insertions(+), 13 deletions(-) diff --git a/cilium/bpf_metadata.cc b/cilium/bpf_metadata.cc index 72a1b3d93..81f043e1d 100644 --- a/cilium/bpf_metadata.cc +++ b/cilium/bpf_metadata.cc @@ -370,10 +370,6 @@ Config::extractSocketMetadata(Network::ConnectionSocket& socket) { return absl::nullopt; } - // We do this first as this likely restores the destination address and - // lets the OriginalDstCluster know the destination address can be used. - socket.connectionInfoProvider().restoreLocalAddress(dst_address); // mark as `restored` - std::string pod_ip, other_ip; if (is_ingress_) { pod_ip = dip->addressAsString(); @@ -516,8 +512,8 @@ Config::extractSocketMetadata(Network::ConnectionSocket& socket) { return absl::optional(Cilium::BpfMetadata::SocketMetadata( mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(), std::move(pod_ip), std::move(src_address), std::move(source_addresses.ipv4_), - std::move(source_addresses.ipv6_), weak_from_this(), proxy_id_, std::move(proxylib_l7proto), - sni)); + std::move(source_addresses.ipv6_), std::move(dst_address), weak_from_this(), proxy_id_, + std::move(proxylib_l7proto), sni)); } Network::FilterStatus Instance::onAccept(Network::ListenerFilterCallbacks& cb) { @@ -540,6 +536,8 @@ Network::FilterStatus Instance::onAccept(Network::ListenerFilterCallbacks& cb) { socket_metadata->configureProxyLibApplicationProtocol(socket); + socket_metadata->configureOriginalDstAddress(socket); + // Make Cilium Policy data available to filters and upstream connection (Cilium TLS Wrapper) as // filter state. cb.filterState().setData( diff --git a/cilium/bpf_metadata.h b/cilium/bpf_metadata.h index e635f51fd..a43946603 100644 --- a/cilium/bpf_metadata.h +++ b/cilium/bpf_metadata.h @@ -36,6 +36,7 @@ struct SocketMetadata : public Logger::Loggable { 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 std::weak_ptr& 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), @@ -44,7 +45,8 @@ struct SocketMetadata : public Logger::Loggable { policy_resolver_(policy_resolver), mark_(mark), original_source_address_(std::move(original_source_address)), source_address_ipv4_(std::move(source_address_ipv4)), - source_address_ipv6_(std::move(source_address_ipv6)) {} + source_address_ipv6_(std::move(source_address_ipv6)), + original_dest_address_(std::move(original_dest_address)) {} std::shared_ptr buildCiliumPolicyFilterState() { return std::make_shared( @@ -76,6 +78,19 @@ struct SocketMetadata : public Logger::Loggable { } } + void configureOriginalDstAddress(Network::ConnectionSocket& socket) { + if (!original_dest_address_) { + return; + } + + // Restoration of the original destination address lets the OriginalDstCluster know the + // destination address that can be used. + ENVOY_LOG(trace, "cilium.bpf_metadata: restoreLocalAddress ({} -> {})", + socket.connectionInfoProvider().localAddress()->asString(), + original_dest_address_->asString()); + socket.connectionInfoProvider().restoreLocalAddress(original_dest_address_); + } + uint32_t ingress_source_identity_; uint32_t source_identity_; bool ingress_; @@ -92,6 +107,7 @@ struct SocketMetadata : public Logger::Loggable { Network::Address::InstanceConstSharedPtr original_source_address_; Network::Address::InstanceConstSharedPtr source_address_ipv4_; Network::Address::InstanceConstSharedPtr source_address_ipv6_; + Network::Address::InstanceConstSharedPtr original_dest_address_; }; /** diff --git a/tests/bpf_metadata.cc b/tests/bpf_metadata.cc index 775fdf15e..f0143569b 100644 --- a/tests/bpf_metadata.cc +++ b/tests/bpf_metadata.cc @@ -159,11 +159,6 @@ TestConfig::~TestConfig() { absl::optional TestConfig::extractSocketMetadata(Network::ConnectionSocket& socket) { - // fake setting the local address. It remains the same as required by the test - // infra, but it will be marked as restored as required by the original_dst - // cluster. - socket.connectionInfoProvider().restoreLocalAddress(original_dst_address); - // TLS filter chain matches this, make namespace part of this (e.g., // "default")? socket.setDetectedTransportProtocol("cilium:default"); @@ -197,7 +192,7 @@ TestConfig::extractSocketMetadata(Network::ConnectionSocket& socket) { return absl::optional(Cilium::BpfMetadata::SocketMetadata( 0, 0, source_identity, is_ingress_, is_l7lb_, port, std::move(pod_ip), nullptr, nullptr, - nullptr, shared_from_this(), 0, std::move(l7proto), "")); + nullptr, original_dst_address, shared_from_this(), 0, std::move(l7proto), "")); } } // namespace BpfMetadata diff --git a/tests/metadata_config_test.cc b/tests/metadata_config_test.cc index 1afac28dd..0e13825d7 100644 --- a/tests/metadata_config_test.cc +++ b/tests/metadata_config_test.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -516,6 +517,23 @@ TEST_F(MetadataConfigTest, ProxyLibConfigured) { socket_metadata->configureProxyLibApplicationProtocol(socket_); } +TEST_F(MetadataConfigTest, RestoreLocalAddress) { + ::cilium::BpfMetadata config{}; + + EXPECT_NO_THROW(initialize(config)); + + auto socket_metadata = config_->extractSocketMetadata(socket_); + EXPECT_TRUE(socket_metadata); + + EXPECT_NE(socket_.connectionInfoProvider().localAddress(), original_dst_address); + EXPECT_FALSE(socket_.connectionInfoProvider().localAddressRestored()); + + socket_metadata->configureOriginalDstAddress(socket_); + + EXPECT_EQ(socket_.connectionInfoProvider().localAddress(), original_dst_address); + EXPECT_TRUE(socket_.connectionInfoProvider().localAddressRestored()); +} + } // namespace } // namespace Cilium } // namespace Envoy From 0e8a1563c8625928f5ad40b3db34911aaaf02db2 Mon Sep 17 00:00:00 2001 From: Marco Hofstetter Date: Tue, 21 Jan 2025 13:39:09 +0100 Subject: [PATCH 2/2] bpf_metadata: only restore local address if it differs According to the Envoy sourcecode, restoration of the local address should only be performed if it changes from the current local address. ``` ... This should only be called when restoring the original destination address of a connection redirected by iptables REDIRECT. The caller is responsible for making sure the new address is actually different. ``` Therefore, this commit introduces a check before restoring the local address. Signed-off-by: Marco Hofstetter --- cilium/bpf_metadata.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cilium/bpf_metadata.h b/cilium/bpf_metadata.h index a43946603..ae0c7d154 100644 --- a/cilium/bpf_metadata.h +++ b/cilium/bpf_metadata.h @@ -83,6 +83,11 @@ struct SocketMetadata : public Logger::Loggable { return; } + if (*original_dest_address_ == *socket.connectionInfoProvider().localAddress()) { + // Only set the local address if it really changed, and mark it as address being restored. + return; + } + // Restoration of the original destination address lets the OriginalDstCluster know the // destination address that can be used. ENVOY_LOG(trace, "cilium.bpf_metadata: restoreLocalAddress ({} -> {})",