Skip to content

Commit

Permalink
socket_option: Change policy_resolver_ to a weak pointer
Browse files Browse the repository at this point in the history
A shared_ptr policy_resolver_ holds on to the passed BpfMetadata::Config,
which owns the policy map reference. This could keep the the policy map
alive when the listeners are destructed, leading to the destruction of
the policy map from a worker thread. Change policy_resolver_ to a weak
pointer to resolve this.

Signed-off-by: Jarno Rajahalme <[email protected]>
  • Loading branch information
jrajahalme committed Nov 29, 2024
1 parent f09ed99 commit c205cb9
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 13 deletions.
2 changes: 2 additions & 0 deletions cilium/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ envoy_cc_library(
name = "socket_option_lib",
hdrs = [
"socket_option.h",
"policy_id.h",
],
repository = "@envoy",
deps = [
Expand Down Expand Up @@ -300,6 +301,7 @@ envoy_cc_library(
hdrs = [
"bpf_metadata.h",
"host_map.h",
"policy_id.h",
],
repository = "@envoy",
deps = [
Expand Down
2 changes: 1 addition & 1 deletion cilium/bpf_metadata.cc
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ Cilium::SocketOptionSharedPtr Config::getMetadata(Network::ConnectionSocket& soc
return std::make_shared<Cilium::SocketOption>(
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);
std::move(source_addresses.ipv6_), weak_from_this(), proxy_id_, sni);
}

Network::FilterStatus Instance::onAccept(Network::ListenerFilterCallbacks& cb) {
Expand Down
9 changes: 1 addition & 8 deletions cilium/host_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "absl/numeric/int128.h"
#include "cilium/api/nphds.pb.h"
#include "cilium/api/nphds.pb.validate.h"
#include "cilium/policy_id.h"

// std::hash specialization for Abseil uint128, needed for unordered_map key.
namespace std {
Expand Down Expand Up @@ -47,14 +48,6 @@ template <typename I> I masked(I addr, unsigned int plen) {
return plen == 0 ? I(0) : addr & ~hton((I(1) << (PLEN_MAX - plen)) - 1);
};

enum ID : uint64_t {
UNKNOWN = 0,
WORLD = 2,
// LocalIdentityFlag is the bit in the numeric identity that identifies
// a numeric identity to have local scope
LocalIdentityFlag = 1 << 24,
};

class PolicyHostDecoder : public Envoy::Config::OpaqueResourceDecoder {
public:
PolicyHostDecoder() : validation_visitor_(ProtobufMessage::getNullValidationVisitor()) {}
Expand Down
15 changes: 11 additions & 4 deletions cilium/socket_option.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "source/common/common/utility.h"

#include "cilium/conntrack.h"
#include "cilium/policy_id.h"
#include "cilium/privileged_service_client.h"

namespace Envoy {
Expand Down Expand Up @@ -227,7 +228,7 @@ class SocketOption : public SocketMarkOption {
Network::Address::InstanceConstSharedPtr original_source_address,
Network::Address::InstanceConstSharedPtr ipv4_source_address,
Network::Address::InstanceConstSharedPtr ipv6_source_address,
const std::shared_ptr<PolicyResolver>& policy_resolver, uint32_t proxy_id,
const std::weak_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),
Expand All @@ -246,11 +247,17 @@ class SocketOption : public SocketMarkOption {
}

uint32_t resolvePolicyId(const Network::Address::Ip* ip) const {
return policy_resolver_->resolvePolicyId(ip);
const auto resolver = policy_resolver_.lock();
if (resolver)
return resolver->resolvePolicyId(ip);
return Cilium::ID::WORLD; // default to WORLD policy ID if resolver is no longer available
}

const PolicyInstanceConstSharedPtr getPolicy() const {
return policy_resolver_->getPolicy(pod_ip_);
const auto resolver = policy_resolver_.lock();
if (resolver)
return resolver->getPolicy(pod_ip_);
return nullptr;
}

// policyUseUpstreamDestinationAddress returns 'true' if policy enforcement should be done on the
Expand All @@ -267,7 +274,7 @@ class SocketOption : public SocketMarkOption {
std::string sni_;

private:
const std::shared_ptr<PolicyResolver> policy_resolver_;
const std::weak_ptr<PolicyResolver> policy_resolver_;
};

using SocketOptionSharedPtr = std::shared_ptr<SocketOption>;
Expand Down

0 comments on commit c205cb9

Please sign in to comment.