From cbe462789356d76099c3a40f641dc58fca79a104 Mon Sep 17 00:00:00 2001 From: Edward Angert Date: Fri, 20 Jun 2025 17:13:36 -0400 Subject: [PATCH 001/690] docs: document how to tag coder users in dx data cloud (#17805) [preview](https://coder.com/docs/@tag-coder-users-dx/admin/integrations/data-cloud) --------- Co-authored-by: EdwardAngert <17991901+EdwardAngert@users.noreply.github.com> --- docs/admin/integrations/dx-data-cloud.md | 87 ++++++++++++++++++++++++ docs/manifest.json | 5 ++ 2 files changed, 92 insertions(+) create mode 100644 docs/admin/integrations/dx-data-cloud.md diff --git a/docs/admin/integrations/dx-data-cloud.md b/docs/admin/integrations/dx-data-cloud.md new file mode 100644 index 0000000000000..62055d69f5f1a --- /dev/null +++ b/docs/admin/integrations/dx-data-cloud.md @@ -0,0 +1,87 @@ +# DX Data Cloud + +[DX](https://getdx.com) is a developer intelligence platform used by engineering +leaders and platform engineers. + +DX uses metadata attributes to assign information to individual users. +While it's common to segment users by `role`, `level`, or `geo`, it’s become increasingly +common to use DX attributes to better understand usage and adoption of tools. + +You can create a `Coder` attribute in DX to segment and analyze the impact of Coder usage on a developer’s work, including: + +- Understanding the needs of power users or low Coder usage across the org +- Correlate Coder usage with qualitative and quantitative engineering metrics, + such as PR throughput, deployment frequency, deep work, dev environment toil, and more. +- Personalize user experiences + +## Requirements + +- A DX subscription +- Access to Coder user data through the Coder CLI, Coder API, an IdP, or an existing Coder-DX integration +- Coordination with your DX Customer Success Manager + +## Extract Your Coder User List + +
+ +You can use the Coder CLI, Coder API, or your Identity Provider (IdP) to extract your list of users. + +If your organization already uses the Coder-DX integration, you can find a list of active Coder users directly within DX. + +### CLI + +Use `users list` to export the list of users to a CSV file: + +```shell +coder users list > users.csv +``` + +Visit the [users list](../../reference/cli/users_list.md) documentation for more options. + +### API + +Use [get users](../../reference/api/users.md#get-users): + +```bash +curl -X GET http://coder-server:8080/api/v2/users \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +To export the results to a CSV file, you can use the `jq` tool to process the JSON response: + +```bash +curl -X GET http://coder-server:8080/api/v2/users \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' | \ + jq -r '.users | (map(keys) | add | unique) as $cols | $cols, (.[] | [.[$cols[]]] | @csv)' > users.csv +``` + +Visit the [get users](../../reference/api/users.md#get-users) documentation for more options. + +### IdP + +If your organization uses a centralized IdP to manage user accounts, you can extract user data directly from your IdP. + +This is particularly useful if you need additional user attributes managed within your IdP. + +
+ +## Contact your DX Customer Success Manager + +Provide the file to your dedicated DX Customer Success Manager (CSM). + +Your CSM will import the CSV of individuals using Coder, as well as usage frequency (if applicable) into DX to create a `Coder` attribute. + +After the attribute is uploaded, you'll have a Coder filter option within your DX reports allowing you to: + +- Perform cohort analysis (Coder user vs non-user) +- Understand unique behaviors and patterns across your Coder users +- Run a [study](https://getdx.com/studies/) or setup a [PlatformX](https://getdx.com/platformx/) event for deeper analysis + +## Related Resources + +- [DX Data Cloud Documentation](https://help.getdx.com/en/) +- [Coder CLI](../../reference/cli/users.md) +- [Coder API](../../reference/api/users.md) +- [PlatformX Integration](./platformx.md) diff --git a/docs/manifest.json b/docs/manifest.json index 2aa9cb0ead9ce..c01eb828ac3a1 100644 --- a/docs/manifest.json +++ b/docs/manifest.json @@ -697,6 +697,11 @@ "description": "Integrate Coder with DX PlatformX", "path": "./admin/integrations/platformx.md" }, + { + "title": "DX Data Cloud", + "description": "Tag Coder Users with DX Data Cloud", + "path": "./admin/integrations/dx-data-cloud.md" + }, { "title": "Hashicorp Vault", "description": "Integrate Coder with Hashicorp Vault", From 579c851285dd25480088865f1c93113af0b323f4 Mon Sep 17 00:00:00 2001 From: Jaayden Halko Date: Fri, 20 Jun 2025 22:18:07 +0100 Subject: [PATCH 002/690] chore: update template opt-in checkbox (#18481) Screenshot 2025-06-20 at 20 01 55 --- .../TemplateSettingsForm.tsx | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx index 1a02d50d06dcf..d6b56fd06e24f 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx @@ -10,6 +10,7 @@ import { } from "api/typesGenerated"; import { PremiumBadge } from "components/Badges/Badges"; import { Button } from "components/Button/Button"; +import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge"; import { FormFields, FormFooter, @@ -17,6 +18,7 @@ import { HorizontalForm, } from "components/Form/Form"; import { IconField } from "components/IconField/IconField"; +import { Link } from "components/Link/Link"; import { Spinner } from "components/Spinner/Spinner"; import { Stack } from "components/Stack/Stack"; import { @@ -25,6 +27,7 @@ import { } from "components/StackLabel/StackLabel"; import { type FormikTouched, useFormik } from "formik"; import type { FC } from "react"; +import { docs } from "utils/docs"; import { displayNameValidator, getFormHelpers, @@ -230,21 +233,37 @@ export const TemplateSettingsForm: FC = ({ size="small" id="use_classic_parameter_flow" name="use_classic_parameter_flow" - checked={form.values.use_classic_parameter_flow} - onChange={form.handleChange} + checked={!form.values.use_classic_parameter_flow} + onChange={(event) => + form.setFieldValue( + "use_classic_parameter_flow", + !event.currentTarget.checked, + ) + } disabled={false} /> } label={ - Use classic workspace creation form + + Enable dynamic parameters for workspace creation + + - - Show the original workspace creation form and workspace - parameters settings form without dynamic parameters or live - updates. Recommended if your provisioners aren't updated or - the dynamic form causes issues. - +
+ The new workspace form allows you to design your template + with new form types and identity-aware conditional + parameters. The form will only present options that are + compatible and available. +
+ + Learn more +
} From 0258f1d771188fceffa0172ba4a8f4c2a1143348 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Sat, 21 Jun 2025 02:36:14 +0500 Subject: [PATCH 003/690] docs: update Railway.com installation link (#18484) --- docs/install/other/index.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/install/other/index.md b/docs/install/other/index.md index f727e5c34bf55..1aa6d195bcdbb 100644 --- a/docs/install/other/index.md +++ b/docs/install/other/index.md @@ -1,6 +1,6 @@ # Alternate install methods -Coder has a number of alternate unofficial install methods. Contributions are +Coder has a number of alternate unofficial installation methods. Contributions are welcome! | Platform Name | Status | Documentation | @@ -9,7 +9,7 @@ welcome! | Terraform (GKE, AKS, LKE, DOKS, IBMCloud K8s, OVHCloud K8s, Scaleway K8s Kapsule) | Unofficial | [GitHub: coder-oss-terraform](https://github.com/ElliotG/coder-oss-tf) | | Fly.io | Unofficial | [Blog: Run Coder on Fly.io](https://coder.com/blog/remote-developer-environments-on-fly-io) | | Garden.io | Unofficial | [GitHub: garden-coder-example](https://github.com/garden-io/garden-coder-example) | -| Railway.app | Unofficial | [Blog: Run Coder on Railway.app](https://coder.com/blog/deploy-coder-on-railway-app) | +| Railway.com | Unofficial | [Run Coder on Railway.com](https://railway.com/deploy/coder) | | Heroku | Unofficial | [Docs: Deploy Coder on Heroku](https://github.com/coder/packages/blob/main/heroku/README.md) | | Render | Unofficial | [Docs: Deploy Coder on Render](https://github.com/coder/packages/blob/main/render/README.md) | | Snapcraft | Unofficial | [Get it from the Snap Store](https://snapcraft.io/coder) | From 0a483ea2b7ff99c878b6d3abc4783bda8f0dbf22 Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 20 Jun 2025 14:34:31 -0800 Subject: [PATCH 004/690] feat: add idle app status (#18415) "Idle" is more accurate than "complete" since: 1. AgentAPI only knows if the screen is active; it has no way of knowing if the task is complete. 2. The LLM might be done with its current prompt, but that does not mean the task is complete either (it likely needs refinement). The "complete" state will be reserved for future definition. Additionally, in the case where the screen goes idle but the LLM never reported a status update, we can get an idle icon without a message, and it looks kinda janky in the UI so if there is no message I display the state text. Closes https://github.com/coder/internal/issues/699 --- cli/exp_mcp.go | 4 +-- cli/exp_mcp_test.go | 4 +-- coderd/apidoc/docs.go | 2 ++ coderd/apidoc/swagger.json | 3 +- coderd/database/dump.sql | 3 +- .../000340_workspace_app_status_idle.down.sql | 15 ++++++++ .../000340_workspace_app_status_idle.up.sql | 1 + coderd/database/models.go | 5 ++- coderd/workspaceagents.go | 5 ++- codersdk/toolsdk/toolsdk.go | 6 ++-- codersdk/workspaceapps.go | 1 + docs/reference/api/builds.md | 2 ++ docs/reference/api/schemas.md | 1 + docs/reference/api/templates.md | 2 ++ site/src/api/typesGenerated.ts | 7 +++- site/src/modules/apps/AppStatusStateIcon.tsx | 5 +++ .../WorkspaceAppStatus.stories.tsx | 20 +++++++++++ .../WorkspaceAppStatus/WorkspaceAppStatus.tsx | 6 ++-- .../WorkspacePage/AppStatuses.stories.tsx | 34 +++++++++++++++++++ site/src/pages/WorkspacePage/AppStatuses.tsx | 5 +-- 20 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 coderd/database/migrations/000340_workspace_app_status_idle.down.sql create mode 100644 coderd/database/migrations/000340_workspace_app_status_idle.up.sql diff --git a/cli/exp_mcp.go b/cli/exp_mcp.go index d487af5691bca..0a1c9fcbeaf87 100644 --- a/cli/exp_mcp.go +++ b/cli/exp_mcp.go @@ -585,10 +585,10 @@ func (s *mcpServer) startWatcher(ctx context.Context, inv *serpent.Invocation) { case event := <-eventsCh: switch ev := event.(type) { case agentapi.EventStatusChange: - // If the screen is stable, assume complete. + // If the screen is stable, report idle. state := codersdk.WorkspaceAppStatusStateWorking if ev.Status == agentapi.StatusStable { - state = codersdk.WorkspaceAppStatusStateComplete + state = codersdk.WorkspaceAppStatusStateIdle } err := s.queue.Push(taskReport{ state: state, diff --git a/cli/exp_mcp_test.go b/cli/exp_mcp_test.go index 08d6fbc4e2ce6..bcfafb0204874 100644 --- a/cli/exp_mcp_test.go +++ b/cli/exp_mcp_test.go @@ -900,7 +900,7 @@ func TestExpMcpReporter(t *testing.T) { { event: makeStatusEvent(agentapi.StatusStable), expected: &codersdk.WorkspaceAppStatus{ - State: codersdk.WorkspaceAppStatusStateComplete, + State: codersdk.WorkspaceAppStatusStateIdle, Message: "doing work", URI: "https://dev.coder.com", }, @@ -948,7 +948,7 @@ func TestExpMcpReporter(t *testing.T) { { event: makeStatusEvent(agentapi.StatusStable), expected: &codersdk.WorkspaceAppStatus{ - State: codersdk.WorkspaceAppStatusStateComplete, + State: codersdk.WorkspaceAppStatusStateIdle, Message: "oops", URI: "", }, diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 1d175333c1271..6844d166d8f1d 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -18081,11 +18081,13 @@ const docTemplate = `{ "type": "string", "enum": [ "working", + "idle", "complete", "failure" ], "x-enum-varnames": [ "WorkspaceAppStatusStateWorking", + "WorkspaceAppStatusStateIdle", "WorkspaceAppStatusStateComplete", "WorkspaceAppStatusStateFailure" ] diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 9d00a7ba34c30..35d39cfbe3839 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -16522,9 +16522,10 @@ }, "codersdk.WorkspaceAppStatusState": { "type": "string", - "enum": ["working", "complete", "failure"], + "enum": ["working", "idle", "complete", "failure"], "x-enum-varnames": [ "WorkspaceAppStatusStateWorking", + "WorkspaceAppStatusStateIdle", "WorkspaceAppStatusStateComplete", "WorkspaceAppStatusStateFailure" ] diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 2a94ef0fe7b4e..17b16abc5715c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -324,7 +324,8 @@ CREATE TYPE workspace_app_open_in AS ENUM ( CREATE TYPE workspace_app_status_state AS ENUM ( 'working', 'complete', - 'failure' + 'failure', + 'idle' ); CREATE TYPE workspace_transition AS ENUM ( diff --git a/coderd/database/migrations/000340_workspace_app_status_idle.down.sql b/coderd/database/migrations/000340_workspace_app_status_idle.down.sql new file mode 100644 index 0000000000000..a5d2095b1cd4a --- /dev/null +++ b/coderd/database/migrations/000340_workspace_app_status_idle.down.sql @@ -0,0 +1,15 @@ +-- It is not possible to delete a value from an enum, so we have to recreate it. +CREATE TYPE old_workspace_app_status_state AS ENUM ('working', 'complete', 'failure'); + +-- Convert the new "idle" state into "complete". This means we lose some +-- information when downgrading, but this is necessary to swap to the old enum. +UPDATE workspace_app_statuses SET state = 'complete' WHERE state = 'idle'; + +-- Swap to the old enum. +ALTER TABLE workspace_app_statuses +ALTER COLUMN state TYPE old_workspace_app_status_state +USING (state::text::old_workspace_app_status_state); + +-- Drop the new enum and rename the old one to the final name. +DROP TYPE workspace_app_status_state; +ALTER TYPE old_workspace_app_status_state RENAME TO workspace_app_status_state; diff --git a/coderd/database/migrations/000340_workspace_app_status_idle.up.sql b/coderd/database/migrations/000340_workspace_app_status_idle.up.sql new file mode 100644 index 0000000000000..1630e3580f45c --- /dev/null +++ b/coderd/database/migrations/000340_workspace_app_status_idle.up.sql @@ -0,0 +1 @@ +ALTER TYPE workspace_app_status_state ADD VALUE IF NOT EXISTS 'idle'; diff --git a/coderd/database/models.go b/coderd/database/models.go index 6a571ffc1d0d4..ce65c4c559d65 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -2628,6 +2628,7 @@ const ( WorkspaceAppStatusStateWorking WorkspaceAppStatusState = "working" WorkspaceAppStatusStateComplete WorkspaceAppStatusState = "complete" WorkspaceAppStatusStateFailure WorkspaceAppStatusState = "failure" + WorkspaceAppStatusStateIdle WorkspaceAppStatusState = "idle" ) func (e *WorkspaceAppStatusState) Scan(src interface{}) error { @@ -2669,7 +2670,8 @@ func (e WorkspaceAppStatusState) Valid() bool { switch e { case WorkspaceAppStatusStateWorking, WorkspaceAppStatusStateComplete, - WorkspaceAppStatusStateFailure: + WorkspaceAppStatusStateFailure, + WorkspaceAppStatusStateIdle: return true } return false @@ -2680,6 +2682,7 @@ func AllWorkspaceAppStatusStateValues() []WorkspaceAppStatusState { WorkspaceAppStatusStateWorking, WorkspaceAppStatusStateComplete, WorkspaceAppStatusStateFailure, + WorkspaceAppStatusStateIdle, } } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index ed3f554a89b75..8282eb9e7d01f 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -359,7 +359,10 @@ func (api *API) patchWorkspaceAgentAppStatus(rw http.ResponseWriter, r *http.Req } switch req.State { - case codersdk.WorkspaceAppStatusStateComplete, codersdk.WorkspaceAppStatusStateFailure, codersdk.WorkspaceAppStatusStateWorking: // valid states + case codersdk.WorkspaceAppStatusStateComplete, + codersdk.WorkspaceAppStatusStateFailure, + codersdk.WorkspaceAppStatusStateWorking, + codersdk.WorkspaceAppStatusStateIdle: // valid states default: httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid state provided.", diff --git a/codersdk/toolsdk/toolsdk.go b/codersdk/toolsdk/toolsdk.go index bb1649efa1993..3b992124005ac 100644 --- a/codersdk/toolsdk/toolsdk.go +++ b/codersdk/toolsdk/toolsdk.go @@ -191,7 +191,7 @@ Bad Tasks Use the "state" field to indicate your progress. Periodically report progress with state "working" to keep the user updated. It is not possible to send too many updates! -ONLY report a "complete" or "failure" state if you have FULLY completed the task. +ONLY report an "idle" or "failure" state if you have FULLY completed the task. `, Schema: aisdk.Schema{ Properties: map[string]any{ @@ -205,10 +205,10 @@ ONLY report a "complete" or "failure" state if you have FULLY completed the task }, "state": map[string]any{ "type": "string", - "description": "The state of your task. This can be one of the following: working, complete, or failure. Select the state that best represents your current progress.", + "description": "The state of your task. This can be one of the following: working, idle, or failure. Select the state that best represents your current progress.", "enum": []string{ string(codersdk.WorkspaceAppStatusStateWorking), - string(codersdk.WorkspaceAppStatusStateComplete), + string(codersdk.WorkspaceAppStatusStateIdle), string(codersdk.WorkspaceAppStatusStateFailure), }, }, diff --git a/codersdk/workspaceapps.go b/codersdk/workspaceapps.go index 556b3adb27b2e..6e95377bbaf42 100644 --- a/codersdk/workspaceapps.go +++ b/codersdk/workspaceapps.go @@ -19,6 +19,7 @@ type WorkspaceAppStatusState string const ( WorkspaceAppStatusStateWorking WorkspaceAppStatusState = "working" + WorkspaceAppStatusStateIdle WorkspaceAppStatusState = "idle" WorkspaceAppStatusStateComplete WorkspaceAppStatusState = "complete" WorkspaceAppStatusStateFailure WorkspaceAppStatusState = "failure" ) diff --git a/docs/reference/api/builds.md b/docs/reference/api/builds.md index 2a0e4b2ede1a0..c47b89d0bbba1 100644 --- a/docs/reference/api/builds.md +++ b/docs/reference/api/builds.md @@ -933,6 +933,7 @@ Status Code **200** | `sharing_level` | `organization` | | `sharing_level` | `public` | | `state` | `working` | +| `state` | `idle` | | `state` | `complete` | | `state` | `failure` | | `lifecycle_state` | `created` | @@ -1695,6 +1696,7 @@ Status Code **200** | `sharing_level` | `organization` | | `sharing_level` | `public` | | `state` | `working` | +| `state` | `idle` | | `state` | `complete` | | `state` | `failure` | | `lifecycle_state` | `created` | diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 8f548478e27a6..e0999f6bb3778 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -9686,6 +9686,7 @@ If the schedule is empty, the user will be updated to use the default schedule.| | Value | |------------| | `working` | +| `idle` | | `complete` | | `failure` | diff --git a/docs/reference/api/templates.md b/docs/reference/api/templates.md index d695be4122951..85e865d8b4b37 100644 --- a/docs/reference/api/templates.md +++ b/docs/reference/api/templates.md @@ -2557,6 +2557,7 @@ Status Code **200** | `sharing_level` | `organization` | | `sharing_level` | `public` | | `state` | `working` | +| `state` | `idle` | | `state` | `complete` | | `state` | `failure` | | `lifecycle_state` | `created` | @@ -3233,6 +3234,7 @@ Status Code **200** | `sharing_level` | `organization` | | `sharing_level` | `public` | | `state` | `working` | +| `state` | `idle` | | `state` | `complete` | | `state` | `failure` | | `lifecycle_state` | `created` | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 98338c24bb2d8..d536ac3a0fe5e 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -3622,11 +3622,16 @@ export interface WorkspaceAppStatus { } // From codersdk/workspaceapps.go -export type WorkspaceAppStatusState = "complete" | "failure" | "working"; +export type WorkspaceAppStatusState = + | "complete" + | "failure" + | "idle" + | "working"; export const WorkspaceAppStatusStates: WorkspaceAppStatusState[] = [ "complete", "failure", + "idle", "working", ]; diff --git a/site/src/modules/apps/AppStatusStateIcon.tsx b/site/src/modules/apps/AppStatusStateIcon.tsx index 829a8288235de..f713f49ed24b0 100644 --- a/site/src/modules/apps/AppStatusStateIcon.tsx +++ b/site/src/modules/apps/AppStatusStateIcon.tsx @@ -5,6 +5,7 @@ import { CircleAlertIcon, CircleCheckIcon, HourglassIcon, + SquareIcon, TriangleAlertIcon, } from "lucide-react"; import type { FC } from "react"; @@ -26,6 +27,10 @@ export const AppStatusStateIcon: FC = ({ const className = cn(["size-4 shrink-0", customClassName]); switch (state) { + case "idle": + return ( + + ); case "complete": return ( diff --git a/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.stories.tsx b/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.stories.tsx index d95444e658d64..0e229467b994b 100644 --- a/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.stories.tsx +++ b/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.stories.tsx @@ -39,6 +39,26 @@ export const Working: Story = { }, }; +export const Idle: Story = { + args: { + status: { + ...MockWorkspaceAppStatus, + state: "idle", + message: "Done for now", + }, + }, +}; + +export const NoMessage: Story = { + args: { + status: { + ...MockWorkspaceAppStatus, + state: "idle", + message: "", + }, + }, +}; + export const LongMessage: Story = { args: { status: { diff --git a/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.tsx b/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.tsx index 0b999f54402a8..587ae9f5b062f 100644 --- a/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.tsx +++ b/site/src/modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus.tsx @@ -5,6 +5,7 @@ import { TooltipProvider, TooltipTrigger, } from "components/Tooltip/Tooltip"; +import capitalize from "lodash/capitalize"; import { AppStatusStateIcon } from "modules/apps/AppStatusStateIcon"; import { cn } from "utils/cn"; @@ -25,6 +26,7 @@ export const WorkspaceAppStatus = ({ ); } + const message = status.message || capitalize(status.state); return (
@@ -40,11 +42,11 @@ export const WorkspaceAppStatus = ({ })} /> - {status.message} + {message}
- {status.message} + {message} diff --git a/site/src/pages/WorkspacePage/AppStatuses.stories.tsx b/site/src/pages/WorkspacePage/AppStatuses.stories.tsx index 90be0f194fef3..c7ec5eb56f417 100644 --- a/site/src/pages/WorkspacePage/AppStatuses.stories.tsx +++ b/site/src/pages/WorkspacePage/AppStatuses.stories.tsx @@ -48,6 +48,40 @@ export const WorkingState: Story = { }, }; +export const IdleState: Story = { + args: { + agent: mockAgent([ + { + ...MockWorkspaceAppStatus, + id: "status-8", + icon: "", + message: "Done for now", + created_at: createTimestamp(5, 20), + uri: "", + state: "idle" as const, + }, + ...MockWorkspaceAppStatuses, + ]), + }, +}; + +export const NoMessage: Story = { + args: { + agent: mockAgent([ + { + ...MockWorkspaceAppStatus, + id: "status-8", + icon: "", + message: "", + created_at: createTimestamp(5, 20), + uri: "", + state: "idle" as const, + }, + ...MockWorkspaceAppStatuses, + ]), + }, +}; + export const LongStatusText: Story = { args: { agent: mockAgent([ diff --git a/site/src/pages/WorkspacePage/AppStatuses.tsx b/site/src/pages/WorkspacePage/AppStatuses.tsx index 35d4db46c3ac9..95e3f9c95a472 100644 --- a/site/src/pages/WorkspacePage/AppStatuses.tsx +++ b/site/src/pages/WorkspacePage/AppStatuses.tsx @@ -12,6 +12,7 @@ import { TooltipProvider, TooltipTrigger, } from "components/Tooltip/Tooltip"; +import capitalize from "lodash/capitalize"; import { timeFrom } from "utils/time"; import { @@ -77,7 +78,7 @@ export const AppStatuses: FC = ({
- {latestStatus.message} + {latestStatus.message || capitalize(latestStatus.state)}
@@ -160,7 +161,7 @@ export const AppStatuses: FC = ({ latest={false} className="size-icon-xs w-[18px]" /> - {status.message} + {status.message || capitalize(status.state)} {formattedTimestamp} From 738442b4d04a00358a0c9b9d732513a7eb5aeb9e Mon Sep 17 00:00:00 2001 From: DevCats Date: Sat, 21 Jun 2025 03:41:12 -0500 Subject: [PATCH 005/690] chore: add amazon-q icon SVG to static icons (#18295) Co-authored-by: Atif Ali --- site/src/theme/icons.json | 1 + site/static/icon/amazon-q.svg | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 site/static/icon/amazon-q.svg diff --git a/site/src/theme/icons.json b/site/src/theme/icons.json index 8474ab0ef15a3..a8aeb780b6b51 100644 --- a/site/src/theme/icons.json +++ b/site/src/theme/icons.json @@ -1,6 +1,7 @@ [ "aider.svg", "almalinux.svg", + "amazon-q.svg", "android-studio.svg", "apache-guacamole.svg", "apple-black.svg", diff --git a/site/static/icon/amazon-q.svg b/site/static/icon/amazon-q.svg new file mode 100644 index 0000000000000..d2e576c033c0b --- /dev/null +++ b/site/static/icon/amazon-q.svg @@ -0,0 +1,17 @@ + + + + + + + + + + + From 725bc3792e3af415790a0baf3f7403ae89752fd9 Mon Sep 17 00:00:00 2001 From: jabacrack Date: Sat, 21 Jun 2025 16:46:30 +0300 Subject: [PATCH 006/690] fix: restrict database access from outside in docker compose installation (#18229) Co-authored-by: Atif Ali --- docker-compose.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index 5f1a1c8b4779e..b5ab4cf0227ff 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -32,8 +32,9 @@ services: # Minimum supported version is 13. # More versions here: https://hub.docker.com/_/postgres image: "postgres:17" - ports: - - "5432:5432" + # Uncomment the next two lines to allow connections to the database from outside the server. + #ports: + # - "5432:5432" environment: POSTGRES_USER: ${POSTGRES_USER:-username} # The PostgreSQL user (useful to connect to the database) POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-password} # The PostgreSQL password (useful to connect to the database) From 49fcffc2661b861dbfc90e021a6744b0e4faf43e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 Jun 2025 09:12:37 +0100 Subject: [PATCH 007/690] fix!: stop workspace before update (#18425) Fixes https://github.com/coder/coder/issues/17840 NOTE: calling this out as a breaking change so that it is highly visible in the changelog. * CLI: Modifies `coder update` to stop the workspace if already running. * UI: Modifies "update" button to always stop the workspace if already running. --- cli/start_test.go | 2 +- cli/stop.go | 51 +++--- cli/testdata/coder_--help.golden | 3 +- cli/testdata/coder_update_--help.golden | 3 +- cli/update.go | 17 +- cli/update_test.go | 114 ++++++++++--- coderd/autobuild/lifecycle_executor_test.go | 24 +-- coderd/coderdtest/coderdtest.go | 8 +- coderd/workspaceapps/apptest/apptest.go | 3 +- coderd/workspacebuilds_test.go | 10 +- coderd/workspaces_test.go | 4 +- docs/manifest.json | 2 +- docs/reference/cli/index.md | 88 +++++----- docs/reference/cli/update.md | 2 +- enterprise/cli/start_test.go | 5 +- enterprise/coderd/workspaces_test.go | 18 +-- site/e2e/helpers.ts | 13 ++ site/src/api/api.test.ts | 151 ++++++++++++------ site/src/api/api.ts | 14 ++ site/src/modules/workspaces/actions.ts | 19 ++- .../WorkspaceActions/Buttons.tsx | 60 ++++--- .../WorkspaceActions/WorkspaceActions.tsx | 33 +++- .../WorkspacesPage/WorkspacesPage.test.tsx | 8 +- .../WorkspacesPage/WorkspacesPageView.tsx | 6 +- 24 files changed, 426 insertions(+), 232 deletions(-) diff --git a/cli/start_test.go b/cli/start_test.go index f64a3f0646303..ec5f0b4735b39 100644 --- a/cli/start_test.go +++ b/cli/start_test.go @@ -358,7 +358,7 @@ func TestStartAutoUpdate(t *testing.T) { coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) if c.Cmd == "start" { - coderdtest.MustTransitionWorkspace(t, member, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, member, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) } version2 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, prepareEchoResponses(stringRichParameters), func(ctvr *codersdk.CreateTemplateVersionRequest) { ctvr.TemplateID = template.ID diff --git a/cli/stop.go b/cli/stop.go index 218c42061db10..ef4813ff0a1a0 100644 --- a/cli/stop.go +++ b/cli/stop.go @@ -37,32 +37,11 @@ func (r *RootCmd) stop() *serpent.Command { if err != nil { return err } - if workspace.LatestBuild.Job.Status == codersdk.ProvisionerJobPending { - // cliutil.WarnMatchedProvisioners also checks if the job is pending - // but we still want to avoid users spamming multiple builds that will - // not be picked up. - cliui.Warn(inv.Stderr, "The workspace is already stopping!") - cliutil.WarnMatchedProvisioners(inv.Stderr, workspace.LatestBuild.MatchedProvisioners, workspace.LatestBuild.Job) - if _, err := cliui.Prompt(inv, cliui.PromptOptions{ - Text: "Enqueue another stop?", - IsConfirm: true, - Default: cliui.ConfirmNo, - }); err != nil { - return err - } - } - wbr := codersdk.CreateWorkspaceBuildRequest{ - Transition: codersdk.WorkspaceTransitionStop, - } - if bflags.provisionerLogDebug { - wbr.LogLevel = codersdk.ProvisionerLogLevelDebug - } - build, err := client.CreateWorkspaceBuild(inv.Context(), workspace.ID, wbr) + build, err := stopWorkspace(inv, client, workspace, bflags) if err != nil { return err } - cliutil.WarnMatchedProvisioners(inv.Stderr, build.MatchedProvisioners, build.Job) err = cliui.WorkspaceBuild(inv.Context(), inv.Stdout, client, build.ID) if err != nil { @@ -71,8 +50,8 @@ func (r *RootCmd) stop() *serpent.Command { _, _ = fmt.Fprintf( inv.Stdout, - "\nThe %s workspace has been stopped at %s!\n", cliui.Keyword(workspace.Name), - + "\nThe %s workspace has been stopped at %s!\n", + cliui.Keyword(workspace.Name), cliui.Timestamp(time.Now()), ) return nil @@ -82,3 +61,27 @@ func (r *RootCmd) stop() *serpent.Command { return cmd } + +func stopWorkspace(inv *serpent.Invocation, client *codersdk.Client, workspace codersdk.Workspace, bflags buildFlags) (codersdk.WorkspaceBuild, error) { + if workspace.LatestBuild.Job.Status == codersdk.ProvisionerJobPending { + // cliutil.WarnMatchedProvisioners also checks if the job is pending + // but we still want to avoid users spamming multiple builds that will + // not be picked up. + cliui.Warn(inv.Stderr, "The workspace is already stopping!") + cliutil.WarnMatchedProvisioners(inv.Stderr, workspace.LatestBuild.MatchedProvisioners, workspace.LatestBuild.Job) + if _, err := cliui.Prompt(inv, cliui.PromptOptions{ + Text: "Enqueue another stop?", + IsConfirm: true, + Default: cliui.ConfirmNo, + }); err != nil { + return codersdk.WorkspaceBuild{}, err + } + } + wbr := codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStop, + } + if bflags.provisionerLogDebug { + wbr.LogLevel = codersdk.ProvisionerLogLevelDebug + } + return client.CreateWorkspaceBuild(inv.Context(), workspace.ID, wbr) +} diff --git a/cli/testdata/coder_--help.golden b/cli/testdata/coder_--help.golden index 1b2dbcf25056b..09dd4c3bce3a5 100644 --- a/cli/testdata/coder_--help.golden +++ b/cli/testdata/coder_--help.golden @@ -57,7 +57,8 @@ SUBCOMMANDS: tokens Manage personal access tokens unfavorite Remove a workspace from your favorites update Will update and start a given workspace if it is out of - date + date. If the workspace is already running, it will be + stopped first. users Manage users version Show coder version whoami Fetch authenticated user info for Coder deployment diff --git a/cli/testdata/coder_update_--help.golden b/cli/testdata/coder_update_--help.golden index 501447add29a7..b7bd7c48ed1e0 100644 --- a/cli/testdata/coder_update_--help.golden +++ b/cli/testdata/coder_update_--help.golden @@ -3,7 +3,8 @@ coder v0.0.0-devel USAGE: coder update [flags] - Will update and start a given workspace if it is out of date + Will update and start a given workspace if it is out of date. If the workspace + is already running, it will be stopped first. Use --always-prompt to change the parameter values of the workspace. diff --git a/cli/update.go b/cli/update.go index cf73992ea7ba4..21f090508d193 100644 --- a/cli/update.go +++ b/cli/update.go @@ -5,6 +5,7 @@ import ( "golang.org/x/xerrors" + "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" "github.com/coder/serpent" ) @@ -18,7 +19,7 @@ func (r *RootCmd) update() *serpent.Command { cmd := &serpent.Command{ Annotations: workspaceCommand, Use: "update ", - Short: "Will update and start a given workspace if it is out of date", + Short: "Will update and start a given workspace if it is out of date. If the workspace is already running, it will be stopped first.", Long: "Use --always-prompt to change the parameter values of the workspace.", Middleware: serpent.Chain( serpent.RequireNArgs(1), @@ -34,6 +35,20 @@ func (r *RootCmd) update() *serpent.Command { return nil } + // #17840: If the workspace is already running, we will stop it before + // updating. Simply performing a new start transition may not work if the + // template specifies ignore_changes. + if workspace.LatestBuild.Transition == codersdk.WorkspaceTransitionStart { + build, err := stopWorkspace(inv, client, workspace, bflags) + if err != nil { + return xerrors.Errorf("stop workspace: %w", err) + } + // Wait for the stop to complete. + if err := cliui.WorkspaceBuild(inv.Context(), inv.Stdout, client, build.ID); err != nil { + return xerrors.Errorf("wait for stop: %w", err) + } + } + build, err := startWorkspace(inv, client, workspace, parameterFlags, bflags, WorkspaceUpdate) if err != nil { return xerrors.Errorf("start workspace: %w", err) diff --git a/cli/update_test.go b/cli/update_test.go index 367a8196aa499..7a7480353c01d 100644 --- a/cli/update_test.go +++ b/cli/update_test.go @@ -34,28 +34,87 @@ func TestUpdate(t *testing.T) { t.Run("OK", func(t *testing.T) { t.Parallel() + // Given: a workspace exists on the latest template version. client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) owner := coderdtest.CreateFirstUser(t, client) - member, memberUser := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + member, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID) template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID) - inv, root := clitest.New(t, "create", - "my-workspace", - "--template", template.Name, - "-y", - ) + ws := coderdtest.CreateWorkspace(t, member, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.Name = "my-workspace" + }) + require.False(t, ws.Outdated, "newly created workspace with active template version must not be outdated") + + // Given: the template version is updated + version2 := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: echo.ApplyComplete, + ProvisionPlan: echo.PlanComplete, + }, template.ID) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version2.ID) + + ctx := testutil.Context(t, testutil.WaitShort) + err := client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{ + ID: version2.ID, + }) + require.NoError(t, err, "failed to update active template version") + + // Then: the workspace is marked as 'outdated' + ws, err = member.WorkspaceByOwnerAndName(ctx, codersdk.Me, "my-workspace", codersdk.WorkspaceOptions{}) + require.NoError(t, err, "member failed to get workspace they themselves own") + require.True(t, ws.Outdated, "workspace must be outdated after template version update") + + // When: the workspace is updated + inv, root := clitest.New(t, "update", ws.Name) clitest.SetupConfig(t, member, root) - err := inv.Run() - require.NoError(t, err) + err = inv.Run() + require.NoError(t, err, "update command failed") + + // Then: the workspace is no longer 'outdated' + ws, err = member.WorkspaceByOwnerAndName(ctx, codersdk.Me, "my-workspace", codersdk.WorkspaceOptions{}) + require.NoError(t, err, "member failed to get workspace they themselves own after update") + require.Equal(t, version2.ID.String(), ws.LatestBuild.TemplateVersionID.String(), "workspace must have latest template version after update") + require.False(t, ws.Outdated, "workspace must not be outdated after update") + + // Then: the workspace must have been started with the new template version + require.Equal(t, int32(3), ws.LatestBuild.BuildNumber, "workspace must have 3 builds after update") + require.Equal(t, codersdk.WorkspaceTransitionStart, ws.LatestBuild.Transition, "latest build must be a start transition") + + // Then: the previous workspace build must be a stop transition with the old + // template version. + // This is important to ensure that the workspace resources are recreated + // correctly. Simply running a start transition with the new template + // version may not recreate resources that were changed in the new + // template version. This can happen, for example, if a user specifies + // ignore_changes in the template. + prevBuild, err := member.WorkspaceBuildByUsernameAndWorkspaceNameAndBuildNumber(ctx, codersdk.Me, ws.Name, "2") + require.NoError(t, err, "failed to get previous workspace build") + require.Equal(t, codersdk.WorkspaceTransitionStop, prevBuild.Transition, "previous build must be a stop transition") + require.Equal(t, version1.ID.String(), prevBuild.TemplateVersionID.String(), "previous build must have the old template version") + }) - ws, err := client.WorkspaceByOwnerAndName(context.Background(), memberUser.Username, "my-workspace", codersdk.WorkspaceOptions{}) - require.NoError(t, err) - require.Equal(t, version1.ID.String(), ws.LatestBuild.TemplateVersionID.String()) + t.Run("Stopped", func(t *testing.T) { + t.Parallel() + + // Given: a workspace exists on the latest template version. + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + owner := coderdtest.CreateFirstUser(t, client) + member, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) + + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID) + template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID) + ws := coderdtest.CreateWorkspace(t, member, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.Name = "my-workspace" + }) + require.False(t, ws.Outdated, "newly created workspace with active template version must not be outdated") + + // Given: the template version is updated version2 := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, &echo.Responses{ Parse: echo.ParseComplete, ProvisionApply: echo.ApplyComplete, @@ -63,20 +122,37 @@ func TestUpdate(t *testing.T) { }, template.ID) _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version2.ID) - err = client.UpdateActiveTemplateVersion(context.Background(), template.ID, codersdk.UpdateActiveTemplateVersion{ + ctx := testutil.Context(t, testutil.WaitShort) + err := client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{ ID: version2.ID, }) - require.NoError(t, err) + require.NoError(t, err, "failed to update active template version") + + // Given: the workspace is in a stopped state. + coderdtest.MustTransitionWorkspace(t, member, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) - inv, root = clitest.New(t, "update", ws.Name) + // Then: the workspace is marked as 'outdated' + ws, err = member.WorkspaceByOwnerAndName(ctx, codersdk.Me, "my-workspace", codersdk.WorkspaceOptions{}) + require.NoError(t, err, "member failed to get workspace they themselves own") + require.True(t, ws.Outdated, "workspace must be outdated after template version update") + + // When: the workspace is updated + inv, root := clitest.New(t, "update", ws.Name) clitest.SetupConfig(t, member, root) err = inv.Run() - require.NoError(t, err) - - ws, err = member.WorkspaceByOwnerAndName(context.Background(), memberUser.Username, "my-workspace", codersdk.WorkspaceOptions{}) - require.NoError(t, err) - require.Equal(t, version2.ID.String(), ws.LatestBuild.TemplateVersionID.String()) + require.NoError(t, err, "update command failed") + + // Then: the workspace is no longer 'outdated' + ws, err = member.WorkspaceByOwnerAndName(ctx, codersdk.Me, "my-workspace", codersdk.WorkspaceOptions{}) + require.NoError(t, err, "member failed to get workspace they themselves own after update") + require.Equal(t, version2.ID.String(), ws.LatestBuild.TemplateVersionID.String(), "workspace must have latest template version after update") + require.False(t, ws.Outdated, "workspace must not be outdated after update") + + // Then: the workspace must have been started with the new template version + require.Equal(t, codersdk.WorkspaceTransitionStart, ws.LatestBuild.Transition, "latest build must be a start transition") + // Then: we expect 3 builds, as we manually stopped the workspace. + require.Equal(t, int32(3), ws.LatestBuild.BuildNumber, "workspace must have 3 builds after update") }) } diff --git a/coderd/autobuild/lifecycle_executor_test.go b/coderd/autobuild/lifecycle_executor_test.go index 65af911122aba..3bca6856534fa 100644 --- a/coderd/autobuild/lifecycle_executor_test.go +++ b/coderd/autobuild/lifecycle_executor_test.go @@ -47,7 +47,7 @@ func TestExecutorAutostartOK(t *testing.T) { }) ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks after the scheduled time go func() { @@ -105,7 +105,7 @@ func TestMultipleLifecycleExecutors(t *testing.T) { ) // Have the workspace stopped so we can perform an autostart - workspace = coderdtest.MustTransitionWorkspace(t, clientA, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, clientA, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Get both clients to perform a lifecycle execution tick next := sched.Next(workspace.LatestBuild.CreatedAt) @@ -203,7 +203,7 @@ func TestExecutorAutostartTemplateUpdated(t *testing.T) { ) // Given: workspace is stopped workspace = coderdtest.MustTransitionWorkspace( - t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) orgs, err := client.OrganizationsByUser(ctx, workspace.OwnerID.String()) require.NoError(t, err) @@ -344,7 +344,7 @@ func TestExecutorAutostartNotEnabled(t *testing.T) { require.Empty(t, workspace.AutostartSchedule) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks way into the future go func() { @@ -384,7 +384,7 @@ func TestExecutorAutostartUserSuspended(t *testing.T) { workspace = coderdtest.MustWorkspace(t, userClient, workspace.ID) // Given: workspace is stopped, and the user is suspended. - workspace = coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) ctx := testutil.Context(t, testutil.WaitShort) @@ -507,7 +507,7 @@ func TestExecutorAutostopAlreadyStopped(t *testing.T) { ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks past the TTL go func() { @@ -578,7 +578,7 @@ func TestExecutorWorkspaceDeleted(t *testing.T) { ) // Given: workspace is deleted - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionDelete) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionDelete) // When: the autobuild executor ticks go func() { @@ -767,7 +767,7 @@ func TestExecutorAutostartMultipleOK(t *testing.T) { }) ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks past the scheduled time go func() { @@ -832,7 +832,7 @@ func TestExecutorAutostartWithParameters(t *testing.T) { }) ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks after the scheduled time go func() { @@ -882,7 +882,7 @@ func TestExecutorAutostartTemplateDisabled(t *testing.T) { }) ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks before the next scheduled time go func() { @@ -1001,7 +1001,7 @@ func TestExecutorRequireActiveVersion(t *testing.T) { cwr.AutostartSchedule = ptr.Ref(sched.String()) }) _ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, ownerClient, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, memberClient, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { + ws = coderdtest.MustTransitionWorkspace(t, memberClient, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { req.TemplateVersionID = inactiveVersion.ID }) require.Equal(t, inactiveVersion.ID, ws.LatestBuild.TemplateVersionID) @@ -1159,7 +1159,7 @@ func TestNotifications(t *testing.T) { coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID) // Stop workspace - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) _ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID) // Wait for workspace to become dormant diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index a8f444c8f632e..b509db10e3eba 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -1245,16 +1245,16 @@ func CreateWorkspace(t testing.TB, client *codersdk.Client, templateID uuid.UUID } // TransitionWorkspace is a convenience method for transitioning a workspace from one state to another. -func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, to database.WorkspaceTransition, muts ...func(req *codersdk.CreateWorkspaceBuildRequest)) codersdk.Workspace { +func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, to codersdk.WorkspaceTransition, muts ...func(req *codersdk.CreateWorkspaceBuildRequest)) codersdk.Workspace { t.Helper() ctx := context.Background() workspace, err := client.Workspace(ctx, workspaceID) require.NoError(t, err, "unexpected error fetching workspace") - require.Equal(t, workspace.LatestBuild.Transition, codersdk.WorkspaceTransition(from), "expected workspace state: %s got: %s", from, workspace.LatestBuild.Transition) + require.Equal(t, workspace.LatestBuild.Transition, from, "expected workspace state: %s got: %s", from, workspace.LatestBuild.Transition) req := codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: workspace.LatestBuild.TemplateVersionID, - Transition: codersdk.WorkspaceTransition(to), + Transition: to, } for _, mut := range muts { @@ -1267,7 +1267,7 @@ func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID _ = AwaitWorkspaceBuildJobCompleted(t, client, build.ID) updated := MustWorkspace(t, client, workspace.ID) - require.Equal(t, codersdk.WorkspaceTransition(to), updated.LatestBuild.Transition, "expected workspace to be in state %s but got %s", to, updated.LatestBuild.Transition) + require.Equal(t, to, updated.LatestBuild.Transition, "expected workspace to be in state %s but got %s", to, updated.LatestBuild.Transition) return updated } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 050e5d2fb9a6e..d0f3acda77278 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -27,7 +27,6 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/coderdtest" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/jwtutils" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/workspaceapps" @@ -1820,7 +1819,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _ = coderdtest.MustTransitionWorkspace(t, appDetails.SDKClient, appDetails.Workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + _ = coderdtest.MustTransitionWorkspace(t, appDetails.SDKClient, appDetails.Workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) u := appDetails.PathAppURL(appDetails.Apps.Owner) resp, err := appDetails.AppClient(t).Request(ctx, http.MethodGet, u.String(), nil) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index ac33c9e92c4f7..081accd097d69 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -585,7 +585,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) // Create a workspace using this template workspace := coderdtest.CreateWorkspace(t, userClient, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID) - coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Create a new version of the template newVersion := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { @@ -598,7 +598,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) cwbr.TemplateVersionID = newVersion.ID }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) - coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Create the workspace build _again_. We are doing this to // ensure we do not create _another_ notification. This is @@ -610,7 +610,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) cwbr.TemplateVersionID = newVersion.ID }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) - coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // We're going to have two notifications (one for the first user and one for the template admin) // By ensuring we only have these two, we are sure the second build didn't trigger more @@ -659,7 +659,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) // Create a workspace using this template workspace := coderdtest.CreateWorkspace(t, userClient, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, workspace.LatestBuild.ID) - coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Create a new version of the template newVersion := coderdtest.CreateTemplateVersion(t, templateAdminClient, first.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { @@ -675,7 +675,7 @@ func TestWorkspaceBuildWithUpdatedTemplateVersionSendsNotification(t *testing.T) }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, build.ID) - coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, userClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Ensure we receive only 1 workspace manually updated notification and to the right user sent := notify.Sent(notificationstest.WithTemplateID(notifications.TemplateWorkspaceManuallyUpdated)) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 1d02622d455e8..4f48136abb3ee 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -3992,7 +3992,7 @@ func TestWorkspaceDormant(t *testing.T) { require.NoError(t, err) // Should be able to stop a workspace while it is dormant. - coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Should not be able to start a workspace while it is dormant. _, err = client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ @@ -4005,7 +4005,7 @@ func TestWorkspaceDormant(t *testing.T) { Dormant: false, }) require.NoError(t, err) - coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart) + coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStop, codersdk.WorkspaceTransitionStart) }) } diff --git a/docs/manifest.json b/docs/manifest.json index c01eb828ac3a1..5fbb98f94b006 100644 --- a/docs/manifest.json +++ b/docs/manifest.json @@ -1699,7 +1699,7 @@ }, { "title": "update", - "description": "Will update and start a given workspace if it is out of date", + "description": "Will update and start a given workspace if it is out of date. If the workspace is already running, it will be stopped first.", "path": "reference/cli/update.md" }, { diff --git a/docs/reference/cli/index.md b/docs/reference/cli/index.md index d72790fc3bfdb..6dae32c4c615c 100644 --- a/docs/reference/cli/index.md +++ b/docs/reference/cli/index.md @@ -22,50 +22,50 @@ Coder — A tool for provisioning self-hosted development environments with Terr ## Subcommands -| Name | Purpose | -|----------------------------------------------------|-------------------------------------------------------------------------------------------------------| -| [completion](./completion.md) | Install or update shell completion scripts for the detected or chosen shell. | -| [dotfiles](./dotfiles.md) | Personalize your workspace by applying a canonical dotfiles repository | -| [external-auth](./external-auth.md) | Manage external authentication | -| [login](./login.md) | Authenticate with Coder deployment | -| [logout](./logout.md) | Unauthenticate your local session | -| [netcheck](./netcheck.md) | Print network debug information for DERP and STUN | -| [notifications](./notifications.md) | Manage Coder notifications | -| [organizations](./organizations.md) | Organization related commands | -| [port-forward](./port-forward.md) | Forward ports from a workspace to the local machine. For reverse port forwarding, use "coder ssh -R". | -| [publickey](./publickey.md) | Output your Coder public key used for Git operations | -| [reset-password](./reset-password.md) | Directly connect to the database to reset a user's password | -| [state](./state.md) | Manually manage Terraform state to fix broken workspaces | -| [templates](./templates.md) | Manage templates | -| [tokens](./tokens.md) | Manage personal access tokens | -| [users](./users.md) | Manage users | -| [version](./version.md) | Show coder version | -| [autoupdate](./autoupdate.md) | Toggle auto-update policy for a workspace | -| [config-ssh](./config-ssh.md) | Add an SSH Host entry for your workspaces "ssh workspace.coder" | -| [create](./create.md) | Create a workspace | -| [delete](./delete.md) | Delete a workspace | -| [favorite](./favorite.md) | Add a workspace to your favorites | -| [list](./list.md) | List workspaces | -| [open](./open.md) | Open a workspace | -| [ping](./ping.md) | Ping a workspace | -| [rename](./rename.md) | Rename a workspace | -| [restart](./restart.md) | Restart a workspace | -| [schedule](./schedule.md) | Schedule automated start and stop times for workspaces | -| [show](./show.md) | Display details of a workspace's resources and agents | -| [speedtest](./speedtest.md) | Run upload and download tests from your machine to a workspace | -| [ssh](./ssh.md) | Start a shell into a workspace or run a command | -| [start](./start.md) | Start a workspace | -| [stat](./stat.md) | Show resource usage for the current workspace. | -| [stop](./stop.md) | Stop a workspace | -| [unfavorite](./unfavorite.md) | Remove a workspace from your favorites | -| [update](./update.md) | Will update and start a given workspace if it is out of date | -| [whoami](./whoami.md) | Fetch authenticated user info for Coder deployment | -| [support](./support.md) | Commands for troubleshooting issues with a Coder deployment. | -| [server](./server.md) | Start a Coder server | -| [features](./features.md) | List Enterprise features | -| [licenses](./licenses.md) | Add, delete, and list licenses | -| [groups](./groups.md) | Manage groups | -| [provisioner](./provisioner.md) | View and manage provisioner daemons and jobs | +| Name | Purpose | +|----------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------| +| [completion](./completion.md) | Install or update shell completion scripts for the detected or chosen shell. | +| [dotfiles](./dotfiles.md) | Personalize your workspace by applying a canonical dotfiles repository | +| [external-auth](./external-auth.md) | Manage external authentication | +| [login](./login.md) | Authenticate with Coder deployment | +| [logout](./logout.md) | Unauthenticate your local session | +| [netcheck](./netcheck.md) | Print network debug information for DERP and STUN | +| [notifications](./notifications.md) | Manage Coder notifications | +| [organizations](./organizations.md) | Organization related commands | +| [port-forward](./port-forward.md) | Forward ports from a workspace to the local machine. For reverse port forwarding, use "coder ssh -R". | +| [publickey](./publickey.md) | Output your Coder public key used for Git operations | +| [reset-password](./reset-password.md) | Directly connect to the database to reset a user's password | +| [state](./state.md) | Manually manage Terraform state to fix broken workspaces | +| [templates](./templates.md) | Manage templates | +| [tokens](./tokens.md) | Manage personal access tokens | +| [users](./users.md) | Manage users | +| [version](./version.md) | Show coder version | +| [autoupdate](./autoupdate.md) | Toggle auto-update policy for a workspace | +| [config-ssh](./config-ssh.md) | Add an SSH Host entry for your workspaces "ssh workspace.coder" | +| [create](./create.md) | Create a workspace | +| [delete](./delete.md) | Delete a workspace | +| [favorite](./favorite.md) | Add a workspace to your favorites | +| [list](./list.md) | List workspaces | +| [open](./open.md) | Open a workspace | +| [ping](./ping.md) | Ping a workspace | +| [rename](./rename.md) | Rename a workspace | +| [restart](./restart.md) | Restart a workspace | +| [schedule](./schedule.md) | Schedule automated start and stop times for workspaces | +| [show](./show.md) | Display details of a workspace's resources and agents | +| [speedtest](./speedtest.md) | Run upload and download tests from your machine to a workspace | +| [ssh](./ssh.md) | Start a shell into a workspace or run a command | +| [start](./start.md) | Start a workspace | +| [stat](./stat.md) | Show resource usage for the current workspace. | +| [stop](./stop.md) | Stop a workspace | +| [unfavorite](./unfavorite.md) | Remove a workspace from your favorites | +| [update](./update.md) | Will update and start a given workspace if it is out of date. If the workspace is already running, it will be stopped first. | +| [whoami](./whoami.md) | Fetch authenticated user info for Coder deployment | +| [support](./support.md) | Commands for troubleshooting issues with a Coder deployment. | +| [server](./server.md) | Start a Coder server | +| [features](./features.md) | List Enterprise features | +| [licenses](./licenses.md) | Add, delete, and list licenses | +| [groups](./groups.md) | Manage groups | +| [provisioner](./provisioner.md) | View and manage provisioner daemons and jobs | ## Options diff --git a/docs/reference/cli/update.md b/docs/reference/cli/update.md index dd2bfa5ff76b5..35c5b34312420 100644 --- a/docs/reference/cli/update.md +++ b/docs/reference/cli/update.md @@ -1,7 +1,7 @@ # update -Will update and start a given workspace if it is out of date +Will update and start a given workspace if it is out of date. If the workspace is already running, it will be stopped first. ## Usage diff --git a/enterprise/cli/start_test.go b/enterprise/cli/start_test.go index b68a95ae1131d..2ef3b8cd801c4 100644 --- a/enterprise/cli/start_test.go +++ b/enterprise/cli/start_test.go @@ -9,7 +9,6 @@ import ( "github.com/coder/coder/v2/cli/clitest" "github.com/coder/coder/v2/coderd/coderdtest" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" @@ -144,7 +143,7 @@ func TestStart(t *testing.T) { if cmd == "start" { // Stop the workspace so that we can start it. - coderdtest.MustTransitionWorkspace(t, c.Client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + coderdtest.MustTransitionWorkspace(t, c.Client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) } // Start the workspace. Every test permutation should // pass. @@ -196,7 +195,7 @@ func TestStart(t *testing.T) { memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) workspace := coderdtest.CreateWorkspace(t, memberClient, template.ID) _ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, memberClient, workspace.LatestBuild.ID) - _ = coderdtest.MustTransitionWorkspace(t, memberClient, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + _ = coderdtest.MustTransitionWorkspace(t, memberClient, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) err := memberClient.UpdateWorkspaceDormancy(ctx, workspace.ID, codersdk.UpdateWorkspaceDormancy{ Dormant: true, }) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index e8a29ccc33959..125647b099282 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -962,7 +962,7 @@ func TestWorkspaceAutobuild(t *testing.T) { // Stop the workspace so we can assert autobuild does nothing // if we breach our inactivity threshold. - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Simulate not having accessed the workspace in a while. ticker <- ws.LastUsedAt.Add(2 * inactiveTTL) @@ -1150,7 +1150,7 @@ func TestWorkspaceAutobuild(t *testing.T) { cwr.AutostartSchedule = ptr.Ref(sched.String()) }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Assert that autostart works when the workspace isn't dormant.. tickCh <- sched.Next(ws.LatestBuild.CreatedAt) @@ -1319,7 +1319,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Create a new version so that we can assert we don't update // to the latest by default. @@ -1360,7 +1360,7 @@ func TestWorkspaceAutobuild(t *testing.T) { // Reset the workspace to the stopped state so we can try // to autostart again. - coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { + coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop, func(req *codersdk.CreateWorkspaceBuildRequest) { req.TemplateVersionID = ws.LatestBuild.TemplateVersionID }) @@ -1420,7 +1420,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) next := ws.LatestBuild.CreatedAt // For each day of the week (Monday-Sunday) @@ -1448,7 +1448,7 @@ func TestWorkspaceAutobuild(t *testing.T) { assert.Equal(t, database.WorkspaceTransitionStart, stats.Transitions[ws.ID]) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) } // Ensure that there is a valid next start at and that is is after @@ -1511,7 +1511,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Our next start at should be Monday require.NotNil(t, ws.NextStartAt) @@ -1573,7 +1573,7 @@ func TestWorkspaceAutobuild(t *testing.T) { }) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) - ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + ws = coderdtest.MustTransitionWorkspace(t, client, ws.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // Check we have a 'NextStartAt' require.NotNil(t, ws.NextStartAt) @@ -2099,7 +2099,7 @@ func TestExecutorAutostartBlocked(t *testing.T) { ) // Given: workspace is stopped - workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) + workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, codersdk.WorkspaceTransitionStart, codersdk.WorkspaceTransitionStop) // When: the autobuild executor ticks into the future go func() { diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index cc91984ae592f..262567b7d7c96 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -1018,9 +1018,22 @@ export const updateWorkspace = async ( await fillParameters(page, richParameters, buildParameters); await page.getByRole("button", { name: /update parameters/i }).click(); + // Wait for the update button to detach. + await page.waitForSelector( + "button[data-testid='workspace-update-button']:enabled", + { state: "detached" }, + ); + // Wait for the workspace to be running again. await page.waitForSelector("text=Workspace status: Running", { state: "visible", }); + // Wait for the stop button to be enabled again + await page.waitForSelector( + "button[data-testid='workspace-stop-button']:enabled", + { + state: "visible", + }, + ); }; export const updateWorkspaceParameters = async ( diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index e72dd5f8d0bad..04536675f8943 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -1,5 +1,7 @@ import { + MockStoppedWorkspace, MockTemplate, + MockTemplateVersion2, MockTemplateVersionParameter1, MockTemplateVersionParameter2, MockWorkspace, @@ -171,65 +173,112 @@ describe("api.ts", () => { }); describe("update", () => { - it("creates a build with start and the latest template", async () => { - jest - .spyOn(API, "postWorkspaceBuild") - .mockResolvedValueOnce(MockWorkspaceBuild); - jest.spyOn(API, "getTemplate").mockResolvedValueOnce(MockTemplate); - await API.updateWorkspace(MockWorkspace); - expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { - transition: "start", - template_version_id: MockTemplate.active_version_id, - rich_parameter_values: [], + describe("given a running workspace", () => { + it("stops with current version before starting with the latest version", async () => { + jest.spyOn(API, "postWorkspaceBuild").mockResolvedValueOnce({ + ...MockWorkspaceBuild, + transition: "stop", + }); + jest.spyOn(API, "postWorkspaceBuild").mockResolvedValueOnce({ + ...MockWorkspaceBuild, + template_version_id: MockTemplateVersion2.id, + transition: "start", + }); + jest.spyOn(API, "getTemplate").mockResolvedValueOnce({ + ...MockTemplate, + active_version_id: MockTemplateVersion2.id, + }); + await API.updateWorkspace(MockWorkspace); + expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { + transition: "stop", + log_level: undefined, + }); + expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { + transition: "start", + template_version_id: MockTemplateVersion2.id, + rich_parameter_values: [], + }); }); - }); - it("fails when having missing parameters", async () => { - jest - .spyOn(API, "postWorkspaceBuild") - .mockResolvedValue(MockWorkspaceBuild); - jest.spyOn(API, "getTemplate").mockResolvedValue(MockTemplate); - jest.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValue([]); - jest - .spyOn(API, "getTemplateVersionRichParameters") - .mockResolvedValue([ + it("fails when having missing parameters", async () => { + jest + .spyOn(API, "postWorkspaceBuild") + .mockResolvedValue(MockWorkspaceBuild); + jest.spyOn(API, "getTemplate").mockResolvedValue(MockTemplate); + jest.spyOn(API, "getWorkspaceBuildParameters").mockResolvedValue([]); + jest + .spyOn(API, "getTemplateVersionRichParameters") + .mockResolvedValue([ + MockTemplateVersionParameter1, + { ...MockTemplateVersionParameter2, mutable: false }, + ]); + + let error = new Error(); + try { + await API.updateWorkspace(MockWorkspace); + } catch (e) { + error = e as Error; + } + + expect(error).toBeInstanceOf(MissingBuildParameters); + // Verify if the correct missing parameters are being passed + expect((error as MissingBuildParameters).parameters).toEqual([ MockTemplateVersionParameter1, { ...MockTemplateVersionParameter2, mutable: false }, ]); + }); - let error = new Error(); - try { + it("creates a build with no parameters if it is already filled", async () => { + jest.spyOn(API, "postWorkspaceBuild").mockResolvedValueOnce({ + ...MockWorkspaceBuild, + transition: "stop", + }); + jest.spyOn(API, "postWorkspaceBuild").mockResolvedValueOnce({ + ...MockWorkspaceBuild, + template_version_id: MockTemplateVersion2.id, + transition: "start", + }); + jest.spyOn(API, "getTemplate").mockResolvedValueOnce(MockTemplate); + jest + .spyOn(API, "getWorkspaceBuildParameters") + .mockResolvedValue([MockWorkspaceBuildParameter1]); + jest.spyOn(API, "getTemplateVersionRichParameters").mockResolvedValue([ + { + ...MockTemplateVersionParameter1, + required: true, + mutable: false, + }, + ]); await API.updateWorkspace(MockWorkspace); - } catch (e) { - error = e as Error; - } - - expect(error).toBeInstanceOf(MissingBuildParameters); - // Verify if the correct missing parameters are being passed - expect((error as MissingBuildParameters).parameters).toEqual([ - MockTemplateVersionParameter1, - { ...MockTemplateVersionParameter2, mutable: false }, - ]); + expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { + transition: "stop", + log_level: undefined, + }); + expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { + transition: "start", + template_version_id: MockTemplate.active_version_id, + rich_parameter_values: [], + }); + }); }); - - it("creates a build with the no parameters if it is already filled", async () => { - jest - .spyOn(API, "postWorkspaceBuild") - .mockResolvedValueOnce(MockWorkspaceBuild); - jest.spyOn(API, "getTemplate").mockResolvedValueOnce(MockTemplate); - jest - .spyOn(API, "getWorkspaceBuildParameters") - .mockResolvedValue([MockWorkspaceBuildParameter1]); - jest - .spyOn(API, "getTemplateVersionRichParameters") - .mockResolvedValue([ - { ...MockTemplateVersionParameter1, required: true, mutable: false }, - ]); - await API.updateWorkspace(MockWorkspace); - expect(API.postWorkspaceBuild).toHaveBeenCalledWith(MockWorkspace.id, { - transition: "start", - template_version_id: MockTemplate.active_version_id, - rich_parameter_values: [], + describe("given a stopped workspace", () => { + it("creates a build with start and the latest template", async () => { + jest + .spyOn(API, "postWorkspaceBuild") + .mockResolvedValueOnce(MockWorkspaceBuild); + jest.spyOn(API, "getTemplate").mockResolvedValueOnce({ + ...MockTemplate, + active_version_id: MockTemplateVersion2.id, + }); + await API.updateWorkspace(MockStoppedWorkspace); + expect(API.postWorkspaceBuild).toHaveBeenCalledWith( + MockStoppedWorkspace.id, + { + transition: "start", + template_version_id: MockTemplateVersion2.id, + rich_parameter_values: [], + }, + ); }); }); }); diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 5b7cde65fb2ce..094502939821b 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2237,6 +2237,7 @@ class ApiMethods { * - Update the build parameters and check if there are missed parameters for * the newest version * - If there are missing parameters raise an error + * - Stop the workspace with the current template version if it is already running * - Create a build with the latest version and updated build parameters */ updateWorkspace = async ( @@ -2274,6 +2275,19 @@ class ApiMethods { throw new MissingBuildParameters(missingParameters, activeVersionId); } + // Stop the workspace if it is already running. + if (workspace.latest_build.status === "running") { + const stopBuild = await this.stopWorkspace(workspace.id); + const awaitedStopBuild = await this.waitForBuild(stopBuild); + // If the stop is canceled halfway through, we bail. + // This is the same behaviour as restartWorkspace. + if (awaitedStopBuild?.status === "canceled") { + return Promise.reject( + new Error("Workspace stop was canceled, not proceeding with update."), + ); + } + } + return this.postWorkspaceBuild(workspace.id, { transition: "start", template_version_id: activeVersionId, diff --git a/site/src/modules/workspaces/actions.ts b/site/src/modules/workspaces/actions.ts index 6a255e2cd2c88..f109c4d9ad1b9 100644 --- a/site/src/modules/workspaces/actions.ts +++ b/site/src/modules/workspaces/actions.ts @@ -6,16 +6,19 @@ import type { Workspace } from "api/typesGenerated"; const actionTypes = [ "start", "starting", - // Replaces start when an update is required. + // Appears beside start when an update is available. "updateAndStart", + // Replaces start when an update is required. + "updateAndStartRequireActiveVersion", "stop", "stopping", "restart", "restarting", - // Replaces restart when an update is required. + // Appears beside restart when an update is available. "updateAndRestart", + // Replaces restart when an update is required. + "updateAndRestartRequireActiveVersion", "deleting", - "update", "updating", "activate", "activating", @@ -74,10 +77,10 @@ export const abilitiesByWorkspaceStatus = ( const actions: ActionType[] = ["stop"]; if (workspace.template_require_active_version && workspace.outdated) { - actions.push("updateAndRestart"); + actions.push("updateAndRestartRequireActiveVersion"); } else { if (workspace.outdated) { - actions.unshift("update"); + actions.unshift("updateAndRestart"); } actions.push("restart"); } @@ -99,10 +102,10 @@ export const abilitiesByWorkspaceStatus = ( const actions: ActionType[] = []; if (workspace.template_require_active_version && workspace.outdated) { - actions.push("updateAndStart"); + actions.push("updateAndStartRequireActiveVersion"); } else { if (workspace.outdated) { - actions.unshift("update"); + actions.unshift("updateAndStart"); } actions.push("start"); } @@ -128,7 +131,7 @@ export const abilitiesByWorkspaceStatus = ( } if (workspace.outdated) { - actions.unshift("update"); + actions.unshift("updateAndStart"); } return { diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/Buttons.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/Buttons.tsx index b02e4473eb57f..e7fc2a4190116 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/Buttons.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/Buttons.tsx @@ -19,21 +19,41 @@ export interface ActionButtonProps { handleAction: (buildParameters?: WorkspaceBuildParameter[]) => void; disabled?: boolean; tooltipText?: string; + isRunning?: boolean; + requireActiveVersion?: boolean; } export const UpdateButton: FC = ({ handleAction, loading, + isRunning, + requireActiveVersion, }) => { return ( - handleAction()} + - - {loading ? <>Updating… : <>Update…} - + handleAction()} + > + {requireActiveVersion ? : } + {loading ? ( + <>Updating… + ) : isRunning ? ( + <>Update and restart… + ) : ( + <>Update and start… + )} + + ); }; @@ -84,19 +104,6 @@ export const StartButton: FC = ({ ); }; -export const UpdateAndStartButton: FC = ({ - handleAction, -}) => { - return ( - - handleAction()}> - - Update and start… - - - ); -}; - export const StopButton: FC = ({ handleAction, loading, @@ -138,19 +145,6 @@ export const RestartButton: FC = ({ ); }; -export const UpdateAndRestartButton: FC = ({ - handleAction, -}) => { - return ( - - handleAction()}> - - Update and restart… - - - ); -}; - export const CancelButton: FC = ({ handleAction }) => { return ( handleAction()}> diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx index 708c90bac6d35..1c38caa14ec21 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx @@ -18,8 +18,6 @@ import { RestartButton, StartButton, StopButton, - UpdateAndRestartButton, - UpdateAndStartButton, UpdateButton, } from "./Buttons"; import { DebugButton } from "./DebugButton"; @@ -81,9 +79,34 @@ export const WorkspaceActions: FC = ({ // A mapping of button type to the corresponding React component const buttonMapping: Record = { - update: , - updateAndStart: , - updateAndRestart: , + updateAndStart: ( + + ), + updateAndStartRequireActiveVersion: ( + + ), + updateAndRestart: ( + + ), + updateAndRestartRequireActiveVersion: ( + + ), updating: , start: ( { } await user.click(screen.getByRole("button", { name: /bulk actions/i })); - const updateButton = await screen.findByText(/update/i); + const updateButton = await screen.findByTestId("bulk-action-update"); await user.click(updateButton); // One click: no running workspaces warning, no dormant workspaces warning. @@ -146,7 +146,7 @@ describe("WorkspacesPage", () => { } await user.click(screen.getByRole("button", { name: /bulk actions/i })); - const updateButton = await screen.findByText(/update/i); + const updateButton = await screen.findByTestId("bulk-action-update"); await user.click(updateButton); // Two clicks: 1 running workspace, no dormant workspaces warning. @@ -184,7 +184,7 @@ describe("WorkspacesPage", () => { } await user.click(screen.getByRole("button", { name: /bulk actions/i })); - const updateButton = await screen.findByText(/update/i); + const updateButton = await screen.findByTestId("bulk-action-update"); await user.click(updateButton); // Two clicks: no running workspaces warning, 1 dormant workspace. @@ -224,7 +224,7 @@ describe("WorkspacesPage", () => { } await user.click(screen.getByRole("button", { name: /bulk actions/i })); - const updateButton = await screen.findByText(/update/i); + const updateButton = await screen.findByTestId("bulk-action-update"); await user.click(updateButton); // Three clicks: 1 running workspace, 1 dormant workspace. diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index db9c72568150e..6563533bc43da 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -171,7 +171,11 @@ export const WorkspacesPageView: FC = ({ - Update… + {" "} + Update… Date: Mon, 23 Jun 2025 10:46:18 +0200 Subject: [PATCH 008/690] feat: persist generated `coder_app` id (#18487) --- coderd/agentapi/apps.go | 2 +- coderd/provisionerdserver/provisionerdserver.go | 13 ++++++++++++- provisioner/terraform/resources.go | 12 ++++++++++++ provisioner/terraform/resources_test.go | 6 ++++++ provisionerd/proto/version.go | 1 + provisionersdk/proto/provisioner.pb.go | 13 +++++++++++-- provisionersdk/proto/provisioner.proto | 1 + site/e2e/provisionerGenerated.ts | 5 +++++ site/e2e/tests/app.spec.ts | 1 + 9 files changed, 50 insertions(+), 4 deletions(-) diff --git a/coderd/agentapi/apps.go b/coderd/agentapi/apps.go index 956e154e89d0d..89c1a873d6310 100644 --- a/coderd/agentapi/apps.go +++ b/coderd/agentapi/apps.go @@ -92,7 +92,7 @@ func (a *AppsAPI) BatchUpdateAppHealths(ctx context.Context, req *agentproto.Bat Health: app.Health, }) if err != nil { - return nil, xerrors.Errorf("update workspace app health for app %q (%q): %w", err, app.ID, app.Slug) + return nil, xerrors.Errorf("update workspace app health for app %q (%q): %w", app.ID, app.Slug, err) } } diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 78dcd4e993b9f..a8f7c63a586fe 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -2595,8 +2595,19 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. openIn = database.WorkspaceAppOpenInSlimWindow } + var appID string + if app.Id == "" || app.Id == uuid.Nil.String() { + appID = uuid.NewString() + } else { + appID = app.Id + } + id, err := uuid.Parse(appID) + if err != nil { + return xerrors.Errorf("parse app uuid: %w", err) + } + dbApp, err := db.InsertWorkspaceApp(ctx, database.InsertWorkspaceAppParams{ - ID: uuid.New(), + ID: id, CreatedAt: dbtime.Now(), AgentID: dbAgent.ID, Slug: slug, diff --git a/provisioner/terraform/resources.go b/provisioner/terraform/resources.go index 686a947f7fcaa..a46953f030dda 100644 --- a/provisioner/terraform/resources.go +++ b/provisioner/terraform/resources.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/awalterschulze/gographviz" + "github.com/google/uuid" tfjson "github.com/hashicorp/terraform-json" "github.com/mitchellh/mapstructure" "golang.org/x/xerrors" @@ -93,6 +94,7 @@ type agentDisplayAppsAttributes struct { // A mapping of attributes on the "coder_app" resource. type agentAppAttributes struct { + ID string `mapstructure:"id"` AgentID string `mapstructure:"agent_id"` // Slug is required in terraform, but to avoid breaking existing users we // will default to the resource name if it is not specified. @@ -522,7 +524,17 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s continue } + id := attrs.ID + if id == "" { + // This should never happen since the "id" attribute is set on creation: + // https://github.com/coder/terraform-provider-coder/blob/cfa101df4635e405e66094fa7779f9a89d92f400/provider/app.go#L37 + logger.Warn(ctx, "coder_app's id was unexpectedly empty", slog.F("name", attrs.Name)) + + id = uuid.NewString() + } + agent.Apps = append(agent.Apps, &proto.App{ + Id: id, Slug: attrs.Slug, DisplayName: attrs.DisplayName, Command: attrs.Command, diff --git a/provisioner/terraform/resources_test.go b/provisioner/terraform/resources_test.go index d21c0a9c573ff..56077eed9eee9 100644 --- a/provisioner/terraform/resources_test.go +++ b/provisioner/terraform/resources_test.go @@ -967,6 +967,9 @@ func TestConvertResources(t *testing.T) { if agent.GetInstanceId() != "" { agent.Auth = &proto.Agent_InstanceId{} } + for _, app := range agent.Apps { + app.Id = "" + } } } @@ -1037,6 +1040,9 @@ func TestConvertResources(t *testing.T) { if agent.GetInstanceId() != "" { agent.Auth = &proto.Agent_InstanceId{} } + for _, app := range agent.Apps { + app.Id = "" + } } } // Convert expectedNoMetadata and resources into a diff --git a/provisionerd/proto/version.go b/provisionerd/proto/version.go index d5ecba99030b3..3ac93d7497860 100644 --- a/provisionerd/proto/version.go +++ b/provisionerd/proto/version.go @@ -37,6 +37,7 @@ import "github.com/coder/coder/v2/apiversion" // - Add new field named `scheduling` to `Prebuild`, with fields for timezone // and schedule rules to define cron-based scaling of prebuilt workspace // instances based on time patterns. +// - Added new field named `id` to `App`, which transports the ID generated by the coder_app provider to be persisted. const ( CurrentMajor = 1 CurrentMinor = 7 diff --git a/provisionersdk/proto/provisioner.pb.go b/provisionersdk/proto/provisioner.pb.go index 81ca588efaf93..047458457fedd 100644 --- a/provisionersdk/proto/provisioner.pb.go +++ b/provisionersdk/proto/provisioner.pb.go @@ -2285,6 +2285,7 @@ type App struct { Hidden bool `protobuf:"varint,11,opt,name=hidden,proto3" json:"hidden,omitempty"` OpenIn AppOpenIn `protobuf:"varint,12,opt,name=open_in,json=openIn,proto3,enum=provisioner.AppOpenIn" json:"open_in,omitempty"` Group string `protobuf:"bytes,13,opt,name=group,proto3" json:"group,omitempty"` + Id string `protobuf:"bytes,14,opt,name=id,proto3" json:"id,omitempty"` // If nil, new UUID will be generated. } func (x *App) Reset() { @@ -2410,6 +2411,13 @@ func (x *App) GetGroup() string { return "" } +func (x *App) GetId() string { + if x != nil { + return x.Id + } + return "" +} + // Healthcheck represents configuration for checking for app readiness. type Healthcheck struct { state protoimpl.MessageState @@ -4474,7 +4482,7 @@ var file_provisionersdk_proto_provisioner_proto_rawDesc = []byte{ 0x6e, 0x66, 0x69, 0x67, 0x5f, 0x70, 0x61, 0x74, 0x68, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0a, 0x63, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x50, 0x61, 0x74, 0x68, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x22, - 0xaa, 0x03, 0x0a, 0x03, 0x41, 0x70, 0x70, 0x12, 0x12, 0x0a, 0x04, 0x73, 0x6c, 0x75, 0x67, 0x18, + 0xba, 0x03, 0x0a, 0x03, 0x41, 0x70, 0x70, 0x12, 0x12, 0x0a, 0x04, 0x73, 0x6c, 0x75, 0x67, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x73, 0x6c, 0x75, 0x67, 0x12, 0x21, 0x0a, 0x0c, 0x64, 0x69, 0x73, 0x70, 0x6c, 0x61, 0x79, 0x5f, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x64, 0x69, 0x73, 0x70, 0x6c, 0x61, 0x79, 0x4e, 0x61, 0x6d, 0x65, 0x12, 0x18, @@ -4500,7 +4508,8 @@ var file_provisionersdk_proto_provisioner_proto_rawDesc = []byte{ 0x18, 0x0c, 0x20, 0x01, 0x28, 0x0e, 0x32, 0x16, 0x2e, 0x70, 0x72, 0x6f, 0x76, 0x69, 0x73, 0x69, 0x6f, 0x6e, 0x65, 0x72, 0x2e, 0x41, 0x70, 0x70, 0x4f, 0x70, 0x65, 0x6e, 0x49, 0x6e, 0x52, 0x06, 0x6f, 0x70, 0x65, 0x6e, 0x49, 0x6e, 0x12, 0x14, 0x0a, 0x05, 0x67, 0x72, 0x6f, 0x75, 0x70, 0x18, - 0x0d, 0x20, 0x01, 0x28, 0x09, 0x52, 0x05, 0x67, 0x72, 0x6f, 0x75, 0x70, 0x22, 0x59, 0x0a, 0x0b, + 0x0d, 0x20, 0x01, 0x28, 0x09, 0x52, 0x05, 0x67, 0x72, 0x6f, 0x75, 0x70, 0x12, 0x0e, 0x0a, 0x02, + 0x69, 0x64, 0x18, 0x0e, 0x20, 0x01, 0x28, 0x09, 0x52, 0x02, 0x69, 0x64, 0x22, 0x59, 0x0a, 0x0b, 0x48, 0x65, 0x61, 0x6c, 0x74, 0x68, 0x63, 0x68, 0x65, 0x63, 0x6b, 0x12, 0x10, 0x0a, 0x03, 0x75, 0x72, 0x6c, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x75, 0x72, 0x6c, 0x12, 0x1a, 0x0a, 0x08, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x76, 0x61, 0x6c, 0x18, 0x02, 0x20, 0x01, 0x28, 0x05, 0x52, diff --git a/provisionersdk/proto/provisioner.proto b/provisionersdk/proto/provisioner.proto index cd4eb4960eb11..12dd709aeba7f 100644 --- a/provisionersdk/proto/provisioner.proto +++ b/provisionersdk/proto/provisioner.proto @@ -266,6 +266,7 @@ message App { bool hidden = 11; AppOpenIn open_in = 12; string group = 13; + string id = 14; // If nil, new UUID will be generated. } // Healthcheck represents configuration for checking for app readiness. diff --git a/site/e2e/provisionerGenerated.ts b/site/e2e/provisionerGenerated.ts index ee53f18d66d58..9b6a0b3109ef7 100644 --- a/site/e2e/provisionerGenerated.ts +++ b/site/e2e/provisionerGenerated.ts @@ -311,6 +311,8 @@ export interface App { hidden: boolean; openIn: AppOpenIn; group: string; + /** If nil, new UUID will be generated. */ + id: string; } /** Healthcheck represents configuration for checking for app readiness. */ @@ -1041,6 +1043,9 @@ export const App = { if (message.group !== "") { writer.uint32(106).string(message.group); } + if (message.id !== "") { + writer.uint32(114).string(message.id); + } return writer; }, }; diff --git a/site/e2e/tests/app.spec.ts b/site/e2e/tests/app.spec.ts index a12c1baccd735..587775b4dc3f8 100644 --- a/site/e2e/tests/app.spec.ts +++ b/site/e2e/tests/app.spec.ts @@ -42,6 +42,7 @@ test("app", async ({ context, page }) => { token, apps: [ { + id: randomUUID(), url: `http://localhost:${addr.port}`, displayName: appName, order: 0, From 0a12ec5fd83c7d38cb9ff958b2f57f7d9f22fa8c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 Jun 2025 10:32:30 +0100 Subject: [PATCH 009/690] fix(enterprise/coderd): skip org membership check for prebuilds user on group patch (#18329) Currently, the prebuilds documentation states: ``` ### Managing resource quotas Prebuilt workspaces can be used in conjunction with [resource quotas](../../users/quotas.md). Because unclaimed prebuilt workspaces are owned by the `prebuilds` user, you can: 1. Configure quotas for any group that includes this user. 1. Set appropriate limits to balance prebuilt workspace availability with resource constraints. If a quota is exceeded, the prebuilt workspace will fail provisioning the same way other workspaces do. ``` If you need to have a separate quota for prebuilds as opposed to regular users, you are required to create a separate group, as quotas are applied to groups. Currently it is not possible to create a separate 'prebuilds' group with only the prebuilds user to add a quota. This PR skips the org membership check specifically for the prebuilds user when patching a group. ![image](https://github.com/user-attachments/assets/2ff566bb-97bd-4c73-917a-903ea54dd7a6) --- enterprise/coderd/groups.go | 6 ++++++ enterprise/coderd/groups_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index cfe5d081271e3..89671e00bd65c 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -171,6 +171,12 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { }) return } + // Skip membership checks for the prebuilds user. There is a valid use case + // for adding the prebuilds user to a single group: in order to set a quota + // allowance specifically for prebuilds. + if id == database.PrebuildsSystemUserID.String() { + continue + } _, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ OrganizationID: group.OrganizationID, UserID: uuid.MustParse(id), diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index f87a9193f5fa4..568825adcd0ea 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -463,6 +463,32 @@ func TestPatchGroup(t *testing.T) { require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) }) + // For quotas to work with prebuilds, it's currently required to add the + // prebuilds user into a group with a quota allowance. + // See: docs/admin/templates/extending-templates/prebuilt-workspaces.md + t.Run("PrebuildsUser", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + userAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleUserAdmin()) + ctx := testutil.Context(t, testutil.WaitLong) + group, err := userAdminClient.CreateGroup(ctx, user.OrganizationID, codersdk.CreateGroupRequest{ + Name: "prebuilds", + QuotaAllowance: 123, + }) + require.NoError(t, err) + + group, err = userAdminClient.PatchGroup(ctx, group.ID, codersdk.PatchGroupRequest{ + Name: "prebuilds", + AddUsers: []string{database.PrebuildsSystemUserID.String()}, + }) + require.NoError(t, err) + }) + t.Run("Everyone", func(t *testing.T) { t.Parallel() t.Run("NoUpdateName", func(t *testing.T) { From c3bc1e75ec0406866ce4eb62c9aeb297e8824af1 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 23 Jun 2025 13:05:39 +0100 Subject: [PATCH 010/690] feat(agent/agentcontainers): fall back to workspace folder name (#18466) This PR changes the logic for how we decide on an agent name. Previously it followed these steps: 1. Use a name from `customizations.coder.name` 2. Use a name from the terraform resource `coder_devcontainer` 3. Use the dev container's friendly name With this change it now does: 1. Use a name from `customizations.coder.name` 2. Use a name from the terraform resource `coder_devcontainer` 3. Use a name from the workspace folder 4. Use the dev container's friendly name We now attempt to construct a valid agent name from the workspace folder. Should we fail to construct a valid agent name from the workspace folder, we will fall back to the dev container's friendly name. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- agent/agentcontainers/api.go | 44 ++++- agent/agentcontainers/api_internal_test.go | 201 +++++++++++++++++++++ agent/agentcontainers/api_test.go | 28 +-- 3 files changed, 255 insertions(+), 18 deletions(-) create mode 100644 agent/agentcontainers/api_internal_test.go diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index ef2b7aa7ebcd2..8896c3217558f 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -8,6 +8,7 @@ import ( "os" "path" "path/filepath" + "regexp" "runtime" "slices" "strings" @@ -39,6 +40,8 @@ const ( // by tmpfs or other mounts. This assumes the container root filesystem is // read-write, which seems sensible for devcontainers. coderPathInsideContainer = "/.coder-agent/coder" + + maxAgentNameLength = 64 ) // API is responsible for container-related operations in the agent. @@ -583,10 +586,11 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code if dc.Container != nil { if !api.devcontainerNames[dc.Name] { // If the devcontainer name wasn't set via terraform, we - // use the containers friendly name as a fallback which - // will keep changing as the devcontainer is recreated. - // TODO(mafredri): Parse the container label (i.e. devcontainer.json) for customization. - dc.Name = safeFriendlyName(dc.Container.FriendlyName) + // will attempt to create an agent name based on the workspace + // folder's name. If it is not possible to generate a valid + // agent name based off of the folder name (i.e. no valid characters), + // we will instead fall back to using the container's friendly name. + dc.Name = safeAgentName(path.Base(filepath.ToSlash(dc.WorkspaceFolder)), dc.Container.FriendlyName) } } @@ -631,6 +635,38 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code api.containersErr = nil } +var consecutiveHyphenRegex = regexp.MustCompile("-+") + +// `safeAgentName` returns a safe agent name derived from a folder name, +// falling back to the container’s friendly name if needed. +func safeAgentName(name string, friendlyName string) string { + // Keep only ASCII letters and digits, replacing everything + // else with a hyphen. + var sb strings.Builder + for _, r := range strings.ToLower(name) { + if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') { + _, _ = sb.WriteRune(r) + } else { + _, _ = sb.WriteRune('-') + } + } + + // Remove any consecutive hyphens, and then trim any leading + // and trailing hyphens. + name = consecutiveHyphenRegex.ReplaceAllString(sb.String(), "-") + name = strings.Trim(name, "-") + + // Ensure the name of the agent doesn't exceed the maximum agent + // name length. + name = name[:min(len(name), maxAgentNameLength)] + + if provisioner.AgentNameRegex.Match([]byte(name)) { + return name + } + + return safeFriendlyName(friendlyName) +} + // safeFriendlyName returns a API safe version of the container's // friendly name. // diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go new file mode 100644 index 0000000000000..bda6371f63e5e --- /dev/null +++ b/agent/agentcontainers/api_internal_test.go @@ -0,0 +1,201 @@ +package agentcontainers + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/coder/coder/v2/provisioner" +) + +func TestSafeAgentName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + folderName string + expected string + }{ + // Basic valid names + { + folderName: "simple", + expected: "simple", + }, + { + folderName: "with-hyphens", + expected: "with-hyphens", + }, + { + folderName: "123numbers", + expected: "123numbers", + }, + { + folderName: "mixed123", + expected: "mixed123", + }, + + // Names that need transformation + { + folderName: "With_Underscores", + expected: "with-underscores", + }, + { + folderName: "With Spaces", + expected: "with-spaces", + }, + { + folderName: "UPPERCASE", + expected: "uppercase", + }, + { + folderName: "Mixed_Case-Name", + expected: "mixed-case-name", + }, + + // Names with special characters that get replaced + { + folderName: "special@#$chars", + expected: "special-chars", + }, + { + folderName: "dots.and.more", + expected: "dots-and-more", + }, + { + folderName: "multiple___underscores", + expected: "multiple-underscores", + }, + { + folderName: "multiple---hyphens", + expected: "multiple-hyphens", + }, + + // Edge cases with leading/trailing special chars + { + folderName: "-leading-hyphen", + expected: "leading-hyphen", + }, + { + folderName: "trailing-hyphen-", + expected: "trailing-hyphen", + }, + { + folderName: "_leading_underscore", + expected: "leading-underscore", + }, + { + folderName: "trailing_underscore_", + expected: "trailing-underscore", + }, + { + folderName: "---multiple-leading", + expected: "multiple-leading", + }, + { + folderName: "trailing-multiple---", + expected: "trailing-multiple", + }, + + // Complex transformation cases + { + folderName: "___very---complex@@@name___", + expected: "very-complex-name", + }, + { + folderName: "my.project-folder_v2", + expected: "my-project-folder-v2", + }, + + // Empty and fallback cases - now correctly uses friendlyName fallback + { + folderName: "", + expected: "friendly-fallback", + }, + { + folderName: "---", + expected: "friendly-fallback", + }, + { + folderName: "___", + expected: "friendly-fallback", + }, + { + folderName: "@#$", + expected: "friendly-fallback", + }, + + // Additional edge cases + { + folderName: "a", + expected: "a", + }, + { + folderName: "1", + expected: "1", + }, + { + folderName: "a1b2c3", + expected: "a1b2c3", + }, + { + folderName: "CamelCase", + expected: "camelcase", + }, + { + folderName: "snake_case_name", + expected: "snake-case-name", + }, + { + folderName: "kebab-case-name", + expected: "kebab-case-name", + }, + { + folderName: "mix3d_C4s3-N4m3", + expected: "mix3d-c4s3-n4m3", + }, + { + folderName: "123-456-789", + expected: "123-456-789", + }, + { + folderName: "abc123def456", + expected: "abc123def456", + }, + { + folderName: " spaces everywhere ", + expected: "spaces-everywhere", + }, + { + folderName: "unicode-café-naïve", + expected: "unicode-caf-na-ve", + }, + { + folderName: "path/with/slashes", + expected: "path-with-slashes", + }, + { + folderName: "file.tar.gz", + expected: "file-tar-gz", + }, + { + folderName: "version-1.2.3-alpha", + expected: "version-1-2-3-alpha", + }, + + // Truncation test for names exceeding 64 characters + { + folderName: "this-is-a-very-long-folder-name-that-exceeds-sixty-four-characters-limit-and-should-be-truncated", + expected: "this-is-a-very-long-folder-name-that-exceeds-sixty-four-characte", + }, + } + + for _, tt := range tests { + t.Run(tt.folderName, func(t *testing.T) { + t.Parallel() + name := safeAgentName(tt.folderName, "friendly-fallback") + + assert.Equal(t, tt.expected, name) + assert.True(t, provisioner.AgentNameRegex.Match([]byte(name))) + }) + } +} diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 4e3e9e4077cd7..a59a3bfd6731e 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -897,8 +897,8 @@ func TestAPI(t *testing.T) { FriendlyName: "project1-container", Running: true, Labels: map[string]string{ - agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project", - agentcontainers.DevcontainerConfigFileLabel: "/workspace/project/.devcontainer/devcontainer.json", + agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project1", + agentcontainers.DevcontainerConfigFileLabel: "/workspace/project1/.devcontainer/devcontainer.json", }, }, { @@ -906,8 +906,8 @@ func TestAPI(t *testing.T) { FriendlyName: "project2-container", Running: true, Labels: map[string]string{ - agentcontainers.DevcontainerLocalFolderLabel: "/home/user/project", - agentcontainers.DevcontainerConfigFileLabel: "/home/user/project/.devcontainer/devcontainer.json", + agentcontainers.DevcontainerLocalFolderLabel: "/home/user/project2", + agentcontainers.DevcontainerConfigFileLabel: "/home/user/project2/.devcontainer/devcontainer.json", }, }, { @@ -915,8 +915,8 @@ func TestAPI(t *testing.T) { FriendlyName: "project3-container", Running: true, Labels: map[string]string{ - agentcontainers.DevcontainerLocalFolderLabel: "/var/lib/project", - agentcontainers.DevcontainerConfigFileLabel: "/var/lib/project/.devcontainer/devcontainer.json", + agentcontainers.DevcontainerLocalFolderLabel: "/var/lib/project3", + agentcontainers.DevcontainerConfigFileLabel: "/var/lib/project3/.devcontainer/devcontainer.json", }, }, }, @@ -1326,7 +1326,7 @@ func TestAPI(t *testing.T) { // Allow initial agent creation and injection to succeed. testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { - assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") + assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=coder") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") assert.Contains(t, envs, "CODER_WORKSPACE_OWNER_NAME=test-user") assert.Contains(t, envs, "CODER_URL=test-subagent-url") @@ -1349,7 +1349,7 @@ func TestAPI(t *testing.T) { // Verify agent was created. require.Len(t, fakeSAC.created, 1) - assert.Equal(t, "test-container", fakeSAC.created[0].Name) + assert.Equal(t, "coder", fakeSAC.created[0].Name) assert.Equal(t, "/workspaces/coder", fakeSAC.created[0].Directory) assert.Len(t, fakeSAC.deleted, 0) @@ -1405,7 +1405,7 @@ func TestAPI(t *testing.T) { WaitStartLoop: for { // Agent reinjection will succeed and we will not re-create the - // agent, nor re-probe pwd. + // agent. mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{testContainer}, }, nil).Times(1) // 1 update. @@ -1468,7 +1468,7 @@ func TestAPI(t *testing.T) { // Expect the agent to be recreated. testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { - assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") + assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=coder") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") assert.Contains(t, envs, "CODER_WORKSPACE_OWNER_NAME=test-user") assert.Contains(t, envs, "CODER_URL=test-subagent-url") @@ -1910,8 +1910,8 @@ func TestAPI(t *testing.T) { Running: true, CreatedAt: time.Now(), Labels: map[string]string{ - agentcontainers.DevcontainerLocalFolderLabel: "/workspaces", - agentcontainers.DevcontainerConfigFileLabel: "/workspace/.devcontainer/devcontainer.json", + agentcontainers.DevcontainerLocalFolderLabel: "/workspaces/coder", + agentcontainers.DevcontainerConfigFileLabel: "/workspaces/coder/.devcontainer/devcontainer.json", }, } ) @@ -1953,13 +1953,13 @@ func TestAPI(t *testing.T) { testutil.RequireSend(ctx, t, fSAC.createErrC, nil) testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { // We expect the wrong workspace agent name passed in first. - assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container") + assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=coder") return nil }) testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { // We then expect the agent name passed here to have been read from the config. assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=custom-name") - assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container") + assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=coder") return nil }) From 2f55e2946695762025638959923a824fea0f3f8e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 Jun 2025 14:07:42 +0100 Subject: [PATCH 011/690] fix: complete job and mark workspace as deleted when no provisioners are available (#18465) Alternate fix for https://github.com/coder/coder/issues/18080 Modifies wsbuilder to complete the provisioner job and mark the workspace as deleted if it is clear that no provisioner will be able to pick up the delete build. This has a significant advantage of not deviating too much from the current semantics of `POST /api/v2/workspacebuilds`. https://github.com/coder/coder/pull/18460 ends up returning a 204 on orphan delete due to no build being created. Downside is that we have to duplicate some responsibilities of provisionerdserver in wsbuilder. There is a slight gotcha to this approach though: if you stop a provisioner and then immediately try to orphan-delete, the job will still be created because of the provisioner heartbeat interval. However you can cancel it and try again. --- cli/delete_test.go | 26 ++- coderd/database/dbmem/dbmem.go | 3 +- coderd/workspacebuilds.go | 45 +++- coderd/workspacebuilds_test.go | 192 +++++++++++++++--- coderd/wsbuilder/wsbuilder.go | 44 ++++ coderd/wsbuilder/wsbuilder_test.go | 188 +++++++++++++++++ .../WorkspaceDeleteDialog.tsx | 90 ++++---- 7 files changed, 502 insertions(+), 86 deletions(-) diff --git a/cli/delete_test.go b/cli/delete_test.go index ecd1c6996df1d..a48ca98627f65 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "io" + "net/http" "testing" "time" @@ -60,28 +61,35 @@ func TestDelete(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) owner := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, templateAdmin, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, workspace.LatestBuild.ID) + + ctx := testutil.Context(t, testutil.WaitShort) inv, root := clitest.New(t, "delete", workspace.Name, "-y", "--orphan") + clitest.SetupConfig(t, templateAdmin, root) - //nolint:gocritic // Deleting orphaned workspaces requires an admin. - clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) pty := ptytest.New(t).Attach(inv) inv.Stderr = pty.Output() go func() { defer close(doneChan) - err := inv.Run() + err := inv.WithContext(ctx).Run() // When running with the race detector on, we sometimes get an EOF. if err != nil { assert.ErrorIs(t, err, io.EOF) } }() pty.ExpectMatch("has been deleted") - <-doneChan + testutil.TryReceive(ctx, t, doneChan) + + _, err := client.Workspace(ctx, workspace.ID) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusGone, cerr.StatusCode()) }) // Super orphaned, as the workspace doesn't even have a user. diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 25b44e2930892..f4403d2f50c75 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -4497,7 +4497,8 @@ func (q *FakeQuerier) GetProvisionerDaemons(_ context.Context) ([]database.Provi defer q.mutex.RUnlock() if len(q.provisionerDaemons) == 0 { - return nil, sql.ErrNoRows + // Returning err=nil here for consistency with real querier + return []database.ProvisionerDaemon{}, nil } // copy the data so that the caller can't manipulate any data inside dbmem // after returning diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 74946d46dcd9f..15614f84b4f70 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -3,6 +3,7 @@ package coderd import ( "context" "database/sql" + "encoding/json" "errors" "fmt" "math" @@ -433,20 +434,56 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { return } + var queuePos database.GetProvisionerJobsByIDsWithQueuePositionRow if provisionerJob != nil { + queuePos.ProvisionerJob = *provisionerJob + queuePos.QueuePosition = 0 if err := provisionerjobs.PostJob(api.Pubsub, *provisionerJob); err != nil { // Client probably doesn't care about this error, so just log it. api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err)) } + + // We may need to complete the audit if wsbuilder determined that + // no provisioner could handle an orphan-delete job and completed it. + if createBuild.Orphan && createBuild.Transition == codersdk.WorkspaceTransitionDelete && provisionerJob.CompletedAt.Valid { + api.Logger.Warn(ctx, "orphan delete handled by wsbuilder due to no eligible provisioners", + slog.F("workspace_id", workspace.ID), + slog.F("workspace_build_id", workspaceBuild.ID), + slog.F("provisioner_job_id", provisionerJob.ID), + ) + buildResourceInfo := audit.AdditionalFields{ + WorkspaceName: workspace.Name, + BuildNumber: strconv.Itoa(int(workspaceBuild.BuildNumber)), + BuildReason: workspaceBuild.Reason, + WorkspaceID: workspace.ID, + WorkspaceOwner: workspace.OwnerName, + } + briBytes, err := json.Marshal(buildResourceInfo) + if err != nil { + api.Logger.Error(ctx, "failed to marshal build resource info for audit", slog.Error(err)) + } + auditor := api.Auditor.Load() + bag := audit.BaggageFromContext(ctx) + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.WorkspaceBuild]{ + Audit: *auditor, + Log: api.Logger, + UserID: provisionerJob.InitiatorID, + OrganizationID: workspace.OrganizationID, + RequestID: provisionerJob.ID, + IP: bag.IP, + Action: database.AuditActionDelete, + Old: previousWorkspaceBuild, + New: *workspaceBuild, + Status: http.StatusOK, + AdditionalFields: briBytes, + }) + } } apiBuild, err := api.convertWorkspaceBuild( *workspaceBuild, workspace, - database.GetProvisionerJobsByIDsWithQueuePositionRow{ - ProvisionerJob: *provisionerJob, - QueuePosition: 0, - }, + queuePos, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 081accd097d69..b9d32a00b139a 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -1,6 +1,7 @@ package coderd_test import ( + "bytes" "context" "database/sql" "errors" @@ -25,6 +26,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbfake" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -371,42 +373,174 @@ func TestWorkspaceBuildsProvisionerState(t *testing.T) { t.Run("Orphan", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - first := coderdtest.CreateFirstUser(t, client) - - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + t.Run("WithoutDelete", func(t *testing.T) { + t.Parallel() + client, store := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, client) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + + r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ + OwnerID: templateAdminUser.ID, + OrganizationID: first.OrganizationID, + }).Do() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Trying to orphan without delete transition fails. + _, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: r.TemplateVersion.ID, + Transition: codersdk.WorkspaceTransitionStart, + Orphan: true, + }) + require.Error(t, err, "Orphan is only permitted when deleting a workspace.") + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) - workspace := coderdtest.CreateWorkspace(t, client, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + t.Run("WithState", func(t *testing.T) { + t.Parallel() + client, store := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, client) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + + r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ + OwnerID: templateAdminUser.ID, + OrganizationID: first.OrganizationID, + }).Do() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Providing both state and orphan fails. + _, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: r.TemplateVersion.ID, + Transition: codersdk.WorkspaceTransitionDelete, + ProvisionerState: []byte(" "), + Orphan: true, + }) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) - // Providing both state and orphan fails. - _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: workspace.LatestBuild.TemplateVersionID, - Transition: codersdk.WorkspaceTransitionDelete, - ProvisionerState: []byte(" "), - Orphan: true, + t.Run("NoPermission", func(t *testing.T) { + t.Parallel() + client, store := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, client) + member, memberUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ + OwnerID: memberUser.ID, + OrganizationID: first.OrganizationID, + }).Do() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Trying to orphan without being a template admin fails. + _, err := member.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: r.TemplateVersion.ID, + Transition: codersdk.WorkspaceTransitionDelete, + Orphan: true, + }) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusForbidden, cerr.StatusCode()) }) - require.Error(t, err) - cerr := coderdtest.SDKError(t, err) - require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) - // Regular orphan operation succeeds. - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: workspace.LatestBuild.TemplateVersionID, - Transition: codersdk.WorkspaceTransitionDelete, - Orphan: true, + t.Run("OK", func(t *testing.T) { + // Include a provisioner so that we can test that provisionerdserver + // performs deletion. + auditor := audit.NewMock() + client, store := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor}) + first := coderdtest.CreateFirstUser(t, client) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + // This is a valid zip file. Without this the job will fail to complete. + // TODO: add this to dbfake by default. + zipBytes := make([]byte, 22) + zipBytes[0] = 80 + zipBytes[1] = 75 + zipBytes[2] = 0o5 + zipBytes[3] = 0o6 + uploadRes, err := client.Upload(ctx, codersdk.ContentTypeZip, bytes.NewReader(zipBytes)) + require.NoError(t, err) + + tv := dbfake.TemplateVersion(t, store). + FileID(uploadRes.ID). + Seed(database.TemplateVersion{ + OrganizationID: first.OrganizationID, + CreatedBy: templateAdminUser.ID, + }). + Do() + + r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ + OwnerID: templateAdminUser.ID, + OrganizationID: first.OrganizationID, + TemplateID: tv.Template.ID, + }).Do() + + auditor.ResetLogs() + // Regular orphan operation succeeds. + build, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: r.TemplateVersion.ID, + Transition: codersdk.WorkspaceTransitionDelete, + Orphan: true, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) + + // Validate that the deletion was audited. + require.True(t, auditor.Contains(t, database.AuditLog{ + ResourceID: build.ID, + Action: database.AuditActionDelete, + })) }) - require.NoError(t, err) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) - _, err = client.Workspace(ctx, workspace.ID) - require.Error(t, err) - require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode()) + t.Run("NoProvisioners", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + client, store := coderdtest.NewWithDatabase(t, &coderdtest.Options{Auditor: auditor}) + first := coderdtest.CreateFirstUser(t, client) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ + OwnerID: templateAdminUser.ID, + OrganizationID: first.OrganizationID, + }).Do() + + // nolint:gocritic // For testing + daemons, err := store.GetProvisionerDaemons(dbauthz.AsSystemReadProvisionerDaemons(ctx)) + require.NoError(t, err) + require.Empty(t, daemons, "Provisioner daemons should be empty for this test") + + // Orphan deletion still succeeds despite no provisioners being available. + build, err := templateAdmin.CreateWorkspaceBuild(ctx, r.Workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: r.TemplateVersion.ID, + Transition: codersdk.WorkspaceTransitionDelete, + Orphan: true, + }) + require.NoError(t, err) + require.Equal(t, codersdk.WorkspaceTransitionDelete, build.Transition) + require.Equal(t, codersdk.ProvisionerJobSucceeded, build.Job.Status) + require.Empty(t, build.Job.Error) + + ws, err := client.Workspace(ctx, r.Workspace.ID) + require.Empty(t, ws) + require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode()) + + // Validate that the deletion was audited. + require.True(t, auditor.Contains(t, database.AuditLog{ + ResourceID: build.ID, + Action: database.AuditActionDelete, + })) + }) }) } diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index b52a20ac1e9db..a996d1594a50d 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -464,6 +464,50 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object return BuildError{http.StatusInternalServerError, "get workspace build", err} } + // If the requestor is trying to orphan-delete a workspace and there are no + // provisioners available, we should complete the build and mark the + // workspace as deleted ourselves. + // There are cases where tagged provisioner daemons have been decommissioned + // without deleting the relevant workspaces, and without any provisioners + // available these workspaces cannot be deleted. + // Orphan-deleting a workspace sends an empty state to Terraform, which means + // it won't actually delete anything. So we actually don't need to execute a + // provisioner job at all for an orphan delete, but deleting without a workspace + // build or provisioner job would result in no audit log entry, which is a deal-breaker. + hasActiveEligibleProvisioner := false + for _, pd := range provisionerDaemons { + age := now.Sub(pd.ProvisionerDaemon.LastSeenAt.Time) + if age <= provisionerdserver.StaleInterval { + hasActiveEligibleProvisioner = true + break + } + } + if b.state.orphan && !hasActiveEligibleProvisioner { + // nolint: gocritic // At this moment, we are pretending to be provisionerd. + if err := store.UpdateProvisionerJobWithCompleteWithStartedAtByID(dbauthz.AsProvisionerd(b.ctx), database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams{ + CompletedAt: sql.NullTime{Valid: true, Time: now}, + Error: sql.NullString{Valid: false}, + ErrorCode: sql.NullString{Valid: false}, + ID: provisionerJob.ID, + StartedAt: sql.NullTime{Valid: true, Time: now}, + UpdatedAt: now, + }); err != nil { + return BuildError{http.StatusInternalServerError, "mark orphan-delete provisioner job as completed", err} + } + + // Re-fetch the completed provisioner job. + if pj, err := store.GetProvisionerJobByID(b.ctx, provisionerJob.ID); err == nil { + provisionerJob = pj + } + + if err := store.UpdateWorkspaceDeletedByID(b.ctx, database.UpdateWorkspaceDeletedByIDParams{ + ID: b.workspace.ID, + Deleted: true, + }); err != nil { + return BuildError{http.StatusInternalServerError, "mark workspace as deleted", err} + } + } + return nil }, nil) if err != nil { diff --git a/coderd/wsbuilder/wsbuilder_test.go b/coderd/wsbuilder/wsbuilder_test.go index 58999a33e6e5e..f6b4756455f14 100644 --- a/coderd/wsbuilder/wsbuilder_test.go +++ b/coderd/wsbuilder/wsbuilder_test.go @@ -839,6 +839,147 @@ func TestWorkspaceBuildWithPreset(t *testing.T) { req.NoError(err) } +func TestWorkspaceBuildDeleteOrphan(t *testing.T) { + t.Parallel() + + t.Run("WithActiveProvisioners", func(t *testing.T) { + t.Parallel() + req := require.New(t) + asrt := assert.New(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var buildID uuid.UUID + + mDB := expectDB(t, + // Inputs + withTemplate, + withInactiveVersion(nil), + withLastBuildFound, + withTemplateVersionVariables(inactiveVersionID, nil), + withRichParameters(nil), + withWorkspaceTags(inactiveVersionID, nil), + withProvisionerDaemons([]database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow{{ + JobID: inactiveJobID, + ProvisionerDaemon: database.ProvisionerDaemon{ + LastSeenAt: sql.NullTime{Valid: true, Time: dbtime.Now()}, + }, + }}), + + // Outputs + expectProvisionerJob(func(job database.InsertProvisionerJobParams) { + asrt.Equal(userID, job.InitiatorID) + asrt.Equal(inactiveFileID, job.FileID) + input := provisionerdserver.WorkspaceProvisionJob{} + err := json.Unmarshal(job.Input, &input) + req.NoError(err) + // store build ID for later + buildID = input.WorkspaceBuildID + }), + + withInTx, + expectBuild(func(bld database.InsertWorkspaceBuildParams) { + asrt.Equal(inactiveVersionID, bld.TemplateVersionID) + asrt.Equal(workspaceID, bld.WorkspaceID) + asrt.Equal(int32(2), bld.BuildNumber) + asrt.Empty(string(bld.ProvisionerState)) + asrt.Equal(userID, bld.InitiatorID) + asrt.Equal(database.WorkspaceTransitionDelete, bld.Transition) + asrt.Equal(database.BuildReasonInitiator, bld.Reason) + asrt.Equal(buildID, bld.ID) + }), + withBuild, + expectBuildParameters(func(params database.InsertWorkspaceBuildParametersParams) { + asrt.Equal(buildID, params.WorkspaceBuildID) + asrt.Empty(params.Name) + asrt.Empty(params.Value) + }), + ) + + ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} + uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan() + // nolint: dogsled + _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + req.NoError(err) + }) + + t.Run("NoActiveProvisioners", func(t *testing.T) { + t.Parallel() + req := require.New(t) + asrt := assert.New(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var buildID uuid.UUID + var jobID uuid.UUID + + mDB := expectDB(t, + // Inputs + withTemplate, + withInactiveVersion(nil), + withLastBuildFound, + withTemplateVersionVariables(inactiveVersionID, nil), + withRichParameters(nil), + withWorkspaceTags(inactiveVersionID, nil), + withProvisionerDaemons([]database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow{}), + + // Outputs + expectProvisionerJob(func(job database.InsertProvisionerJobParams) { + asrt.Equal(userID, job.InitiatorID) + asrt.Equal(inactiveFileID, job.FileID) + input := provisionerdserver.WorkspaceProvisionJob{} + err := json.Unmarshal(job.Input, &input) + req.NoError(err) + // store build ID for later + buildID = input.WorkspaceBuildID + // store job ID for later + jobID = job.ID + }), + + withInTx, + expectBuild(func(bld database.InsertWorkspaceBuildParams) { + asrt.Equal(inactiveVersionID, bld.TemplateVersionID) + asrt.Equal(workspaceID, bld.WorkspaceID) + asrt.Equal(int32(2), bld.BuildNumber) + asrt.Empty(string(bld.ProvisionerState)) + asrt.Equal(userID, bld.InitiatorID) + asrt.Equal(database.WorkspaceTransitionDelete, bld.Transition) + asrt.Equal(database.BuildReasonInitiator, bld.Reason) + asrt.Equal(buildID, bld.ID) + }), + withBuild, + expectBuildParameters(func(params database.InsertWorkspaceBuildParametersParams) { + asrt.Equal(buildID, params.WorkspaceBuildID) + asrt.Empty(params.Name) + asrt.Empty(params.Value) + }), + + // Because no provisioners were available and the request was to delete --orphan + expectUpdateProvisionerJobWithCompleteWithStartedAtByID(func(params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams) { + asrt.Equal(jobID, params.ID) + asrt.False(params.Error.Valid) + asrt.True(params.CompletedAt.Valid) + asrt.True(params.StartedAt.Valid) + }), + expectUpdateWorkspaceDeletedByID(func(params database.UpdateWorkspaceDeletedByIDParams) { + asrt.Equal(workspaceID, params.ID) + asrt.True(params.Deleted) + }), + expectGetProvisionerJobByID(func(job database.ProvisionerJob) { + asrt.Equal(jobID, job.ID) + }), + ) + + ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} + uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan() + // nolint: dogsled + _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + req.NoError(err) + }) +} + func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) { t.Parallel() @@ -1107,6 +1248,53 @@ func expectProvisionerJob( } } +// expectUpdateProvisionerJobWithCompleteWithStartedAtByID asserts a call to +// expectUpdateProvisionerJobWithCompleteWithStartedAtByID and runs the provided +// assertions against it. +func expectUpdateProvisionerJobWithCompleteWithStartedAtByID(assertions func(params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams)) func(mTx *dbmock.MockStore) { + return func(mTx *dbmock.MockStore) { + mTx.EXPECT().UpdateProvisionerJobWithCompleteWithStartedAtByID(gomock.Any(), gomock.Any()). + Times(1). + DoAndReturn( + func(ctx context.Context, params database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams) error { + assertions(params) + return nil + }, + ) + } +} + +// expectUpdateWorkspaceDeletedByID asserts a call to UpdateWorkspaceDeletedByID +// and runs the provided assertions against it. +func expectUpdateWorkspaceDeletedByID(assertions func(params database.UpdateWorkspaceDeletedByIDParams)) func(mTx *dbmock.MockStore) { + return func(mTx *dbmock.MockStore) { + mTx.EXPECT().UpdateWorkspaceDeletedByID(gomock.Any(), gomock.Any()). + Times(1). + DoAndReturn( + func(ctx context.Context, params database.UpdateWorkspaceDeletedByIDParams) error { + assertions(params) + return nil + }, + ) + } +} + +// expectGetProvisionerJobByID asserts a call to GetProvisionerJobByID +// and runs the provided assertions against it. +func expectGetProvisionerJobByID(assertions func(job database.ProvisionerJob)) func(mTx *dbmock.MockStore) { + return func(mTx *dbmock.MockStore) { + mTx.EXPECT().GetProvisionerJobByID(gomock.Any(), gomock.Any()). + Times(1). + DoAndReturn( + func(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) { + job := database.ProvisionerJob{ID: id} + assertions(job) + return job, nil + }, + ) + } +} + func withBuild(mTx *dbmock.MockStore) { mTx.EXPECT().GetWorkspaceBuildByID(gomock.Any(), gomock.Any()).Times(1). DoAndReturn(func(ctx context.Context, id uuid.UUID) (database.WorkspaceBuild, error) { diff --git a/site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx b/site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx index 8f5179b0b64da..2cfb74f2765c3 100644 --- a/site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx +++ b/site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx @@ -43,6 +43,18 @@ export const WorkspaceDeleteDialog: FC = ({ const hasError = !deletionConfirmed && userConfirmationText.length > 0; const displayErrorMessage = hasError && !isFocused; const inputColor = hasError ? "error" : "primary"; + // Orphaning is sort of a "last resort" that should really only + // be used under the following circumstances: + // a) Terraform is failing to apply while deleting, which + // usually means that builds are failing as well. + // b) No provisioner is available to delete the workspace, which will + // cause the job to remain in the "pending" state indefinitely. + // The assumption here is that an admin will cancel the job, in which + // case we want to allow them to perform an orphan-delete. + const canOrphan = + canDeleteFailedWorkspace && + (workspace.latest_build.status === "failed" || + workspace.latest_build.status === "canceled"); return ( = ({ "data-testid": "delete-dialog-name-confirmation", }} /> - { - // Orphaning is sort of a "last resort" that should really only - // be used if Terraform is failing to apply while deleting, which - // usually means that builds are failing as well. - canDeleteFailedWorkspace && - workspace.latest_build.status === "failed" && ( -
-
- { - setOrphanWorkspace(!orphanWorkspace); - }} - className="option" - name="orphan_resources" - checked={orphanWorkspace} - data-testid="orphan-checkbox" - /> -
-
-

Orphan Resources

- - As a Template Admin, you may skip resource cleanup to - delete a failed workspace. Resources such as volumes and - virtual machines will not be destroyed.  - - Learn more... - - -
-
- ) - } + {canOrphan && ( +
+
+ { + setOrphanWorkspace(!orphanWorkspace); + }} + className="option" + name="orphan_resources" + checked={orphanWorkspace} + data-testid="orphan-checkbox" + /> +
+
+

Orphan Resources

+ + As a Template Admin, you may skip resource cleanup to delete + a failed workspace. Resources such as volumes and virtual + machines will not be destroyed.  + + Learn more... + + +
+
+ )} } From 183eaa21dc2d856febdbe9ef602f80eba64c2d92 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Jun 2025 13:46:08 +0000 Subject: [PATCH 012/690] ci: bump the github-actions group with 5 updates (#18497) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore ` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore ` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore ` will remove the ignore condition of the specified dependency and ignore conditions
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/ci.yaml | 4 ++-- .github/workflows/docker-base.yaml | 2 +- .github/workflows/docs-ci.yaml | 2 +- .github/workflows/dogfood.yaml | 6 +++--- .github/workflows/release.yaml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0537cc16b7f0c..b0a27b2db4de7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -902,7 +902,7 @@ jobs: # the check to pass. This is desired in PRs, but not in mainline. - name: Publish to Chromatic (non-mainline) if: github.ref != 'refs/heads/main' && github.repository_owner == 'coder' - uses: chromaui/action@c50adf8eaa8c2878af3263499a73077854de39d4 # v12.2.0 + uses: chromaui/action@b5848056bb67ce5f1cccca8e62a37cbd9dd42871 # v13.0.1 env: NODE_OPTIONS: "--max_old_space_size=4096" STORYBOOK: true @@ -934,7 +934,7 @@ jobs: # infinitely "in progress" in mainline unless we re-review each build. - name: Publish to Chromatic (mainline) if: github.ref == 'refs/heads/main' && github.repository_owner == 'coder' - uses: chromaui/action@c50adf8eaa8c2878af3263499a73077854de39d4 # v12.2.0 + uses: chromaui/action@b5848056bb67ce5f1cccca8e62a37cbd9dd42871 # v13.0.1 env: NODE_OPTIONS: "--max_old_space_size=4096" STORYBOOK: true diff --git a/.github/workflows/docker-base.yaml b/.github/workflows/docker-base.yaml index 01e24c62d9bf7..f74e71ba2909b 100644 --- a/.github/workflows/docker-base.yaml +++ b/.github/workflows/docker-base.yaml @@ -60,7 +60,7 @@ jobs: # This uses OIDC authentication, so no auth variables are required. - name: Build base Docker image via depot.dev - uses: depot/build-push-action@636daae76684e38c301daa0c5eca1c095b24e780 # v1.14.0 + uses: depot/build-push-action@2583627a84956d07561420dcc1d0eb1f2af3fac0 # v1.15.0 with: project: wl5hnrrkns context: base-build-context diff --git a/.github/workflows/docs-ci.yaml b/.github/workflows/docs-ci.yaml index 3994f8e69d24c..30ac97706d9f8 100644 --- a/.github/workflows/docs-ci.yaml +++ b/.github/workflows/docs-ci.yaml @@ -28,7 +28,7 @@ jobs: - name: Setup Node uses: ./.github/actions/setup-node - - uses: tj-actions/changed-files@d52d20fa3f981cb852b861fd8f55308b5fe29637 # v45.0.7 + - uses: tj-actions/changed-files@666c9d29007687c52e3c7aa2aac6c0ffcadeadc3 # v45.0.7 id: changed-files with: files: | diff --git a/.github/workflows/dogfood.yaml b/.github/workflows/dogfood.yaml index df73479994516..b5b2447bd44bf 100644 --- a/.github/workflows/dogfood.yaml +++ b/.github/workflows/dogfood.yaml @@ -35,7 +35,7 @@ jobs: uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Setup Nix - uses: nixbuild/nix-quick-install-action@5bb6a3b3abe66fd09bbf250dce8ada94f856a703 # v30 + uses: nixbuild/nix-quick-install-action@889f3180bb5f064ee9e3201428d04ae9e41d54ad # v31 - uses: nix-community/cache-nix-action@135667ec418502fa5a3598af6fb9eb733888ce6a # v6.1.3 with: @@ -72,7 +72,7 @@ jobs: uses: depot/setup-action@b0b1ea4f69e92ebf5dea3f8713a1b0c37b2126a5 # v1.6.0 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@b5ca514318bd6ebac0fb2aedd5d36ec1b5c232a2 # v3.10.0 + uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1 - name: Login to DockerHub if: github.ref == 'refs/heads/main' @@ -82,7 +82,7 @@ jobs: password: ${{ secrets.DOCKERHUB_PASSWORD }} - name: Build and push Non-Nix image - uses: depot/build-push-action@636daae76684e38c301daa0c5eca1c095b24e780 # v1.14.0 + uses: depot/build-push-action@2583627a84956d07561420dcc1d0eb1f2af3fac0 # v1.15.0 with: project: b4q6ltmpzh token: ${{ secrets.DEPOT_TOKEN }} diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 032961dd2e5c5..69043dc833362 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -364,7 +364,7 @@ jobs: # This uses OIDC authentication, so no auth variables are required. - name: Build base Docker image via depot.dev if: steps.image-base-tag.outputs.tag != '' - uses: depot/build-push-action@636daae76684e38c301daa0c5eca1c095b24e780 # v1.14.0 + uses: depot/build-push-action@2583627a84956d07561420dcc1d0eb1f2af3fac0 # v1.15.0 with: project: wl5hnrrkns context: base-build-context From ba08d38563ef22870254967c7274eedcaa9b55f5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Jun 2025 13:54:51 +0000 Subject: [PATCH 013/690] chore: bump github.com/coder/terraform-provider-coder/v2 from 2.6.0 to 2.7.0 (#18499) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/coder/terraform-provider-coder/v2&package-manager=go_modules&previous-version=2.6.0&new-version=2.7.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index ef52718460cdd..3bf63f08fafd0 100644 --- a/go.mod +++ b/go.mod @@ -101,7 +101,7 @@ require ( github.com/coder/quartz v0.2.1 github.com/coder/retry v1.5.1 github.com/coder/serpent v0.10.0 - github.com/coder/terraform-provider-coder/v2 v2.6.0 + github.com/coder/terraform-provider-coder/v2 v2.7.0 github.com/coder/websocket v1.8.13 github.com/coder/wgtunnel v0.1.13-0.20240522110300-ade90dfb2da0 github.com/coreos/go-oidc/v3 v3.14.1 diff --git a/go.sum b/go.sum index ee7587bfdd7b1..7433167763cc4 100644 --- a/go.sum +++ b/go.sum @@ -928,8 +928,8 @@ github.com/coder/tailscale v1.1.1-0.20250611020837-f14d20d23d8c h1:d/qBIi3Ez7Kko github.com/coder/tailscale v1.1.1-0.20250611020837-f14d20d23d8c/go.mod h1:l7ml5uu7lFh5hY28lGYM4b/oFSmuPHYX6uk4RAu23Lc= github.com/coder/terraform-config-inspect v0.0.0-20250107175719-6d06d90c630e h1:JNLPDi2P73laR1oAclY6jWzAbucf70ASAvf5mh2cME0= github.com/coder/terraform-config-inspect v0.0.0-20250107175719-6d06d90c630e/go.mod h1:Gz/z9Hbn+4KSp8A2FBtNszfLSdT2Tn/uAKGuVqqWmDI= -github.com/coder/terraform-provider-coder/v2 v2.6.0 h1:ybSVxkblpFdanNX7hibex41yvwjswUlA3RPh4BAHjBI= -github.com/coder/terraform-provider-coder/v2 v2.6.0/go.mod h1:WrdLSbihuzH1RZhwrU+qmkqEhUbdZT/sjHHdarm5b5g= +github.com/coder/terraform-provider-coder/v2 v2.7.0 h1:0WV89TuboHnoy4J8MwNnCtNjF3Onx2Ny959FC2ClGQg= +github.com/coder/terraform-provider-coder/v2 v2.7.0/go.mod h1:WrdLSbihuzH1RZhwrU+qmkqEhUbdZT/sjHHdarm5b5g= github.com/coder/trivy v0.0.0-20250527170238-9416a59d7019 h1:MHkv/W7l9eRAN9gOG0qZ1TLRGWIIfNi92273vPAQ8Fs= github.com/coder/trivy v0.0.0-20250527170238-9416a59d7019/go.mod h1:eqk+w9RLBmbd/cB5XfPZFuVn77cf/A6fB7qmEVeSmXk= github.com/coder/websocket v1.8.13 h1:f3QZdXy7uGVz+4uCJy2nTZyM0yTBj8yANEHhqlXZ9FE= From 659b787b9fee2de9a5b1286684eb8c0da558b556 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 Jun 2025 11:31:53 -0500 Subject: [PATCH 014/690] chore: set wsbuilder to use preview parameters (#18474) Use richer `previewtypes.Parameter` for `wsbuilder`. This is a pre-requirement to adding dynamic parameter validation. The richer type contains more information than the `db` parameter, so the conversion is lossless. --- coderd/database/db2sdk/db2sdk.go | 53 ++++++++++++++++++++++++++++++ coderd/dynamicparameters/static.go | 8 +++-- coderd/wsbuilder/wsbuilder.go | 12 ++++--- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go index 4a7871f21d15d..e926844d2b156 100644 --- a/coderd/database/db2sdk/db2sdk.go +++ b/coderd/database/db2sdk/db2sdk.go @@ -21,6 +21,7 @@ import ( "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/render" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionersdk/proto" @@ -96,6 +97,49 @@ func TemplateVersionParameters(params []database.TemplateVersionParameter) ([]co return out, nil } +func TemplateVersionParameterFromPreview(param previewtypes.Parameter) (codersdk.TemplateVersionParameter, error) { + descriptionPlaintext, err := render.PlaintextFromMarkdown(param.Description) + if err != nil { + return codersdk.TemplateVersionParameter{}, err + } + + sdkParam := codersdk.TemplateVersionParameter{ + Name: param.Name, + DisplayName: param.DisplayName, + Description: param.Description, + DescriptionPlaintext: descriptionPlaintext, + Type: string(param.Type), + FormType: string(param.FormType), + Mutable: param.Mutable, + DefaultValue: param.DefaultValue.AsString(), + Icon: param.Icon, + Required: param.Required, + Ephemeral: param.Ephemeral, + Options: List(param.Options, TemplateVersionParameterOptionFromPreview), + // Validation set after + } + if len(param.Validations) > 0 { + validation := param.Validations[0] + sdkParam.ValidationError = validation.Error + if validation.Monotonic != nil { + sdkParam.ValidationMonotonic = codersdk.ValidationMonotonicOrder(*validation.Monotonic) + } + if validation.Regex != nil { + sdkParam.ValidationRegex = *validation.Regex + } + if validation.Min != nil { + //nolint:gosec // No other choice + sdkParam.ValidationMin = ptr.Ref(int32(*validation.Min)) + } + if validation.Max != nil { + //nolint:gosec // No other choice + sdkParam.ValidationMax = ptr.Ref(int32(*validation.Max)) + } + } + + return sdkParam, nil +} + func TemplateVersionParameter(param database.TemplateVersionParameter) (codersdk.TemplateVersionParameter, error) { options, err := templateVersionParameterOptions(param.Options) if err != nil { @@ -299,6 +343,15 @@ func templateVersionParameterOptions(rawOptions json.RawMessage) ([]codersdk.Tem return options, nil } +func TemplateVersionParameterOptionFromPreview(option *previewtypes.ParameterOption) codersdk.TemplateVersionParameterOption { + return codersdk.TemplateVersionParameterOption{ + Name: option.Name, + Description: option.Description, + Value: option.Value.AsString(), + Icon: option.Icon, + } +} + func OAuth2ProviderApp(accessURL *url.URL, dbApp database.OAuth2ProviderApp) codersdk.OAuth2ProviderApp { return codersdk.OAuth2ProviderApp{ ID: dbApp.ID, diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index 14988a2d162c0..fec5de2581aef 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -28,6 +28,12 @@ func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRe } params := db2sdk.List(dbTemplateVersionParameters, TemplateVersionParameter) + + for i, param := range params { + // Update the diagnostics to validate the 'default' value. + // We do not have a user supplied value yet, so we use the default. + params[i].Diagnostics = append(params[i].Diagnostics, previewtypes.Diagnostics(param.Valid(param.Value))...) + } return &staticRender{ staticParams: params, }, nil @@ -137,7 +143,5 @@ func TemplateVersionParameter(it database.TemplateVersionParameter) previewtypes // unfortunate we have to do this, but it will return the default form_type // for a given set of conditions. _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) - - param.Diagnostics = append(param.Diagnostics, previewtypes.Diagnostics(param.Valid(param.Value))...) return param } diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index a996d1594a50d..577ec5eca4577 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -14,11 +14,13 @@ import ( "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/coder/coder/v2/apiversion" + "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/provisioner/terraform/tfparse" "github.com/coder/coder/v2/provisionersdk" sdkproto "github.com/coder/coder/v2/provisionersdk/proto" + previewtypes "github.com/coder/preview/types" "github.com/google/uuid" "github.com/sqlc-dev/pqtype" @@ -71,7 +73,7 @@ type Builder struct { templateVersion *database.TemplateVersion templateVersionJob *database.ProvisionerJob terraformValues *database.TemplateVersionTerraformValue - templateVersionParameters *[]database.TemplateVersionParameter + templateVersionParameters *[]previewtypes.Parameter templateVersionVariables *[]database.TemplateVersionVariable templateVersionWorkspaceTags *[]database.TemplateVersionWorkspaceTag lastBuild *database.WorkspaceBuild @@ -716,7 +718,7 @@ func (b *Builder) getParameters() (names, values []string, err error) { } for _, templateVersionParameter := range templateVersionParameters { - tvp, err := db2sdk.TemplateVersionParameter(templateVersionParameter) + tvp, err := db2sdk.TemplateVersionParameterFromPreview(templateVersionParameter) if err != nil { return nil, nil, BuildError{http.StatusInternalServerError, "failed to convert template version parameter", err} } @@ -780,7 +782,7 @@ func (b *Builder) getLastBuildParameters() ([]database.WorkspaceBuildParameter, return values, nil } -func (b *Builder) getTemplateVersionParameters() ([]database.TemplateVersionParameter, error) { +func (b *Builder) getTemplateVersionParameters() ([]previewtypes.Parameter, error) { if b.templateVersionParameters != nil { return *b.templateVersionParameters, nil } @@ -792,8 +794,8 @@ func (b *Builder) getTemplateVersionParameters() ([]database.TemplateVersionPara if err != nil && !xerrors.Is(err, sql.ErrNoRows) { return nil, xerrors.Errorf("get template version %s parameters: %w", tvID, err) } - b.templateVersionParameters = &tvp - return tvp, nil + b.templateVersionParameters = ptr.Ref(db2sdk.List(tvp, dynamicparameters.TemplateVersionParameter)) + return *b.templateVersionParameters, nil } func (b *Builder) getTemplateVersionVariables() ([]database.TemplateVersionVariable, error) { From c1b35bf2f6fa71af87dc5222ade2c62fc20bb708 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 Jun 2025 11:58:52 -0500 Subject: [PATCH 015/690] chore: use database in current context for file cache (#18490) Using the db.Store when in a TX causes a deadlock for dbmem. In production, this can cause a deadlock if at the current conn pool limit. --- coderd/coderd.go | 2 +- coderd/dynamicparameters/render.go | 4 +- coderd/files/cache.go | 67 +++++++++++++----------------- coderd/files/cache_test.go | 46 +++++++++++--------- coderd/files/closer.go | 6 ++- 5 files changed, 62 insertions(+), 63 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 929c9f44a7a8b..4507cd1dd7605 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -574,7 +574,7 @@ func New(options *Options) *API { TemplateScheduleStore: options.TemplateScheduleStore, UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore, AccessControlStore: options.AccessControlStore, - FileCache: files.NewFromStore(options.Database, options.PrometheusRegistry, options.Authorizer), + FileCache: files.New(options.PrometheusRegistry, options.Authorizer), Experiments: experiments, WebpushDispatcher: options.WebPushDispatcher, healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{}, diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 8e7df929505f1..733b2f2ab5f5d 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -169,14 +169,14 @@ func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache * var templateFS fs.FS var err error - templateFS, err = cache.Acquire(fileCtx, r.job.FileID) + templateFS, err = cache.Acquire(fileCtx, db, r.job.FileID) if err != nil { return nil, xerrors.Errorf("acquire template file: %w", err) } var moduleFilesFS *files.CloseFS if r.terraformValues.CachedModuleFiles.Valid { - moduleFilesFS, err = cache.Acquire(fileCtx, r.terraformValues.CachedModuleFiles.UUID) + moduleFilesFS, err = cache.Acquire(fileCtx, db, r.terraformValues.CachedModuleFiles.UUID) if err != nil { return nil, xerrors.Errorf("acquire module files: %w", err) } diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 170abb10b1ff7..32e03e0b8f209 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -20,38 +20,15 @@ import ( ) type FileAcquirer interface { - Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) + Acquire(ctx context.Context, db database.Store, fileID uuid.UUID) (*CloseFS, error) } -// NewFromStore returns a file cache that will fetch files from the provided -// database. -func NewFromStore(store database.Store, registerer prometheus.Registerer, authz rbac.Authorizer) *Cache { - fetch := func(ctx context.Context, fileID uuid.UUID) (CacheEntryValue, error) { - // Make sure the read does not fail due to authorization issues. - // Authz is checked on the Acquire call, so this is safe. - //nolint:gocritic - file, err := store.GetFileByID(dbauthz.AsFileReader(ctx), fileID) - if err != nil { - return CacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err) - } - - content := bytes.NewBuffer(file.Data) - return CacheEntryValue{ - Object: file.RBACObject(), - FS: archivefs.FromTarReader(content), - Size: int64(len(file.Data)), - }, nil - } - - return New(fetch, registerer, authz) -} - -func New(fetch fetcher, registerer prometheus.Registerer, authz rbac.Authorizer) *Cache { +// New returns a file cache that will fetch files from a database +func New(registerer prometheus.Registerer, authz rbac.Authorizer) *Cache { return (&Cache{ - lock: sync.Mutex{}, - data: make(map[uuid.UUID]*cacheEntry), - fetcher: fetch, - authz: authz, + lock: sync.Mutex{}, + data: make(map[uuid.UUID]*cacheEntry), + authz: authz, }).registerMetrics(registerer) } @@ -110,9 +87,8 @@ func (c *Cache) registerMetrics(registerer prometheus.Registerer) *Cache { // loaded into memory exactly once. We hold those files until there are no // longer any open connections, and then we remove the value from the map. type Cache struct { - lock sync.Mutex - data map[uuid.UUID]*cacheEntry - fetcher + lock sync.Mutex + data map[uuid.UUID]*cacheEntry authz rbac.Authorizer // metrics @@ -142,8 +118,6 @@ type cacheEntry struct { value *lazy.ValueWithError[CacheEntryValue] } -type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error) - var _ fs.FS = (*CloseFS)(nil) // CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() @@ -163,12 +137,12 @@ func (f *CloseFS) Close() { f.close() } // // Safety: Every call to Acquire that does not return an error must have a // matching call to Release. -func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { +func (c *Cache) Acquire(ctx context.Context, db database.Store, fileID uuid.UUID) (*CloseFS, error) { // It's important that this `Load` call occurs outside `prepare`, after the // mutex has been released, or we would continue to hold the lock until the // entire file has been fetched, which may be slow, and would prevent other // files from being fetched in parallel. - it, err := c.prepare(ctx, fileID).Load() + it, err := c.prepare(ctx, db, fileID).Load() if err != nil { c.release(fileID) return nil, err @@ -195,14 +169,14 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) }, nil } -func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithError[CacheEntryValue] { +func (c *Cache) prepare(ctx context.Context, db database.Store, fileID uuid.UUID) *lazy.ValueWithError[CacheEntryValue] { c.lock.Lock() defer c.lock.Unlock() entry, ok := c.data[fileID] if !ok { value := lazy.NewWithError(func() (CacheEntryValue, error) { - val, err := c.fetcher(ctx, fileID) + val, err := fetch(ctx, db, fileID) // Always add to the cache size the bytes of the file loaded. if err == nil { @@ -269,3 +243,20 @@ func (c *Cache) Count() int { return len(c.data) } + +func fetch(ctx context.Context, store database.Store, fileID uuid.UUID) (CacheEntryValue, error) { + // Make sure the read does not fail due to authorization issues. + // Authz is checked on the Acquire call, so this is safe. + //nolint:gocritic + file, err := store.GetFileByID(dbauthz.AsFileReader(ctx), fileID) + if err != nil { + return CacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err) + } + + content := bytes.NewBuffer(file.Data) + return CacheEntryValue{ + Object: file.RBACObject(), + FS: archivefs.FromTarReader(content), + Size: int64(len(file.Data)), + }, nil +} diff --git a/coderd/files/cache_test.go b/coderd/files/cache_test.go index 5efb4ba19be28..a5a5dfae268ca 100644 --- a/coderd/files/cache_test.go +++ b/coderd/files/cache_test.go @@ -8,8 +8,8 @@ import ( "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" - "github.com/spf13/afero" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "golang.org/x/sync/errgroup" "cdr.dev/slog/sloggers/slogtest" @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/coderd/rbac" @@ -58,7 +59,7 @@ func TestCacheRBAC(t *testing.T) { require.Equal(t, 0, cache.Count()) rec.Reset() - _, err := cache.Acquire(nobody, file.ID) + _, err := cache.Acquire(nobody, db, file.ID) require.Error(t, err) require.True(t, rbac.IsUnauthorizedError(err)) @@ -75,18 +76,18 @@ func TestCacheRBAC(t *testing.T) { require.Equal(t, 0, cache.Count()) // Read the file with a file reader to put it into the cache. - a, err := cache.Acquire(cacheReader, file.ID) + a, err := cache.Acquire(cacheReader, db, file.ID) require.NoError(t, err) require.Equal(t, 1, cache.Count()) // "nobody" should not be able to read the file. - _, err = cache.Acquire(nobody, file.ID) + _, err = cache.Acquire(nobody, db, file.ID) require.Error(t, err) require.True(t, rbac.IsUnauthorizedError(err)) require.Equal(t, 1, cache.Count()) // UserReader can - b, err := cache.Acquire(userReader, file.ID) + b, err := cache.Acquire(userReader, db, file.ID) require.NoError(t, err) require.Equal(t, 1, cache.Count()) @@ -110,16 +111,21 @@ func TestConcurrency(t *testing.T) { ctx := dbauthz.AsFileReader(t.Context()) const fileSize = 10 - emptyFS := afero.NewIOFS(afero.NewReadOnlyFs(afero.NewMemMapFs())) var fetches atomic.Int64 reg := prometheus.NewRegistry() - c := files.New(func(_ context.Context, _ uuid.UUID) (files.CacheEntryValue, error) { + + dbM := dbmock.NewMockStore(gomock.NewController(t)) + dbM.EXPECT().GetFileByID(gomock.Any(), gomock.Any()).DoAndReturn(func(mTx context.Context, fileID uuid.UUID) (database.File, error) { fetches.Add(1) - // Wait long enough before returning to make sure that all of the goroutines + // Wait long enough before returning to make sure that all the goroutines // will be waiting in line, ensuring that no one duplicated a fetch. time.Sleep(testutil.IntervalMedium) - return files.CacheEntryValue{FS: emptyFS, Size: fileSize}, nil - }, reg, &coderdtest.FakeAuthorizer{}) + return database.File{ + Data: make([]byte, fileSize), + }, nil + }).AnyTimes() + + c := files.New(reg, &coderdtest.FakeAuthorizer{}) batches := 1000 groups := make([]*errgroup.Group, 0, batches) @@ -137,7 +143,7 @@ func TestConcurrency(t *testing.T) { g.Go(func() error { // We don't bother to Release these references because the Cache will be // released at the end of the test anyway. - _, err := c.Acquire(ctx, id) + _, err := c.Acquire(ctx, dbM, id) return err }) } @@ -164,14 +170,15 @@ func TestRelease(t *testing.T) { ctx := dbauthz.AsFileReader(t.Context()) const fileSize = 10 - emptyFS := afero.NewIOFS(afero.NewReadOnlyFs(afero.NewMemMapFs())) reg := prometheus.NewRegistry() - c := files.New(func(_ context.Context, _ uuid.UUID) (files.CacheEntryValue, error) { - return files.CacheEntryValue{ - FS: emptyFS, - Size: fileSize, + dbM := dbmock.NewMockStore(gomock.NewController(t)) + dbM.EXPECT().GetFileByID(gomock.Any(), gomock.Any()).DoAndReturn(func(mTx context.Context, fileID uuid.UUID) (database.File, error) { + return database.File{ + Data: make([]byte, fileSize), }, nil - }, reg, &coderdtest.FakeAuthorizer{}) + }).AnyTimes() + + c := files.New(reg, &coderdtest.FakeAuthorizer{}) batches := 100 ids := make([]uuid.UUID, 0, batches) @@ -184,9 +191,8 @@ func TestRelease(t *testing.T) { batchSize := 10 for openedIdx, id := range ids { for batchIdx := range batchSize { - it, err := c.Acquire(ctx, id) + it, err := c.Acquire(ctx, dbM, id) require.NoError(t, err) - require.Equal(t, emptyFS, it.FS) releases[id] = append(releases[id], it.Close) // Each time a new file is opened, the metrics should be updated as so: @@ -257,7 +263,7 @@ func cacheAuthzSetup(t *testing.T) (database.Store, *files.Cache, *coderdtest.Re // Dbauthz wrap the db db = dbauthz.New(db, rec, logger, coderdtest.AccessControlStorePointer()) - c := files.NewFromStore(db, reg, rec) + c := files.New(reg, rec) return db, c, rec } diff --git a/coderd/files/closer.go b/coderd/files/closer.go index 9bd98fdd60caf..560786c78f80e 100644 --- a/coderd/files/closer.go +++ b/coderd/files/closer.go @@ -6,6 +6,8 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/database" ) // CacheCloser is a cache wrapper used to close all acquired files. @@ -38,7 +40,7 @@ func (c *CacheCloser) Close() { c.closers = nil } -func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) { +func (c *CacheCloser) Acquire(ctx context.Context, db database.Store, fileID uuid.UUID) (*CloseFS, error) { c.mu.Lock() defer c.mu.Unlock() @@ -46,7 +48,7 @@ func (c *CacheCloser) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, return nil, xerrors.New("cache is closed, and cannot acquire new files") } - f, err := c.cache.Acquire(ctx, fileID) + f, err := c.cache.Acquire(ctx, db, fileID) if err != nil { return nil, err } From 7254c08af49c1f7e706ae5343d659f2d3b39067e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 Jun 2025 12:17:58 -0500 Subject: [PATCH 016/690] chore: remove parallel queries in the same transaction (#18489) Parallel concurrent queries cannot be run in the same tx Was getting this error: https://stackoverflow.com/questions/78472996/go-postgres-pq-unexpected-parse-response-c-with-queryrow --- coderd/dynamicparameters/render.go | 107 ++++++++++++----------------- 1 file changed, 43 insertions(+), 64 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 733b2f2ab5f5d..d9ae7f9a4a6b5 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -9,7 +9,6 @@ import ( "time" "github.com/google/uuid" - "golang.org/x/sync/errgroup" "golang.org/x/xerrors" "github.com/coder/coder/v2/apiversion" @@ -244,8 +243,6 @@ func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui return nil // already fetched } - var g errgroup.Group - // You only need to be able to read the organization member to get the owner // data. Only the terraform files can therefore leak more information than the // caller should have access to. All this info should be public assuming you can @@ -266,72 +263,54 @@ func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui return xerrors.Errorf("fetch user: %w", err) } - var ownerRoles []previewtypes.WorkspaceOwnerRBACRole - g.Go(func() error { - // nolint:gocritic // This is kind of the wrong query to use here, but it - // matches how the provisioner currently works. We should figure out - // something that needs less escalation but has the correct behavior. - row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsProvisionerd(ctx), ownerID) - if err != nil { - return err - } - roles, err := row.RoleNames() - if err != nil { - return err + // nolint:gocritic // This is kind of the wrong query to use here, but it + // matches how the provisioner currently works. We should figure out + // something that needs less escalation but has the correct behavior. + row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsProvisionerd(ctx), ownerID) + if err != nil { + return xerrors.Errorf("user roles: %w", err) + } + roles, err := row.RoleNames() + if err != nil { + return xerrors.Errorf("expand roles: %w", err) + } + ownerRoles := make([]previewtypes.WorkspaceOwnerRBACRole, 0, len(roles)) + for _, it := range roles { + if it.OrganizationID != uuid.Nil && it.OrganizationID != r.data.templateVersion.OrganizationID { + continue } - ownerRoles = make([]previewtypes.WorkspaceOwnerRBACRole, 0, len(roles)) - for _, it := range roles { - if it.OrganizationID != uuid.Nil && it.OrganizationID != r.data.templateVersion.OrganizationID { - continue - } - var orgID string - if it.OrganizationID != uuid.Nil { - orgID = it.OrganizationID.String() - } - ownerRoles = append(ownerRoles, previewtypes.WorkspaceOwnerRBACRole{ - Name: it.Name, - OrgID: orgID, - }) + var orgID string + if it.OrganizationID != uuid.Nil { + orgID = it.OrganizationID.String() } - return nil - }) + ownerRoles = append(ownerRoles, previewtypes.WorkspaceOwnerRBACRole{ + Name: it.Name, + OrgID: orgID, + }) + } - var publicKey string - g.Go(func() error { - // The correct public key has to be sent. This will not be leaked - // unless the template leaks it. - // nolint:gocritic - key, err := r.db.GetGitSSHKey(dbauthz.AsProvisionerd(ctx), ownerID) - if err != nil { - return err - } - publicKey = key.PublicKey - return nil - }) + // The correct public key has to be sent. This will not be leaked + // unless the template leaks it. + // nolint:gocritic + key, err := r.db.GetGitSSHKey(dbauthz.AsProvisionerd(ctx), ownerID) + if err != nil { + return xerrors.Errorf("ssh key: %w", err) + } - var groupNames []string - g.Go(func() error { - // The groups need to be sent to preview. These groups are not exposed to the - // user, unless the template does it through the parameters. Regardless, we need - // the correct groups, and a user might not have read access. - // nolint:gocritic - groups, err := r.db.GetGroups(dbauthz.AsProvisionerd(ctx), database.GetGroupsParams{ - OrganizationID: r.data.templateVersion.OrganizationID, - HasMemberID: ownerID, - }) - if err != nil { - return err - } - groupNames = make([]string, 0, len(groups)) - for _, it := range groups { - groupNames = append(groupNames, it.Group.Name) - } - return nil + // The groups need to be sent to preview. These groups are not exposed to the + // user, unless the template does it through the parameters. Regardless, we need + // the correct groups, and a user might not have read access. + // nolint:gocritic + groups, err := r.db.GetGroups(dbauthz.AsProvisionerd(ctx), database.GetGroupsParams{ + OrganizationID: r.data.templateVersion.OrganizationID, + HasMemberID: ownerID, }) - - err = g.Wait() if err != nil { - return err + return xerrors.Errorf("groups: %w", err) + } + groupNames := make([]string, 0, len(groups)) + for _, it := range groups { + groupNames = append(groupNames, it.Group.Name) } r.currentOwner = &previewtypes.WorkspaceOwner{ @@ -341,7 +320,7 @@ func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui Email: mem.Email, LoginType: string(user.LoginType), RBACRoles: ownerRoles, - SSHPublicKey: publicKey, + SSHPublicKey: key.PublicKey, Groups: groupNames, } return nil From 82af2e019d3fb0771c20b4b86191a34ce972546d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 Jun 2025 12:35:15 -0500 Subject: [PATCH 017/690] feat: implement dynamic parameter validation (#18482) # What does this do? This does parameter validation for dynamic parameters in `wsbuilder`. All input parameters are validated in `coder/coder` before being sent to terraform. The heart of this PR is [`ResolveParameters`](https://github.com/coder/coder/blob/b65001e89c0577199a8e470c138c51e91cf2350c/coderd/dynamicparameters/resolver.go#L30-L30). # What else changes? `wsbuilder` now needs to load the terraform files into memory to succeed. This does add a larger memory requirement to workspace builds. # Future work - Sort autostart handling workspaces by template version id. So workspaces with the same template version only load the terraform files once from the db, and store them in the cache. --- cli/server.go | 2 +- coderd/autobuild/lifecycle_executor.go | 7 +- coderd/coderdtest/coderdtest.go | 2 + coderd/dynamicparameters/render.go | 4 +- coderd/dynamicparameters/render_test.go | 35 ++ coderd/dynamicparameters/resolver.go | 189 +++++++++++ coderd/parameters_test.go | 40 ++- coderd/testdata/parameters/modules/main.tf | 44 ++- coderd/util/slice/slice.go | 10 + coderd/workspacebuilds.go | 1 + coderd/workspaces.go | 1 + coderd/wsbuilder/wsbuilder.go | 232 +++++++++---- coderd/wsbuilder/wsbuilder_test.go | 99 +++--- enterprise/coderd/coderd.go | 2 +- enterprise/coderd/dynamicparameters_test.go | 311 ++++++++++++++++++ enterprise/coderd/prebuilds/claim_test.go | 4 +- .../coderd/prebuilds/metricscollector_test.go | 8 +- enterprise/coderd/prebuilds/reconcile.go | 5 + enterprise/coderd/prebuilds/reconcile_test.go | 43 ++- .../testdata/parameters/dynamic/main.tf | 16 +- .../testdata/parameters/ephemeral/main.tf | 25 ++ .../testdata/parameters/numbers/main.tf | 20 ++ .../coderd/testdata/parameters/regex/main.tf | 18 + 23 files changed, 960 insertions(+), 158 deletions(-) create mode 100644 coderd/dynamicparameters/render_test.go create mode 100644 coderd/dynamicparameters/resolver.go create mode 100644 enterprise/coderd/testdata/parameters/ephemeral/main.tf create mode 100644 enterprise/coderd/testdata/parameters/numbers/main.tf create mode 100644 enterprise/coderd/testdata/parameters/regex/main.tf diff --git a/cli/server.go b/cli/server.go index 7d587f5e1ae14..9af9965210d72 100644 --- a/cli/server.go +++ b/cli/server.go @@ -1125,7 +1125,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. autobuildTicker := time.NewTicker(vals.AutobuildPollInterval.Value()) defer autobuildTicker.Stop() autobuildExecutor := autobuild.NewExecutor( - ctx, options.Database, options.Pubsub, options.PrometheusRegistry, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, coderAPI.AccessControlStore, logger, autobuildTicker.C, options.NotificationsEnqueuer, coderAPI.Experiments) + ctx, options.Database, options.Pubsub, coderAPI.FileCache, options.PrometheusRegistry, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, coderAPI.AccessControlStore, logger, autobuildTicker.C, options.NotificationsEnqueuer, coderAPI.Experiments) autobuildExecutor.Run() jobReaperTicker := time.NewTicker(vals.JobReaperDetectorInterval.Value()) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index b0cba60111335..f71d267028270 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -17,6 +17,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" @@ -35,6 +36,7 @@ type Executor struct { ctx context.Context db database.Store ps pubsub.Pubsub + fileCache *files.Cache templateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore] accessControlStore *atomic.Pointer[dbauthz.AccessControlStore] auditor *atomic.Pointer[audit.Auditor] @@ -61,13 +63,14 @@ type Stats struct { } // New returns a new wsactions executor. -func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, reg prometheus.Registerer, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], acs *atomic.Pointer[dbauthz.AccessControlStore], log slog.Logger, tick <-chan time.Time, enqueuer notifications.Enqueuer, exp codersdk.Experiments) *Executor { +func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, fc *files.Cache, reg prometheus.Registerer, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], acs *atomic.Pointer[dbauthz.AccessControlStore], log slog.Logger, tick <-chan time.Time, enqueuer notifications.Enqueuer, exp codersdk.Experiments) *Executor { factory := promauto.With(reg) le := &Executor{ //nolint:gocritic // Autostart has a limited set of permissions. ctx: dbauthz.AsAutostart(ctx), db: db, ps: ps, + fileCache: fc, templateScheduleStore: tss, tick: tick, log: log.Named("autobuild"), @@ -276,7 +279,7 @@ func (e *Executor) runOnce(t time.Time) Stats { } } - nextBuild, job, _, err = builder.Build(e.ctx, tx, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) + nextBuild, job, _, err = builder.Build(e.ctx, tx, e.fileCache, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) if err != nil { return xerrors.Errorf("build workspace with transition %q: %w", nextTransition, err) } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index b509db10e3eba..55e62561af60a 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -52,6 +52,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/quartz" "github.com/coder/coder/v2/coderd" @@ -359,6 +360,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can ctx, options.Database, options.Pubsub, + files.New(prometheus.NewRegistry(), options.Authorizer), prometheus.NewRegistry(), &templateScheduleStore, &auditor, diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index d9ae7f9a4a6b5..ed9b2734b259c 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -49,7 +49,7 @@ type loader struct { // Prepare is the entrypoint for this package. It loads the necessary objects & // files from the database and returns a Renderer that can be used to render the // template version's parameters. -func Prepare(ctx context.Context, db database.Store, cache *files.Cache, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { +func Prepare(ctx context.Context, db database.Store, cache files.FileAcquirer, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { l := &loader{ templateVersionID: versionID, } @@ -137,7 +137,7 @@ func (r *loader) loadData(ctx context.Context, db database.Store) error { // Static parameter rendering is required to support older template versions that // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. -func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { +func (r *loader) Renderer(ctx context.Context, db database.Store, cache files.FileAcquirer) (Renderer, error) { err := r.loadData(ctx, db) if err != nil { return nil, xerrors.Errorf("load data: %w", err) diff --git a/coderd/dynamicparameters/render_test.go b/coderd/dynamicparameters/render_test.go new file mode 100644 index 0000000000000..c71230c14e19b --- /dev/null +++ b/coderd/dynamicparameters/render_test.go @@ -0,0 +1,35 @@ +package dynamicparameters_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/dynamicparameters" +) + +func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) { + t.Parallel() + + for v, dyn := range map[string]bool{ + "": false, + "na": false, + "0.0": false, + "0.10": false, + "1.4": false, + "1.5": false, + "1.6": true, + "1.7": true, + "1.8": true, + "2.0": true, + "2.17": true, + "4.0": true, + } { + t.Run(v, func(t *testing.T) { + t.Parallel() + + does := dynamicparameters.ProvisionerVersionSupportsDynamicParameters(v) + require.Equal(t, dyn, does) + }) + } +} diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go new file mode 100644 index 0000000000000..bda422ad4b840 --- /dev/null +++ b/coderd/dynamicparameters/resolver.go @@ -0,0 +1,189 @@ +package dynamicparameters + +import ( + "context" + "fmt" + + "github.com/google/uuid" + "github.com/hashicorp/hcl/v2" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/codersdk" +) + +type parameterValueSource int + +const ( + sourceDefault parameterValueSource = iota + sourcePrevious + sourceBuild + sourcePreset +) + +type parameterValue struct { + Value string + Source parameterValueSource +} + +//nolint:revive // firstbuild is a control flag to turn on immutable validation +func ResolveParameters( + ctx context.Context, + ownerID uuid.UUID, + renderer Renderer, + firstBuild bool, + previousValues []database.WorkspaceBuildParameter, + buildValues []codersdk.WorkspaceBuildParameter, + presetValues []database.TemplateVersionPresetParameter, +) (map[string]string, hcl.Diagnostics) { + previousValuesMap := slice.ToMapFunc(previousValues, func(p database.WorkspaceBuildParameter) (string, string) { + return p.Name, p.Value + }) + + // Start with previous + values := parameterValueMap(slice.ToMapFunc(previousValues, func(p database.WorkspaceBuildParameter) (string, parameterValue) { + return p.Name, parameterValue{Source: sourcePrevious, Value: p.Value} + })) + + // Add build values (overwrite previous values if they exist) + for _, buildValue := range buildValues { + values[buildValue.Name] = parameterValue{Source: sourceBuild, Value: buildValue.Value} + } + + // Add preset values (overwrite previous and build values if they exist) + for _, preset := range presetValues { + values[preset.Name] = parameterValue{Source: sourcePreset, Value: preset.Value} + } + + // originalValues is going to be used to detect if a user tried to change + // an immutable parameter after the first build. + originalValues := make(map[string]parameterValue, len(values)) + for name, value := range values { + // Store the original values for later use. + originalValues[name] = value + } + + // Render the parameters using the values that were supplied to the previous build. + // + // This is how the form should look to the user on their workspace settings page. + // This is the original form truth that our validations should initially be based on. + output, diags := renderer.Render(ctx, ownerID, values.ValuesMap()) + if diags.HasErrors() { + // Top level diagnostics should break the build. Previous values (and new) should + // always be valid. If there is a case where this is not true, then this has to + // be changed to allow the build to continue with a different set of values. + + return nil, diags + } + + // The user's input now needs to be validated against the parameters. + // Mutability & Ephemeral parameters depend on sequential workspace builds. + // + // To enforce these, the user's input values are trimmed based on the + // mutability and ephemeral parameters defined in the template version. + for _, parameter := range output.Parameters { + // Ephemeral parameters should not be taken from the previous build. + // They must always be explicitly set in every build. + // So remove their values if they are sourced from the previous build. + if parameter.Ephemeral { + v := values[parameter.Name] + if v.Source == sourcePrevious { + delete(values, parameter.Name) + } + } + + // Immutable parameters should also not be allowed to be changed from + // the previous build. Remove any values taken from the preset or + // new build params. This forces the value to be the same as it was before. + // + // We do this so the next form render uses the original immutable value. + if !firstBuild && !parameter.Mutable { + delete(values, parameter.Name) + prev, ok := previousValuesMap[parameter.Name] + if ok { + values[parameter.Name] = parameterValue{ + Value: prev, + Source: sourcePrevious, + } + } + } + } + + // This is the final set of values that will be used. Any errors at this stage + // are fatal. Additional validation for immutability has to be done manually. + output, diags = renderer.Render(ctx, ownerID, values.ValuesMap()) + if diags.HasErrors() { + return nil, diags + } + + // parameterNames is going to be used to remove any excess values that were left + // around without a parameter. + parameterNames := make(map[string]struct{}, len(output.Parameters)) + for _, parameter := range output.Parameters { + parameterNames[parameter.Name] = struct{}{} + + if !firstBuild && !parameter.Mutable { + // Immutable parameters should not be changed after the first build. + // They can match the original value though! + if parameter.Value.AsString() != originalValues[parameter.Name].Value { + var src *hcl.Range + if parameter.Source != nil { + src = ¶meter.Source.HCLBlock().TypeRange + } + + // An immutable parameter was changed, which is not allowed. + // Add the failed diagnostic to the output. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Immutable parameter changed", + Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name), + Subject: src, + }) + } + } + + // TODO: Fix the `hcl.Diagnostics(...)` type casting. It should not be needed. + if hcl.Diagnostics(parameter.Diagnostics).HasErrors() { + // All validation errors are raised here. + diags = diags.Extend(hcl.Diagnostics(parameter.Diagnostics)) + } + + // If the parameter has a value, but it was not set explicitly by the user at any + // build, then save the default value. An example where this is important is if a + // template has a default value of 'region = us-west-2', but the user never sets + // it. If the default value changes to 'region = us-east-1', we want to preserve + // the original value of 'us-west-2' for the existing workspaces. + // + // parameter.Value will be populated from the default at this point. So grab it + // from there. + if _, ok := values[parameter.Name]; !ok && parameter.Value.IsKnown() && parameter.Value.Valid() { + values[parameter.Name] = parameterValue{ + Value: parameter.Value.AsString(), + Source: sourceDefault, + } + } + } + + // Delete any values that do not belong to a parameter. This is to not save + // parameter values that have no effect. These leaky parameter values can cause + // problems in the future, as it makes it challenging to remove values from the + // database + for k := range values { + if _, ok := parameterNames[k]; !ok { + delete(values, k) + } + } + + // Return the values to be saved for the build. + return values.ValuesMap(), diags +} + +type parameterValueMap map[string]parameterValue + +func (p parameterValueMap) ValuesMap() map[string]string { + values := make(map[string]string, len(p)) + for name, paramValue := range p { + values[name] = paramValue.Value + } + return values +} diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 794ff8db3354d..a18000673b50e 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -100,10 +100,11 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.Equal(t, -1, preview.ID) require.Empty(t, preview.Diagnostics) - require.Len(t, preview.Parameters, 1) - require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) - require.True(t, preview.Parameters[0].Value.Valid) - require.Equal(t, "CL", preview.Parameters[0].Value.Value) + require.Len(t, preview.Parameters, 2) + coderdtest.AssertParameter(t, "jetbrains_ide", preview.Parameters). + Exists().Value("CL") + coderdtest.AssertParameter(t, "region", preview.Parameters). + Exists().Value("na") }) // OldProvisioners use the static parameters in the dynamic param flow @@ -241,10 +242,11 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.Equal(t, -1, preview.ID) require.Empty(t, preview.Diagnostics) - require.Len(t, preview.Parameters, 1) - require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) - require.True(t, preview.Parameters[0].Value.Valid) - require.Equal(t, "CL", preview.Parameters[0].Value.Value) + require.Len(t, preview.Parameters, 2) + coderdtest.AssertParameter(t, "jetbrains_ide", preview.Parameters). + Exists().Value("CL") + coderdtest.AssertParameter(t, "region", preview.Parameters). + Exists().Value("na") _ = stream.Close(websocket.StatusGoingAway) wrk := coderdtest.CreateWorkspace(t, setup.client, setup.template.ID, func(request *codersdk.CreateWorkspaceRequest) { @@ -253,6 +255,10 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { Name: preview.Parameters[0].Name, Value: "GO", }, + { + Name: preview.Parameters[1].Name, + Value: "eu", + }, } request.EnableDynamicParameters = true }) @@ -260,22 +266,24 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { params, err := setup.client.WorkspaceBuildParameters(ctx, wrk.LatestBuild.ID) require.NoError(t, err) - require.Len(t, params, 1) - require.Equal(t, "jetbrains_ide", params[0].Name) - require.Equal(t, "GO", params[0].Value) + require.ElementsMatch(t, []codersdk.WorkspaceBuildParameter{ + {Name: "jetbrains_ide", Value: "GO"}, {Name: "region", Value: "eu"}, + }, params) + + regionOptions := []string{"na", "af", "sa", "as"} // A helper function to assert params doTransition := func(t *testing.T, trans codersdk.WorkspaceTransition) { t.Helper() - fooVal := coderdtest.RandomUsername(t) + regionVal := regionOptions[0] + regionOptions = regionOptions[1:] // Choose the next region on the next build + bld, err := setup.client.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: setup.template.ActiveVersionID, Transition: trans, RichParameterValues: []codersdk.WorkspaceBuildParameter{ - // No validation, so this should work as is. - // Overwrite the value on each transition - {Name: "foo", Value: fooVal}, + {Name: "region", Value: regionVal}, }, EnableDynamicParameters: ptr.Ref(true), }) @@ -286,7 +294,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.NoError(t, err) require.ElementsMatch(t, latestParams, []codersdk.WorkspaceBuildParameter{ {Name: "jetbrains_ide", Value: "GO"}, - {Name: "foo", Value: fooVal}, + {Name: "region", Value: regionVal}, }) } diff --git a/coderd/testdata/parameters/modules/main.tf b/coderd/testdata/parameters/modules/main.tf index 18f14ece154f2..21bb235574d3f 100644 --- a/coderd/testdata/parameters/modules/main.tf +++ b/coderd/testdata/parameters/modules/main.tf @@ -1,5 +1,47 @@ -terraform {} +terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.5.3" + } + } +} module "jetbrains_gateway" { source = "jetbrains_gateway" } + +data "coder_parameter" "region" { + name = "region" + display_name = "Where would you like to travel to next?" + type = "string" + form_type = "dropdown" + mutable = true + default = "na" + order = 1000 + + option { + name = "North America" + value = "na" + } + + option { + name = "South America" + value = "sa" + } + + option { + name = "Europe" + value = "eu" + } + + option { + name = "Africa" + value = "af" + } + + option { + name = "Asia" + value = "as" + } +} diff --git a/coderd/util/slice/slice.go b/coderd/util/slice/slice.go index 2a510e24d2b53..bb2011c05d1b2 100644 --- a/coderd/util/slice/slice.go +++ b/coderd/util/slice/slice.go @@ -230,3 +230,13 @@ func Convert[F any, T any](a []F, f func(F) T) []T { } return tmp } + +func ToMapFunc[T any, K comparable, V any](a []T, cnv func(t T) (K, V)) map[K]V { + m := make(map[K]V, len(a)) + + for i := range a { + k, v := cnv(a[i]) + m[k] = v + } + return m +} diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 15614f84b4f70..526be7e4e4f89 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -392,6 +392,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { workspaceBuild, provisionerJob, provisionerDaemons, err = builder.Build( ctx, tx, + api.FileCache, func(action policy.Action, object rbac.Objecter) bool { // Special handling for prebuilt workspace deletion if object.RBACObject().Type == rbac.ResourceWorkspace.Type && action == policy.ActionDelete { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index b1520776464c0..58375ae4c4d45 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -724,6 +724,7 @@ func createWorkspace( workspaceBuild, provisionerJob, provisionerDaemons, err = builder.Build( ctx, db, + api.FileCache, func(action policy.Action, object rbac.Objecter) bool { return api.Authorize(r, action, object) }, diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 577ec5eca4577..aaccd337ee793 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -13,8 +13,8 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsyntax" - "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/coderd/dynamicparameters" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/provisioner/terraform/tfparse" @@ -65,8 +65,9 @@ type Builder struct { templateVersionPresetID uuid.UUID // used during build, makes function arguments less verbose - ctx context.Context - store database.Store + ctx context.Context + store database.Store + fileCache *files.CacheCloser // cache of objects, so we only fetch once template *database.Template @@ -82,7 +83,8 @@ type Builder struct { lastBuildJob *database.ProvisionerJob parameterNames *[]string parameterValues *[]string - templateVersionPresetParameterValues []database.TemplateVersionPresetParameter + templateVersionPresetParameterValues *[]database.TemplateVersionPresetParameter + parameterRender dynamicparameters.Renderer prebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage verifyNoLegacyParametersOnce bool @@ -258,6 +260,7 @@ func (e BuildError) Unwrap() error { func (b *Builder) Build( ctx context.Context, store database.Store, + fileCache *files.Cache, authFunc func(action policy.Action, object rbac.Objecter) bool, auditBaggage audit.WorkspaceBuildBaggage, ) ( @@ -269,6 +272,10 @@ func (b *Builder) Build( return nil, nil, nil, xerrors.Errorf("create audit baggage: %w", err) } + b.fileCache = files.NewCacheCloser(fileCache) + // Always close opened files during the build + defer b.fileCache.Close() + // Run the build in a transaction with RepeatableRead isolation, and retries. // RepeatableRead isolation ensures that we get a consistent view of the database while // computing the new build. This simplifies the logic so that we do not need to worry if @@ -592,10 +599,54 @@ func (b *Builder) getTemplateTerraformValues() (*database.TemplateVersionTerrafo } vals, err := b.store.GetTemplateVersionTerraformValues(b.ctx, v.ID) if err != nil { - return nil, xerrors.Errorf("get template version terraform values %s: %w", v.JobID, err) + if !xerrors.Is(err, sql.ErrNoRows) { + return nil, xerrors.Errorf("builder get template version terraform values %s: %w", v.JobID, err) + } + + // Old versions do not have terraform values, so we can ignore ErrNoRows and use an empty value. + vals = database.TemplateVersionTerraformValue{ + TemplateVersionID: v.ID, + UpdatedAt: time.Time{}, + CachedPlan: nil, + CachedModuleFiles: uuid.NullUUID{}, + ProvisionerdVersion: "", + } } b.terraformValues = &vals - return b.terraformValues, err + return b.terraformValues, nil +} + +func (b *Builder) getDynamicParameterRenderer() (dynamicparameters.Renderer, error) { + if b.parameterRender != nil { + return b.parameterRender, nil + } + + tv, err := b.getTemplateVersion() + if err != nil { + return nil, xerrors.Errorf("get template version to get parameters: %w", err) + } + + job, err := b.getTemplateVersionJob() + if err != nil { + return nil, xerrors.Errorf("get template version job to get parameters: %w", err) + } + + tfVals, err := b.getTemplateTerraformValues() + if err != nil { + return nil, xerrors.Errorf("get template version terraform values: %w", err) + } + + renderer, err := dynamicparameters.Prepare(b.ctx, b.store, b.fileCache, tv.ID, + dynamicparameters.WithTemplateVersion(*tv), + dynamicparameters.WithProvisionerJob(*job), + dynamicparameters.WithTerraformValues(*tfVals), + ) + if err != nil { + return nil, xerrors.Errorf("get template version renderer: %w", err) + } + + b.parameterRender = renderer + return renderer, nil } func (b *Builder) getLastBuild() (*database.WorkspaceBuild, error) { @@ -617,6 +668,19 @@ func (b *Builder) getLastBuild() (*database.WorkspaceBuild, error) { return b.lastBuild, nil } +// firstBuild returns true if this is the first build of the workspace, i.e. there are no prior builds. +func (b *Builder) firstBuild() (bool, error) { + _, err := b.getLastBuild() + if xerrors.Is(err, sql.ErrNoRows) { + // first build! + return true, nil + } + if err != nil { + return false, err + } + return false, nil +} + func (b *Builder) getBuildNumber() (int32, error) { bld, err := b.getLastBuild() if xerrors.Is(err, sql.ErrNoRows) { @@ -654,67 +718,91 @@ func (b *Builder) getParameters() (names, values []string, err error) { return *b.parameterNames, *b.parameterValues, nil } - templateVersionParameters, err := b.getTemplateVersionParameters() + // Always reject legacy parameters. + err = b.verifyNoLegacyParameters() if err != nil { - return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch template version parameters", err} + return nil, nil, BuildError{http.StatusBadRequest, "Unable to build workspace with unsupported parameters", err} + } + + if b.usingDynamicParameters() { + names, values, err = b.getDynamicParameters() + } else { + names, values, err = b.getClassicParameters() } + + if err != nil { + return nil, nil, xerrors.Errorf("get parameters: %w", err) + } + + b.parameterNames = &names + b.parameterValues = &values + return names, values, nil +} + +func (b *Builder) getDynamicParameters() (names, values []string, err error) { lastBuildParameters, err := b.getLastBuildParameters() if err != nil { return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch last build parameters", err} } - if b.templateVersionPresetID != uuid.Nil { - // Fetch and cache these, since we'll need them to override requested values if a preset was chosen - presetParameters, err := b.store.GetPresetParametersByPresetID(b.ctx, b.templateVersionPresetID) - if err != nil { - return nil, nil, BuildError{http.StatusInternalServerError, "failed to get preset parameters", err} - } - b.templateVersionPresetParameterValues = presetParameters - } - err = b.verifyNoLegacyParameters() + + presetParameterValues, err := b.getPresetParameterValues() if err != nil { - return nil, nil, BuildError{http.StatusBadRequest, "Unable to build workspace with unsupported parameters", err} + return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch preset parameter values", err} } - lastBuildParameterValues := db2sdk.WorkspaceBuildParameters(lastBuildParameters) - resolver := codersdk.ParameterResolver{ - Rich: lastBuildParameterValues, + render, err := b.getDynamicParameterRenderer() + if err != nil { + return nil, nil, BuildError{http.StatusInternalServerError, "failed to get dynamic parameter renderer", err} } - // Dynamic parameters skip all parameter validation. - // Deleting a workspace also should skip parameter validation. - // Pass the user's input as is. - if b.usingDynamicParameters() { - // TODO: The previous behavior was only to pass param values - // for parameters that exist. Since dynamic params can have - // conditional parameter existence, the static frame of reference - // is not sufficient. So assume the user is correct, or pull in the - // dynamic param code to find the actual parameters. - latestValues := make(map[string]string, len(b.richParameterValues)) - for _, latest := range b.richParameterValues { - latestValues[latest.Name] = latest.Value + firstBuild, err := b.firstBuild() + if err != nil { + return nil, nil, BuildError{http.StatusInternalServerError, "failed to check if first build", err} + } + + buildValues, diagnostics := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild, + lastBuildParameters, + b.richParameterValues, + presetParameterValues) + + if diagnostics.HasErrors() { + // TODO: Improve the error response. The response should include the validations for each failed + // parameter. The response should also indicate it's a validation error or a more general form failure. + // For now, any error is sufficient. + return nil, nil, BuildError{ + Status: http.StatusBadRequest, + Message: fmt.Sprintf("%d errors occurred while resolving parameters", len(diagnostics)), + Wrapped: diagnostics, } + } - // Merge the inputs with values from the previous build. - for _, last := range lastBuildParameterValues { - // TODO: Ideally we use the resolver here and look at parameter - // fields such as 'ephemeral'. This requires loading the terraform - // files. For now, just send the previous inputs as is. - if _, exists := latestValues[last.Name]; exists { - // latestValues take priority, so skip this previous value. - continue - } - names = append(names, last.Name) - values = append(values, last.Value) - } + names = make([]string, 0, len(buildValues)) + values = make([]string, 0, len(buildValues)) + for k, v := range buildValues { + names = append(names, k) + values = append(values, v) + } - for _, value := range b.richParameterValues { - names = append(names, value.Name) - values = append(values, value.Value) - } + return names, values, nil +} + +func (b *Builder) getClassicParameters() (names, values []string, err error) { + templateVersionParameters, err := b.getTemplateVersionParameters() + if err != nil { + return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch template version parameters", err} + } + lastBuildParameters, err := b.getLastBuildParameters() + if err != nil { + return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch last build parameters", err} + } + presetParameterValues, err := b.getPresetParameterValues() + if err != nil { + return nil, nil, BuildError{http.StatusInternalServerError, "failed to fetch preset parameter values", err} + } - b.parameterNames = &names - b.parameterValues = &values - return names, values, nil + lastBuildParameterValues := db2sdk.WorkspaceBuildParameters(lastBuildParameters) + resolver := codersdk.ParameterResolver{ + Rich: lastBuildParameterValues, } for _, templateVersionParameter := range templateVersionParameters { @@ -725,7 +813,7 @@ func (b *Builder) getParameters() (names, values []string, err error) { value, err := resolver.ValidateResolve( tvp, - b.findNewBuildParameterValue(templateVersionParameter.Name), + b.findNewBuildParameterValue(templateVersionParameter.Name, presetParameterValues), ) if err != nil { // At this point, we've queried all the data we need from the database, @@ -743,8 +831,8 @@ func (b *Builder) getParameters() (names, values []string, err error) { return names, values, nil } -func (b *Builder) findNewBuildParameterValue(name string) *codersdk.WorkspaceBuildParameter { - for _, v := range b.templateVersionPresetParameterValues { +func (b *Builder) findNewBuildParameterValue(name string, presets []database.TemplateVersionPresetParameter) *codersdk.WorkspaceBuildParameter { + for _, v := range presets { if v.Name == name { return &codersdk.WorkspaceBuildParameter{ Name: v.Name, @@ -949,6 +1037,24 @@ func (b *Builder) getTemplateVersionWorkspaceTags() ([]database.TemplateVersionW return *b.templateVersionWorkspaceTags, nil } +func (b *Builder) getPresetParameterValues() ([]database.TemplateVersionPresetParameter, error) { + if b.templateVersionPresetParameterValues != nil { + return *b.templateVersionPresetParameterValues, nil + } + + if b.templateVersionPresetID == uuid.Nil { + return []database.TemplateVersionPresetParameter{}, nil + } + + // Fetch and cache these, since we'll need them to override requested values if a preset was chosen + presetParameters, err := b.store.GetPresetParametersByPresetID(b.ctx, b.templateVersionPresetID) + if err != nil { + return nil, xerrors.Errorf("failed to get preset parameters: %w", err) + } + b.templateVersionPresetParameterValues = ptr.Ref(presetParameters) + return *b.templateVersionPresetParameterValues, nil +} + // authorize performs build authorization pre-checks using the provided authFunc func (b *Builder) authorize(authFunc func(action policy.Action, object rbac.Objecter) bool) error { // Doing this up front saves a lot of work if the user doesn't have permission. @@ -1117,21 +1223,5 @@ func (b *Builder) usingDynamicParameters() bool { return false } - vals, err := b.getTemplateTerraformValues() - if err != nil { - return false - } - - if !ProvisionerVersionSupportsDynamicParameters(vals.ProvisionerdVersion) { - return false - } - return true } - -func ProvisionerVersionSupportsDynamicParameters(version string) bool { - major, minor, err := apiversion.Parse(version) - // If the api version is not valid or less than 1.6, we need to use the static parameters - useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6) - return !useStaticParams -} diff --git a/coderd/wsbuilder/wsbuilder_test.go b/coderd/wsbuilder/wsbuilder_test.go index f6b4756455f14..f07e2f99f774a 100644 --- a/coderd/wsbuilder/wsbuilder_test.go +++ b/coderd/wsbuilder/wsbuilder_test.go @@ -8,6 +8,10 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/provisionersdk" "github.com/google/uuid" @@ -94,11 +98,12 @@ func TestBuilder_NoOptions(t *testing.T) { asrt.Empty(params.Value) }), ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -133,11 +138,12 @@ func TestBuilder_Initiator(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Initiator(otherUserID) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -178,11 +184,12 @@ func TestBuilder_Baggage(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Initiator(otherUserID) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"}) req.NoError(err) } @@ -216,11 +223,12 @@ func TestBuilder_Reason(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Reason(database.BuildReasonAutostart) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -259,11 +267,12 @@ func TestBuilder_ActiveVersion(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).ActiveVersion() // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -373,11 +382,12 @@ func TestWorkspaceBuildWithTags(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(buildParameters) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -455,11 +465,12 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) t.Run("UsePreviousParameterValues", func(t *testing.T) { @@ -502,11 +513,12 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -533,17 +545,17 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { mDB := expectDB(t, // Inputs withTemplate, - withInactiveVersion(richParameters), + withInactiveVersionNoParams(), withLastBuildFound, withTemplateVersionVariables(inactiveVersionID, nil), - withRichParameters(nil), withParameterSchemas(inactiveJobID, schemas), withWorkspaceTags(inactiveVersionID, nil), ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart) - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) bldErr := wsbuilder.BuildError{} req.ErrorAs(err, &bldErr) asrt.Equal(http.StatusBadRequest, bldErr.Status) @@ -575,11 +587,12 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { // Outputs // no transaction, since we failed fast while validation build parameters ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).RichParameterValues(nextBuildParameters) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) bldErr := wsbuilder.BuildError{} req.ErrorAs(err, &bldErr) asrt.Equal(http.StatusBadRequest, bldErr.Status) @@ -639,12 +652,13 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -702,12 +716,13 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -763,13 +778,14 @@ func TestWorkspaceBuildWithRichParameters(t *testing.T) { }), withBuild, ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). RichParameterValues(nextBuildParameters). VersionID(activeVersionID) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) } @@ -829,13 +845,14 @@ func TestWorkspaceBuildWithPreset(t *testing.T) { asrt.Empty(params.Value) }), ) + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionStart). ActiveVersion(). TemplateVersionPresetID(presetID) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) } @@ -899,8 +916,10 @@ func TestWorkspaceBuildDeleteOrphan(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan() + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) @@ -974,38 +993,13 @@ func TestWorkspaceBuildDeleteOrphan(t *testing.T) { ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan() + fc := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) // nolint: dogsled - _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + _, _, _, err := uut.Build(ctx, mDB, fc, nil, audit.WorkspaceBuildBaggage{}) req.NoError(err) }) } -func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) { - t.Parallel() - - for v, dyn := range map[string]bool{ - "": false, - "na": false, - "0.0": false, - "0.10": false, - "1.4": false, - "1.5": false, - "1.6": true, - "1.7": true, - "1.8": true, - "2.0": true, - "2.17": true, - "4.0": true, - } { - t.Run(v, func(t *testing.T) { - t.Parallel() - - does := wsbuilder.ProvisionerVersionSupportsDynamicParameters(v) - require.Equal(t, dyn, does) - }) - } -} - type txExpect func(mTx *dbmock.MockStore) func expectDB(t *testing.T, opts ...txExpect) *dbmock.MockStore { @@ -1052,7 +1046,7 @@ func withInTx(mTx *dbmock.MockStore) { ) } -func withActiveVersion(params []database.TemplateVersionParameter) func(mTx *dbmock.MockStore) { +func withActiveVersionNoParams() func(mTx *dbmock.MockStore) { return func(mTx *dbmock.MockStore) { mTx.EXPECT().GetTemplateVersionByID(gomock.Any(), activeVersionID). Times(1). @@ -1082,6 +1076,12 @@ func withActiveVersion(params []database.TemplateVersionParameter) func(mTx *dbm UpdatedAt: time.Now(), CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, }, nil) + } +} + +func withActiveVersion(params []database.TemplateVersionParameter) func(mTx *dbmock.MockStore) { + return func(mTx *dbmock.MockStore) { + withActiveVersionNoParams()(mTx) paramsCall := mTx.EXPECT().GetTemplateVersionParameters(gomock.Any(), activeVersionID). Times(1) if len(params) > 0 { @@ -1092,7 +1092,7 @@ func withActiveVersion(params []database.TemplateVersionParameter) func(mTx *dbm } } -func withInactiveVersion(params []database.TemplateVersionParameter) func(mTx *dbmock.MockStore) { +func withInactiveVersionNoParams() func(mTx *dbmock.MockStore) { return func(mTx *dbmock.MockStore) { mTx.EXPECT().GetTemplateVersionByID(gomock.Any(), inactiveVersionID). Times(1). @@ -1122,6 +1122,13 @@ func withInactiveVersion(params []database.TemplateVersionParameter) func(mTx *d UpdatedAt: time.Now(), CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, }, nil) + } +} + +func withInactiveVersion(params []database.TemplateVersionParameter) func(mTx *dbmock.MockStore) { + return func(mTx *dbmock.MockStore) { + withInactiveVersionNoParams()(mTx) + paramsCall := mTx.EXPECT().GetTemplateVersionParameters(gomock.Any(), inactiveVersionID). Times(1) if len(params) > 0 { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index f46848812a69e..9b168c8e3f366 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -1164,7 +1164,7 @@ func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.Reconciliatio return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer } - reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds, + reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.AGPL.FileCache, api.DeploymentValues.Prebuilds, api.Logger.Named("prebuilds"), quartz.NewReal(), api.PrometheusRegistry, api.NotificationsEnqueuer) return reconciler, prebuilds.NewEnterpriseClaimer(api.Database) } diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index 60d68fecd87d1..8bbe5da470f42 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -1,15 +1,19 @@ package coderd_test import ( + "context" _ "embed" "os" "testing" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" @@ -17,6 +21,289 @@ import ( "github.com/coder/websocket" ) +func TestDynamicParameterBuild(t *testing.T) { + t.Parallel() + + owner, _, _, first := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ + Options: &coderdtest.Options{IncludeProvisionerDaemon: true}, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + + orgID := first.OrganizationID + + templateAdmin, templateAdminData := coderdtest.CreateAnotherUser(t, owner, orgID, rbac.ScopedRoleOrgTemplateAdmin(orgID)) + + coderdtest.CreateGroup(t, owner, orgID, "developer") + coderdtest.CreateGroup(t, owner, orgID, "admin", templateAdminData) + coderdtest.CreateGroup(t, owner, orgID, "auditor") + + // Create a set of templates to test with + numberValidation, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/numbers/main.tf"))), + }) + + regexValidation, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/regex/main.tf"))), + }) + + ephemeralValidation, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/ephemeral/main.tf"))), + }) + + // complexValidation does conditional parameters, conditional options, and more. + complexValidation, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/dynamic/main.tf"))), + }) + + t.Run("NumberValidation", func(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: numberValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "number", Value: `7`}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + }) + + t.Run("TooLow", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: numberValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "number", Value: `-10`}, + }, + }) + require.ErrorContains(t, err, "Number must be between 0 and 10") + }) + + t.Run("TooHigh", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: numberValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "number", Value: `15`}, + }, + }) + require.ErrorContains(t, err, "Number must be between 0 and 10") + }) + }) + + t.Run("RegexValidation", func(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: regexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "string", Value: `Hello World!`}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + }) + + t.Run("NoValue", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: regexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{}, + }) + require.ErrorContains(t, err, "All messages must start with 'Hello'") + }) + + t.Run("Invalid", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: regexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "string", Value: `Goodbye!`}, + }, + }) + require.ErrorContains(t, err, "All messages must start with 'Hello'") + }) + }) + + t.Run("EphemeralValidation", func(t *testing.T) { + t.Parallel() + + t.Run("OK_EphemeralNoPrevious", func(t *testing.T) { + t.Parallel() + + // Ephemeral params do not take the previous values into account. + ctx := testutil.Context(t, testutil.WaitShort) + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: ephemeralValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "required", Value: `Hello World!`}, + {Name: "defaulted", Value: `Changed`}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + assertWorkspaceBuildParameters(ctx, t, templateAdmin, wrk.LatestBuild.ID, map[string]string{ + "required": "Hello World!", + "defaulted": "Changed", + }) + + bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStart, + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "required", Value: `Hello World, Again!`}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) + assertWorkspaceBuildParameters(ctx, t, templateAdmin, bld.ID, map[string]string{ + "required": "Hello World, Again!", + "defaulted": "original", // Reverts back to the original default value. + }) + }) + + t.Run("Immutable", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: numberValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "number", Value: `7`}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + assertWorkspaceBuildParameters(ctx, t, templateAdmin, wrk.LatestBuild.ID, map[string]string{ + "number": "7", + }) + + _, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStart, + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "number", Value: `8`}, + }, + }) + require.ErrorContains(t, err, `Parameter "number" is not mutable`) + }) + + t.Run("RequiredMissing", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: ephemeralValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{}, + }) + require.ErrorContains(t, err, "Required parameter not provided") + }) + }) + + t.Run("ComplexValidation", func(t *testing.T) { + t.Parallel() + + t.Run("OK", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: complexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "groups", Value: `["admin"]`}, + {Name: "colors", Value: `["red"]`}, + {Name: "thing", Value: "apple"}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + }) + + t.Run("BadGroup", func(t *testing.T) { + // Template admin is not in the "auditor" group, so this should fail. + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: complexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "groups", Value: `["auditor", "admin"]`}, + {Name: "colors", Value: `["red"]`}, + {Name: "thing", Value: "apple"}, + }, + }) + require.ErrorContains(t, err, "is not a valid option") + }) + + t.Run("BadColor", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: complexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "groups", Value: `["admin"]`}, + {Name: "colors", Value: `["purple"]`}, + }, + }) + require.ErrorContains(t, err, "is not a valid option") + require.ErrorContains(t, err, "purple") + }) + + t.Run("BadThing", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: complexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "groups", Value: `["admin"]`}, + {Name: "colors", Value: `["red"]`}, + {Name: "thing", Value: "leaf"}, + }, + }) + require.ErrorContains(t, err, "must be defined as one of options") + require.ErrorContains(t, err, "leaf") + }) + + t.Run("BadNumber", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + _, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: complexValidation.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "groups", Value: `["admin"]`}, + {Name: "colors", Value: `["green"]`}, + {Name: "thing", Value: "leaf"}, + {Name: "number", Value: "100"}, + }, + }) + require.ErrorContains(t, err, "Number must be between 0 and 10") + }) + }) +} + // TestDynamicParameterTemplate uses a template with some dynamic elements, and // tests the parameters, values, etc are all as expected. func TestDynamicParameterTemplate(t *testing.T) { @@ -127,3 +414,27 @@ func TestDynamicParameterTemplate(t *testing.T) { coderdtest.AssertParameter(t, "thing", resp.Parameters). Exists().Value("banana").Options("banana", "ocean", "sky") } + +func assertWorkspaceBuildParameters(ctx context.Context, t *testing.T, client *codersdk.Client, buildID uuid.UUID, values map[string]string) { + t.Helper() + + params, err := client.WorkspaceBuildParameters(ctx, buildID) + require.NoError(t, err) + + for name, value := range values { + param, ok := slice.Find(params, func(parameter codersdk.WorkspaceBuildParameter) bool { + return parameter.Name == name + }) + if !ok { + assert.Failf(t, "parameter not found", "expected parameter %q to exist with value %q", name, value) + continue + } + assert.Equalf(t, value, param.Value, "parameter %q should have value %q", name, value) + } + + for _, param := range params { + if _, ok := values[param.Name]; !ok { + assert.Failf(t, "unexpected parameter", "parameter %q should not exist", param.Name) + } + } +} diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index ec1d6dbbd5a98..67c1f0dd21ade 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/quartz" "github.com/coder/coder/v2/coderd/coderdtest" @@ -164,7 +165,8 @@ func TestClaimPrebuild(t *testing.T) { }) defer provisionerCloser.Close() - reconciler := prebuilds.NewStoreReconciler(spy, pubsub, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(spy, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) var claimer agplprebuilds.Claimer = prebuilds.NewEnterpriseClaimer(spy) api.AGPL.PrebuildsClaimer.Store(&claimer) diff --git a/enterprise/coderd/prebuilds/metricscollector_test.go b/enterprise/coderd/prebuilds/metricscollector_test.go index 3b8461979c87a..057f310fa2bc2 100644 --- a/enterprise/coderd/prebuilds/metricscollector_test.go +++ b/enterprise/coderd/prebuilds/metricscollector_test.go @@ -13,6 +13,8 @@ import ( prometheus_client "github.com/prometheus/client_model/go" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/quartz" "github.com/coder/coder/v2/coderd/database" @@ -198,7 +200,8 @@ func TestMetricsCollector(t *testing.T) { }) clock := quartz.NewMock(t) db, pubsub := dbtestutil.NewDB(t) - reconciler := prebuilds.NewStoreReconciler(db, pubsub, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) ctx := testutil.Context(t, testutil.WaitLong) createdUsers := []uuid.UUID{database.PrebuildsSystemUserID} @@ -334,7 +337,8 @@ func TestMetricsCollector_DuplicateTemplateNames(t *testing.T) { logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) clock := quartz.NewMock(t) db, pubsub := dbtestutil.NewDB(t) - reconciler := prebuilds.NewStoreReconciler(db, pubsub, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(db, pubsub, cache, codersdk.PrebuildsConfig{}, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) ctx := testutil.Context(t, testutil.WaitLong) collector := prebuilds.NewMetricsCollector(db, logger, reconciler) diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 911336d36c426..8240ac78a58bc 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/quartz" "github.com/coder/coder/v2/coderd/audit" @@ -40,6 +41,7 @@ type StoreReconciler struct { store database.Store cfg codersdk.PrebuildsConfig pubsub pubsub.Pubsub + fileCache *files.Cache logger slog.Logger clock quartz.Clock registerer prometheus.Registerer @@ -57,6 +59,7 @@ var _ prebuilds.ReconciliationOrchestrator = &StoreReconciler{} func NewStoreReconciler(store database.Store, ps pubsub.Pubsub, + fileCache *files.Cache, cfg codersdk.PrebuildsConfig, logger slog.Logger, clock quartz.Clock, @@ -66,6 +69,7 @@ func NewStoreReconciler(store database.Store, reconciler := &StoreReconciler{ store: store, pubsub: ps, + fileCache: fileCache, logger: logger, cfg: cfg, clock: clock, @@ -780,6 +784,7 @@ func (c *StoreReconciler) provision( _, provisionerJob, _, err := builder.Build( ctx, db, + c.fileCache, func(_ policy.Action, _ rbac.Objecter) bool { return true // TODO: harden? }, diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index de8c65546334e..8524786dec404 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -13,7 +13,9 @@ import ( "github.com/stretchr/testify/assert" "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/files" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" "github.com/coder/coder/v2/coderd/util/slice" @@ -53,7 +55,8 @@ func TestNoReconciliationActionsIfNoPresets(t *testing.T) { ReconciliationInterval: serpent.Duration(testutil.WaitLong), } logger := testutil.Logger(t) - controller := prebuilds.NewStoreReconciler(db, ps, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) // given a template version with no presets org := dbgen.Organization(t, db, database.Organization{}) @@ -98,7 +101,8 @@ func TestNoReconciliationActionsIfNoPrebuilds(t *testing.T) { ReconciliationInterval: serpent.Duration(testutil.WaitLong), } logger := testutil.Logger(t) - controller := prebuilds.NewStoreReconciler(db, ps, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) // given there are presets, but no prebuilds org := dbgen.Organization(t, db, database.Organization{}) @@ -375,7 +379,8 @@ func TestPrebuildReconciliation(t *testing.T) { if useBrokenPubsub { pubSub = &brokenPublisher{Pubsub: pubSub} } - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) // Run the reconciliation multiple times to ensure idempotency // 8 was arbitrary, but large enough to reasonably trust the result @@ -452,7 +457,8 @@ func TestMultiplePresetsPerTemplateVersion(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -577,7 +583,8 @@ func TestPrebuildScheduling(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -681,7 +688,8 @@ func TestInvalidPreset(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -745,7 +753,8 @@ func TestDeletionOfPrebuiltWorkspaceWithInvalidPreset(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry(), newNoopEnqueuer()) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -841,7 +850,8 @@ func TestSkippingHardLimitedPresets(t *testing.T) { db, pubSub := dbtestutil.NewDB(t) fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock, registry, fakeEnqueuer) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer) // Template admin to receive a notification. templateAdmin := dbgen.User(t, db, database.User{ @@ -1003,7 +1013,8 @@ func TestHardLimitedPresetShouldNotBlockDeletion(t *testing.T) { db, pubSub := dbtestutil.NewDB(t) fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock, registry, fakeEnqueuer) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer) // Template admin to receive a notification. templateAdmin := dbgen.User(t, db, database.User{ @@ -1215,7 +1226,8 @@ func TestRunLoop(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, pubSub := dbtestutil.NewDB(t) - reconciler := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) ownerID := uuid.New() dbgen.User(t, db, database.User{ @@ -1345,7 +1357,8 @@ func TestFailedBuildBackoff(t *testing.T) { t, &slogtest.Options{IgnoreErrors: true}, ).Leveled(slog.LevelDebug) db, ps := dbtestutil.NewDB(t) - reconciler := prebuilds.NewStoreReconciler(db, ps, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(db, ps, cache, cfg, logger, clock, prometheus.NewRegistry(), newNoopEnqueuer()) // Given: an active template version with presets and prebuilds configured. const desiredInstances = 2 @@ -1461,9 +1474,11 @@ func TestReconciliationLock(t *testing.T) { wg.Add(1) go func() { defer wg.Done() + cache := files.New(prometheus.NewRegistry(), &coderdtest.FakeAuthorizer{}) reconciler := prebuilds.NewStoreReconciler( db, ps, + cache, codersdk.PrebuildsConfig{}, slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug), quartz.NewMock(t), @@ -1501,7 +1516,8 @@ func TestTrackResourceReplacement(t *testing.T) { fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() - reconciler := prebuilds.NewStoreReconciler(db, ps, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer) + cache := files.New(registry, &coderdtest.FakeAuthorizer{}) + reconciler := prebuilds.NewStoreReconciler(db, ps, cache, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer) // Given: a template admin to receive a notification. templateAdmin := dbgen.User(t, db, database.User{ @@ -1656,7 +1672,8 @@ func TestExpiredPrebuildsMultipleActions(t *testing.T) { db, pubSub := dbtestutil.NewDB(t) fakeEnqueuer := newFakeEnqueuer() registry := prometheus.NewRegistry() - controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, clock, registry, fakeEnqueuer) + cache := files.New(registry, &coderdtest.FakeAuthorizer{}) + controller := prebuilds.NewStoreReconciler(db, pubSub, cache, cfg, logger, clock, registry, fakeEnqueuer) // Set up test environment with a template, version, and preset ownerID := uuid.New() diff --git a/enterprise/coderd/testdata/parameters/dynamic/main.tf b/enterprise/coderd/testdata/parameters/dynamic/main.tf index 615f57dc9c074..a6926f46b66a2 100644 --- a/enterprise/coderd/testdata/parameters/dynamic/main.tf +++ b/enterprise/coderd/testdata/parameters/dynamic/main.tf @@ -1,8 +1,7 @@ terraform { required_providers { coder = { - source = "coder/coder" - version = "2.5.3" + source = "coder/coder" } } } @@ -52,6 +51,7 @@ locals { "red" : ["apple", "ruby"] "yellow" : ["banana"] "blue" : ["ocean", "sky"] + "green" : ["grass", "leaf"] } } @@ -101,3 +101,15 @@ data "coder_parameter" "cool" { order = 102 default = "true" } + +data "coder_parameter" "number" { + count = contains(local.selected, "green") ? 1 : 0 + name = "number" + type = "number" + order = 103 + validation { + error = "Number must be between 0 and 10" + min = 0 + max = 10 + } +} diff --git a/enterprise/coderd/testdata/parameters/ephemeral/main.tf b/enterprise/coderd/testdata/parameters/ephemeral/main.tf new file mode 100644 index 0000000000000..f632fcf11aea4 --- /dev/null +++ b/enterprise/coderd/testdata/parameters/ephemeral/main.tf @@ -0,0 +1,25 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + } + } +} + +data "coder_workspace_owner" "me" {} + +data "coder_parameter" "required" { + name = "required" + type = "string" + mutable = true + ephemeral = true +} + + +data "coder_parameter" "defaulted" { + name = "defaulted" + type = "string" + mutable = true + ephemeral = true + default = "original" +} diff --git a/enterprise/coderd/testdata/parameters/numbers/main.tf b/enterprise/coderd/testdata/parameters/numbers/main.tf new file mode 100644 index 0000000000000..c4950db326419 --- /dev/null +++ b/enterprise/coderd/testdata/parameters/numbers/main.tf @@ -0,0 +1,20 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + } + } +} + +data "coder_workspace_owner" "me" {} + +data "coder_parameter" "number" { + name = "number" + type = "number" + mutable = false + validation { + error = "Number must be between 0 and 10" + min = 0 + max = 10 + } +} diff --git a/enterprise/coderd/testdata/parameters/regex/main.tf b/enterprise/coderd/testdata/parameters/regex/main.tf new file mode 100644 index 0000000000000..9fbaa5e245056 --- /dev/null +++ b/enterprise/coderd/testdata/parameters/regex/main.tf @@ -0,0 +1,18 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + } + } +} + +data "coder_workspace_owner" "me" {} + +data "coder_parameter" "string" { + name = "string" + type = "string" + validation { + error = "All messages must start with 'Hello'" + regex = "^Hello" + } +} From 4699393522f25468ed002bc7f3a259a74960c4ce Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 23 Jun 2025 20:50:44 +0200 Subject: [PATCH 018/690] fix: upsert `coder_app` resources in case they are persistent (#18509) --- coderd/agentapi/subagent.go | 7 +- coderd/database/dbauthz/dbauthz.go | 34 ++-- coderd/database/dbauthz/dbauthz_test.go | 4 +- coderd/database/dbgen/dbgen.go | 2 +- coderd/database/dbmem/dbmem.go | 94 ++++++----- coderd/database/dbmetrics/querymetrics.go | 14 +- coderd/database/dbmock/dbmock.go | 30 ++-- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 155 ++++++++++-------- coderd/database/queries/workspaceapps.sql | 23 ++- .../provisionerdserver/provisionerdserver.go | 6 +- coderd/workspaces_test.go | 87 +++++++++- 12 files changed, 297 insertions(+), 161 deletions(-) diff --git a/coderd/agentapi/subagent.go b/coderd/agentapi/subagent.go index c00bfecc5ff17..1868ad39bd362 100644 --- a/coderd/agentapi/subagent.go +++ b/coderd/agentapi/subagent.go @@ -12,12 +12,13 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/quartz" + agentproto "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisioner" - "github.com/coder/quartz" ) type SubAgentAPI struct { @@ -164,8 +165,8 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create } } - _, err := a.Database.InsertWorkspaceApp(ctx, database.InsertWorkspaceAppParams{ - ID: uuid.New(), + _, err := a.Database.UpsertWorkspaceApp(ctx, database.UpsertWorkspaceAppParams{ + ID: uuid.New(), // NOTE: we may need to maintain the app's ID here for stability, but for now we'll leave this as-is. CreatedAt: createdAt, AgentID: subAgent.ID, Slug: app.Slug, diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index adb2007918f8d..3c6f691616d52 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -3938,23 +3938,6 @@ func (q *querier) InsertWorkspaceAgentStats(ctx context.Context, arg database.In return q.db.InsertWorkspaceAgentStats(ctx, arg) } -func (q *querier) InsertWorkspaceApp(ctx context.Context, arg database.InsertWorkspaceAppParams) (database.WorkspaceApp, error) { - // NOTE(DanielleMaywood): - // It is possible for there to exist an agent without a workspace. - // This means that we want to allow execution to continue if - // there isn't a workspace found to allow this behavior to continue. - workspace, err := q.db.GetWorkspaceByAgentID(ctx, arg.AgentID) - if err != nil && !errors.Is(err, sql.ErrNoRows) { - return database.WorkspaceApp{}, err - } - - if err := q.authorizeContext(ctx, policy.ActionUpdate, workspace); err != nil { - return database.WorkspaceApp{}, err - } - - return q.db.InsertWorkspaceApp(ctx, arg) -} - func (q *querier) InsertWorkspaceAppStats(ctx context.Context, arg database.InsertWorkspaceAppStatsParams) error { if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceSystem); err != nil { return err @@ -5181,6 +5164,23 @@ func (q *querier) UpsertWorkspaceAgentPortShare(ctx context.Context, arg databas return q.db.UpsertWorkspaceAgentPortShare(ctx, arg) } +func (q *querier) UpsertWorkspaceApp(ctx context.Context, arg database.UpsertWorkspaceAppParams) (database.WorkspaceApp, error) { + // NOTE(DanielleMaywood): + // It is possible for there to exist an agent without a workspace. + // This means that we want to allow execution to continue if + // there isn't a workspace found to allow this behavior to continue. + workspace, err := q.db.GetWorkspaceByAgentID(ctx, arg.AgentID) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return database.WorkspaceApp{}, err + } + + if err := q.authorizeContext(ctx, policy.ActionUpdate, workspace); err != nil { + return database.WorkspaceApp{}, err + } + + return q.db.UpsertWorkspaceApp(ctx, arg) +} + func (q *querier) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) { if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil { return false, err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 0ccd867040116..f5077b7bdcda9 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -4114,7 +4114,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { APIKeyScope: database.AgentKeyScopeEnumAll, }).Asserts(ws, policy.ActionCreateAgent) })) - s.Run("InsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) { + s.Run("UpsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) { _ = dbgen.User(s.T(), db, database.User{}) u := dbgen.User(s.T(), db, database.User{}) o := dbgen.Organization(s.T(), db, database.Organization{}) @@ -4130,7 +4130,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { _ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: j.ID, TemplateVersionID: tv.ID}) res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: j.ID}) agent := dbgen.WorkspaceAgent(s.T(), db, database.WorkspaceAgent{ResourceID: res.ID}) - check.Args(database.InsertWorkspaceAppParams{ + check.Args(database.UpsertWorkspaceAppParams{ ID: uuid.New(), AgentID: agent.ID, Health: database.WorkspaceAppHealthDisabled, diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 5b03fd0eb1396..5b94b2e789c5f 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -778,7 +778,7 @@ func ProvisionerKey(t testing.TB, db database.Store, orig database.ProvisionerKe } func WorkspaceApp(t testing.TB, db database.Store, orig database.WorkspaceApp) database.WorkspaceApp { - resource, err := db.InsertWorkspaceApp(genCtx, database.InsertWorkspaceAppParams{ + resource, err := db.UpsertWorkspaceApp(genCtx, database.UpsertWorkspaceAppParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, dbtime.Now()), AgentID: takeFirst(orig.AgentID, uuid.New()), diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index f4403d2f50c75..117148a908979 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -10018,48 +10018,6 @@ func (q *FakeQuerier) InsertWorkspaceAgentStats(_ context.Context, arg database. return nil } -func (q *FakeQuerier) InsertWorkspaceApp(_ context.Context, arg database.InsertWorkspaceAppParams) (database.WorkspaceApp, error) { - if err := validateDatabaseType(arg); err != nil { - return database.WorkspaceApp{}, err - } - - q.mutex.Lock() - defer q.mutex.Unlock() - - if arg.SharingLevel == "" { - arg.SharingLevel = database.AppSharingLevelOwner - } - - if arg.OpenIn == "" { - arg.OpenIn = database.WorkspaceAppOpenInSlimWindow - } - - // nolint:gosimple - workspaceApp := database.WorkspaceApp{ - ID: arg.ID, - AgentID: arg.AgentID, - CreatedAt: arg.CreatedAt, - Slug: arg.Slug, - DisplayName: arg.DisplayName, - Icon: arg.Icon, - Command: arg.Command, - Url: arg.Url, - External: arg.External, - Subdomain: arg.Subdomain, - SharingLevel: arg.SharingLevel, - HealthcheckUrl: arg.HealthcheckUrl, - HealthcheckInterval: arg.HealthcheckInterval, - HealthcheckThreshold: arg.HealthcheckThreshold, - Health: arg.Health, - Hidden: arg.Hidden, - DisplayOrder: arg.DisplayOrder, - OpenIn: arg.OpenIn, - DisplayGroup: arg.DisplayGroup, - } - q.workspaceApps = append(q.workspaceApps, workspaceApp) - return workspaceApp, nil -} - func (q *FakeQuerier) InsertWorkspaceAppStats(_ context.Context, arg database.InsertWorkspaceAppStatsParams) error { err := validateDatabaseType(arg) if err != nil { @@ -13192,6 +13150,58 @@ func (q *FakeQuerier) UpsertWorkspaceAgentPortShare(_ context.Context, arg datab return psl, nil } +func (q *FakeQuerier) UpsertWorkspaceApp(ctx context.Context, arg database.UpsertWorkspaceAppParams) (database.WorkspaceApp, error) { + err := validateDatabaseType(arg) + if err != nil { + return database.WorkspaceApp{}, err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + if arg.SharingLevel == "" { + arg.SharingLevel = database.AppSharingLevelOwner + } + if arg.OpenIn == "" { + arg.OpenIn = database.WorkspaceAppOpenInSlimWindow + } + + buildApp := func(id uuid.UUID, createdAt time.Time) database.WorkspaceApp { + return database.WorkspaceApp{ + ID: id, + CreatedAt: createdAt, + AgentID: arg.AgentID, + Slug: arg.Slug, + DisplayName: arg.DisplayName, + Icon: arg.Icon, + Command: arg.Command, + Url: arg.Url, + External: arg.External, + Subdomain: arg.Subdomain, + SharingLevel: arg.SharingLevel, + HealthcheckUrl: arg.HealthcheckUrl, + HealthcheckInterval: arg.HealthcheckInterval, + HealthcheckThreshold: arg.HealthcheckThreshold, + Health: arg.Health, + Hidden: arg.Hidden, + DisplayOrder: arg.DisplayOrder, + OpenIn: arg.OpenIn, + DisplayGroup: arg.DisplayGroup, + } + } + + for i, app := range q.workspaceApps { + if app.ID == arg.ID { + q.workspaceApps[i] = buildApp(app.ID, app.CreatedAt) + return q.workspaceApps[i], nil + } + } + + workspaceApp := buildApp(arg.ID, arg.CreatedAt) + q.workspaceApps = append(q.workspaceApps, workspaceApp) + return workspaceApp, nil +} + func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) { err := validateDatabaseType(arg) if err != nil { diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 0450776785d42..3a5bfac1ae57b 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -2440,13 +2440,6 @@ func (m queryMetricsStore) InsertWorkspaceAgentStats(ctx context.Context, arg da return r0 } -func (m queryMetricsStore) InsertWorkspaceApp(ctx context.Context, arg database.InsertWorkspaceAppParams) (database.WorkspaceApp, error) { - start := time.Now() - app, err := m.s.InsertWorkspaceApp(ctx, arg) - m.queryLatencies.WithLabelValues("InsertWorkspaceApp").Observe(time.Since(start).Seconds()) - return app, err -} - func (m queryMetricsStore) InsertWorkspaceAppStats(ctx context.Context, arg database.InsertWorkspaceAppStatsParams) error { start := time.Now() r0 := m.s.InsertWorkspaceAppStats(ctx, arg) @@ -3287,6 +3280,13 @@ func (m queryMetricsStore) UpsertWorkspaceAgentPortShare(ctx context.Context, ar return r0, r1 } +func (m queryMetricsStore) UpsertWorkspaceApp(ctx context.Context, arg database.UpsertWorkspaceAppParams) (database.WorkspaceApp, error) { + start := time.Now() + r0, r1 := m.s.UpsertWorkspaceApp(ctx, arg) + m.queryLatencies.WithLabelValues("UpsertWorkspaceApp").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m queryMetricsStore) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) { start := time.Now() r0, r1 := m.s.UpsertWorkspaceAppAuditSession(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index dbd8f5ca0753c..067ad062e1dd8 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -5150,21 +5150,6 @@ func (mr *MockStoreMockRecorder) InsertWorkspaceAgentStats(ctx, arg any) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertWorkspaceAgentStats", reflect.TypeOf((*MockStore)(nil).InsertWorkspaceAgentStats), ctx, arg) } -// InsertWorkspaceApp mocks base method. -func (m *MockStore) InsertWorkspaceApp(ctx context.Context, arg database.InsertWorkspaceAppParams) (database.WorkspaceApp, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "InsertWorkspaceApp", ctx, arg) - ret0, _ := ret[0].(database.WorkspaceApp) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// InsertWorkspaceApp indicates an expected call of InsertWorkspaceApp. -func (mr *MockStoreMockRecorder) InsertWorkspaceApp(ctx, arg any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertWorkspaceApp", reflect.TypeOf((*MockStore)(nil).InsertWorkspaceApp), ctx, arg) -} - // InsertWorkspaceAppStats mocks base method. func (m *MockStore) InsertWorkspaceAppStats(ctx context.Context, arg database.InsertWorkspaceAppStatsParams) error { m.ctrl.T.Helper() @@ -6924,6 +6909,21 @@ func (mr *MockStoreMockRecorder) UpsertWorkspaceAgentPortShare(ctx, arg any) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpsertWorkspaceAgentPortShare", reflect.TypeOf((*MockStore)(nil).UpsertWorkspaceAgentPortShare), ctx, arg) } +// UpsertWorkspaceApp mocks base method. +func (m *MockStore) UpsertWorkspaceApp(ctx context.Context, arg database.UpsertWorkspaceAppParams) (database.WorkspaceApp, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpsertWorkspaceApp", ctx, arg) + ret0, _ := ret[0].(database.WorkspaceApp) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UpsertWorkspaceApp indicates an expected call of UpsertWorkspaceApp. +func (mr *MockStoreMockRecorder) UpsertWorkspaceApp(ctx, arg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpsertWorkspaceApp", reflect.TypeOf((*MockStore)(nil).UpsertWorkspaceApp), ctx, arg) +} + // UpsertWorkspaceAppAuditSession mocks base method. func (m *MockStore) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4cfd0d1c4da5f..06f51785aa1af 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -530,7 +530,6 @@ type sqlcQuerier interface { InsertWorkspaceAgentScriptTimings(ctx context.Context, arg InsertWorkspaceAgentScriptTimingsParams) (WorkspaceAgentScriptTiming, error) InsertWorkspaceAgentScripts(ctx context.Context, arg InsertWorkspaceAgentScriptsParams) ([]WorkspaceAgentScript, error) InsertWorkspaceAgentStats(ctx context.Context, arg InsertWorkspaceAgentStatsParams) error - InsertWorkspaceApp(ctx context.Context, arg InsertWorkspaceAppParams) (WorkspaceApp, error) InsertWorkspaceAppStats(ctx context.Context, arg InsertWorkspaceAppStatsParams) error InsertWorkspaceAppStatus(ctx context.Context, arg InsertWorkspaceAppStatusParams) (WorkspaceAppStatus, error) InsertWorkspaceBuild(ctx context.Context, arg InsertWorkspaceBuildParams) error @@ -671,6 +670,7 @@ type sqlcQuerier interface { UpsertTemplateUsageStats(ctx context.Context) error UpsertWebpushVAPIDKeys(ctx context.Context, arg UpsertWebpushVAPIDKeysParams) error UpsertWorkspaceAgentPortShare(ctx context.Context, arg UpsertWorkspaceAgentPortShareParams) (WorkspaceAgentPortShare, error) + UpsertWorkspaceApp(ctx context.Context, arg UpsertWorkspaceAppParams) (WorkspaceApp, error) // // The returned boolean, new_or_stale, can be used to deduce if a new session // was started. This means that a new row was inserted (no previous session) or diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index fe32851f0e002..3291e85b7576d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -16673,7 +16673,68 @@ func (q *sqlQuerier) GetWorkspaceAppsCreatedAfter(ctx context.Context, createdAt return items, nil } -const insertWorkspaceApp = `-- name: InsertWorkspaceApp :one +const insertWorkspaceAppStatus = `-- name: InsertWorkspaceAppStatus :one +INSERT INTO workspace_app_statuses (id, created_at, workspace_id, agent_id, app_id, state, message, uri) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8) +RETURNING id, created_at, agent_id, app_id, workspace_id, state, message, uri +` + +type InsertWorkspaceAppStatusParams struct { + ID uuid.UUID `db:"id" json:"id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + WorkspaceID uuid.UUID `db:"workspace_id" json:"workspace_id"` + AgentID uuid.UUID `db:"agent_id" json:"agent_id"` + AppID uuid.UUID `db:"app_id" json:"app_id"` + State WorkspaceAppStatusState `db:"state" json:"state"` + Message string `db:"message" json:"message"` + Uri sql.NullString `db:"uri" json:"uri"` +} + +func (q *sqlQuerier) InsertWorkspaceAppStatus(ctx context.Context, arg InsertWorkspaceAppStatusParams) (WorkspaceAppStatus, error) { + row := q.db.QueryRowContext(ctx, insertWorkspaceAppStatus, + arg.ID, + arg.CreatedAt, + arg.WorkspaceID, + arg.AgentID, + arg.AppID, + arg.State, + arg.Message, + arg.Uri, + ) + var i WorkspaceAppStatus + err := row.Scan( + &i.ID, + &i.CreatedAt, + &i.AgentID, + &i.AppID, + &i.WorkspaceID, + &i.State, + &i.Message, + &i.Uri, + ) + return i, err +} + +const updateWorkspaceAppHealthByID = `-- name: UpdateWorkspaceAppHealthByID :exec +UPDATE + workspace_apps +SET + health = $2 +WHERE + id = $1 +` + +type UpdateWorkspaceAppHealthByIDParams struct { + ID uuid.UUID `db:"id" json:"id"` + Health WorkspaceAppHealth `db:"health" json:"health"` +} + +func (q *sqlQuerier) UpdateWorkspaceAppHealthByID(ctx context.Context, arg UpdateWorkspaceAppHealthByIDParams) error { + _, err := q.db.ExecContext(ctx, updateWorkspaceAppHealthByID, arg.ID, arg.Health) + return err +} + +const upsertWorkspaceApp = `-- name: UpsertWorkspaceApp :one INSERT INTO workspace_apps ( id, @@ -16697,10 +16758,29 @@ INSERT INTO display_group ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) RETURNING id, created_at, agent_id, display_name, icon, command, url, healthcheck_url, healthcheck_interval, healthcheck_threshold, health, subdomain, sharing_level, slug, external, display_order, hidden, open_in, display_group -` - -type InsertWorkspaceAppParams struct { + ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) +ON CONFLICT (id) DO UPDATE SET + display_name = EXCLUDED.display_name, + icon = EXCLUDED.icon, + command = EXCLUDED.command, + url = EXCLUDED.url, + external = EXCLUDED.external, + subdomain = EXCLUDED.subdomain, + sharing_level = EXCLUDED.sharing_level, + healthcheck_url = EXCLUDED.healthcheck_url, + healthcheck_interval = EXCLUDED.healthcheck_interval, + healthcheck_threshold = EXCLUDED.healthcheck_threshold, + health = EXCLUDED.health, + display_order = EXCLUDED.display_order, + hidden = EXCLUDED.hidden, + open_in = EXCLUDED.open_in, + display_group = EXCLUDED.display_group, + agent_id = EXCLUDED.agent_id, + slug = EXCLUDED.slug +RETURNING id, created_at, agent_id, display_name, icon, command, url, healthcheck_url, healthcheck_interval, healthcheck_threshold, health, subdomain, sharing_level, slug, external, display_order, hidden, open_in, display_group +` + +type UpsertWorkspaceAppParams struct { ID uuid.UUID `db:"id" json:"id"` CreatedAt time.Time `db:"created_at" json:"created_at"` AgentID uuid.UUID `db:"agent_id" json:"agent_id"` @@ -16722,8 +16802,8 @@ type InsertWorkspaceAppParams struct { DisplayGroup sql.NullString `db:"display_group" json:"display_group"` } -func (q *sqlQuerier) InsertWorkspaceApp(ctx context.Context, arg InsertWorkspaceAppParams) (WorkspaceApp, error) { - row := q.db.QueryRowContext(ctx, insertWorkspaceApp, +func (q *sqlQuerier) UpsertWorkspaceApp(ctx context.Context, arg UpsertWorkspaceAppParams) (WorkspaceApp, error) { + row := q.db.QueryRowContext(ctx, upsertWorkspaceApp, arg.ID, arg.CreatedAt, arg.AgentID, @@ -16769,67 +16849,6 @@ func (q *sqlQuerier) InsertWorkspaceApp(ctx context.Context, arg InsertWorkspace return i, err } -const insertWorkspaceAppStatus = `-- name: InsertWorkspaceAppStatus :one -INSERT INTO workspace_app_statuses (id, created_at, workspace_id, agent_id, app_id, state, message, uri) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8) -RETURNING id, created_at, agent_id, app_id, workspace_id, state, message, uri -` - -type InsertWorkspaceAppStatusParams struct { - ID uuid.UUID `db:"id" json:"id"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - WorkspaceID uuid.UUID `db:"workspace_id" json:"workspace_id"` - AgentID uuid.UUID `db:"agent_id" json:"agent_id"` - AppID uuid.UUID `db:"app_id" json:"app_id"` - State WorkspaceAppStatusState `db:"state" json:"state"` - Message string `db:"message" json:"message"` - Uri sql.NullString `db:"uri" json:"uri"` -} - -func (q *sqlQuerier) InsertWorkspaceAppStatus(ctx context.Context, arg InsertWorkspaceAppStatusParams) (WorkspaceAppStatus, error) { - row := q.db.QueryRowContext(ctx, insertWorkspaceAppStatus, - arg.ID, - arg.CreatedAt, - arg.WorkspaceID, - arg.AgentID, - arg.AppID, - arg.State, - arg.Message, - arg.Uri, - ) - var i WorkspaceAppStatus - err := row.Scan( - &i.ID, - &i.CreatedAt, - &i.AgentID, - &i.AppID, - &i.WorkspaceID, - &i.State, - &i.Message, - &i.Uri, - ) - return i, err -} - -const updateWorkspaceAppHealthByID = `-- name: UpdateWorkspaceAppHealthByID :exec -UPDATE - workspace_apps -SET - health = $2 -WHERE - id = $1 -` - -type UpdateWorkspaceAppHealthByIDParams struct { - ID uuid.UUID `db:"id" json:"id"` - Health WorkspaceAppHealth `db:"health" json:"health"` -} - -func (q *sqlQuerier) UpdateWorkspaceAppHealthByID(ctx context.Context, arg UpdateWorkspaceAppHealthByIDParams) error { - _, err := q.db.ExecContext(ctx, updateWorkspaceAppHealthByID, arg.ID, arg.Health) - return err -} - const insertWorkspaceAppStats = `-- name: InsertWorkspaceAppStats :exec INSERT INTO workspace_app_stats ( diff --git a/coderd/database/queries/workspaceapps.sql b/coderd/database/queries/workspaceapps.sql index f3f8e4066970b..9a6fbf9251788 100644 --- a/coderd/database/queries/workspaceapps.sql +++ b/coderd/database/queries/workspaceapps.sql @@ -10,7 +10,7 @@ SELECT * FROM workspace_apps WHERE agent_id = $1 AND slug = $2; -- name: GetWorkspaceAppsCreatedAfter :many SELECT * FROM workspace_apps WHERE created_at > $1 ORDER BY slug ASC; --- name: InsertWorkspaceApp :one +-- name: UpsertWorkspaceApp :one INSERT INTO workspace_apps ( id, @@ -34,7 +34,26 @@ INSERT INTO display_group ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) RETURNING *; + ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) +ON CONFLICT (id) DO UPDATE SET + display_name = EXCLUDED.display_name, + icon = EXCLUDED.icon, + command = EXCLUDED.command, + url = EXCLUDED.url, + external = EXCLUDED.external, + subdomain = EXCLUDED.subdomain, + sharing_level = EXCLUDED.sharing_level, + healthcheck_url = EXCLUDED.healthcheck_url, + healthcheck_interval = EXCLUDED.healthcheck_interval, + healthcheck_threshold = EXCLUDED.healthcheck_threshold, + health = EXCLUDED.health, + display_order = EXCLUDED.display_order, + hidden = EXCLUDED.hidden, + open_in = EXCLUDED.open_in, + display_group = EXCLUDED.display_group, + agent_id = EXCLUDED.agent_id, + slug = EXCLUDED.slug +RETURNING *; -- name: UpdateWorkspaceAppHealthByID :exec UPDATE diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index a8f7c63a586fe..580520e6e4b4b 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -28,6 +28,7 @@ import ( protobuf "google.golang.org/protobuf/proto" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk/drpcsdk" @@ -2606,7 +2607,8 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. return xerrors.Errorf("parse app uuid: %w", err) } - dbApp, err := db.InsertWorkspaceApp(ctx, database.InsertWorkspaceAppParams{ + // If workspace apps are "persistent", the ID will not be regenerated across workspace builds, so we have to upsert. + dbApp, err := db.UpsertWorkspaceApp(ctx, database.UpsertWorkspaceAppParams{ ID: id, CreatedAt: dbtime.Now(), AgentID: dbAgent.ID, @@ -2635,7 +2637,7 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. OpenIn: openIn, }) if err != nil { - return xerrors.Errorf("insert app: %w", err) + return xerrors.Errorf("upsert app: %w", err) } snapshot.WorkspaceApps = append(snapshot.WorkspaceApps, telemetry.ConvertWorkspaceApp(dbApp)) } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 4f48136abb3ee..74554962fbd1e 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -19,6 +19,8 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog" + "github.com/coder/terraform-provider-coder/v2/provider" + "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" @@ -42,7 +44,6 @@ import ( "github.com/coder/coder/v2/provisioner/echo" "github.com/coder/coder/v2/provisionersdk/proto" "github.com/coder/coder/v2/testutil" - "github.com/coder/terraform-provider-coder/v2/provider" ) func TestWorkspace(t *testing.T) { @@ -4614,3 +4615,87 @@ func TestWorkspaceFilterHasAITask(t *testing.T) { require.NoError(t, err) require.Len(t, res.Workspaces, 5) } + +func TestWorkspaceAppUpsertRestart(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }) + user := coderdtest.CreateFirstUser(t, client) + + // Define an app to be created with the workspace + apps := []*proto.App{ + { + Id: uuid.NewString(), + Slug: "test-app", + DisplayName: "Test App", + Command: "test-command", + Url: "http://localhost:8080", + Icon: "/test.svg", + }, + } + + // Create template version with workspace app + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: []*proto.Response{{ + Type: &proto.Response_Apply{ + Apply: &proto.ApplyComplete{ + Resources: []*proto.Resource{{ + Name: "test-resource", + Type: "example", + Agents: []*proto.Agent{{ + Id: uuid.NewString(), + Name: "dev", + Auth: &proto.Agent_Token{}, + Apps: apps, + }}, + }}, + }, + }, + }}, + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + + // Create template and workspace + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Verify initial workspace has the app + workspace, err := client.Workspace(ctx, workspace.ID) + require.NoError(t, err) + require.Len(t, workspace.LatestBuild.Resources[0].Agents, 1) + agent := workspace.LatestBuild.Resources[0].Agents[0] + require.Len(t, agent.Apps, 1) + require.Equal(t, "test-app", agent.Apps[0].Slug) + require.Equal(t, "Test App", agent.Apps[0].DisplayName) + + // Stop the workspace + stopBuild := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStop) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, stopBuild.ID) + + // Restart the workspace (this will trigger upsert for the app) + startBuild := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, startBuild.ID) + + // Verify the workspace restarted successfully + workspace, err = client.Workspace(ctx, workspace.ID) + require.NoError(t, err) + require.Equal(t, codersdk.WorkspaceStatusRunning, workspace.LatestBuild.Status) + + // Verify the app is still present after restart (upsert worked) + require.Len(t, workspace.LatestBuild.Resources[0].Agents, 1) + agent = workspace.LatestBuild.Resources[0].Agents[0] + require.Len(t, agent.Apps, 1) + require.Equal(t, "test-app", agent.Apps[0].Slug) + require.Equal(t, "Test App", agent.Apps[0].DisplayName) + + // Verify the provisioner job completed successfully (no error) + require.Equal(t, codersdk.ProvisionerJobSucceeded, workspace.LatestBuild.Job.Status) + require.Empty(t, workspace.LatestBuild.Job.Error) +} From 7dd90f3076fc7894cb8cbfa9e1e225c598d246af Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 23 Jun 2025 11:44:26 -0800 Subject: [PATCH 019/690] chore: show iframe controls on preview slug only (#18480) --- site/src/pages/TaskPage/TaskAppIframe.tsx | 70 +++++------ site/src/pages/TaskPage/TaskPage.stories.tsx | 117 +++++++++++-------- 2 files changed, 106 insertions(+), 81 deletions(-) diff --git a/site/src/pages/TaskPage/TaskAppIframe.tsx b/site/src/pages/TaskPage/TaskAppIframe.tsx index 860fc64cbbcec..b995dfec771b6 100644 --- a/site/src/pages/TaskPage/TaskAppIframe.tsx +++ b/site/src/pages/TaskPage/TaskAppIframe.tsx @@ -58,42 +58,44 @@ export const TaskAppIFrame: FC = ({ return (
-
- + {app.slug === "preview" && ( +
+ - {/* Possibly we will put a URL bar here, but for now we cannot due to - * cross-origin restrictions in iframes. */} -
+ {/* Possibly we will put a URL bar here, but for now we cannot due to + * cross-origin restrictions in iframes. */} +
- - - - - - - - - Open app in new tab - - - - -
+ + + + + + + + + Open app in new tab + + + + +
+ )}