Commit Graph

9 Commits

Author SHA1 Message Date
Pim van Pelt
055cf9f830 Update docs and header comments for the IP_PKTINFO attribution model
The SO_BINDTODEVICE → IP_PKTINFO switch in the previous commit
was a semantic change: the module no longer touches outgoing
routing at all, and several places in the docs and the module's
top-of-file comment still described the old mechanism.

- README.md and debian/control now describe attribution as
  reading the ingress ifindex per connection from the kernel's
  IP_PKTINFO / IPV6_PKTINFO cmsg, and explicitly call out that
  the DSR / maglev return-path constraint is what makes the
  change necessary.
- docs/design.md FR-1.1 / FR-1.5 / FR-1.6 are rewritten to
  forbid SO_BINDTODEVICE and to describe the cmsg-based lookup.
  NFR-6.1 notes these are ordinary unprivileged socket options.
  The "Components" / "Composes With" sections and the
  "Alternatives Considered" entry are brought in line — and a
  new entry records SO_BINDTODEVICE as a rejected alternative
  with the exact failure mode seen on an IPng production box.
- docs/config-guide.md already carried the new description;
  unchanged here.
- src/ngx_http_ipng_stats_module.c's top-level block comment is
  rewritten to match; the section header above init_module goes
  from "rebind listen sockets with SO_BINDTODEVICE" to "enable
  IP_PKTINFO on listen sockets, resolve ifindexes".

Three SO_BINDTODEVICE mentions deliberately remain in the source
and one in the design doc's alternatives table — all of them
explain that the module *avoids* the option, which is itself
load-bearing documentation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 15:16:10 +02:00
Pim van Pelt
450391af6b Switch per-device attribution from SO_BINDTODEVICE to IP_PKTINFO
SO_BINDTODEVICE pins both ingress *and* egress to the bound
interface — the kernel uses the listening socket's device
binding when choosing the output interface for the SYN-ACK,
which is sent before accept() returns and therefore can't be
fixed up in userspace. That's fatal for maglev / DSR
deployments where the SYN arrives through a GRE tunnel but the
return path has to leave via the default route; the SYN-ACK
goes out the GRE and is dropped by the uplink, so every new
connection times out.

Rework the listen plumbing so the module never touches
SO_BINDTODEVICE. init_module now enables IP_PKTINFO and
IPV6_RECVPKTINFO on every HTTP listening socket and resolves
each configured `device=` name to an ifindex. At request time
resolve_source calls getsockopt(IP_PKTOPTIONS) on the accepted
fd to read the per-connection in(6)_pktinfo cmsg the kernel
stashed during the handshake, then matches (ifindex, family)
against the bindings table. The listening sockets remain plain
wildcards, so the return path follows the normal routing table
and DSR works.

The wrapper also no longer clones or rebinds sockets: it still
dedups per (cscf, sockaddr) so multiple device-tagged listens
in a single server block coexist, and dedups bindings on
(device, family) so the same device can carry different tags
for v4 and v6 (e.g. tag2-v4 / tag2-v6) but not pointlessly
duplicate when a listen include is shared across server blocks.

Drive-by fixes to unblock `make pkg-deb` after a prior
`make build-asan`:
- debian/rules overrides dh_clean to exclude build/, since
  nginx-asan's install creates nobody:0700 temp dirs dh_clean
  can't traverse.
- Makefile's build-asan removes those unused runtime temp dirs
  so the tree is clean afterwards.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 15:00:46 +02:00
Pim van Pelt
1f144f4c19 Fix shared-listen-include pattern across multiple server blocks
v0.3.0's listen wrapper treated every listen beyond the first at a
given sockaddr as a skip-core "duplicate", which was correct for
two `listen 80 device=X/Y;` lines in one server block but broke on
the real deployment pattern where every server-*.conf pulls in the
same `include listens.conf;`. Symptoms:

  * every server block after the first ended up with no listen
    directive processed, so nginx assigned them the default
    `*:80`, producing a flood of "conflicting server name"
    warnings and attaching every server block to an unrelated
    wildcard bind;
  * the bindings list grew linearly with the number of server
    blocks, so init_module tried to create (server_blocks) ×
    (devices × families) listening sockets and hit EMFILE.

Replace the single dedup with two independent checks:

  * listens_seen is a (cscf, sockaddr) ledger. The core listen
    handler is invoked at most once per (server block, sockaddr),
    matching nginx's own duplicate check so server-block N just
    attaches its cscf to the existing address via
    ngx_http_add_server.

  * `bind` is added only for the first global occurrence of each
    sockaddr; subsequent cscfs inherit opt.set/opt.bind from the
    first, which is what keeps nginx's "duplicate listen options"
    check happy across server blocks.

  * bindings dedup on (sockaddr, device) globally, so init_module
    creates one socket per unique pair regardless of how many
    server blocks reference it.

Add a regression test at tests/01-module/ that wires three server
blocks to the same ipng-listens.inc and asserts that nginx -t is
clean and exactly four sockets are bound on port 8080.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 13:15:04 +02:00
Pim van Pelt
df05bae8a3 Support multiple device-pinned listens sharing a single port
Nginx's config-level duplicate-listen check rejected the
documented pattern of `listen 80 device=X ipng_source_tag=A;
listen 80 device=Y ipng_source_tag=B;` with "a duplicate listen
0.0.0.0:80", and even when the dedup was bypassed the kernel
refused the second bind() because the first socket was already
holding the port without SO_BINDTODEVICE.

The listen wrapper now detects same-sockaddr duplicates before
the core handler sees them and records them with `needs_clone=1`.
In init_module, phase 1 clones an ngx_listening_t for each such
duplicate, phase 3 closes every inherited naked fd, and phase 4
rebinds every target with SO_REUSEADDR + SO_REUSEPORT +
SO_BINDTODEVICE set before bind(). SO_REUSEPORT keeps
`nginx -s reload` from colliding with the still-bound sockets
held by old workers during graceful drain; IPV6_V6ONLY matches
nginx's default so the IPv6 listen doesn't claim the IPv4
wildcard and collide with sibling IPv4-specific listens.

Restructure 01-module to cover the pattern end-to-end: four
device-pinned listens on port 8080 (eth1 shares tag `tag1`
across v4 and v6; eth2 splits into `tag2-v4` / `tag2-v6`),
clients and server both get IPv6 addresses, and a new
"Per-(device, family) request count accuracy" case proves that
10 requests on each of the four combinations yields tag1=20,
tag2-v4=10, tag2-v6=10. Mgmt/direct traffic moves to port 9180
so it no longer clashes with the shared-port wildcards.

Document the constraint in docs/user-guide.md: all listens on
a given port must carry `device=`, and direct traffic belongs
on a separate port.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 11:45:40 +02:00
Pim van Pelt
fdef2a552b Harden scrape rendering and add AddressSanitizer test suite
Move all heap allocation out of the slab-mutex critical section in
render_prom/render_json: snapshot cardinality under a brief lock,
allocate aggs/snaps/string tables outside the lock, then re-acquire
only to deep-copy strings and walk the LRU into the pre-allocated
buffers. A worker crash during output buffer allocation can no
longer leave the shared-memory zone locked, and a corrupt cardinality
count is caught by a 10k sanity cap rather than causing a runaway
ngx_pcalloc.

Add build-asan and tests/02-asan/: a full sanitizer-instrumented
nginx + module built via apt-source, and a 2-node containerlab
Robot suite that drives reload storms, concurrent scrape-during-reload,
and intern-table growth, failing if AddressSanitizer or UBSan
reports anything on stderr. The two Robot suites now check for
their required build artifacts up front so `make robot-test` no
longer rebuilds them on every invocation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 10:58:51 +02:00
Pim van Pelt
cdcbb07c9a Release v0.2.0: single-source the version, wildcard it in docs
Introduces a VERSION variable in the top-level Makefile as the
authoritative source for the module's reported version. A new
version-header target writes src/version.h only when the content
would change, so no-op rebuilds don't rewrite the file. The C source
#includes that header in place of a hardcoded #define; the
user-guide's install example is wildcarded
(libnginx-mod-http-ipng-stats_*_amd64.deb) so it doesn't drift.

The design doc still references v0.2.0 by name — operators read it as
a point-in-time description, not a moving target.

debian/changelog keeps its own 0.2.0-1 entry because dpkg reads the
package version from there directly; the e2e test is updated to match
the JSON schema bump to 2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-17 14:45:15 +02:00
Pim van Pelt
b3ad74cbde Reduce scrape cardinality: class codes, per-(source,vip) histograms, byte histograms
Collapses the status-code dimension of the counter key into six class
lanes (1xx..5xx/unknown) so per-(source,vip) counter cardinality no
longer grows with the number of distinct three-digit responses nginx
serves. Histogram series drop the code label entirely and aggregate
across classes. Adds nginx_ipng_latency_total with a code class label
so average latency per class can still be computed off the scrape.
Adds nginx_ipng_bytes_{in,out} histograms with configurable boundaries
via the new ipng_stats_byte_buckets directive. Bumps JSON schema to 2.

Operators who need full three-digit-code resolution should consume the
ipng_stats_logtail stream off-host; the stats zone intentionally trades
that resolution for a bounded scrape size.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-17 14:36:16 +02:00
Pim van Pelt
87050bcf13 Add logtail if=$variable filtering and update log format examples
- ipng_stats_logtail now accepts an optional if=$variable parameter
  that suppresses log lines when the variable is empty or "0",
  following the same semantics as nginx's access_log if=. The
  condition is checked before format rendering for zero overhead on
  filtered requests. Filtered requests are still counted by stats.
- Log format examples updated to include $scheme for http/https
  visibility, and renamed to ipng_stats_logtail to match production.
- Robot test added for the if= filter (19 tests, 19 pass).
- FR-8.5 added to design doc for the if= semantics.
2026-04-16 18:49:22 +02:00
Pim van Pelt
5a7e2f77f1 Add ngx_http_ipng_stats_module: per-VIP, per-device traffic counters
Full implementation of the nginx dynamic module with:
- SO_BINDTODEVICE-based per-interface traffic attribution
- Per-worker lock-free counters flushed to shared memory
- Prometheus text and JSON scrape endpoint at configurable location
- UDP-only global logtail (ipng_stats_logtail) for fire-and-forget
  access log streaming
- $ipng_source_tag nginx variable for use in log_format/map
- Histogram buckets, EWMA rate gauges, zone meta-metrics
- Debian packaging (libnginx-mod-http-ipng-stats)
- Robot Framework end-to-end tests via containerlab
- SPDX Apache-2.0 headers on all source files
2026-04-16 17:41:23 +02:00