Skip to content

Conversation

@ghaskins
Copy link
Contributor

@ghaskins ghaskins commented Dec 18, 2025

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

@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Dec 18, 2025
Copy link
Contributor

@github-actions github-actions bot left a 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 transformation

Alternative:

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
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

❌ Patch coverage is 95.13514% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.59%. Comparing base (fb475a8) to head (3bc1314).

Files with missing lines Patch % Lines
pkg/authz/authorizers/config.go 94.11% 2 Missing and 2 partials ⚠️
cmd/thv-operator/pkg/controllerutil/authz.go 88.88% 1 Missing and 1 partial ⚠️
pkg/authz/authorizers/cedar/core.go 95.65% 1 Missing and 1 partial ⚠️
pkg/authz/config.go 88.88% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 19, 2025
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 19, 2025
@github-actions github-actions bot dismissed their stale review December 19, 2025 02:52

Large PR justification has been provided. Thank you!

@github-actions
Copy link
Contributor

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

Copy link
Contributor

@yrobla yrobla left a 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
jhrozek previously approved these changes Dec 19, 2025
Copy link
Contributor

@jhrozek jhrozek left a 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>
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 19, 2025
@ghaskins
Copy link
Contributor Author

@yrobla I just force-pushed an update to my branch with the following changes

  • I rebased on top of todays latest (fb475a8)
  • I added a new patch for the docs/authz changes you requested

The original patch is unchanged, other than the noted rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

3 participants