Audit build outcomes/kira pilot #5143
Merged
Conversation
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
coadler
approved these changes
Nov 21, 2022
coderd/audit.go
Outdated
Comment on lines
193
to
197
| var agent string | ||
|
|
||
| if dblog.UserAgent.Valid { | ||
| agent = dblog.UserAgent.String | ||
| } |
There was a problem hiding this comment.
I don't think this should be necessary. You can access dblog.UserAgent.String safely without checking if it's valid, it'll just be an empty string, so both outcomes are essentially the same.
| @@ -967,6 +1035,19 @@ func convertWorkspaceTransition(transition database.WorkspaceTransition) (sdkpro | |||
| } | |||
| } | |||
|
|
|||
| func determineAuditAction(transition database.WorkspaceTransition) database.AuditAction { | |||
There was a problem hiding this comment.
I think auditActionFromTransition would make this more clearer.
| JobID: job.ID, | ||
| Action: auditAction, | ||
| New: workspaceBuild, | ||
| Status: 200, |
There was a problem hiding this comment.
Suggested change
| Status: 200, | |
| Status: http.StatusOK, |
|
|
||
| wriBytes, err := json.Marshal(workspaceResourceInfo) | ||
| if err != nil { | ||
| server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) |
There was a problem hiding this comment.
Suggested change
| server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) | |
| server.Logger.Error(ctx, "marshal resource info", slog.Error(err)) |
| @@ -596,11 +635,12 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete | |||
| return nil, xerrors.Errorf("get workspace build: %w", err) | |||
| } | |||
|
|
|||
| workspace, getWorkspaceError := server.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) | |||
There was a problem hiding this comment.
I think this GetWorkspaceByID needs to stay in the transaction, otherwise it's unsafe to update it. You could make it assign to a variable outside of the function to access the info!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
resolves #4724
Previously, all
workspace_buildaudit logs showed 201 status codes - regardless of success or failure. This was because we were logging the initiation of the request - but not the outcome.We are now logging the outcome (and have stopped logging the initiation for now). If a build fails, the audit log will reflect that failure (status code
500). If it is successful, the audit log will reflect that success (status code200). The tradeoff is that there may be a delay in log generation as we have to wait for the build to finish. There are no diffs forworkspace_buildaudit logs. I will add links to build logs in a fast follow to this PR so users can investigate failures.