- Notifications
You must be signed in to change notification settings - Fork928
feat: add endpoint to get listening ports in agent#4260
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you think about pushing port data instead of pulling it? This seems like it could lead to alot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we make this something less magical and explains to the reader that this calls out tolocalhost:4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yep! done by making adoStatisticsRequest method that does the magic for you instead
Uh oh!
There was an error while loading.Please reload this page.
| lp := &listeningPortsHandler{} | ||
| r.Get("/api/v0/listening-ports", lp.handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 commitd9635a8.
Uh oh!
There was an error while loading.Please reload this page.
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: Asherasher@coder.com