[🚀 Feature]: Allow user to provide a different base URL for the grid UI

StellarNear commented 1 year ago

Feature and motivation

For the moment the base URL of a grid is locked to be /ui/ at root level. For my usage i had to manually change the path in the file : selenium/grid-ui/package.json at line 38 "homepage": "/ui" to "homepage": "/selenium/ui"

Because for my usage the base URL host multiple services (so i have my URL like this www.myUrl.com/selenium/ui)

Currently i found no other way to provide the grid a custom base without having to recompile everything after a manual change.

Usage example

The grid could simply read an environment variable such as GRID_BASE_URL and if this variable exist use it instead of the /ui written in the code

github-actions[bot] commented 1 year ago

@StellarNear, thank you for creating this issue. We will troubleshoot it as soon as we can.

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

krmahadevan commented 1 year ago

@StellarNear i remember adding this capability as part of https://github.com/SeleniumHQ/selenium/pull/11271

Can you please check this once again on 4.8.0?

StellarNear commented 1 year ago

@krmahadevan Oh i didn't catched this new feature (i was using an older version of the grid) So i will have to add --sub-path as a launching argument for the router jar part and it will do all the configuration even for the JS redirect of everything in the UI of the grid from the front end ?

krmahadevan commented 1 year ago

@StellarNear yes. Can you please try and post back if you noticed any gaps? All selenium grid internal Urls should work with the prefix now.

StellarNear commented 1 year ago

Ok i did test with the sub-path option and the UI of the grid is still hardcoded. For i used --sub-path /selenium example i got some 404 because the js and other files are searched into path /ui/static and not /selenium/ui/static <!doctype html><html lang="en"><head><meta charset="utf-8"/><link href="/ui/favicon.svg" rel="icon" type="image/svg"><meta content="width=device-width,initial-scale=1" name="viewport"/><link href="/ui/logo192.png" rel="apple-touch-icon"/><link href="/ui/manifest.json" rel="manifest"/><title>Selenium Grid</title><script defer="defer" src="/ui/static/js/main.415a1a1c.js"></script><link href="/ui/static/css/main.ce2bf04a.css" rel="stylesheet"></head><body><noscript>You need to enable JavaScript to run this app.</noscript><div id="root"></div></body></html>

You can see the code of the page still has href etc pointing to /ui without any concern of my rebasing. So for now i will still have to recompile everything which is sad because i want to be able to use directly your image.

As a final tough i think your feature is a big nice start to what i need but a little step will be needed to allow the UI to use the subpath also.

diemol commented 1 year ago

@StellarNear Can you share the details showing how you tried it? I tested the feature and it was working both the UI and the commands.

StellarNear commented 1 year ago

i did a kubernetes deployment like this one :

apiVersion: v1
kind: Service
metadata:
  name: selenium-hub
  namespace: default
  labels:
    app.kubernetes.io/name: selenium
    app.kubernetes.io/component: hub
spec:
  selector:
    app.kubernetes.io/name: selenium
    app.kubernetes.io/component: hub
  ports:
    - name: web
      protocol: TCP
      port: 4444
      targetPort: 4444
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: selenium-hub
  namespace: default
  labels:
    app.kubernetes.io/name: selenium
    app.kubernetes.io/component: hub
spec:
  replicas: 1
  selector:
    matchLabels:
      app.kubernetes.io/name: selenium
      app.kubernetes.io/component: hub
  template:
    metadata:
      labels:
        app.kubernetes.io/name: selenium
        app.kubernetes.io/component: hub
    spec:
      containers:
        - name: selenium-hub
          image: selenium/hub:4.8.0-20230210
          imagePullPolicy: "IfNotPresent"
          env:
            - name: SE_BIND_HOST
              value: "false"
            - name: SE_SESSION_REQUEST_TIMEOUT
              value: "600"
            - name: SE_SESSIONQUEUE_REQUEST_TIMEOUT
              value: "600"
            - name: SE_SESSION_RETRY_INTERVAL
              value: "5"
            - name: SE_OPTS
              value: "--registration-secret 42 --sub-path /selenium"
          ports:
            - containerPort: 4444
              protocol: TCP
          resources:
            limits:
              cpu: "0.5"
              memory: 384Mi
            requests:
              cpu: "0.25"
              memory: 256Mi

The pod starts nicely and run like it supposed to i guess since the log show that is up and running like you can see in this log :

15:24:32.034 INFO [BoundZmqEventBus.<init>] - XPUB binding to [binding to tcp://*:4442, advertising as tcp://**.***.*.**:4442], XSUB binding to [binding to tcp://*:4443, advertising as tcp://**.***.*.**:4443]
15:24:32.327 INFO [UnboundZmqEventBus.<init>] - Connecting to tcp://**.***.*.**:4442 and tcp://**.***.*.**:4443
15:24:32.417 INFO [UnboundZmqEventBus.<init>] - Sockets created
15:24:33.419 INFO [UnboundZmqEventBus.<init>] - Event bus ready
15:24:36.613 INFO [Hub.execute] - Started Selenium Hub 4.8.0 (revision 267030adea): http://**.***.*.**:4444

yashcho commented 1 year ago

Even to me it is the case, as I am doing it through config.toml file, below is content

[network] sub-path = "selenium"

Endpoints such as /static/js and /static/css is working with /ui instead of /selenium/ui

Note : This is strange, if I remember well when I tested the 4.8.0 jar on 31st Jan and 1st Feb 2023 it was working, suddenly I started to see this issue when I checked today again.

yashcho commented 1 year ago

Any hints @diemol / @krmahadevan ?

krmahadevan commented 1 year ago

@yashcho - I dont have a k8s cluster at my disposal and minikube is having some issues at my end.

I did try to spawn the hub using java -jar selenium-server-4.8.1.jar hub --sub-path "krishnan" and then load up the UI and it seems to be fine for me.

diemol commented 1 year ago

Yes, I was trying the same as @krmahadevan and it works. It also works when using the Docker images.

Please check if you need to do some extra routing in Kubernetes.

EDIT: Command to try it with Docker

docker run -d -p 4444:4444 -e SE_OPTS="--sub-path /diego" --shm-size="2g" selenium/standalone-firefox:4.8.1-20230221

yashcho commented 1 year ago

Thanks @krmahadevan and @diemol, I tried once again and now it seems I found the issue.

So when I open the Hub UI URL without adding the sub-path name manually it works i.e. "http://localhost:4444/" it redirects to "http://localhost:4444/selenium/ui", but if I provide sub-path in URL it won't work i.e. "http://localhost:4444/selenium" it redirects to "http://localhost:4444/selenium/ui" but will show blank page and above error will pop-up.

Can you confirm on this?

diemol commented 1 year ago

No, still works.

issue-11647

StellarNear commented 1 year ago

I did strip all my kubernetes ingress to the strict minimum to be as close as possible to the docker usage. I can confirm that it now redirect nicely to the /ui . But i have a blank page an di can see that it's still trying to reach files from the static folder without any repath see on the screenshot below (404 Not Found https://integration-dev.k8s.qct/ui/static/js/main.415a1a1c.js)

diemol commented 1 year ago

OK, that is probably a bug. Because it should be able to re-route all requests. Most likely this is the webapp directly.

yashcho commented 1 year ago

Even for graphql endpoint that is the case.

yashcho commented 1 year ago

@diemol should there be a separate issue created for it or will you be checking on the same?

diemol commented 1 year ago

We can use this one. However there is no time frame where this will be done.

yashcho commented 1 year ago

I confirm that selenium hosted under a defined endpoints for a route or with only selenium endpoint as context-path/sub-path does not re-route all internal endpoints and fails to load UI.

So one has to add endpoints such as ui, graphql and selenium to route with defined endpoints or run under root path to work,

If possible can you point me to the code where the bug might be so that I can try to fix and send you a PR?

@diemol / @krmahadevan I tried to reproduce the same scenario under docker where the route has only limited/defined endpoint registered and it doesn't work. Below are some snips

Note : A plain docker image works which is not under defined endpoint route, because there it has no restriction on endpoints everything can start with root context.

yashcho commented 1 year ago

I tried to dig into the code and found two places (below are links) were this subpath need to be added for rerouting the internal endpoints, currently I have hardcode them as "selenium" on my local branch

https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/grid-ui/package.json#L38

https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/grid-ui/src/config.ts#L12

@krmahadevan / @diemol if you have any hint on how those places can have dynamic value based on sub-path flag it will be helpful to frame the PR. :-)

krmahadevan commented 1 year ago

I tried to dig into the code and found two places (below are links) were this subpath need to be added for rerouting the internal endpoints, currently I have hardcode them as "selenium" on my local branch

https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/grid-ui/package.json#L38

https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/grid-ui/src/config.ts#L12

@krmahadevan / @diemol if you have any hint on how those places can have dynamic value based on sub-path flag it will be helpful to frame the PR. :-)

Right now the GridUiRoute`` merely retrieves them using a ClassPathResource which internally fetches the content from the jar.

We could do something like this

Extend org.openqa.selenium.grid.web.JarFileResource and call it RootAwareJarFileResource
Enhance org.openqa.selenium.grid.web.ClassPathResource#ClassPathResource to accept the prefix as well
If prefix was given it would use RootAwareJarFileResource and if prefix was not provided it would use JarFileResource
The read() method within RootAwareJarFileResource would ensure that it invokes super.read() to get the contents and then it does a find/replace and fixes the prefixes properly on the retrieved bytes, and then returns back the resource.

I think that should help take care of this.

yashcho commented 1 year ago

Thanks @krmahadevan, I have almost done with 3rd step but for 4th step I am not able to understand below points :

Why read() method as I don't see any footprints in JarFileResource constructor?
Do we need to enhance JarFileResource and RootAwareJarFileResource class to accept prefix and change accordingly?
Should the byte array again be converted in String and then find+replace operation be applied?

It would be good if you answer above questions, so that I can finalize my branch and send PR. :)

krmahadevan commented 1 year ago

@yashcho - I was suggesting something like this

  public GridUiRoute(String prefix) {
    Require.nonNull(prefix, "Prefix cannot be null");
    URL uiRoot = GridUiRoute.class.getResource(GRID_RESOURCE_WITH_PREFIX);
    if (uiRoot != null) {
      ResourceHandler uiHandler = new ResourceHandler(new ClassPathResource(uiRoot, GRID_RESOURCE, prefix));
      HttpResponse uiRedirect = new HttpResponse()
        .setStatus(HTTP_MOVED_TEMP)
        .addHeader("Location", prefix.concat("/ui"));

      Supplier<HttpHandler> redirectHandler = () -> req -> uiRedirect;

      Routable appendRoute = Route.combine(
        consoleRoute(prefix, redirectHandler),
        uiRoute(prefix, () -> uiHandler)
      );
      if (!prefix.isEmpty()) {
        appendRoute = Route.combine(appendRoute, redirectRoute(prefix, redirectHandler));
      }

      routes = Route.combine(get("/").to(redirectHandler), appendRoute);
    } else {
      LOG.warning("It was not possible to load the Grid UI.");
      Json json = new Json();
      routes = Route.matching(req -> false).to(() -> new NoHandler(json));
    }
  }

ClassPathResource constructor would look like

  public ClassPathResource(URL resourceUrl, String stripPrefix, String prefix) {

    Require.nonNull("Resource URL", resourceUrl);
    Require.nonNull("Prefix to strip", stripPrefix);

    if ("jar".equals(resourceUrl.getProtocol())) {
      try {
        JarURLConnection juc = (JarURLConnection) resourceUrl.openConnection();
        JarFile jarFile = juc.getJarFile();

        this.delegate =
          (prefix == null || prefix.isEmpty()) ? new JarFileResource(jarFile, juc.getEntryName(), stripPrefix) :
            new PrefixAwareFileResource(jarFile, juc.getEntryName(), stripPrefix, prefix);
      } catch (IOException e) {
        throw new UncheckedIOException(e);
      }
    } else {
      throw new IllegalArgumentException("Unable to handle scheme of type " + resourceUrl.getProtocol());
    }
  }

PrefixAwareFileResource would look like below

import java.util.Optional;
import java.util.jar.JarFile;

public class PrefixAwareFileResource extends JarFileResource {

  private final String prefix;

  public PrefixAwareFileResource(JarFile jarFile, String entryName,
    String stripPrefix, String prefix) {
    super(jarFile, entryName, stripPrefix);
    this.prefix = prefix;
  }

  @Override
  public Optional<byte[]> read() {
    Optional<byte[]> read = super.read();
    //Add logic to transform contents which would do the following
    //1. Translate the byte[] into string
    //2. Do find/replace to adjust the urls such that "prefix" is prefixed
    //3. Convert the string back into byte[] array
    return read;
  }
}

yashcho commented 1 year ago

More than thanks for pseudo code, so I was on right track let me send the PR after testing.

yashcho commented 1 year ago

So @krmahadevan I have added the logic and tested the jar, but it seems this read() method is never invoked from Class PrefixAwareJarFileResource.

Did I missed some place?

krmahadevan commented 1 year ago

@yashcho - I am not sure what you mean. the read() method in the Resource interface is there to facilitate reading from a resource. So it should get called. You may be missing something at your end. Please double check to track the paths that call the read() method from the Resource interface. That should help get started.

yashcho commented 1 year ago

Thanks I tried to debug the code and found the flow still checking why the logs below super.read() are not getting visible in logs console.

yashcho commented 1 year ago

One point to inform @krmahadevan if I add the subpath statically in package.json and config.tsx file and check the path URL in logs coming from JarFileResource class file or more specifically read method. It shows no addition/prefix of subpath (Below is one sample), do you think it is right way?

javascript/grid-ui/build/index.html

Note : It is accessible and under browser developer tools I see URL is prefixed with subpath

krmahadevan commented 1 year ago

@yashcho - You mean you altered the files that were present in https://github.com/SeleniumHQ/selenium/tree/trunk/javascript/grid-ui and then you built the selenium grid using bazel build grid and then setup debug points wherein you are seeing this ?

Those files are basically being served from within the jar. I am not sure how you are running your tests as well. So its hard for me to comment.

yashcho commented 1 year ago

Sorry for not giving context :

I have altered file in https://github.com/SeleniumHQ/selenium/tree/trunk/javascript/grid-ui and added LOGGER in JarFileResource class file then built selenium grid jar using bazel build grid

Once the jar is built I map it in docker container which is setup based on the requirement I added above. So once I access the URL over browser under container console Entry path are shown as below but if I open developer tool under Network section all the URL are changed with subpath added explicitly for file in grid-ui folder .

javascript/grid-ui/build/index.html

krmahadevan commented 1 year ago

@yashcho - What is the question. I am now confused. Are you asking if the fix that I proposed was the right one? I dont have a definite answer (it was a calculated guess based on my limited understanding of the codebase). I haven't taken a jab at this for this use case, but going by the flow, that was the logical place wherein the change was to be done.

yashcho commented 1 year ago

Yeah one question was the fix you proposed was the right one? (Which you answered already), second was the Logs getting printed is right? (Which I suppose will be hard to answer)

But in general I got the point, what you want me to look and fix. Let me check where should I "prefix" to url path to get it prefixed. :)

StellarNear commented 1 year ago

Any news on the feasibility of this ? i didn't go on the code as deep as you but since it can be done by "just" changing the line 38 of the field "homepage" on the file selenium/grid-ui/package.json and after that if you rebuild everything works but yea i guess that making it dynamic in code will be longer but i guess you can find a way :)

yashcho commented 1 year ago

So in general I completed with code in regards to java, but the CSS part pops warning for fetching the fonts so finding a way to fix it still. Will try to work on it today..

yashcho commented 1 year ago

Hello @krmahadevan and @diemol,

I have created the MR, please let me know my mistakes.. :-)

codyja commented 1 year ago

This is great work! I'm curious if this will allow for the subpath in front of the /graphql endpoint as well?

yashcho commented 1 year ago

Yes @codyja it will work for both endpoint i.e. /ui and /graphql, currently doing this works but the dynamic css throws the warning, and for that I am not sure were to change more.

StellarNear commented 1 year ago

Awesome work ! :) Should we expect this commit to be merged in the next version ? or is the reviewing/merging process long ?

StellarNear commented 1 year ago

By the way, @yashcho did you test if your fix also redirect nicely the live video freed of sessions ? (it was also not working with the current code)

yashcho commented 1 year ago

Sorry @StellarNear I would not be able to send the video as I am testing the functionality in a internal project. If you can send the issue from new jar based on my MR codebase. I can try to give you the resolution.

JordanZimmitti commented 1 year ago

@yashcho The work you have done here is fantastic! We are also having an issue where the video feed of live sessions is not showing up in the UI. Have you had a chance to look into this at all?

yashcho commented 1 year ago

@JordanZimmitti thanks for compliments.. :)

In respect to your question, no I didn't checked that part but most probably @diemol have checked it because my MR was closed and not merged. As @diemol did some easy fix.. :)

StellarNear commented 1 year ago

not merged ? so your fix will not be released in the next 4.9.0-java ?

yashcho commented 1 year ago

Nope.. it will be this commit done by @diemol for 4.9.0-java release I suppose but you correct me @diemol .. :)

JordanZimmitti commented 1 year ago

@yashcho Ahh so some changes are not released yet? Do you know if they are they slated to make it into the next release?

diemol commented 1 year ago

This has already been released.

JordanZimmitti commented 1 year ago

@diemol Is there something special that needs to be configured in the helm chart to get the video feed of live sessions to work?

yashcho commented 1 year ago

This has already been released.

May I know in which version, because release notes of 4.9 doesn't contain this detail?

diemol commented 1 year ago

@diemol Is there something special that needs to be configured in the helm chart to get the video feed of live sessions to work?

Helm charts do not support video for now.

diemol commented 1 year ago

This has already been released.

May I know in which version, because release notes of 4.9 doesn't contain this detail?

Have you checked the most recent post? https://www.selenium.dev/blog/2023/selenium-4-9-0-released/

SeleniumHQ / selenium

[🚀 Feature]: Allow user to provide a different base URL for the grid UI #11647

Feature and motivation

Usage example