- Notifications
You must be signed in to change notification settings - Fork 158
[#3109] Implement authorization abstraction #3110
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
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.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification [Explain why this PR must be large, such as:] - Generated code that cannot be split - Large refactoring that must be atomic - Multiple related changes that would break if separated - Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## main #3110 +/- ## ========================================== + Coverage 57.08% 63.59% +6.50% ========================================== Files 341 343 +2 Lines 33940 34031 +91 ========================================== + Hits 19376 21642 +2266 + Misses 12961 10649 -2312 - Partials 1603 1740 +137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Large PR justification has been provided. Thank you!
| ✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
yrobla 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.
Nice work! My only comment is to update the docs/authz file to reflect this new pluggable system
jhrozek 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.
Looks good! The refactoring to a pluggable authorization framework is clean and the factory pattern works well.
One thing I noticed: the swagger docs only show version/type for the Config struct since the authorizer-specific fields come from MarshalJSON. Considered fixing it but it would mean exporting internal fields just for docs - not worth the tradeoff.
This patch implements an 'authorizer' abstraction under pkg/authz, and then moves the Cedar implementation as the first/canonical form under pkg/authz/authorizers/cedar. The configuration schema remains untouched, though the mechanism for loading configuration has been reworked to avoid violating the authorizer abstraction with Cedar-isms. This fixes stacklok#3109 Signed-off-by: Greg Haskins <greg@manetu.com>
Signed-off-by: Greg Haskins <greg@manetu.com>
This patch implements an 'authorizer' abstraction under pkg/authz, and then moves the Cedar implementation as the first/canonical form under pkg/authz/authorizers/cedar.
The configuration schema remains untouched, though the mechanism for loading configuration has been reworked to avoid violating the authorizer abstraction with Cedar-isms.
Large PR Justification
Large refactoring that must be atomic
This fixes #3109