- Notifications
You must be signed in to change notification settings - Fork927
fix(cli): add check for DisableOwnerWorkspaceExec in scaletest#14417
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
| func TestScaleTestWorkspaceTraffic_UseHostLogin(t *testing.T) { | ||
| ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitMedium) | ||
| defer cancelFunc() | ||
| log := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) | ||
| client := coderdtest.New(t, &coderdtest.Options{ | ||
| Logger: &log, | ||
| IncludeProvisionerDaemon: true, | ||
| DeploymentValues: coderdtest.DeploymentValues(t, func(dv *codersdk.DeploymentValues) { | ||
| dv.DisableOwnerWorkspaceExec = true | ||
| }), | ||
| }) |
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.
Does this test not run ingo tests?
I ask because these roles are global, and it is an unfortunate consequence atm. (maybe custom roles can solve this without being global in the future).
You can see how I reset the global state here when using this option:
coder/coderd/rbac/roles_test.go
Lines 63 to 66 ine6ed3c3
| rbac.ReloadBuiltinRoles(&rbac.RoleOptions{ | |
| NoOwnerWorkspaceExec:true, | |
| }) | |
| t.Cleanup(func() {rbac.ReloadBuiltinRoles(nil) }) |
and of course, not.Parallel()
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.
As far as I can tell, each package is its own test binary. So the global variable would only affect tests run in the same package.
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 added arequire.FailNow() and validated that the test does indeed get run.
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.
CI is passing, so the global-ness is not impacting the rest 🤔
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, Cian is right, you can think of each package as being built as its own test binary (basically that's what actually happens too), so global scope changes are constrained to that package. This is one of the test parallelism mechanisms in Go too (run different packages concurrently) and works irrespective oft.Parallel().
coderd/coderdtest/coderdtest.go Outdated
| if err := options.Authorizer.Authorize(context.Background(), ownerSubj, policy.ActionSSH, rbac.ResourceWorkspace); err != nil { | ||
| if rbac.IsUnauthorizedError(err) { | ||
| t.Fatal("DisableOwnerWorkspaceExec was set in some other test. Please move this test to its own package so that it does not impact other tests!") | ||
| } | ||
| require.NoError(t, 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.
This is a great check 👍
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 since thecoderdtest now asserts the global is correct and CI is passing.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
6914862 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
--use-host-logintocoder exp scaletest workspace-trafficgetScaletestWorkspacesto conditionally filter workspaces ifCODER_DISABLE_OWNER_WORKSPACE_ACCESSis setCODER_DISABLE_OWNER_WORKSPACE_ACCESSis set and scaletest workspaces are filtered out due to ownership mismatch.coderdtest.Newto detect cross-test bleed ofCODER_DISABLE_OWNER_WORKSPACE_ACCESSand fast-fail.