v1.30 Backports 2024-12-03

.bazelrc

@@ -25,7 +25,7 @@ build:release --fission=dbg
 
 # Manual link stamping, forces link to include current git SHA even if binary is otherwise
 # upto-date
-build --define manual_stamp=manual_stamp
+build:release --define manual_stamp=manual_stamp
 
 # Always have LD_LIBRARY_PATH=/usr/cross-compat/lib defined in the test environment.
 # The path does not need to exist, but can be created when needed for running tests.

.github/workflows/cilium-integration-tests.yaml

@@ -147,6 +147,7 @@ jobs:
           --helm-set operator.image.useDigest=false \
           --helm-set operator.image.pullPolicy=Never \
           --helm-set debug.enabled=true \
+          --helm-set envoy.enabled=false \
           --helm-set debug.verbose=envoy
 
         cilium hubble enable
@@ -155,7 +156,7 @@ jobs:
 
     - name: Execute Cilium L7 Connectivity Tests
       shell: bash
-      run: cilium connectivity test --test=l7
+      run: cilium connectivity test --test="l7|sni|check-log-errors"
 
     - name: Gather Cilium system dump
       if: failure()

Makefile

@@ -164,14 +164,14 @@ proxylib/libcilium.so:
 .PHONY: envoy-test-deps
 envoy-test-deps: $(COMPILER_DEP) SOURCE_VERSION proxylib/libcilium.so
 	@$(ECHO_BAZEL)
-	$(BAZEL) $(BAZEL_OPTS) build --build_tests_only -c fastbuild $(BAZEL_BUILD_OPTS) $(BAZEL_TEST_OPTS) //tests/... $(BAZEL_FILTER)
+	$(BAZEL) $(BAZEL_OPTS) build $(BAZEL_BUILD_OPTS) $(BAZEL_TEST_OPTS) //tests/... @envoy//test/integration:tcp_proxy_integration_test $(BAZEL_FILTER)
 
 .PHONY: envoy-tests
 envoy-tests: $(COMPILER_DEP) SOURCE_VERSION proxylib/libcilium.so
 	@$(ECHO_BAZEL)
-	$(BAZEL) $(BAZEL_OPTS) test -c fastbuild $(BAZEL_BUILD_OPTS) $(BAZEL_TEST_OPTS) //tests/... $(BAZEL_FILTER)
-	# To validate the upstream integration tests to make sure that our custom patches didn't break anything
-	$(BAZEL) $(BAZEL_OPTS) test -c fastbuild $(BAZEL_BUILD_OPTS) $(BAZEL_TEST_OPTS) @envoy//test/integration:tcp_proxy_integration_test
+	# Upstream tcp_proxy_integration_test included to validate that our custom patches
+	# didn't break anything
+	$(BAZEL) $(BAZEL_OPTS) test $(BAZEL_BUILD_OPTS) $(BAZEL_TEST_OPTS) //tests/... @envoy//test/integration:tcp_proxy_integration_test $(BAZEL_FILTER)
 
 .PHONY: \
 	install \

WORKSPACE

@@ -39,6 +39,8 @@ git_repository(
         # https://github.com/envoyproxy/envoy/pull/31894
         "@//patches:0004-Patch-cel-cpp-to-not-break-build.patch",
         "@//patches:0005-original_dst_cluster-Avoid-multiple-hosts-for-the-sa.patch",
+        "@//patches:0007-tcp_proxy-Check-for-nullptr-in-watermark-ASSERTs.patch",
+        "@//patches:0008-thread_local-reset-slot-in-worker-threads-first.patch",
     ],
     # // clang-format off: Envoy's format check: Only repository_locations.bzl may contains URL references
     remote = "https://github.com/envoyproxy/envoy.git",

cilium/bpf_metadata.cc

@@ -443,7 +443,7 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc
     mark = ((is_ingress_) ? 0x0A00 : 0x0B00) | cluster_id | identity_id;
   }
   return std::make_shared<Cilium::SocketOption>(
-      policy, mark, ingress_source_identity, source_identity, is_ingress_, is_l7lb_, dip->port(),
+      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_), shared_from_this(), proxy_id_, sni);
 }

cilium/network_filter.cc

@@ -135,6 +135,11 @@ Network::FilterStatus Instance::onNewConnection() {
       ENVOY_LOG(warn, "cilium.network (egress): No destination address ");
       return false;
     }
+    const auto& policy = option->getPolicy();
+    if (!policy) {
+      ENVOY_LOG_MISC(warn, "cilium.network: No policy found for pod {}", option->pod_ip_);
+      return false;
+    }
     if (!option->ingress_) {
       const auto dip = dst_address->ip();
       if (!dip) {
@@ -146,7 +151,7 @@ Network::FilterStatus Instance::onNewConnection() {
       destination_identity = option->resolvePolicyId(dip);
 
       if (option->ingress_source_identity_ != 0) {
-        auto ingress_port_policy = option->initial_policy_->findPortPolicy(true, destination_port_);
+        auto ingress_port_policy = policy->findPortPolicy(true, destination_port_);
         if (!ingress_port_policy.allowed(option->ingress_source_identity_, sni)) {
           ENVOY_CONN_LOG(
               debug,
@@ -157,7 +162,7 @@ Network::FilterStatus Instance::onNewConnection() {
       }
     }
 
-    auto port_policy = option->initial_policy_->findPortPolicy(option->ingress_, destination_port_);
+    auto port_policy = policy->findPortPolicy(option->ingress_, destination_port_);
 
     remote_id_ = option->ingress_ ? option->identity_ : destination_identity;
     if (!port_policy.allowed(remote_id_, sni)) {

cilium/network_policy.cc

@@ -606,6 +606,13 @@ class PortNetworkPolicyRules : public Logger::Loggable<Logger::Id::config> {
     }
   }
 
+  ~PortNetworkPolicyRules() {
+    if (!Thread::MainThread::isMainOrTestThread()) {
+      IS_ENVOY_BUG("PortNetworkPolicyRules: Destructor executing in a worker thread, while "
+                   "only main thread should destruct xDS resources");
+    }
+  }
+
   bool allowed(uint32_t remote_id, Envoy::Http::RequestHeaderMap& headers,
                Cilium::AccessLog::Entry& log_entry, bool& denied) const {
     // Empty set matches any payload from anyone

cilium/network_policy.h

@@ -109,7 +109,12 @@ class IPAddressPair {
 
 class PolicyInstance {
 public:
-  virtual ~PolicyInstance() = default;
+  virtual ~PolicyInstance() {
+    if (!Thread::MainThread::isMainOrTestThread()) {
+      IS_ENVOY_BUG("PolicyInstance: Destructor executing in a worker thread, while "
+                   "only main thread should destruct xDS resources");
+    }
+  };
 
   virtual bool allowed(bool ingress, uint32_t remote_id, uint16_t port,
                        Envoy::Http::RequestHeaderMap& headers,

cilium/secret_watcher.cc

@@ -34,7 +34,13 @@ SecretWatcher::SecretWatcher(const NetworkPolicyMap& parent, const std::string&
       secret_provider_(secretProvider(parent.transportFactoryContext(), sds_name)),
       update_secret_(readAndWatchSecret()) {}
 
-SecretWatcher::~SecretWatcher() { delete load(); }
+SecretWatcher::~SecretWatcher() {
+  if (!Thread::MainThread::isMainOrTestThread()) {
+    ENVOY_LOG(error, "SecretWatcher: Destructor executing in a worker thread, while "
+                     "only main thread should destruct xDS resources");
+  }
+  delete load();
+}
 
 Envoy::Common::CallbackHandlePtr SecretWatcher::readAndWatchSecret() {
   store();

cilium/socket_option.h

@@ -222,19 +222,18 @@ class SocketMarkOption : public Network::Socket::Option,
 
 class SocketOption : public SocketMarkOption {
 public:
-  SocketOption(PolicyInstanceConstSharedPtr policy, uint32_t mark, uint32_t ingress_source_identity,
-               uint32_t source_identity, bool ingress, bool l7lb, uint16_t port,
-               std::string&& pod_ip,
+  SocketOption(uint32_t mark, uint32_t ingress_source_identity, uint32_t source_identity,
+               bool ingress, bool l7lb, uint16_t port, std::string&& pod_ip,
                Network::Address::InstanceConstSharedPtr original_source_address,
                Network::Address::InstanceConstSharedPtr ipv4_source_address,
                Network::Address::InstanceConstSharedPtr ipv6_source_address,
-               const std::shared_ptr<PolicyResolver>& policy_id_resolver, uint32_t proxy_id,
+               const std::shared_ptr<PolicyResolver>& policy_resolver, uint32_t proxy_id,
                absl::string_view sni)
       : SocketMarkOption(mark, source_identity, original_source_address, ipv4_source_address,
                          ipv6_source_address),
-        ingress_source_identity_(ingress_source_identity), initial_policy_(policy),
-        ingress_(ingress), is_l7lb_(l7lb), port_(port), pod_ip_(std::move(pod_ip)),
-        proxy_id_(proxy_id), sni_(sni), policy_id_resolver_(policy_id_resolver) {
+        ingress_source_identity_(ingress_source_identity), ingress_(ingress), is_l7lb_(l7lb),
+        port_(port), pod_ip_(std::move(pod_ip)), proxy_id_(proxy_id), sni_(sni),
+        policy_resolver_(policy_resolver) {
     ENVOY_LOG(debug,
               "Cilium SocketOption(): source_identity: {}, "
               "ingress: {}, port: {}, pod_ip: {}, source_addresses: {}/{}/{}, mark: {:x} (magic "
@@ -244,15 +243,14 @@ class SocketOption : public SocketMarkOption {
               ipv4_source_address_ ? ipv4_source_address_->asString() : "",
               ipv6_source_address_ ? ipv6_source_address_->asString() : "", mark_, mark & 0xff00,
               mark & 0xff, mark >> 16, proxy_id_, sni_);
-    ASSERT(initial_policy_ != nullptr);
   }
 
   uint32_t resolvePolicyId(const Network::Address::Ip* ip) const {
-    return policy_id_resolver_->resolvePolicyId(ip);
+    return policy_resolver_->resolvePolicyId(ip);
   }
 
   const PolicyInstanceConstSharedPtr getPolicy() const {
-    return policy_id_resolver_->getPolicy(pod_ip_);
+    return policy_resolver_->getPolicy(pod_ip_);
   }
 
   // policyUseUpstreamDestinationAddress returns 'true' if policy enforcement should be done on the
@@ -261,7 +259,6 @@ class SocketOption : public SocketMarkOption {
 
   // Additional ingress policy enforcement is performed if ingress_source_identity is non-zero
   uint32_t ingress_source_identity_;
-  const PolicyInstanceConstSharedPtr initial_policy_; // Never NULL
   bool ingress_;
   bool is_l7lb_;
   uint16_t port_;
@@ -270,7 +267,7 @@ class SocketOption : public SocketMarkOption {
   std::string sni_;
 
 private:
-  const std::shared_ptr<PolicyResolver> policy_id_resolver_;
+  const std::shared_ptr<PolicyResolver> policy_resolver_;
 };
 
 using SocketOptionSharedPtr = std::shared_ptr<SocketOption>;

cilium/tls_wrapper.cc

@@ -37,6 +37,11 @@ class SslSocketWrapper : public Network::TransportSocket {
     // TLS a raw socket is used instead,
     const auto option = Cilium::GetSocketOption(callbacks.connection().socketOptions());
     if (option) {
+      const auto& policy = option->getPolicy();
+      if (!policy) {
+        ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: No policy found for pod {}", option->pod_ip_);
+        return;
+      }
       // Resolve the destination security ID and port
       uint32_t destination_identity = 0;
       uint32_t destination_port = option->port_;
@@ -65,8 +70,7 @@ class SslSocketWrapper : public Network::TransportSocket {
       const auto& sni = option->sni_;
 
       auto remote_id = option->ingress_ ? option->identity_ : destination_identity;
-      auto port_policy =
-          option->initial_policy_->findPortPolicy(option->ingress_, destination_port);
+      auto port_policy = policy->findPortPolicy(option->ingress_, destination_port);
       const Envoy::Ssl::ContextConfig* config = nullptr;
       bool raw_socket_allowed = false;
       Envoy::Ssl::ContextSharedPtr ctx =
@@ -91,7 +95,7 @@ class SslSocketWrapper : public Network::TransportSocket {
         // Set the callbacks
         socket_->setTransportSocketCallbacks(callbacks);
       } else {
-        option->initial_policy_->tlsWrapperMissingPolicyInc();
+        policy->tlsWrapperMissingPolicyInc();
 
         std::string ipStr("<none>");
         if (option->ingress_) {
@@ -109,11 +113,12 @@ class SslSocketWrapper : public Network::TransportSocket {
             ipStr = dip->addressAsString();
           }
         }
-        ENVOY_LOG_MISC(warn,
-                       "cilium.tls_wrapper: Could not get {} TLS context for {} IP {} (id {}) port "
-                       "{} sni \"{}\" and raw socket is not allowed",
-                       is_client ? "client" : "server", option->ingress_ ? "source" : "destination",
-                       ipStr, remote_id, destination_port, sni);
+        ENVOY_LOG_MISC(
+            warn,
+            "cilium.tls_wrapper: Could not get {} TLS context for pod {} on {} IP {} (id {}) port "
+            "{} sni \"{}\" and raw socket is not allowed",
+            is_client ? "client" : "server", option->pod_ip_,
+            option->ingress_ ? "source" : "destination", ipStr, remote_id, destination_port, sni);
       }
     } else {
       ENVOY_LOG_MISC(warn, "cilium.tls_wrapper: Can not correlate connection with Cilium Network "

patches/0007-tcp_proxy-Check-for-nullptr-in-watermark-ASSERTs.patch

@@ -0,0 +1,32 @@
+From 1e054ee1e266386fc53026c327ff915232f76ece Mon Sep 17 00:00:00 2001
+From: Jarno Rajahalme <[email protected]>
+Date: Mon, 2 Dec 2024 08:58:54 +0100
+Subject: [PATCH 7/8] tcp_proxy: Check for nullptr in watermark ASSERTs
+
+Signed-off-by: Jarno Rajahalme <[email protected]>
+
+diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
+index f28bf3f2ec..e8a37bdbda 100644
+--- a/source/common/tcp_proxy/tcp_proxy.cc
++++ b/source/common/tcp_proxy/tcp_proxy.cc
+@@ -389,7 +389,7 @@ void Filter::UpstreamCallbacks::onEvent(Network::ConnectionEvent event) {
+
+ void Filter::UpstreamCallbacks::onAboveWriteBufferHighWatermark() {
+   // TCP Tunneling may call on high/low watermark multiple times.
+-  ASSERT(parent_->config_->tunnelingConfigHelper() || !on_high_watermark_called_);
++  ASSERT(parent_ == nullptr || parent_->config_->tunnelingConfigHelper() || !on_high_watermark_called_);
+   on_high_watermark_called_ = true;
+
+   if (parent_ != nullptr) {
+@@ -400,7 +400,7 @@ void Filter::UpstreamCallbacks::onAboveWriteBufferHighWatermark() {
+
+ void Filter::UpstreamCallbacks::onBelowWriteBufferLowWatermark() {
+   // TCP Tunneling may call on high/low watermark multiple times.
+-  ASSERT(parent_->config_->tunnelingConfigHelper() || on_high_watermark_called_);
++  ASSERT(parent_ == nullptr || parent_->config_->tunnelingConfigHelper() || on_high_watermark_called_);
+   on_high_watermark_called_ = false;
+
+   if (parent_ != nullptr) {
+-- 
+2.34.1
+

patches/0008-thread_local-reset-slot-in-worker-threads-first.patch

@@ -0,0 +1,79 @@
+From 1a0f7e26bc8b2905d87c64f7fea9df42e874c28a Mon Sep 17 00:00:00 2001
+From: Jarno Rajahalme <[email protected]>
+Date: Mon, 23 Dec 2024 22:43:15 +0100
+Subject: [PATCH 8/8] thread_local: reset slot in worker threads first
+
+Thread local slots refer to their data via shared pointers. Reset the
+shared pointer first in the worker threads, and last in the main thread
+so that the referred object is destructed in the main thread instead of
+some random worker thread. This prevents xDS stream synchronization bugs
+if the slot happens to refer to an SDS secret.
+
+Signed-off-by: Jarno Rajahalme <[email protected]>
+
+diff --git a/source/common/thread_local/thread_local_impl.cc b/source/common/thread_local/thread_local_impl.cc
+index bd417164b1..34d1c0f1b2 100644
+--- a/source/common/thread_local/thread_local_impl.cc
++++ b/source/common/thread_local/thread_local_impl.cc
+@@ -177,7 +177,8 @@ void InstanceImpl::removeSlot(uint32_t slot) {
+              free_slot_indexes_.end(),
+          fmt::format("slot index {} already in free slot set!", slot));
+   free_slot_indexes_.push_back(slot);
+-  runOnAllThreads([slot]() -> void {
++
++  auto cb = [slot]() -> void {
+     // This runs on each thread and clears the slot, making it available for a new allocations.
+     // This is safe even if a new allocation comes in, because everything happens with post() and
+     // will be sequenced after this removal. It is also safe if there are callbacks pending on
+@@ -185,7 +186,12 @@ void InstanceImpl::removeSlot(uint32_t slot) {
+     if (slot < thread_local_data_.data_.size()) {
+       thread_local_data_.data_[slot] = nullptr;
+     }
+-  });
++  };
++  // 'cb' is called in the main thread after it has been called on all worker threads.
++  // This makes sure the last shared pointer reference is released in the main thread,
++  // so that the thread local data is destructed in the main thread instead of some random
++  // worker thread.
++  runOnAllWorkerThreads(cb, cb);
+ }
+
+ void InstanceImpl::runOnAllThreads(std::function<void()> cb) {
+@@ -220,6 +226,22 @@ void InstanceImpl::runOnAllThreads(std::function<void()> cb,
+   }
+ }
+
++void InstanceImpl::runOnAllWorkerThreads(std::function<void()> cb,
++                                         std::function<void()> worker_threads_complete_cb) {
++  ASSERT_IS_MAIN_OR_TEST_THREAD();
++  ASSERT(!shutdown_);
++
++  std::shared_ptr<std::function<void()>> cb_guard(
++      new std::function<void()>(cb), [this, worker_threads_complete_cb](std::function<void()>* cb) {
++        main_thread_dispatcher_->post(worker_threads_complete_cb);
++        delete cb;
++      });
++
++  for (Event::Dispatcher& dispatcher : registered_threads_) {
++    dispatcher.post([cb_guard]() -> void { (*cb_guard)(); });
++  }
++}
++
+ void InstanceImpl::setThreadLocal(uint32_t index, ThreadLocalObjectSharedPtr object) {
+   if (thread_local_data_.data_.size() <= index) {
+     thread_local_data_.data_.resize(index + 1);
+diff --git a/source/common/thread_local/thread_local_impl.h b/source/common/thread_local/thread_local_impl.h
+index 90753101b6..108cf85152 100644
+--- a/source/common/thread_local/thread_local_impl.h
++++ b/source/common/thread_local/thread_local_impl.h
+@@ -75,6 +75,7 @@ private:
+   void removeSlot(uint32_t slot);
+   void runOnAllThreads(std::function<void()> cb);
+   void runOnAllThreads(std::function<void()> cb, std::function<void()> main_callback);
++  void runOnAllWorkerThreads(std::function<void()> cb, std::function<void()> main_callback);
+   static void setThreadLocal(uint32_t index, ThreadLocalObjectSharedPtr object);
+
+   static thread_local ThreadLocalData thread_local_data_;
+-- 
+2.34.1
+

tests/bpf_metadata.cc

@@ -183,9 +183,9 @@ Cilium::SocketOptionSharedPtr TestConfig::getMetadata(Network::ConnectionSocket&
     ENVOY_LOG_MISC(info, "setRequestedApplicationProtocols({})", l7proto);
   }
 
-  return std::make_shared<Cilium::SocketOption>(policy, 0, 0, source_identity, is_ingress_,
-                                                is_l7lb_, port, std::move(pod_ip), nullptr, nullptr,
-                                                nullptr, shared_from_this(), 0, "");
+  return std::make_shared<Cilium::SocketOption>(0, 0, source_identity, is_ingress_, is_l7lb_, port,
+                                                std::move(pod_ip), nullptr, nullptr, nullptr,
+                                                shared_from_this(), 0, "");
 }
 
 } // namespace BpfMetadata