internetarchive / internetarchive/openlibrary
deploy.sh: the `nginx -t` gate added in #13535 cannot fail
- Dominant language
- Python
- Stars
- 6.7k
- Forks
- 2k
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 138
Description
## Summary
The `nginx -t` gate added in #13535 cannot fail. It validates the **previous** ModSecurity ruleset, prints ✓, and lets a broken config through to the restart that takes the site down — the exact failure it was built to prevent (olsystem#420).
Two further holes were found while fixing it: the gate is not reached at all on the deploy path a rule fix actually takes, and it validates a config pairing that never runs in production.
## Why it cannot fail
`check_nginx_config` runs `docker compose exec` against the **live** nginx container.
A bind mount is resolved to an inode when the container starts. `deploy_olsystem` does not update `/opt/olsystem` in place — it does:
```
mv /opt/olsystem /opt/olsystem_previous
mv /opt/olsystem_new /opt/olsystem
```
The running container keeps the old inode, which is now reachable at `/opt/olsystem_previous`. So `exec ... nginx -t` parses the ruleset that was already there, passes, and the crash still arrives at restart.
The comment shipped in #13535 reasoned that directory bind mounts are safe because file replacement inside them is visible live. That is true for in-place edits and false when the whole directory is replaced by `mv`. The same comment describes that mechanism correctly for `copy_to_servers` and then fails to apply it to olsystem, one function earlier.
### Measured
Ubuntu 24.04 / Docker 29.2.0 / overlayfs, native bind mount:
| case | container reads |
|---|---|
| in-place edit of a file inside the mount | **NEW** |
| `mv`-swap (what `deploy_olsystem` does) | **OLD** — host inode 262558, orphan 262556, container 262556 |
| fresh container (`run --rm`) | **NEW** |
And `docker compose run --service-ports` fails with `Bind for 0.0.0.0:8080 failed: port is already allocated`, so the fresh-container check must never use that flag against a live host.
## Hole 2 — the gate is not reached on the path that matters
`check_nginx_config` was called only from `deploy_wizard`. The CLI dispatch calls `deploy_olsystem` bare, and `deploy_olsystem`'s own `[Next]` text advertises exactly that standalone invocation:
```
echo "time SERVER_SUFFIX='$SERVER_SUFFIX' ./scripts/deployment/deploy.sh olsystem"
```
A ModSecurity rule fix is an olsystem-only change — nobody runs the full weekly wizard to ship one rule — so the olsystem#420 shape is precisely the path with no gate. Making the gate work does not help if it is not reached.
This was raised in #13535's own "Notes for review", accepted there, and merged without it.
## Hole 3 — it validates a pairing that never runs
`docker/nginx.conf` and `docker/web_nginx.conf` bind-mount out of `/opt/openlibrary`, which is still pre-deploy when the olsystem gate runs. So the gate tests **new olsystem against old nginx.conf**, a combination that never serves traffic. Coupled changes slip through both ways:
- an openlibrary PR adding `include /olsystem/etc/nginx/thing.conf` alongside an olsystem PR adding that file — passes the gate, `[emerg]`s at restart;
- the reverse — aborts a healthy deploy.
The gate cannot be correct at a single position: at no single moment does the tree hold both the new rules and the new conf until `deploy_openlibrary` has run.
## All three nginx hosts matter
`modsecurity on` appears in `docker/web_nginx.conf` and nowhere else, so ol-www0 is the only host loading the WAF ruleset. But every nginx service mounts the shared `docker/nginx.conf`, which includes seven files out of `/olsystem` — `logging.conf`, `tagger.js`, `deny.conf`, `is_blessed_ip.conf`, `is_blessed_ua.conf`, `is_sus_ip.conf`, `ua_rate_limit_key.conf`. A bad olsystem include crash-loops `infobase_nginx` and `covers_nginx` just as readily.
## The shape of the bug
A check that reads something other than what will actually run. Worth naming because it recurred four times inside this one change — the inode comment, a coverage claim, the failure text, and a progress banner were each correct when written and false by the time they would have been read.
---
Fix in the linked PR. Nothing has been run against production.
Contributor guide
Assessment
This issue has not been assessed yet.