feat(coderd): add endpoint to list provisioner jobs #16029

mafredri · 2025-01-03T15:48:40Z

Stack:

feat(coderd): add endpoint to list provisioner jobs

Closes #15190
Updates #15084
Supercedes #15940

mafredri · 2025-01-03T17:09:09Z

Would appreciate some rbac/dbauthz pointers here:

=== FAIL: coderd/database/dbauthz TestMethodTestSuite/TestExtraMethods/GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner/NotAuthorized (0.00s)
    setup_test.go:250: 
        	Error Trace:	/home/runner/work/coder/coder/coderd/database/dbauthz/setup_test.go:250
        	            				/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.10.0/suite/suite.go:115
        	Error:      	An error is expected but got nil.
        	Test:       	TestMethodTestSuite/TestExtraMethods/GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner/NotAuthorized
        	Messages:   	error string should have a good message
    setup_test.go:251: 
        	Error Trace:	/home/runner/work/coder/coder/coderd/database/dbauthz/setup_test.go:251
        	            				/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.10.0/suite/suite.go:115
        	Error:      	An error is expected but got nil.
        	Test:       	TestMethodTestSuite/TestExtraMethods/GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner/NotAuthorized
        	Messages:   	method should an error with disallow authz
    setup_test.go:252: 
        	Error Trace:	/home/runner/work/coder/coder/coderd/database/dbauthz/setup_test.go:252
        	            				/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.10.0/suite/suite.go:115
        	Error:      	Should be in error chain:
        	            	expected: %!q(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
        	            	in chain: 
        	Test:       	TestMethodTestSuite/TestExtraMethods/GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner/NotAuthorized
        	Messages:   	error should be NotAuthorizedError

I'm actually not sure what we should enforce here.

mtojek · 2025-01-07T11:54:57Z

codersdk/provisionerdaemons.go

+	OrganizationID   uuid.UUID            `json:"organization_id" format:"uuid" table:"organization id"`
+	Input            ProvisionerJobInput  `json:"input" table:"input,recursive_inline"`
+	Type             ProvisionerJobType   `json:"type" table:"type"`
+	AvailableWorkers []uuid.UUID          `json:"available_workers,omitempty" format:"uuid" table:"available workers"`


Thinking about future use cases, should we transform worker UUIDs into structures?

Fair. You might be able to add some stats about them. Like their tags? Idk

The provisioner daemons endpoint (#16028) supports filtering by IDs, so you could request them from there. I don't think we should prematurely add data/structure until we know what/if it's needed.

coderd/util/slice/slice.go

coderd/provisionerjobs.go

johnstcn · 2025-01-07T12:20:27Z

coderd/database/modelmethods.go

+	var input codersdk.ProvisionerJobInput
+	_ = json.Unmarshal(p.Input, &input) // Best effort.
+
+	// TODO(mafredri): Do we need to check provisioner permissions as well (p.AvailableProvisioners?).


The only case I could imagine needing to check the permissions on the provisioner is if you are taking into account user-scoped provisioners {"scope": "user", "owner": "abc123..."}.

The questions I'd raise here are:

Do we want organization members to be able to view provisioners/jobs owned by other users?

Do we want organization members to be able to view provisioners/jobs in other organizations?

For provisioner jobs, the answer for 1) should probably be no, as there may be inputs to provisioners that they're not meant to see.
For provisioner daemons, I don't think there's as much of a potential for information leakage.

Do we want organization members to be able to view provisioners/jobs owned by other users?

Provisioners are owned by an organization. All org members can read all provisioners today:

coder/coderd/rbac/roles.go

Lines 418 to 420 in 12991ff

// All users can see the provisioner daemons for workspace

// creation.

ResourceProvisionerDaemon.Type: {policy.ActionRead},

We have organizations to isolate provisioners if needed. No more granularity is currently supported.

Seeing other jobs should not be allowed. Checking permission of a list of jobs is a slow process.

Do we want organization members to be able to view provisioners/jobs in other organizations?

no we do not.

I just realized the premise for this RBACObject is a bit broken. Essentially we would need access to template ID or workspace ID to check user-level permissions, but we only have template version ID and workspace build ID.

I don't think we want to introduce new RBAC resources for either of these so what would be a good way to solve it? Have a custom database.ProvisionerJobWithRBAC or something where we have/join in the required IDs?

@mafredri it is a limitation that we currently have.

template_version inherits the permissions from the template. We do not have a resource for the version. The same is true for the workspace_build. So you have to fetch the related resource to check the permissions.

To check a template_import provisioner_job, you need to fetch the template version, then the template, then check the perms.

This is done because if you add a new resource, there is no method today to add related permissions. Meaning, you cannot say "If you can read template A, then you can read version B and job C". So without the related permissions, we have to duplicate the perms consistently. Which can lead to bugs, so we just do the data relation fetched 🤷‍♂️

It is a limitation we have today, and I do not see it changing anytime soon.

or something where we have/join in the required IDs

We'd have to go this way. Maybe some new VIEW. Right now we just pay the round trip costs of multiple fetches 😢. I'd support a new VIEW that joins in the right resource IDs if we need it for listing purposes.

Thanks for the detailed info @Emyrk. For now I decided to limit jobs to owners and template admins as it simplifies the RBAC. I've opened #16160 to track future work improving this. Feel free to post on the issue if I've missed something.

// cc @johnstcn

I do not love leaving this open, but I get it.

As long as we don't break any existing APIs that might fetch a provisioner job today, I can be ok with it.

@Emyrk fair. Most of the existing APIs don't apply any RBAC at all AFAICT, the just call directly out to DB in dbauthz.

The exception is GetProvisionerJobByID fetches workspace/template based on job type, but this one wasn't updated to take into account the new rbac.ResourceProvisionerJobs which would break existing functionality.

Most of the existing APIs don't apply any RBAC at all AFAICT, the just call directly out to DB in dbauthz.

Yup, ideally all RBAC checks live in the dbauthz.

coderd/provisionerjobs.go

johnstcn · 2025-01-07T12:33:12Z

coderd/provisionerjobs_test.go

+	// owner := coderdtest.CreateFirstUser(t, client)
+	// _, memberUser := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
+
+	db, ps := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())


Disgusting idea: use dbtestutil.NewDBWithSQLDB and just insert the raw rows you want 😅

coderd/provisionerjobs.go

codersdk/provisionerdaemons.go

Emyrk · 2025-01-07T20:03:10Z

codersdk/provisionerdaemons.go

+	OrganizationID   uuid.UUID            `json:"organization_id" format:"uuid" table:"organization id"`
+	Input            ProvisionerJobInput  `json:"input" table:"input,recursive_inline"`
+	Type             ProvisionerJobType   `json:"type" table:"type"`
+	AvailableWorkers []uuid.UUID          `json:"available_workers,omitempty" format:"uuid" table:"available workers"`


Fair. You might be able to add some stats about them. Like their tags? Idk

Closes #15190 Updates #15084 Supercedes #15940

mtojek

I believe it is in a mergeable state now. Good job!

johnstcn

Nice work! 👍

johnstcn · 2025-01-17T10:13:52Z

coderd/database/dbauthz/dbauthz.go

+	var detail string
+	if e.Err != nil {
+		detail = ": " + e.Err.Error()
+	}
+	return "unauthorized" + detail


johnstcn · 2025-01-17T10:15:51Z

coderd/provisionerjobs.go

+// @Param status query codersdk.ProvisionerJobStatus false "Filter results by status" enums(pending,running,succeeded,canceling,canceled,failed)
+// @Success 200 {array} codersdk.ProvisionerJob
+// @Router /organizations/{organization}/provisionerjobs [get]
+func (api *API) provisionerJobs(rw http.ResponseWriter, r *http.Request) {


nit, non-blocking: organizationProvisionerJobs perhaps?

johnstcn · 2025-01-17T10:17:13Z

coderd/provisionerjobs.go

+	if !api.Authorize(r, policy.ActionRead, rbac.ResourceProvisionerJobs.InOrg(org.ID)) {
+		httpapi.ResourceNotFound(rw)
+		return
+	}


👍 This is fine for the moment.

mafredri mentioned this pull request Jan 3, 2025

feat(coderd): add endpoint to list provisioner daemons #16028

Merged

mafredri changed the title ~~feat(coderd): add endpoint to list provisioner jobs~~ [2/2] feat(coderd): add endpoint to list provisioner jobs Jan 3, 2025

mafredri changed the base branch from main to mafredri/feat-coderd-provisioner-list January 3, 2025 15:48

github-actions bot assigned mafredri Jan 3, 2025

mafredri changed the title ~~[2/2] feat(coderd): add endpoint to list provisioner jobs~~ [2/3] feat(coderd): add endpoint to list provisioner jobs Jan 3, 2025

mafredri mentioned this pull request Jan 3, 2025

feat(cli): add provisioner list and provisioner jobs list #16030

Merged

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch 2 times, most recently from 664d9b7 to 77717c6 Compare January 3, 2025 16:08

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from 346ca29 to ebaf498 Compare January 3, 2025 16:08

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from 77717c6 to 90a610a Compare January 3, 2025 16:13

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch 2 times, most recently from ed828f6 to 8bee6f5 Compare January 3, 2025 16:25

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch 2 times, most recently from acee984 to ad6da75 Compare January 3, 2025 16:54

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from 8bee6f5 to d76be47 Compare January 3, 2025 16:54

mafredri changed the title ~~[2/3] feat(coderd): add endpoint to list provisioner jobs~~ feat(coderd): add endpoint to list provisioner jobs Jan 3, 2025

mafredri requested review from Emyrk, johnstcn and mtojek January 7, 2025 09:03

mtojek reviewed Jan 7, 2025

View reviewed changes

johnstcn reviewed Jan 7, 2025

View reviewed changes

Emyrk reviewed Jan 7, 2025

View reviewed changes

mafredri mentioned this pull request Jan 13, 2025

feat: add provisioner daemon and jobs endpoints and commands #15940

Closed

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from d76be47 to 0383f34 Compare January 13, 2025 13:03

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from ad6da75 to e4b4e2f Compare January 13, 2025 13:03

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from 0383f34 to be53b25 Compare January 13, 2025 14:05

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch 2 times, most recently from d967f4b to b1fbc82 Compare January 13, 2025 15:32

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from 9d3635f to 0bf651b Compare January 13, 2025 15:35

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from a37b1bd to c924457 Compare January 14, 2025 14:41

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from f7b1083 to fb4c933 Compare January 14, 2025 14:42

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from c924457 to 9525abd Compare January 14, 2025 14:42

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from fb4c933 to aed586b Compare January 14, 2025 14:45

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from 9525abd to 51f628c Compare January 14, 2025 14:45

mafredri force-pushed the mafredri/feat-coderd-provisioner-list branch from aed586b to 2ad4bb4 Compare January 14, 2025 16:21

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from 51f628c to 1b40a44 Compare January 14, 2025 16:21

Base automatically changed from mafredri/feat-coderd-provisioner-list to main January 14, 2025 16:40

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from 1b40a44 to b6eb60d Compare January 14, 2025 17:32

mafredri added 6 commits January 15, 2025 10:12

feat(coderd): add endpoint to list provisioner jobs

9e72a30

Closes #15190 Updates #15084 Supercedes #15940

stringenums

48364ce

fix review comments

c0bb557

handle zero input in testing

fa1ee2a

dbauthz test fixes

342e27f

wip rbac stuff

8d5fca2

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from b6eb60d to 8d5fca2 Compare January 15, 2025 15:50

adjust provisioner jobs rbac for owner and template admin only

765c93a

mafredri mentioned this pull request Jan 16, 2025

Improve provisioner jobs RBAC with a PostgreSQL VIEW #16160

Open

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch 4 times, most recently from d0477c6 to dd49c08 Compare January 16, 2025 12:53

fix tests

0fd9aa7

mafredri force-pushed the mafredri/feat-coderd-provisioner-jobs-list branch from dd49c08 to 0fd9aa7 Compare January 16, 2025 12:54

fix dbmem test

2ecdf81

mtojek approved these changes Jan 17, 2025

View reviewed changes

johnstcn approved these changes Jan 17, 2025

View reviewed changes

mafredri merged commit 3864c7e into main Jan 20, 2025
36 checks passed

mafredri deleted the mafredri/feat-coderd-provisioner-jobs-list branch January 20, 2025 09:18

github-actions bot locked and limited conversation to collaborators Jan 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coderd): add endpoint to list provisioner jobs #16029

feat(coderd): add endpoint to list provisioner jobs #16029

mafredri commented Jan 3, 2025 •

edited

Loading

mafredri commented Jan 3, 2025 •

edited

Loading

mtojek Jan 7, 2025

Emyrk Jan 7, 2025

mafredri Jan 14, 2025

johnstcn Jan 7, 2025

Emyrk Jan 14, 2025

mafredri Jan 15, 2025 •

edited

Loading

Emyrk Jan 15, 2025

mafredri Jan 16, 2025

Emyrk Jan 16, 2025

mafredri Jan 16, 2025

Emyrk Jan 16, 2025

johnstcn Jan 7, 2025

Emyrk Jan 7, 2025

mtojek left a comment

johnstcn left a comment

johnstcn Jan 17, 2025

johnstcn Jan 17, 2025

johnstcn Jan 17, 2025

	// All users can see the provisioner daemons for workspace
	// creation.
	ResourceProvisionerDaemon.Type: {policy.ActionRead},

feat(coderd): add endpoint to list provisioner jobs #16029

feat(coderd): add endpoint to list provisioner jobs #16029

Conversation

mafredri commented Jan 3, 2025 • edited Loading

mafredri commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri commented Jan 3, 2025 •

edited

Loading

mafredri commented Jan 3, 2025 •

edited

Loading

mafredri Jan 15, 2025 •

edited

Loading