internetarchive / internetarchive/openlibrary

deploy.sh: the `nginx -t` gate added in #13535 cannot fail

Open
#13,619 1 comment 0 reactions 1 assignee Claimed by @mekarpeles View on GitHub
Affects: Configuration Affects: Server Lead: @cdrini Module: Docker Needs: Breakdown Needs: Staff / Internal Needs: Staff Decision Priority: 1 Type: Bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.