- Notifications
You must be signed in to change notification settings - Fork1.1k
chore: add dbauthz to unhanger tests#14394
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
spikecurtis commentedAug 22, 2024
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
196399c to3a2c955Compare
Emyrk left a comment
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.
Minor nits, not blocking
Uh oh!
There was an error while loading.Please reload this page.
| ) | ||
| detector:=unhanger.New(ctx,db,pubsub,log,tickCh).WithStatsChannel(statsCh) | ||
| detector:=unhanger.New(ctx,wrapDBAuthz(db,log),pubsub,log,tickCh).WithStatsChannel(statsCh) |
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.
Feels odd to have a rawdb laying around outside this function call scope. I'd probably do this to not have adb laying around. Up to you 🤷♂️
var (ctx=testutil.Context(t,testutil.WaitLong)rdb,pubsub=dbtestutil.NewDB(t)db=wrapDBAuthz(rdb,slogtest.Make(t,nil))log=slogtest.Make(t,nil)tickCh=make(chan time.Time)statsCh=make(chan unhanger.Stats))
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.
The annoying thing about that is that I'd have to set up a dbauthz principal for the calls that write the job, user, template, etc. Authz for those operations is irrelevant to this test.
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.
Ah, yea it is annoying.dbgen gets around that with agenCtx:
coder/coderd/database/dbgen/dbgen.go
Line 35 in40bf367
| vargenCtx=dbauthz.As(context.Background(), rbac.Subject{ |
If you use dbgen to handle all the initial state, you can ignore that requirement.
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.
Almost, but not quite all state isdbgen. It make sense to me to keep this way, since it really is just the unhanger under test here, so only it need the dbauthz.
3a2c955 tob4ba388Compareb4ba388 to4ce4adaComparespikecurtis commentedAug 22, 2024
Merge activity
|
Uh oh!
There was an error while loading.Please reload this page.
relates to#14393
customer is having issues with the unhanger failing to kill a hung build. I noticed that our unhanger unit tests don't actually use the dbauthz. This PR adds authz to the unhanger tests.
Many tests failed on template import and template dry-run jobs, because the test code inserts build jobs without a corresponding template version, and our authz layer uses permission on the template version as a proxy for permission on the job.
I'mpretty sure that our actual API calls ensure that there is always a template version for the the job, but for some future work, it would be nice to have the database enforce this.