LCORE-511: Fix type checking issues in k8s authentication module#1329
LCORE-511: Fix type checking issues in k8s authentication module#1329tisnik merged 1 commit intolightspeed-core:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPyright now type-checks Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as AuthenticationService
participant K8sAPI as KubernetesAPI
participant Authz as KubernetesAuthz
Client->>App: calls endpoint with token
App->>App: get_user_info(token)
App->>K8sAPI: create TokenReview (token)
K8sAPI-->>App: TokenReviewStatus (user, uid?, groups)
App->>App: validate uid and cluster id (from config/API)
App->>Authz: create SubjectAccessReview (user, groups, verb, resource)
Authz-->>App: SAR status (allowed/denied)
App-->>Client: 200 OK / 403 Forbidden (uses validated uid/cluster info)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
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
| # 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] |
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.
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
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) :)
anik120
force-pushed
the
fix-pyright-issues
branch
from
8ac19bc to
68fbc44
Compare
+100 thanks for pointing out the pythonic way to solve this! Addressed them. |
src/authentication/k8s.py
Outdated
| cluster_id = version_data["spec"]["clusterID"] | ||
| # Type validation: ensure we got a dict-like object | ||
| if not isinstance(version_data, dict): | ||
| raise TypeError( |
There was a problem hiding this comment.
can it happen in reality? How the TypeError be handled by the rest of the service?
There was a problem hiding this comment.
This was only added for satisfying the type checker, since we know the kube library's get_cluster_custom_object() always returns a dict when successful. If the API call fails, it raises ApiException.
But this was before I learned about casting, so changing all of this to just
version_data = cast(dict, custom_objects_api.get_cluster_custom_object(...))
:)
src/authentication/k8s.py
Outdated
| not in {None, Path()} | ||
| else k8s_config.ssl_ca_cert | ||
| ) | ||
| if ca_cert_path and ca_cert_path != Path(): |
There was a problem hiding this comment.
will empty path works or not?
There was a problem hiding this comment.
Apologies, I simplified this check now. Since ca_cert_path is Optional[FilePath], Pydantic will validate that it's either None or a valid file path. And then the check becomes: if ca_cert_path: and that's it.
There was a problem hiding this comment.
ok, I guess I'll add is not None later (to be super clear what is going on)
There was a problem hiding this comment.
@anik120 you will be doing changes in rest of PR, so pls. explicitly test for None or not None. Because just if X have suprising meanings in Python (unfortunately this language is pretty weak in this area)
There was a problem hiding this comment.
@tisnik sorry I missed this comment earlier, addressed it in the latest push, PTAL
anik120
force-pushed
the
fix-pyright-issues
branch
from
68fbc44 to
f8f7e35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/authentication/k8s.py (1)
320-347:⚠️ Potential issue | 🟠 MajorValidate
usernamebefore using it in the auth flow.
uidis now guarded (lines 333–340), butuser_info.user.usernameis still dereferenced without validation at lines 321, 346, and 370. Since bothuserandusernameare optional in Kubernetes 30.1.0 models (user=None,username=None), the code will raiseAttributeErrorbefore the UID guard if either is missing. More critically, line 370 returnsusernamein atuple[str, str, bool, str]contract without ensuring it is a non-empty string.Add an upstream validation block matching the pattern used for
uid:+ # Kubernetes client library has incomplete type stubs + user = user_info.user # type: ignore[union-attr] + if user is None or not isinstance(user.username, str) or not user.username: + logger.error("Authenticated Kubernetes user is missing a username") + response = InternalServerErrorResponse( + response="Internal server error", + cause="Authenticated Kubernetes user is missing a username", + ) + raise HTTPException(**response.model_dump()) + username = user.username + - # Kubernetes client library has incomplete type stubs - if user_info.user.username == "kube:admin": # type: ignore[union-attr] + if username == "kube:admin": try: - user_info.user.uid = K8sClientSingleton.get_cluster_id() # type: ignore[union-attr] + user.uid = K8sClientSingleton.get_cluster_id()Then replace all subsequent
user_info.user.usernameanduser_info.userreferences with the guarded local variablesusernameanduser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 320 - 347, The code uses user_info.user.username directly (e.g., in the "kube:admin" check, V1SubjectAccessReview creation, and the tuple return) without validation; add an upstream guard mirroring the uid guard: extract user = user_info.user and username = user.username, validate that username is a non-empty string (log and raise the same InternalServerErrorResponse/HTTPException on failure), and use these local variables everywhere instead of dereferencing user_info.user.username or user_info.user; update the "kube:admin" check to use username, the SAR construction to use username and user.groups, and the tuple return to return the validated username so the function contract remains satisfied.
🤖 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 363-365: The code reads response.status.allowed without ensuring
response.status is present, which can raise AttributeError; update the check in
the function handling the SelfSubjectAccessReview response so it first guards
response.status (e.g., if not response.status or not response.status.allowed)
before accessing .allowed and then set response =
ForbiddenResponse.endpoint(user_id=user_uid) when the guard fails; ensure this
replaces the current unchecked "if not response.status.allowed" usage so
incomplete Kubernetes responses are handled safely.
---
Outside diff comments:
In `@src/authentication/k8s.py`:
- Around line 320-347: The code uses user_info.user.username directly (e.g., in
the "kube:admin" check, V1SubjectAccessReview creation, and the tuple return)
without validation; add an upstream guard mirroring the uid guard: extract user
= user_info.user and username = user.username, validate that username is a
non-empty string (log and raise the same
InternalServerErrorResponse/HTTPException on failure), and use these local
variables everywhere instead of dereferencing user_info.user.username or
user_info.user; update the "kube:admin" check to use username, the SAR
construction to use username and user.groups, and the tuple return to return the
validated username so the function contract remains satisfied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 208e000a-526c-4afb-ae66-09085c99fc69
📒 Files selected for processing (2)
pyproject.tomlsrc/authentication/k8s.py
💤 Files with no reviewable changes (1)
- pyproject.toml
| ) | ||
| cluster_id = spec.get("clusterID") | ||
| if not isinstance(cluster_id, str) or not cluster_id.strip(): | ||
| raise ClusterIDUnavailableError( |
There was a problem hiding this comment.
I noticed that ClusterIDUnavailableError is raised a few times in this module. It appears to be a leftover from road-core. Currently, the only place where it’s caught is in the block starting at L324, which doesn’t make use of the error message and instead wraps everything into a generic 500 response.
I realize this is outside the scope of the current task, but if you’re open to it, I think it would be beneficial to:
- avoid catching broad
Exceptions - differentiate between specific error cases
- standardize error handling under the
UnauthorizedResponsemodel (create customcauses and add corresponding examples to the model'smodel_config)
@tisnik WDYT?
anik120
force-pushed
the
fix-pyright-issues
branch
from
f8f7e35 to
b924fd3
Compare
|
Thanks for the review @asimurka! I think I managed to remove all type:ignore statements now, PTAL Regarding the I am starting to work on the new PR right now, will send a review request as soon as it's ready. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/authentication/k8s.py (2)
322-327:⚠️ Potential issue | 🟠 MajorValidate the TokenReview user payload before using it.
cast()only silences pyright here. If Kubernetes returns an authenticated status with a partial user object, Lines 325 and 350 can raiseAttributeError, and Line 377 can still returnNonethrough a declaredstr. Please guarduser_info.useranduser.usernameonce, then use validatedusername/user_uidlocals throughout.🛠️ Suggested fix
- # Cast user to proper type for type checking - user = cast(kubernetes.client.V1UserInfo, user_info.user) + user = user_info.user + if user is None or not isinstance(user.username, str) or not user.username: + logger.error("Authenticated Kubernetes user is missing a username") + response = InternalServerErrorResponse( + response="Internal server error", + cause="Authenticated Kubernetes user is missing a username", + ) + raise HTTPException(**response.model_dump()) + + username = user.username - if user.username == "kube:admin": + if username == "kube:admin": try: - user.uid = K8sClientSingleton.get_cluster_id() + user_uid = K8sClientSingleton.get_cluster_id() except ClusterIDUnavailableError as e: logger.error("Failed to get cluster ID: %s", e) response = InternalServerErrorResponse( response="Internal server error", cause="Unable to retrieve cluster ID", ) raise HTTPException(**response.model_dump()) from e + else: + user_uid = user.uid # Validate that uid is present and is a string - user_uid = user.uid if not isinstance(user_uid, str) or not user_uid: logger.error("Authenticated Kubernetes user is missing a UID") response = InternalServerErrorResponse( response="Internal server error", cause="Authenticated Kubernetes user is missing a UID", ) raise HTTPException(**response.model_dump()) @@ - user=user.username, + user=username, groups=user.groups, @@ - username = cast(str, user.username) return ( user_uid, username,Run this to verify the Kubernetes models still allow these fields to be missing at runtime:
#!/bin/bash set -euo pipefail python -m pip install --quiet kubernetes==30.1.0 python - <<'PY' import inspect from kubernetes.client import V1TokenReviewStatus, V1UserInfo print("V1TokenReviewStatus.__init__ =", inspect.signature(V1TokenReviewStatus.__init__)) print("V1UserInfo.__init__ =", inspect.signature(V1UserInfo.__init__)) PY sed -n '320,380p' src/authentication/k8s.py | cat -nExpected result: both signatures show optional
user/usernameparameters, confirming the cast is not a runtime guard.Also applies to: 336-344, 350-351, 377-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 322 - 327, The TokenReview handler currently casts user_info.user and then dereferences attributes (user.username, user.uid) which can be None at runtime; instead, first check that user_info is not None and that user_info.user exists, then extract validated locals like username = getattr(user_info.user, "username", None) and user_uid = getattr(user_info.user, "uid", None) and use those locals throughout (replace direct uses of user.username/user.uid and the cast), and handle the None cases explicitly (early return or fallback) where K8sClientSingleton.get_cluster_id() or subsequent logic expects a non-None str; update all spots referenced (uses around user_info/user.username at the blocks using K8sClientSingleton.get_cluster_id(), the later comparisons and the final return) to rely on these validated locals.
370-374:⚠️ Potential issue | 🟠 MajorDon't cast away a missing SAR status.
Line 371 can still be
Noneat runtime, so Line 373 may raiseAttributeErroroutside thetry/exceptand turn an incomplete SubjectAccessReview response into an unhandled 500. Please keep an explicitNoneguard before reading.allowed.🛠️ Suggested fix
- sar_status = cast( - kubernetes.client.V1SubjectAccessReviewStatus, sar_response.status - ) - if not sar_status.allowed: + sar_status = sar_response.status + if sar_status is None: + logger.error("SubjectAccessReview response is missing status") + response = ServiceUnavailableResponse( + backend_name="Kubernetes API", + cause="Unable to perform authorization check", + ) + raise HTTPException(**response.model_dump()) + if not sar_status.allowed: response = ForbiddenResponse.endpoint(user_id=user_uid) raise HTTPException(**response.model_dump())Run this to verify the client model still permits
status=None:#!/bin/bash set -euo pipefail python -m pip install --quiet kubernetes==30.1.0 python - <<'PY' import inspect from kubernetes.client import V1SubjectAccessReview print("V1SubjectAccessReview.__init__ =", inspect.signature(V1SubjectAccessReview.__init__)) PY sed -n '370,375p' src/authentication/k8s.py | cat -nExpected result: the constructor signature includes
status=None, so the cast does not remove this runtime path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/k8s.py` around lines 370 - 374, The code currently casts sar_response.status to sar_status and immediately accesses sar_status.allowed which can be None at runtime; change the logic around V1SubjectAccessReview handling so you explicitly guard for a missing status before reading .allowed: check if sar_response.status is None or (sar_response.status and not sar_response.status.allowed), and in those cases set response = ForbiddenResponse.endpoint(user_id=user_uid); only read .allowed after confirming sar_response.status is not None (use sar_response.status rather than an unconditional cast to sar_status) so you avoid AttributeError when status is absent.
🤖 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 322-327: The TokenReview handler currently casts user_info.user
and then dereferences attributes (user.username, user.uid) which can be None at
runtime; instead, first check that user_info is not None and that user_info.user
exists, then extract validated locals like username = getattr(user_info.user,
"username", None) and user_uid = getattr(user_info.user, "uid", None) and use
those locals throughout (replace direct uses of user.username/user.uid and the
cast), and handle the None cases explicitly (early return or fallback) where
K8sClientSingleton.get_cluster_id() or subsequent logic expects a non-None str;
update all spots referenced (uses around user_info/user.username at the blocks
using K8sClientSingleton.get_cluster_id(), the later comparisons and the final
return) to rely on these validated locals.
- Around line 370-374: The code currently casts sar_response.status to
sar_status and immediately accesses sar_status.allowed which can be None at
runtime; change the logic around V1SubjectAccessReview handling so you
explicitly guard for a missing status before reading .allowed: check if
sar_response.status is None or (sar_response.status and not
sar_response.status.allowed), and in those cases set response =
ForbiddenResponse.endpoint(user_id=user_uid); only read .allowed after
confirming sar_response.status is not None (use sar_response.status rather than
an unconditional cast to sar_status) so you avoid AttributeError when status is
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c42666e4-981c-470a-920e-4add8874322c
📒 Files selected for processing (2)
pyproject.tomlsrc/authentication/k8s.py
💤 Files with no reviewable changes (1)
- pyproject.toml
src/authentication/k8s.py
Outdated
| ) | ||
| raise HTTPException(**response.model_dump()) from e | ||
|
|
||
| # Validate that uid is present and is a string |
There was a problem hiding this comment.
I think this entire code block is unnecessary. The return value of get_cluster_id (assigned to user.uid) is always a string, so the additional handling doesn’t seem needed. Also, this logic wasn’t present in the original implementation, which suggests it may be redundant.
There was a problem hiding this comment.
Yes this is basically another cast. Moving this down above the return statement and making this a simple cast also works.
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
b924fd3 to
b4d3d65
Compare
4 participants
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