Skip to content

Support per-remap geo DB handles in header_rewrite - #13042

Merged
cmcfarlen merged 1 commit into
apache:masterfrom
cmcfarlen:hrw-mmdb-loading
Apr 27, 2026
Merged

Support per-remap geo DB handles in header_rewrite#13042
cmcfarlen merged 1 commit into
apache:masterfrom
cmcfarlen:hrw-mmdb-loading

Conversation

@cmcfarlen

Copy link
Copy Markdown
Contributor

Replace the single global geo DB handle with a cache of handles keyed by path, allowing different remap rules to use different MMDB files via --geo-db-path. The handle is threaded through RulesConfig -> Resources -> ConditionGeo::get_geo_*() methods. The cache deduplicates when multiple rules share the same path.

Replace the single global geo DB handle with a cache of handles
keyed by path, allowing different remap rules to use different
MMDB files via --geo-db-path. The handle is threaded through
RulesConfig -> Resources -> ConditionGeo::get_geo_*() methods.
The cache deduplicates when multiple rules share the same path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
(cherry picked from commit 8713ad4)
@cmcfarlen cmcfarlen added this to the 11.0.0 milestone Mar 30, 2026
@cmcfarlen
cmcfarlen requested review from Copilot and zwoop March 30, 2026 22:55
@cmcfarlen cmcfarlen self-assigned this Mar 30, 2026
@cmcfarlen cmcfarlen added the header_rewrite header_rewrite plugin label Mar 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the header_rewrite plugin’s Geo condition plumbing to support per-remap Geo DB “handles” (selected via --geo-db-path), enabling different remap rule instances to use different MaxMind DB files while deduplicating handles for identical paths.

Changes:

  • Thread a geo_handle pointer from RulesConfigResourcesConditionGeo::get_geo_*() calls.
  • Replace the single MaxMind global DB with a path-keyed cache of opened DB handles.
  • Update GeoIP and MaxMind condition implementations to use the passed handle instead of global state.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
plugins/header_rewrite/resources.h Adds Resources::geo_handle to carry a per-instance Geo handle through request evaluation.
plugins/header_rewrite/header_rewrite.cc Initializes a Geo handle per global/remap instance and attaches it to Resources during execution.
plugins/header_rewrite/conditions.h Updates ConditionGeo virtual API to accept a geo_handle parameter.
plugins/header_rewrite/conditions.cc Passes res.geo_handle into Geo lookups during evaluation and value appends.
plugins/header_rewrite/conditions_geo.h Updates Geo backends’ initLibrary and lookup signatures to accept/return a handle.
plugins/header_rewrite/conditions_geo_maxmind.cc Implements a path-keyed cache of MaxMind DB handles and uses per-handle schema + MMDB state.
plugins/header_rewrite/conditions_geo_geoip.cc Introduces a singleton handle set for GeoIP DB objects and routes lookups through the handle.
Comments suppressed due to low confidence (1)

plugins/header_rewrite/header_rewrite.cc:92

  • RulesConfig now owns a per-instance geo_handle, but there is no participation in the handle cache lifetime (no release/decrement in ~RulesConfig / TSRemapDeleteInstance). If the MaxMind cache is updated to close handles when unused, RulesConfig should release its handle so handles don’t become permanently pinned after remap rule reloads.
  RulesConfig(int timezone, int inboundIpSource, void *geo_handle = nullptr)
    : _timezone(timezone), _inboundIpSource(inboundIpSource), _geo_handle(geo_handle)
  {
    Dbg(dbg_ctl, "RulesConfig CTOR");
    _cont = TSContCreate(cont_rewrite_headers, nullptr);
    TSContDataSet(_cont, static_cast<void *>(this));
  }

  ~RulesConfig()
  {
    Dbg(dbg_ctl, "RulesConfig DTOR");
    TSContDestroy(_cont);
  }

Comment on lines +38 to 45
struct MmdbHandle {
MMDB_s db;
MmdbSchema schema = MmdbSchema::NESTED;
};

static std::map<std::string, MmdbHandle *> gMmdbCache;
static std::mutex gMmdbCacheMutex;

Copilot AI Mar 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new MaxMind handle cache stores heap-allocated MmdbHandle instances (and their MMDB_s mmaps) for the lifetime of the process, but there is no corresponding MMDB_close()/delete path. This can lead to unbounded growth in mapped memory / open files if remap instances are recreated over time with different --geo-db-path values. Consider adding reference counting and cleanup (e.g., close+erase when the last RulesConfig using a path is deleted), or a bounded/evicting cache with explicit teardown on shutdown/reload.

Copilot uses AI. Check for mistakes.

@zwoop zwoop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Copilot's comment is a red herring mostly, the use cases it fears are just not likely to happen.

@cmcfarlen
cmcfarlen merged commit a26a2d8 into apache:master Apr 27, 2026
19 checks passed
@cmcfarlen
cmcfarlen deleted the hrw-mmdb-loading branch April 27, 2026 22:37
cmcfarlen added a commit that referenced this pull request Jun 9, 2026
Replace the single global geo DB handle with a cache of handles
keyed by path, allowing different remap rules to use different
MMDB files via --geo-db-path. The handle is threaded through
RulesConfig -> Resources -> ConditionGeo::get_geo_*() methods.
The cache deduplicates when multiple rules share the same path.

(cherry picked from commit 8713ad4)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
(cherry picked from commit a26a2d8)
@cmcfarlen cmcfarlen modified the milestones: 11.0.0, 10.2.0 Jun 16, 2026
cmcfarlen added a commit to cmcfarlen/trafficserver that referenced this pull request Jul 29, 2026
Replace the single global geo DB handle with a cache of handles
keyed by path, allowing different remap rules to use different
MMDB files via --geo-db-path. The handle is threaded through
RulesConfig -> Resources -> ConditionGeo::get_geo_*() methods.
The cache deduplicates when multiple rules share the same path.


(cherry picked from commit 8713ad4)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

header_rewrite header_rewrite plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants