aerokube / selenoid

Selenium Hub successor running browsers within containers. Scalable, immutable, self hosted Selenium-Grid on any platform with single binary.
https://aerokube.com/selenoid/latest/
Apache License 2.0
2.57k stars 321 forks source link

Support missing for JSON Version #1322

Open LukeIGS opened 1 year ago

LukeIGS commented 1 year ago

Some clients (for example, Ferrum for ruby) utilize this endpoint to gather version information about the browser, as well as the websocket debugger url.

vania-pooh commented 1 year ago

Probably we just need to support this: https://github.com/aerokube/selenoid/issues/1063

LukeIGS commented 1 year ago

in Ferrum's case at least, it doesn't seem to have any care "where" the websocket endpoint lives. Selenium for ruby doesn't really care beyond its initial checks either; However, for some reason if you bypass those checks it dies when you attempt to call anything using The bidirectional api with a network timeout.

If you bypass any of that both "will" connect to the endpoint, allowing you to execute raw cdp commands just fine, though for a good connection from Ferrum at least, we would need the json/version endpoint to be supported alongside the current json/protocal as per this spec, as ferrum uses the former to gather version information like the websocket endpoint for the cluster.

GET /json/version

{
    "Browser": "Chrome/72.0.3601.0",
    "Protocol-Version": "1.3",
    "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3601.0 Safari/537.36",
    "V8-Version": "7.2.233",
    "WebKit-Version": "537.36 (@cfede9db1d154de0468cb0538479f34c0755a0f4)",
    "webSocketDebuggerUrl": "ws://localhost:9222/devtools/browser/b0b8a4fb-bb17-4359-9533-a8d9f3908bd8"
}

Given this selenium and ferrum both don't really care where the websocket lives, but for whatever reason, Target.getTargets will always return an empty response... so while you can execute raw cdp commands just fine like Performance.getMetrics, any event based responses just time out.

Edit: Upon closer inspection, selenium's ruby code seems to look for the CDP websocket the same way, so just supporing json/version per spec would likely fix a lot of our CDP related woes.

LukeIGS commented 1 year ago

specs for relevance. https://chromedevtools.github.io/devtools-protocol

LukeIGS commented 1 year ago

Ferrum attempting to fetch this endpoint: https://github.com/rubycdp/ferrum/blob/5bf72255a1fa2b27783e3549c13ccbd83529154c/lib/ferrum/browser/process.rb#L66

Selenium-webdriver for ruby also attempting to fetch this endpoint: https://github.com/SeleniumHQ/selenium/blob/trunk/rb/lib/selenium/webdriver/chromium/driver.rb#L50

LukeIGS commented 1 year ago

Looks like the change would take place here... https://github.com/aerokube/images/blob/e943c3a5f4706ca3b8a5d284fc09f63b642e0806/static/chrome/devtools/devtools.go#L81

Selenoid's CDP router seems to only define json/protocol, it should probably be more up to the linked specs and support the following at the bare minimum: GET /json/list and GET /json - used by multiple cdp clients to get a target list for bidirectional apis GET /json/version - Used to describe version information and websocket location WebSocket /devtools/page/{targetId} - Used to attach to a given taget via websocket

Though it might be a good idea to just follow the specs completely for CDP functionality. se:cdp is a vendor namespaced extension, if selenoid wants to masquerade as selenium, we would need that defined, as it would just make it compatible with a lot of clients out of box, even if it's not in the w3c spec, however that's a separate concern than this is I think.

vania-pooh commented 1 year ago

@LukeIGS we were aware of these API when this binary was initially created. However the issue is that all these APIs return WebSocket URLs pointing to 127.0.0.1 whereas in Selenoid case this should be Selenoid host and some session ID. So we decided to not patch response of this URL as too complicated operation.

LukeIGS commented 1 year ago

would it be possible to simply take the incoming host of the request and return that as part of the response? I believe that's how selenium handles it.

vania-pooh commented 1 year ago

@LukeIGS what do you mean by "incoming host"? How is this expected to work when our tools are running behind one or several reverse-proxies and behind a fault-tolerant load balancer?

LukeIGS commented 1 year ago

most load balancers and proxies should be forwarding the host header [of the request] to the service in question given they are configured in a sane way. Granted this wouldn't work if there were context paths involved, at least not on its own. The proxy shoudl also be forwarding the path header in most cases of the request i'd expect.

LukeIGS commented 1 year ago

this is all especially true if you're using any sort of https termination, as the proxy would need to forward tls encrypted requests to the service stack in question (be it kubernetes or docker), and the host header is used to compare against the expected host of a served tls certificate.

LukeIGS commented 1 year ago

quick sketch of a go function that would probably do what we need

type versionTransport struct {
    http.RoundTripper
}

func (t *versionTransport) RoundTrip(req *http.Request) (resp *http.Response, err error) {
    resp, err = t.RoundTripper.RoundTrip(req)
    if err != nil {
            return nil, err
    }
    b, err := ioutil.ReadAll(resp.Body)
    if err != nil {
            return nil, err
    }
    err = resp.Body.Close()
    if err != nil {
            return nil, err
    }
    b = bytes.Replace(b, 
        []byte("\"webSocketDebuggerUrl\": \"ws://localhost:9222"), 
        []byte( fmt.Sprint("\"webSocketDebuggerUrl\": \"ws://%s", req.Host)), 
        -1,
    )
    body := ioutil.NopCloser(bytes.NewReader(b))
    resp.Body = body
    resp.ContentLength = int64(len(b))
    resp.Header.Set("Content-Length", strconv.Itoa(len(b)))
    return resp, nil
}

func version(w http.ResponseWriter, r *http.Request) {

    h, err := devtoolsHost()
    if err != nil {
        log.Printf("[DEVTOOLS_HOST_ERROR] [%v]", err)
        http.Error(w, fmt.Sprintf("Failed to detect devtools host: %v", err), http.StatusInternalServerError)
        return
    }
    u := &url.URL{
        Host:   h,
        Scheme: "http",
        Path:   "/json/version",
    }
    log.Printf("[PROTOCOL] [%s]", u.String())
    (&httputil.ReverseProxy{
        Director: func(r *http.Request) {
            r.Host = "localhost"
            r.URL = u
        },
        Transport: &versionTransport{http.DefaultTransport},
    }).ServeHTTP(w, r)
}

With my (albeit) limited knowledge of go, this function would probably do something akin to what we'd need provided we were working with a proxy that was routing on host alone..

I'm sure someone with better go would be able to write something way better though.

andrii-rymar commented 1 year ago

@LukeIGS have you tried to build a custom Selenoid version with the changes you proposed?

LukeIGS commented 1 year ago

I have not. I've mostly been switching to just using playwright instead of w3c driver based solutions.

github-actions[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Juice10 commented 7 months ago

I'd still love support for Ferrum if possible, but it might be worth considering playwright-ruby-client instead (haven't done that yet, it seems much less popular than Ferrum).

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Juice10 commented 5 months ago

bump

vania-pooh commented 5 months ago

PRs are welcome.