[matthewberry]

Problem

next/previous links (and any other URL built from request.url) start with http:// in production, for example:

next: http://cleandb-api.platform.moleculemaker.org/api/v1/search?ec_number=1.1.1.1&offset=6&limit=2

Traefik terminates TLS and sends X-Forwarded-Proto: https, but uvicorn trusts X-Forwarded-* only from 127.0.0.1 by default (FORWARDED_ALLOW_IPS, uvicorn 0.34.2 config.py), and Traefik connects from a pod address. Production answers http:// with a 301 to https://, so GET clients that follow redirects still work, but the links are wrong. The access log also records Traefik's address for every request instead of the client's.

Fix

• chart/templates/deployment.yaml: a controller.extraEnv passthrough, rendered both with existingSecret (next to envFrom) and without it (after the five CLEAN_DB_* variables). Before this, the chart had no way to set an extra variable.
• chart/values.mmli2.yaml: FORWARDED_ALLOW_IPS=10.42.0.0/16, the mmli2 cluster's pod network. chart/values.yaml gets extraEnv: [].
• The image is unchanged, so no cluster details are built into it.

What gets trusted: every pod in the cluster, not only Traefik. Traefik's pod IP changes when it restarts, so it can't be pinned. For clients outside the cluster, uvicorn logs the last X-Forwarded-For entry outside the trusted network, which is the one the ingress appended, so they can't spoof their address. A pod inside the cluster can, by calling the Service directly with its own X-Forwarded-For. "*" would be worse: uvicorn then logs the first entry, which an outside client can set (middleware/proxy_headers.py, get_trusted_client_host).

Cluster facts (read-only): production runs uvicorn 0.34.2 with FORWARDED_ALLOW_IPS unset. The Service is ClusterIP only. All nine nodes' pod CIDRs fall inside 10.42.0.0/16 (10.42.{0,1,2,3,4,8,9,10,11}.0/24), including 10.42.9.0/24 on mmli2-worker-cpu-01, where Traefik's pod (10.42.9.118) runs. Traefik v3.7.16 runs with no forwardedHeaders settings; I haven't verified whether it strips client-supplied X-Forwarded-For or appends to it, and this setting gives the right client address either way. The live Ingress serves cleandb-api.platform.moleculemaker.org, which only values.mmli2.yaml lists, so that's the file Argo CD uses.

Testing

Chart: helm template with values.yaml is byte-for-byte unchanged. With values.mmli2.yaml, the only change is the one env entry next to the existing envFrom. With existingSecret empty, the five CLEAN_DB_* variables still render, followed by the extra one. helm lint passes for both values files, and kubectl apply --dry-run=client accepts the deployment.

Image: the CI-built pr-28 image (no FORWARDED_ALLOW_IPS in its config), run with its own uv run uvicorn command on a Docker network with subnet 10.42.0.0/16, against a seeded Postgres. Requests came from a curl container on the same network (10.42.0.5):

FORWARDED_ALLOW_IPS	Request headers	next link	Logged client
10.42.0.0/16, set as the chart sets it	proxy-style: X-Forwarded-Proto: https, X-Forwarded-For: 6.6.6.6, 203.0.113.9	https://…	203.0.113.9 (the entry the proxy appended)
10.42.0.0/16	direct call with X-Forwarded-For: 1.2.3.4	https://…	1.2.3.4 (in-cluster callers are trusted)
unset, as production today	either	http://…	10.42.0.5 (the caller)

Deploying

This reaches production through an Argo CD sync of the chart, not a pod restart. The changed pod template rolls out a new pod by itself. After it's live: check that a next link starts with https://, and that new access-log lines show client addresses rather than 10.42.x.x.

Things to know

• Access logs will contain end users' IP addresses instead of Traefik's. This was considered and accepted.
• If the cluster's pod network changes, links quietly go back to http:// (nothing breaks). Update values.mmli2.yaml if it does.

[matthewberry]

I found no functional bug, but I have three concerns about the setting itself (inline). None of these were reproduced. The first is based on reading the installed uvicorn 0.34.2 uvicorn/middleware/proxy_headers.py.

Three more points about the PR body:

• The evidence for the /16 doesn't add up. The body lists the nodes' pod networks as 10.42.{0,2,3}.0/24, but Traefik's address, 10.42.9.118, falls in none of them. Either that list is incomplete or Traefik's address comes from somewhere else, so it isn't yet shown that Traefik's source address is always inside 10.42.0.0/16.
• The value being shipped wasn't tested as trusted. The trusted case was tested with 127.0.0.0/8, and nobody built the image and checked that the ENV takes effect under uv run uvicorn. The table row labeled "as production today" doesn't match production, which leaves the variable unset (so only 127.0.0.1 is trusted by default). This shows the approach works, not that this change does.
• Client IPs in logs: once Traefik is trusted, the access log records each end user's IP instead of Traefik's. The body mentions this; it's a data-handling policy question rather than a code defect, but it's worth an explicit decision.

The branch is 3 commits behind main. None of them touch the Dockerfile, and main still builds links from request.url, so the fix still applies.

[matthewberry]

From inside the cluster this works the same as "*". 10.42.0.0/16 covers every pod in every namespace, not just Traefik. A pod can call the Service directly with X-Forwarded-For: 1.2.3.4 and X-Forwarded-Proto: https, and uvicorn accepts both. When an in-cluster caller goes through the ingress, every address in the forwarded list is in 10.42.x.x. uvicorn's ProxyHeadersMiddleware walks the list from the right, and when every entry is trusted it returns the first one, which the caller chose. So the guarantee this comment gives only holds for traffic from outside the cluster. Narrowing to Traefik's address(es), or saying in the comment that in-cluster callers are trusted, would fix the mismatch.

[matthewberry]

Agreed, and confirmed by running it: on a 10.42.0.0/16 Docker network, a direct call to the app with X-Forwarded-For: 1.2.3.4 got 1.2.3.4 logged and an https:// link. Narrowing to Traefik isn't practical, because its pod IP changes whenever it restarts. In 53d96b2 the comment (now in chart/values.mmli2.yaml) says that every pod in the cluster is trusted, not only Traefik, and that the guarantee is for outside clients.

[matthewberry]

Agreed. Pinning Traefik isn't workable while its IP changes on restart, and the comment in values.mmli2.yaml now states the actual guarantee (outside clients only). For a ClusterIP-only Service, that tradeoff is fine.

[matthewberry]

The repo's chart gives no way to do this override. With existingSecret set (the default in chart/values.yaml), chart/templates/deployment.yaml renders only envFrom: secretRef; otherwise it renders the five fixed CLEAN_DB_* variables. An operator who adds FORWARDED_ALLOW_IPS to the values has it silently dropped, and links stay http://. Today the only options are a template change or re-sealing the secret.

[matthewberry]

Confirmed: with existingSecret set, the template rendered only envFrom. Fixed in 53d96b2 with a controller.extraEnv passthrough, rendered in both branches of the template. helm template with values.yaml is unchanged; with values.mmli2.yaml the only change is the one env entry; with existingSecret empty the five CLEAN_DB_* variables still render, followed by the extra one.

[matthewberry]

Confirmed. I rendered 53d96b2 with helm template: values.yaml gives only envFrom, as before; values.mmli2.yaml gives envFrom plus the single FORWARDED_ALLOW_IPS entry; and with existingSecret cleared, the five CLEAN_DB_* variables render, followed by the extra one. kubectl apply --dry-run=client accepts the mmli2 deployment.

[matthewberry]

This puts one cluster's pod network into a shared image. The comment names mmli2, but chart/values.yaml deploys to fastapi.cleandb.mmli1.ncsa.illinois.edu. If mmli1's pod network isn't 10.42.0.0/16, or either network changes later, links quietly go back to http:// and nothing signals it. Setting the variable in the chart's env, with a passthrough in the template, would keep cluster details out of the image and fix the override problem on line 35 too.

[matthewberry]

Agreed on keeping cluster details out of the image, and done in 53d96b2: the ENV is gone from the Dockerfile, and FORWARDED_ALLOW_IPS is set in values.mmli2.yaml through the new extraEnv passthrough.

On mmli1: chart/values.yaml on main points at fastapi.cleandb.mmli2.ncsa.illinois.edu. mmli1 was there in the chart's first commit (9a98ca0) and replaced in 79c8daf. The live Ingress serves cleandb-api.platform.moleculemaker.org, which only values.mmli2.yaml lists, so that's the file Argo CD deploys; values.yaml gets extraEnv: [].

[matthewberry]

You're right about mmli1, and I retract that part. chart/values.yaml points at fastapi.cleandb.mmli2.ncsa.illinois.edu on main and on this PR's first commit, afd4cd7, so it wasn't a stale-branch issue. I was simply wrong. Moving the value into values.mmli2.yaml resolves the rest of this comment.

[matthewberry]

Thanks. All three inline points are addressed in 53d96b2 (replies inline), and the description is rewritten. On the description points:

• /16 evidence: right, my node list was cut off by head -3. All nine nodes' pod CIDRs are inside 10.42.0.0/16, including 10.42.9.0/24 on the node Traefik runs on, so its 10.42.9.118 is covered. The description now lists them all.
• Shipped value not tested as trusted: right. I've now run the CI-built image under its own uv run uvicorn command on a Docker network with subnet 10.42.0.0/16, with FORWARDED_ALLOW_IPS=10.42.0.0/16 set the way the chart sets it: proxy-style headers gave an https:// link and the proxy-appended client address in the log. With the variable unset, as production runs today, links stay http://. You're also right that the old "as production today" row used a different value; the new table uses unset.
• Client IPs in logs: decided; real client addresses in the access log are acceptable.
• Branch behind main: merged in (00a6acb).

[matthewberry]

Thanks, this resolves everything from my review. The full node list puts Traefik's 10.42.9.118 inside the /16, and the image test on a 10.42.0.0/16 network, with the variable set the way the chart sets it, is the test that was missing. I didn't re-run the kubectl node queries or the image test myself. What I did verify was the chart rendering (reply on line 35). No further concerns.