- Notifications
You must be signed in to change notification settings - Fork927
feat: Implement view for workspace builds to include rbac info#6371
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
Removes the need to fetch the workspace to run an rbac check.
| func (q *querier) GetLatestWorkspaceBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) (database.WorkspaceBuild, error) { | ||
| if _, err := q.GetWorkspaceByID(ctx, workspaceID); err != nil { | ||
| return database.WorkspaceBuild{}, err | ||
| } | ||
| return q.db.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID) | ||
| return fetch(q.log, q.auth, q.db.GetLatestWorkspaceBuildByWorkspaceID)(ctx, workspaceID) | ||
| } |
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 the diff we are going for. Before we had to do a workspace fetch, now we do not.
This happens for a lot of objects (template versions, provisoner jobs, params, etc).workspace_builds is just the first I am doing as an example. If this pattern works, I will continue to work on others.
Are there any workarounds to get VSCode to highlight the files nicely? Like maybe naming the template file |
Or perhaps we could update the workspace config file to add a custom syntax highlighting grammar or something? |
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.
It'd be nice if the full object with the extra RBAC-required fields wasn't returned from the querier at all (to avoid having two objects), but I understand that is difficult because of sqlc. The templating approach seems better in that regard, but it definitely needs to have syntax highlighting to work.
| w, err := q.db.GetWorkspaceByID(ctx, arg.WorkspaceID) | ||
| if err != nil { | ||
| return database.WorkspaceBuild{}, err | ||
| return database.WorkspaceBuildThin{}, err | ||
| } |
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.
Since you still have the workspace here, this doesn't need to be thin right? Or is it a requirement because of the sqlc generating the interface?
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.
It's because of SQLc interface
It is actually because we use Now one way around this is to leave the sqlc interface when using coder/coderd/database/dbauthz/querier.go Lines 1336 to 1352 inc3218f7
This is because rbac requires a fetch before we insert/update anyway. |
I tried for a long time. I can make it |
Going with#6429 |
Uh oh!
There was an error while loading.Please reload this page.
Using Go templates instead:#6429
What is this
Creates a view for workspace builds to add RBAC information to the object directly, removing the need for an extra database query.
This does create 2
WorkspaceBuildobjects. One isWorkspaceBuildand one isWorkspacBuildThin. The thin version is returned by inserts/updates because you cannot insert into a view. I just expand the returned value immediately:coder/coderd/workspacebuilds.go
Lines 604 to 605 in0b98766
To return
WorkspaceBuildwith joined fields, it gets complicated with either a postgres function or rule.Why do this?
Currently workspace_builds require fetching the related workspace to run RBAC checks. This requires an extra DB round trip. Instead, we should join this information to the build.
Cost
The cost of a view is using migrations to maintain the view. If a column is added to
workspace_buildstable, we need to update the view.Alternative (please read this part!)
I was originally removing SQLc and just using SQLx directly. To reuse join information, I was going to usego templates. This required a bit of infrastructure topull the queries into Golang. The final interface for this little package wasquite clean.
Downsides
The go templates do not have great syntax highlighting support. On Goland, you can get apretty good experience, but not on vs-code which a lot of our team uses.
The go template syntax highlighting breaks if you use go templates for dynamic queries, which is the alternative to using a view. This means making this a template looks like this.
{{ define"workspace_builds_rbac" }}(SELECTworkspace_builds.*,workspaces.organization_idAS organization_id,workspaces.owner_idAS workspace_owner_idFROMworkspace_buildsINNER JOINworkspacesONworkspace_builds.workspace_id=workspaces.id){{ end }}-- To use the template above{{ define"GetWorkspaceBuildByID" }}SELECT*FROM {{ template"workspace_builds_rbac" }}WHEREid= @build_id{{ end }}If this is preferred, I can go back to the sqlx route.
Upsides
CASE WHENcan be removed into go template conditionals that conditionally write additional WHERE clauses.