feat(coderd/agentapi): allow inserting apps for sub agents #18129
Conversation
Allow the creation of workspace apps when creating a sub agent.
I think the dbauthz change is out-of-scope for this change
| message App { | ||
| message Healthcheck { | ||
| int32 interval = 1; | ||
| int32 threshold = 2; | ||
| string url = 3; | ||
| } | ||
|
|
||
| enum OpenIn { | ||
| SLIM_WINDOW = 0; | ||
| TAB = 1; | ||
| } | ||
|
|
||
| enum Share { | ||
| OWNER = 0; | ||
| AUTHENTICATED = 1; | ||
| PUBLIC = 2; | ||
| } | ||
|
|
||
| string slug = 1; | ||
| optional string command = 2; | ||
| optional string display_name = 3; | ||
| optional bool external = 4; | ||
| optional string group = 5; | ||
| optional Healthcheck healthcheck = 6; | ||
| optional bool hidden = 7; | ||
| optional string icon = 8; | ||
| optional OpenIn open_in = 9; | ||
| optional int32 order = 10; | ||
| optional Share share = 11; | ||
| optional bool subdomain = 12; | ||
| optional string url = 13; | ||
| } |
There was a problem hiding this comment.
I'm aware we already have a WorkspaceApp structure defined in agent.proto, but that is used as part of the Manifest response structure.
The reasoning for creating a new structure is that the existing WorkspaceApp is for an existing workspace app, whereas this new version is for requesting to make a new workspace app.
The design of this structure is based on the terraform provider coder_app resource.
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to insert apps when creating sub agents by extending the API, database logic, and corresponding proto definitions.
- Added a new test case ("CreateSubAgentWithApps") validating sub-agent creation with associated apps.
- Updated the CreateSubAgent function to iterate over and insert apps with validations.
- Extended the proto definition to include a new App message with nested Healthcheck and enums for OpenIn and Share.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| coderd/agentapi/subagent_test.go | Added tests for creating sub agents including the apps parameter with various scenarios. |
| coderd/agentapi/subagent.go | Added business logic to process and insert the apps passed via the CreateSubAgent request. |
| agent/proto/agent.proto | Extended the proto message for CreateSubAgentRequest with a nested App message and related fields. |
There was a problem hiding this comment.
Because you add new types here, we should also update the comments in tailnet/proto/version.go to reflect this. It hasn't been shipped yet AFAIK so it should be OK to just keep the same version.
The other potential issue I see here is correctly surfacing validation errors from the CreateSubAgent() calls to the user. I don't see any calls to this yet, but we'll want to make sure that users are able to correctly address issues that cause validation to fail.
Instead of returning an untyped error, we instead return a typed error. This will allow matching on the error at the call site, allowing it to be dispayed nicely to the user.
@johnstcn As the new types added are all under the |
@DanielleMaywood fine with me 👍 |
| }) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("insert workspace app: %w", err) | ||
| } |
There was a problem hiding this comment.
There are a lot of paths where we may error out here, do we want to prevent sub agent creation even if an app is wrongly configured? This makes backwards compatibility also harder where if we ever have to change the customize structure, sub agents may start failing rather than individual apps.
There was a problem hiding this comment.
I can definitely see where you're coming from though I'm not sure which approach is the best. @johnstcn what are your thoughts here?
There was a problem hiding this comment.
Have made it so the creation of an agent will not fail if an app fails to be created 👍
Wrap the calls to InsertWorkspaceAgent and InsertWorkspaceApp in a transaction so that they either all succeed or none succeed.
This change allows an agent to still be created and returned, even when an app fails to be created. This is made possible by returning each individual app creation error.
coderd/agentapi/subagent.go
Outdated
| health := database.WorkspaceAppHealthDisabled | ||
| if app.Healthcheck == nil { | ||
| app.Healthcheck = &agentproto.CreateSubAgentRequest_App_Healthcheck{} | ||
| } | ||
| if app.Healthcheck.Url != "" { | ||
| health = database.WorkspaceAppHealthInitializing | ||
| } | ||
|
|
||
| sharingLevel := database.AppSharingLevelOwner | ||
| switch app.GetShare() { | ||
| case agentproto.CreateSubAgentRequest_App_AUTHENTICATED: | ||
| sharingLevel = database.AppSharingLevelAuthenticated | ||
| case agentproto.CreateSubAgentRequest_App_PUBLIC: | ||
| sharingLevel = database.AppSharingLevelPublic | ||
| } | ||
|
|
||
| openIn := database.WorkspaceAppOpenInSlimWindow | ||
| if app.GetOpenIn() == agentproto.CreateSubAgentRequest_App_TAB { | ||
| openIn = database.WorkspaceAppOpenInTab | ||
| } |
There was a problem hiding this comment.
Perhaps we should skip the default and have switch for each type with a default that returns an error? Thinking about future changes to the API and potential silent breakages/incorrect behavior that will go unnoticed otherwise.
| }() | ||
| if err != nil { | ||
| appErr := &agentproto.CreateSubAgentResponse_AppCreationError{ | ||
| Index: int32(i), //nolint:gosec // This would only overflow if we created 2 billion apps. |
Relates to coder/internal#614
Introduces a new field to the
CreateSubAgentrequest. This new field allows adding workspace apps to a sub agent at the point of creation.