Skip to content

feat: add NATS KV cache bucket for username→sub lookups#44

Open
jordane wants to merge 1 commit into
mainfrom
jme/LFXV2-1561
Open

feat: add NATS KV cache bucket for username→sub lookups#44
jordane wants to merge 1 commit into
mainfrom
jme/LFXV2-1561

Conversation

@jordane
Copy link
Copy Markdown
Member

@jordane jordane commented Apr 22, 2026

Summary

  • Introduces auth-service-username-sub-cache NATS JetStream KV bucket (168h TTL) to cache Auth0 username→sub lookups
  • Cache is checked before each Auth0 Management API call; misses fall through to Auth0 and populate the cache on success
  • Bucket is best-effort: if unavailable at startup the service logs a warning and continues without caching (graceful degradation)

Changes

  • charts/.../nats-kv-buckets.yaml — new KV bucket manifest (unconditional, 168h TTL, history=1)
  • charts/.../values.yamlusername_sub_cache_kv_bucket defaults
  • pkg/constants/storage.goKVBucketNameUsernameSubCache constant
  • internal/infrastructure/nats/client.go — best-effort bucket init on startup
  • internal/service/message_handler.gokvCache interface, cache-aside logic in UsernameToSub
  • cmd/server/service/providers.go — wires KV store into the message handler
  • internal/service/message_handler_test.go — cache hit and miss tests

Closes LFXV2-1561

🤖 Generated with Claude Code

Introduce the auth-service-username-sub-cache KV bucket (168h TTL) to
cache Auth0 username→sub results, reducing Management API pressure and
lookup latency. Cache is checked before each Auth0 call; a miss falls
through to Auth0 and populates the cache on success.

Issue: LFXV2-1561

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Jordan Evans <jevans@linuxfoundation.org>
Copilot AI review requested due to automatic review settings April 22, 2026 18:30
@jordane jordane requested a review from a team as a code owner April 22, 2026 18:30
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2c71cd5d-c5b3-4f0e-befc-701955dff888

📥 Commits

Reviewing files that changed from the base of the PR and between 92b991e and f1eb79d.

📒 Files selected for processing (7)
  • charts/lfx-v2-auth-service/templates/nats-kv-buckets.yaml
  • charts/lfx-v2-auth-service/values.yaml
  • cmd/server/service/providers.go
  • internal/infrastructure/nats/client.go
  • internal/service/message_handler.go
  • internal/service/message_handler_test.go
  • pkg/constants/storage.go

Walkthrough

This pull request introduces a username-to-subscription caching layer using NATS JetStream KV buckets. It adds Helm manifests, configuration values, KV bucket initialization, cache-aside lookup logic in the message handler, and comprehensive tests.

Changes

Cohort / File(s) Summary
Helm Chart Configuration
charts/lfx-v2-auth-service/templates/nats-kv-buckets.yaml, charts/lfx-v2-auth-service/values.yaml
Added conditional KeyValue bucket manifest for username sub cache with configurable settings (history, storage type, compression, size limits, TTL) and corresponding values configuration with creation and persistence flags.
Constants
pkg/constants/storage.go
Added new exported constant KVBucketNameUsernameSubCache defining the NATS KV bucket name as "auth-service-username-sub-cache".
NATS Client Infrastructure
internal/infrastructure/nats/client.go
Updated NewClient to initialize username sub cache bucket as best-effort (logs warning on failure and continues), and refactored Authelia-related buckets to use explicit inline slice with immediate failure on initialization errors.
Service Layer - Caching Logic
internal/service/message_handler.go
Added kvCache interface abstraction, usernameSubCache field to messageHandlerOrchestrator, and WithUsernameSubCacheForMessageHandler option. Implemented cache-aside lookup in UsernameToSub that retrieves from cache on hit or queries user reader and populates cache on miss.
Service Layer - Tests
internal/service/message_handler_test.go
Added mock KV entry and cache test doubles; introduced two test cases validating cache-hit (no user reader call) and cache-miss (user reader called and cache populated) behaviors.
Service Provider Configuration
cmd/server/service/providers.go
Updated QueueSubscriptions to conditionally append WithUsernameSubCacheForMessageHandler orchestrator option when KV store is available.

Sequence Diagram

sequenceDiagram
    participant MH as Message Handler
    participant KVC as KV Cache
    participant UR as User Reader
    
    MH->>MH: UsernameToSub(username)
    alt Cache Available
        MH->>KVC: Get(username)
        alt Cache Hit
            KVC-->>MH: UserID
            MH-->>MH: Return cached UserID
        else Cache Miss
            KVC-->>MH: KeyNotFound error
            MH->>UR: SearchUser(username)
            UR-->>MH: user (with UserID)
            MH->>KVC: Put(username, UserID)
            KVC-->>MH: Success
            MH-->>MH: Return UserID
        end
    else Cache Unavailable
        MH->>UR: SearchUser(username)
        UR-->>MH: user (with UserID)
        MH-->>MH: Return UserID
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the primary change: adding a NATS KV cache bucket for username→sub lookups, which aligns with the main objective across all modified files.
Description check ✅ Passed The description is directly related to the changeset, providing a clear summary of the caching feature, implementation approach, affected files, and associated issue.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jme/LFXV2-1561

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a NATS JetStream KeyValue bucket to act as a cache for Auth0 username→sub lookups, using cache-aside logic to reduce Auth0 Management API calls while allowing graceful degradation when the bucket is unavailable.

Changes:

  • Adds a new KV bucket (auth-service-username-sub-cache) via Helm with a 168h TTL and history=1.
  • Initializes the bucket best-effort at NATS client startup and wires the KV store into the message handler.
  • Implements cache-aside behavior in UsernameToSub and adds cache hit/miss unit tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
charts/lfx-v2-auth-service/templates/nats-kv-buckets.yaml Adds a Helm-managed JetStream KV manifest for the username→sub cache bucket.
charts/lfx-v2-auth-service/values.yaml Introduces default values for configuring the new cache bucket (TTL, size, etc.).
pkg/constants/storage.go Adds a constant for the new KV bucket name.
internal/infrastructure/nats/client.go Best-effort initialization of the cache KV bucket at startup.
cmd/server/service/providers.go Wires the KV store into the message handler orchestrator when available.
internal/service/message_handler.go Adds a minimal KV interface and cache-aside lookup/populate logic in UsernameToSub.
internal/service/message_handler_test.go Adds unit tests covering cache hit and cache miss behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +171 to +178
// The username→sub cache bucket is best-effort: if it doesn't exist or is
// unavailable the service still works, just without caching.
if err := client.KeyValueStore(ctx, constants.KVBucketNameUsernameSubCache); err != nil {
slog.WarnContext(ctx, "username→sub cache bucket unavailable, caching disabled",
"error", err,
"bucket", constants.KVBucketNameUsernameSubCache,
)
} else {
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

This best-effort initialization still emits an error-level log because KeyValueStore() logs slog.ErrorContext before returning the error. As a result, when the bucket is missing/unavailable startup will log both an ERROR and this WARN, which contradicts the PR description (“logs a warning and continues”) and may trigger error-based alerting. Consider adding a non-error logging path for optional buckets (e.g., detect/handle missing-bucket/JetStream-unavailable errors here, or add a KeyValueStore variant/flag that doesn’t log at error level for expected best-effort failures).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@bramwelt bramwelt left a comment

Choose a reason for hiding this comment

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

Just a minor nit.

maxValueSize: {{ .Values.nats.username_sub_cache_kv_bucket.maxValueSize }}
maxBytes: {{ .Values.nats.username_sub_cache_kv_bucket.maxBytes }}
compression: {{ .Values.nats.username_sub_cache_kv_bucket.compression }}
ttl: {{ .Values.nats.username_sub_cache_kv_bucket.ttl }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you include replicas here as well please?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants