- Notifications
You must be signed in to change notification settings - Fork926
chore: Update rego to be partial execution friendly#3449
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
Unfortunately this does slow down the unit tests :(
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/rbac/authz.go Outdated
| @@ -11,24 +11,35 @@ import ( | |||
| type Authorizer interface { | |||
| ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error | |||
| PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action Action, objectType string) (PreparedAuthorized, error) | |||
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 function is only used internally, so seems like it might not need to be exported like this.
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 need it for my Fake authorizer to work for some unit tests. I can't fake it without an interface
recordingAuthorizer always returns false and save the auth calls so I can assert them in my tests.
coderd/authorize.go Outdated
| return objects, nil | ||
| } | ||
| objecType := objects[0].RBACObject().Type | ||
| objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objecType, objects) |
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.
Seems like all of this logic could be in therbac package itself, then we wouldn't need therecordingAuthorizer code. Then functions could just callrbac.AuthorizeFilter directly.
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.
You are right about the object type stuff 👍
As forrecordingAuthorizer. That is needed for my unit test that checks all the routes.recordingAuthorizer is never used outside unit tests.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/rbac/authz.go Outdated
| @@ -11,24 +11,35 @@ import ( | |||
| type Authorizer interface { | |||
| ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error | |||
| PrepareByRoleName(ctx context.Context, subjectID string, roles []string, action Action, objectType string) (PreparedAuthorized, error) | |||
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 don't see any use case for exposing thePreparedAuthorized to consumers of authorization --- why not just expose the Filter() method here?
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 wish I could, but you can't make generic methods on structs. The nice thing about the genericFilter() method is you can pass in and return the same typed slice.
If I made it a non-generic function, you'd have to take your slice of[]Workspace, convert it to[]rbac.Object, and then convert it back to[]Workspace. I could do this all in somerbac.Authorize, but it takes a lot of slice copying I prefer to avoid.
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.
Well, that sucks. Go generics continue to disappoint.
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.
Yea, it's really dumb because I don't see why it's not possible. Like I would like to know the objection to it.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/users_test.go Outdated
| @@ -1209,7 +1209,7 @@ func TestPaginatedUsers(t *testing.T) { | |||
| t.Parallel() | |||
| // This test takes longer than a long time. | |||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) | |||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) | |||
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.
why did we increase timeout if we decreased RBAC time?
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 am unsure why this test was timing out. I can spend more time and look into it
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 test takes <3s on my local machine. I have no clue why it was timing out... going to drop back to *2 and see what I get.
- Move unit test to _internal- Remove example_test
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What this does
Updates to a new rego policy that passes all the same tests. The new rego policy can be partially evaluated into simple queries. A unit was added to ensure the new policy always can be converted into these simple queries.
Performance
This does not affect performance when authorizing a single object. It only affects when using
Filter.Below is the
Filteras is, vs the new filtering usingPartialevaluation to optimize subsequent Auth() calls. Note this benchmark is using the new rego policy. The PR#3426 is using adifferent rego policy. So the benchmarks are expected to be different.Correctness
RBAC has a pretty good test suite, so I can pretty confidently say the behavior matches what we had before. Meaning false positives/negatives should not occur.
Notes
There is a difference between rego's
PartialandPartialResult.PartialResultdoes not optimize in the same wayPartialdoes. The reasonPartialis much faster, is that the policy converts down to a set of queries based on the unknown fields (our new rego policy is designed to compress to these queries!)opa eval --partial --format=pretty 'data.authz.allow = true' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner -i input.jsonThe number of queries and the content of the queries depends on the user's roles. The intention is to convert these queries into SQL
WHEREclauses combined by anOR. Until we do that, we need to run through the queries ourselves, which as seen in the last benchmark, is quite fast.