Skip to content

feat: Enable workspace debug logging #6838

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Mar 30, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Mar 28, 2023

Related: #6186
Related: #4767

This PR enables workspace debug logging on the backend side.

TODO:

  • RBAC: make sure that only the template author is authorized to run debug builds
  • unit tests

Tested:

curl 'http://localhost:3000/api/v2/workspaces/cd44df27-c7c0-4fa3-aa65-9483160cb2c7/builds' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: pl,en-US;q=0.9,en;q=0.8,pl-PL;q=0.7' \
...
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw '{"transition":"start","log_level":"debug","rich_parameter_values":[{"name":"Security Groups 2","value":"[\"Web Server Security Group\",\"Database Security Group\",\"Backend Security Group\"]"}]}' \
  --compressed

mind the "log_level":"debug" parameter

Follow-up:

  • site UI changes (a button?) + authcheck (only template authors can trigger debug builds)

@mtojek mtojek self-assigned this Mar 28, 2023
@mtojek mtojek marked this pull request as ready for review March 29, 2023 11:16
@BrunoQuaresma BrunoQuaresma requested a review from johnstcn March 29, 2023 18:10
t.Parallel()

client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: instead of running this as the "admin" user, we should instead do as above and create another user with template admin permissions. This would more properly test that the template author has the correct permissions to create this sort of job.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the hint! Refactored.

@@ -59,6 +65,8 @@ type CreateWorkspaceBuildRequest struct {
// This will not delete old params not included in this list.
ParameterValues []CreateParameterRequest `json:"parameter_values,omitempty"`
RichParameterValues []WorkspaceBuildParameter `json:"rich_parameter_values,omitempty"`

LogLevel ProvisionerLogLevel `json:"log_level,omitempty" validate:"omitempty,oneof=debug"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Add a comment so this gets documented in the API doc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

return response, true
}

provisionerLogLevel := proto.LogLevel_value[strings.ToUpper(config.ProvisionerLogLevel)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid having to upper/lowercase this? Not blocking, just curious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that the only way to do this is to switch to log_level: DEBUG, so that it's upper-case everywhere. Not sure if it's worth it.

Comment on lines 1254 to 1267
for {
log, ok := <-logs
if !ok {
break
}

if log.Output == "dont-want-it" {
require.Failf(t, "unexpected log message", "%s log message shouldn't be logged: %s", log.Level, log.Output)
}

if log.Output == "done" {
return
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: although it's in a naked for, this shouldn't hang forever as WorkspaceBuildLogsAfter exits on <-ctx.Done()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Refactored.

@mtojek mtojek merged commit 0ba200c into coder:main Mar 30, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants