LCORE-511: Fix type checking issues in k8s authentication module#1329
Open
anik120 wants to merge 1 commit intolightspeed-core:mainfrom
Open
LCORE-511: Fix type checking issues in k8s authentication module#1329anik120 wants to merge 1 commit intolightspeed-core:mainfrom
anik120 wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
WalkthroughPyright config now includes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
anik120
force-pushed
the
fix-pyright-issues
branch
2 times, most recently
from
143b105 to
fc66a8d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/authentication/k8s.py`:
- Around line 102-105: Remove the trailing whitespace after the assert statement
in the singleton accessor so the line matching "assert cls._instance is not
None" has no extra spaces at the end; update the file's whitespace so the return
line ("return cls._instance # type: ignore[return-value]") remains unchanged
and run linters to confirm C0303 is resolved.
- Around line 169-172: The code currently coerces a possibly missing or None
clusterID into a string (cluster_id = str(version_data["spec"]["clusterID"])),
which can produce bogus values; change the logic in the function that reads
version_data so you explicitly validate that version_data is a dict, that "spec"
exists and contains a non-null, non-empty "clusterID" of the expected
type/format, and if it is missing/None/invalid raise ClusterIDUnavailableError
instead of coercing—use the symbols version_data, cluster_id,
"spec"["clusterID"], and ClusterIDUnavailableError to locate and implement this
check.
- Around line 247-249: The function currently declares a return type of
Optional[kubernetes.client.V1TokenReview] but actually returns response.status
(a V1TokenReviewStatus); update the function's return annotation from
Optional[kubernetes.client.V1TokenReview] to
Optional[kubernetes.client.V1TokenReviewStatus] and remove the two trailing
"type: ignore[union-attr]" comments on the response.status lines so the
signature and returned value types match (locate the function by the signature
declaring Optional[kubernetes.client.V1TokenReview] and the lines that reference
response.status).
- Around line 349-356: The __call__ method currently uses user_info.user.uid
(V1UserInfo.uid) without validating it; ensure uid is not None for
non-`kube:admin` flows before building ForbiddenResponse.endpoint or returning
the tuple: if user_info.user.uid is missing, construct response =
ForbiddenResponse.endpoint(user_id=...) with a clear placeholder or empty string
avoided (use the same pattern as the earlier ForbiddenResponse creation) and
raise HTTPException(**response.model_dump()); only after confirming uid is
present return (uid, user_info.user.username, ...) so the returned tuple matches
the declared tuple[str, str, bool, str] signature and downstream callers of
ForbiddenResponse.endpoint() never receive a None user_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccfc2777-95ce-4347-b61c-b6afdc827639
📒 Files selected for processing (2)
pyproject.tomlsrc/authentication/k8s.py
💤 Files with no reviewable changes (1)
- pyproject.toml
src/authentication/k8s.py
Outdated
Comment on lines
+102
to
+105
| # At this point _instance is guaranteed to be initialized | ||
| # but we need this anyway for type checker | ||
| assert cls._instance is not None | ||
| return cls._instance # type: ignore[return-value] |
Contributor
There was a problem hiding this comment.
Drop the trailing whitespace on Line 103.
This hunk is already failing CI on C0303.
🧰 Tools
🪛 GitHub Actions: Python linter
[error] 103-103: Pylint: Trailing whitespace detected (C0303).
🪛 GitHub Check: Bandit
[notice] 104-104:
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/authentication/k8s.py` around lines 102 - 105, Remove the trailing
whitespace after the assert statement in the singleton accessor so the line
matching "assert cls._instance is not None" has no extra spaces at the end;
update the file's whitespace so the return line ("return cls._instance # type:
ignore[return-value]") remains unchanged and run linters to confirm C0303 is
resolved.
src/authentication/k8s.py
Outdated
Comment on lines
+247
to
+249
| # Kubernetes client library has incomplete type stubs | ||
| if response.status.authenticated: # type: ignore[union-attr] | ||
| return response.status # type: ignore[union-attr] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python -m pip install --quiet kubernetes==30.1.0
python - <<'PY'
from kubernetes.client import V1TokenReview, V1TokenReviewStatus
print("V1TokenReview.status =", V1TokenReview.openapi_types.get("status"))
print("V1TokenReviewStatus.user =", V1TokenReviewStatus.openapi_types.get("user"))
PYRepository: lightspeed-core/lightspeed-stack
Length of output: 591
🏁 Script executed:
cat -n src/authentication/k8s.py | sed -n '230,260p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1514
🏁 Script executed:
cat -n src/authentication/k8s.py | sed -n '210,235p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1101
Fix the return type annotation to match what the function actually returns.
The function signature at line 222 declares return type Optional[kubernetes.client.V1TokenReview], but line 251 returns response.status, which is of type Optional[kubernetes.client.V1TokenReviewStatus]. This type mismatch forces the type: ignore[union-attr] suppressions and violates the coding guideline requiring complete type annotations.
Change the return type to Optional[kubernetes.client.V1TokenReviewStatus] and remove the type ignore comments:
Proposed fix
-def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReview]:
+def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReviewStatus]:
@@
- if response.status.authenticated: # type: ignore[union-attr]
- return response.status # type: ignore[union-attr]
+ status = response.status
+ if status is not None and status.authenticated:
+ return status🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/authentication/k8s.py` around lines 247 - 249, The function currently
declares a return type of Optional[kubernetes.client.V1TokenReview] but actually
returns response.status (a V1TokenReviewStatus); update the function's return
annotation from Optional[kubernetes.client.V1TokenReview] to
Optional[kubernetes.client.V1TokenReviewStatus] and remove the two trailing
"type: ignore[union-attr]" comments on the response.status lines so the
signature and returned value types match (locate the function by the signature
declaring Optional[kubernetes.client.V1TokenReview] and the lines that reference
response.status).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/authentication/k8s.py (3)
222-223:⚠️ Potential issue | 🟠 MajorFix
get_user_inforeturn annotation to match the actual returned model.Line 222 declares
Optional[V1TokenReview], but Lines 250-251 returnresponse.status(V1TokenReviewStatus). This is a concrete type mismatch.Proposed fix
-def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReview]: +def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReviewStatus]: @@ - if response.status.authenticated: # type: ignore[union-attr] - return response.status # type: ignore[union-attr] + status = response.status + if status is not None and status.authenticated: + return status#!/bin/bash # Verify declared return type and concrete returned expression in src/authentication/k8s.py set -euo pipefail python - <<'PY' import ast, pathlib p = pathlib.Path("src/authentication/k8s.py") tree = ast.parse(p.read_text()) for n in tree.body: if isinstance(n, ast.FunctionDef) and n.name == "get_user_info": print("Function:", n.name) print("Declared return:", ast.unparse(n.returns) if n.returns else None) for r in [x for x in ast.walk(n) if isinstance(x, ast.Return) and x.value is not None]: print("Return expr:", ast.unparse(r.value)) break PYAs per coding guidelines, "Use complete type annotations for function parameters and return types".
Also applies to: 249-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 222 - 223, The declared return type of get_user_info is incorrect: it currently says Optional[V1TokenReview] but the function returns response.status (a V1TokenReviewStatus or None). Update the function return annotation to Optional[kubernetes.client.V1TokenReviewStatus] (or V1TokenReviewStatus if you guarantee non-None) and add/import the V1TokenReviewStatus symbol if not already imported; adjust any related type hints/comments in get_user_info and its callers to match the concrete returned model.
351-360:⚠️ Potential issue | 🟠 MajorGuard
user_info.user.uidbefore using it in forbidden responses and the return tuple.At Lines 353-360,
uidis treated as alwaysstr, but the Kubernetes user model can carryNone. That violatestuple[str, str, bool, str]and can propagate invalid IDs toForbiddenResponse.endpoint.Proposed fix
+ user_uid = user_info.user.uid # type: ignore[union-attr] + if not isinstance(user_uid, str) or not user_uid: + response = InternalServerErrorResponse( + response="Internal server error", + cause="Authenticated Kubernetes user is missing a UID", + ) + raise HTTPException(**response.model_dump()) + # Kubernetes client library has incomplete type stubs if not response.status.allowed: # type: ignore[union-attr] response = ForbiddenResponse.endpoint( - user_id=user_info.user.uid # type: ignore[union-attr] + user_id=user_uid ) raise HTTPException(**response.model_dump()) return ( - user_info.user.uid, # type: ignore[union-attr] + user_uid, user_info.user.username, # type: ignore[union-attr] self.skip_userid_check, token, )#!/bin/bash # Verify declared tuple contract and direct uid usages in __call__ set -euo pipefail rg -n -C3 'async def __call__|tuple\[str, str, bool, str\]|user_info\.user\.uid|ForbiddenResponse\.endpoint' src/authentication/k8s.pyAs per coding guidelines, "Use complete type annotations for function parameters and return types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 351 - 360, The code uses user_info.user.uid directly in ForbiddenResponse.endpoint and the return tuple (expected tuple[str,str,bool,str]) but uid can be None; update the __call__ flow to guard and normalize uid: check that user_info.user and user_info.user.uid are present and a str before calling ForbiddenResponse.endpoint or including it in the returned tuple, and if not present create an appropriate error response (e.g., construct ForbiddenResponse.endpoint or raise HTTPException) with a clear fallback/diagnostic rather than passing None; adjust any type assertions/comments around user_info.user.uid and ensure the returned tuple values satisfy the declared types.
171-175:⚠️ Potential issue | 🟠 MajorFail closed on invalid
clusterIDinstead of coercing it tostr.At Line 174,
str(version_data["spec"]["clusterID"])can turn malformed values (e.g.,None) into cacheable"None", which bypasses the intended error path. Validatespec.clusterIDas a non-empty string before caching.Proposed fix
- # Kubernetes client library returns untyped dict-like objects - cluster_id = str(version_data["spec"]["clusterID"]) # type: ignore[index] + # Kubernetes client library returns untyped dict-like objects + spec = version_data.get("spec") + cluster_id = spec.get("clusterID") if isinstance(spec, dict) else None + if not isinstance(cluster_id, str) or not cluster_id.strip(): + raise ClusterIDUnavailableError("Failed to get cluster ID") cls._cluster_id = cluster_id return cluster_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 171 - 175, The code currently coerces version_data["spec"]["clusterID"] with str(...) which can convert invalid values like None into "None" and cache them; instead, read and validate the nested keys from version_data (ensure "spec" exists and is a dict, and "clusterID" exists), check that the cluster_id value is a non-empty string (e.g., isinstance(value, str) and value.strip() != ""), and only then assign cls._cluster_id = cluster_id; if the check fails, raise a ValueError or skip caching so invalid values are not stored. Use the existing variables/version_data and cls._cluster_id to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/authentication/k8s.py`:
- Around line 222-223: The declared return type of get_user_info is incorrect:
it currently says Optional[V1TokenReview] but the function returns
response.status (a V1TokenReviewStatus or None). Update the function return
annotation to Optional[kubernetes.client.V1TokenReviewStatus] (or
V1TokenReviewStatus if you guarantee non-None) and add/import the
V1TokenReviewStatus symbol if not already imported; adjust any related type
hints/comments in get_user_info and its callers to match the concrete returned
model.
- Around line 351-360: The code uses user_info.user.uid directly in
ForbiddenResponse.endpoint and the return tuple (expected
tuple[str,str,bool,str]) but uid can be None; update the __call__ flow to guard
and normalize uid: check that user_info.user and user_info.user.uid are present
and a str before calling ForbiddenResponse.endpoint or including it in the
returned tuple, and if not present create an appropriate error response (e.g.,
construct ForbiddenResponse.endpoint or raise HTTPException) with a clear
fallback/diagnostic rather than passing None; adjust any type
assertions/comments around user_info.user.uid and ensure the returned tuple
values satisfy the declared types.
- Around line 171-175: The code currently coerces
version_data["spec"]["clusterID"] with str(...) which can convert invalid values
like None into "None" and cache them; instead, read and validate the nested keys
from version_data (ensure "spec" exists and is a dict, and "clusterID" exists),
check that the cluster_id value is a non-empty string (e.g., isinstance(value,
str) and value.strip() != ""), and only then assign cls._cluster_id =
cluster_id; if the check fails, raise a ValueError or skip caching so invalid
values are not stored. Use the existing variables/version_data and
cls._cluster_id to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae95568a-8cda-4783-9c7f-55431321965e
📒 Files selected for processing (2)
pyproject.tomlsrc/authentication/k8s.py
💤 Files with no reviewable changes (1)
- pyproject.toml
anik120
force-pushed
the
fix-pyright-issues
branch
from
fc66a8d to
8ac19bc
Compare
jrobertboos
reviewed
Mar 16, 2026
Contributor
jrobertboos
left a comment
jrobertboos
left a comment
There was a problem hiding this comment.
Overall LGTM and my comments r just nits/personal preference so feel free to merge (I am just not a fan of assert in prod code) :)
Fix type checking errors in `src/authentication/k8s.py` caused by incomplete type annotations in the Kubernetes Python client library. The Kubernetes Python client library (`kubernetes-client`) has incomplete type annotations for many of its model classes and API responses. This PR adds targeted type ignore comments to suppress false positives while maintaining type safety for the rest of the codebase. Changes: - Added explicit type conversions for `k8s_config.host` and `k8s_config.ssl_ca_cert` to ensure proper string types - Added `# type: ignore` directives for Kubernetes client objects that lack proper type stubs: - `V1TokenReview.status` and `.user` attributes - `V1SubjectAccessReview.status.allowed` attribute - Custom object API responses (dict-like objects with dynamic structure) - Added type assertions to help Pyright understand singleton initialization pattern Note: There is no functional changes to authentication logic being introduced in this PR, code behaviour remains identical. Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
anik120
force-pushed
the
fix-pyright-issues
branch
from
8ac19bc to
68fbc44
Compare
Contributor
Author
+100 thanks for pointing out the pythonic way to solve this! Addressed them. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Fix type checking errors in
src/authentication/k8s.pycaused by incomplete type annotations in the Kubernetes Python client library.The Kubernetes Python client library (
kubernetes-client) has incomplete type annotations for many of its model classes and API responses. This PR adds targeted type ignore comments to suppress false positives while maintaining type safety for the rest of the codebase.Note: There is no functional changes to authentication logic being introduced in this PR, code behaviour remains identical.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit