Skip to content

Conversation

@raptorsun
Copy link
Contributor

@raptorsun raptorsun commented Aug 12, 2025

Description

Documentation on how to configurate authentication with the 4 modules: k8s, jwk-token, noop, noop-token.

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

Related Tickets & Documents

  • Related Issue #
  • Closes # LCORE-429

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Documentation
    • Overhauled README table of contents with a new top-level section and reorganized project/configuration hierarchy, including a dedicated Authentication subtree.
    • Added a comprehensive Authentication guide covering supported methods (K8s, JSON Web Keyset, No-op and No-op-with-token), configuration options, usage notes, and example YAML snippets.
    • Reformatted several ToC entries for improved hierarchy and readability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 12, 2025

Walkthrough

README.md updated: Table of Contents reorganized (new top-level lightspeed-stack and About The Project); Authentication section added at the document bottom describing k8s, jwk-token, noop, and noop-with-token modules with YAML examples; purely documentation changes, no code/API edits. (49 words)

Changes

Cohort / File(s) Summary of Changes
Documentation: README restructure & auth docs
README.md
Overhauled Table of Contents (added top-level lightspeed-stack, "About The Project", restructured indentation/hierarchy); added a comprehensive Authentication section (modules: k8s, jwk-token, noop, noop-with-token) including YAML/config examples and fields (k8s_cluster_api, k8s_ca_cert_path, skip_tls_verification, jwk_config, jwt_configuration, etc.). Documentation appears duplicated in places; no code or public API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • LCORE-304: Sync docs #238 — Adds the AuthenticationConfiguration model and related config entries (k8s/JWK/noop), closely matching the README's new authentication documentation.

Suggested reviewers

  • tisnik

Poem

I hopped through headings, nibbling lines of lore,
Brought k8s, JWK, and no-op through the door.
Two tunnels carved where one might do—
YAML crumbs to follow, shiny and new.
A whisker twitch, the docs now gleam—happy stream! 🐇✨

✨ Finishing Touches
🧪 Generate unit tests
  • 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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 2

🧹 Nitpick comments (3)
README.md (3)

256-276: Fix typo (“neccessary”) and align list style; add security nudge on TLS.

  • Typo: “neccessary” → “necessary”.
  • Lists should use asterisks (markdownlint MD004).
  • Add a short caution that skip_tls_verification should be used only for dev; prefer setting k8s_ca_cert_path.

Apply this diff:

-   This step is not neccessary.
+   This step is not necessary.
-   When running outside a kubernetes cluster or connecting to external Kubernetes clusters, Lightspeed Stack requires the cluster connection details in the configuration file: 
-   - `k8s_cluster_api` Kubernetes Cluster API URL. The URL of the K8S/OCP API server where tokens are validated.
-   - `k8s_ca_cert_path` Path to the CA certificate file for clusters with self-signed certificates.
-   - `skip_tls_verification` Whether to skip TLS verification.
+   When running outside a Kubernetes cluster or connecting to external Kubernetes clusters, Lightspeed Stack requires the cluster connection details in the configuration file:
+   * `k8s_cluster_api` Kubernetes Cluster API URL. The URL of the K8S/OCP API server where tokens are validated.
+   * `k8s_ca_cert_path` Path to the CA certificate file for clusters with self-signed certificates.
+   * `skip_tls_verification` Whether to skip TLS verification (use only for development; prefer configuring `k8s_ca_cert_path`).

300-313: Grammar fix (“modules”), list style (MD004), and add a production-safety note.

  • “2 authentication module” → “2 authentication modules”.
  • Use asterisks for unordered lists.
  • Recommend adding a note that these are for development/testing and should not be used in production.

Apply this diff:

-Lightspeed Stack provides 2 authentication module to bypass the authentication and authorization checks:
-- `noop` No operation authentication (default)
-- `noop-with-token` No operation authentication accepting a bearer token
+Lightspeed Stack provides 2 authentication modules to bypass authentication and authorization checks:
+* `noop` No operation authentication (default)
+* `noop-with-token` No operation authentication accepting a bearer token
+
+> Note: These modules should be used for development or trusted environments only. Do not use in production deployments.

242-313: Add a short “Verification” subsection with concrete curl examples for each module.

The PR description asked for testing steps and verification details. Adding a minimal, copy-pasteable section improves operator experience.

Proposed insertion after the No-op section:

+### Verification
+
+Below are quick checks to validate your authentication setup:
+
+#### K8s module
+1. Obtain a Kubernetes bearer token with `get` permission on `/ls-access`.
+2. Call the service:
+   ```bash
+   curl -H "Authorization: Bearer ${K8S_TOKEN}" http://HOST:PORT/v1/liveness
+   ```
+3. Expect 200 OK if token is valid and RBAC allows access.
+
+#### JWK (jwk-token) module
+1. Obtain a JWT compatible with your JWK issuer and claims mapping.
+2. Call the service:
+   ```bash
+   curl -H "Authorization: Bearer ${JWT}" http://HOST:PORT/v1/liveness
+   ```
+3. Expect 200 OK if signature/claims validate against the configured JWK.
+
+#### No-op module
+```bash
+curl http://HOST:PORT/v1/liveness
+```
+Expect 200 OK (no token required).
+
+#### No-op-with-token module
+```bash
+curl -H "Authorization: Bearer any-token" http://HOST:PORT/v1/liveness
+```
+Expect 200 OK (token is accepted but not validated).

I can open a follow-up PR to add this if you prefer.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90f4c20 and 026519b.

📒 Files selected for processing (1)
  • README.md (2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md

245-245: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


246-246: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


247-247: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


248-248: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


267-267: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


268-268: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


269-269: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


283-283: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


284-284: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


285-285: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


286-286: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


287-287: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


288-288: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


303-303: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


304-304: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)

🔇 Additional comments (2)
README.md (2)

16-33: ToC additions look fine; anchors match new sections.

The new Authentication subtree and re-indentation are coherent, and anchors appear consistent with headings below.


283-299: Documentation is accurate and consistent—no changes needed

  • The module name jwk-token matches AUTH_MOD_JWK_TOKEN in src/constants.py.
  • The jwk_config and jwt_configuration keys are defined in src/models/config.py and referenced in src/auth/jwk_token.py.
  • Unordered lists in this section use dashes (-), which is consistent with other lists in README.md; there’s no MD004 violation.

@tisnik
Copy link
Contributor

tisnik commented Aug 12, 2025

@raptorsun please rebase as @onmete made changes in README in between

Signed-off-by: Haoyu Sun <hasun@redhat.com>
@raptorsun
Copy link
Contributor Author

rebased now @tisnik
PTAL

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: 0

🧹 Nitpick comments (5)
README.md (5)

246-247: Hyphenate the section title for readability and consistency.

“K8s based” → “K8s-based”.

-### K8s based authentication
+### K8s-based authentication

251-276: Clarify ServiceAccount binding details and scope for non-resource URLs.

  • Non-resource URLs are granted via ClusterRole (you used ClusterRole correctly).
  • If using a ServiceAccount subject, namespace is required.

Add a short note after the YAML block to prevent misconfiguration.

 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
@@
     apiGroup: rbac.authorization.k8s.io

+Note: When binding a ServiceAccount, include its namespace, for example:
+
+```
+subjects:

    • kind: ServiceAccount
  • name: SOME_SA
  • namespace: SOME_NAMESPACE
    +```

---

`279-298`: **Fix grammar, capitalization, and unordered list style (MD004).**

- Spelling: “neccessary” → “necessary”
- Capitalization: “kubernetes” → “Kubernetes”
- Clarify wording about in-cluster vs kubeconfig
- Use asterisks for bullets per markdownlint project rules


```diff
 2. Configure the Kubernetes authentication settings. 
-   When deploying Lightspeed Stack in a Kubernetes cluster, it is not required to specify cluster connection details.
-   It automatically picks up the in-cluster configuration or through a kubeconfig file.
-   This step is not neccessary.
-   When running outside a kubernetes cluster or connecting to external Kubernetes clusters, Lightspeed Stack requires the cluster connection details in the configuration file: 
-   - `k8s_cluster_api` Kubernetes Cluster API URL. The URL of the K8S/OCP API server where tokens are validated.
-   - `k8s_ca_cert_path` Path to the CA certificate file for clusters with self-signed certificates.
-   - `skip_tls_verification` Whether to skip TLS verification.
+   When deploying Lightspeed Stack in a Kubernetes cluster, you typically don’t need to specify cluster connection details.
+   It will automatically use the in-cluster configuration or your kubeconfig file.
+   No additional configuration is necessary in that case.
+   When running outside a Kubernetes cluster or connecting to an external Kubernetes cluster, specify the following in the configuration file:
+   * `k8s_cluster_api` Kubernetes API server URL where tokens are validated (K8s/OCP).
+   * `k8s_ca_cert_path` Path to the CA certificate file for clusters with self-signed certificates.
+   * `skip_tls_verification` Whether to skip TLS verification (only for development; not recommended for production).

304-311: Switch unordered list markers to asterisks (MD004) and tighten wording.

Align with repository markdownlint rules and tighten phrasing slightly.

-To configure JWK based authentication, you need to specify the following settings in the configuration file:
-- `module` must be set to `jwk-token`
-- `jwk_config` JWK configuration settings must set at least `url` field:
-  - `url`: The URL of the JWK endpoint.
-  - `jwt_configuration`: JWT configuration settings.
-    - `user_id_claim`: The key of the user ID in JWT claim.
-    - `username_claim`: The key of the username in JWT claim.
+To configure JWK-based authentication, specify the following settings:
+* `module` must be set to `jwk-token`
+* `jwk_config` (JWK settings) must set at least the `url` field:
+  * `url`: The URL of the JWK endpoint.
+  * `jwt_configuration`: JWT settings.
+    * `user_id_claim`: The JWT claim key that contains the user ID.
+    * `username_claim`: The JWT claim key that contains the username.

324-335: Fix pluralization, add production caution, and switch bullet style (MD004).

  • “2 authentication module” → “2 authentication modules”
  • Warn that these are not for production unless explicitly intended
  • Use asterisks for bullets
-Lightspeed Stack provides 2 authentication module to bypass the authentication and authorization checks:
-- `noop` No operation authentication (default)
-- `noop-with-token` No operation authentication accepting a bearer token
+Lightspeed Stack provides 2 authentication modules to bypass authentication and authorization checks (intended primarily for development/testing):
+* `noop` No operation authentication (default)
+* `noop-with-token` No operation authentication accepting a bearer token
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 026519b and e3bc9b3.

📒 Files selected for processing (1)
  • README.md (2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md

289-289: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


290-290: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


291-291: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


305-305: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


306-306: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


307-307: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


308-308: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


309-309: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


310-310: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


325-325: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)


326-326: Unordered list style
Expected: asterisk; Actual: dash

(MD004, ul-style)

⏰ 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). (1)
  • GitHub Check: e2e_tests
🔇 Additional comments (3)
README.md (3)

240-245: Module list LGTM; naming is consistent with body (jwk-token).

The supported modules list matches the sections below and resolves the previous jwt/jwk inconsistency.


16-33: README TOC is already up to date with main
Your branch coderabbit_388 is rebased onto the latest main—no further action needed.


312-320: No issuer/audience fields in JWK configuration
A search of JwkConfiguration and JwkTokenAuthDependency shows only url and the nested jwt_configuration (user_id_claim/username_claim) are supported. There are no issuer or audience settings in code, so the README snippet correctly reflects the implementation—no changes necessary.

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

* [Prerequisites](#prerequisites)
* [Installation](#installation)
* [Configuration](#configuration)
* [Integration with Llama Stack](#integration-with-llama-stack)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: these changes will be rewritten by vim-markdown-toc tool (but whatever)

@tisnik tisnik merged commit e02198e into lightspeed-core:main Aug 12, 2025
18 checks passed
@raptorsun raptorsun deleted the lcore-429 branch August 13, 2025 07:37
@coderabbitai coderabbitai bot mentioned this pull request Oct 1, 2025
18 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.

2 participants