Skip to content

refactor: split browser and auth-proxy ingresses.#586

Open
pantierra wants to merge 1 commit into
mainfrom
fix/unified-ingress-routing
Open

refactor: split browser and auth-proxy ingresses.#586
pantierra wants to merge 1 commit into
mainfrom
fix/unified-ingress-routing

Conversation

@pantierra

@pantierra pantierra commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Closes #568.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude finished @pantierra's task in 3m 46s —— View job


Claude's PR Review

  • Gather context (diff, changed files)
  • Review templates (networking.tpl, ingress/middleware yaml)
  • Review tests
  • Ponytail pass for over-engineering
  • Post final review

✅ No blocking issues — safe to merge.

Traced the full flow: eoapi.ingressFlags gating → eoapi.ingressManifest rendering for the three ingress variants (main/auth-proxy/browser) → the nginx vs. traefik branches for rewrite/strip-prefix/redirect middleware. The nginx/traefik split (separate rewrite-free auth-proxy-ingress only for nginx, auth-proxy kept on the main ingress for traefik with /stac excluded from strip-prefix) is internally consistent, and the accompanying test changes (ingress_test.yaml, stac-auth-proxy-ingress_test.yaml, browser_redirect_test.yaml, traefik_middleware_test.yaml, stac_browser_tests.yaml) exercise the new branches (nginx-only auth-proxy ingress, traefik main-ingress passthrough, browser-only rendering, SB_pathPrefix env var, annotation stripping). Dropping the old semverCompare fallbacks for apiVersion/ingressClassName is fine since Chart.yaml already pins kubeVersion: '>=1.32.0-0', so that code was already dead.

Note: I could not execute helm unittest/helm template/helm lint in this sandboxed environment (commands required interactive approval that isn't available here), so this review is based on static reading of the templates and diffs, not a live render. Recommend confirming CI (View job run link below) passes the chart's unit test suite before merging.

No correctness, security, or best-practice issues found in the diff.

Simplify (ponytail)

  • charts/eoapi/templates/_helpers/networking.tpl:239-242eoapi.browserPathPrefix has a single call site (services/browser/deployment.yaml); could inline the trailing-slash check there and drop the named template.
  • charts/eoapi/templates/_helpers/networking.tpl:71-84eoapi.ingressCommonAnnotations merges $owned then unsets $omit keys; for the auth-proxy/browser variants it'd be simpler to just never merge the nginx-rewrite keys into $owned in the first place instead of merge-then-unset.

Both are minor style nits, not blocking.


💰 Estimated review cost: $1.27 · 3m46s · 32 turns

@pantierra
pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from b2068f8 to 652a443 Compare July 24, 2026 00:07
@pantierra
pantierra force-pushed the fix/unified-ingress-routing branch 3 times, most recently from 0c5165d to c4bb6ea Compare July 24, 2026 00:34
@pantierra
pantierra requested a review from lhoupert July 24, 2026 00:41
@pantierra
pantierra marked this pull request as ready for review July 24, 2026 00:41
@pantierra
pantierra force-pushed the fix/unified-ingress-routing branch from c4bb6ea to 364dceb Compare July 24, 2026 00:42
@pantierra
pantierra force-pushed the fix/unified-ingress-routing branch from 364dceb to 81e90d4 Compare July 24, 2026 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant