refactor: centralize unified ingress logic.

[pantierra]

No description provided.

[lhoupert]

Hey! I opened a PR #569 to contribute to your own PR by adding some tests to lock the bug1 I identified in #568 and fix also a bug in the networking _helper

[claude]

Claude finished @pantierra's task in 5m 47s —— View job

---

Review complete ✅

• Gather context (read diff, changed files, CLAUDE.md)
• Review correctness/security/best-practices
• Apply ponytail-review for over-engineering
• Post final review

✅ No blocking issues — safe to merge.

This is a solid centralization refactor of the ingress logic into charts/eoapi/templates/_helpers/networking.tpl, removing duplication that previously existed between ingress.yaml and traefik-middleware.yaml. I traced the new helpers (eoapi.enabledIngressServices, eoapi.ingressCommonAnnotations, eoapi.ingressRules, eoapi.ingressTls, eoapi.browserIngressEnabled/Path/PathPrefix) against the previous implementation and the accompanying test suite (ingress_test.yaml, browser_redirect_test.yaml, stac-auth-proxy-ingress_test.yaml, validation_tests.yaml), and the behavior is preserved/improved:

• The owned-annotations-merge-last pattern in eoapi.ingressCommonAnnotations (networking.tpl:64-74) correctly ensures chart-managed keys (rewrite-target, strip-prefix, router.middlewares) can't be clobbered by user-supplied ingress.annotations, and this is documented in values.yaml/docs/unified-ingress.md.
• The nil-safety fixes for browser.service (((.Values.browser.service).port | default 8080)) and the regexQuoteMeta fix for nginx regex paths are already in place and covered by dedicated tests ("browser.service null falls back to port 8080", mockOidcServer path tests).
• Browser now gets its own ingress resource (browser-ingress.yaml) with no path-stripping, consistent with the new SB_pathPrefix env var added in deployment.yaml so the pod handles its own prefix — this is a deliberate, well-tested change, not a regression.
• eoapi.validateBrowserDocServerPaths correctly guards the new browser/docServer root-path collision case, with a test for it.
• Removed dead pathType/certManager* values/schema fields are confirmed unused elsewhere in the repo.

One non-blocking observation: ingress.annotations is shared between the main and browser ingresses, so a user setting nginx.ingress.kubernetes.io/rewrite-target there (e.g. copy-pasted from an old config) would silently apply to the browser ingress too, where it's no longer appropriate now that the browser handles its own prefix. This is already called out in both values.yaml and docs/unified-ingress.md, so it's a documented tradeoff rather than a bug — just flagging in case a future defensive strip of those specific keys on the browser ingress is worth considering.

I was unable to execute helm lint/helm unittest in this sandbox (command execution requires interactive approval that isn't available here), so this review is based on careful manual template tracing rather than a live run — worth confirming CI is green before merging.

No findings from the ponytail (over-engineering) pass — the shared helpers are justified by genuine dual call sites (main ingress + browser ingress), and the refactor is a net reduction in duplicated logic rather than new abstraction for its own sake.

---

💰 Estimated review cost: $1.53 · 5m46s · 36 turns