- Notifications
You must be signed in to change notification settings - Fork1k
feat(coderd): add matched provisioner daemons information to more places#15688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes from1 commit
Commits
Show all changes
16 commits Select commitHold shift + click to select a range
ba944ab feat(coderd/wsbuilder): return provisioners available at time of insert
johnstcn4e51f20 feat(coderd): add matched provisioner daemons information to addition…
johnstcn47036e8 fix linter complaint re nil ptr deref
johnstcn16be03b add test coverage for matched provisioners with workspace build creation
johnstcn4304a06 skip for non-postgres
johnstcn9ef68dd add tests for workspace creation
johnstcn38788d5 revert fe changes in this pr
johnstcn1c95ffe coderd/wsbuilder: use dbauthz.AsSystemReadProvisionerDaemons instead …
johnstcn98521be refactor: extract WarnMatchedProvisioners to cliutil
johnstcne1423f5 fixup! refactor: extract WarnMatchedProvisioners to cliutil
johnstcn517a505 feat(cli): add provisioner warning to create/start/stop commands
johnstcnc4295ef Apply suggestions from code review
johnstcnc5fb83b feat(coderd): add endpoint for matched provisioners of template versi…
johnstcn3bd62fd feat(cli): delete: warn on no matched provisioners
johnstcn2f625bc address linter complaint
johnstcn848338e add test for cli/delete
johnstcnFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
feat(coderd): add matched provisioner daemons information to addition…
…al template-related endpoints
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
commit4e51f2069e0b7a61743a259a8b9ac00f8dfa887f
There are no files selected for viewing
23 changes: 23 additions & 0 deletionscoderd/database/dbauthz/dbauthz.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -317,6 +317,23 @@ var ( | ||
| }), | ||
| Scope: rbac.ScopeAll, | ||
| }.WithCachedASTValue() | ||
| subjectSystemReadProvisionerDaemons = rbac.Subject{ | ||
| FriendlyName: "System", | ||
johnstcn marked this conversation as resolved. OutdatedShow resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| ID: uuid.Nil.String(), | ||
| Roles: rbac.Roles([]rbac.Role{ | ||
| { | ||
| Identifier: rbac.RoleIdentifier{Name: "system-read-provisioner-daemons"}, | ||
| DisplayName: "Coder", | ||
| Site: rbac.Permissions(map[string][]policy.Action{ | ||
| rbac.ResourceProvisionerDaemon.Type: {policy.ActionRead}, | ||
| }), | ||
| Org: map[string][]rbac.Permission{}, | ||
| User: []rbac.Permission{}, | ||
| }, | ||
| }), | ||
| Scope: rbac.ScopeAll, | ||
| }.WithCachedASTValue() | ||
| ) | ||
| // AsProvisionerd returns a context with an actor that has permissions required | ||
| @@ -359,6 +376,12 @@ func AsSystemRestricted(ctx context.Context) context.Context { | ||
| return context.WithValue(ctx, authContextKey{}, subjectSystemRestricted) | ||
| } | ||
| // AsSystemReadProvisionerDaemons returns a context with an actor that has permissions | ||
| // to read provisioner daemons. | ||
| func AsSystemReadProvisionerDaemons(ctx context.Context) context.Context { | ||
| return context.WithValue(ctx, authContextKey{}, subjectSystemReadProvisionerDaemons) | ||
| } | ||
Comment on lines +379 to +384 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. self-review: I wanted to avoid sprinkling | ||
| var AsRemoveActor = rbac.Subject{ | ||
| ID: "remove-actor", | ||
| } | ||
96 changes: 90 additions & 6 deletionscoderd/templateversions.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -22,6 +22,7 @@ import ( | ||
| "github.com/coder/coder/v2/coderd/audit" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| "github.com/coder/coder/v2/coderd/database/db2sdk" | ||
| "github.com/coder/coder/v2/coderd/database/dbauthz" | ||
| "github.com/coder/coder/v2/coderd/database/dbtime" | ||
| "github.com/coder/coder/v2/coderd/database/provisionerjobs" | ||
| "github.com/coder/coder/v2/coderd/externalauth" | ||
| @@ -32,6 +33,7 @@ import ( | ||
| "github.com/coder/coder/v2/coderd/rbac/policy" | ||
| "github.com/coder/coder/v2/coderd/render" | ||
| "github.com/coder/coder/v2/coderd/tracing" | ||
| "github.com/coder/coder/v2/coderd/util/ptr" | ||
| "github.com/coder/coder/v2/codersdk" | ||
| "github.com/coder/coder/v2/examples" | ||
| "github.com/coder/coder/v2/provisioner/terraform/tfparse" | ||
| @@ -60,6 +62,22 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| var matchedProvisioners *codersdk.MatchedProvisioners | ||
| if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Do we only ever expect a single job to be returned by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. In this instance, we should get either 0 or 1: Contributor
| ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: jobs[0].ProvisionerJob.OrganizationID, | ||
| WantTags: jobs[0].ProvisionerJob.Tags, | ||
| }) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err)) | ||
| } else { | ||
| matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval)) | ||
| } | ||
| } | ||
| schemas, err := api.Database.GetParameterSchemasByJobID(ctx, jobs[0].ProvisionerJob.ID) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| err = nil | ||
| @@ -77,7 +95,7 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { | ||
| warnings = append(warnings, codersdk.TemplateVersionWarningUnsupportedWorkspaces) | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]),matchedProvisioners, warnings)) | ||
| } | ||
| // @Summary Patch template version by ID | ||
| @@ -173,7 +191,23 @@ func (api *API) patchTemplateVersion(rw http.ResponseWriter, r *http.Request) { | ||
| return | ||
| } | ||
| var matchedProvisioners *codersdk.MatchedProvisioners | ||
| if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending { | ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: jobs[0].ProvisionerJob.OrganizationID, | ||
| WantTags: jobs[0].ProvisionerJob.Tags, | ||
| }) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err)) | ||
| } else { | ||
| matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval)) | ||
| } | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(updatedTemplateVersion, convertProvisionerJob(jobs[0]), matchedProvisioners, nil)) | ||
| } | ||
| // @Summary Cancel template version by ID | ||
| @@ -868,8 +902,23 @@ func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) { | ||
| }) | ||
| return | ||
| } | ||
| var matchedProvisioners *codersdk.MatchedProvisioners | ||
| if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending { | ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: jobs[0].ProvisionerJob.OrganizationID, | ||
| WantTags: jobs[0].ProvisionerJob.Tags, | ||
| }) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err)) | ||
| } else { | ||
| matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval)) | ||
| } | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]),matchedProvisioners, nil)) | ||
| } | ||
| // @Summary Get template version by organization, template, and name | ||
| @@ -934,7 +983,23 @@ func (api *API) templateVersionByOrganizationTemplateAndName(rw http.ResponseWri | ||
| return | ||
| } | ||
| var matchedProvisioners *codersdk.MatchedProvisioners | ||
| if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending { | ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: jobs[0].ProvisionerJob.OrganizationID, | ||
| WantTags: jobs[0].ProvisionerJob.Tags, | ||
| }) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err)) | ||
| } else { | ||
| matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval)) | ||
| } | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), matchedProvisioners, nil)) | ||
Comment on lines +1023 to +1039 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. self-review: This addresses an issue I noticed in the frontend where it quickly "flashes" with the tag warning message but then gets overwritten when the FE re-requests the template version. Adding it to other template-related endpoints for posterity. | ||
| } | ||
| // @Summary Get previous template version by organization, template, and name | ||
| @@ -1020,7 +1085,23 @@ func (api *API) previousTemplateVersionByOrganizationTemplateAndName(rw http.Res | ||
| return | ||
| } | ||
| var matchedProvisioners *codersdk.MatchedProvisioners | ||
| if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending { | ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: jobs[0].ProvisionerJob.OrganizationID, | ||
| WantTags: jobs[0].ProvisionerJob.Tags, | ||
| }) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err)) | ||
| } else { | ||
| matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval)) | ||
| } | ||
| } | ||
| httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(previousTemplateVersion, convertProvisionerJob(jobs[0]), matchedProvisioners, nil)) | ||
| } | ||
| // @Summary Archive template unused versions by template id | ||
| @@ -1540,7 +1621,10 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht | ||
| // Check for eligible provisioners. This allows us to return a warning to the user if they | ||
| // submit a job for which no provisioner is available. | ||
| // nolint: gocritic // The user hitting this endpoint may not have | ||
| // permission to read provisioner daemons, but we want to show them | ||
| // information about the provisioner daemons that are available. | ||
| eligibleProvisioners, err := tx.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{ | ||
| OrganizationID: organization.ID, | ||
| WantTags: provisionerJob.Tags, | ||
| }) | ||
29 changes: 27 additions & 2 deletionscoderd/templateversions_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -50,6 +50,12 @@ func TestTemplateVersion(t *testing.T) { | ||
| tv, err := client.TemplateVersion(ctx, version.ID) | ||
| authz.AssertChecked(t, policy.ActionRead, tv) | ||
| require.NoError(t, err) | ||
| if assert.Equal(t, tv.Job.Status, codersdk.ProvisionerJobPending) { | ||
| assert.NotNil(t, tv.MatchedProvisioners) | ||
| assert.Zero(t, tv.MatchedProvisioners.Available) | ||
| assert.Zero(t, tv.MatchedProvisioners.Count) | ||
| assert.False(t, tv.MatchedProvisioners.MostRecentlySeen.Valid) | ||
| } | ||
Comment on lines +53 to +58 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. zero because includeProvisionerDaemon is not set | ||
| assert.Equal(t, "bananas", tv.Name) | ||
| assert.Equal(t, "first try", tv.Message) | ||
| @@ -87,8 +93,14 @@ func TestTemplateVersion(t *testing.T) { | ||
| client1, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) | ||
| tv, err := client1.TemplateVersion(ctx, version.ID) | ||
| require.NoError(t, err) | ||
| if assert.Equal(t, tv.Job.Status, codersdk.ProvisionerJobPending) { | ||
| assert.NotNil(t, tv.MatchedProvisioners) | ||
| assert.Zero(t, tv.MatchedProvisioners.Available) | ||
| assert.Zero(t, tv.MatchedProvisioners.Count) | ||
| assert.False(t, tv.MatchedProvisioners.MostRecentlySeen.Valid) | ||
| } | ||
| }) | ||
| } | ||
| @@ -158,6 +170,12 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { | ||
| require.NoError(t, err) | ||
| require.Equal(t, "bananas", version.Name) | ||
| require.Equal(t, provisionersdk.ScopeOrganization, version.Job.Tags[provisionersdk.TagScope]) | ||
| if assert.Equal(t, version.Job.Status, codersdk.ProvisionerJobPending) { | ||
| assert.NotNil(t, version.MatchedProvisioners) | ||
| assert.Equal(t, version.MatchedProvisioners.Available, 1) | ||
| assert.Equal(t, version.MatchedProvisioners.Count, 1) | ||
| assert.True(t, version.MatchedProvisioners.MostRecentlySeen.Valid) | ||
| } | ||
Comment on lines +173 to +178 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. includeProvisionerDaemon is set here! | ||
| require.Len(t, auditor.AuditLogs(), 2) | ||
| assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs()[1].Action) | ||
| @@ -790,8 +808,15 @@ func TestTemplateVersionByName(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) | ||
| defer cancel() | ||
| tv, err := client.TemplateVersionByName(ctx, template.ID, version.Name) | ||
| require.NoError(t, err) | ||
| if assert.Equal(t, tv.Job.Status, codersdk.ProvisionerJobPending) { | ||
| assert.NotNil(t, tv.MatchedProvisioners) | ||
| assert.Zero(t, tv.MatchedProvisioners.Available) | ||
| assert.Zero(t, tv.MatchedProvisioners.Count) | ||
| assert.False(t, tv.MatchedProvisioners.MostRecentlySeen.Valid) | ||
| } | ||
| }) | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.