Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: Drop resource_id support in rbac system#3426

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

Merged
Emyrk merged 7 commits intomainfromstevenmasley/rbac_no_ids
Aug 9, 2022

Conversation

Emyrk
Copy link
Member

@EmyrkEmyrk commentedAug 9, 2022
edited
Loading

What this does

This dropsresource_id from the RBAC permissions. This is a loss of a feature from RBAC. The goal is to reduce complexity of the rego policy, and reduce the number of permissions on actors.

At the present, noresource_id permissions are used. So this removes excess functionality that has no real planned use in the current roadmap.

Why did we have resource_id?

Intended to support:

  • Workspace agent tokens
    • This is handled outside the RBAC permission system
  • Roles as a resource (solved)
    • Solution needs to be determined. This intended solution was building a graph using permission nodes. Although "clean" from an additional code perspective, visualizing and displaying the output of this was going to be a challenge. It is likely easier to just build static lists of approved assign/remove roles for each role.
    • We can still implement this making each role a unique resource type.
  • Workspace sharing (ACL lists on object?)
    • Solution was to assign permission nodes to other users to share my workspace?? This solution was technically supported, but a bit convoluted as it requires assigning permissions to another user. It would also be hard to list all users that this workspace is shared with.

What do we gain?

By removingresource_id, there is only 3 properties of an object we need to knowAuthorize?:object_type,owner_id, andorg_owner. This reduction only removes 1 property, but drastically reduces the number of unique objects in the system. This can drastically speedupAuthorize() calls on large sets and enables future optimizations.

Can we revert this?

Yes. We can revert this, however I don't recommend we doPhase 2.

I think it is better to have a need arise and implement this back, then have this feature be available and possibly misused because it is "just there".

What this does not do

This does not reduce RBAC authorization benchmarks.

resource_id was implemented to support: - Workspace agent tokens - Roles as a resource - Workspace sharingThe additional complexity is not required and will ease supportof rbac moving forward.
@EmyrkEmyrk marked this pull request as ready for reviewAugust 9, 2022 16:58
Copy link
Member

@kylecarbskylecarbs left a comment

Choose a reason for hiding this comment

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

I like the simplification here. Could we add a benchmark so we can see how much this improves the performance?

@Emyrk
Copy link
MemberAuthor

I like the simplification here. Could we add a benchmark so we can see how much this improves the performance?

This won't improve performance at all, but it allows me to use my new rego query that supports partials. I just need to drop this requirement first. I can add benchmarks for the sake of benchmarks though 👍

@kylecarbs
Copy link
Member

Ahh, I see. Should we just add that Rego query as part of this then?

@Emyrk
Copy link
MemberAuthor

Ahh, I see. Should we just add that Rego query as part of this then?

I have it like 90% written. I figured I'd drop this functionality, get the tests passing, then I can swap the rego without a sweat because if the tests pass, the implementation behavior is identical.

Feels "cleaner" to make that a different PR. If I add benchmarks to this, it also gives us a better comparison and we can go back in time in our git history a bit cleaner.

kylecarbs reacted with thumbs up emoji

@kylecarbs
Copy link
Member

Makes sense to me! I approve.

Let's link the PRs in your future one, just so the timeline makes sense.

Emyrk reacted with thumbs up emoji

@Emyrk
Copy link
MemberAuthor

Emyrk commentedAug 9, 2022
edited
Loading

Benchmarks:

$ go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out -count=3 goos: linuxgoarch: amd64pkg: github.com/coder/coder/coderd/rbaccpu: Intel(R) Core(TM) i7-6770HQ CPU @ 2.60GHzBenchmarkRBACFilter/NoRoles-8              13080             92679 ns/op       37322 B/op            684 allocs/opBenchmarkRBACFilter/Admin-8                 3624            316964 ns/op      106771 B/op           2157 allocs/opBenchmarkRBACFilter/OrgAdmin-8              3650            275078 ns/op       90953 B/op           1827 allocs/opBenchmarkRBACFilter/OrgMember-8             3897            294138 ns/op      101517 B/op           2072 allocs/opBenchmarkRBACFilter/ManyRoles-8             2319            503633 ns/op      161066 B/op           3245 allocs/op

@EmyrkEmyrkenabled auto-merge (squash)August 9, 2022 18:15
@EmyrkEmyrk merged commitdb665e7 intomainAug 9, 2022
@EmyrkEmyrk deleted the stevenmasley/rbac_no_ids branchAugust 9, 2022 18:16
Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

👍

Emyrk reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@johnstcnjohnstcnjohnstcn left review comments

@kylecarbskylecarbskylecarbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@Emyrk@kylecarbs@johnstcn

[8]ページ先頭

©2009-2025 Movatter.jp