Skip to content
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.

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

[Bug] Subviews like /recordings/create are not accessible when visited directly #1284

Open
tthvo opened this issue Jun 12, 2024 · 11 comments
Open
Labels
bug Something isn't working

Comments

@tthvo
Copy link
Member

tthvo commented Jun 12, 2024

Current Behavior

If I directly visit views by specifying their paths such as:

/topology/create-custom-target
/rules/create
/recordings/create

For example, https://cryostat-sample-myns.apps-crc.testing/recordings/create. A blank page is showed:

image

with the following console's error:

image

Expected Behavior

The sub views should render correctly!

Steps To Reproduce

  • Launch an OpenShift instance. For example, with OpenShift Local, crc start.
  • On latest cryostat-operator's commit on main:
    make deploy_bundle create_cryostat_cr sample_app

Environment

CRC version: 2.26.0+233df0
OpenShift version: 4.13.9
OS: Fedora 40

Anything else?

This only happens if I visit the sub-view directly. That is: If I go /recordings -> Click Create -> /recordings/create, all is fine.

@tthvo tthvo added the bug Something isn't working label Jun 12, 2024
@tthvo tthvo moved this to Todo in 3.0.0 release Jun 12, 2024
@andrewazores
Copy link
Member

Seems like going to /recordings/create makes the client try to look for the other asset bundles relative to /recordings instead of relative to /, so it tries to load /recordings/runtime.abcd1234.bundle.js instead of /runtime.abcd1234.bundle.js. Cryostat doesn't recognize this path so it responds with the index.html again, which results in the MIME type and SyntaxError errors.

Probably happening since #1175 then.

@andrewazores andrewazores moved this from Todo to Stretch Goals in 3.0.0 release Jun 12, 2024
@andrewazores
Copy link
Member

Now that the standard Cryostat deployment includes an auth proxy, it might be possible to solve the original https://github.com/cryostatio/cryostat/issues/1810 using an auth proxy configuration plus Route/Ingress path configuration, and back out #1175.

@andrewazores andrewazores moved this to Backlog in 4.0.0 release Jun 12, 2024
@andrewazores andrewazores changed the title [Bug] Sub View are not accessible when visisted directly [Bug] Subviews are not accessible when visited directly Jun 12, 2024
@andrewazores
Copy link
Member

This affects 2.4.1-snapshot too so I'm pretty sure #1175 is the culprit.

@tthvo
Copy link
Member Author

tthvo commented Jun 12, 2024

Seems like going to /recordings/create makes the client try to look for the other asset bundles relative to /recordings instead of relative to /, so it tries to load /recordings/runtime.abcd1234.bundle.js instead of /runtime.abcd1234.bundle.js. Cryostat doesn't recognize this path so it responds with the index.html again, which results in the MIME type and SyntaxError errors.
This affects 2.4.1-snapshot too so I'm pretty sure #1175 is the culprit.

Right, make sense! That explained the logged error :(

Now that the standard Cryostat deployment includes an auth proxy, it might be possible to solve the original https://github.com/cryostatio/cryostat/issues/1810 using an auth proxy configuration plus Route/Ingress path configuration, and back out #1175.

Just curious what's the plan for this fix?

@andrewazores
Copy link
Member

Currently no plan, that was just a very rough idea. It would be nice to have the ability to go directly to paths like /recordings/create, but not if it comes at the expense of becoming impossible to deploy Cryostat instances on paths other than /.

@tthvo
Copy link
Member Author

tthvo commented Jun 12, 2024

I am thinking if no errors with with the regular single level /recordings, we can make /recordings/create route into /create-recording for the sub view, basically flattening the route paths. That should help with the issue? Might not be the best UX.

@andrewazores
Copy link
Member

That's one potential way around, but I think it's pretty limiting. It makes sense right now because none of the UI paths are parameterized, so applying such a transformation looks like it works. It wouldn't work anymore if we had any paths like /topology/:nodeId (just a random example), and adopting such a scheme would block us from ever having paths like that in the future.

@tthvo
Copy link
Member Author

tthvo commented Jun 12, 2024

ah okayy pretty bad idea then :((

@andrewazores andrewazores changed the title [Bug] Subviews are not accessible when visited directly [Bug] Subviews like /recordings/create are not accessible when visited directly Jun 12, 2024
@tthvo
Copy link
Member Author

tthvo commented Jun 12, 2024

well it would break if I configure istio to route to Cryostat with a 2-level prefix /tools/cryostat... Is there a way to webpack/web-client to figure this out other than relative path .?

Not very urgent I guess but just curious from my end as I am looking into the istio stack :D

@andrewazores
Copy link
Member

Deploying Cryostat onto a path other than / isn't actually implemented yet, there are just a few pieces here and there that are working toward making that possible.

If there is some other router or reverse proxy in front of Cryostat (or in front of the auth proxy) then it should be possible to do this by having the router/proxy take requests like /foo/recordings, rewrite that to /recordings, and pass that request to the Cryostat container. By reconfiguring the router/proxy to change the /foo prefix, in theory this could work with / or /tools/cryostat or whatever path prefix is desired. This still won't work for /tools/cryostat/recordings/create though, because of the relative base href . issue we see here.

Backing out #1175 and adding in the router/proxy in front sounds like it could be a full solution to allow deployment to a non-root path as well as restoring direct links to deeper paths like /recordings/create.

If the auth proxy can be configured to do this, then great - we have our general-purpose upstream solution.

If it needs to be another proxy, either in front of the auth proxy or between the auth proxy and Cryostat, then we would need to have some more discussion about it. The additional container would need to be something we can disable, because if most users are not using this feature then it doesn't make sense to force them to deploy another container and pay for its footprint if it just ends up transparently passing requests.

@tthvo
Copy link
Member Author

tthvo commented Jun 12, 2024

For the cryostat service we have a VirtualService ( istio ) that will redirect the traffic sent to : http://my-k8-host/cryostat/ to the cryostat service and together with that also rewriting the url to / ( simply removing cryostat )

Ahh okayy! As in https://github.com/cryostatio/cryostat/issues/1810, istio can allow rewriting back the path to / so I guess it can depend on the service mesh solution that the users choose and we do document it. Though, in the end, like u said, the bug with , because of the relative base href . issue we see here. will sill persist. I would expect webpack to have a way to figure it out but no idea :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants