PMM-15279 Productize the SEP nginx reverse proxy

[yyyyyyyan]

Ticket number: PMM-15279

Feature build: pending — this branch cannot be exercised end-to-end alone; see Dependencies below.

Ships the nginx reverse-proxy configuration that routes /sep/ to the SEP side-car as first-class shipped configuration. Today this exists only as a hand-maintained overlay in SEP's feature-build harness, which mounts a rendered pmm.conf over the stock one and reaches the side-car at a fixed Compose IP — neither of which can ship.

Targets PMM-15205-sep-fb, the PMM-15205 epic's integration branch, where every child of that epic lands before the epic goes to main. It extends the same PMM_ENABLE_SEP gate and the same parser.go case as #5700 (PMM-15238), which is already merged to main. Entirely opt-in: with the flag unset the only shipped delta is one inert glob include.

How it works

pmm.conf gains a single line inside its server block:

include /etc/nginx/sep.d/*.conf;

A glob include whose directory is empty or absent is a no-op, so the shipped config is behaviourally inert with the flag off. The drop-in deliberately does not live in conf.d/: nginx.conf includes that directory at http context, where location is not allowed, and nginx would refuse to start.

The entrypoint renders build/ansible/roles/nginx/files/sep/sep.conf.template into /etc/nginx/sep.d/sep.conf when PMM_ENABLE_SEP is enabled, and clears the directory when it is not — idempotent in both directions. The template reaches the image via the existing COPY ansible /opt/ansible, the same mechanism #5700 relies on for postgres-sep. The existing nginx -t gate already runs before supervisord starts, so it validates the rendered drop-in for free.

Two values are substituted at start:

• Upstream — set $sep_upstream "<host>:<port>"; + proxy_pass http://$sep_upstream;. Because the upstream is a variable, nginx defers resolution to request time: the config loads with SEP absent, and a side-car that restarts on a new address is picked up within the resolver TTL without restarting pmm-server. The variable must carry no URI component — a trailing / replaces the request URI rather than stripping a prefix.
• Resolver — read from /etc/resolv.conf at container start rather than hardcoded to Docker's 127.0.0.11. The AMI and OVF images run this same entrypoint: their systemd unit is podman run … --net pmm_default … ${PMM_IMAGE} with no command override, and the image's CMD is the entrypoint. Under Podman that network is served by aardvark-dns at a different address — measured 10.89.0.1 there versus 127.0.0.11 under Docker — so a hardcoded resolver would have silently broken those distributions.

Two details about the resolver that are easy to get wrong, and are the reason it looks the way it does:

• It is declared inside location /sep/, not at the drop-in's top level. The drop-in is included in the server block, so a top-level resolver would override the http-level resolver 8.8.8.8 8.8.4.4 for every other location that resolves a name at request time — location = /percona-blog/feed does exactly that via proxy_pass $feed. Scoping it keeps the flag from changing DNS for anything but /sep/.
• An IPv4 nameserver is preferred, with a bracketed fallback when only an unscoped IPv6 one is available. nginx requires IPv6 resolver addresses in brackets; a bare one is [emerg] invalid port in resolver, which fails nginx -t and therefore stops the whole server from booting — on a network the operator never configured for SEP. A scoped address (fe80::1%eth0) is skipped rather than stripped of its zone, and a resolv.conf offering nothing else is a named fatal: nginx rejects [fe80::1%eth0] as an invalid IPv6 address and has no other syntax for the interface scope, so dropping the zone would yield a config that passes nginx -t and can never route DNS — every /sep/ request timing out into the 503 instead of the server refusing to start.

PMM_SEP_ADDRESS is new, optional, and defaults to sep:9000. It is validated as <host>:<port> with the port bounded to 1–65535 before interpolation, because the value is written into an nginx config: an unvalidated one is a config-injection vector, and because a variable proxy_pass is parsed per request, an out-of-range port would otherwise pass nginx -t and surface only as a runtime 502.

One /sep/ prefix, not five top-level names

The issue left the namespace question open, and this settles it on a single /sep/ prefix. The alternative — proxying /api, /sep_app, /stream-logs, /execution-events, /files — claims five generic top-level names at pmm-server's document root for a side-car. PMM's own API lives at /v1/, so a top-level /api owned by SEP forecloses the most obvious future name for PMM's own API surface. /sep/ reserves exactly one name and is self-describing.

Stripping the prefix in nginx was considered and rejected: with SEP's root_path unset, its url_for-generated URLs in API payloads come back unprefixed, so stripping converts a one-line SEP change into an audit of every generated URL.

The cost is that SEP must serve itself under the prefix — and it now does: SEP's app is constructed with root_path=sep_settings.ROOT_PATH, and its shipped side-car profile sets ROOT_PATH: /sep, which matches the location /sep/ hardcoded here. PMM deliberately does not add a PMM_SEP_PREFIX variable; the prefix is fixed topology on both sides, and a knob would create a second source of truth.

One consequence worth stating: forwarding one prefix proxies SEP's whole surface, where the five-prefix design incidentally firewalled off everything it did not name. That is bounded by SEP's own auth (below) and shrinks further when SEP's legacy SSR layer is removed.

This made three of the issue's original acceptance criteria stale; they were realigned on the ticket before implementation, and the sibling FE tickets were updated to target /sep/*.

What changed

File	Change
build/ansible/roles/nginx/files/conf.d/pmm.conf	one include line inside server
build/ansible/roles/nginx/files/sep/sep.conf.template	new — the /sep/ location, its resolver, and the unavailable-fallback
build/ansible/roles/nginx/tasks/main.yml	create /etc/nginx/sep.d/ at build time (pmm:root, 0775)
build/docker/server/entrypoint.sh	render the drop-in when the flag is on, clear it when off
managed/utils/envvars/parser.go (+ test)	recognise PMM_SEP_ADDRESS
managed/services/server/logs.go (+ test)	collect the drop-in into the support bundle
docker-compose.yml, the Compose sample environment file	pass through and document PMM_SEP_ADDRESS

Creating sep.d/ at build time rather than only in the entrypoint is what makes the arbitrary-UID-with-GID-0 (OpenShift) case work.

The support bundle globs /etc/nginx/sep.d/*.conf rather than naming the file: the collector records a read error as a zip entry and logs it at error level, so a statically listed path that is absent by design would put a spurious error in every bundle on the default configuration. TestFiles skips the drop-in for the same reason — it asserts an exact filename list, which cannot hold for a file whose presence depends on the flag.

One line from #5700 is reworded: its warning read ignoring PMM_ENABLE_SEP, the embedded PostgreSQL is not in use. That was accurate when the flag only drove the database exposure; now the flag also drives the reverse proxy, which is independent of which database SEP uses, so the message would over-claim.

Trust boundary

/sep/ sets auth_request off, so PMM contributes no authentication or authorization to anything under that prefix — all of it is SEP's. This is not incidental: pmm-managed's /auth_request forwards the client's Authorization header to Grafana, so leaving it on would reject the SEP bearer the embedded UI is meant to send, even for a valid PMM session. AC-7 also forbids synthesizing headers here.

The practical surface is narrow — SEP requires a bearer on its API and streaming routes, including GETs — but the enforcement lives entirely on SEP's side, and nothing in PMM would notice a SEP route added without it. Worth carrying into the Tech-Preview risk notes rather than leaving implicit.

Testing

shellcheck build/docker/server/entrypoint.sh is clean (also at -S style), and go test ./managed/utils/envvars/... passes with the new PMM_SEP_ADDRESS assertion added to the existing SEP subtest. make check is clean.

The rest was exercised in a container rig built from the real shipped configs — this branch's nginx.conf, pmm.conf and sep.conf.template, with the drop-in rendered by this branch's actual entrypoint block, on nginx:1.26-alpine (the version tasks/main.yml pins) — against an echo/stream stub standing in for the side-car, under Docker and Podman.

AC	Scenario	Result
1	nginx -t, flag off, sep.d empty and absent	passes both ways; config delta is the include line alone
2	GET /sep/api/things?x=1&y=2	SEP saw path /sep/api/things, query x=1&y=2
3	Flag on → restart with it unset	drop-in written, then directory emptied — including a planted stale .conf
4	Flag on, side-car absent	nginx starts, no host not found in upstream; /sep/ returns the 503 JSON
5	Side-car moved 172.18.0.3 → 172.18.0.4, pmm-server not restarted	next request 200 (restart count still 0)
6	SSE stream; 75 s idle; WebSocket upgrade	5 chunks ~1 s apart; 200 after 75 s idle (the 60 s default would 504); 101 Switching Protocols with a bidirectional frame
7	Without / with Authorization	absent stays absent; Bearer … arrives byte-identical; server-level X-Forwarded-For still reaches SEP
8	All 26 stock locations, plus /september and /sep-report, flag on	zero reached the side-car
9	25 MB POST /sep/files/upload	200, all 26,214,400 bytes received (location raises the server's 10m)
10	Side-car down, maintenance off	503 + {"code":14,…} in 9 ms, not the maintenance page

Both resolver behaviours were checked against a positive control rather than asserted:

• Scoping. A probe location mirroring /percona-blog/feed (set $probe http://sep:9000/health; proxy_pass $probe;) returns 502 with the shipped drop-in — public DNS cannot resolve a container name — and 200 when the same resolver is moved to the drop-in's top level, i.e. the leak is real and the location scoping closes it, with /sep/ still 200 either way.
• IPv6. Six /etc/resolv.conf shapes. IPv4-only, IPv6-then-IPv4 and IPv6-only yield a resolver that passes nginx -t (10.0.0.53, 10.0.0.53, [fd00::1]); scoped-then-unscoped now prefers the later usable nameserver ([fd00::1]); scoped-only and empty each exit 1 with their own message. Controls confirm the three forms nginx rejects: unbracketed fe80::1%eth0 is [emerg] invalid port in resolver, bracketed-with-zone [fe80::1%eth0] is [emerg] invalid IPv6 address in resolver, and a bare fd00::1 is invalid port. The scoped-only and scoped-then-unscoped rows were added after review — the first revision stripped the zone, which passed nginx -t and produced a resolver that could not route.

Also covered: percent-encoding survives un-decoded (a%20b.txt); a 502 returned by SEP itself passes through unchanged rather than becoming the JSON body (proxy_intercept_errors is off); bare /sep 301s to /sep/; malformed PMM_SEP_ADDRESS, an out-of-range port, an empty /etc/resolv.conf and a missing template each exit 1 with a named message and leave no partial drop-in; and the UID matrix — 1000:0 and 4001:0 succeed, 2000:2000 fails loudly. Under Podman on a pmm_default network the resolver was derived as 10.89.0.1 and requests proxied end-to-end, confirming the AMI/OVF path.

Where this evidence stops. The rig is not a built PMM server image, so two claims are only partly discharged: AC-4's "the rest of PMM stays fully usable" and AC-8's "unaffected" were verified as non-shadowing (no stock path is captured by /sep/, none leaks to the side-car) — the rig's stock upstreams are absent, so those locations return errors there by construction and their functional behaviour needs the feature build. AC-6's incremental delivery reproduced both with and without X-Accel-Buffering: no, so the rig confirms streaming works but does not isolate that header as the mechanism; real behaviour depends on SEP marking its own streams, which the no-proxy_buffering off design rests on. That dependency was subsequently checked against SEP's source rather than left as an assumption: the side-car runs sep_app, and every streaming route that app serves — two in app/sep/routes/stream_logs.py, one in app/sep/routes/download_files.py — sets X-Accel-Buffering: no. The StreamingResponses that do not are served by a different program on a different port and are unreachable through /sep/. TestFiles fails identically with and without this change (it needs a live PostgreSQL and container paths), so it neither covers nor is broken by the logs.go edit; the glob extracted out of it is covered by TestSepConfigFiles, which needs neither.

Known limitations

• Writes fail until the SPA sends a bearer. SEP requires a Bearer on mutating methods, and the embedded SPA currently sends none; the harness overlay hid this by injecting a static internal token, which is deliberately dropped here. Reads succeed; every POST/PUT/PATCH/DELETE gets 401/403 from SEP until the FE token-exchange ticket lands. Do not "fix" this by reintroducing injection.
• X-Forwarded-Proto reports the hop into nginx, not the client's original scheme. Behind an external TLS terminator forwarding to :8080, SEP sees http and may generate http:// absolute URLs. map cannot fix this from the drop-in, since map is http-context only and the drop-in is included inside server. No other PMM location sends this header at all, so nothing regresses — but nothing tracks it either.
• No upstream keepalive. A variable proxy_pass cannot reference an upstream block, and Connection $connection_upgrade is close for non-WebSocket requests, so every /sep/ request opens a fresh connection to the side-car. Acceptable at Tech-Preview scale, worth knowing before anyone benchmarks it.
• client_max_body_size 100m is a bound, not a measurement. It clears the server-level 10m for SEP uploads; real SEP payload sizes have not been measured.
• proxy_read_timeout is a blanket 3600s for the whole prefix; there is no per-route knowledge available to scope it, and the streams need it. A hung request holds a connection for an hour rather than 60s.
• The flag is read only at container start. Changing it on a running container does nothing — supervisord has no reload hook for nginx and the repo contains no nginx -s reload. This matches every other entrypoint-consumed PMM_* flag.
• Maintenance mode still wins. While maintenance.html exists, the server-level check runs in the rewrite phase, before location selection, so /sep/ requests get the maintenance page too. Correct behaviour, but it bounds AC-10.
• PMM_SEP_ADDRESS accepts no IPv6 literal. The validation regex admits a host of [A-Za-z0-9._-] plus a port, so [::1]:9000 is rejected and the container exits 1 — before nginx, which would accept a bracketed literal in a variable proxy_pass, is ever consulted. A side-car reachable only over IPv6 cannot be addressed. Widening the regex is small, but it adds a branch to a container-start hard gate, so it belongs in the rig rather than in a late edit.
• The flag cannot select the proxy alone while the builtin PostgreSQL is in use. postgres-sep exits 1 when PMM_ENABLE_SEP is set and PMM_SEP_POSTGRES_PASSWORD is empty, and it runs earlier in the entrypoint under set -o errexit. So an operator who wants only the reverse proxy — SEP on its own database, PMM still on its builtin one — must supply a password nothing will use, or set PMM_DISABLE_BUILTIN_POSTGRES and accept its wider consequences. The reworded warning above covers the HA and disabled-builtin cases, not this one.
• Nothing in PMM enforces the X-Accel-Buffering contract. Leaving proxy_buffering on rests on SEP marking its own streams. That holds for every streaming route the side-car serves today (verified above), but a route added later without the header is silently buffered, and the failure is invisible from PMM — no log, no warning, no test. Forwarding one prefix instead of five is what makes the set of routes this has to hold for open-ended.
• The support-bundle archive entry is covered by review only. The glob itself is now unit-tested (TestSepConfigFiles, over a temporary directory: drop-ins collected, non-.conf ignored, absent and empty each yielding nothing). What is still uncovered is that the collected path is read into the zip — that runs through the shared append loop in files(), whose dozen-plus config paths are all hardcoded, so reaching it needs an injectable config root.
• PMM-15238: Expose the built-in PostgreSQL to SEP #5700's own notes say "Docker only — the AMI and OVF distributions do not run this entrypoint." That is not accurate, as measured above; the issue's out-of-scope note has been corrected, and no code change to PMM-15238: Expose the built-in PostgreSQL to SEP #5700 is required — its gate is env-based and its subnet derivation works under Podman too.

Dependencies

Nothing is blocking merge into the epic branch any more: #5700 (PMM-15238) is on main, and the FE re-pathing onto /sep/* reached this PR's base with #5653 (PMM-15216). The SEP-side prefix support is no longer blocking — SEP's app now takes root_path from settings and its side-car profile ships ROOT_PATH: /sep.

A working feature build additionally needs the FE token exchange, the Grafana service account, and the ticket delivering SEP's secret key and database credentials — the side-car will not start without a secret key. Ordering inside the FB matters even though merge order does not: retiring the harness overlay must not land ahead of the FE token exchange, or the stack reports as working while silently failing every write. The harness also names its service differently from the sep:9000 default, so PMM_SEP_ADDRESS must be set there; the failure looks like a proxy fault rather than a missing variable.

The feature build is where the rig's blind spots get covered: SSE incrementality against real SEP, AC-5 recovery against a real restarted side-car, resolver derivation on an actual AMI, and the arbitrary-UID write path into sep.d/ in the built image.

• API Docs updated — not applicable, no API endpoints added, removed or altered.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (PMM-15205-sep-fb@af9fb1a). Learn more about missing BASE report.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             PMM-15205-sep-fb    #5759   +/-   ##
===================================================
  Coverage                    ?   44.22%           
===================================================
  Files                       ?      304           
  Lines                       ?    33119           
  Branches                    ?        0           
===================================================
  Hits                        ?    14646           
  Misses                      ?    16953           
  Partials                    ?     1520           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[theTibi]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: b5290998-5f7d-4421-95c5-9041b8006203

📥 Commits

Reviewing files that changed from the base of the PR and between aebb1c9 and 5d7065d.

📒 Files selected for processing (4)

• build/ansible/roles/nginx/files/sep/sep.conf.template
• build/docker/server/entrypoint.sh
• managed/services/server/logs.go
• managed/services/server/logs_test.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• percona/pmm-qa (manual)
• percona/pmm (manual)

🚧 Files skipped from review as they are similar to previous changes (1)

• build/ansible/roles/nginx/files/sep/sep.conf.template

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

The change adds SEP reverse-proxy configuration, startup validation and generation, the PMM_SEP_ADDRESS setting, NGINX directory provisioning, and optional SEP configuration collection in server logs.

Changes

SEP reverse-proxy integration

Layer	Summary
SEP proxy configuration	NGINX provisions and loads optional SEP configuration. The proxy handles /sep/ requests, unavailable responses, DNS resolution, forwarded headers, WebSockets, long reads, and large requests.
Startup configuration and environment handling	PMM_SEP_ADDRESS is documented and passed to the server. Startup validates the address, generates or removes SEP proxy configuration, and excludes the variable from managed settings parsing.
Optional SEP configuration collection	Server log collection discovers optional SEP NGINX files. Tests cover enabled, absent, empty, and template-only configuration directories.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant NGINX
  participant SEP
  Client->>NGINX: Request /sep/
  NGINX->>SEP: Proxy request with forwarded headers
  SEP-->>NGINX: Response
  NGINX-->>Client: SEP response

Loading

Merge Risk: 🔵 Low · up to 5d706

The opt-in /sep/ route exposes SEP through PMM's HTTPS listener while delegating authentication entirely to SEP, and invalid SEP configuration can prevent PMM Server from starting. The PR is mergeable with explicit owner awareness that SEP must protect every exposed endpoint and that deployment configuration errors affect overall server availability.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description is complete and directly related to the changes. It includes the ticket number, feature-build status, implementation details, testing evidence, dependencies, limitations, and the API d…
Title check	✅ Passed	The title clearly identifies the main change: productizing the SEP NGINX reverse proxy. It is concise, specific, and relevant to the changeset.

Full details: Description check

Explanation

The description is complete and directly related to the changes. It includes the ticket number, feature-build status, implementation details, testing evidence, dependencies, limitations, and the API documentation applicability note.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Adds an opt-in nginx reverse proxy that exposes the SEP sidecar under /sep/.

Changes:

• Renders and validates SEP nginx configuration at container startup.
• Adds dynamic DNS resolution and SEP-unavailable handling.
• Documents the environment variables and includes SEP configuration in support bundles.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
.env.example	Documents SEP settings.
docker-compose.yml	Passes SEP environment variables.
build/docker/server/entrypoint.sh	Renders the SEP proxy configuration.
build/ansible/roles/nginx/tasks/main.yml	Creates the SEP configuration directory.
build/ansible/roles/nginx/files/sep/sep.conf.template	Defines /sep/ proxy behavior.
build/ansible/roles/nginx/files/conf.d/pmm.conf	Includes SEP drop-ins.
managed/utils/envvars/parser.go	Recognizes the SEP address variable.
managed/utils/envvars/parser_test.go	Extends SEP environment parsing coverage.
managed/services/server/logs.go	Collects SEP configuration in support bundles.
managed/services/server/logs_test.go	Adjusts support-bundle expectations.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[marcuscruz-percona]

Reviewed against PMM-15279's ten acceptance criteria and the repo conventions. This one is close to done, and the parts the ticket flagged as traps are all handled deliberately rather than by accident.

The proxy_pass detail is the one I most expected to find wrong, since the ticket explicitly warns that "a variable in proxy_pass changes how nginx passes the request URI upstream" — and the template gets it right, carrying no URI component, with a comment saying why a trailing slash would break it. Keeping the resolver inside the location rather than at file scope is the other call I would have missed: the file is included in the server block, so a file-scope resolver would have redirected DNS for every other request-time lookup, /percona-blog/feed included. Noting that error_page and proxy_set_header are both non-additive, and that dropping the inherited 401/403/503 handlers is only safe while auth_request is off, is precisely the reasoning that usually goes unwritten and then gets broken six months later.

Deriving the resolver from /etc/resolv.conf instead of hardcoding 127.0.0.11 is a justified superset of what the ticket asked for, given its own note that AMI/OVF run this same entrypoint under Podman. And resolving the five-root-prefixes-versus-/sep/ tension in the ticket toward /sep/ is the right reading: every acceptance criterion is written against /sep/, and the Details section explicitly permitted the single-prefix option.

I checked AC8 by reading the whole of pmm.conf on this branch rather than trusting the prefix list — there are no regex locations anywhere in it, only the three exact matches (= /, = /dashboard/snapshots, = /percona-blog/feed), so nothing can pre-empt /sep/ and /sep/ collides with no existing prefix. AC1, AC2, AC3, AC4, AC5, AC7, AC9 and AC10 all hold on static reading; the map $http_upgrade $connection_upgrade the headers depend on is where the ticket said it would be.

Verdict

Approve once the resolver value is validated and the title loses its colon. Both are small. Everything else below is comment-level, and one item is a question rather than a request.

Must-fix

1. Validate SEP_RESOLVER the way SEP_ADDRESS is already validated — one guard fixes two defects.

Defect one. A nameserver line with no address renders resolver []. The IPv4 scan prints an empty $2, command substitution strips it to "", the -z guard correctly falls through — and the IPv6 scan then prints the brackets unconditionally:

$ printf 'search example.com\nnameserver\nnameserver 8.8.8.8\n' > rc1
$ R=$(awk '/^nameserver/ && $2 !~ /:/ { print $2; exit }' rc1)
$ [ -z "$R" ] && R=$(awk '/^nameserver/ && $2 !~ /%/ { print "[" $2 "]"; exit }' rc1)
$ echo "SEP_RESOLVER='$R'"
SEP_RESOLVER='[]'

[] is non-empty, so both -z guards and the scoped-IPv6 FATAL are skipped, and nginx -t -e /dev/stdout at :228 then aborts the container with an opaque parse error instead of the FATAL message written for exactly this case.

Defect two. awk takes $2 as a whitespace-delimited field, so any payload without spaces survives into the config:

$ printf 'nameserver 8.8.8.8;}location/x{deny\n' > rc2
$ R=$(awk '/^nameserver/ && $2 !~ /:/ { print $2; exit }' rc2)
$ sed -e "s|__SEP_RESOLVER__|${R}|" tpl
resolver 8.8.8.8;}location/x{deny valid=10s;

That closes the /sep/ block and opens an attacker-chosen one. Exploitability is low, since /etc/resolv.conf is written by the container runtime and an operator who can set compose dns: is already privileged — but the file's own comment validates SEP_ADDRESS because "an unvalidated value is a config-injection vector," and then leaves the other interpolated value unvalidated. (For the record, & is not the problem I first suspected: & in a sed replacement renders harmlessly as __SEP_RESOLVER__evil.)

A single check — ^[0-9.]+$ or ^\[[0-9a-fA-F:]+\]$ — rejects the injection and rejects [], since the bracket branch requires at least one character inside.

2. The title's colon lands in v3 history. dev/docs/process/GIT_AND_GITHUB.md:21 fixes the template as PMM-XXXX Short summary up to 50 characters. with no colon, and AGENTS.md repeats it. fdd14129a and the PR title both use PMM-15279:; the other two commits comply. Since squash-merge takes the PR title, this is the one item that cannot be fixed after merge.

Smaller things

3. AC6 is partial — proxy_buffering off is omitted. The Details section is specific: the streaming locations "need proxy_http_version 1.1, proxy_buffering off, the Upgrade/Connection upgrade headers, and a read timeout well above nginx's 60s default." Three of four are here, with the fourth delegated to SEP's X-Accel-Buffering: no. nginx does honour that per response, so AC6 is satisfiable, and the argument in the comment — stream knowledge stays in SEP rather than being encoded as routes here — is a reasonable one I would not overrule. The cost is that incremental delivery becomes contingent on an upstream header PMM neither sets nor enforces, and a future SEP stream route that omits it buffers silently with no failure visible on this side. Does SEP treat that header as a guarantee for every streaming response? If yes, this is fine as designed and worth saying so in the template comment.

4. ipv6=off makes an IPv6-only network permanently unreachable. ipv6=off suppresses AAAA lookups for the proxied name, independent of the resolver's own address family — so where SEP has only an AAAA record, every /sep/ request resolves to nothing and lands in @sep_unavailable forever. The entrypoint's fallback to a bracketed IPv6 resolver is exactly the case where that is most likely: it picks an IPv6 resolver, then tells nginx not to ask for IPv6 addresses. Narrow, since dual-stack is the norm, but the two settings point opposite ways.

5. A SEP-only misconfiguration becomes a PMM-wide boot refusal. entrypoint.sh:197-205 makes an empty or scoped-only-IPv6 resolv.conf fatal whenever the flag is on. The messages are good and the choice is deliberate, but it trades AC4's "the rest of PMM stays fully usable" for the container not starting at all. Rendering nothing and warning would keep the rest of PMM up. Worth a conscious decision rather than a default, especially since sep-secrets on #5762 adds more entrypoint-fatal paths under the same flag.

6. Two comments overstate what the checks do. "The digit count is capped so the range test below cannot be handed a value that overflows the shell's integer parsing" — [ -lt ] parses decimal without error, and five digits is nowhere near overflow. Relatedly, sep:0080 passes both checks and renders literally:

$ a=sep:0080; p=${a##*:}; [[ "$a" =~ ^[A-Za-z0-9._-]+:[0-9]{1,5}$ ]] && [ "$p" -ge 1 ] && echo "ACCEPTED port='$p'"
ACCEPTED port='0080'

7. sepConfigFiles's error branch is unreachable. filepath.Glob returns only ErrBadPattern, and the pattern is built from the sepNginxConfigDir constant, so err is always nil — the log line and the "absent directory is not an error" subtest exercise nothing. Not harmful, just dead weight.

Notes, not asks

• PMM_SEP_ADDRESS contradicts the ticket's "this introduces no new variable," and the sep:9000 default alone would satisfy the spec. But it is registered at parser.go:124, documented in .env.example with the default explained, and passed through in docker-compose.yml — so the handling is complete and it is genuinely useful where the side-car's service name differs. Recording it as scope rather than asking for its removal.
• The logs.go support-bundle collection and TestSepConfigFiles are not in the ticket either, but they avoid a spurious read error in every bundle once sep.conf exists, which is the kind of thing nobody comes back for. Fine by me.
• TestSepConfigFiles passes locally. TestFiles does not, before or after this PR — it has a pre-existing dependency on /etc/nginx/* and /srv/prometheus/* — so the new sep.conf guard and the slices.Concat integration have no coverage that runs outside a container. Not this PR's debt; worth knowing the guard is unexercised.
• location /sep/ does not match /sep with no trailing slash, and there is no bare location / to catch it. Almost certainly irrelevant if the SEP UI only ever emits /sep/…, but there is no redirect either.
• auth_request off in location /sep/ opens an unauthenticated path on PMM's port. The ticket requires exactly this and the template documents that SEP checks its own bearer, so I am not contesting it — just flagging it as the sort of thing that deserves a named sign-off rather than passing through unremarked.
• Three separate awk scans of /etc/resolv.conf could be one pass yielding family and scoped-only together, which would also make item 1 harder to write.
• Not updating build/AGENTS.md: I checked the trigger list, and it fires on top-level directory, tech-stack or build-target changes. roles/nginx/files/sep/ is none of those, so I would not block on it.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[yyyyyyyan]

Thanks marcuscruz-percona — checking AC8 by reading the whole of pmm.conf rather than trusting the prefix list is the part I could not have done for myself, and the error_page/proxy_set_header non-additivity reading is the one I most wanted a second pair of eyes on.

All of it is addressed in 5d7065d except item 6, where I think the measurement goes the other way. Two items were behaviour choices rather than defects, so I want to be explicit about which way I took them.

1. SEP_RESOLVER validation — confirmed, both defects. Reproduced exactly as written. An addressless nameserver line yields [] from the bracket branch, which is non-empty and therefore skips both -z guards and the FATAL written for precisely that case; and $2 being whitespace-delimited carries 8.8.8.8;}location/x{deny verbatim into the rendered config.

Your guard is in, ^([0-9.]+|\[[0-9a-fA-F:]+\])$, sited after the existing FATAL block. I also added $2 != "" to both awk scans, because the guard alone would report an addressless-only resolv.conf as "not a usable address" when the truthful message is the one already written: no nameserver found. With both:

shape	before	after
IPv4 only	10.0.0.53	10.0.0.53
IPv6 then IPv4	10.0.0.53	10.0.0.53
IPv6 only	[fd00::1]	[fd00::1]
scoped IPv6 only	fatal	fatal
scoped then unscoped	[fd00::1]	[fd00::1]
addressless then real	resolver [], nginx -t aborts	8.8.8.8
addressless only	resolver [], nginx -t aborts	fatal, "no nameserver found"
injection payload	interpolated verbatim	fatal, names the value
empty	fatal	fatal

I swept the rest of the tree for the same construct: this template is the only sed interpolation into a config under build/, and both of its placeholders are now validated, so there is nowhere else to carry the fix.

2. Title colon — fixed. The PR title is now PMM-15279 Productize the SEP nginx reverse proxy. You were right that it was the one item that could not be repaired after merge.

3. AC6 — yes, and now recorded in the template. SEP sets X-Accel-Buffering: no from a single shared constant used by every streaming route, and that constant is frozen specifically so a route needing an extra header has to copy it rather than mutate the shared object. That is what makes "every streaming response carries it" a property rather than a habit, which was the thing your question was really asking. Written into the comment beside proxy_http_version.

4. ipv6=off — removed. You are right that the two settings point opposite ways, and the bracketed-IPv6 fallback is exactly where it bites. I dropped it rather than making it track the resolver family: the contradiction is the defect, and one AAAA query per resolution against a container DNS that answers an empty NOERROR is not worth a conditional and a second placeholder.

5. Boot refusal — kept fatal, deliberately. I weighed warn-and-skip and would rather the container refuse to start. An operator who set PMM_ENABLE_SEP gets told exactly why it cannot work, where the degrade turns that into /sep/ 404ing with the explanation buried in a startup log nobody reads. Your framing of the cost is right — it does trade AC4's "the rest of PMM stays fully usable" — and I would rather pay that visibly than quietly. The new resolver guard follows the same shape for consistency.

6. The overflow comment — I think this one is wrong, with a caveat. [ errors on a value past int64 rather than parsing it:

$ [ 9223372036854775808 -gt 65535 ]; echo $?
bash: [: 9223372036854775808: integer expected
2

Both halves of [ "$p" -lt 1 ] || [ "$p" -gt 65535 ] then exit 2, so the if is false and the range check fails open — which is what the {1,5} cap prevents. 9223372036854775807 compares fine, so the boundary is int64 rather than five digits, but the mechanism the comment describes is real.

The caveat is your sep:0080 example, which is right: it passes both checks and renders literally. nginx parses that through ngx_atoi as port 80, so it works, and I left it rather than adding a leading-zero rule for a value that behaves correctly.

7. Dead error branch — removed. filepath.Glob returns only ErrBadPattern and the pattern is a directory joined with a literal suffix, so nothing reached it; sepConfigFiles sheds its ctx parameter with it. The three subtests stay — they were never exercising the error path, but "an absent directory yields nothing" is the actual contract and worth pinning.

On the notes: collapsing the three awk passes into one is a fair reading, and I left them separate because each answers a different question and a merged version would have to encode in flags the precedence the fall-through currently expresses positionally. The bare /sep with no trailing slash is real; SEP only ever emits /sep/… today, so I have not added a redirect. TestSepConfigFiles passes; TestFiles still does not, for the pre-existing /etc/nginx and /srv/prometheus reasons you found.