From b052ad1588c78c7a09177cc138c0bea3ee766b0d Mon Sep 17 00:00:00 2001 From: Colin Megill Date: Tue, 3 Nov 2026 18:44:10 -0800 Subject: [PATCH 1/2] ci: every nginx upstream must be a container the production web role runs The 2.43.0 deploy answered 502 for every static asset at the origin because nginx sent those paths to a file-server container that production web boxes never start; the hotfix sent them back through Node. Nothing would have caught the same mistake again. A new check, `make check-nginx-upstreams` (also called verbatim by a hosted workflow), parses every upstream host from the nginx templates and the rendered Rust route locations, reads the services the web role starts from the deploy hook and the Compose file, and fails naming the upstream, the config file and the hook line when an upstream names a service the web role does not run. An empty, commented exception list is the only way around it. The 2.43.0 shape fails it; edge passes. The hook reader joins continued lines, tokenizes only the container-start lines, finishes one-line helpers, and names a line in every error. The README states what it does not read: includes of files that are not .conf or .template, and shell constructs it cannot follow. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/nginx-upstreams.yml | 20 ++ Makefile | 4 + docs/local-checks.md | 1 + file-server/nginx/README.md | 28 ++ file-server/nginx/production_upstreams.py | 325 ++++++++++++++++++ .../nginx/test-production-upstreams.py | 266 ++++++++++++++ 6 files changed, 644 insertions(+) create mode 100644 .github/workflows/nginx-upstreams.yml create mode 100644 file-server/nginx/production_upstreams.py create mode 100644 file-server/nginx/test-production-upstreams.py diff --git a/.github/workflows/nginx-upstreams.yml b/.github/workflows/nginx-upstreams.yml new file mode 100644 index 000000000..f67dd64a5 --- /dev/null +++ b/.github/workflows/nginx-upstreams.yml @@ -0,0 +1,20 @@ +name: Nginx production upstreams +on: + pull_request: + paths: ['file-server/**', 'scripts/after_install.sh', 'docker-compose.yml', 'Makefile', '.github/workflows/nginx-upstreams.yml'] + push: + branches: [edge] + paths: ['file-server/**', 'scripts/after_install.sh', 'docker-compose.yml', 'Makefile', '.github/workflows/nginx-upstreams.yml'] +permissions: + contents: read +jobs: + upstreams: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: {python-version: '3.12'} + - name: Install template renderer dependency + run: sudo apt-get update && sudo apt-get install -y gettext-base + - name: Check production upstreams + run: make check-nginx-upstreams diff --git a/Makefile b/Makefile index 22dcc471f..0fbdafce5 100644 --- a/Makefile +++ b/Makefile @@ -414,3 +414,7 @@ check-delphi-characterization: ## Run delphi-characterization checks .PHONY: check-server-integration check-server-integration: ## Run server-integration checks @bash ci/local/server-integration.sh + +.PHONY: check-nginx-upstreams +check-nginx-upstreams: ## Check that production starts every nginx upstream + @python3 file-server/nginx/test-production-upstreams.py diff --git a/docs/local-checks.md b/docs/local-checks.md index 3fe482421..5c5da02a9 100644 --- a/docs/local-checks.md +++ b/docs/local-checks.md @@ -29,6 +29,7 @@ check. No command reduces the test selection or changes an assertion. | Comment recordings | `make check-post-comments-recordings` | Docker, Python 3 | | Collective statements | `make check-collective-statement-recordings` | Node 22.23.4, Python 3.12, PostgreSQL 17.11, DynamoDB Local | | Delphi characterization | `make check-delphi-characterization` | Node 22.23.4, Python 3.12, PostgreSQL 17.11, DynamoDB Local | +| Nginx production upstreams | `make check-nginx-upstreams` | Python 3.12, gettext envsubst | | Routing recordings | `make check-routing-recordings` | Node 22.23.4, PostgreSQL 17.11 | | Server integration | `make check-server-integration` | Node 22.23.4, Python 3, mkcert, Docker | | Delphi result reads | `bash scripts/test-delphi-result-reads.sh --ci` | Node 24.22.1, pinned Rust, Docker | diff --git a/file-server/nginx/README.md b/file-server/nginx/README.md index 1ff409bab..4796b124a 100644 --- a/file-server/nginx/README.md +++ b/file-server/nginx/README.md @@ -122,3 +122,31 @@ when that cached image predates them. `NGINX_RENDERED_DIR` optionally saves `nginx -T` output for the tested routes-off and routes-on selections. The harness owns a unique container/network prefix and uses ephemeral loopback ports. `X54_NGINX_PREFIX` can override that prefix. + +### Production upstream check + +Run `make check-nginx-upstreams` from the repository root (Python 3.12+ and +`envsubst`, provided by gettext, are required). The Nginx production upstreams +workflow runs the same command for changes to the proxy, Compose file or +production startup hook. No Docker daemon, database or environment file is used. + +The check reads the web role's image list and Compose startup commands in +`scripts/after_install.sh`, compares them with `docker-compose.yml` service +names and dependencies, then reads every `.template` and `.conf` file below +the nginx directory. Includes are not followed; included files with other +extensions are outside coverage. It also runs `render-rust-api.sh` with all route templates selected, using the +Compose default Rust upstream. Only the renderer's container-local template +path is relocated to a temporary directory. Literal proxy hosts, upstream +pool members and host variables defined by `set` or `map` must name a service +the web role starts. Exceptions belong in `production_upstreams.py` with an +explanation; the list is currently empty. + +This is a source topology check, not a live readiness probe: conditional Rust +startup counts as available when its routes are selected. Deployment-specific +environment overrides, DNS reachability and container health are outside its +scope. The shell and Compose readers support the current explicit startup +and short dependency-list forms, not general shell evaluation or arbitrary +YAML. Unsupported startup arguments, dependencies and host variables fail; +changes to those source forms need matching reader tests. The deployment hook +is never executed. The test includes the short-lived migration invocation in +its startup review but does not count it as a serving upstream. diff --git a/file-server/nginx/production_upstreams.py b/file-server/nginx/production_upstreams.py new file mode 100644 index 000000000..e80ae3138 --- /dev/null +++ b/file-server/nginx/production_upstreams.py @@ -0,0 +1,325 @@ +"""Read the checked-in production topology; never execute the deployment hook. + +This deliberately accepts the shell/Compose forms used by after_install.sh, +not arbitrary shell or YAML. Changes to those forms must extend this reader. +Nginx variables used as hosts must resolve through a literal set or map; +unknown host expressions are errors, not omitted upstreams. +Only .conf/.template files below the nginx directory are collected. Includes +are not followed: an included file with another extension is outside coverage. +Other *_pass directives are rejected until their address syntax is supported. +The shell reader handles the current line-oriented conditionals and function +definitions; it does not execute shell or trace calls into helper functions. +Startup moved into a helper or a new shell form needs a matching reader change. +""" +import os +from pathlib import Path +import re +import shlex +import subprocess +import tempfile + +# No external upstreams are currently needed. Any future exception must name +# the host here and explain why production can reach it without a Compose service. +EXCEPTIONS = {} + + +def require(condition, message): + if not condition: + raise ValueError(message) + + +def logical_lines(script): + """Join shell continuations, retaining the first physical line number.""" + pending, start = '', None + for number, line in enumerate(script.splitlines(), 1): + if start is None: + start = number + # An odd trailing backslash count escapes the newline. + trailing = len(line) - len(line.rstrip('\\')) + if trailing % 2: + pending += line[:-1] + continue + yield start, pending + line + pending, start = '', None + require(start is None, f'incomplete shell continuation at line {start}: {pending}') + + +def web_lines(script): + """Yield possible web-role lines, including shared top-level commands.""" + active, conditions, function = True, [], None + for number, line in logical_lines(script): + stripped = line.strip() + if function is not None: + if re.fullmatch(r'}\s*(?:#.*)?', stripped): + function = None + continue + if re.match(r'^[a-zA-Z_][a-zA-Z_0-9]*\(\)\s*\{', stripped): + if not re.search(r';\s*}\s*(?:#.*)?$', stripped): + function = (number, stripped) + continue + if not stripped or stripped.startswith('#'): + continue + keyword = stripped.split(None, 1)[0] + if keyword in {'if', 'elif'}: + condition = line.split('; then', 1)[0] + role = re.search(r'"\$SERVICE_FROM_FILE"\s+(==|=|!=)\s+"([a-z-]+)"', condition) + require('$SERVICE_FROM_FILE' not in condition or role is not None, + f'unrecognized role condition at line {number}: {stripped}') + # Other conditions can take either arm; include both possibilities. + matches = None if role is None else ((role[2] == 'server') != (role[1] == '!=')) + if keyword == 'if': + parent, previous, opening = active, False, (number, stripped) + else: + require(conditions, f'elif without if at line {number}: {stripped}') + parent, previous, opening = conditions.pop() + current = parent and not previous and matches is not False + if re.search(r'\bfi\s*(?:;|$)', stripped): + if current: + yield number, line + continue + conditions.append((parent, previous or matches is True, opening)) + active = current + elif keyword.rstrip(';') == 'else': + require(conditions, f'else without if at line {number}: {stripped}') + parent, previous, _ = conditions[-1] + active = parent and not previous + elif keyword.rstrip(';') == 'fi': + require(conditions, f'fi without if at line {number}: {stripped}') + active, _, _ = conditions.pop() + if active: + yield number, line + if function is not None: + number, line = function + raise ValueError(f'incomplete shell function at line {number}: {line}') + if conditions: + number, line = conditions[-1][2] + raise ValueError(f'incomplete shell conditional at line {number}: {line}') + + +def foreground_run(command): + """Recognize disposable foreground runs; never count them as services.""" + if not command or command[0] != 'run': + return False + arguments = iter(command[1:]) + disposable = False + takes_value = {'--network', '--env-file', '--stop-timeout', '--name', '--entrypoint', + '--user', '-u', '--workdir', '-w', '--volume', '-v', '--mount', '--env', '-e', + '--pull', '--log-driver', '--log-opt', '--platform', '--add-host', '--label'} + for word in arguments: + if word in {'--rm', '--rm=true'}: + disposable = True + elif word in {'--init', '-i', '--interactive', '-t', '--tty', '--detach=false'}: + continue + elif word in takes_value: + if next(arguments, None) is None: + return False + elif '=' in word and word.split('=', 1)[0] in takes_value: + continue + elif word.startswith('-') or word in {';', '&&', '||'}: + return False + else: + # The first non-option is the image; later words are its command, + # not Docker options (e.g. an application's own --rm flag). + return disposable + return False + + +def web_services(script, compose): + # Inspect the image case and actual startup separately: pulling an image + # alone does not mean a service runs. Rust is conditional on route selection. + images = re.findall(r'^\s*server\) ROLE_IMAGES="([a-z0-9 -]+)" ;;$', script, re.M) + require(len(images) == 1, 'unrecognized web ROLE_IMAGES assignment') + images = set(images[0].split()) + started = set() + for number, line in web_lines(script): + # Unrelated shell text (including multi-line messages) needs no lexer. + if not re.search(r'\bdocker\b', line) and '${COMPOSE[@]}' not in line: + continue + location = f'line {number}: {line.strip()}' + try: + lexer = shlex.shlex(line, posix=True, punctuation_chars=';&|') + lexer.whitespace_split = True + words = list(lexer) + except ValueError as error: + raise ValueError(f'shell tokenization failed at {location}: {error}') from error + for index, word in enumerate(words): + if word != 'docker': + continue + command = words[index + 1:] + if command[:1] == ['container']: + command = command[1:] + if command[:1] in (['run'], ['start']): + require(foreground_run(command), f'unrecognized direct container startup at {location}') + if '${COMPOSE[@]}' not in words: + continue + command = words[words.index('${COMPOSE[@]}') + 1:] + if command in (['--profile', 'rust-api', 'down'], ['config', '--quiet']): + continue + require(command and command[0] == 'up', f'unrecognized web Compose command at {location}') + for word in command[1:]: + if word in {'-d', '--no-deps', '--no-build', '--force-recreate', 'then', ';'}: + continue + require(re.fullmatch(r'[a-z][a-z0-9-]*', word), f'unrecognized startup argument {word!r} at {location}') + started.add(word) + require(started == images, f'web image/start mismatch: images={sorted(images)}, started={sorted(started)}') + # Service names are two-space keys under services, not similarly indented + # networks/volumes. Reject alternative formatting rather than infer a name. + section = re.search(r'^services:\n(.*?)(?=^[^\s#]|\Z)', compose, re.M | re.S) + require(section is not None, 'missing Compose services block') + services = dict(re.findall(r'^ ([a-z][a-z0-9-]*):\n(.*?)(?=^ [^\s#]|\Z)', section[1], re.M | re.S)) + require(started <= services.keys(), f'web services absent from Compose: {sorted(started - services.keys())}') + # Current dependencies use short list syntax. Every dependency of a web + # service must already be in the explicit start list (no implicit additions). + for name in started: + dependencies = re.findall(r'^ depends_on:\n((?:^ .*\n)+)', services[name], re.M) + require(len(dependencies) == len(re.findall(r'^ depends_on:', services[name], re.M)), + f'unrecognized Compose dependency syntax for {name}') + for deps in dependencies: + names = set() + for line in deps.splitlines(): + match = re.fullmatch(r' - ["\']?([a-z][a-z0-9-]*)["\']?', line) + require(match is not None, f'unrecognized Compose dependency for {name}: {line}') + names.add(match[1]) + require(names <= started, f'Compose dependencies not explicitly started for {name}: {sorted(names - started)}') + return started + + +def directives(text): + # Braces inside ${variables} and quoted tokens are not block delimiters. + pattern = r'''\s+|\#[^\n]*|"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[{};]|(?:\$\{[^}]+\}|\\.|[^\s{};"'\#])+''' + tokens = [] + pos = 0 + for match in re.finditer(pattern, text): + require(match.start() == pos, 'unrecognized nginx token') + pos = match.end() + token = match[0] + if token.isspace() or token.startswith('#'): + continue + if token[:1] in {'"', "'"}: + token = token[1:-1] + tokens.append(token) + require(pos == len(text), 'unrecognized nginx trailing text') + stack, words, result = [], [], [] + for token in tokens: + if token == '{': + require(words, 'nginx block without header') + stack.append(tuple(words)) + words = [] + elif token == '}': + require(stack and not words, 'unbalanced nginx block') + stack.pop() + elif token == ';': + require(words, 'empty nginx directive') + result.append((tuple(stack), words)) + words = [] + else: + words.append(token) + require(not stack and not words, 'incomplete nginx configuration') + return result + + +def nginx_hosts(documents, sources=None): + """Resolve hosts, optionally recording config files along each reference.""" + rows = [(name, *row) for name, text in documents.items() for row in directives(text)] + variables, pools, targets = {}, {}, [] + for name, stack, words in rows: + if stack and stack[-1][0] == 'upstream': + pool = stack[-1][1] + pools.setdefault(pool, []) + if words[0] == 'server': + require(len(words) >= 2, 'upstream server without address') + pools[pool].append((words[1], name)) + targets.append((words[1], name)) + if stack and stack[-1][0] == 'map': + variable = stack[-1][-1] + if words[0] not in {'hostnames', 'volatile', 'include'}: + require(len(words) == 2, 'unrecognized nginx map value') + variables.setdefault(variable, set()).add((words[1], name)) + if words[0] == 'set': + require(len(words) == 3, 'unrecognized nginx set') + variables.setdefault(words[1], set()).add((words[2], name)) + require(not words[0].endswith('_pass') or words[0] == 'proxy_pass', + f'unsupported nginx upstream directive: {words[0]}') + if words[0] == 'proxy_pass': + require(len(words) == 2 and re.match(r'https?://', words[1]), 'unrecognized proxy_pass') + targets.append((re.sub(r'^https?://', '', words[1]), name)) + + def resolve(value, origins, seen=frozenset()): + if not value: + return set() # Empty map default selects the Node fallback. + # Request suffixes after a resolved host do not determine its identity. + value = re.split(r'[/ ?]', value, maxsplit=1)[0] + match = re.match(r'^\$(?:\{(\w+)\}|(\w+))', value) + if match: + key = '$' + (match[1] or match[2]) + suffix = value[match.end():] + require(not suffix or suffix in {'$request_uri', '$uri'} or re.fullmatch(r':\d+(?:\$request_uri|\$uri)?', suffix), + f'unrecognized variable host suffix: {value}') + require(key in variables and key not in seen, f'unresolved nginx host: {key}') + return set().union(*(resolve(v, origins | {name}, seen | {key}) for v, name in variables[key])) + # Ports may be rendered defaults; unresolved variables anywhere in the + # authority are errors. Unix sockets require an explicit implementation. + require(re.fullmatch(r'[A-Za-z0-9._-]+(?::\d+)?', value), f'unresolved nginx address: {value}') + host = value.split(':')[0] + if host in pools: + require(host not in seen and pools[host], f'empty or circular upstream: {host}') + return set().union(*(resolve(v, origins | {name}, seen | {host}) for v, name in pools[host])) + if sources is not None: + sources.setdefault(host, set()).update(origins) + return {host} + + return set().union(*(resolve(value, {name}) for value, name in targets)) + + +def check(root): + root = Path(root) + services = web_services((root / 'scripts/after_install.sh').read_text(), + (root / 'docker-compose.yml').read_text()) + folder = root / 'file-server/nginx' + paths = sorted(folder.rglob('*.template')) + sorted(folder.rglob('*.conf')) + require(paths, 'no nginx templates found') + defaults = {'API_SERVER_PORT': '5000', 'STATIC_FILES_PORT': '8080'} + upstreams = re.findall(r'^\s*- RUST_API_UPSTREAM=\$\{RUST_API_UPSTREAM:-([^}]+)\}$', + (root / 'docker-compose.yml').read_text(), re.M) + require(len(upstreams) == 1, 'unrecognized Compose Rust upstream default') + documents = {} + for path in paths: + if path == folder / 'rust-api/upstreams.conf.template': + continue # Its map placeholders are checked after the real renderer. + value = path.read_text() + for key, default in defaults.items(): + value = value.replace('${' + key + '}', default) + documents[str(path.relative_to(root))] = value + # Select every route template, which includes the representative Node + # fallback, simple API, regex API and page forms, and future route templates. + routes = sorted(p.name.removesuffix('.location.conf.template') + for p in (folder / 'rust-api').glob('*.location.conf.template') + if p.name != 'node.location.conf.template') + require(routes, 'no Rust route templates found') + with tempfile.TemporaryDirectory(prefix='nginx-upstreams-') as temporary: + temp = Path(temporary) + # Only relocate the renderer's container-local template directory. + renderer = (folder / 'render-rust-api.sh').read_text() + require('/etc/nginx/rust-api-templates/' in renderer, 'renderer template path changed') + renderer = renderer.replace('/etc/nginx/rust-api-templates/', str(folder / 'rust-api') + '/') + script = temp / 'render.sh' + script.write_text(renderer) + env = {'PATH': os.environ['PATH'], 'RUST_API_ROUTES': ','.join(routes), + 'RUST_API_UPSTREAM': upstreams[0], **defaults} + result = subprocess.run(['sh', str(script), str(temp / 'rendered')], env=env, + capture_output=True, text=True) + require(result.returncode == 0, + f'Rust route renderer failed (exit {result.returncode}): {result.stderr.strip()}') + for path in sorted((temp / 'rendered').glob('*.conf')): + require(path.stat().st_size > 0, f'empty rendered file: {path.name}') + documents['rendered/' + path.name] = path.read_text() + sources = {} + hosts = nginx_hosts(documents, sources) + require(hosts, 'no nginx upstream hosts found') + for host, reason in EXCEPTIONS.items(): + require(reason.strip(), f'exception needs an explanation: {host}') + missing = hosts - services - EXCEPTIONS.keys() + details = {host: sorted(sources[host]) for host in sorted(missing)} + require(not missing, f'nginx upstreams absent from production web role: {sorted(missing)}; configs: {details}') + return {'hosts': sorted(hosts), 'web_services': sorted(services), + 'source_configs': len(paths), 'rendered_routes': len(routes)} diff --git a/file-server/nginx/test-production-upstreams.py b/file-server/nginx/test-production-upstreams.py new file mode 100644 index 000000000..8ac36a584 --- /dev/null +++ b/file-server/nginx/test-production-upstreams.py @@ -0,0 +1,266 @@ +#!/usr/bin/env python3 +"""Production upstream availability, without starting containers or reading .env.""" +from pathlib import Path +import unittest +import tempfile + +from production_upstreams import check, nginx_hosts, web_services + +ROOT = Path(__file__).resolve().parents[2] + +# Small inputs keep parser tests independent of deployment-file formatting. +HOOK = ''' +case "$SERVICE_FROM_FILE" in + server) ROLE_IMAGES="server nginx-proxy polis-api" ;; +esac +sudo docker run --rm --network host --env-file .env polis-migrate:deploy deploy +worker_daemon() { + sudo docker run -d worker-image +} +if [ "$SERVICE_FROM_FILE" == "server" ]; then + "${COMPOSE[@]}" up -d server nginx-proxy --no-build + if [ "$POLIS_API_START" = 1 ]; then + "${COMPOSE[@]}" up -d --no-deps --no-build --force-recreate polis-api + fi +elif [ "$SERVICE_FROM_FILE" == "delphi" ]; then + sudo docker run -d delphi-image +fi +''' +COMPOSE = '''services: + server: + image: node + nginx-proxy: + depends_on: + - "server" + environment: + - RUST_API_UPSTREAM=${RUST_API_UPSTREAM:-polis-api:5100} + polis-api: + image: rust +''' + + +class ProductionUpstreams(unittest.TestCase): + def test_repository(self): + result = check(ROOT) + self.assertTrue(result['hosts']) + print(result, flush=True) + + def test_proxy_forms(self): + docs = {'example.conf': ''' + # proxy_pass http://ignored:80; + upstream pool { server server:5000; server client-participation-alpha:4321 backup; } + map $uri $target { default ""; /api polis-api:5100; } + set $node server:5000; + location / { proxy_pass "http://pool"; } + location /api { proxy_pass http://$target$request_uri; } + location /node { proxy_pass http://$node; } + '''} + self.assertEqual(nginx_hosts(docs), {'server', 'client-participation-alpha', 'polis-api'}) + + def test_unresolved_hosts_are_errors(self): + for value in ['http://$unknown', 'http://${UNKNOWN}:80', 'http://unix:/tmp/app.sock:']: + with self.subTest(value=value), self.assertRaises(ValueError): + nginx_hosts({'example.conf': f'proxy_pass {value};'}) + + def test_every_pool_member_is_checked(self): + self.assertEqual(nginx_hosts({'a': 'upstream unused { server absent:80; }'}), {'absent'}) + self.assertEqual(nginx_hosts({'a': 'proxy_pass https://outside.example:443/path;'}), {'outside.example'}) + + def replace_once(self, text, old, new): + self.assertEqual(text.count(old), 1, f'mutation needs exactly one occurrence: {old!r}') + return text.replace(old, new, 1) + + def fixture(self, root): + (root / 'scripts').mkdir() + (root / 'scripts/after_install.sh').write_text(HOOK) + (root / 'docker-compose.yml').write_text(COMPOSE) + folder = root / 'file-server/nginx' + (folder / 'rust-api').mkdir(parents=True) + (folder / 'rust-api/example.location.conf.template').write_text( + 'location /example { proxy_pass http://polis-api:5100; }') + (folder / 'render-rust-api.sh').write_text('''#!/bin/sh +# /etc/nginx/rust-api-templates/ +mkdir -p "$1" +printf 'location /example { proxy_pass http://%s; }' "$RUST_API_UPSTREAM" > "$1/example.conf" +''') + + def test_images_are_not_startup(self): + self.assertEqual(web_services(HOOK, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + changed = self.replace_once(HOOK, 'up -d --no-deps --no-build --force-recreate polis-api', + 'up -d --no-deps --no-build --force-recreate server') + with self.assertRaisesRegex(ValueError, 'image/start'): + web_services(changed, COMPOSE) + changed = self.replace_once(COMPOSE, ' polis-api:', ' absent-service:') + with self.assertRaisesRegex(ValueError, 'Compose'): + web_services(HOOK, changed) + + def test_changed_startup_syntax_is_not_silently_ignored(self): + changed = self.replace_once(HOOK, 'up -d server nginx-proxy --no-build', 'up -d $WEB_SERVICES') + with self.assertRaisesRegex(ValueError, r'line .*\$WEB_SERVICES'): + web_services(changed, COMPOSE) + + def test_compose_host_default_is_checked(self): + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + self.fixture(root) + changed = self.replace_once(COMPOSE, '${RUST_API_UPSTREAM:-polis-api:5100}', + '${RUST_API_UPSTREAM:-file-server:8080}') + (root / 'docker-compose.yml').write_text(changed) + with self.assertRaisesRegex(ValueError, 'file-server'): + check(root) + + def test_new_dependency_syntax_requires_review(self): + changed = self.replace_once(COMPOSE, 'depends_on:\n - "server"', 'depends_on: [file-server]') + with self.assertRaisesRegex(ValueError, 'dependency'): + web_services(HOOK, changed) + + def test_flags_can_precede_service_names(self): + changed = self.replace_once(HOOK, 'up -d server nginx-proxy --no-build', + 'up --no-build -d server nginx-proxy') + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_foreground_run_options_can_change(self): + for command in [ + 'docker run --rm --stop-timeout 30 --network host other-image deploy', + 'docker container run --network=host --rm=true other-image deploy', + ]: + with self.subTest(command=command): + changed = self.replace_once(HOOK, + 'docker run --rm --network host --env-file .env polis-migrate:deploy deploy', command) + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_continued_shell_commands(self): + for old, new in [ + ('--network host --env-file', '--network host \\\n --env-file'), + ('up -d server nginx-proxy --no-build', 'up -d server \\\n nginx-proxy --no-build'), + ]: + with self.subTest(old=old): + changed = self.replace_once(HOOK, old, new) + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + login = 'aws ecr get-login-password --region us-east-1 \\\n | sudo docker login --username AWS --password-stdin "$REGISTRY"\n' + self.assertEqual(web_services(login + HOOK, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_multiline_message_does_not_need_shell_tokenization(self): + message = 'echo "Migration applied.\n Starting the role\'s services next."\n' + self.assertEqual(web_services(HOOK + message, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_single_line_helper_is_complete(self): + for definition in [ + 'restart_unit() { sudo systemctl restart --no-block "$1"; }', + 'image_ref() { printf "%s" "$1"; } # image lookup', + 'start_worker() { docker run -d worker-image; }', + ]: + with self.subTest(definition=definition): + changed = definition + '\n' + HOOK + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_shared_start_after_single_line_helper_is_not_skipped(self): + changed = 'image_ref() { printf "%s" "$1"; }\n' + changed += 'sudo docker run -d file-server-image\n' + HOOK + with self.assertRaisesRegex(ValueError, 'line 2: sudo docker run -d file-server-image'): + web_services(changed, COMPOSE) + + def test_shell_errors_name_the_opening_line(self): + for suffix, message in [ + ('docker run --rm "unfinished\n', 'No closing quotation'), + ('"${COMPOSE[@]}" up "unfinished\n', 'No closing quotation'), + ('helper() {\n echo unfinished\n', 'incomplete shell function'), + ('if [ "$OPTIONAL" = 1 ]; then\n echo unfinished\n', 'incomplete shell conditional'), + ('docker run --rm \\', 'incomplete shell continuation'), + ]: + with self.subTest(suffix=suffix): + with self.assertRaises(ValueError) as caught: + web_services(HOOK + suffix, COMPOSE) + self.assertIn(message, str(caught.exception)) + self.assertIn(f'line {len(HOOK.splitlines()) + 1}:', str(caught.exception)) + changed = 'sudo docker run \\\n -d image\n' + HOOK + with self.assertRaisesRegex(ValueError, r'line 1: sudo docker run\s+-d image'): + web_services(changed, COMPOSE) + + def test_common_foreground_options(self): + for option in ['--pull never', '--log-driver none', '--log-opt max-size=10m', + '--platform linux/arm64', '--add-host example:127.0.0.1', '--label role=migration']: + with self.subTest(option=option): + changed = self.replace_once(HOOK, 'run --rm --network', f'run --rm {option} --network') + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_host_sources_follow_maps_and_pools(self): + docs = { + 'pool.conf': 'upstream pool { server missing-service:8080; }', + 'map.conf': 'map $uri $target { default ""; /api pool; }', + 'site.conf': 'proxy_pass http://$target;', + 'other.conf': 'proxy_pass http://server:5000;', + } + sources = {} + self.assertEqual(nginx_hosts(docs, sources), {'missing-service', 'server'}) + self.assertEqual(sources['missing-service'], {'pool.conf', 'map.conf', 'site.conf'}) + self.assertEqual(sources['server'], {'other.conf'}) + + def test_missing_host_names_the_config(self): + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + self.fixture(root) + folder = root / 'file-server/nginx' + (folder / 'extra.conf').write_text('proxy_pass http://missing-service:8080;') + with self.assertRaises(ValueError) as caught: + check(root) + self.assertIn('missing-service', str(caught.exception)) + self.assertIn('file-server/nginx/extra.conf', str(caught.exception)) + + def test_other_roles_and_function_definitions_do_not_start_web_services(self): + changed = self.replace_once(HOOK, ' sudo docker run -d delphi-image', + ' sudo docker start delphi-image') + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_direct_serving_start_names_the_line(self): + for command in ['sudo docker run -d image', 'docker container start image', + 'docker run --rm -d image', 'docker run --rm --detach=true image', + 'docker run image --rm', 'docker run --rm --unknown image', + 'docker run --rm image; docker run -d serving-image']: + for position in ['shared', 'web']: + with self.subTest(command=command, position=position): + changed = (command + '\n' + HOOK if position == 'shared' else + self.replace_once(HOOK, ' "${COMPOSE[@]}" up -d server', + command + '\n "${COMPOSE[@]}" up -d server')) + with self.assertRaises(ValueError) as caught: + web_services(changed, COMPOSE) + self.assertIn(command, str(caught.exception)) + self.assertRegex(str(caught.exception), r'line \d+:') + + def test_nested_and_alternative_role_branches(self): + for condition, selected in [('== "server"', True), ('!= "server"', False), + ('== "delphi"', False)]: + extra = f'''if [ "$SERVICE_FROM_FILE" {condition} ]; then + if [ "$OPTIONAL" = 1 ]; then + docker start unexpected-service + fi +else + if [ "$OPTIONAL" = 1 ]; then echo "optional"; fi +fi +''' + with self.subTest(condition=condition): + if selected: + with self.assertRaisesRegex(ValueError, 'docker start unexpected-service'): + web_services(HOOK + extra, COMPOSE) + else: + self.assertEqual(web_services(HOOK + extra, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + with self.assertRaisesRegex(ValueError, 'docker start shared-service'): + web_services(HOOK + extra + 'docker start shared-service\n', COMPOSE) + + def test_renderer_error_includes_stderr(self): + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + self.fixture(root) + (root / 'file-server/nginx/render-rust-api.sh').write_text( + '# /etc/nginx/rust-api-templates/\necho "unknown route(s): example" >&2\nexit 1\n') + with self.assertRaisesRegex(ValueError, r'unknown route\(s\): example'): + check(root) + + def test_other_pass_directives_require_an_explicit_reader(self): + for name in ['grpc_pass', 'uwsgi_pass', 'fastcgi_pass', 'scgi_pass', 'memcached_pass']: + with self.subTest(name=name), self.assertRaisesRegex(ValueError, name): + nginx_hosts({'example.conf': f'{name} file-server:8080;'}) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 4a6635d26289e408aaf03a7cab89c152c7453984 Mon Sep 17 00:00:00 2001 From: Colin Megill Date: Wed, 4 Nov 2026 09:32:11 -0800 Subject: [PATCH 2/2] ci: the nginx upstream check's hook reader follows compose commands, continued lines and one-line helpers The reader matched compose only as a bare docker-compose or docker compose followed by config, so the hook's own /usr/local/bin/docker-compose start was neither refused nor counted, while plugin-style forms (a COMPOSE array, a hook moved to docker compose, command -v) were refused. It now matches the executable's basename, skips the global options before the subcommand, and accepts config or no subcommand. It also stops a comment's trailing backslash from swallowing the next line, closes one-line helper functions (including those ending in "& }"), refuses direct docker compose starts, reviews docker by absolute path, parses spaced and function-keyword declarations, and names the line in every error, including the ROLE_IMAGES one. Six fixture groups cover these; the 2.43.0 shape still fails naming file-server and its template. Co-Authored-By: Claude Fable 5.1 --- file-server/nginx/production_upstreams.py | 74 ++++++++- .../nginx/test-production-upstreams.py | 142 ++++++++++++++++++ 2 files changed, 208 insertions(+), 8 deletions(-) diff --git a/file-server/nginx/production_upstreams.py b/file-server/nginx/production_upstreams.py index e80ae3138..6b58438af 100644 --- a/file-server/nginx/production_upstreams.py +++ b/file-server/nginx/production_upstreams.py @@ -30,13 +30,37 @@ def require(condition, message): def logical_lines(script): """Join shell continuations, retaining the first physical line number.""" - pending, start = '', None + pending, start, quote = '', None, None for number, line in enumerate(script.splitlines(), 1): if start is None: start = number - # An odd trailing backslash count escapes the newline. + # A shell comment begins at an unquoted word boundary. Track quotes + # across physical lines without tokenizing unrelated shell messages. + boundary, comment, index = True, False, 0 + while index < len(line): + char = line[index] + if quote == "'": + if char == "'": + quote = None + elif char == '\\': + index += 1 + boundary = False + elif quote == '"': + if char == '"': + quote = None + elif char in {'"', "'"}: + quote = char + boundary = False + elif char == '#' and boundary: + comment = True + break + else: + boundary = char.isspace() or char in ';|&()' + index += 1 + # An odd trailing backslash count escapes the newline, except in a + # comment: its newline ends the comment even after a backslash. trailing = len(line) - len(line.rstrip('\\')) - if trailing % 2: + if trailing % 2 and not comment and quote != "'": pending += line[:-1] continue yield start, pending + line @@ -53,8 +77,9 @@ def web_lines(script): if re.fullmatch(r'}\s*(?:#.*)?', stripped): function = None continue - if re.match(r'^[a-zA-Z_][a-zA-Z_0-9]*\(\)\s*\{', stripped): - if not re.search(r';\s*}\s*(?:#.*)?$', stripped): + if re.match(r'^(?:function\s+[a-zA-Z_][a-zA-Z_0-9]*(?:\s*\(\))?' + r'|[a-zA-Z_][a-zA-Z_0-9]*\s*\(\))\s*\{', stripped): + if not re.search(r'[;&]\s*}\s*(?:#.*)?$', stripped): function = (number, stripped) continue if not stripped or stripped.startswith('#'): @@ -128,7 +153,13 @@ def web_services(script, compose): # Inspect the image case and actual startup separately: pulling an image # alone does not mean a service runs. Rust is conditional on route selection. images = re.findall(r'^\s*server\) ROLE_IMAGES="([a-z0-9 -]+)" ;;$', script, re.M) - require(len(images) == 1, 'unrecognized web ROLE_IMAGES assignment') + if len(images) != 1: + candidates = [f'line {number}: {line.strip()}' + for number, line in enumerate(script.splitlines(), 1) + if re.match(r'^\s*server\)', line)] + location = ('at ' + '; '.join(candidates) if candidates + else '(no server) case line found)') + raise ValueError(f'unrecognized web ROLE_IMAGES assignment {location}') images = set(images[0].split()) started = set() for number, line in web_lines(script): @@ -137,15 +168,42 @@ def web_services(script, compose): continue location = f'line {number}: {line.strip()}' try: - lexer = shlex.shlex(line, posix=True, punctuation_chars=';&|') + lexer = shlex.shlex(line, posix=True, punctuation_chars='();&|') lexer.whitespace_split = True words = list(lexer) except ValueError as error: raise ValueError(f'shell tokenization failed at {location}: {error}') from error for index, word in enumerate(words): + word = os.path.basename(word) + command = words[index + 1:] + # Finding a binary is not an invocation. Continue scanning so a + # later invocation on this same line is still checked. + if index >= 2 and words[index - 2:index] == ['command', '-v']: + continue + if word == 'docker-compose' or (word == 'docker' and command[:1] == ['compose']): + if word == 'docker': + command = command[1:] + # Array definitions and substitutions terminate at shell + # punctuation; global options precede the Compose subcommand. + stop = next((i for i, token in enumerate(command) + if token and all(c in '();&|' for c in token)), len(command)) + command = command[:stop] + options = {'-f', '--file', '--profile', '--env-file', '-p', + '--project-name', '--project-directory'} + while command: + if command[0] in options: + require(len(command) >= 2, + f'missing Compose option value at {location}') + command = command[2:] + elif '=' in command[0] and command[0].split('=', 1)[0] in options: + command = command[1:] + else: + break + require(command[:1] in ([], ['config']), + f'unrecognized direct Compose command at {location}; use ${{COMPOSE[@]}}') + continue if word != 'docker': continue - command = words[index + 1:] if command[:1] == ['container']: command = command[1:] if command[:1] in (['run'], ['start']): diff --git a/file-server/nginx/test-production-upstreams.py b/file-server/nginx/test-production-upstreams.py index 8ac36a584..7011ed6f4 100644 --- a/file-server/nginx/test-production-upstreams.py +++ b/file-server/nginx/test-production-upstreams.py @@ -184,6 +184,148 @@ def test_common_foreground_options(self): changed = self.replace_once(HOOK, 'run --rm --network', f'run --rm {option} --network') self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + def test_comment_backslash_does_not_continue_to_startup(self): + for comment in ['# previous form: docker run --rm \\', ' # continued example \\']: + with self.subTest(comment=comment): + self.assertEqual(web_services(HOOK + comment, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + changed = comment + '\nsudo docker run -d file-server-image\n' + HOOK + with self.assertRaisesRegex(ValueError, 'line 2: sudo docker run -d file-server-image'): + web_services(changed, COMPOSE) + + def test_background_single_line_helper_does_not_hide_startup(self): + for definition in ['warm_cache() { sudo systemctl start polis-cache & }', + 'warm_cache() { sudo systemctl start polis-cache & } # helper']: + with self.subTest(definition=definition): + self.assertEqual(web_services(definition + '\n' + HOOK, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + changed = definition + '\nsudo docker run -d file-server-image\n' + HOOK + with self.assertRaisesRegex(ValueError, 'line 2: sudo docker run -d file-server-image'): + web_services(changed, COMPOSE) + + def test_direct_compose_commands_require_the_shared_array(self): + for executable in ['sudo docker compose', 'docker-compose']: + for command in ['up -d file-server', '--profile static up -d file-server', + 'start file-server', 'down']: + for position in ['shared', 'web']: + with self.subTest(executable=executable, command=command, position=position): + line = f'{executable} {command}' + changed = (line + '\n' + HOOK if position == 'shared' else + self.replace_once(HOOK, ' "${COMPOSE[@]}" up -d server', + line + '\n "${COMPOSE[@]}" up -d server')) + with self.assertRaises(ValueError) as caught: + web_services(changed, COMPOSE) + number = changed.splitlines().index(line) + 1 + self.assertIn(f'line {number}: {line}', str(caught.exception)) + self.assertIn('direct Compose command', str(caught.exception)) + for command in ['config', 'config --quiet']: + with self.subTest(executable=executable, command=command): + self.assertEqual(web_services(f'{executable} {command}\n' + HOOK, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + changed = self.replace_once(HOOK, ' sudo docker run -d delphi-image', + f' {executable} up -d delphi-image') + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + + def test_compose_binary_paths_are_checked(self): + for executable in ['sudo /usr/local/bin/docker-compose', + '/opt/bin/docker-compose', 'sudo /usr/bin/docker compose']: + for options in ['', '-f a.yml -f b.yml ', '--profile static ']: + with self.subTest(executable=executable, options=options): + line = f'{executable} {options}up -d file-server' + with self.assertRaisesRegex(ValueError, 'direct Compose command at line 1:'): + web_services(line + '\n' + HOOK, COMPOSE) + changed = self.replace_once(HOOK, ' sudo docker run -d delphi-image', line) + self.assertEqual(web_services(changed, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + + def test_compose_global_options_before_subcommand(self): + for executable in ['sudo /usr/local/bin/docker-compose', 'sudo docker compose']: + for option in ['-f a.yml', '--file a.yml', '--profile rust-api', + '--env-file test.env', '-p project', '--project-name project', + '--project-directory /tmp/example', '--profile=rust-api']: + with self.subTest(executable=executable, option=option): + config = f'X=$({executable} {option} config --format json | jq .)\n' + self.assertEqual(web_services(config + HOOK, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + with self.assertRaisesRegex(ValueError, 'direct Compose command at line 1:'): + web_services(f'{executable} {option} up -d file-server\n' + HOOK, COMPOSE) + for option in ['-f', '--profile', '--env-file', '-p', '--project-directory']: + with self.subTest(executable=executable, missing_value=option): + with self.assertRaisesRegex(ValueError, 'Compose option.*line 1:'): + web_services(f'COMPOSE=({executable} {option})\n' + HOOK, COMPOSE) + + def test_compose_array_definitions(self): + for executable in ['sudo /usr/local/bin/docker-compose', 'sudo docker compose', + 'sudo /usr/bin/docker compose']: + for options in ['', ' -f a.yml -f b.yml', ' --profile rust-api -p example']: + with self.subTest(executable=executable, options=options): + self.assertEqual(web_services(f'COMPOSE=({executable}{options})\n' + HOOK, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + with self.assertRaisesRegex(ValueError, 'direct Compose command at line 1:'): + web_services(f'COMPOSE=({executable}{options}); {executable} up -d file-server\n' + HOOK, COMPOSE) + + def test_compose_binary_lookup(self): + for executable in ['docker-compose', '/usr/local/bin/docker-compose']: + for suffix in [' >/dev/null || echo "not installed" >&2', '']: + with self.subTest(executable=executable, suffix=suffix): + self.assertEqual(web_services(f'command -v {executable}{suffix}\n' + HOOK, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + with self.assertRaisesRegex(ValueError, 'direct Compose command at line 1:'): + web_services(f'command -v {executable} && {executable} up -d file-server\n' + HOOK, COMPOSE) + + def test_trailing_comment_backslash_does_not_hide_startup(self): + for prefix in ['echo done # note ', 'echo "# quoted" # note ', + "echo '# quoted' # note ", 'echo done;# note ', + 'echo \\#escaped # note ', 'echo word#hash # note ']: + with self.subTest(prefix=prefix): + line = prefix + '\\' + self.assertEqual(web_services(HOOK + line, COMPOSE), + {'server', 'nginx-proxy', 'polis-api'}) + with self.assertRaisesRegex(ValueError, 'direct container startup at line 2:'): + web_services(line + '\nsudo docker run -d file-server-image\n' + HOOK, COMPOSE) + # Quoted, escaped and embedded hashes still permit real continuations. + for argument in ['"#"', "'#'", r'\#', 'value#hash']: + with self.subTest(argument=argument): + line = f'sudo docker run --rm --label {argument} \\\n -d file-server-image\n' + with self.assertRaisesRegex(ValueError, 'direct container startup at line 1:'): + web_services(line + HOOK, COMPOSE) + + def test_absolute_docker_binary_is_checked(self): + for executable in ['/usr/bin/docker', 'sudo /usr/local/bin/docker']: + with self.subTest(executable=executable): + changed = self.replace_once(HOOK, 'sudo docker run --rm', executable + ' run --rm') + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + for command in ['run -d file-server-image', 'container start file-server']: + with self.assertRaisesRegex(ValueError, 'direct container startup at line 1:'): + web_services(f'{executable} {command}\n' + HOOK, COMPOSE) + + def test_function_header_spacing_and_keyword(self): + for header in ['worker_daemon () {', 'worker_daemon\t() {', + 'function worker_daemon {', 'function worker_daemon() {', + 'function worker_daemon () {']: + with self.subTest(header=header): + changed = self.replace_once(HOOK, 'worker_daemon() {', header) + self.assertEqual(web_services(changed, COMPOSE), {'server', 'nginx-proxy', 'polis-api'}) + changed += 'sudo docker run -d file-server-image\n' + with self.assertRaisesRegex(ValueError, 'sudo docker run -d file-server-image'): + web_services(changed, COMPOSE) + + def test_role_images_error_names_candidate_lines(self): + assignment = ' server) ROLE_IMAGES="server nginx-proxy polis-api" ;;' + for replacement in [assignment.replace('server) ', 'server) '), + assignment + '\n' + assignment, ' server) ;;']: + with self.subTest(replacement=replacement): + changed = self.replace_once(HOOK, assignment, replacement) + with self.assertRaises(ValueError) as caught: + web_services(changed, COMPOSE) + message = str(caught.exception) + self.assertIn('unrecognized web ROLE_IMAGES assignment', message) + for number, line in enumerate(changed.splitlines(), 1): + if 'server)' in line: + self.assertIn(f'line {number}: {line.strip()}', message) + with self.assertRaisesRegex(ValueError, r'no server\) case line found'): + web_services('', COMPOSE) + def test_host_sources_follow_maps_and_pools(self): docs = { 'pool.conf': 'upstream pool { server missing-service:8080; }',