diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 17d3d199639cc..75a958fcc8968 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -209,9 +209,17 @@ func WorkspaceAgentScript(t testing.TB, db database.Store, orig database.Workspa return scripts[0] } -func WorkspaceAgentScriptTimings(t testing.TB, db database.Store, script database.WorkspaceAgentScript, count int) []database.WorkspaceAgentScriptTiming { - timings := make([]database.WorkspaceAgentScriptTiming, count) - for i := range count { +func WorkspaceAgentScripts(t testing.TB, db database.Store, count int, orig database.WorkspaceAgentScript) []database.WorkspaceAgentScript { + scripts := make([]database.WorkspaceAgentScript, 0, count) + for range count { + scripts = append(scripts, WorkspaceAgentScript(t, db, orig)) + } + return scripts +} + +func WorkspaceAgentScriptTimings(t testing.TB, db database.Store, scripts []database.WorkspaceAgentScript) []database.WorkspaceAgentScriptTiming { + timings := make([]database.WorkspaceAgentScriptTiming, len(scripts)) + for i, script := range scripts { timings[i] = WorkspaceAgentScriptTiming(t, db, database.WorkspaceAgentScriptTiming{ ScriptID: script.ID, }) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 507f040abbd9b..50e9a1bac055b 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -6018,6 +6018,15 @@ func (q *FakeQuerier) GetWorkspaceAgentScriptTimingsByBuildID(ctx context.Contex WorkspaceAgentName: agent.Name, }) } + + // We want to only return the first script run for each Script ID. + slices.SortFunc(rows, func(a, b database.GetWorkspaceAgentScriptTimingsByBuildIDRow) int { + return a.StartedAt.Compare(b.StartedAt) + }) + rows = slices.CompactFunc(rows, func(e1, e2 database.GetWorkspaceAgentScriptTimingsByBuildIDRow) bool { + return e1.ScriptID == e2.ScriptID + }) + return rows, nil } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index ea2b7be288adb..ec75a8ed4f56e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -11656,7 +11656,7 @@ func (q *sqlQuerier) GetWorkspaceAgentMetadata(ctx context.Context, arg GetWorks const getWorkspaceAgentScriptTimingsByBuildID = `-- name: GetWorkspaceAgentScriptTimingsByBuildID :many SELECT - workspace_agent_script_timings.script_id, workspace_agent_script_timings.started_at, workspace_agent_script_timings.ended_at, workspace_agent_script_timings.exit_code, workspace_agent_script_timings.stage, workspace_agent_script_timings.status, + DISTINCT ON (workspace_agent_script_timings.script_id) workspace_agent_script_timings.script_id, workspace_agent_script_timings.started_at, workspace_agent_script_timings.ended_at, workspace_agent_script_timings.exit_code, workspace_agent_script_timings.stage, workspace_agent_script_timings.status, workspace_agent_scripts.display_name, workspace_agents.id as workspace_agent_id, workspace_agents.name as workspace_agent_name @@ -11666,6 +11666,7 @@ INNER JOIN workspace_agents ON workspace_agents.id = workspace_agent_scripts.wor INNER JOIN workspace_resources ON workspace_resources.id = workspace_agents.resource_id INNER JOIN workspace_builds ON workspace_builds.job_id = workspace_resources.job_id WHERE workspace_builds.id = $1 +ORDER BY workspace_agent_script_timings.script_id, workspace_agent_script_timings.started_at ` type GetWorkspaceAgentScriptTimingsByBuildIDRow struct { diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index df7c829861cb2..52d8b5275fc97 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -304,7 +304,7 @@ RETURNING workspace_agent_script_timings.*; -- name: GetWorkspaceAgentScriptTimingsByBuildID :many SELECT - workspace_agent_script_timings.*, + DISTINCT ON (workspace_agent_script_timings.script_id) workspace_agent_script_timings.*, workspace_agent_scripts.display_name, workspace_agents.id as workspace_agent_id, workspace_agents.name as workspace_agent_name @@ -313,4 +313,5 @@ INNER JOIN workspace_agent_scripts ON workspace_agent_scripts.id = workspace_age INNER JOIN workspace_agents ON workspace_agents.id = workspace_agent_scripts.workspace_agent_id INNER JOIN workspace_resources ON workspace_resources.id = workspace_agents.resource_id INNER JOIN workspace_builds ON workspace_builds.job_id = workspace_resources.job_id -WHERE workspace_builds.id = $1; \ No newline at end of file +WHERE workspace_builds.id = $1 +ORDER BY workspace_agent_script_timings.script_id, workspace_agent_script_timings.started_at; diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index b27af7d0093a0..241fa385681e6 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -109,37 +109,20 @@ func ExtractWorkspaceAgentAndLatestBuild(opts ExtractWorkspaceAgentAndLatestBuil return } - //nolint:gocritic // System needs to be able to get owner roles. - roles, err := opts.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), row.WorkspaceTable.OwnerID) + subject, _, err := UserRBACSubject(ctx, opts.DB, row.WorkspaceTable.OwnerID, rbac.WorkspaceAgentScope(rbac.WorkspaceAgentScopeParams{ + WorkspaceID: row.WorkspaceTable.ID, + OwnerID: row.WorkspaceTable.OwnerID, + TemplateID: row.WorkspaceTable.TemplateID, + VersionID: row.WorkspaceBuild.TemplateVersionID, + })) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error checking workspace agent authorization.", + Message: "Internal error with workspace agent authorization context.", Detail: err.Error(), }) return } - roleNames, err := roles.RoleNames() - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal server error", - Detail: err.Error(), - }) - return - } - - subject := rbac.Subject{ - ID: row.WorkspaceTable.OwnerID.String(), - Roles: rbac.RoleIdentifiers(roleNames), - Groups: roles.Groups, - Scope: rbac.WorkspaceAgentScope(rbac.WorkspaceAgentScopeParams{ - WorkspaceID: row.WorkspaceTable.ID, - OwnerID: row.WorkspaceTable.OwnerID, - TemplateID: row.WorkspaceTable.TemplateID, - VersionID: row.WorkspaceBuild.TemplateVersionID, - }), - }.WithCachedASTValue() - ctx = context.WithValue(ctx, workspaceAgentContextKey{}, row.WorkspaceAgent) ctx = context.WithValue(ctx, latestBuildContextKey{}, row.WorkspaceBuild) // Also set the dbauthz actor for the request. diff --git a/coderd/templateversions.go b/coderd/templateversions.go index e9297d02e2a55..d47a3f96cefc1 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -1597,11 +1597,9 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } // Ensure the "owner" tag is properly applied in addition to request tags and coder_workspace_tags. - // Tag order precedence: - // 1) User-specified tags in the request - // 2) Tags parsed from coder_workspace_tags data source in template file - // 2 may clobber 1. - tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags) + // User-specified tags in the request will take precedence over tags parsed from `coder_workspace_tags` + // data sources defined in the template file. + tags := provisionersdk.MutateTags(apiKey.UserID, parsedTags, req.ProvisionerTags) var templateVersion database.TemplateVersion var provisionerJob database.ProvisionerJob diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 7d386988fe16d..75c05c6af76b2 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -355,10 +355,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "a": "1", "b": "2"}, }, { - name: "main.tf with workspace tags and request tags", + name: "main.tf with request tags not clobbering workspace tags", files: map[string]string{ `main.tf`: ` - // This file is the same as the above, except for this comment. + // This file is, once again, the same as the above, except + // for a slightly different comment. variable "a" { type = string default = "1" @@ -381,9 +382,57 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { } }`, }, - reqTags: map[string]string{"baz": "zap", "foo": "noclobber"}, + reqTags: map[string]string{"baz": "zap"}, wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "baz": "zap", "a": "1", "b": "2"}, }, + { + name: "main.tf with request tags clobbering workspace tags", + files: map[string]string{ + `main.tf`: ` + // This file is the same as the above, except for this comment. + variable "a" { + type = string + default = "1" + } + data "coder_parameter" "b" { + type = string + default = "2" + } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } + resource "null_resource" "test" {} + data "coder_workspace_tags" "tags" { + tags = { + "foo": "bar", + "a": var.a, + "b": data.coder_parameter.b.value, + } + }`, + }, + reqTags: map[string]string{"baz": "zap", "foo": "clobbered"}, + wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "clobbered", "baz": "zap", "a": "1", "b": "2"}, + }, + // FIXME(cian): we should skip evaluating tags for which values have already been provided. + { + name: "main.tf with variable missing default value but value is passed in request", + files: map[string]string{ + `main.tf`: ` + variable "a" { + type = string + } + data "coder_workspace_tags" "tags" { + tags = { + "a": var.a, + } + }`, + }, + reqTags: map[string]string{"a": "b"}, + // wantTags: map[string]string{"owner": "", "scope": "organization", "a": "b"}, + expectError: `provisioner tag "a" evaluated to an empty value`, + }, { name: "main.tf with disallowed workspace tag value", files: map[string]string{ diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index feb748ad29250..88599a807e262 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/propagation" + "golang.org/x/exp/slices" "golang.org/x/xerrors" "cdr.dev/slog" @@ -1421,6 +1422,47 @@ func TestWorkspaceBuildTimings(t *testing.T) { } }) + t.Run("MultipleTimingsForSameAgentScript", func(t *testing.T) { + t.Parallel() + + // Given: a build with multiple timings for the same script + build := makeBuild(t) + resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{ + JobID: build.JobID, + }) + agent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{ + ResourceID: resource.ID, + }) + script := dbgen.WorkspaceAgentScript(t, db, database.WorkspaceAgentScript{ + WorkspaceAgentID: agent.ID, + }) + timings := make([]database.WorkspaceAgentScriptTiming, 3) + scriptStartedAt := dbtime.Now() + for i := range timings { + timings[i] = dbgen.WorkspaceAgentScriptTiming(t, db, database.WorkspaceAgentScriptTiming{ + StartedAt: scriptStartedAt, + EndedAt: scriptStartedAt.Add(1 * time.Minute), + ScriptID: script.ID, + }) + + // Add an hour to the previous "started at" so we can + // reliably differentiate the scripts from each other. + scriptStartedAt = scriptStartedAt.Add(1 * time.Hour) + } + + // When: fetching timings for the build + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + t.Cleanup(cancel) + res, err := client.WorkspaceBuildTimings(ctx, build.ID) + require.NoError(t, err) + + // Then: return a response with the first agent script timing + require.Len(t, res.AgentScriptTimings, 1) + + require.Equal(t, timings[0].StartedAt.UnixMilli(), res.AgentScriptTimings[0].StartedAt.UnixMilli()) + require.Equal(t, timings[0].EndedAt.UnixMilli(), res.AgentScriptTimings[0].EndedAt.UnixMilli()) + }) + t.Run("AgentScriptTimings", func(t *testing.T) { t.Parallel() @@ -1432,10 +1474,10 @@ func TestWorkspaceBuildTimings(t *testing.T) { agent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{ ResourceID: resource.ID, }) - script := dbgen.WorkspaceAgentScript(t, db, database.WorkspaceAgentScript{ + scripts := dbgen.WorkspaceAgentScripts(t, db, 5, database.WorkspaceAgentScript{ WorkspaceAgentID: agent.ID, }) - agentScriptTimings := dbgen.WorkspaceAgentScriptTimings(t, db, script, 5) + agentScriptTimings := dbgen.WorkspaceAgentScriptTimings(t, db, scripts) // When: fetching timings for the build ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) @@ -1445,6 +1487,12 @@ func TestWorkspaceBuildTimings(t *testing.T) { // Then: return a response with the expected timings require.Len(t, res.AgentScriptTimings, 5) + slices.SortFunc(res.AgentScriptTimings, func(a, b codersdk.AgentScriptTiming) int { + return a.StartedAt.Compare(b.StartedAt) + }) + slices.SortFunc(agentScriptTimings, func(a, b database.WorkspaceAgentScriptTiming) int { + return a.StartedAt.Compare(b.StartedAt) + }) for i := range res.AgentScriptTimings { timingRes := res.AgentScriptTimings[i] genTiming := agentScriptTimings[i] diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 6a2856dcbbe76..bc4cdab6b2b65 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -3757,10 +3757,10 @@ func TestWorkspaceTimings(t *testing.T) { agent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{ ResourceID: resource.ID, }) - script := dbgen.WorkspaceAgentScript(t, db, database.WorkspaceAgentScript{ + scripts := dbgen.WorkspaceAgentScripts(t, db, 3, database.WorkspaceAgentScript{ WorkspaceAgentID: agent.ID, }) - dbgen.WorkspaceAgentScriptTimings(t, db, script, 3) + dbgen.WorkspaceAgentScriptTimings(t, db, scripts) // When: fetching the timings ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) diff --git a/enterprise/coderd/gitsshkey_test.go b/enterprise/coderd/gitsshkey_test.go new file mode 100644 index 0000000000000..a4978ac8fdad3 --- /dev/null +++ b/enterprise/coderd/gitsshkey_test.go @@ -0,0 +1,81 @@ +package coderd_test + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/agentsdk" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/provisioner/echo" + "github.com/coder/coder/v2/testutil" +) + +// TestAgentGitSSHKeyCustomRoles tests that the agent can fetch its git ssh key when +// the user has a custom role in a second workspace. +func TestAgentGitSSHKeyCustomRoles(t *testing.T) { + t.Parallel() + + owner, _ := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + codersdk.FeatureMultipleOrganizations: 1, + codersdk.FeatureExternalProvisionerDaemons: 1, + }, + }, + }) + + // When custom roles exist in a second organization + org := coderdenttest.CreateOrganization(t, owner, coderdenttest.CreateOrganizationOptions{ + IncludeProvisionerDaemon: true, + }) + + ctx := testutil.Context(t, testutil.WaitShort) + //nolint:gocritic // required to make orgs + newRole, err := owner.CreateOrganizationRole(ctx, codersdk.Role{ + Name: "custom", + OrganizationID: org.ID.String(), + DisplayName: "", + SitePermissions: nil, + OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ + codersdk.ResourceTemplate: {codersdk.ActionRead, codersdk.ActionCreate, codersdk.ActionUpdate}, + }), + UserPermissions: nil, + }) + require.NoError(t, err) + + // Create the new user + client, _ := coderdtest.CreateAnotherUser(t, owner, org.ID, rbac.RoleIdentifier{Name: newRole.Name, OrganizationID: org.ID}) + + // Create the workspace + agent + authToken := uuid.NewString() + version := coderdtest.CreateTemplateVersion(t, client, org.ID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionPlan: echo.PlanComplete, + ProvisionApply: echo.ProvisionApplyWithAgent(authToken), + }) + project := coderdtest.CreateTemplate(t, client, org.ID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, project.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + agentClient := agentsdk.New(client.URL) + agentClient.SetSessionToken(authToken) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + agentKey, err := agentClient.GitSSHKey(ctx) + require.NoError(t, err) + require.NotEmpty(t, agentKey.PrivateKey) +} diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index cce93dcc3a8fc..d074444bee10a 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1217,7 +1217,8 @@ func TestWorkspaceTagsTerraform(t *testing.T) { createTemplateVersionRequestTags map[string]string // the coder_workspace_tags bit of main.tf. // you can add more stuff here if you need - tfWorkspaceTags string + tfWorkspaceTags string + skipCreateWorkspace bool }{ { name: "no tags", @@ -1304,8 +1305,8 @@ func TestWorkspaceTagsTerraform(t *testing.T) { }`, }, { - name: "does not override static tag", - provisionerTags: map[string]string{"foo": "bar"}, + name: "overrides static tag from request", + provisionerTags: map[string]string{"foo": "baz"}, createTemplateVersionRequestTags: map[string]string{"foo": "baz"}, tfWorkspaceTags: ` data "coder_workspace_tags" "tags" { @@ -1313,6 +1314,9 @@ func TestWorkspaceTagsTerraform(t *testing.T) { "foo" = "bar" } }`, + // When we go to create the workspace, there won't be any provisioner + // matching tag foo=bar. + skipCreateWorkspace: true, }, } { tc := tc @@ -1352,13 +1356,15 @@ func TestWorkspaceTagsTerraform(t *testing.T) { coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, tv.ID) tpl := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, tv.ID) - // Creating a workspace as a non-privileged user must succeed - ws, err := member.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ - TemplateID: tpl.ID, - Name: coderdtest.RandomUsername(t), - }) - require.NoError(t, err, "failed to create workspace") - coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, ws.LatestBuild.ID) + if !tc.skipCreateWorkspace { + // Creating a workspace as a non-privileged user must succeed + ws, err := member.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ + TemplateID: tpl.ID, + Name: coderdtest.RandomUsername(t), + }) + require.NoError(t, err, "failed to create workspace") + coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, ws.LatestBuild.ID) + } }) } } diff --git a/examples/templates/docker/main.tf b/examples/templates/docker/main.tf index 9359a44c75773..c496e3892cc0b 100644 --- a/examples/templates/docker/main.tf +++ b/examples/templates/docker/main.tf @@ -184,7 +184,7 @@ resource "docker_container" "workspace" { ip = "host-gateway" } volumes { - container_path = "/home/${local.username}" + container_path = "/home/coder" volume_name = docker_volume.home_volume.name read_only = false }