refactor: split browser and auth-proxy ingresses. by pantierra · Pull Request #586 · developmentseed/eoapi-k8s

pantierra · 2026-07-23T23:42:49Z

Closes #568.

claude · 2026-07-23T23:43:09Z

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-242 — eoapi.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-84 — eoapi.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

lhoupert · 2026-07-29T14:33:30Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: /stac
+      - equal:
+          path: metadata.annotations["nginx.ingress.kubernetes.io/rewrite-target"]
+          value: /$2


It seems that these notContains assertions can never fail without any: true
The assertion seems to pass even when the path is present, and fails correctly once any: true is added.

lhoupert · 2026-07-29T14:35:13Z

+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: Prefix
            path: /stac
-            backend:
-              service:
-                name: RELEASE-NAME-stac-auth-proxy
-                port:
-                  number: 8080
-      - contains:
+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: ImplementationSpecific
-            path: /browser(/|$)(.*)
-            backend:
-              service:
-                name: RELEASE-NAME-browser
-                port:
-                  number: 8080
+            path: /browser


SAme issue as above

lhoupert · 2026-07-29T14:39:03Z

+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: Prefix
            path: /stac
-            backend:
-              service:
-                name: RELEASE-NAME-stac-auth-proxy
-                port:
-                  number: 8080
-      - contains:
+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: ImplementationSpecific
-            path: /browser(/|$)(.*)
-            backend:
-              service:
-                name: RELEASE-NAME-browser
-                port:
-                  number: 8080
+            path: /browser


Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

pathType: Prefix

path: /stac

backend:

service:

name: RELEASE-NAME-stac-auth-proxy

port:

number: 8080

- contains:

- notContains:

path: spec.rules[0].http.paths

content:

pathType: ImplementationSpecific

path: /browser(/|$)(.*)

backend:

service:

name: RELEASE-NAME-browser

port:

number: 8080

path: /browser

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /browser

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /browser(/|$)(.*)

any: true

lhoupert · 2026-07-29T14:52:09Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: "/browser"


Sane issue than https://github.com/developmentseed/eoapi-k8s/pull/586/changes#r3675257745

Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser"

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser"

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser(/|$)(.*)"

any: true

lhoupert · 2026-07-29T14:54:13Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: /stac


Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac(/|$)(.*)

any: true

lhoupert · 2026-07-30T14:04:21Z

+{{- $_ := set $annotations "traefik.ingress.kubernetes.io/router.entrypoints" ($root.Values.ingress.entrypoints | toString) -}}
+{{- end -}}
+{{- $annotations = mergeOverwrite $annotations ($root.Values.ingress.annotations | default dict) -}}
+{{- $annotations = mergeOverwrite $annotations $owned -}}


While testing this branch with Claude it noticed that a user supplied traefik.ingress.kubernetes.io/router.middlewares in ingress.annotations can get silently dropped

To reproduce:

helm template eoapi charts/eoapi \ --set ingress.className=traefik \ --set browser.enabled=true --set raster.enabled=true \ --set 'ingress.annotations.traefik\.ingress\.kubernetes\.io/router\.middlewares=default-forward-auth@kubernetescrd'

Ingress router.middlewares value rendered

eoapi-browser-ingress default-eoapi-browser-redirect-middleware@kubernetescrd

eoapi-ingress (main) default-eoapi-strip-prefix-middleware@kubernetescrd

default-forward-auth@kubernetescrd, the value set isn't in either one. It's not appended or merged, just replaced, with no warning 😬

Would it make sense to append instead of overwrite? e.g. in eoapi.ingressCommonAnnotations, just before the final merge:

{{- $mwKey := "traefik.ingress.kubernetes.io/router.middlewares" -}} {{- if and (hasKey $annotations $mwKey) (hasKey $owned $mwKey) -}} {{- $_ := set $owned $mwKey (printf "%s,%s" (get $annotations $mwKey) (get $owned $mwKey)) -}} {{- end -}}

With that change, the same command renders:

Ingress router.middlewares value rendered

eoapi-browser-ingress default-forward-auth@kubernetescrd,default-eoapi-browser-redirect-middleware@kubernetescrd

eoapi-ingress (main) default-forward-auth@kubernetescrd,defauleware@kubernetescrd

what do you think?

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from b2068f8 to 652a443 Compare July 24, 2026 00:07

This was referenced Jul 24, 2026

refactor: extract unified ingress helpers. #546

Merged

Enabling stac-auth-proxy on Traefik breaks routing twice: dangling strip-prefix middleware (whole host 404s), and probes miss ROOT_PATH (pod never Ready) #568

Open

pantierra force-pushed the fix/unified-ingress-routing branch 3 times, most recently from 0c5165d to c4bb6ea Compare July 24, 2026 00:34

pantierra requested a review from lhoupert July 24, 2026 00:41

pantierra marked this pull request as ready for review July 24, 2026 00:41

pantierra force-pushed the fix/unified-ingress-routing branch from c4bb6ea to 364dceb Compare July 24, 2026 00:42

feat: split browser and auth-proxy ingresses.

81e90d4

pantierra force-pushed the fix/unified-ingress-routing branch from 364dceb to 81e90d4 Compare July 24, 2026 00:46

lhoupert reviewed Jul 29, 2026

View reviewed changes

lhoupert reviewed Jul 30, 2026

View reviewed changes

lhoupert requested changes Jul 30, 2026

View reviewed changes

Navigation Menu

Uh oh!

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

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

pantierra commented Jul 23, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 23, 2026 •

edited by github-actions Bot

Loading

Uh oh!

lhoupert Jul 29, 2026

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026

Uh oh!

lhoupert Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ingress	router.middlewares value rendered
eoapi-browser-ingress	default-eoapi-browser-redirect-middleware@kubernetescrd
eoapi-ingress (main)	default-eoapi-strip-prefix-middleware@kubernetescrd

Ingress	router.middlewares value rendered
eoapi-browser-ingress	default-forward-auth@kubernetescrd,default-eoapi-browser-redirect-middleware@kubernetescrd
eoapi-ingress (main)	default-forward-auth@kubernetescrd,defauleware@kubernetescrd

Uh oh!

Conversation

pantierra commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Jul 23, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude's PR Review

Simplify (ponytail)

Uh oh!

lhoupert Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pantierra commented Jul 23, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited by github-actions Bot

Loading

lhoupert Jul 29, 2026 •

edited

Loading

lhoupert Jul 29, 2026 •

edited

Loading

lhoupert Jul 29, 2026 •

edited

Loading