- Notifications
You must be signed in to change notification settings - Fork1k
fix: add constraint and runtime check for provisioner logs size limit#18893
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
fix: add constraint and runtime check for provisioner logs size limit#18893
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| err=s.Database.UpdateProvisionerJobLogsLength(ctx, database.UpdateProvisionerJobLogsLengthParams{ | ||
| ID:parsedID, | ||
| LogsLength:int32(newLogSize),// #nosec G115 - Log output length is limited to 1MB (2^20) which fits in an int32. | ||
| }) | ||
| iferr!=nil { | ||
| ifdatabase.IsProvisionerJobLogsLimitError(err) { | ||
| err=s.Database.UpdateProvisionerJobLogsOverflowed(ctx, database.UpdateProvisionerJobLogsOverflowedParams{ | ||
| ID:parsedID, | ||
| LogsOverflowed:true, | ||
| }) | ||
| iferr!=nil { | ||
| s.Logger.Error(ctx,"failed to set logs overflowed flag",slog.F("job_id",parsedID),slog.Error(err)) | ||
| } | ||
| return&proto.UpdateJobResponse{ | ||
| Canceled:job.CanceledAt.Valid, | ||
| },nil | ||
| } | ||
| s.Logger.Error(ctx,"failed to update logs length",slog.F("job_id",parsedID),slog.Error(err)) | ||
| returnnil,xerrors.Errorf("update logs length: %w",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.
I think it might be a good idea to store the current logs length and overflow state as variables in this Go code. With that change we would update the length var when anInsert call is successful, and we can determine prior to insertion whether the log we want to insert will cause the overflow. That way we can avoid the extras.Database.UpdateProvisionerJobLogsLength call on every log line after an overflow has occurred.
coderabbitaibot commentedJul 21, 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.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| id> @created_after | ||
| )ORDER BY idASC; | ||
| -- name: GetProvisionerJobLogSize :one |
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 can remove this query now I think.
| level:"error", | ||
| output:"Startup logs exceeded the max size of 1MB!", | ||
| output: | ||
| "Startup logs exceeded the max size of 1MB, and will not continue to be written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.", |
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.
should the other places for the logging also contain the more descriptive message, inWorkspaceBuildLogs.tsx andWorkspaceBuildPageView.tsx?
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.
Yeah I think so :)
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.
Approving, just one bit I'm unsure about in the rbac stuff.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| func (q*querier)UpdateProvisionerJobLogsLength(ctx context.Context,arg database.UpdateProvisionerJobLogsLengthParams)error { | ||
| // Not sure what the rbac should be here, going with this for now |
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 only bit I'm unsure of, maybe include the same comment as the other provisioner related functions have?
// TODO: Remove this once we have a proper rbac check for provisioner jobs.// Details in https://github.com/coder/coder/issues/16160or ask@mafredri since he opened that issue?
bcpeinhardtJul 30, 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.
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.
Our manual testing was actually from the admin account, we should look at this again before merging I think.
Edit: I tested with a member account and everything works fine. I could be way off here but I think the action.Update on the ResourceProvisionerJob is coming from the template admin?
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 are several other queries which perform the same rbac check we do to update fields on the ProvisionerJobs table, so I feel comfortable merging this and revisiting the provisioner jobs rbac in the future.
e4dc2d9 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR sets a constraint of 1MB on the provisioner job logs written to the database. This is consistent with the constraint we place on workspace agent logs:
coder/coderd/database/dump.sql
Line 2030 in4ac6be6
It also adds a message printed to the front end about the provisioner log overflow, and updates the message printed to the front end when workspace startup logs exceed the max, as it was causing some customers to think their startup script had failed to run.
Closes#17992