feat(ocw-studio): drop the nginx sidecar and let APISix talk to Granian directly - #5345
Conversation
There was a problem hiding this comment.
Pull request overview
Removes OCW Studio’s nginx sidecar, routing APISix directly to Granian while preserving static-file behavior.
Changes:
- Adds Granian static caching and port-aware probe support.
- Replaces nginx routes with APISix rules.
- Documents the migration and rollout plan.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/ol_infrastructure/components/services/k8s.py |
Adds static expiry, resolved ports, and dynamic probes. |
src/ol_infrastructure/components/services/apisix_gateway_api.py |
Documents numeric backend-port requirements. |
src/ol_infrastructure/applications/ocw_studio/files/web.conf_granian |
Removes obsolete nginx configuration. |
src/ol_infrastructure/applications/ocw_studio/__main__.py |
Configures Granian static serving and APISix routes. |
src/bridge/lib/magic_numbers.py |
Defines the static asset cache duration. |
docs/plans/remove-nginx-sidecar.md |
Updates migration status and guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ixes Address Copilot's review on #5345. The dnt-policy route was actually broken: the APISix mocking plugin schema is `anyOf: [required: response_example, required: response_schema]` (apisix/plugins/mocking.lua, verified at the pinned 3.17), so a config carrying neither fails validation and the route would never have served its 204. Adds an empty response_example and a text/plain content_type. That route also becomes an Exact path match. OLApisixHTTPRoute hardcoded PathPrefix for every rule, so /.well-known/dnt-policy.txt/anything would have collected the mocked 204 too, where the nginx `location = ` block it replaces matched only the exact path. OLApisixHTTPRouteConfig gains path_match_type, defaulting to PathPrefix so no existing route changes. /static/hash.txt stays a prefix deliberately -- nginx matched that one with a `location ~*` regex, which caught descendants as well. static_path_expires becomes NonNegativeInt: Granian documents 0 as "disable the cache header", which PositiveInt rejected. build_args already emitted it correctly. The probe_configs resolution switches from `or` to an explicit `is None` check. With `or`, a caller passing an empty mapping to disable probes entirely -- which the previous literal-dict default allowed -- would silently get the defaults back. Tests, on the two behaviors that had none. GranianConfig.static_path_expires gets the same treatment the neighboring static-mount args already had, zero included. The port-alignment tests are the more valuable half: the Service port, the exported application_lb_service_port, the container port and the default probe ports are computed in four different places, and when they disagree the symptom is a 502 on every request with an empty Pulumi diff. They cover the sidecar-free, sidecar-enabled and explicit-application_port paths, and both probe_configs overrides. Confirmed they fail if the resolved port is disconnected from default_probe_configs. OLApplicationK8s exposes application_service so the Service half of that assertion is reachable, matching the existing application_deployment attribute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZAdba4d5cCrrfhWiwec77
|
Addressed all six Copilot threads in 7f60083 — all fixed, none declined. One was a genuine bug, not a nit: the The rest: Re-previewed CI after the changes: 3 create, 8 update, 1 delete, 103 unchanged, with the dnt-policy rule now rendering |
|
Reviewed this with an eye toward rollout risk rather than correctness — the component refactor itself looks right, and I verified the 1. The apply window is likely worse than "a brief 502"The PR notes "no ordering guarantee" between the Service port and the HTTPRoute
With the startup probe allowing up to 2 minutes, this is plausibly a multi-minute full-traffic outage rather than a blip. Cheapest fix is probably to make it a two-phase change: publish both 8071 and 8073 on the Service for one deploy, land that, then drop 8071 in a follow-up. Failing that, this should be an announced maintenance apply rather than a routine CI→prod promotion, and the PR body's "watch for a brief 502 window" is worth restating in stronger terms. 2.
|
|
Point 3 fixed in 6669455: OLApisixHTTPRouteConfig gains backend_import_nginx_config, and a model validator rejects backend_service_port="http" when it's explicitly False, turning the named-port trap into a pulumi preview failure instead of a silent 502. ocw_studio's four routes now set it. Points 1 and 2 are addressed as decisions rather than code: Point 2 (Granian flag vs. deployed SHA) — verified and closed. ocw-studio's master pins granian==2.7.4 (bumped 2026-05-07 for a security fix), which I confirmed independently supports --static-path-mount/--static-path-route/--static-path-expires against the PyPI wheel. OCW_STUDIO_DOCKER_SHA tracks the "latest"-tagged image built off master, and master ships releases continuously, so the risk of the promoted image predating that flag support is negligible. Point 1 (502 window during the port switch) — accepted as-is rather than split into a two-phase rollout. ocw_studio is low-traffic, so the exposure of a brief apply-time window is judged acceptable here; not worth the added complexity of decoupling the Service/Route port switch from the sidecar removal for this app. |
…an directly Stage 2 of docs/plans/remove-nginx-sidecar.md, and the first app to actually exercise Granian's static serving -- odl_video_service had dj_static.Cling doing that already, so this is where a misbehaving static path would have shown up before mit_learn and mitxonline depend on it. STATIC_ROOT is the relative "staticfiles" against the image's WORKDIR of /src, i.e. the same emptyDir the collectstatic init container populates, and STATIC_URL is /static/ -- Granian's default route -- so the mount needs no route override. Three of the sidecar's location blocks move to APISix rules. hash.txt is real here rather than vestigial: the Dockerfile stamps $GIT_REF into it and useAppVersionCheck polls it on every route change to force a post-deploy reload, so it keeps its no-cache treatment. The /static CORS route exists because ocw_studio has no shared plugin config -- without it the header the sidecar added would just vanish, since Granian sets none. dnt-policy becomes a mocking 204 so crawlers are answered at the gateway instead of on a blocking thread. Two component gaps had to close first: - GranianConfig gains static_path_expires. Granian defaults to a 1-day Cache-Control, an order of magnitude short of the `expires max` the sidecar used on content-hashed assets. - probe_configs no longer defaults to a literal dict pinned to DEFAULT_NGINX_PORT. It defaults to None and the component builds the django-health-check probes against the resolved application port, so an app dropping its sidecar does not have to restate all three probes to change a number. Verified no-op against learn_ai, which still has its sidecar and default probes: no Deployment diff. The named-port trap is the one that would have caused an outage. OLApisixHTTPRoute has to hand Gateway API a numeric port and maps the name "http" to a hardcoded 8071, so an app on that path keeps routing to the sidecar's port after the sidecar is gone and 502s on everything, with nothing in the Pulumi diff to hint at it. OLApplicationK8s now publishes the resolved number as application_lb_service_port and every rule here uses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZAdba4d5cCrrfhWiwec77
…ecedent The stage 2 note called ocw_studio "the first app to actually exercise Granian's static serving", which is wrong -- edxapp LMS and Studio have been running static_path_mounts=/openedx/staticfiles in production since before this project started, and the plan already names them as the precedent it generalizes. What is actually true of ocw_studio is narrower: it is the first of the seven sidecar apps where Granian picks up /static/* traffic that nginx was serving, since odl_video_service had dj_static.Cling doing that already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZAdba4d5cCrrfhWiwec77
…ixes Address Copilot's review on #5345. The dnt-policy route was actually broken: the APISix mocking plugin schema is `anyOf: [required: response_example, required: response_schema]` (apisix/plugins/mocking.lua, verified at the pinned 3.17), so a config carrying neither fails validation and the route would never have served its 204. Adds an empty response_example and a text/plain content_type. That route also becomes an Exact path match. OLApisixHTTPRoute hardcoded PathPrefix for every rule, so /.well-known/dnt-policy.txt/anything would have collected the mocked 204 too, where the nginx `location = ` block it replaces matched only the exact path. OLApisixHTTPRouteConfig gains path_match_type, defaulting to PathPrefix so no existing route changes. /static/hash.txt stays a prefix deliberately -- nginx matched that one with a `location ~*` regex, which caught descendants as well. static_path_expires becomes NonNegativeInt: Granian documents 0 as "disable the cache header", which PositiveInt rejected. build_args already emitted it correctly. The probe_configs resolution switches from `or` to an explicit `is None` check. With `or`, a caller passing an empty mapping to disable probes entirely -- which the previous literal-dict default allowed -- would silently get the defaults back. Tests, on the two behaviors that had none. GranianConfig.static_path_expires gets the same treatment the neighboring static-mount args already had, zero included. The port-alignment tests are the more valuable half: the Service port, the exported application_lb_service_port, the container port and the default probe ports are computed in four different places, and when they disagree the symptom is a 502 on every request with an empty Pulumi diff. They cover the sidecar-free, sidecar-enabled and explicit-application_port paths, and both probe_configs overrides. Confirmed they fail if the resolved port is disconnected from default_probe_configs. OLApplicationK8s exposes application_service so the Service half of that assertion is reachable, matching the existing application_deployment attribute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZAdba4d5cCrrfhWiwec77
…nginx sidecar feoh flagged that _resolve_backend_port silently maps the port name "http" to the nginx sidecar's port (8071) with no way to see whether the backend actually has a sidecar, so a route left on "http" after an app drops its sidecar 502s on every request with nothing in the Pulumi diff to hint at it. Six more apps are queued behind this migration, so make the mistake a pulumi preview failure instead of a silent runtime 502: OLApisixHTTPRouteConfig gains backend_import_nginx_config, and a model validator rejects backend_service_port="http" when it's explicitly False. ocw_studio's four routes now set it, engaging the check for the app this PR migrates.
6669455 to
86634b6
Compare
Sentry/Seer flagged that path.rstrip("*").rstrip("/") ran unconditionally,
so an Exact route with a trailing slash (e.g. "/api/") silently became
"/api" -- a different path under Gateway API's PathExact semantics. No
current route hits this (dnt-policy has no trailing slash), but it's a
live trap for the six app migrations queued behind this one. Only strip
the trailing "/" for PathPrefix, where it's a no-op either way.
What are the relevant tickets?
N/A — stage 2 of
docs/plans/remove-nginx-sidecar.md. Follows #5281 (odl_video_service).Description (What does it do?)
Removes the
nginx:1.31.3sidecar from the OCW Studio deployment so APISix speaks directly to Granian.Granian's static serving is already the production path for edxapp LMS and Studio (
k8s_resources.py—static_path_mounts=["/openedx/staticfiles"]), which is the precedent this rollout generalizes. ocw_studio is the first of the seven sidecar apps where it carries/static/*that nginx used to serve; OVS went first but its WSGI app was already wrapped indj_static.Cling.Static
STATIC_ROOTis the relative"staticfiles", resolved against the image'sWORKDIR /src, so it lands on/src/staticfiles— the same emptyDir the collectstatic init container populates.STATIC_URLis/static/, which is Granian's default static route, so the mount needs no route override.The sidecar's
locationblocks, translatedlocation ~* /static/(.*$)→expires max--static-path-mount /src/staticfiles --static-path-expires 315360000add_header Access-Control-Allow-Origin */static/*route withresponse-rewritelocation ~* /static/hash.txt→expires -1+Cache-Control private/static/hash.txtroute withresponse-rewritesettingCache-Control: private, no-cachelocation = /.well-known/dnt-policy.txt→return 204mockingplugin (response_status: 204,with_mock_header: false)location = /nginx-healthX-Forwarded-Proto $my_schemeclient_max_body_size 25Mclient_max_body_size: 0and does not override it, so this loosens the cap. Nothing here appears to rely on itTwo of those deserve a note:
hash.txtis load-bearing here, not vestigial. The Dockerfile stamps$GIT_REFinto/src/static/hash.txt, anduseAppVersionCheckpolls/static/hash.txton every route change to force a reload after a deploy. It must not inherit the 10-yearmax-ageGranian now stamps on that directory. The fetch already passescache: "no-store", so this only matters for intermediaries — and ocw_studio sits directly behind APISix with no CDN, so there are none today. Preserved anyway./static/*CORS route is not redundant. Granian sets no CORS header at all, and unlike mit_learn/mitxonline this app has no shared plugin config supplyingcors— so without this route the header the sidecar added would silently disappear./.well-known/dnt-policy.txtwas kept rather than dropped: it costs onemockingplugin and keeps a crawled path off Granian's blocking-thread pool, where Django would answer it with a 404.Two component gaps had to close first
GranianConfig.static_path_expires. Granian's static handler emitsCache-Control: max-age=<n>and defaults to 86400 (src/files.rs) — an order of magnitude short of theexpires maxthe sidecar used on content-hashed assets.STATIC_ASSET_MAX_AGE_SECONDS(315360000) inbridge/lib/magic_numbers.pyis what nginx'sexpires maxactually resolves to. Verified the flag exists in granian v2.7.4, the version pinned in the ocw-studio image.probe_configsno longer hardcodesDEFAULT_NGINX_PORT. It was a literal dict default pinned to 8071, so every app dropping its sidecar had to restate all three probes just to change a number (which is exactly what #5281 did). It now defaults toNoneand the component builds the django-health-check probes against the resolved application port viadefault_probe_configs(). The six apps that passprobe_configsexplicitly are untouched and still own the port they name.The named-port trap — the part that would have caused an outage
OLApisixHTTPRoutehas to hand Gateway API a numericbackendRef.port, so_resolve_backend_portmaps the port name"http"to a hardcoded8071—DEFAULT_NGINX_PORT. Any app on the Gateway API route path (ocw_studio, xpro, learn_ai) that drops its sidecar therefore keeps routing to the sidecar's port and 502s on every request, with nothing in the Pulumi diff to hint at it — the route resource simply does not change.OLApplicationK8snow publishes the resolved number asapplication_lb_service_port, and all four rules here use it. This trap does not affect theApisixRouteCRD path (odl_video_service), which takes the number directly.Not in this PR
xpro is the other single-static-directory app and the same shape, but #5344 is concurrently changing xpro's Granian concurrency. Overlapping two live changes on one app makes any regression ambiguous, so xpro waits for that to land and settle.
How can this be tested?
pulumi previewagainst the CI stack — 3 creates, 8 updates, 1 delete, 104 unchanged:What to check in the diff:
--static-path-mount /src/staticfiles --static-path-expires 315360000nginxcontainer disappears and thenginx-configConfigMap is deletedcontainerPort, the Serviceport/targetPort, and all three probe ports move8071 → 8073backendRefs[].port: 8073, in this order:/static/hash.txt,/static,/.well-known/dnt-policy.txt,/— HTTPRoute has no priority field, so precedence comes from longest matching path prefix, which reproduces nginx's orderingPluginConfigresources, one per non-passthrough ruleAlso verified the
probe_configsrefactor is a no-op for apps that still have their sidecar —pulumi previewoflearn_aiCI (default probes, sidecar on) shows no Deployment diff, only pre-existing unrelated Fastly drift.After applying to CI, against
ocw-studio-ci.odl.mit.edu:Watch for a brief 502 window during the apply: the Service port and the HTTPRoute
backendRefs[].portchange in the same update with no ordering guarantee between them.🤖 Generated with Claude Code
https://claude.ai/code/session_01RZAdba4d5cCrrfhWiwec77