feat: add AgentAPI using DRPC #10811
Conversation
|
This PR is getting huge so I'm going to leave it for now and I'll do the rest of the TODOs in follow ups. |
agent/proto/convert.go
Outdated
| } | ||
|
|
||
| var sharingLevel codersdk.WorkspaceAppSharingLevel | ||
| switch protoApp.SharingLevel { |
There was a problem hiding this comment.
One thing we could do to future proof here is introduce an AllWorkspaceAppSharingLevels slice in codersdk then walk this slice and match on
strings.ToLower(protoApp.SharingLevel.String())
There was a problem hiding this comment.
I did this with maps instead to avoid having to iterate over all enums every time
coderd/workspaceagents.go
Outdated
| func (api *API) workspaceAgentManifest(rw http.ResponseWriter, r *http.Request) { | ||
| // @Success 101 | ||
| // @Router /workspaceagents/me/rpc [get] | ||
| func (api *API) workspaceAgentRPC(rw http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
this function is way to long and hard to follow. It needs to be refactored into at least three pieces:
- Authz --- is this the latest build?
- Connection tracking --- pings, updating connection times, disconnect
- Serving dRPC
| return ensureLatestBuild, build, true | ||
| } | ||
|
|
||
| func (api *API) agentConnectionUpdate(ctx context.Context, workspaceAgent database.WorkspaceAgent, workspaceID uuid.UUID, conn *websocket.Conn) (func(closeCtx context.Context, ensureLatestBuildFn func() error), bool) { |
There was a problem hiding this comment.
This signature seems needlessly convoluted. Why do we need to return a function to the caller, for the caller to just call it once?
|
|
||
| // Ensure the resource is still valid! | ||
| // We only accept agents for resources on the latest build. | ||
| ensureLatestBuild := func() error { |
There was a problem hiding this comment.
making this function a closure makes it harder to understand because you need to pass this anonymous func() error around, out of this function and into the thing that handles the pings.
If you made it a regular function that accepted a database.Store and a database.WorkspaceBuild then it would simplify the return values of ensureLatestBuild and agentConnectionUpdate
| // We use a custom heartbeat routine here instead of `httpapi.Heartbeat` | ||
| // because we want to log the agent's last ping time. | ||
| var lastPing atomic.Pointer[time.Time] | ||
| lastPing.Store(ptr.Ref(time.Now())) // Since the agent initiated the request, assume it's alive. |
There was a problem hiding this comment.
This has the structure of storing a bunch of state in a top-level function and then creating several closures over that data, which are all inlined. I think it would be easier to understand if there were a struct that stores the data and then methods on the struct.
Creates
coderd/workspaceagentapi.gowhich implements DRPCagentproto.DRPCAgentServer. The server is "instanced" per agent (as it will be created upon receiving a websocket connection from the agent).TODO:
CoordinateTailnetI'm going to leave this one unimplemented for now and Spike can implement it when it's readyUpdates #10530