-
Notifications
You must be signed in to change notification settings - Fork 66
add authz developer guide #9751
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
andrewjstone
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.
This is great! Thank you for adding this @davepacheco.
I'm likely to be adding synthetic resources soon for the trust quorum, and will open PRs with any necessary changes.
|
|
||
| * Writing, modifying, and reviewing implementation code is easy because all the authz checks just check exactly the thing you're doing. You don't have to reason about the authz policy while looking at the implementation. | ||
| * Writing, modifying, and reviewing the policy is also easier without having to consider the implementation. | ||
| * We're able to write xref:../nexus/db-queries/src/policy_test/mod.rs[totally comprehensive tests] on the policy without having to exercise every single code path in Nexus. |
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.
These links don't work.
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.
Hmm. They do for me, both locally when I render with asciidoctor and also on GitHub when I look at https://github.com/oxidecomputer/omicron/blob/06dd48aa9195bc355fe7cb6333c0d90774a11d10/docs/authz-dev-guide.adoc. How are you viewing 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.
They do work from that link. I was viewing them from https://github.com/oxidecomputer/omicron/pull/9751/changes with the rich diff toggled. They don't work there, which is not a big deal. Just another bit of GitHub nonsense.
| ---- | ||
| #[endpoint { | ||
| method = GET, | ||
| path = "/v1/disks/{disk}", |
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 think for real newbies, it would be good to call out that DiskPath is the raw identifier here and maps to the {disk} parameter.
Then in the App Layer section below we can give an example of taking that raw identifier and turning it into an authz object.
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 took a swing at this. I did leave some stuff out. In the real code, there's an additional thing here: the optional project selector as a query parameter. Both the disk and the project selector could be ids or names. Then we wind up calling disk_lookup() to figure it all out and translate it into a LookupPath invocation. We do use this pattern in a few places but it's not the general case. So I skipped over a bunch of it. In the example here, I said either you'll have a disk id or a project name (or id) and disk name, and just showed the LookupPath code for those.
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 new changes look great. Thanks @davepacheco!
I still need to do some fact-checking on this (Claude Code wrote the initial draft) and potentially some editing. Feedback welcome, especially from anybody who's added their own authz resources! CC @andrewjstone @charliepark.