- Notifications
You must be signed in to change notification settings - Fork1.1k
feat(coderd): add endpoint to list provisioner jobs#16029
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
664d9b7 to77717c6Compare346ca29 toebaf498Compare77717c6 to90a610aCompareed828f6 to8bee6f5Compareacee984 toad6da75Compare8bee6f5 tod76be47Comparemafredri commentedJan 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Would appreciate some rbac/dbauthz pointers here: I'm actually not sure what we should enforce here. |
| OrganizationID uuid.UUID`json:"organization_id" format:"uuid" table:"organization id"` | ||
| InputProvisionerJobInput`json:"input" table:"input,recursive_inline"` | ||
| TypeProvisionerJobType`json:"type" table:"type"` | ||
| AvailableWorkers []uuid.UUID`json:"available_workers,omitempty" format:"uuid" table:"available workers"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thinking about future use cases, should we transform worker UUIDs into structures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair. You might be able to add some stats about them. Like their tags? Idk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/database/modelmethods.go Outdated
| varinput codersdk.ProvisionerJobInput | ||
| _=json.Unmarshal(p.Input,&input)// Best effort. | ||
| // TODO(mafredri): Do we need to check provisioner permissions as well (p.AvailableProvisioners?). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 provisionerjobs, the answer for 1) should probably be no, as there may be inputs to provisioners that they're not meant to see.
For provisionerdaemons, I don't think there's as much of a potential for information leakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
Lines 418 to 420 in12991ff
| // 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just realized the premise for thisRBACObject is a bit broken. Essentially we would need access to template ID or workspace ID to check user-level permissions, but we only have templateversion ID and workspacebuild 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 customdatabase.ProvisionerJobWithRBAC or something where we have/join in the required IDs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mafredri it is a limitation that we currently have.
template_version inherits the permissions from thetemplate. We do not have a resource for the version. The same is true for theworkspace_build. So you have to fetch the related resource to check the permissions.
To check atemplate_importprovisioner_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 newVIEW. Right now we just pay the round trip costs of multiple fetches 😢. I'd support a newVIEW that joins in the right resource IDs if we need it for listing purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 isGetProvisionerJobByID fetches workspace/template based on job type, but this one wasn't updated to take into account the newrbac.ResourceProvisionerJobs which would break existing functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| // owner := coderdtest.CreateFirstUser(t, client) | ||
| // _, memberUser := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) | ||
| db,ps:=dbtestutil.NewDB(t,dbtestutil.WithDumpOnFailure()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Disgusting idea: usedbtestutil.NewDBWithSQLDB and just insert the raw rows you want 😅
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| OrganizationID uuid.UUID`json:"organization_id" format:"uuid" table:"organization id"` | ||
| InputProvisionerJobInput`json:"input" table:"input,recursive_inline"` | ||
| TypeProvisionerJobType`json:"type" table:"type"` | ||
| AvailableWorkers []uuid.UUID`json:"available_workers,omitempty" format:"uuid" table:"available workers"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair. You might be able to add some stats about them. Like their tags? Idk
d76be47 to0383f34Comparead6da75 toe4b4e2fCompare0383f34 tobe53b25Compared967f4b tob1fbc82Compare9d3635f to0bf651bComparea37b1bd toc924457Comparef7b1083 tofb4c933Comparec924457 to9525abdComparefb4c933 toaed586bCompare9525abd to51f628cCompareaed586b to2ad4bb4Compare51f628c to1b40a44Compare1b40a44 tob6eb60dCompareb6eb60d to8d5fca2Compared0477c6 todd49c08Comparedd49c08 to0fd9aa7Compare
mtojek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe it is in a mergeable state now. Good job!
johnstcn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work! 👍
| vardetailstring | ||
| ife.Err!=nil { | ||
| detail=": "+e.Err.Error() | ||
| } | ||
| return"unauthorized"+detail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice 👍
| // @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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit, non-blocking:organizationProvisionerJobs perhaps?
| if!api.Authorize(r,policy.ActionRead,rbac.ResourceProvisionerJobs.InOrg(org.ID)) { | ||
| httpapi.ResourceNotFound(rw) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 This is fine for the moment.
3864c7e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Stack:
feat(coderd): add endpoint to list provisioner jobs
Closes#15190
Updates#15084
Supercedes#15940