-
Notifications
You must be signed in to change notification settings - Fork 18
feat(policy): policy should maintain a cache of entitlement policy #2327
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
Draft
jakedoublev
wants to merge
28
commits into
main
Choose a base branch
from
feat/authz-v2-cache-policy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
4923115
feat(policy): cache entitlement policy in authz v2
jakedoublev c86d9d7
hook to run after services are started
jakedoublev 12e2c4d
policy config
jakedoublev 9096218
consume policy cache
jakedoublev 0bfd2e3
lint fixes
jakedoublev 603fb63
fix cache utilization in auth service and lower defaults
jakedoublev cc99503
lint fixes
jakedoublev 5747db1
test: sleep to ensure roundtrip tests hit cached policy
jakedoublev 281f41d
put back rttests
jakedoublev a5859e1
put back authz doing any caching
jakedoublev a14f9ae
set cache on relevant policy services and refactor to servicesStarted…
jakedoublev a668dd7
improve log
jakedoublev 9505dd0
working cache
jakedoublev 3ba044c
lint fixes
jakedoublev b1ec09a
fix limit/offset
jakedoublev eae73f6
rm debounce on write-triggered refresh
jakedoublev f1d2fbb
Merge branch 'main' into feat/authz-v2-cache-policy
jakedoublev 5abe1fa
Merge branch 'main' into feat/authz-v2-cache-policy
jakedoublev ffa528c
rm mutation cache refreshes
jakedoublev e18c04a
disable caching by default
jakedoublev 0103264
fixes
jakedoublev f0a56a6
ensure close is called to shut down db clients and caches
jakedoublev 184cfff
lint fixes
jakedoublev f864a33
lint fixes
jakedoublev 07d6849
Merge remote-tracking branch 'origin' into feat/authz-v2-cache-policy
jakedoublev a1f8ac6
ristretto cache/go-cache implementation
jakedoublev b7cd66c
lint fix
jakedoublev 57873d8
fix shutdown panic
jakedoublev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
"log/slog" | ||
|
||
"connectrpc.com/connect" | ||
"github.com/opentdf/platform/protocol/go/common" | ||
"github.com/opentdf/platform/protocol/go/policy" | ||
"github.com/opentdf/platform/protocol/go/policy/attributes" | ||
"github.com/opentdf/platform/protocol/go/policy/attributes/attributesconnect" | ||
|
@@ -24,6 +25,14 @@ type AttributesService struct { //nolint:revive // AttributesService is a valid | |
logger *logger.Logger | ||
config *policyconfig.Config | ||
trace.Tracer | ||
cache *policyconfig.EntitlementPolicyCache // Cache for attributes and subject mappings | ||
} | ||
|
||
func OnServicesStarted(svc *AttributesService) serviceregistry.OnServicesStartedHook { | ||
return func(ctx context.Context) error { | ||
svc.cache = policyconfig.GetSharedEntitlementPolicyCache(ctx, svc.dbClient, svc.logger, svc.config) | ||
return nil | ||
} | ||
} | ||
|
||
func OnConfigUpdate(as *AttributesService) serviceregistry.OnConfigUpdateHook { | ||
|
@@ -44,16 +53,18 @@ func OnConfigUpdate(as *AttributesService) serviceregistry.OnConfigUpdateHook { | |
func NewRegistration(ns string, dbRegister serviceregistry.DBRegister) *serviceregistry.Service[attributesconnect.AttributesServiceHandler] { | ||
as := new(AttributesService) | ||
onUpdateConfigHook := OnConfigUpdate(as) | ||
onStartHook := OnServicesStarted(as) | ||
|
||
return &serviceregistry.Service[attributesconnect.AttributesServiceHandler]{ | ||
Close: as.Close, | ||
ServiceOptions: serviceregistry.ServiceOptions[attributesconnect.AttributesServiceHandler]{ | ||
Namespace: ns, | ||
DB: dbRegister, | ||
ServiceDesc: &attributes.AttributesService_ServiceDesc, | ||
ConnectRPCFunc: attributesconnect.NewAttributesServiceHandler, | ||
GRPCGatewayFunc: attributes.RegisterAttributesServiceHandler, | ||
OnConfigUpdate: onUpdateConfigHook, | ||
Namespace: ns, | ||
DB: dbRegister, | ||
ServiceDesc: &attributes.AttributesService_ServiceDesc, | ||
ConnectRPCFunc: attributesconnect.NewAttributesServiceHandler, | ||
GRPCGatewayFunc: attributes.RegisterAttributesServiceHandler, | ||
OnConfigUpdate: onUpdateConfigHook, | ||
OnServicesStarted: onStartHook, | ||
RegisterFunc: func(srp serviceregistry.RegistrationParams) (attributesconnect.AttributesServiceHandler, serviceregistry.HandlerServer) { | ||
logger := srp.Logger | ||
cfg, err := policyconfig.GetSharedPolicyConfig(srp.Config) | ||
|
@@ -65,15 +76,19 @@ func NewRegistration(ns string, dbRegister serviceregistry.DBRegister) *servicer | |
as.logger = logger | ||
as.dbClient = policydb.NewClient(srp.DBClient, logger, int32(cfg.ListRequestLimitMax), int32(cfg.ListRequestLimitDefault)) | ||
as.config = cfg | ||
|
||
return as, nil | ||
}, | ||
}, | ||
} | ||
} | ||
|
||
// Close gracefully shuts down the service, closing the database client. | ||
// Close gracefully shuts down the attributes service's cache and database client. | ||
func (s *AttributesService) Close() { | ||
s.logger.Info("gracefully shutting down attributes service") | ||
if s.cache != nil { | ||
s.cache.Stop() | ||
} | ||
s.dbClient.Close() | ||
} | ||
|
||
|
@@ -120,6 +135,50 @@ func (s *AttributesService) ListAttributes(ctx context.Context, | |
state := req.Msg.GetState().String() | ||
s.logger.Debug("listing attribute definitions", slog.String("state", state)) | ||
|
||
// If active state and caching enabled, return from cache instead of DB | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The caching logic in Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
isActiveState := req.Msg.GetState() == common.ActiveStateEnum_ACTIVE_STATE_ENUM_ACTIVE | ||
if s.cache.IsEnabled() && isActiveState { | ||
s.logger.Debug("returning cached attributes") | ||
|
||
limit := req.Msg.GetPagination().GetLimit() | ||
if limit <= 0 { | ||
limit = int32(s.config.ListRequestLimitDefault) | ||
} | ||
offset := req.Msg.GetPagination().GetOffset() | ||
|
||
// Validate limit against the max configured value | ||
maxLimit := int32(s.config.ListRequestLimitMax) | ||
if maxLimit > 0 && limit > maxLimit { | ||
return nil, db.StatusifyError(db.ErrListLimitTooLarge, db.ErrTextListLimitTooLarge) | ||
} | ||
|
||
// Get all cached attributes | ||
cachedAttrs, total, err := s.cache.ListCachedAttributes(ctx, limit, offset) | ||
if err != nil { | ||
s.logger.Error("failed to retrieve cached attributes", slog.Any("error", err)) | ||
return nil, db.StatusifyError(err, db.ErrTextListRetrievalFailed) | ||
} | ||
|
||
// Calculate next offset using the same logic as the DB implementation | ||
var nextOffset int32 | ||
next := offset + limit | ||
if next < total { | ||
nextOffset = next | ||
} // else nextOffset remains 0 | ||
|
||
rsp := &attributes.ListAttributesResponse{ | ||
Attributes: cachedAttrs, | ||
Pagination: &policy.PageResponse{ | ||
CurrentOffset: offset, | ||
Total: total, | ||
NextOffset: nextOffset, | ||
}, | ||
} | ||
return connect.NewResponse(rsp), nil | ||
} | ||
|
||
s.logger.Debug("querying database for attributes") | ||
|
||
rsp, err := s.dbClient.ListAttributes(ctx, req.Msg) | ||
if err != nil { | ||
return nil, db.StatusifyError(err, db.ErrTextListRetrievalFailed) | ||
|
@@ -365,7 +424,6 @@ func (s *AttributesService) DeactivateAttributeValue(ctx context.Context, req *c | |
s.logger.Audit.PolicyCRUDSuccess(ctx, auditParams) | ||
|
||
rsp.Value = updated | ||
|
||
return connect.NewResponse(rsp), nil | ||
} | ||
|
||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RunServicesStartedHooks
call appears twice in this function; remove the earlier invocation to avoid running the hooks twice.Copilot uses AI. Check for mistakes.