Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Oct 23, 2025

Description

Example configurations with quota limiter setup

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Summary by CodeRabbit

  • Documentation
    • Added example configuration files demonstrating quota limiting setup with PostgreSQL and SQLite backends. Examples include user and cluster-level quota configurations, conversation caching, and scheduler settings for Lightspeed Core Service deployments.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

Two new example YAML configuration files are introduced for Lightspeed Core Service quota limiting setups: one using PostgreSQL backend and another using SQLite. Each defines server settings, llama_stack client configuration, authentication, conversation caching, and quota handlers with user and cluster limiters.

Changes

Cohort / File(s) Summary
Quota Limiter Example Configurations
examples/quota-limiter-configuration-pg.yaml, examples/quota-limiter-configuration-sqlite.yaml
Added two example YAML files demonstrating quota limiter setup with different storage backends (PostgreSQL and SQLite). Each file includes server configuration, llama_stack client settings, authentication, conversation cache, and quota handlers with user and cluster limiter definitions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰 Two configs hop into the warren today,
One PostgreSQL, one SQLite's way,
Quotas and limits all neatly arranged,
With clusters and users—delightfully changed! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Example configurations with quota limiter setup" directly and accurately describes the primary changes in the changeset. The PR adds two new YAML example configuration files (one with PostgreSQL and one with SQLite) that demonstrate quota limiter setup for Lightspeed Core Service. The title is concise, clear, and avoids vague terminology or noise, making it easy for teammates scanning the repository history to understand that this change introduces new configuration examples. The title appropriately reflects the main point of the changeset without attempting to cover all implementation details.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (4)
examples/quota-limiter-configuration-sqlite.yaml (2)

36-46: Add clarification for commented-out byok_rag section.

The BYOK RAG configuration block is disabled without explanation. Add a brief comment explaining when this should be enabled or remove it if not relevant to this example.

-#byok_rag:
-#  - rag_id: ocp_docs
+# Uncomment below to enable Bring-Your-Own-Knowledge RAG if needed:
+# byok_rag:
+#   - rag_id: ocp_docs

52-61: Clarify quota limiter period semantics in comments.

The limiter names suggest "monthly" cadence, but the periods are "2 hours" and "1 day". Add inline comments to clarify whether these reset at the configured period (not monthly) or if the names are misleading.

  limiters:
    - name: user_monthly_limits
      type: user_limiter
+     # Quota resets every 2 hours (not monthly, despite the name)
      initial_quota: 10
      quota_increase: 10
      period: "2 hours"
    - name: cluster_monthly_limits
      type: cluster_limiter
+     # Quota resets every 1 day (not monthly, despite the name)
      initial_quota: 100
      quota_increase: 10
      period: "1 day"

Consider renaming to clarify the actual reset period (e.g., user_periodic_limits, cluster_daily_limits).

examples/quota-limiter-configuration-pg.yaml (2)

33-34: Enable SSL for database connections in production example.

Both database connection blocks (conversation_cache and quota_handlers) have SSL and GSS encryption disabled. For a production-oriented example, these should be enabled or at minimum documented with security implications.

-    ssl_mode: disable
-    gss_encmode: disable
+    ssl_mode: require  # Use 'disable' only for local development/testing
+    gss_encmode: disable

Alternatively, add a comment explaining the security tradeoff:

    # ⚠️  Only disable SSL for local development; enable for production
-    ssl_mode: disable
-    gss_encmode: disable
+    ssl_mode: require
+    gss_encmode: require

Also applies to: 43-44


46-55: Align limiter documentation with sqlite variant; clarify period semantics.

Like the sqlite configuration, the limiter names suggest "monthly" cadence but reset periods are "2 hours" and "1 day". Add clarifying comments and consider renaming for consistency.

  limiters:
    - name: user_monthly_limits
      type: user_limiter
+     # Quota resets every 2 hours (not monthly, despite the name)
      initial_quota: 10
      quota_increase: 10
      period: "2 hours"
    - name: cluster_monthly_limits
      type: cluster_limiter
+     # Quota resets every 1 day (not monthly, despite the name)
      initial_quota: 100
      quota_increase: 10
      period: "1 day"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a03923f and cd3faf4.

📒 Files selected for processing (2)
  • examples/quota-limiter-configuration-pg.yaml (1 hunks)
  • examples/quota-limiter-configuration-sqlite.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)

Comment on lines +3 to +5
host: 0.0.0.0
port: 8080
auth_enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add security warning for development-only configuration.

This configuration is unsafe for production: all interfaces exposed (0.0.0.0), authentication disabled, and SSL/encryption disabled on database connections (lines 33, 43).

Add a prominent warning at the file header:

+# ⚠️  WARNING: This is a development/example configuration only.
+# Do NOT use in production. Ensure:
+# - auth_enabled is set to true
+# - host is restricted to 127.0.0.1 or a specific network interface
+# - All credentials are updated from placeholder values
+# - Enable ssl_mode and gss_encmode for database connections
+#
 name: Lightspeed Core Service (LCS)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In examples/quota-limiter-configuration-pg.yaml around lines 3 to 5, add a
prominent top-of-file security warning comment stating this configuration is for
development only (exposes all interfaces 0.0.0.0, has auth disabled, and DB
SSL/encryption disabled — see DB settings at lines 33 and 43), and include
recommended actions: limit host to localhost, enable authentication, enable
SSL/encryption for DB connections, and avoid committing this file or use
environment-specific overrides for production.

# use_as_library_client: true
# library_client_config_path: <path-to-llama-stack-run.yaml-file>
url: http://localhost:8321
api_key: xyzzy
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Replace hardcoded secrets with environment variable references.

Like the SQLite variant, this configuration includes plaintext credentials in multiple locations: the API key (line 17), conversation cache credentials (lines 30-32), and quota handler credentials (lines 41-42). These should be externalized.

Apply environment variable substitution pattern consistently:

  url: http://localhost:8321
-  api_key: xyzzy
+  api_key: ${LLAMA_STACK_API_KEY}  # Set via environment variable
  postgres:
    host: "127.0.0.1"
    port: 5432
    db: test
-    user: tester
-    password: 123qwe
+    user: ${DB_USER}  # Change for production
+    password: ${DB_PASSWORD}  # Change for production
    ssl_mode: disable
    gss_encmode: disable

Repeat the same pattern for the quota_handlers postgres section below.

Also applies to: 30-32, 41-42

🤖 Prompt for AI Agents
In examples/quota-limiter-configuration-pg.yaml around lines 17, 30-32 and
41-42, replace the hardcoded secrets (api_key and all Postgres credentials in
the conversation cache and quota_handlers sections) with environment-variable
references following the same pattern used in the SQLite variant; create clear
env var names (e.g., QUOTA_API_KEY,
CONVERSATION_DB_HOST/PORT/USER/PASSWORD/NAME,
QUOTA_HANDLER_DB_USER/PASSWORD/HOST/PORT/NAME) and update each credential field
to read from those env vars consistently so no plaintext secrets remain in the
file.

Comment on lines +3 to +5
host: 0.0.0.0
port: 8080
auth_enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add security warning for development-only configuration.

The service is configured to bind to all interfaces (0.0.0.0) with authentication disabled (auth_enabled: false). This is unsafe for any networked or production environment.

Add a prominent comment at the file header:

+# ⚠️  WARNING: This is a development/example configuration only.
+# Do NOT use in production. Ensure:
+# - auth_enabled is set to true
+# - host is restricted to 127.0.0.1 or a specific network interface
+# - All credentials are updated from placeholder values
+#
 name: Lightspeed Core Service (LCS)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In examples/quota-limiter-configuration-sqlite.yaml around lines 3 to 5, the
config binds to 0.0.0.0 and has auth_enabled: false which is unsafe; add a
prominent header comment above these lines warning this configuration is for
development only, explaining that binding to all interfaces and disabling auth
exposes the service to networks, and advise enabling authentication and
restricting bind address before using in any non-development or production
environment.

# use_as_library_client: true
# library_client_config_path: <path-to-llama-stack-run.yaml-file>
url: http://localhost:8321
api_key: xyzzy
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Replace hardcoded secrets with environment variable references or clear placeholders.

Example configurations should not include actual (or easily guessable) credentials. Users who copy this file may inadvertently deploy with insecure defaults. Use environment variable substitution or add prominent comments indicating which values must be changed.

Consider updating to:

  url: http://localhost:8321
-  api_key: xyzzy
+  api_key: ${LLAMA_STACK_API_KEY}  # Set via environment variable

And for database credentials:

  postgres:
    host: 127.0.0.1
    db: test
-    user: tester
-    password: 123qwe
+    user: ${DB_USER}  # Change for production
+    password: ${DB_PASSWORD}  # Change for production

Also add a comment at the top of the file warning about these placeholders.

Also applies to: 30-32

🤖 Prompt for AI Agents
In examples/quota-limiter-configuration-sqlite.yaml around line 17 (and also
lines 30-32), the file contains hardcoded secrets (e.g., api_key and database
credentials); replace these with environment variable references or obvious
placeholders (for example use ${API_KEY} or <YOUR_API_KEY>), update any DB
username/password/host entries to use env vars or placeholders, and add a
prominent comment at the top of the file warning that values are placeholders
and must be replaced or provided via environment variables before deployment.

Comment on lines +48 to +50
quota_handlers:
sqlite:
db_path: quota.sqlite
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use absolute path or document the working directory context for SQLite database.

The relative path quota.sqlite (line 50) may cause the database to be created in unpredictable locations depending on where the service is executed. For production deployments, this could lead to data loss or permission issues.

Apply this diff to clarify the path:

  sqlite:
-      db_path: quota.sqlite
+      db_path: /var/lib/lightspeed/quota.sqlite  # Adjust path as needed for your environment

Alternatively, add a comment explaining that the path is relative to the service's working directory and should be updated to an absolute path in production.

🤖 Prompt for AI Agents
In examples/quota-limiter-configuration-sqlite.yaml around lines 48-50, the
sqlite db_path uses a relative path ("quota.sqlite") which can create the DB in
unpredictable locations; update db_path to an absolute filesystem path
appropriate for the deployment (e.g., /var/lib/your-service/quota.sqlite) or, if
leaving it relative, add a clear comment above db_path stating that the path is
relative to the service's working directory and must be changed to an absolute
path for production to avoid data loss and permission issues.

@tisnik tisnik merged commit 166074d into lightspeed-core:main Oct 23, 2025
18 of 20 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 10, 2025
15 tasks
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.

1 participant