fix(nginx): re-resolve upstreams so a container restart cannot break ingest - #4492
fix(nginx): re-resolve upstreams so a container restart cannot break ingest#4492spawnia wants to merge 2 commits into
Conversation
…ingest nginx resolves the service names in an upstream block once at startup and keeps the address for the life of the worker process. Docker does not guarantee a container the same address across starts, so any restart can leave nginx proxying to a container that no longer owns the old address. Ingest then answers 502 on every request while the web UI keeps working, and nothing recovers until someone restarts nginx by hand. The "resolve" parameter makes nginx re-resolve per DNS TTL, bounded by "valid". That is two lookups per 10s against Docker's in-process embedded DNS regardless of request volume, so the cost does not scale with load. Co-authored-by: Ilias Aaguida <ilias.aaguida@beta.gouv.fr>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 068b91e. Configure here.
Pinning 127.0.0.11 assumed Docker's embedded DNS. Podman answers on the network gateway instead, so "resolve" would have failed every lookup there and nginx would have returned 502 for every request. The nginx image already derives the address from /etc/resolv.conf into NGINX_LOCAL_RESOLVERS, which reaches the config through envsubst only, so nginx.conf is now mounted as a template and rendered over the image default. NGINX_ENVSUBST_FILTER limits substitution to that one variable so the nginx runtime variables in the config survive.
|
Deployed this on our self-hosted instance (Sentry 24.12.2, ~57 containers, Verified the rendered config inside the container: Then reproduced the original failure scenario: Before this change, that exact sequence made every ingest request return 502 while the UI stayed fully functional, which cost us ~3 hours of events. |
|
Hey @aminvakil, would you mind sharing why you closed this? I put decent effort into this, and I believe it fixes a real problem. Would love to be able to get closer to upstream and not need to keep local patches around. |
|
I've told this a thousand times, this is too much change for a specific problem which happens in very specific conditions, there is no easy way to fix this, but this fix is not correct. I would not definitely agree with merging a config change which has 9 documentation lines above that. What is this seriously? |
|
Edit: This is just a joke, do not read it. Thank you for putting this together. I have now reviewed the change from several angles, including the nginx angle, the Docker angle, the Podman angle, the DNS angle, the YAML angle, the shared-memory angle, the temporal-container-identity angle, and, perhaps most importantly, the angle from which a configuration file begins to question whether it is still a configuration file or has quietly become an orchestration framework with opinions about municipal zoning. At first glance, the proposed behavior sounds straightforward: nginx resolves an upstream hostname, a container restarts, the container may receive another address, and nginx should eventually become aware of that address without requiring a human to restart nginx. However, “straightforward” is often only a temporary visual artifact caused by observing a system from sufficiently far away. Once we zoom in, we discover DNS, shared memory, entrypoint scripts, environment substitution, Compose interpolation, multiple container engines, IPv4, IPv6, resolver validity windows, upstream keepalive pools, page sizes, comments, and the philosophical question of whether a restarted container is the same container or merely another container carrying the previous container’s service name. Before considering the implementation itself, I think we need to clarify what “resolve” means in this context. It may mean:
The PR primarily addresses the first meaning, while implicitly attempting the second, but the configuration introduces enough machinery that we may accidentally arrive at the fourth before reaching the fifth. This is not necessarily wrong, but it should be acknowledged because otherwise a future reader may assume the resolver merely resolves, when in reality it participates in a broader resolution lifecycle involving Docker, Podman, Regarding the comments above the resolver directive: the ratio between explanatory comments and executable configuration is currently approximately nine-to-one. This creates a substantial semantic mass around a single directive. While comments are free at runtime, they are not free in the cognitive economy of the repository. Every comment must be parsed by a human resolver, cached for an unspecified validity period, and potentially invalidated after a maintainer restart. Unlike Docker DNS, human DNS does not reliably answer on The following block is particularly significant: resolver ${NGINX_LOCAL_RESOLVERS} valid=10s ipv6=off;There are several independent dimensions here. First, Second, the value is generated by an entrypoint script rather than being intrinsically known to nginx. This means nginx does not know its resolver; it knows a textual representation of what another shell script believed This introduces the following chain:
This is circular, although not necessarily incorrectly circular. Wheels are circular and generally regarded as useful. Infinite recursion is also circular and is regarded less favorably. We should establish whether this is wheel-circular or recursion-circular before merging. Third, The PR explains that ten seconds bounds the outage window, but technically it bounds one particular dimension of one possible outage window, assuming the resolver is available, the answer is current, the container has started, the port is listening, the network namespace exists, the upstream zone has enough memory, the worker processing the timer is alive, and time itself continues advancing monotonically. This is probably acceptable, but the number should not acquire an authority greater than the assumptions beneath it. Fourth, The upstream zones introduce another category of concern: zone relay 64k;
zone sentry 64k;The use of the word “zone” suggests geographic or municipal boundaries, but the zone is actually shared memory. This discrepancy should be considered carefully. An operator reading “zone relay” may reasonably expect a designated area in which relaying is permitted. Instead, it allocates memory for upstream state. No zoning map is supplied, no planning permission is requested, and there is no public consultation period. The More concretely, nginx requires upstream zones to be at least eight system pages. Therefore, The mount transition also deserves extended consideration: target: /etc/nginx/templates/nginx.conf.templatePreviously, the file was mounted as the nginx configuration. It is now mounted as a template from which the nginx configuration is produced. This changes the ontological status of In other words, There is also a concrete compatibility issue. Compose merges volume entries by their target path. Existing override files that mount a custom configuration at If the custom mount is read-only, the entrypoint fails while trying to write to it, and nginx does not start. If it is writable, the rendering process may overwrite the custom configuration. We therefore have two modes:
This is a refreshingly balanced failure model, but probably not the balance users expect. The environment variable filter introduces additional punctuation concerns: NGINX_ENVSUBST_FILTER: "^NGINX_LOCAL_RESOLVERS$$"There are two dollar signs because one dollar sign must survive Compose so that another component can understand that the first dollar sign was intended as a regex end anchor rather than as an invitation to interpolate a Compose variable. This is technically explainable, but it means the reader must simultaneously understand YAML quoting, Compose interpolation, shell environment semantics, awk regular expressions, and A single line therefore spans at least five interpretation layers:
I realize that this list contains six layers after promising five. This demonstrates the core problem: once substitution begins, the layer count itself is not stable. The proposed test plan is directionally useful, but I believe it should be expanded to cover several additional scenarios:
I would also like the DNS cost described in units beyond queries per second. For example:
This is not because those units are operationally meaningful, but because the current explanation has already established a high standard of numerical specificity, and consistency requires us to continue until the numbers stop helping. At a broader architectural level, this PR transfers responsibility for upstream identity from nginx startup to a periodically refreshed DNS answer. That is likely the correct mechanism, but responsibility transfers should be explicit. Previously nginx said, “I resolved this once, and therefore it shall remain true.” After this change nginx says, “I resolved this recently, and therefore it is probably still true for up to ten seconds.” This is epistemically healthier but introduces uncertainty into a system that previously expressed confidence even when wrong. We should decide whether nginx’s goal is certainty, correctness, recency, availability, or merely avoiding 502 responses long enough for a human to finish lunch. These goals overlap but are not identical. My current conclusion is that the underlying incident is real, the stale-address behavior is real, dynamic resolution is a legitimate nginx mechanism, and the implementation is conceptually reasonable. However, the change also introduces a templating pipeline, changes a public mount target, depends on image entrypoint behavior, allocates page-size-sensitive shared-memory zones, disables IPv6 resolution, introduces a ten-second temporal policy, and requires two dollar signs to communicate one dollar sign through multiple interpretive layers. Before merging, I would therefore request the following:
Until those points are addressed, I am not comfortable approving the current version. To be clear, this is not because the resolver fails to resolve the upstream. It is because resolving the upstream requires resolving the resolver, rendering the resolver into a template, preserving the variables that must not be resolved by the renderer, allocating a zone in which the resolved state may be shared, and ensuring that the mechanism used to resolve a stale address does not itself create a stale understanding of where the configuration lives. In summary: the proposed resolution may resolve the unresolved upstream, but the resolution process has not yet fully resolved the unresolved questions introduced by resolving it. Please resolve accordingly. |
|
Thanks @aminvakil for letting me in on your frustration. I apologize on behalf of my agent, my PR was lazy in form and lacked the polish that busy maintainers deserve. I am now taking the time to work through the actionable suggestions in your admittedly funny review comment and plan to come back with a better and more readable solution. I do care about getting this fixed, and I do care about maintainability and staying in control. |
Feel free to push to this branch and reopen this or create another PR, that's ok both. |
Sorry, I just realized what you meant, that review comment is a complete joke, yes, there are reasonable findings in it as well, but it's missing the big picture completely which I've stated before. You see it's just trying to fixing something broken rather than rejecting the whole idea. Ignore that comment please, that's not me. |
|
I did push mll-lab@4a8ee28, essentially a polished version of the change this PR originally proposed. Mostly just cleans up the syntax though and uses an extra file instead of a more unconventional templating approach. Feel free to ignore if you think the |

Supersedes #4295, keeping @ilias-115 as co-author. Same fix as #3894 asked for, rescoped around the objections raised there and in #4295.
Why
nginx resolves the service names inside
upstream { server ... }exactly once, at startup, and caches the answer for the life of the worker process. Docker does not guarantee a container the same address across starts. So after any restart nginx can be proxying to an address that now belongs to a different container — and it will keep doing that forever, because nothing in nginx ever looks again.The failure is quiet, which is the part that hurts.
webandrelayare separate upstreams, so the UI stays fully functional while every ingest request returns 502. We lost roughly three hours of events to this before a human noticed; no health check on our side could have caught it, because nginx itself was healthy.#3914 helps but does not close it. It restarts nginx when Compose restarts
relayorweb, which coversdocker compose restartand not much else. Our incident began after a host-level Docker restart, where nginx came up 12 s beforerelaydespitedepends_on— nginx cached the address of whatever container held it at that moment.condition: service_healthyis not available either, sincerelaydeclares no healthcheck.The DNS cost, since that was the sticking point
The concern in #3894 was "a DNS lookup every 5 seconds is not a good idea, especially under heavy load". That framing does not apply to
resolve: re-resolution is driven by a timer per upstream server, never by a request.With two upstream servers and
valid=10sthe steady state is 0.2 queries/s, total, forever — the same at 1 req/s as at 10k req/s. Each one goes to the container engine's own DNS on the same host, so there is no network hop and no external resolver involved. Requests never wait on it: nginx keeps serving from the cached address while the timer refreshes in the background.valid=10srather than 30s or 60s because it bounds the outage, not the load. The load is the same at any value; only the worst-case window during which nginx still points at a dead address changes.Where the resolver address comes from
resolveneeds aresolver, and only the container engine's own DNS knows the namesrelayandwebat all — a resolver from the host's/etc/resolv.confcannot resolve them. Its address is not the same everywhere:127.0.0.11on Docker, the network gateway on Podman, which this repo also supports.So nothing is hardcoded. The nginx image already derives the address from the container's
/etc/resolv.confin/docker-entrypoint.d/15-local-resolvers.envshand exports it asNGINX_LOCAL_RESOLVERS, but that only reaches the config throughenvsubst.nginx.confis therefore mounted at/etc/nginx/templates/nginx.conf.templateand rendered over the image default viaNGINX_ENVSUBST_OUTPUT_DIR.NGINX_ENVSUBST_FILTERrestricts substitution to that single variable, so the nginx runtime variables in the config ($remote_addr,$request_id, and the rest) are left alone.The only user-visible consequence is the bind mount target. Anyone who mounts their own
nginx.confover/etc/nginx/nginx.confkeeps working exactly as before — the template is simply not rendered for them, and they supply their ownresolver.No image bump needed
resolvein nginx OSS landed in 1.27.3, anddocker-compose.ymlcurrently pinsnginx:1.31.4-alpine, so this applies to master as-is. It is worth noting for anyone backporting: on the 1.25.4 that older release tags carry, nginx refuses to start with this config.Test plan
That the template renders to the engine's actual DNS address, that the nginx runtime variables survive
envsubst, and that the result is a valid config:On a user-defined bridge network that prints
resolver 127.0.0.11 valid=10s ipv6=off;, leaves$remote_addrand$request_iduntouched, and the config test passes. I have no Podman host to try, so the gateway case rests on15-local-resolvers.envshreading the same/etc/resolv.confnginx would have used anyway — a Podman run in CI would settle it.The version floor is real, worth noting for anyone backporting:
Then the behaviour that matters — force
relayto a new address and confirm ingest survives without touching nginx:Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.