chore: convert dbauthz tests to also run with Postgres #15862
Conversation
|
I will take a look a this today. I really regret not making the DB a mock 😢. That would be the best, but fixing some ~400 tests to a mock sounds like the absolute worst thing. We might be able to leverage this coder/coderd/database/dbauthz/setup_test.go Lines 349 to 353 in e931831 |
There was a problem hiding this comment.
The tests now take ~18s on my machine. not that bad given there is 1973 test cases 🤯
If we remove the actual authorizer from the fake, then this LGTM 👍
At some flex week in the future, I'll look to removing the need for the DB and we can remove the new query added to remove foreign keys.
coderd/coderdtest/authorize.go
Outdated
| RegoAuthorizer *rbac.RegoAuthorizer | ||
| sqlFilter string |
There was a problem hiding this comment.
Do we include RegoAuthorizer only to get the CompileToSQL function?
I ask because the FakeAuthorizer should really not use the rego authorizer at all.
| t1 := dbgen.Template(s.T(), db, database.Template{}) | ||
| check.Args(database.UpdateWorkspacesTTLByTemplateIDParams{ | ||
| TemplateID: t1.ID, | ||
| }).Asserts(t1, policy.ActionUpdate) | ||
| })) | ||
| s.Run("UpdateTemplateActiveVersionByID", s.Subtest(func(db database.Store, check *expects) { | ||
| dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) |
There was a problem hiding this comment.
We do have this for some common cases:
tv := dbfake.TemplateVersion(s.T(), db).Do()
check.Args(database.UpdateTemplateActiveVersionByIDParams{
ID: tv.Template.ID,
ActiveVersionID: tv.TemplateVersion.ID,
}).Asserts(tv.Template, policy.ActionUpdate).Returns()There was a problem hiding this comment.
Oh nice, I wasn't aware. It's going to come in handy in future conversions. Since you'll be working on removing the need for the DB altogether, I suppose it's fine if we leave the code as is without converting to use dbfake?
There was a problem hiding this comment.
@hugodutka yea, it is fine 👍
Mainly just showing dbfake is a nice spot for dbgen "tooling"
coderd/coderdtest/authorize.go
Outdated
| func (*fakePreparedAuthorizer) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) { | ||
| return "not a valid sql string", nil | ||
| func (f *fakePreparedAuthorizer) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) { | ||
| if f.Original.sqlFilter != "" { | ||
| return f.Original.sqlFilter, nil | ||
| } | ||
| return f.PreparedRegoAuthorizer.CompileToSQL(ctx, cfg) |
There was a problem hiding this comment.
I'd remove the real rego authorizer and just always return true as the string literal for the response as the default. Would that work?
| fakeAuthorizer := &coderdtest.FakeAuthorizer{} | ||
| db, _ := dbtestutil.NewDB(t) | ||
| fakeAuthorizer := &coderdtest.FakeAuthorizer{ | ||
| RegoAuthorizer: rbac.NewAuthorizer(prometheus.NewRegistry()), |
There was a problem hiding this comment.
As mentioned above, lets remove the need for a real authorizer.
| if testCase.outOfOrder { | ||
| rec.AssertOutOfOrder(s.T(), actor, pairs...) |
2f188e8 to
4ab19f7
Compare
|
@Emyrk thanks for the tip about removing the RegoAuthorizer from FakeAuthorizer. I didn't take the time to fully understand the authorization system and wasn't sure whether Let me know if this looks good to you. |
Another PR to address #15109.