feat: add endpoint to get listening ports in agent #4260
Conversation
| agentConn, release, err := api.workspaceAgentCache.Acquire(r, workspaceAgent.ID) | ||
| if err != nil { | ||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Internal error dialing workspace agent.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| defer release() | ||
|
|
||
| portsResponse, err := agentConn.ListeningPorts(ctx) | ||
| if err != nil { | ||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Internal error fetching listening ports.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
What do you think about pushing port data instead of pulling it? This seems like it could lead to a lot of workspace traffic with page reloads, and we've learned from v1 that dynamic data like this (especially on main pages) can be sketchy.
There was a problem hiding this comment.
This will only be loaded when people click the port forward button. There will be more traffic if workspaces push it to coderd rather than if we load it when the user clicks the button which won't be that often.
There was a problem hiding this comment.
To add to this, I talked with dean and I think the syscall overhead is way too much for a responsive push system for such a small feature like this. Because we cache the response for 1 second on the agent side this already has built in protection for the workspace.
codersdk/agentconn.go
Outdated
| } | ||
|
|
||
| func (c *AgentConn) ListeningPorts(ctx context.Context) (ListeningPortsResponse, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://agent-stats/api/v0/listening-ports", nil) |
There was a problem hiding this comment.
Can we make this something less magical and explains to the reader that this calls out to localhost:4?
There was a problem hiding this comment.
yep! done by making a doStatisticsRequest method that does the magic for you instead
| lp := &listeningPortsHandler{} | ||
| r.Get("/api/v0/listening-ports", lp.handler) |
There was a problem hiding this comment.
Instead of creating an API for statistics, we should just handle this single port for right now on a handler. We probably don't even need Chi and can just directly serve it.
There was a problem hiding this comment.
@f0ssel and I would like to convert the stats websocket code in the agent to be in this webserver too, which is why it has path based routing. I've only reserved 8 ports for Coder so we can't just create a new http server for each function
This reverts commit d9635a8.
Similar to #1824 but done through a HTTP server and a HTTP endpoint rather than a raw TCP protocol.
This will be used for showing a list of currently listening ports in the port-forwarding popup in the dashboard.
4on tailnet agent which serves a HTTP server called the "statistics server", we will probably add resource usage and other handlers here as time goes on. I opted for a HTTP server as it feels simpler to me than implementing request/response over TCP by hand, and we don't need streaming support as ports don't change super often./api/v0/listening-portsto the agent statistics server which returns all currently listening TCP ports. This result is cached in the agent for 1 second. Ports above9(inclusive) will be returned.9(inclusive) on port forwarding traffic via devurls. This is not enforced for CLI port-forwarding intentionally./api/v2/workspaceagents/{id}/listening-portson coderd to get listening ports. This just connects to the corresponding agent on the statistics server and forwards the response.Co-authored-by: Asher asher@coder.com