Skip to content

nginx: route /api/v1/tools to the service, and test that every API route is routed - #342

Merged
openipc-ai merged 2 commits into
masterfrom
tools-nginx-location
Sep 29, 2026
Merged

openipc-ai merged 2 commits into
masterfrom
tools-nginx-location

Conversation

@openipc-ai

Copy link
Copy Markdown
Collaborator

Follow-up to #341, found after it went to production.

What's broken: the tools push (PUT /api/v1/tools/<name>) and its listing (GET /api/v1/tools) have no location in either vhost. They fall through to @fallback, which answers with a 302 to the home page.

Effect: ipctool's first release push after OpenIPC/ipctool#225 reached nothing. So http://openipc.org/ipctool and the NFS export stay empty, and the uget and NFS instructions on /cameras/report lead nowhere.

This PR:

  • Proxies ^~ /api/v1/tools to the web role in prod and dev, with an 8 MB body limit (the service allows 4 MB).
  • Exempts the three push addresses from the datacentre block, because GitHub's runners are on Azure.
  • Adds TestEveryAPIRouteReachesTheService. For every /api/ route in routes.json, it picks the location nginx would choose (exact match, then the longest prefix, then the first matching regex) and requires that location to proxy to the service. Run against master's vhosts, it fails for exactly these two routes; with this change it passes.

Checks: service/run.sh go test ./deploytest/ passes and deploy/nginx/check-config.sh passes.

After merge:

  1. deploy/push-nginx.sh --apply
  2. Re-run ipctool's release workflow (the step change is OpenIPC/ipctool's tools-push-check-status PR).
  3. Check GET /api/v1/tools, http://openipc.org/ipctool, and an NFS mount from a lab camera.

…ute is routed

The tools push (PUT /api/v1/tools/<name>) and its listing had no location
in either vhost. They fell through to @fallback and answered a 302 to the
home page, so ipctool's first release push after #341 reached nothing and
http://openipc.org/ipctool stayed empty. They are now proxied to the web
role, and the three push addresses are exempt from the datacentre block,
because GitHub's runners are Azure.

TestEveryAPIRouteReachesTheService picks, for every /api/ route in
routes.json, the location nginx would choose (exact, longest prefix, then
regex) and requires that it proxies to the service. On master's vhosts it
fails for exactly these two routes.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Route tools API requests to the web service in both nginx vhosts

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Route tools uploads and listings to the web service instead of redirecting to the home page.
• Allow authenticated release pushes from Azure-hosted GitHub runners through the production
 datacentre block.
• Test that every declared API route reaches the service through both TLS vhosts.
Diagram

graph TD
  Runner["Release runner"] --> Block{"Datacentre exception"} --> Nginx["Prod/dev nginx"] --> Web["Web service"]
  Client["API client"] --> Nginx
  Routes["Route registry"] --> Test["Routing test"] --> Nginx
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use separate exact-match tools locations
  • ➕ Restricts proxying to the listing path and explicitly named upload paths.
  • ➖ Duplicates proxy configuration across endpoints and vhosts.
  • ➖ Requires nginx changes when supported tool names change.

Recommendation: Keep the shared tools prefix location: it covers listing and named uploads with one rule per vhost, while the service validates tool names. Retain exact URI exceptions in the production datacentre map so only the three intended push addresses bypass that block.

Files changed (4) +111 / -0

Bug fix (3) +34 / -0
openipc-datacentre-block.confExempt three ipctool push addresses from the datacentre block +4/-0

Exempt three ipctool push addresses from the datacentre block

• Adds exact URI exemptions for the three supported ipctool binaries. This lets Azure-hosted GitHub release runners reach the authenticated upload endpoint without lifting the broader datacentre block.

deploy/nginx/conf.d/openipc-datacentre-block.conf

org.openipcProxy production tools API requests to the web role +15/-0

Proxy production tools API requests to the web role

• Adds a tools API location that forwards listings and uploads to port 3002 instead of the fallback redirect. It buffers uploads and sets an 8 MB nginx body limit above the service's 4 MB limit.

deploy/nginx/sites-available/org.openipc

org.openipc.devProxy development tools API requests to the web role +15/-0

Proxy development tools API requests to the web role

• Mirrors the production tools location but forwards requests to development's port 3012. It applies the same upload buffering, body limit, and forwarding headers.

deploy/nginx/sites-available/org.openipc.dev

Tests (1) +77 / -0
apiroutes_test.goCheck declared API routes against nginx location selection +77/-0

Check declared API routes against nginx location selection

• Adds a deployment test that samples every '/api/' route in 'service/routes.json' against both TLS vhosts. It models exact, longest-prefix, and first-matching-regex selection, then checks that the selected location proxies to the service.

service/deploytest/apiroutes_test.go

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Staging tool responses omit noindex ✓ Resolved
Description
The new staging tools location sets its own add_header directives but does not repeat the server's
X-Robots-Tag: noindex, nofollow, noarchive header. Because location-level headers replace
inherited headers, responses from GET /api/v1/tools and the push endpoint lose the staging site's
noindex policy.
Code

deploy/nginx/sites-available/org.openipc.dev[R304-305]

+        add_header Strict-Transport-Security max-age=15768000;
+        add_header X-Served-By go always;
Evidence
The staging server declares the robots header, while the new location declares two other response
headers but not that one. A neighboring staging location explicitly documents and handles nginx's
replacement of inherited add_header directives.

deploy/nginx/sites-available/org.openipc.dev[90-100]
deploy/nginx/sites-available/org.openipc.dev[297-305]
deploy/nginx/sites-available/org.openipc.dev[120-127]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new staging tools location overrides inherited response headers and omits the staging noindex header.
## Fix Focus Areas
- deploy/nginx/sites-available/org.openipc.dev[297-306]
## Recommended Fix
Add `add_header X-Robots-Tag "noindex, nofollow, noarchive" always;` to the staging tools location, alongside its other response headers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Tests miss wrong upstreams for tools ✓ Resolved
Description
TestEveryAPIRouteReachesTheService accepts any literal loopback port beginning with 30 instead
of checking the route's role and environment. A staging tools location pointed at production's web
port 3002, rather than staging's 3012, would pass this new test despite sending pushes to the
wrong service.
Code

service/deploytest/apiroutes_test.go[R30-31]

+			if !strings.Contains(l.Body, "proxy_pass http://127.0.0.1:30") && !strings.Contains(l.Body, "proxy_pass http://$openipc_up_") {
+				t.Errorf("%s: %s %s lands in `%s`, which does not proxy to the service", name, rt.Method, rt.Path, l.Header)
Evidence
The tools route is declared for the web role, and the two newly added locations use different
environment-specific ports. The test's substring check accepts either port for either vhost.

service/routes.json[140-153]
deploy/nginx/sites-available/org.openipc[356-364]
deploy/nginx/sites-available/org.openipc.dev[297-305]
service/deploytest/apiroutes_test.go[16-32]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The API route test accepts upstream ports from either environment and role, so a misdirected tools push can pass.
## Fix Focus Areas
- service/deploytest/apiroutes_test.go[16-32]
## Recommended Fix
Map each vhost and route role to its expected upstream, then require the selected location to proxy to that upstream. Where a location uses an upstream variable, validate its configured destination as well.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread deploy/nginx/sites-available/org.openipc.dev
Comment thread service/deploytest/apiroutes_test.go Outdated
…cks the upstream

- TestInheritedHeaders read only server-level add_header lines indented four
  spaces, and dev's X-Robots-Tag is indented three. So every dev location
  that set a header of its own dropped the noindex policy: the ten added by
  #341 and this PR, and the wizard, explorer, build push and vendor-firmware
  locations before them. The test now reads both indents, and all fourteen
  repeat the header.
- TestEveryAPIRouteReachesTheService now requires the upstream of the route's
  role in the vhost's own environment (3002/3003 prod, 3012/3013 dev, or
  that environment's openipc-route variable). With dev's tools location
  pointed at 3002 it fails.
@openipc-ai
openipc-ai merged commit 90c79ee into master Sep 29, 2026
2 checks passed
@openipc-ai
openipc-ai deleted the tools-nginx-location branch September 29, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant