BYOIDC: unprivileged user login using secrets from cluster#892
BYOIDC: unprivileged user login using secrets from cluster#892dbasunag merged 5 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/lgtm', '/wip', '/hold', '/cherry-pick', '/verified'} |
📝 WalkthroughWalkthroughAdd BYOIDC support: new kubeconfig fixture; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
utilities/user_utils.py (2)
117-147: Simplify error handling inget_oidc_tokensThe core flow looks good, but the error handling can be tightened:
except Exception as e: raise eis redundant; it adds no behavior over letting the exception propagate, and static analysis (TRY203/TRY201) rightly flags it.- The
AssertionErrorfor missing tokens is fine for test infra, but you don’t need atryaround the whole block just for that.You can simplify to:
def get_oidc_tokens(admin_client: DynamicClient, username: str, password: str) -> tuple[str, str]: @@ - try: - LOGGER.info(f"Requesting token for user {username} in byoidc environment") - response = requests.post( - url=url, - headers=headers, - data=data, - allow_redirects=True, - timeout=30, - verify=True, # Set to False if you need to skip SSL verification - ) - response.raise_for_status() - json_response = response.json() - - # Validate that we got an access token - if "id_token" not in json_response or "refresh_token" not in json_response: - LOGGER.error("Warning: No id_token or refresh_token in response") - raise AssertionError(f"No id_token or refresh_token in response: {json_response}") - return json_response["id_token"], json_response["refresh_token"] - except Exception as e: - raise e + LOGGER.info(f"Requesting token for user {username} in byoidc environment") + response = requests.post( + url=url, + headers=headers, + data=data, + allow_redirects=True, + timeout=30, + verify=True, # Set to False if you need to skip SSL verification + ) + response.raise_for_status() + json_response = response.json() + + # Validate that we got an access token + if "id_token" not in json_response or "refresh_token" not in json_response: + LOGGER.error("Warning: No id_token or refresh_token in response") + raise AssertionError(f"No id_token or refresh_token in response: {json_response}") + return json_response["id_token"], json_response["refresh_token"]This keeps behavior identical while removing unnecessary noise.
151-156: Guardrails aroundget_byoidc_issuer_urland duplicationThe helper correctly mirrors the pattern used elsewhere, but it mixes a few strong assumptions:
- It assumes
authentication.instance.spec.oidcProvidersis a non-empty list and indexes[0].- It uses bare
assertstatements, which give limited context if misconfigured.For slightly clearer failures in misconfigured clusters and easier reuse, consider:
def get_byoidc_issuer_url(admin_client: DynamicClient) -> str: - authentication = get_cluster_authentication(admin_client=admin_client) - assert authentication is not None - url = authentication.instance.spec.oidcProviders[0].issuer.issuerURL - assert url is not None - return url + authentication = get_cluster_authentication(admin_client=admin_client) + if authentication is None: + raise RuntimeError("Cluster Authentication resource 'cluster' not found; cannot resolve BYOIDC issuer URL") + + providers = getattr(authentication.instance.spec, "oidcProviders", None) or [] + if not providers: + raise RuntimeError("No OIDC providers configured on Authentication resource; cannot resolve BYOIDC issuer URL") + + url = providers[0].issuer.issuerURL + if not url: + raise RuntimeError("OIDC provider issuerURL is empty; cannot resolve BYOIDC issuer URL") + return urlAlso, if
tests/model_registry/utils.pystill defines a duplicateget_byoidc_issuer_url, consider switching it to import this shared helper to avoid divergence over time.tests/conftest.py (2)
400-416: Clarify type hints and error path for missing unprivileged userIn
unprivileged_client, you now correctly raise when an unprivileged client is requested butnon_admin_user_passwordisNone. One small polish:
- The fixture
non_admin_user_passwordreturnstuple[str, str] | None, but the parameter here is annotated as justtuple[str, str]. Aligning the type hint improves static checking and communicates theNone-guard more clearly.For example:
-def unprivileged_client( +def unprivileged_client( admin_client: DynamicClient, use_unprivileged_client: bool, kubconfig_filepath: str, - non_admin_user_password: tuple[str, str], + non_admin_user_password: tuple[str, str] | None, is_byoidc: bool, ) -> Generator[DynamicClient, Any, Any]: @@ - elif non_admin_user_password is None: - raise ValueError("Unprivileged user not provisioned") + elif non_admin_user_password is None: + raise ValueError("Unprivileged user not provisioned (user credentials secret missing?)")Behavior stays the same; the interface and error message become clearer.
417-453: BYOIDC unprivileged client construction is sound; consider a small robustness tweakThe BYOIDC branch correctly:
- Fetches OIDC tokens and issuer using the new helpers.
- Builds an in-memory kubeconfig with a single OIDC user.
- Updates the current context’s user and uses
get_client(config_dict=..., context=..., persist_config=False)to avoid touching on-disk kubeconfig.Two small optional improvements:
- Context lookup robustness
Using a list comprehension and[0]will raise an unhelpfulIndexErrorif the current context name is missing. You can both satisfy the static hint and improve the error withnext(...):- current_context_name = kubeconfig_content["current-context"] - current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0] + current_context_name = kubeconfig_content["current-context"] + try: + current_context = next( + c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name + ) + except StopIteration: + raise RuntimeError(f"Context '{current_context_name}' not found in kubeconfig") from None
- Minor comment nit
The comment# replace the users - we only need this one userscould be clarified to “user”, but that’s purely cosmetic.Overall, the OIDC-based unprivileged client wiring looks good.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(3 hunks)utilities/user_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/conftest.py (2)
utilities/user_utils.py (2)
get_oidc_tokens(117-148)get_byoidc_issuer_url(151-156)tests/model_registry/utils.py (1)
get_byoidc_issuer_url(838-843)
utilities/user_utils.py (1)
utilities/infra.py (1)
get_cluster_authentication(1215-1219)
🪛 Ruff (0.14.6)
tests/conftest.py
415-415: Avoid specifying long messages outside the exception class
(TRY003)
446-446: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
utilities/user_utils.py
145-145: Abstract raise to an inner function
(TRY301)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
147-148: Remove exception handler; error is immediately re-raised
(TRY203)
148-148: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (3)
utilities/user_utils.py (1)
6-7: Imports for BYOIDC helpers look consistentThe added imports (
requests,DynamicClient,get_cluster_authentication) align with the new helpers and are used correctly; no issues from a dependency/usage standpoint.Also applies to: 13-13
tests/conftest.py (2)
65-65: Wiring new BYOIDC helpers into testsImporting
get_oidc_tokensandget_byoidc_issuer_urlhere is the right place for centralizing BYOIDC behavior in fixtures; the usage below matches their intended purpose.
345-347: Secret selection for BYOIDC vs non-BYOIDC looks correctThe updated
non_admin_user_passwordfixture:
- Properly switches between
byoidc-credentials/oidcandopenldap/openldapbased onis_byoidc.- Keeps the base64 decode/split logic unchanged and targets the first username containing
"user", preserving existing behavior.- Logs a clear error and returns
Nonewhen the secret isn’t found, allowing the caller to surface a more explicit failure.This is a clean, localized change to make the unprivileged-user credentials source mode-aware.
Also applies to: 354-356, 357-365, 371-372
jiripetrlik
left a comment
jiripetrlik
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you for making the unprivileged client work.
jgarciao
left a comment
jgarciao
left a comment
There was a problem hiding this comment.
LGTM. Did you run it also on clusters with regular authentication to verify automation kept working?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/conftest.py (4)
345-372: Consider using explicit retrieval for consistency.The function correctly selects secrets based on the
is_byoidcflag. However, Line 364 uses direct list indexingusers_Secret[0], whereas Line 367 usesnext()with a generator. For consistency and clarity, consider using the same pattern.Apply this diff for consistency:
- data = users_Secret[0].instance.data + secret = next(iter(users_Secret)) + data = secret.instance.data
414-415: Consider extracting exception message to a constant (optional).Static analysis suggests avoiding long exception messages specified directly in the raise statement. While this is a minor style issue, you could extract the message to improve maintainability.
Apply this diff:
+ UNPRIVILEGED_USER_NOT_PROVISIONED = "Unprivileged user not provisioned" + elif non_admin_user_password is None: - raise ValueError("Unprivileged user not provisioned") + raise ValueError(UNPRIVILEGED_USER_NOT_PROVISIONED)
421-422: Add error handling for kubeconfig file operations.The code opens the kubeconfig file without handling potential
FileNotFoundError. If the file doesn't exist at the specified path, the fixture will fail with an unclear error.Apply this diff to add defensive error handling:
- with open(kubconfig_filepath) as fd: - kubeconfig_content = yaml.safe_load(fd) + try: + with open(kubconfig_filepath) as fd: + kubeconfig_content = yaml.safe_load(fd) + except FileNotFoundError as e: + raise ValueError(f"Kubeconfig file not found at {kubconfig_filepath}") from e
445-447: Usenext()for safer context lookup.The code uses a list comprehension followed by indexing
[0], which could raise anIndexErrorif the context is not found. Usingnext()with a default or explicit error message would be more robust and Pythonic.Apply this diff:
- current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0] + current_context = next( + (c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name), + None + ) + if current_context is None: + raise ValueError(f"Context '{current_context_name}' not found in kubeconfig")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/conftest.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/conftest.py (2)
utilities/user_utils.py (2)
get_oidc_tokens(117-148)get_byoidc_issuer_url(151-156)tests/model_registry/utils.py (1)
get_byoidc_issuer_url(838-843)
🪛 Ruff (0.14.6)
tests/conftest.py
415-415: Avoid specifying long messages outside the exception class
(TRY003)
446-446: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
🔇 Additional comments (4)
tests/conftest.py (4)
65-65: LGTM!The new imports are necessary for BYOIDC authentication and are used appropriately in the
unprivileged_clientfixture.
376-383: LGTM!The fixture correctly prioritizes the
KUBECONFIGenvironment variable when it points to a valid file, falling back to the default~/.kube/configpath.
457-481: LGTM!The existing non-BYOIDC authentication flow is correctly preserved and continues to work as before, with the admin account properly restored after creating the unprivileged client.
417-453: Unable to complete verification due to repository access restrictions.I attempted to verify the BYOIDC flow review comment but encountered limitations:
- Repository Clone Failed: The codebase could not be accessed in the sandbox environment
- Web Search Unsuccessful: The
get_oidc_tokensandget_byoidc_issuer_urlfunctions are not publicly documented or indexedWhat the review comment requires verification of:
- Error handling in BYOIDC token retrieval functions
- Kubeconfig structure compatibility with Kubernetes client expectations
- Edge case coverage in the BYOIDC flow
To proceed, you would need to:
- Provide direct access to the repository or the specific function implementations
- Clarify if this is a private repository and confirm the correct repository URL
- Share the implementation code for
get_oidc_tokensandget_byoidc_issuer_urldirectlyThe original review comment's concerns are reasonable—validating error handling in authentication flows and client compatibility are important—but I cannot confirm whether the issues actually exist without examining the implementations.
|
/lgtm @asmigala, could u please fix pre-commit |
asmigala
dismissed stale reviews from rhods-ci-bot, lugi0, jgarciao, and jiripetrlik
via
65bb2ad
asmigala
force-pushed
the
byoidc-unprivileged-user-login
branch
from
4524655 to
65bb2ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utilities/user_utils.py (1)
125-130: Remove redundant exception handler and fix log message inconsistency.
- The
except Exception as e: raise eblock is redundant and should be removed entirely.- Line 126 logs "Warning:" but uses
LOGGER.error- this is inconsistent.Apply this diff:
# Validate that we got an access token if "id_token" not in json_response or "refresh_token" not in json_response: - LOGGER.error("Warning: No id_token or refresh_token in response") - raise AssertionError(f"No id_token or refresh_token in response: {json_response}") + LOGGER.error("No id_token or refresh_token in response") + raise AssertionError("Missing id_token or refresh_token in OIDC response") return json_response["id_token"], json_response["refresh_token"] - except Exception as e: - raise etests/conftest.py (2)
417-419: Redundant call toget_byoidc_issuer_url.
get_byoidc_issuer_url(admin_client)is called here on line 419, but it's already called internally withinget_oidc_tokens(line 100 inuser_utils.py). This results in a duplicate fetch of the cluster authentication resource.Consider refactoring
get_oidc_tokensto also return the issuer URL, or callget_byoidc_issuer_urlonce beforeget_oidc_tokensand pass it as a parameter:elif is_byoidc: - tokens = get_oidc_tokens(admin_client, non_admin_user_password[0], non_admin_user_password[1]) issuer = get_byoidc_issuer_url(admin_client) + tokens = get_oidc_tokens(admin_client, non_admin_user_password[0], non_admin_user_password[1])Then update
get_oidc_tokensto accept the issuer URL as a parameter instead of callingget_byoidc_issuer_urlinternally.
445-447: Usenext()instead of list comprehension with index access.The list comprehension with
[0]index access is less idiomatic and can raiseIndexErrorif no matching context is found.Apply this diff:
# get the current context and modify the referenced user in place current_context_name = kubeconfig_content["current-context"] - current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0] + current_context = next(c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name) current_context["context"]["user"] = non_admin_user_password[0]Note:
next()raisesStopIterationif no match is found, which provides a clearer error thanIndexError. If you want a more descriptive error, you can use:current_context = next( (c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name), None ) if current_context is None: raise ValueError(f"Context '{current_context_name}' not found in kubeconfig")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(3 hunks)utilities/user_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/conftest.py (1)
utilities/user_utils.py (2)
get_oidc_tokens(99-130)get_byoidc_issuer_url(133-138)
utilities/user_utils.py (3)
utilities/exceptions.py (1)
ExceptionUserLogin(137-138)utilities/infra.py (2)
login_with_user_password(449-473)get_cluster_authentication(1215-1219)tests/model_registry/utils.py (1)
get_byoidc_issuer_url(838-843)
🪛 Ruff (0.14.6)
tests/conftest.py
415-415: Avoid specifying long messages outside the exception class
(TRY003)
446-446: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
utilities/user_utils.py
127-127: Abstract raise to an inner function
(TRY301)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
129-130: Remove exception handler; error is immediately re-raised
(TRY203)
130-130: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (6)
utilities/user_utils.py (2)
6-14: LGTM on imports.The new imports for
requests,DynamicClient, andget_cluster_authenticationare properly organized and necessary for the new BYOIDC functionality.
133-138: Verify duplicate function intests/model_registry/utils.py.This review comment claims an identical
get_byoidc_issuer_urlfunction exists at lines 837-842 intests/model_registry/utils.py. If confirmed, consider consolidating by importing from the utility module instead.tests/conftest.py (4)
65-65: LGTM on import.The new imports for BYOIDC utilities are correctly added.
354-372: LGTM on BYOIDC credential lookup logic.The conditional logic for selecting between BYOIDC (
byoidc-credentialsinoidcnamespace) and OpenLDAP credentials is correct. The updated error message is more generic and appropriate.Minor: Variable
users_Secret(line 357, 364) uses mixed case - considerusers_secretfor consistency with Python naming conventions.
375-383: LGTM on kubeconfig path resolution.The fixture correctly prioritizes the
KUBECONFIGenvironment variable over the default~/.kube/configpath. The file existence check on the env var path is appropriate.
449-455: LGTM on in-memory client creation.Using
config_dictwithpersist_config=Falsecorrectly creates the client without modifying the kubeconfig file on disk. This is the appropriate approach for the BYOIDC flow.
asmigala
force-pushed
the
byoidc-unprivileged-user-login
branch
from
d66e310 to
e102ec6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/conftest.py (1)
417-455: LGTM! Consider minor optimization.The BYOIDC authentication flow is well-implemented. It correctly:
- Retrieves tokens and issuer URL
- Builds an in-memory kubeconfig with OIDC auth-provider configuration
- Sets
persist_config=Falseto avoid modifying the original kubeconfig fileOptional optimization: Line 446 could use
next()instead of list comprehension for better idiomaticity:- current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0] + current_context = next(c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name)Based on static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(3 hunks)utilities/user_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/conftest.py (2)
utilities/user_utils.py (2)
get_oidc_tokens(99-130)get_byoidc_issuer_url(133-138)tests/model_registry/utils.py (1)
get_byoidc_issuer_url(838-843)
utilities/user_utils.py (3)
utilities/exceptions.py (1)
ExceptionUserLogin(137-138)utilities/infra.py (2)
login_with_user_password(449-473)get_cluster_authentication(1215-1219)tests/model_registry/utils.py (1)
get_byoidc_issuer_url(838-843)
🪛 Ruff (0.14.6)
tests/conftest.py
415-415: Avoid specifying long messages outside the exception class
(TRY003)
446-446: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
utilities/user_utils.py
127-127: Abstract raise to an inner function
(TRY301)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
129-130: Remove exception handler; error is immediately re-raised
(TRY203)
130-130: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (7)
utilities/user_utils.py (2)
6-7: LGTM!The new imports are necessary for the BYOIDC token retrieval functionality.
133-138: Address code duplication and add boundary check.This function is duplicated in
tests/model_registry/utils.py(lines 837-842). Additionally, line 136 directly accessesoidcProviders[0]without verifying the list is non-empty, which could raise anIndexError.Recommendations:
- Consolidate the duplicate implementations into this single location and update imports in
tests/model_registry/utils.pyto use this version.- Add a boundary check before accessing
oidcProviders[0].tests/conftest.py (5)
65-65: LGTM!The import change correctly reflects the new BYOIDC authentication utilities, replacing the removed
get_unprivileged_contextfunction.
345-372: LGTM!The fixture correctly branches between BYOIDC and OpenLDAP credential sources. The secret selection logic appropriately chooses
byoidc-credentialsfrom theoidcnamespace for BYOIDC environments andopenldapfrom theopenldapnamespace otherwise.
376-384: LGTM!The fixture correctly resolves the kubeconfig path by prioritizing the
KUBECONFIGenvironment variable when it points to an existing file, with a sensible fallback to the default location.
392-396: LGTM!The fixture correctly determines BYOIDC mode by checking if the cluster authentication type is "OIDC", with proper handling of the None case.
457-481: LGTM!The non-BYOIDC authentication flow correctly handles the OpenLDAP-based login, context switching, and cleanup by restoring the original user context.
asmigala
force-pushed
the
byoidc-unprivileged-user-login
branch
from
719c188 to
a0b8a15
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
utilities/user_utils.py (1)
99-131: Remove the redundanttry/exceptwrapper inget_oidc_tokensThe
try/except Exception as e: raise eblock adds no value, just re-raises the same exception and triggers Ruff warnings. You can simplify by letting exceptions bubble up directly.Suggested refactor:
def get_oidc_tokens(admin_client: DynamicClient, username: str, password: str) -> tuple[str, str]: @@ - try: - LOGGER.info(f"Requesting token for user {username} in byoidc environment") - response = requests.post( - url=url, - headers=headers, - data=data, - allow_redirects=True, - timeout=30, - verify=True, # Set to False if you need to skip SSL verification - ) - response.raise_for_status() - json_response = response.json() - - # Validate that we got an access token - if "id_token" not in json_response or "refresh_token" not in json_response: - LOGGER.error("Warning: No id_token or refresh_token in response") - raise AssertionError(f"No id_token or refresh_token in response: {json_response}") - return json_response["id_token"], json_response["refresh_token"] - except Exception as e: - raise e + LOGGER.info(f"Requesting token for user {username} in byoidc environment") + response = requests.post( + url=url, + headers=headers, + data=data, + allow_redirects=True, + timeout=30, + verify=True, # Set to False if you need to skip SSL verification + ) + response.raise_for_status() + json_response = response.json() + + # Validate that we got an access token + if "id_token" not in json_response or "refresh_token" not in json_response: + LOGGER.error("Warning: No id_token or refresh_token in response") + raise AssertionError(f"No id_token or refresh_token in response: {json_response}") + return json_response["id_token"], json_response["refresh_token"]This keeps behavior identical while satisfying the linter and simplifying the code.
🧹 Nitpick comments (2)
tests/conftest.py (2)
345-347:non_admin_user_passwordsecret selection logic is sound; consider a clearer failure for malformed secretsThe fixture correctly:
- Short-circuits when
use_unprivileged_clientisFalse.- Chooses
byoidc-credentials/oidcvsopenldap/openldapbased onis_byoidc.- Decodes
users/passwordsfrom the secret and picks the first entry containing"user".The only minor edge case is:
first_user_index = next(index for index, user in enumerate(users) if "user" in user)If the secret content changes and no username contains
"user", this will raiseStopIteration, which can be a bit opaque when debugging CI failures. If you care about a more explicit error, you could wrap this in a small helper that raisesValueErrorwith a clearer message, but it's not required for correctness.Also applies to: 354-365, 371-372
375-383: BYOIDCunprivileged_clientflow looks correct; minor readability tweak possible for context lookupThe BYOIDC branch does the right things:
- Fails fast if
non_admin_user_passwordisNone.- Uses
get_oidc_tokensandget_byoidc_issuer_urlto build an OIDCuserentry.- Replaces
kubeconfig_content["users"]with a single OIDC user and rewires the current context to that username.- Creates the client from
config_dictwithpersist_config=False, so the on-disk kubeconfig is left untouched.- Keeps the existing LDAP-based login flow unchanged for non-BYOIDC clusters.
One optional readability improvement is in the current-context lookup:
current_context_name = kubeconfig_content["current-context"] current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0]You could make this slightly clearer (and align with Ruff’s
RUF015suggestion) by usingnext(...):current_context_name = kubeconfig_content["current-context"] - current_context = [c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name][0] + current_context = next( + c for c in kubeconfig_content["contexts"] if c["name"] == current_context_name + )Functionally it’s equivalent, so this is purely a style/nitpick change.
Also applies to: 400-406, 414-453
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/conftest.py(3 hunks)tests/model_registry/utils.py(1 hunks)utilities/user_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/conftest.py (1)
utilities/user_utils.py (2)
get_oidc_tokens(99-130)get_byoidc_issuer_url(133-138)
tests/model_registry/utils.py (1)
utilities/user_utils.py (1)
get_byoidc_issuer_url(133-138)
utilities/user_utils.py (2)
utilities/exceptions.py (1)
ExceptionUserLogin(137-138)utilities/infra.py (2)
login_with_user_password(449-473)get_cluster_authentication(1215-1219)
🪛 Ruff (0.14.6)
tests/conftest.py
415-415: Avoid specifying long messages outside the exception class
(TRY003)
446-446: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
utilities/user_utils.py
127-127: Abstract raise to an inner function
(TRY301)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
129-130: Remove exception handler; error is immediately re-raised
(TRY203)
130-130: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (3)
tests/model_registry/utils.py (1)
37-37: Centralizingget_byoidc_issuer_urlimport looks goodImporting
get_byoidc_issuer_urlfromutilities.user_utilsremoves duplication in this module and aligns with the earlier request to centralize issuer URL retrieval. No issues from this change alone.utilities/user_utils.py (1)
133-138:get_byoidc_issuer_urlimplementation is consistent and reusableFetching the issuer URL via
get_cluster_authenticationand asserting non-Nonevalues matches how other codepaths treat cluster auth; centralizing this helper here is a good move and aligns with its new usage in tests and model registry utils.tests/conftest.py (1)
65-65: BYOIDC detection and helper imports are wired cleanlyImporting
get_oidc_tokens/get_byoidc_issuer_urlhere and derivingis_byoidcfrom theAuthenticationresource gives a single, clear switch for BYOIDC vs non-BYOIDC behavior across fixtures. The wiring looks consistent and low-risk.Also applies to: 386-397
|
Status of building tag latest: success. |
…ub-io#892) Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
7 participants
Use secrets on cluster to log in as unprivileged user for BYOIDC
Description
Previously, there was a workaround for BYOIDC clusters that required that a context with an unprivileged user already exists in the kubeconfig.
Changed to reuse the same logic as with regular clusters, reading the user credentials from a secret on the cluster. Also rewrote the login to use native oidc support in kubernetes client.
How Has This Been Tested?
Locally ran workbenches, llama_stack and rag tests
Merge criteria:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.