feat(query): Auto-detect UI base path from browser URL (ADR-009)#8568
Merged
Conversation
Propose replacing backend injection of <base href> with an inline script in index.html that detects the URL prefix from window.location at page-load time. This removes the need for --query.base-path to control UI delivery, enables a single pod to serve under multiple external prefixes, and fixes the proxy-rewrite use case (issue jaegertracing#5157) where the external prefix differs from the internal one. Signed-off-by: Yuri Shkuro <github@ysh.us>
The UI now auto-detects its URL prefix via an inline script in index.html (see jaeger-ui commit). Remove the basePathPattern regexp and the base-path rewrite logic from loadAndEnrichIndexHTML; the --query.base-path flag is retained for HTTP route registration only. Update static_handler_test.go to reflect that the backend no longer rewrites the <base> tag, and remove the now-irrelevant invalid-base- path validation test cases from TestNewStaticAssetsHandlerErrors. Also expand ADR-009 implementation plan with per-step detail. Signed-off-by: Yuri Shkuro <github@ysh.us>
Document unit tests (detect-base-path, static_handler) and manual integration tests covering all three use cases: UC-1 direct access, UC-2 single pod multiple prefixes, and UC-3 proxy prefix rewrite. Signed-off-by: Yuri Shkuro <github@ysh.us>
Add three Docker Compose stacks under scripts/e2e/reverse-proxy/ (UC-1, UC-2, UC-3) with permanent httpd.conf and docker-compose.yml files, a driver script scripts/e2e/ui-reverse-proxy.sh, a GitHub Actions workflow ci-e2e-ui-reverse-proxy.yml, and a make target ui-reverse-proxy-integration-test. Also add JAEGER_UI_DIR variable to BuildBinaries.mk and rebuild-ui.sh to allow building from an external jaeger-ui checkout instead of the submodule (used during local development of the ADR-009 UI changes). All three use cases pass locally on arm64 (UC-1/2/3 PASSED). ADR-009 status updated to Accepted and test plan updated with actual setup details and results. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
yurishkuro
added
the
changelog:new-feature
…y tests Replace per-UC ad-hoc JS-only asset check with check_static_assets helper that greps all static/ references from the live index.html and verifies each one returns 200 via the external prefix. This covers favicon, CSS bundle, and JS bundle in every UC, and auto-adapts to future asset names. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the ADR-009 approach for serving Jaeger UI behind reverse proxies by removing backend <base href> rewriting and adding CI-backed reverse-proxy E2E coverage. It also improves local UI build ergonomics by allowing the jaeger-ui submodule path to be overridden.
Changes:
- Stop injecting
<base href>injaeger-querystatic handler and adjust related unit tests. - Add reverse-proxy E2E test harness (UC-1/2/3) plus a dedicated CI workflow and make target to run it.
- Allow overriding the
jaeger-uicheckout path viaJAEGER_UI_DIRin build scripts.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/makefiles/IntegrationTests.mk | Adds a make target to run the new UI reverse-proxy E2E script. |
| scripts/makefiles/BuildBinaries.mk | Introduces JAEGER_UI_DIR for building UI assets from a non-default location. |
| scripts/build/rebuild-ui.sh | Threads JAEGER_UI_DIR through the UI rebuild script. |
| scripts/e2e/ui-reverse-proxy.sh | New E2E script that validates UC-1/2/3 reverse-proxy scenarios. |
| scripts/e2e/reverse-proxy/uc1/docker-compose.yml | UC-1 compose setup (prefix forwarded unchanged). |
| scripts/e2e/reverse-proxy/uc2/docker-compose.yml | UC-2 compose setup (single Jaeger under multiple external prefixes). |
| scripts/e2e/reverse-proxy/uc2/httpd.conf | UC-2 Apache config for dual-prefix forwarding/stripping. |
| scripts/e2e/reverse-proxy/uc3/docker-compose.yml | UC-3 compose setup (external prefix rewritten to internal). |
| scripts/e2e/reverse-proxy/uc3/httpd.conf | UC-3 Apache config for prefix rewrite. |
| docs/adr/README.md | Adds ADR-009 to the ADR index. |
| docs/adr/009-ui-base-path-auto-detection.md | New ADR documenting the auto-detection design and test plan. |
| cmd/jaeger/internal/extension/jaegerquery/internal/static_handler.go | Removes backend base-path injection from index.html. |
| cmd/jaeger/internal/extension/jaegerquery/internal/static_handler_test.go | Updates expectations and removes invalid base-path tests. |
| .github/workflows/ci-e2e-ui-reverse-proxy.yml | New CI workflow to run the reverse-proxy E2E test. |
| .github/workflows/ci-e2e-all.yml | Wires the new UI reverse-proxy workflow into the E2E fan-out. |
Comments suppressed due to low confidence (1)
cmd/jaeger/internal/extension/jaegerquery/internal/static_handler_test.go:172
- This test no longer covers invalid BasePath inputs ("x", "x/", "/x/") after removing the injection-time validation. Since BasePath still affects HTTP route registration, allowing invalid values to pass can lead to broken routing and/or double-slash patterns at runtime. Please add BasePath validation somewhere appropriate and restore/update these assertions to ensure invalid values are rejected early.
func TestNewStaticAssetsHandlerErrors(t *testing.T) {
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{
UIConfig: UIConfig{
ConfigFile: "fixture/invalid-config",
},
Logger: zap.NewNop(),
})
require.Error(t, err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (47.62%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8568 +/- ##
===========================================
- Coverage 96.52% 47.62% -48.90%
===========================================
Files 330 174 -156
Lines 17354 10126 -7228
===========================================
- Hits 16751 4823 -11928
- Misses 454 4813 +4359
- Partials 149 490 +341
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…pose file Replace three separate per-UC docker-compose files with one file that brings up all six containers (three jaeger + three httpd) in parallel on isolated networks. Each service uses a network alias so all httpd configs can address the backend as "jaeger" without naming conflicts. The shell script now does a single compose up/down and runs UC checks sequentially against the already-running stack. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
…roxy workflow Also drop the --platform flag from docker build since the binary arch already matches the runner. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
yurishkuro
changed the title
::group:: must appear at the start of the line; wrapping it in log() prepended a timestamp and broke the fold. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
yurishkuro
commented
May 12, 2026
… fix CI Address review comments on PR jaegertracing#8568: - RegisterRoutes: add validation that BasePath starts with '/' and normalize trailing slash, matching the pattern used in jaegerai. Panics on invalid input (same as http.ServeMux on bad patterns). - static_handler_test.go: change expectedBaseHTML for basePath="" and "/" cases from the old backend-injected '<base href="/"' to 'data-inject-target="BASE_URL"' (backend no longer writes <base href>). Add TestRegisterRoutesInvalidBasePath covering paths without leading '/'. - ci-e2e-ui-reverse-proxy.yml: fix comment ("single job", not "matrix job"); pin alpine:3.21 instead of alpine:latest; add JAEGER_UI_PR=3874 to build from the unmerged UI PR until submodule is updated. - rebuild-ui.sh: support JAEGER_UI_PR env var to gh pr checkout before building (temporary, to be removed after UI PR merges). Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
cmd/jaeger/internal/extension/jaegerquery/internal/static_handler.go:209
- RegisterRoutes panics on invalid BasePath (must start with '/'), which shifts misconfiguration detection from handler creation to route registration and makes the failure mode less consistent (NewStaticAssetsHandler returns errors, but BasePath errors now panic later). Consider moving BasePath validation/normalization into QueryOptions validation or NewStaticAssetsHandler so startup fails deterministically with an error message before partially initializing other components.
if basePath == "" || basePath == "/" {
basePath = "/"
} else {
if !strings.HasPrefix(basePath, "/") {
panic(fmt.Sprintf("invalid base path %q: must start with '/'", basePath))
}
basePath = strings.TrimSuffix(basePath, "/")
nvm is a shell function unavailable in GitHub Actions runners. CI already has the correct Node.js version set by setup-node.js action. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
CI Summary ReportMetrics Comparison✅ No significant metric changes Code Coverage❌ Coverage 51.4% (baseline 97.2%) ➡️ View CI run | View publish logs |
…e-proxy/ Per yurishkuro's review: move all httpd configs and the docker-compose file into examples/reverse-proxy/ alongside the existing UC-1 example. Also pin alpine:latest to alpine:3.21 for reproducible local builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
The Context section described prior behavior; now that the ADR is implemented it consistently uses past tense. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Documents X-Forwarded-Prefix header as a potential improvement for unambiguous prefix detection in edge cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
…xity tradeoffs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
jaegertracingbot
approved these changes
May 13, 2026
Without it GET /alt matched the root pass-through rule and forwarded as /alt to Jaeger (404) instead of redirecting the browser to /alt/ for correct prefix detection. Consistent with UC-1 and UC-3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Previously BasePath was validated in NewStaticAssetsHandler and trimmed in RegisterRoutes, so other consumers (apiv3, jaegerai, server.go) saw the raw value and could produce double-slash route patterns with inputs like /jaeger//. Normalizing at Config.Validate() ensures all downstream handlers see a clean value. Remove the now-redundant per-handler logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
… (ADR-009) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
… 301 The root ProxyPass "/" was intercepting GET /alt before the Redirect directive could redirect to /alt/. Adding ProxyPass "/alt" "!" excludes that exact path from proxying so the Redirect fires correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
… config ProxyPass "/" intercepts all requests before mod_alias Redirect can fire. Using RewriteRule with an exact-match pattern (^/alt$) works because mod_rewrite runs in the URI translation phase alongside the proxy module, and the [R=301,L] flags issue the redirect before any proxy rules apply. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
strings.TrimRight only removed trailing slashes, leaving paths like //jaeger or /jaeger//foo which would break net/http mux routing since the mux cleans request paths by collapsing // but patterns starting with // or containing // may never match. Use path.Clean to detect these cases and reject them explicitly rather than silently normalizing to a different path than the user intended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
- Include dot segments in the base_path error message since path.Clean also normalizes /./ segments, not just traversal and duplicate slashes. - Reword the data-inject-target assertion message to accurately describe what is being checked (the <base> tag marker, not the inline script). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Jaeger UI today requires
--query.base-pathto be set correctly on the backend whenever the UI is served at a URL prefix. This has two fundamental limitations:<base href>— the root cause of issue [Feature]: Support external URL prefix #5157 and why PR Feature Implementation for External URL prefix #5219 was never merged (UC-3).Resolves #5157
Description of the changes
ADR-009 (
docs/adr/009-ui-base-path-auto-detection.md): documents the problem, the three use cases, the design decision, and alternatives considered.UI change (companion PR jaeger-ui#3874): replaces the static
<base href="/">inindex.htmlwith an inline synchronous<script>that readswindow.location.pathname— the actual external URL as seen by the browser — strips any known SPA sub-path suffix, and writes the correct<base href>before the browser's preload scanner processes any asset tags. Falls back to stripping the last path segment for unknown routes, and shows a plain-text error message in#jaeger-ui-rootif React fails to mount.Backend change (
static_handler.go): removes the regexp-based<base href>injection fromloadAndEnrichIndexHTML. The--query.base-pathflag is kept — it still controls HTTP route registration (e.g./jaeger/api/traces).RegisterRoutesvalidates and normalizesBasePath(panics if no leading/, trims trailing/).E2E tests (
examples/reverse-proxy/,scripts/e2e/ui-reverse-proxy.sh): a single Docker Compose file inexamples/reverse-proxy/brings up three isolated stacks (three Jaeger + three Apache httpd containers on separate networks), then the driver script verifies all three use cases:examples/reverse-proxy/setup/and/alt/) — previously impossible/external/→/internal/— the previously broken case (issue [Feature]: Support external URL prefix #5157)Each UC checks: 301 redirect for no-trailing-slash URL, index.html served, inline script present, all static assets reachable, API endpoints (
/api/services,/api/operations) reachable, unknown-route fallback message present, and trace deep-link (when a trace is available).Run locally with:
make ui-reverse-proxy-integration-test(builds UI + binary + Docker image automatically). UsePAUSE=trueto keep containers alive after the tests pass for manual inspection.CI (
.github/workflows/ci-e2e-ui-reverse-proxy.yml): new workflow builds the UI (temporarily from unmerged jaeger-ui PR #3874) + Jaeger binary + Docker image and runs the e2e script; wired intoci-e2e-all.yml. TheJAEGER_UI_PRmechanism will be removed once the companion UI PR is merged and the submodule is pinned.How was this change tested?
static_handler_test.go: all test cases assertdata-inject-target="BASE_URL"marker presence (backend must not rewrite the base tag);TestRegisterRoutesInvalidBasePathcovers invalidBasePathvalues.AI Usage in this PR (choose one)