New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Implementation for External URL prefix #5219

Open

Sidhartha-Biswal wants to merge 14 commits into jaegertracing:main from Sidhartha-Biswal:main

Sidhartha-Biswal commented Feb 21, 2024

Which problem is this PR solving?

Resolves #5157

Description of the changes

Implemented code for query.ui-base-path CLI flag.

Added a new field UIBasePath to the StaticAssetsHandlerOptions struct.

Created a new assetsFS instance using the UIBasePath field if it is non-empty.

How was this change tested?

Tested manually and it did not work. So need some suggestions to proceed ahead.

Sidhartha-Biswal added 4 commits

February 20, 2024 16:27


Update static_handler.go

e3ffd8c

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update flags.go

874327c

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update flags.go

8941b9e

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update static_handler.go

5d5e85b

Signed-off-by: Sidhartha-Biswal <[email protected]>

Sidhartha-Biswal requested a review from a team as a code owner

February 21, 2024 12:18

Sidhartha-Biswal requested a review from jkowall

February 21, 2024 12:18

Author

Sidhartha-Biswal commented Feb 21, 2024

Hi @yurishkuro,

Please find the below configuration.
QUERY_BASE_PATH: /jaeger
QUERY_UI_BASE_PATH: /hub/jaeger

The static files loading error is still there.

Could you please check the code changes and give some suggestions to proceed ahead?


Update static_handler.go

9ec0d3f

Signed-off-by: Sidhartha-Biswal <[email protected]>

Member

yurishkuro commented Feb 22, 2024

How is this different from the other PR?

Author

Sidhartha-Biswal commented Feb 28, 2024 •

edited

Hi @yurishkuro,

We have tried setting the QUERY_UI_BASE_PATH="/hub/jaeger" and tried to access the Jaeger UI. But still we found that static assets are not getting loaded.
With the changes we have tried, the QUERY_UI_BASE_PATH value is reflecting in the in the index.html. This way the changes are different from the other PR.

manisha5464 commented Mar 3, 2024

Hi @yurishkuro
Can you please describe what is the relation between QUERY_BASE_PATH and QUERY_UI_BASE_PATH?
Can we set both the parameters at the same time?
Like for example QUERY_BASE_PATH: /jaeger and QUERY_UI_BASE_PATH: /hub/jaeger, then which path should take the priority?
Which path should reflect in index.html?

I was also facing a similar issue, unable to load the static assets in Jaeger UI when both the parameters are set. I was trying with the code changes in this PR and also with those mentioned in this [https://github.com//pull/5189]. Unfortunately still the issue exists and not able to load the static assets.

Can you please provide some suggestions that would help to proceed further.


Update static_handler.go

b8274aa

Signed-off-by: Sidhartha-Biswal <[email protected]>

Author

Sidhartha-Biswal commented Mar 11, 2024

Hi @yurishkuro,

We have tried adding the QUERY_UI_BASE_PATH parameter.
We tested the latest code changes by setting the QUERY_UI_BASE_PATH ="/hub/jaeger" and QUERY_BASE_PATH="/jaeger"
With the latest changes pushed we see the below log in Query Service. The QUERY_UI_BASE_PATH is not getting appended to static folder. Therefore static files are not loaded into Jaeger UI.

Expected log entry should have url entry like below
"url": "/hub/jaeger/static/index.css"

Below is the code change I have tried for appending the UIBasePath to static folder, but still we see the issue.

Can you please review the latest changes and let us know where we are missing.

yurishkuro reviewed

View reviewed changes

Member

yurishkuro left a comment

Getting closer

cmd/query/app/static_handler.go Outdated

  if sH.options.BasePath == "" {
  sH.options.BasePath = "/"
  }
+ if sH.options.UIBasePath == "" {
+ sH.options.UIBasePath = "/ui" // default value

Member

yurishkuro Mar 11, 2024

It should default to BasePath

Author

Sidhartha-Biswal Mar 12, 2024

Modified the code so that it should default to BasePath.

cmd/query/app/static_handler.go

  if sH.options.BasePath == "" {
  sH.options.BasePath = "/"
  }
+ if sH.options.UIBasePath == "" {
+ sH.options.UIBasePath = "/ui" // default value
+ }
  if sH.options.BasePath != "/" {

Member

yurishkuro Mar 11, 2024

BasePath no longer relevant to indexBytes, only ui path

Author

Sidhartha-Biswal Mar 12, 2024

Updated the UIBasePath instead of BasePath

cmd/query/app/static_handler.go Outdated

Comment on lines 152 to 156

+ } else {
+ if !strings.HasPrefix(sH.options.BasePath, "/") || strings.HasSuffix(sH.options.BasePath, "/") {
+ return nil, fmt.Errorf("invalid base path '%s'. Must start but not end with a slash '/', e.g. '/jaeger/ui'", sH.options.BasePath)
+ }
+ indexBytes = basePathPattern.ReplaceAll(indexBytes, []byte(fmt.Sprintf(`<base href="%s/"`, sH.options.BasePath)))

Member

yurishkuro Mar 11, 2024

Suggested change

 } else {

 if !strings.HasPrefix(sH.options.BasePath, "/") || strings.HasSuffix(sH.options.BasePath, "/") {

 return nil, fmt.Errorf("invalid base path '%s'. Must start but not end with a slash '/', e.g. '/jaeger/ui'", sH.options.BasePath)

 }

 indexBytes = basePathPattern.ReplaceAll(indexBytes, []byte(fmt.Sprintf(`<base href="%s/"`, sH.options.BasePath)))

Author

Sidhartha-Biswal Mar 12, 2024

Implemented the suggested code change as mentioned above.

cmd/query/app/static_handler.go Outdated

  if sH.options.BasePath != "/" {
- if !strings.HasPrefix(sH.options.BasePath, "/") || strings.HasSuffix(sH.options.BasePath, "/") {
- return nil, fmt.Errorf("invalid base path '%s'. Must start but not end with a slash '/', e.g. '/jaeger/ui'", sH.options.BasePath)
+ if sH.options.UIBasePath != "/ui" {

Member

yurishkuro Mar 11, 2024

Check not needed

Author

Sidhartha-Biswal Mar 12, 2024

Removed the check.

cmd/query/app/static_handler.go Outdated

 // RegisterRoutes registers routes for this handler on the given router
 func (sH *StaticAssetsHandler) RegisterRoutes(router *mux.Router) {
  fileServer := http.FileServer(sH.assetsFS)
- if sH.options.BasePath != "/" {
- fileServer = http.StripPrefix(sH.options.BasePath+"/", fileServer)
+ if sH.options.UIBasePath != "/" {

Member

yurishkuro Mar 11, 2024

File serving is unaffected by ui path

Author

Sidhartha-Biswal Mar 12, 2024

Removed the code that is not required.

Sidhartha-Biswal added 3 commits

March 12, 2024 13:30


Update static_handler.go

a7e13b6

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update static_handler.go

b1b171a

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update static_handler.go

566050d

Signed-off-by: Sidhartha-Biswal <[email protected]>

Author

Sidhartha-Biswal commented Mar 12, 2024 •

edited

Hi @yurishkuro,

Tested with the latest changes, we still see issues with loading the static assets as shown below.

In Query logs, we now see the Query_UI_Base_Path getting appended to static fodler. But in Jaeger UI, still shows not loading errors.


Update flags.go

0d52fe4

Signed-off-by: Sidhartha-Biswal <[email protected]>

Author

Sidhartha-Biswal commented Mar 14, 2024

Hi @yurishkuro,

With the latest changes, I have tried to test with the below combination of QUERY_BASE_PATH and QUERY_UI_BASE_PATH values and please find the results below.

When both the QUERY_BASE_PATH & QUERY_UI_BASE_PATH values are different as shown above, static assets are not getting loaded in Jaeger UI with the below errors in console log.

Can you please review the code changes and help us with some suggestions what we are missing.

yurishkuro reviewed

View reviewed changes

Member

yurishkuro left a comment

Your PR still has no changes to the configuration of the reverse proxy. How are you expecting it to work if you're giving different prefixes and nothing in between that is rewriting the URLs?

Author

Sidhartha-Biswal commented Mar 18, 2024

Hi @yurishkuro,

Is it mandatory to use a reverse proxy when both the QUERY_BASE_PATH & QUERY_UI_BASE_PATH parameters are set at the same time?

The changes in reverseproxy configuration which you mentioned above are they related to source code changes or test code changes?

Member

yurishkuro commented Mar 18, 2024

It's a test file

Author

Sidhartha-Biswal commented Mar 26, 2024

Hi @yurishkuro,

Thanks for the support.
Currently when we set both paths and only using QUERY_UI_BASE_PATH we are able to access query UI but not able to access using QUERY_BASE_PATH, we have a requirement to set both the paths( QUERY_BASE_PATH and QUERY_UI_BASE_PATH) at the same time, and the expectation is that query UI should be able to access using both paths. Is this valid use case and is it possible?

Member

yurishkuro commented Mar 26, 2024

This is a valid use case:

flowchart LR
    Browser -->|/foo/bar/...\nUI_BASE_PATH| Proxy
    Proxy -->|/baz/...\nBASE_PATH| JaegerQuery
    InternalClient -->|/baz/api/traces/...\nBASE_PATH| JaegerQuery

Author

Sidhartha-Biswal commented Mar 27, 2024

Hi @yurishkuro,

Just to clarify my understanding based on the code changes and above diagram, I am writing again here the below scenarios. Please correct it if I am wrong.

When proxy is involved and both the parameters are set, say UI_BASE_PATH = /foo/bar and BASE_PATH=/baz, Query-UI can only be accessed via UI_BASE_PATH. There is no possibility to access the Query-UI via BASE_PATH. Please correct it if I am wrong.
When proxy is not involved, there is no need to set the UI_BASE_PATH and Query-UI can be accessed via BASE_PATH
When proxy is not involved, Is it still a valid case to set both the parameters UI_BASE_PATH and BASE_PATH?

Member

yurishkuro commented Mar 27, 2024

There is no possibility to access the Query-UI via BASE_PATH. Please correct it if I am wrong.

Correct. Jaeger can serve only one version of the UI assets which are dynamically modified to match UI_BASE_PATH.

When proxy is not involved, there is no need to set the UI_BASE_PATH and Query-UI can be accessed via BASE_PATH

Correct, because Jaeger will simply default UI_BASE_PATH to BASE_PATH if the former is not set.

When proxy is not involved, Is it still a valid case to set both the parameters UI_BASE_PATH and BASE_PATH?

No. Traffic coming into Jaeger is only recognized if it's relative to BASE_PATH. If you try to access it via different URL you will get 404.

Author

Sidhartha-Biswal commented Mar 28, 2024

Hi @yurishkuro,

Thanks for all your support and inputs.
We have a new use-case for this requirement where we have two products "X" & "Y", both the products are currently using gateway1(has no-proxy i.e no-rewrite mechanism). Now product "Y" is migrating from gateway1(has no-proxy i.e no-rewrite mechanism) to gateway2( has proxy i.e rewrite mechanism). Both the products "X" & "Y" use a single configuration file where QUERY_BASE_PATH = /jaeger and QUERY_UI_BASE_PATH = /hub/jaeger. Since product "Y" is being migrated from gateway1 to gateway2, expectation is Query-UI has to be accessed using both the gateways parallelly.

We have tested accessing Query-UI for both the products using this configuration with the latest changes.

But currently when both values are set, Product "Y" using gateway2 is able to access Query-UI.
Product "X" using gateway1 and Product "Y" using gateway1 have the issue accessing Query-UI

Unfortunately we don't have a possibility to have a separate configuration for both the gateways. So in this case, do you think can we achieve this kind of use-case requirement from Jaeger side in any way?

Member

yurishkuro commented Mar 28, 2024

The only way to run UI under two external URLs is if the rewriting is taken care of by the proxy, not only on the way in, but in the responses as well (i.e. altering the returned HTML and static assets by replacing the prefix). I don't know how difficult the output rewriting would be and if proxies support that out of the box.

If you're in the middle of migration, I would just run a second instance of Jaeger UI, since it's stateless (unless you're using in-process storage like Badger, then you would need to reconfigure it as a remote storage so that it can be shared across multiple Jaeger processes).

Author

Sidhartha-Biswal commented Apr 9, 2024

Hi @yurishkuro,

Thanks for all the support. I am confused with the below two responses given by you. Can you please clarify them
For the below case:

1. When proxy is involved and both the parameters are set, say UI_BASE_PATH = /foo/bar and BASE_PATH=/baz, Query-UI can only be accessed via UI_BASE_PATH. There is no possibility to access the Query-UI via BASE_PATH. Please correct it if I am wrong.

But in the latest comment, you have mentioned that it is still possible to run UI under two external URL's if rewriting can be take care by proxy.....

The only way to run UI under two external URLs is if the rewriting is taken care of by the proxy, not only on the way in, but in the responses as well (i.e. altering the returned HTML and static assets by replacing the prefix). I don't know how difficult the output rewriting would be and if proxies support that out of the box.

The above two comments seem to be confusing & contradicting for me. Which one is correct?
Can you please clarify on this.

Author

Sidhartha-Biswal commented Apr 26, 2024

Hi @yurishkuro,

Thanks for the support, sorry for delayed reply we were busy with some other works, we wanted resume to this activity.
Currently we wanted to merge these changes which are working fine in proxy case in our environment.

But in Jaeger not sure how to test it, can you please suggest where tests needs to be added and how it should be tested.


Merge branch 'main' into main

d911507

Member

yurishkuro commented Apr 26, 2024

the end to end test is here: https://github.com/jaegertracing/jaeger/tree/main/examples/reverse-proxy

Author

Sidhartha-Biswal commented May 2, 2024

Hi @yurishkuro,

Thanks for the inputs.
In our environment with a proxy setup when we try to test with the latest changes, we don't see any issue accessing the Jaeger-UI. But in order to manually test using the reverseproxy example provided by you above, we need to build the jaeger-all-in-one image with our changes I guess. But when we try to run the ./build-all-in-one-image in our local, we are facing some issues with node.js version while building it. We are trying to fix it. But in meantime, we observed that a job was triggered for the commit where different stages like build, test, publish were executed. So if possible can you please provide us the jaeger-all-in-one-image tag with our changes. So that we can use that image and try to execute the docker compose up and check if we are able to access the Jaeger-UI using the UI-Base-Path.

Sidhartha-Biswal added 2 commits

May 3, 2024 15:58


Update httpd.conf

Signed-off-by: Sidhartha-Biswal <[email protected]>


Update docker-compose.yml

77313ea

Signed-off-by: Sidhartha-Biswal <[email protected]>

Author

Sidhartha-Biswal commented May 3, 2024

Hi @yurishkuro,

We were going through the reverse-proxy file shared by you above. We have made the the changes in the PR as per our understanding. We are facing some issues while building the jaeger-all-in-one-image locally which is preventing us from manually checking the Jaeger-UI. Can you please review the changes and let us know if we have to make some modifications in the test files.


Update docker-compose.yml

Signed-off-by: Sidhartha-Biswal <[email protected]>

Author

Sidhartha-Biswal commented May 7, 2024 •

edited

Hi @yurishkuro,

Thanks for all the inputs.
In our environment with a proxy setup when we try to test with the latest changes, we don't see any issue accessing the Jaeger-UI. The changes seems to be working fine as we are able to access the Jaeger-UI via UI-Base-Path.

But in order to manually test using the reverseproxy example provided by you above, we need to build the jaeger-all-in-one image with our changes I guess. But when we try to run the ./build-all-in-one-image in our local, we are facing one or other issue. Currently we are seeing the below issue.

We are trying to fix the above error. But in meantime, we observed that a job was triggered for the commit where different stages like build, test, publish were executed. So if possible can you please provide us the jaeger-all-in-one-image tag with our changes. So that we can use that image and try to execute the docker compose up and check if we are able to access the Jaeger-UI using the UI-Base-Path.

Can you please help us with this & also can you please confirm if the changes in this PR are fine?

yurishkuro mentioned this pull request

[Feature]: Support external URL prefix #5157

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment