Skip to content

Conversation

elizabethhealy
Copy link
Member

@elizabethhealy elizabethhealy commented Jun 23, 2025

Proposed Changes

  • Centralizes Keycloak entity retrieval logic into helper functions (retrieve.go)
  • Adds caching for Keycloak lookups (clients, users, groups, group members)
  • Updates entity resolution code to use cached helpers
  • Improves performance by reducing redundant Keycloak requests
  • No API changes; internal refactor only
  • Adds tests for cache integration and expiration

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @elizabethhealy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the performance of the Keycloak Entity Resolution Service (ERS) by implementing a robust caching mechanism. It aims to reduce the number of direct calls to the Keycloak server for frequently requested client, user, and group information, thereby improving response times and reducing load on the identity provider.

Highlights

  • Caching Implementation: Introduced a new svcCache *cache.Cache parameter to several core Keycloak ERS functions, including EntityResolution, CreateEntityChainsFromTokens, expandGroup, getEntitiesFromToken, and getServiceAccountClient, enabling the propagation of a caching mechanism throughout the service.
  • Refactored Keycloak API Calls: Replaced direct calls to the gocloak client (e.g., GetClients, GetUsers, GetGroups, GetGroup, GetGroupMembers) with new, dedicated retrieve helper functions. These helpers now encapsulate the logic for fetching data from Keycloak, integrating with the new caching layer.
  • New Caching Module: Added a new file, service/entityresolution/keycloak/v2/retrieve.go, which contains the implementation of the caching-aware retrieve helper functions. This module also includes a generic retrieveWithKey function that centralizes the cache lookup, retrieval, and storage logic for various Keycloak entity types.
  • Cache Key Strategy: Defined clear cache key formats for different Keycloak entities (clients, users, groups, and group members) to ensure consistent and effective caching.
  • Unit Tests for Caching: Introduced a new test file, service/entityresolution/keycloak/v2/retrieve_test.go, with comprehensive unit tests specifically designed to verify the correct caching behavior (cache hits, misses, and expiration) of the newly implemented retrieve functions.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Keycloak calls, now less, Cached data, swift success, Performance, no stress.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request successfully integrates caching into the Keycloak Entity Resolution Service (ERS) by introducing a new retrieve.go file that centralizes caching logic. The new unit tests for the caching functions in retrieve_test.go are well-written and effectively verify cache hits, misses, and expiration. The refactoring improves modularity and efficiency by reducing redundant Keycloak API calls. However, there are a few areas for improvement regarding consistency in applying the caching mechanism and enhancing test coverage for the end-to-end caching flow.

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 530.252974ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 348.834863ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.770703ms
Throughput 287.55 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.97100657s
Average Latency 378.367522ms
Throughput 131.68 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.736396022s
Average Latency 266.440425ms
Throughput 187.01 requests/second

@elizabethhealy elizabethhealy changed the title feat(ers): Add caching to keycloak ERS feat(authz): Add caching to keycloak ERS Jun 23, 2025
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 499.056393ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 336.819102ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 336.459888ms
Throughput 297.21 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.601543566s
Average Latency 364.237471ms
Throughput 136.61 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.794486073s
Average Latency 257.139078ms
Throughput 193.84 requests/second

@elizabethhealy elizabethhealy marked this pull request as ready for review June 23, 2025 16:37
@elizabethhealy elizabethhealy requested a review from a team as a code owner June 23, 2025 16:37
pflynn-virtru
pflynn-virtru previously approved these changes Jun 23, 2025
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 529.509088ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 326.627285ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 354.146298ms
Throughput 282.37 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.259075364s
Average Latency 371.140115ms
Throughput 134.20 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.304566817s
Average Latency 251.843639ms
Throughput 197.59 requests/second

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 541.881938ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 344.849194ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 343.622857ms
Throughput 291.02 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.68898685s
Average Latency 364.984596ms
Throughput 136.28 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.322906959s
Average Latency 262.491518ms
Throughput 189.95 requests/second

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 538.470255ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 333.916161ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.540138ms
Throughput 287.74 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.642056504s
Average Latency 364.801471ms
Throughput 136.46 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.70707942s
Average Latency 255.983835ms
Throughput 194.50 requests/second

jakedoublev
jakedoublev previously approved these changes Jun 23, 2025
jakedoublev
jakedoublev previously approved these changes Jun 23, 2025
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 523.253537ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 308.811605ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 345.706914ms
Throughput 289.26 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.699881009s
Average Latency 375.667172ms
Throughput 132.63 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.787736201s
Average Latency 266.676022ms
Throughput 186.65 requests/second

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 524.724377ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 330.994195ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.133411ms
Throughput 288.07 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 36.012461226s
Average Latency 358.452805ms
Throughput 138.84 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 25.347609928s
Average Latency 252.269401ms
Throughput 197.26 requests/second

@jakedoublev jakedoublev added this pull request to the merge queue Jun 23, 2025
Merged via the queue into main with commit f5b0a06 Jun 23, 2025
29 checks passed
@jakedoublev jakedoublev deleted the dspx-1299-keycloak-ers-caching branch June 23, 2025 20:37
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.7.0](service/v0.6.0...service/v0.7.0)
(2025-06-24)


### ⚠ BREAKING CHANGES

* **policy:** disable kas grants in favor of key mappings
([#2220](#2220))

### Features

* **authz:** Add caching to keycloak ERS
([#2466](#2466))
([f5b0a06](f5b0a06))
* **authz:** auth svc registered resource GetDecision support
([#2392](#2392))
([5405674](5405674))
* **authz:** authz v2 GetBulkDecision
([#2448](#2448))
([0da3363](0da3363))
* **authz:** cache entitlement policy within authorization service
([#2457](#2457))
([c16361c](c16361c))
* **authz:** ensure logging parity between authz v2 and v1
([#2443](#2443))
([ef68586](ef68586))
* **core:** add cache manager
([#2449](#2449))
([2b062c5](2b062c5))
* **core:** consume RPC interceptor request context metadata in logging
([#2442](#2442))
([2769c48](2769c48))
* **core:** DSPX-609 - add cli-client to keycloak provisioning
([#2396](#2396))
([48e7489](48e7489))
* **core:** ERS cache setup, fix cache initialization
([#2458](#2458))
([d0c6938](d0c6938))
* inject logger and cache manager to key managers
([#2461](#2461))
([9292162](9292162))
* **kas:** expose provider config from key details.
([#2459](#2459))
([0e7d39a](0e7d39a))
* **main:** Add Close() method to cache manager
([#2465](#2465))
([32630d6](32630d6))
* **policy:** disable kas grants in favor of key mappings
([#2220](#2220))
([30f8cf5](30f8cf5))
* **policy:** Restrict deletion of pc with used key.
([#2414](#2414))
([3b40a46](3b40a46))
* **sdk:** allow Connect-Protocol-Version RPC header for cors
([#2437](#2437))
([4bf241e](4bf241e))


### Bug Fixes

* **core:** remove generics on new platform cache manager and client
([#2456](#2456))
([98c3c16](98c3c16))
* **core:** replace opentdf-public client with cli-client
([#2422](#2422))
([fb18525](fb18525))
* **deps:** bump github.com/casbin/casbin/v2 from 2.106.0 to 2.107.0 in
/service in the external group
([#2416](#2416))
([43afd48](43afd48))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to
0.5.0 in /service
([#2470](#2470))
([3a73fc9](3a73fc9))
* **deps:** bump github.com/opentdf/platform/sdk from 0.4.7 to 0.5.0 in
/service ([#2473](#2473))
([ad37476](ad37476))
* **deps:** bump the external group across 1 directory with 2 updates
([#2450](#2450))
([9d8d1f1](9d8d1f1))
* **deps:** bump the external group across 1 directory with 2 updates
([#2472](#2472))
([d45b3c8](d45b3c8))
* only request a token when near expiration
([#2370](#2370))
([556d95e](556d95e))
* **policy:** fix casing bug and get provider config on update.
([#2403](#2403))
([a52b8f9](a52b8f9))
* **policy:** properly formatted pem in test fixtures
([#2409](#2409))
([54ffd23](54ffd23))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants