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 @lhoupert's task in 2m 44s —— View job

Claude finished @lhoupert's requested review

Gather context (diff, changed files)
Review helper template logic (networking.tpl)
Review ingress/browser/auth-proxy templates
Review Helm unit tests
Apply ponytail-review pass
Post final review

✅ No blocking issues — safe to merge.

Both previously-flagged review issues are properly addressed in the latest commit:

The notContains assertions in stac-auth-proxy-ingress_test.yaml / ingress_test.yaml now all carry any: true (verified every occurrence), so they actually fail when the path is present.
The Traefik router.middlewares silent-drop bug is fixed in eoapi.ingressCommonAnnotations (charts/eoapi/templates/_helpers/networking.tpl:83): when both the user-supplied annotation and the chart-owned strip-prefix middleware are present, they're now comma-joined (user,owned) instead of the chart value clobbering the user's. Confirmed by the updated test "traefik chart middlewares merge with user router.middlewares annotation".

I traced the new eoapi.ingressFlags / eoapi.ingressManifest / eoapi.apiIngressPaths gating logic end-to-end for all three ingress resources (main, auth-proxy-ingress, browser-ingress) across nginx and traefik, and it's internally consistent:

nginx + auth-proxy service → routed only via the rewrite-free auth-proxy-ingress (main ingress excludes it).
traefik + auth-proxy service → stays on the main ingress (no separate resource), matching the "keep auth-proxy on main ingress for traefik" test.
browser is fully split out via eoapi.browserIngressEnabled, independent of the other gating flags, and no longer participates in Traefik strip-prefix (matches the doc update).

No correctness, security, or best-practice issues found in the template or test changes. kubeVersion: '>=1.32.0-0' in Chart.yaml means dropping the old semverCompare apiVersion/ingressClassName fallbacks in the consolidated eoapi.ingressManifest is safe, not a regression.

One non-blocking observation: the dedicated unit test that rendered profiles/experimental.yaml end-to-end ("should route correctly with experimental profile") was removed in ingress_test.yaml and replaced with hand-picked value sets rather than an equivalent test against the real profile file. That profile is the one combination (nginx + auth-proxy + browser + docServer + mockOidcServer) most likely to interact across all three new ingress resources at once — worth a follow-up test rendering the actual profile file if you want that safety net back, but not required for this PR.

Note: I couldn't execute helm unittest locally in this sandbox (tool permissions), so the above is from static template/test tracing rather than an actual test run — the repo's own "CI tests" workflow run for this PR was still in progress at review time.

Simplify (ponytail): none — the refactor consolidates three near-duplicate ingress templates into shared eoapi.ingressManifest/eoapi.ingressRules/eoapi.ingressCommonAnnotations helpers, which is a net reduction in duplication rather than added complexity.

💰 Estimated review cost: $0.74 · 2m44s · 30 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

pantierra · 2026-08-25T11:20:59Z

Thanks for the review, @lhoupert! I addressed the mentioned issues and cleaned up a bit more. I'd appreciate another round of review.

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

Closed

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 2 times, most recently 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

Comment thread charts/eoapi/templates/_helpers/networking.tpl

lhoupert requested changes Jul 30, 2026

View reviewed changes

pantierra mentioned this pull request Aug 24, 2026

Deploying using nginx ingress results in 404 for /browser #593

Open

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from 52bb6b6 to a7e5d7d Compare August 25, 2026 11:11

pantierra requested a review from lhoupert August 25, 2026 11:18

pantierra force-pushed the fix/unified-ingress-routing branch from a7e5d7d to 7fc2658 Compare August 25, 2026 11:20

pantierra force-pushed the fix/unified-ingress-routing branch from 7fc2658 to c07ea81 Compare August 25, 2026 11:36

lhoupert mentioned this pull request Aug 25, 2026

chore(deps): upgraded stac-browser to version 5.0.0. #598

Merged

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from c07ea81 to 2665787 Compare August 25, 2026 16:22

feat: split browser and auth-proxy ingresses.

fbe6257

pantierra force-pushed the fix/unified-ingress-routing branch from 2665787 to fbe6257 Compare September 1, 2026 14:49

Merge branch 'main' into fix/unified-ingress-routing

b23f609

lhoupert approved these changes Sep 3, 2026

View reviewed changes

pantierra merged commit b5249fd into main Sep 3, 2026
5 checks passed

pantierra deleted the fix/unified-ingress-routing branch September 3, 2026 10:56

ds-release-bot Bot mentioned this pull request Sep 3, 2026

chore(main): release 0.16.0 #602

Merged

Navigation Menu

Uh oh!

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

refactor: split browser and auth-proxy ingresses.#586
pantierra merged 2 commits 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!

pantierra Aug 25, 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!

Uh oh!

pantierra commented Aug 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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 finished @lhoupert's requested review

Uh oh!

lhoupert Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

pantierra Aug 25, 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!

Uh oh!

pantierra commented Aug 25, 2026

Uh oh!

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