fix(client): use host from BaseURL for websockets

[thp-canonical]

Properly pass the Host (localhost for UNIX domain sockets, the host and port values for TCP sockets) to the websockets library.

[thp-canonical]

Manual test instructions: With the master branch checked out, apply the following patch to make Pebble connect via TCP (+ patch out access checks for exec-related endpoints that don't work with TCP yet):

diff --git a/cmd/pebble/main.go b/cmd/pebble/main.go
index d7ef4b8..22e67f1 100644
--- a/cmd/pebble/main.go
+++ b/cmd/pebble/main.go
@@ -19,10 +19,16 @@ import (
        "os"

        "github.com/canonical/pebble/internals/cli"
+       "github.com/canonical/pebble/client"
 )

 func main() {
-       if err := cli.Run(nil); err != nil {
+       opts := &cli.RunOptions{
+               ClientConfig: &client.Config{
+                       BaseURL: "http://localhost:9999/",
+               },
+       }
+       if err := cli.Run(opts); err != nil {
                fmt.Fprintf(cli.Stderr, "error: %v\n", err)
                os.Exit(1)
        }
diff --git a/internals/daemon/api.go b/internals/daemon/api.go
index 16c1db6..e3da75f 100644
--- a/internals/daemon/api.go
+++ b/internals/daemon/api.go
@@ -50,7 +50,7 @@ var API = []*Command{{
        POST:        v1PostChange,
 }, {
        Path:       "/v1/changes/{id}/wait",
-       ReadAccess: UserAccess{},
+       ReadAccess: OpenAccess{},
        GET:        v1GetChangeWait,
 }, {
        Path:        "/v1/services",
@@ -84,11 +84,11 @@ var API = []*Command{{
        GET:        v1GetLogs,
 }, {
        Path:        "/v1/exec",
-       WriteAccess: UserAccess{},
+       WriteAccess: OpenAccess{},
        POST:        v1PostExec,
 }, {
        Path:       "/v1/tasks/{task-id}/websocket/{websocket-id}",
-       ReadAccess: UserAccess{},
+       ReadAccess: OpenAccess{},
        GET:        v1GetTaskWebsocket,
 }, {
        Path:        "/v1/signals",

Build Pebble (master branch):

go build ./cmd/pebble

Launch a test instance:

mkdir pebble_dir
PEBBLE=$(pwd)/pebble_dir ./pebble run --verbose --http=127.0.0.1:9999

There's one error message that's unrelated to this websocket patch (it's because the client connects via TCP, and /v1/services has UserAccess{}, which isn't supported over TCP) -- the API endpoints that we need for exec are therefore patched using OpenAccess{} above:

<...> [pebble] 127.0.0.1:<...> POST /v1/services <...>µs 401
<...> [pebble] Cannot start default services: access denied

From another terminal, try connecting using a web socket:

./pebble exec sh
error: cannot connect to "control" websocket: dial tcp 127.0.0.1:80: connect: connection refused

(note that it tries to connect to port 80, i.e. ws://localhost/...)

Then apply the same diff as above to this branch, rebuild, start the Pebble daemon again, and try connecting with the client. This now works as expected over port 9999.

[benhoyt]

Thanks for the repro instructions.

It's definitely a little annoying that we have to add to the already-shipped client.Requester interface. However, looking over it, I'm not sure there's a way around it. Are we okay with this potential breakage? We have to have that value for creating new websockets (it's definitely not always localhost in the general case). So I think it was really an oversight in the original design of Requester. What do you think @flotter -- do you see any way to achieve this that doesn't involve adding a new Requester method?

[benhoyt]

For reference, Thomas's earlier fix (https://github.com/canonical/pebble/pull/389/files) worked without the change to client.Requester, but only because we were injecting a custom transport that overrides the host.

[thp-canonical]

It's definitely a little annoying that we have to add to the already-shipped client.Requester interface. However, looking over it, I'm not sure there's a way around it.

Well, we can store the host string in type Client instead, avoiding the need to change an interface. host can be a private member of Client and accessed from getTaskWebsocket just as well.

Assuming nobody injects a custom Requester for a client, having the host there shouldn't cause problems. However, then we're back to whether we want a potential implementer of the client.Requester interface fail loudly ("you need to implement the Host() method now") or silently fail potentially sometime in the future (Requester doing something fancy, but because getTaskWebsocket() uses host from client, it silently connects to the wrong endpoint until fixed - just like it does now).

389 is of course also an option, but do note that without additional work, the Host: header sent to the server will likely be fixed to "localhost" in the #389 case (fixable?) whereas here the Host: header should be the correct one. As long as we don't use/consume the Host: header on the server side, it shouldn't matter, but getting the Host: header right in the first place feels more "correct".

[benhoyt]

Well, we can store the host string in type Client instead, avoiding the need to change an interface. host can be a private member of Client and accessed from getTaskWebsocket just as well.

Ah yes, of course. That seems a lot better than changing the interface.

because getTaskWebsocket() uses host from client, it silently connects to the wrong endpoint until fixed

But if host string is on Client, getTaskWebsocket() will keep working in that case, right? We can still have the baseURL/host on Client, but even if later we allow a customisable Requester, it'll get the right host with the proposed change (adding host to Client) -- is that right?

In any case, adding Client.host string seems like a better direction to me.

[thp-canonical]

But if host string is on Client, getTaskWebsocket() will keep working in that case, right? We can still have the baseURL/host on Client, but even if later we allow a customisable Requester, it'll get the right host with the proposed change (adding host to Client) -- is that right?

Yes, that's right. Updated PR.