-
Notifications
You must be signed in to change notification settings - Fork 323
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
Support missing for JSON Version #1322
Comments
Probably we just need to support this: #1063 |
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.
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. |
specs for relevance. |
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: |
Looks like the change would take place here... 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: Though it might be a good idea to just follow the specs completely for CDP functionality. |
@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. |
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. |
@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? |
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. |
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. |
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. |
@LukeIGS have you tried to build a custom Selenoid version with the changes you proposed? |
I have not. I've mostly been switching to just using playwright instead of w3c driver based solutions. |
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. |
I'd still love support for Ferrum if possible, but it might be worth considering |
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. |
bump |
PRs are welcome. |
Some clients (for example, Ferrum for ruby) utilize this endpoint to gather version information about the browser, as well as the websocket debugger url.
The text was updated successfully, but these errors were encountered: