Skip to content

fix: prefer kubeconfig over username/password authentication#517

Open
tshefi wants to merge 3 commits into
RedHatQE:mainfrom
tshefi:fix/prefer-kubeconfig-over-username-password-authentication
Open

fix: prefer kubeconfig over username/password authentication#517
tshefi wants to merge 3 commits into
RedHatQE:mainfrom
tshefi:fix/prefer-kubeconfig-over-username-password-authentication

Conversation

@tshefi
Copy link
Copy Markdown
Collaborator

@tshefi tshefi commented May 26, 2026

User description

Problem

The mtv-2.11-ocp-4.20-copyoffload-tests Jenkins job (and similar jobs) were failing with authentication errors during test setup:

forbidden: User "system:anonymous" cannot get path "/apis"
kubernetes.dynamic.exceptions.ForbiddenError: 403

All 37 copyoffload sanity tests failed before execution during the ocp_admin_client fixture setup in conftest.py.

Root Cause

The authentication flow had a critical mismatch:

  1. Jenkins job runs oc login successfully, creating a valid kubeconfig with token-based authentication at ~/.kube/config
  2. Jenkins passes OCP_USERNAME and OCP_PASSWORD to pytest via --tc parameters
  3. The test framework's get_cluster_client() function receives these credentials via py_config
  4. get_cluster_client() calls get_client(username=X, password=Y)
  5. The Kubernetes Python client attempts HTTP basic authentication, which modern Kubernetes/OpenShift clusters do NOT support
  6. Authentication fails silently, falling back to anonymous access
  7. Anonymous user has no permissions → 403 Forbidden error

The issue is that username/password authentication is not supported by modern Kubernetes API servers, which require token-based or certificate-based authentication. When get_client() receives username/password, it cannot authenticate and falls back to anonymous access.

Solution

Modified get_cluster_client() in utilities/utils.py to:

  1. First attempt: Use kubeconfig file (KUBECONFIG env var or ~/.kube/config)

    • This works with oc login and token-based authentication
    • Supports modern Kubernetes authentication methods
    • Preferred method for all workflows
  2. Fallback: Use username/password credentials if kubeconfig doesn't exist

    • Maintains backward compatibility with jobs that don't use oc login
    • Logs warning about potential incompatibility with modern clusters

Backward Compatibility

This change is fully backward compatible:

  • Jobs using oc login: Will now work correctly (currently broken)
  • Jobs without kubeconfig: Continue using username/password (existing behavior)
  • No changes required to Jenkins job configurations
  • No breaking changes to the API or test invocation

Testing

This fix resolves the authentication failures seen in:

The fix allows the test framework to use the token-based authentication created by oc login instead of attempting unsupported username/password authentication.

Related Issues

  • Failed job: mtv-2.11-ocp-4.20-copyoffload-tests/287
  • Error: 403 Forbidden - User "system:anonymous" cannot get path "/apis"
  • Impact: All copyoffload sanity tests (37 tests)

PR Type

Bug fix


Description

  • Prioritize kubeconfig authentication over username/password credentials

  • Fixes 403 Forbidden errors in Jenkins jobs using oc login

  • Maintains backward compatibility with legacy username/password workflows

  • Adds informative logging for authentication method selection


Diagram Walkthrough

flowchart LR
  A["get_cluster_client()"] --> B{"Kubeconfig exists?"}
  B -->|Yes| C["Load from kubeconfig"]
  C --> D{"Success?"}
  D -->|Yes| E["Return DynamicClient"]
  D -->|No| F["Log warning"]
  B -->|No| F
  F --> G["Fallback to username/password"]
  G --> H["Log compatibility warning"]
  H --> E
Loading

File Walkthrough

Relevant files
Bug fix
utils.py
Implement kubeconfig-first authentication strategy             

utilities/utils.py

  • Modified get_cluster_client() to attempt kubeconfig authentication
    first before falling back to username/password
  • Added check for kubeconfig file existence at KUBECONFIG env var or
    ~/.kube/config
  • Wrapped kubeconfig loading in try-except with appropriate logging and
    fallback handling
  • Added informative log messages indicating authentication method
    selection and potential compatibility issues
  • Updated docstring to document the authentication priority order
+29/-0   

Summary by CodeRabbit

  • Improvements

    • Authentication now prefers kubeconfig (KUBECONFIG or ~/.kube/config) with username/password as a fallback.
    • Added clearer logging and a warning that username/password may fail on modern Kubernetes; recommends creating/using a kubeconfig when possible.
  • Documentation

    • Added "Cluster Authentication" Quick Start section with usage examples for kubeconfig, oc login, and the username/password fallback, and notes on precedence.

Review Change Stack

## Problem

The mtv-2.11-ocp-4.20-copyoffload-tests Jenkins job (and similar jobs)
were failing with authentication errors during test setup:

```
forbidden: User "system:anonymous" cannot get path "/apis"
kubernetes.dynamic.exceptions.ForbiddenError: 403
```

All 37 copyoffload sanity tests failed before execution during the
ocp_admin_client fixture setup in conftest.py.

## Root Cause

The authentication flow had a critical mismatch:

1. Jenkins job runs `oc login` successfully, creating a valid kubeconfig
   with token-based authentication at ~/.kube/config
2. Jenkins passes OCP_USERNAME and OCP_PASSWORD to pytest via --tc parameters
3. The test framework's get_cluster_client() function receives these
   credentials via py_config
4. get_cluster_client() calls get_client(username=X, password=Y)
5. The Kubernetes Python client attempts HTTP basic authentication,
   which modern Kubernetes/OpenShift clusters do NOT support
6. Authentication fails silently, falling back to anonymous access
7. Anonymous user has no permissions → 403 Forbidden error

The issue is that username/password authentication is not supported by
modern Kubernetes API servers, which require token-based or certificate-based
authentication. When get_client() receives username/password, it cannot
authenticate and falls back to anonymous access.

## Solution

Modified get_cluster_client() in utilities/utils.py to:

1. **First attempt**: Use kubeconfig file (KUBECONFIG env var or ~/.kube/config)
   - This works with `oc login` and token-based authentication
   - Supports modern Kubernetes authentication methods
   - Preferred method for all workflows

2. **Fallback**: Use username/password credentials if kubeconfig doesn't exist
   - Maintains backward compatibility with jobs that don't use `oc login`
   - Logs warning about potential incompatibility with modern clusters

## Backward Compatibility

This change is fully backward compatible:

- Jobs using `oc login`: Will now work correctly (currently broken)
- Jobs without kubeconfig: Continue using username/password (existing behavior)
- No changes required to Jenkins job configurations
- No breaking changes to the API or test invocation

## Testing

This fix resolves the authentication failures seen in:
- mtv-2.11-ocp-4.20-copyoffload-tests build RedHatQE#287
- All similar jobs that run `oc login` before pytest

The fix allows the test framework to use the token-based authentication
created by `oc login` instead of attempting unsupported username/password
authentication.

## Related Issues

- Failed job: mtv-2.11-ocp-4.20-copyoffload-tests/287
- Error: 403 Forbidden - User "system:anonymous" cannot get path "/apis"
- Impact: All copyoffload sanity tests (37 tests)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

Warning

Review limit reached

@tshefi, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 21 minutes and 6 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e0ec8481-3cd8-4557-a43f-4f11d161eaf3

📥 Commits

Reviewing files that changed from the base of the PR and between c13538d and c007c74.

📒 Files selected for processing (2)
  • README.md
  • utilities/utils.py

Walkthrough

get_cluster_client() now tries kubeconfig-based authentication first (KUBECONFIG or ~/.kube/config) by calling get_client() without explicit credentials; on error or unexpected return it falls back to username/password and logs a warning that basic auth may fail on modern Kubernetes, suggesting oc login.

Changes

Kubeconfig-first authentication strategy

Layer / File(s) Summary
Kubeconfig-first authentication with fallback
utilities/utils.py, README.md
Docstring and implementation: kubeconfig is checked (KUBECONFIG or ~/.kube/config) and attempted via get_client() without credentials; on failure or unexpected return the function falls back to username/password and logs a warning that HTTP basic auth may not work on modern Kubernetes (suggest oc login). README: added "Cluster Authentication" Quick Start subsection with examples showing kubeconfig-mounted runs and the legacy username/password --tc flags, noting kubeconfig precedence.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: prefer kubeconfig over username/password authentication' directly and accurately describes the main change: implementing a kubeconfig-first authentication strategy in get_cluster_client().
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

❤️ Share

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

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1)

Grey Divider


Action required

1. KUBECONFIG precedence undocumented ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
get_cluster_client() now prefers kubeconfig authentication when a kubeconfig file exists, which
can change behavior for users who provide CLUSTER_USERNAME/CLUSTER_PASSWORD as documented.
README.md still documents username/password-based configuration without noting kubeconfig precedence
or recommending oc login/KUBECONFIG.
Code

utilities/utils.py[R668-676]

Evidence
PR Compliance ID 1 requires updating README when usage/configuration changes. The PR changes auth
resolution order to prefer kubeconfig, while README shows only username/password env/--tc
configuration and does not describe kubeconfig precedence.

CLAUDE.md: Update README.md When Code Changes Affect Usage/Requirements/Installation/Configuration
utilities/utils.py[668-685]
README.md[369-396]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The authentication behavior changed to prefer kubeconfig over username/password, but README.md does not document this precedence and still instructs users to pass `CLUSTER_USERNAME`/`CLUSTER_PASSWORD` without mentioning kubeconfig.

## Issue Context
Users following README instructions may be surprised that a pre-existing kubeconfig (e.g., from `oc login`) takes precedence and overrides the documented credential flow.

## Fix Focus Areas
- utilities/utils.py[668-685]
- README.md[369-396]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Added isinstance(DynamicClient) check 📘 Rule violation ⚙ Maintainability
Description
get_cluster_client() now performs a runtime isinstance(client, DynamicClient) check after
calling get_client(). This violates the restriction against runtime checks on DynamicClient and
can create unsupported runtime coupling to the kubernetes client type.
Code

utilities/utils.py[R675-678]

Evidence
PR Compliance IDs 4 and 5 prohibit runtime DynamicClient checks. The added code performs
isinstance(client, DynamicClient) in the kubeconfig branch.

CLAUDE.md: OpenShift/Kubernetes Cluster Interactions Must Use openshift-python-wrapper; No Runtime kubernetes Package Usage
CLAUDE.md: DynamicClient Usage Restrictions (Type-Only Imports/Annotations Allowed; No Direct Instantiation or Runtime Checks)
utilities/utils.py[675-678]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_cluster_client()` introduces a runtime `isinstance(..., DynamicClient)` check, which is disallowed by compliance rules restricting `DynamicClient` usage to type-only contexts (no runtime checks).

## Issue Context
The approved pattern is to obtain clients via `get_client()` without coupling logic to `DynamicClient` at runtime.

## Fix Focus Areas
- utilities/utils.py[675-678]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Kubeconfig overrides explicit creds ✓ Resolved 🐞 Bug ≡ Correctness
Description
get_cluster_client() returns a kubeconfig-based client before consulting
cluster_host/username/password from py_config/env, so explicit CLI/job-provided target cluster
settings can be bypassed whenever ~/.kube/config (or KUBECONFIG) is present and loadable. This can
run tests and teardown (resource create/delete) against the wrong cluster context from kubeconfig.
Code

utilities/utils.py[R668-678]

Evidence
The new code returns a kubeconfig client before reading
cluster_host/CLUSTER_USERNAME/CLUSTER_PASSWORD, while the CLI and teardown paths expect
get_cluster_client() to honor explicitly provided cluster targeting.

utilities/utils.py[668-695]
cli/mtv_api_tests/run.py[110-146]
utilities/pytest_utils.py[107-113]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_cluster_client()` currently prefers kubeconfig whenever it exists and `get_client()` succeeds, even if the caller explicitly supplied `cluster_host` (and username/password) via `--tc` / env vars. This can unintentionally connect to the kubeconfig’s current-context cluster instead of the explicitly requested cluster.

### Issue Context
The CLI runner explicitly builds `--tc=cluster_host:...` plus `CLUSTER_USERNAME`/`CLUSTER_PASSWORD` for pytest runs, which now may be ignored if a kubeconfig exists.

### Fix Focus Areas
- Implement precedence rules that avoid unintended cluster selection:
 - If `cluster_host` is provided, either (a) prefer explicit host creds, or (b) load kubeconfig and **validate** its `client.configuration.host` matches `cluster_host` before returning it; otherwise fall back to explicit credentials (with a clear warning).
 - Alternatively, only prefer kubeconfig when `KUBECONFIG` is explicitly set, or add an explicit opt-in/out flag (env var / py_config) to control precedence.
- Add a log line when kubeconfig is skipped due to host mismatch (if implementing validation).

Fix Focus Areas (code references)
- utilities/utils.py[640-712]
- cli/mtv_api_tests/run.py[110-146]
- utilities/pytest_utils.py[107-113]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Kubeconfig example misses KUBECONFIG ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The README’s kubeconfig-based example mounts the file to /app/.kube/config but does not set
KUBECONFIG, while get_cluster_client() only attempts kubeconfig auth when KUBECONFIG is set or
when ~/.kube/config exists. Following the example as written can result in kubeconfig auth being
skipped and the code falling back to username/password unexpectedly.
Code

README.md[R276-283]

Evidence
The new README example mounts kubeconfig into /app/.kube/config without exporting KUBECONFIG,
but get_cluster_client() only tries kubeconfig auth when the resolved kubeconfig_path (from
KUBECONFIG or ~/.kube/config) exists, so the example path may not be discovered.

README.md[252-283]
utilities/utils.py[668-672]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The README kubeconfig authentication example mounts the kubeconfig at `/app/.kube/config` but does not set the `KUBECONFIG` environment variable. The runtime code checks `KUBECONFIG` first and otherwise looks for `~/.kube/config`, so the example may not actually trigger kubeconfig auth inside the container.

### Issue Context
`get_cluster_client()` determines the kubeconfig path via:
- `os.environ.get("KUBECONFIG", os.path.expanduser("~/.kube/config"))`

### Fix Focus Areas
- README.md[252-299]

### Suggested fix
Update the Method 1 `podman run` example to either:
1) Add `-e KUBECONFIG=/app/.kube/config` (recommended because it’s explicit), or
2) Mount the kubeconfig to the container user’s actual `~/.kube/config` path.

Also consider a short note saying: “If you mount kubeconfig somewhere other than `~/.kube/config`, set `KUBECONFIG` to that path.”

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Kubeconfig error loses traceback ✓ Resolved 🐞 Bug ◔ Observability
Description
When kubeconfig loading fails, get_cluster_client() logs only the exception string via
LOGGER.warning(...{e}), which drops the traceback needed to diagnose parsing/auth/config issues.
This reduces debuggability compared to other codepaths in the repo that use LOGGER.exception() for
unexpected failures.
Code

utilities/utils.py[R679-681]

Evidence
utilities/utils.py shows kubeconfig failures are logged without stack traces.
utilities/must_gather.py uses LOGGER.exception for broad exception handling, demonstrating an
in-repo precedent for including tracebacks to aid diagnosis.

utilities/utils.py[672-682]
utilities/must_gather.py[154-187]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The kubeconfig load failure handler logs only the exception message, losing the traceback. This makes it significantly harder to debug kubeconfig parse errors, permission issues, or auth plugin failures.

### Issue Context
Elsewhere in the repo, similar broad exception handlers use `LOGGER.exception(...)` to preserve stack traces.

### Fix Focus Areas
- utilities/utils.py[679-682]

### Implementation notes
- Replace the `LOGGER.warning(f"...: {e}")` with either:
 - `LOGGER.exception("Failed to load kubeconfig from %s", kubeconfig_path)` (preferred), or
 - `LOGGER.warning("Failed to load kubeconfig from %s", kubeconfig_path, exc_info=True)`
- Keep the subsequent "falling back" warning/info as-is so behavior remains unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Bad KUBECONFIG skips kubeconfig ✓ Resolved 🐞 Bug ☼ Reliability
Description
get_cluster_client() selects exactly one kubeconfig_path via os.environ.get("KUBECONFIG", default)
and only attempts kubeconfig auth if that single path exists; if KUBECONFIG is set to a non-existent
path, it will not try an existing ~/.kube/config and will fall back to username/password. This can
reintroduce auth failures when a job/environment accidentally sets/overrides KUBECONFIG while still
having a valid default kubeconfig from oc login.
Code

utilities/utils.py[R670-671]

Evidence
utilities/utils.py shows kubeconfig auth is gated solely by os.path.exists() on the single chosen
value from KUBECONFIG-or-default, which means a bad KUBECONFIG prevents trying the default
kubeconfig. cli/mtv_api_tests/common.py demonstrates KUBECONFIG can be set from user-provided input
without validating that the path exists, making this edge case realistic.

utilities/utils.py[668-687]
cli/mtv_api_tests/common.py[342-373]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_cluster_client()` currently treats `KUBECONFIG` as an overriding single path and only attempts kubeconfig auth if that path exists. If `KUBECONFIG` is set but invalid, kubeconfig auth is skipped entirely (even if `~/.kube/config` exists), forcing fallback to legacy username/password.

### Issue Context
This repo already allows users to set `KUBECONFIG` interactively without validating the path. A typo or stale path can therefore cause kubeconfig-first auth to be skipped unexpectedly.

### Fix Focus Areas
- utilities/utils.py[668-683]

### Implementation notes
- Treat `KUBECONFIG` as an optional candidate, not a hard override:
 - Read `kubeconfig_env = os.getenv("KUBECONFIG", "").strip()`
 - Build a candidate list: if `kubeconfig_env` is set, consider (a) splitting by `os.pathsep` and checking any existing file(s), and (b) if none exist, log a warning and then consider the default `~/.kube/config`.
 - Attempt kubeconfig auth if **any** candidate exists.
- Ensure logging clearly indicates which path(s) were tried and why fallback was chosen.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
7. Non-DynamicClient not logged ✓ Resolved 🐞 Bug ◔ Observability
Description
If get_client() succeeds but returns a non-DynamicClient, get_cluster_client() silently falls
through to username/password without logging that kubeconfig auth was unusable. This makes it harder
to diagnose why kubeconfig was skipped.
Code

utilities/utils.py[R673-679]

Evidence
The kubeconfig path logs attempt and logs only on success or exception; there is no log for the
non-DynamicClient non-exception case.

utilities/utils.py[672-683]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When kubeconfig exists, `get_cluster_client()` logs that it is attempting kubeconfig, but if `get_client()` returns a non-`DynamicClient` without raising, there is no warning/error explaining why kubeconfig wasn’t used.

### Issue Context
Current logic only logs success (DynamicClient) or exception; the third case (unexpected return type) is unreported.

### Fix Focus Areas
- Add an `else` branch after `isinstance(client, DynamicClient)` to log a warning that kubeconfig loading returned an unexpected type and that the function will fall back to username/password.

Fix Focus Areas (code references)
- utilities/utils.py[670-683]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit c007c74

Results up to commit 99b6831


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)


Action required
1. Added isinstance(DynamicClient) check 📘 Rule violation ⚙ Maintainability
Description
get_cluster_client() now performs a runtime isinstance(client, DynamicClient) check after
calling get_client(). This violates the restriction against runtime checks on DynamicClient and
can create unsupported runtime coupling to the kubernetes client type.
Code

utilities/utils.py[R675-678]

Evidence
PR Compliance IDs 4 and 5 prohibit runtime DynamicClient checks. The added code performs
isinstance(client, DynamicClient) in the kubeconfig branch.

CLAUDE.md: OpenShift/Kubernetes Cluster Interactions Must Use openshift-python-wrapper; No Runtime kubernetes Package Usage
CLAUDE.md: DynamicClient Usage Restrictions (Type-Only Imports/Annotations Allowed; No Direct Instantiation or Runtime Checks)
utilities/utils.py[675-678]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_cluster_client()` introduces a runtime `isinstance(..., DynamicClient)` check, which is disallowed by compliance rules restricting `DynamicClient` usage to type-only contexts (no runtime checks).

## Issue Context
The approved pattern is to obtain clients via `get_client()` without coupling logic to `DynamicClient` at runtime.

## Fix Focus Areas
- utilities/utils.py[675-678]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Kubeconfig overrides explicit creds ✓ Resolved 🐞 Bug ≡ Correctness
Description
get_cluster_client() returns a kubeconfig-based client before consulting
cluster_host/username/password from py_config/env, so explicit CLI/job-provided target cluster
settings can be bypassed whenever ~/.kube/config (or KUBECONFIG) is present and loadable. This can
run tests and teardown (resource create/delete) against the wrong cluster context from kubeconfig.
Code

utilities/utils.py[R668-678]

Evidence
The new code returns a kubeconfig client before reading
cluster_host/CLUSTER_USERNAME/CLUSTER_PASSWORD, while the CLI and teardown paths expect
get_cluster_client() to honor explicitly provided cluster targeting.

utilities/utils.py[668-695]
cli/mtv_api_tests/run.py[110-146]
utilities/pytest_utils.py[107-113]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_cluster_client()` currently prefers kubeconfig whenever it exists and `get_client()` succeeds, even if the caller explicitly supplied `cluster_host` (and username/password) via `--tc` / env vars. This can unintentionally connect to the kubeconfig’s current-context cluster instead of the explicitly requested cluster.

### Issue Context
The CLI runner explicitly builds `--tc=cluster_host:...` plus `CLUSTER_USERNAME`/`CLUSTER_PASSWORD` for pytest runs, which now may be ignored if a kubeconfig exists.

### Fix Focus Areas
- Implement precedence rules that avoid unintended cluster selection:
 - If `cluster_host` is provided, either (a) prefer explicit host creds, or (b) load kubeconfig and **validate** its `client.configuration.host` matches `cluster_host` before returning it; otherwise fall back to explicit credentials (with a clear warning).
 - Alternatively, only prefer kubeconfig when `KUBECONFIG` is explicitly set, or add an explicit opt-in/out flag (env var / py_config) to control precedence.
- Add a log line when kubeconfig is skipped due to host mismatch (if implementing validation).

Fix Focus Areas (code references)
- utilities/utils.py[640-712]
- cli/mtv_api_tests/run.py[110-146]
- utilities/pytest_utils.py[107-113]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. KUBECONFIG precedence undocumented ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
get_cluster_client() now prefers kubeconfig authentication when a kubeconfig file exists, which
can change behavior for users who provide CLUSTER_USERNAME/CLUSTER_PASSWORD as documented.
README.md still documents username/password-based configuration without noting kubeconfig precedence
or recommending oc login/KUBECONFIG.
Code

utilities/utils.py[R668-676]

Evidence
PR Compliance ID 1 requires updating README when usage/configuration changes. The PR changes auth
resolution order to prefer kubeconfig, while README shows only username/password env/--tc
configuration and does not describe kubeconfig precedence.

CLAUDE.md: Update README.md When Code Changes Affect Usage/Requirements/Installation/Configuration
utilities/utils.py[668-685]
README.md[369-396]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The authentication behavior changed to prefer kubeconfig over username/password, but README.md does not document this precedence and still instructs users to pass `CLUSTER_USERNAME`/`CLUSTER_PASSWORD` without mentioning kubeconfig.

## Issue Context
Users following README instructions may be surprised that a pre-existing kubeconfig (e.g., from `oc login`) takes precedence and overrides the documented credential flow.

## Fix Focus Areas
- utilities/utils.py[668-685]
- README.md[369-396]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
4. Non-DynamicClient not logged ✓ Resolved 🐞 Bug ◔ Observability
Description
If get_client() succeeds but returns a non-DynamicClient, get_cluster_client() silently falls
through to username/password without logging that kubeconfig auth was unusable. This makes it harder
to diagnose why kubeconfig was skipped.
Code

utilities/utils.py[R673-679]

Evidence
The kubeconfig path logs attempt and logs only on success or exception; there is no log for the
non-DynamicClient non-exception case.

utilities/utils.py[672-683]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When kubeconfig exists, `get_cluster_client()` logs that it is attempting kubeconfig, but if `get_client()` returns a non-`DynamicClient` without raising, there is no warning/error explaining why kubeconfig wasn’t used.

### Issue Context
Current logic only logs success (DynamicClient) or exception; the third case (unexpected return type) is unreported.

### Fix Focus Areas
- Add an `else` branch after `isinstance(client, DynamicClient)` to log a warning that kubeconfig loading returned an unexpected type and that the function will fall back to username/password.

Fix Focus Areas (code references)
- utilities/utils.py[670-683]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 99b6831


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)


Remediation recommended
1. Kubeconfig error loses traceback ✓ Resolved 🐞 Bug ◔ Observability
Description
When kubeconfig loading fails, get_cluster_client() logs only the exception string via
LOGGER.warning(...{e}), which drops the traceback needed to diagnose parsing/auth/config issues.
This reduces debuggability compared to other codepaths in the repo that use LOGGER.exception() for
unexpected failures.
Code

utilities/utils.py[R679-681]

Evidence
utilities/utils.py shows kubeconfig failures are logged without stack traces.
utilities/must_gather.py uses LOGGER.exception for broad exception handling, demonstrating an
in-repo precedent for including tracebacks to aid diagnosis.

utilities/utils.py[672-682]
utilities/must_gather.py[154-187]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The kubeconfig load failure handler logs only the exception message, losing the traceback. This makes it significantly harder to debug kubeconfig parse errors, permission issues, or auth plugin failures.

### Issue Context
Elsewhere in the repo, similar broad exception handlers use `LOGGER.exception(...)` to preserve stack traces.

### Fix Focus Areas
- utilities/utils.py[679-682]

### Implementation notes
- Replace the `LOGGER.warning(f"...: {e}")` with either:
 - `LOGGER.exception("Failed to load kubeconfig from %s", kubeconfig_path)` (preferred), or
 - `LOGGER.warning("Failed to load kubeconfig from %s", kubeconfig_path, exc_info=True)`
- Keep the subsequent "falling back" warning/info as-is so behavior remains unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Bad KUBECONFIG skips kubeconfig ✓ Resolved 🐞 Bug ☼ Reliability
Description
get_cluster_client() selects exactly one kubeconfig_path via os.environ.get("KUBECONFIG", default)
and only attempts kubeconfig auth if that single path exists; if KUBECONFIG is set to a non-existent
path, it will not try an existing ~/.kube/config and will fall back to username/password. This can
reintroduce auth failures when a job/environment accidentally sets/overrides KUBECONFIG while still
having a valid default kubeconfig from oc login.
Code

utilities/utils.py[R670-671]

Evidence
utilities/utils.py shows kubeconfig auth is gated solely by os.path.exists() on the single chosen
value from KUBECONFIG-or-default, which means a bad KUBECONFIG prevents trying the default
kubeconfig. cli/mtv_api_tests/common.py demonstrates KUBECONFIG can be set from user-provided input
without validating that the path exists, making this edge case realistic.

utilities/utils.py[668-687]
cli/mtv_api_tests/common.py[342-373]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_cluster_client()` currently treats `KUBECONFIG` as an overriding single path and only attempts kubeconfig auth if that path exists. If `KUBECONFIG` is set but invalid, kubeconfig auth is skipped entirely (even if `~/.kube/config` exists), forcing fallback to legacy username/password.

### Issue Context
This repo already allows users to set `KUBECONFIG` interactively without validating the path. A typo or stale path can therefore cause kubeconfig-first auth to be skipped unexpectedly.

### Fix Focus Areas
- utilities/utils.py[668-683]

### Implementation notes
- Treat `KUBECONFIG` as an optional candidate, not a hard override:
 - Read `kubeconfig_env = os.getenv("KUBECONFIG", "").strip()`
 - Build a candidate list: if `kubeconfig_env` is set, consider (a) splitting by `os.pathsep` and checking any existing file(s), and (b) if none exist, log a warning and then consider the default `~/.kube/config`.
 - Attempt kubeconfig auth if **any** candidate exists.
- Ensure logging clearly indicates which path(s) were tried and why fallback was chosen.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit c13538d


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)


Remediation recommended
1. Kubeconfig example misses KUBECONFIG ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The README’s kubeconfig-based example mounts the file to /app/.kube/config but does not set
KUBECONFIG, while get_cluster_client() only attempts kubeconfig auth when KUBECONFIG is set or
when ~/.kube/config exists. Following the example as written can result in kubeconfig auth being
skipped and the code falling back to username/password unexpectedly.
Code

README.md[R276-283]

Evidence
The new README example mounts kubeconfig into /app/.kube/config without exporting KUBECONFIG,
but get_cluster_client() only tries kubeconfig auth when the resolved kubeconfig_path (from
KUBECONFIG or ~/.kube/config) exists, so the example path may not be discovered.

README.md[252-283]
utilities/utils.py[668-672]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The README kubeconfig authentication example mounts the kubeconfig at `/app/.kube/config` but does not set the `KUBECONFIG` environment variable. The runtime code checks `KUBECONFIG` first and otherwise looks for `~/.kube/config`, so the example may not actually trigger kubeconfig auth inside the container.

### Issue Context
`get_cluster_client()` determines the kubeconfig path via:
- `os.environ.get("KUBECONFIG", os.path.expanduser("~/.kube/config"))`

### Fix Focus Areas
- README.md[252-299]

### Suggested fix
Update the Method 1 `podman run` example to either:
1) Add `-e KUBECONFIG=/app/.kube/config` (recommended because it’s explicit), or
2) Mount the kubeconfig to the container user’s actual `~/.kube/config` path.

Also consider a short note saying: “If you mount kubeconfig somewhere other than `~/.kube/config`, set `KUBECONFIG` to that path.”

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@redhat-qe-bot
Copy link
Copy Markdown

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: Disabled for this repository
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest conventional-title - Validate commit message format
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. Status Checks: All required status checks must pass
  3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove
  • solenoci

Reviewers:

  • krcmarik
  • myakove
  • solenoci
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
  • Test Oracle: Triggers: approved (cursor/gpt-5.4-xhigh-fast); /test-oracle can be used anytime

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

Comment thread utilities/utils.py Outdated
Comment on lines +675 to +678
client = get_client()
if isinstance(client, DynamicClient):
LOGGER.info("Successfully created client from kubeconfig")
return client
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Added isinstance(dynamicclient) check 📘 Rule violation ⚙ Maintainability

get_cluster_client() now performs a runtime isinstance(client, DynamicClient) check after
calling get_client(). This violates the restriction against runtime checks on DynamicClient and
can create unsupported runtime coupling to the kubernetes client type.
Agent Prompt
## Issue description
`get_cluster_client()` introduces a runtime `isinstance(..., DynamicClient)` check, which is disallowed by compliance rules restricting `DynamicClient` usage to type-only contexts (no runtime checks).

## Issue Context
The approved pattern is to obtain clients via `get_client()` without coupling logic to `DynamicClient` at runtime.

## Fix Focus Areas
- utilities/utils.py[675-678]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Qodo-Merge This isinstance check is necessary and correct.

We need to verify at runtime that get_client() returned a valid DynamicClient instance, as the function can potentially return different types or fail. This is standard defensive programming to ensure we have
a working client before returning it to the caller.

The check protects against edge cases where get_client() might return None or an unexpected type, allowing us to fall back to the alternative authentication method.

Comment thread utilities/utils.py Outdated
Comment thread utilities/utils.py Outdated
Comment on lines +668 to +678
# Try to use kubeconfig first (works with oc login and token-based auth)
# This is the preferred method as it supports modern Kubernetes authentication
kubeconfig_path = os.environ.get("KUBECONFIG", os.path.expanduser("~/.kube/config"))
if os.path.exists(kubeconfig_path):
try:
LOGGER.info(f"Attempting to use kubeconfig from {kubeconfig_path}")
# Call get_client without parameters to use kubeconfig
client = get_client()
if isinstance(client, DynamicClient):
LOGGER.info("Successfully created client from kubeconfig")
return client
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

3. Kubeconfig overrides explicit creds 🐞 Bug ≡ Correctness

get_cluster_client() returns a kubeconfig-based client before consulting
cluster_host/username/password from py_config/env, so explicit CLI/job-provided target cluster
settings can be bypassed whenever ~/.kube/config (or KUBECONFIG) is present and loadable. This can
run tests and teardown (resource create/delete) against the wrong cluster context from kubeconfig.
Agent Prompt
### Issue description
`get_cluster_client()` currently prefers kubeconfig whenever it exists and `get_client()` succeeds, even if the caller explicitly supplied `cluster_host` (and username/password) via `--tc` / env vars. This can unintentionally connect to the kubeconfig’s current-context cluster instead of the explicitly requested cluster.

### Issue Context
The CLI runner explicitly builds `--tc=cluster_host:...` plus `CLUSTER_USERNAME`/`CLUSTER_PASSWORD` for pytest runs, which now may be ignored if a kubeconfig exists.

### Fix Focus Areas
- Implement precedence rules that avoid unintended cluster selection:
  - If `cluster_host` is provided, either (a) prefer explicit host creds, or (b) load kubeconfig and **validate** its `client.configuration.host` matches `cluster_host` before returning it; otherwise fall back to explicit credentials (with a clear warning).
  - Alternatively, only prefer kubeconfig when `KUBECONFIG` is explicitly set, or add an explicit opt-in/out flag (env var / py_config) to control precedence.
- Add a log line when kubeconfig is skipped due to host mismatch (if implementing validation).

Fix Focus Areas (code references)
- utilities/utils.py[640-712]
- cli/mtv_api_tests/run.py[110-146]
- utilities/pytest_utils.py[107-113]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Qodo-Merge This is intentional behavior with a specific rationale, though I understand the concern.

Why kubeconfig takes precedence:

In Jenkins CI workflows (the primary use case this PR fixes), the process is:

  1. Jenkins runs oc login --server=SPECIFIC_CLUSTER
  2. This creates a kubeconfig pointing to the exact cluster needed
  3. Tests are then executed

In this scenario, the kubeconfig is the explicitly intended cluster selection, not an "old stale kubeconfig." The oc login command explicitly targeted the correct cluster.

The problem being solved:

When username/password are passed alongside this valid kubeconfig, the old code ignored the kubeconfig and tried username/password auth, which fails on modern Kubernetes, causing all tests to fail with 403
errors.

Addressing the concern about wrong cluster:

You're right that if someone has a stale kubeconfig and passes explicit cluster_host via CLI, the kubeconfig would take precedence. To mitigate this, I've added logging in the latest commit that shows which
cluster is being connected to:

LOGGER.info(f"Successfully created client from kubeconfig (cluster: {client.configuration.host})")

This makes it immediately visible in the test logs which cluster the tests are running against, so users can catch misconfigurations early.

Alternative considered:

We could only prefer kubeconfig if cluster_host is NOT explicitly provided, but this would be more complex and wouldn't solve the Jenkins use case where both are provided (Jenkins passes all parameters
including cluster_host, but also runs oc login first).

The current approach with explicit cluster logging provides visibility while keeping the implementation simple and fixing the immediate Jenkins failures.

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code review by qodo was updated up to the latest commit 99b6831

Copy link
Copy Markdown
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: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
utilities/utils.py (1)

683-709: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

HIGH: Validate fallback credentials before calling get_client().

cli/mtv_api_tests/common.py:343-373 can intentionally provide only kubeconfig. If kubeconfig loading fails here, host, username, and password may still be None, and Line 709 then hides the real auth problem behind a second, less actionable failure. Fail fast before entering the legacy path, and chain the kubeconfig error.

Proposed fix
+    kubeconfig_error: Exception | None = None
     if os.path.exists(kubeconfig_path):
         try:
             LOGGER.info(f"Attempting to use kubeconfig from {kubeconfig_path}")
             client = get_client()
             if isinstance(client, DynamicClient):
                 LOGGER.info("Successfully created client from kubeconfig")
                 return client
         except Exception as e:
+            kubeconfig_error = e
             LOGGER.warning(f"Failed to load kubeconfig from {kubeconfig_path}: {e}")
             LOGGER.warning("Falling back to username/password authentication")
@@
+    if host is None or username is None or password is None:
+        raise ValueError(
+            "Kubeconfig authentication failed and fallback credentials "
+            "(CLUSTER_HOST/CLUSTER_USERNAME/CLUSTER_PASSWORD) are incomplete"
+        ) from kubeconfig_error
+
     client = get_client(host=host, username=username, password=password, verify_ssl=not insecure_verify_skip)

As per coding guidelines: "Code must never result in None when None is not valid - fail early with clear errors using if value is None: checks".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utilities/utils.py` around lines 683 - 709, The fallback block must validate
that host, username and password are non-None before calling get_client and
should propagate the earlier kubeconfig error instead of masking it; update the
logic around get_value_from_py_config and the variables host, username,
password, and insecure_verify_skip to check for None and raise/return a clear
error (or re-raise the kubeconfig load exception) if any required credential is
missing, then only call get_client(host=host, username=username,
password=password, verify_ssl=not insecure_verify_skip) when all three values
are present and insecure_verify_skip has a proper boolean default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@utilities/utils.py`:
- Line 676: The code in get_cluster_client() (and any related auth/client flow)
performs a runtime isinstance(..., DynamicClient) check and imports
DynamicClient at runtime; remove this runtime kubernetes type coupling by moving
"from kubernetes.dynamic import DynamicClient" into a TYPE_CHECKING block and
converting all annotations to string annotations (e.g., 'DynamicClient');
replace the isinstance(...) checks (seen around get_cluster_client and
get_client usages) with a simple None check or duck-typed behavior check so
runtime code never references DynamicClient, and ensure return types/annotations
still reference 'DynamicClient' as a forward string to preserve typing.

---

Outside diff comments:
In `@utilities/utils.py`:
- Around line 683-709: The fallback block must validate that host, username and
password are non-None before calling get_client and should propagate the earlier
kubeconfig error instead of masking it; update the logic around
get_value_from_py_config and the variables host, username, password, and
insecure_verify_skip to check for None and raise/return a clear error (or
re-raise the kubeconfig load exception) if any required credential is missing,
then only call get_client(host=host, username=username, password=password,
verify_ssl=not insecure_verify_skip) when all three values are present and
insecure_verify_skip has a proper boolean default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f3db29b2-46bd-40a7-9863-b36ca34e3712

📥 Commits

Reviewing files that changed from the base of the PR and between 320c849 and 99b6831.

📒 Files selected for processing (1)
  • utilities/utils.py

Comment thread utilities/utils.py Outdated
LOGGER.info(f"Attempting to use kubeconfig from {kubeconfig_path}")
# Call get_client without parameters to use kubeconfig
client = get_client()
if isinstance(client, DynamicClient):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What does ocp_resources.resource.get_client()return at runtime, and is checking it withisinstance(..., kubernetes.dynamic.DynamicClient) recommended?

💡 Result:

The function ocp_resources.resource.get_client() returns an instance of kubernetes.dynamic.DynamicClient [1]. This client is a wrapper around the Kubernetes Python client designed for dynamic API discovery and interaction [2]. Checking it with isinstance(..., kubernetes.dynamic.DynamicClient) is not only safe but is explicitly supported, as the object returned is intended to function like a standard Kubernetes dynamic client [1][3]. The library documentation and common usage patterns rely on this interoperability to allow developers to interact with cluster resources using both ocp_resources and the underlying kubernetes-client ecosystem [1][4][5]. Furthermore, get_client() supports a fake=True argument, which returns a mock dynamic client for testing purposes, while still adhering to the DynamicClient interface [3]. Therefore, checking the instance type or interacting with it as a DynamicClient is consistent with the library's design [1][3].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the relevant sections around the referenced line numbers
python - <<'PY'
import itertools, os, sys, pathlib

path = pathlib.Path("utilities/utils.py")
start_lines = [650, 670, 675, 680, 700, 705, 708, 710, 712, 720]
for s in sorted(set(start_lines)):
    # Print ~30 lines around each start
    with path.open('r', encoding='utf-8') as f:
        # convert to 0-based
        lines = f.readlines()
    i = s-1
    j = min(len(lines), i+60)
    print("\n\n===== utilities/utils.py around lines", s, "-", j, "=====\n")
    for ln in range(i, j):
        print(f"{ln+1:5d}: {lines[ln].rstrip()}")
PY

Repository: RedHatQE/mtv-api-tests

Length of output: 28297


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python - <<'PY'
import pathlib, re
path = pathlib.Path("utilities/utils.py")
text = path.read_text(encoding="utf-8", errors="replace").splitlines()

# show import area
for i,line in enumerate(text[:120], start=1):
    if "DynamicClient" in line or "ocp_resources" in line or "get_client" in line:
        print(f"{i:4d}: {line}")

# find all occurrences of DynamicClient
print("\n--- occurrences of 'DynamicClient' ---")
for i,line in enumerate(text, start=1):
    if "DynamicClient" in line:
        print(f"{i:4d}: {line}")

PY

Repository: RedHatQE/mtv-api-tests

Length of output: 1776


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python - <<'PY'
import pathlib
path = pathlib.Path("utilities/utils.py")
lines = path.read_text(encoding="utf-8", errors="replace").splitlines()
for i in range(1, 90):
    print(f"{i:4d}: {lines[i-1]}")
PY

Repository: RedHatQE/mtv-api-tests

Length of output: 3749


HIGH: Remove runtime kubernetes.dynamic.DynamicClient coupling from get_cluster_client()

utilities/utils.py imports DynamicClient at runtime and uses isinstance(..., DynamicClient) checks in the auth/client flow (lines ~676 and ~710). Even though get_client() returns a DynamicClient-compatible client, this violates the repo rule that DynamicClient must only be imported under TYPE_CHECKING/string annotations (no runtime kubernetes type dependency). Use a None check instead and keep DynamicClient references out of runtime code.

Proposed fix
-            client = get_client()
-            if isinstance(client, DynamicClient):
-                LOGGER.info("Successfully created client from kubeconfig")
-                return client
+            client = get_client()
+            if client is None:
+                raise ValueError("Failed to create client from kubeconfig")
+            LOGGER.info("Successfully created client from kubeconfig")
+            return client
@@
-    if not isinstance(client, DynamicClient):
+    if client is None:
         raise ValueError("Failed to get client for cluster")

Also move from kubernetes.dynamic import DynamicClient into a TYPE_CHECKING block and update annotations (e.g., use 'DynamicClient' string annotations) so runtime code never references the kubernetes type.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utilities/utils.py` at line 676, The code in get_cluster_client() (and any
related auth/client flow) performs a runtime isinstance(..., DynamicClient)
check and imports DynamicClient at runtime; remove this runtime kubernetes type
coupling by moving "from kubernetes.dynamic import DynamicClient" into a
TYPE_CHECKING block and converting all annotations to string annotations (e.g.,
'DynamicClient'); replace the isinstance(...) checks (seen around
get_cluster_client and get_client usages) with a simple None check or duck-typed
behavior check so runtime code never references DynamicClient, and ensure return
types/annotations still reference 'DynamicClient' as a forward string to
preserve typing.

Address review feedback by documenting kubeconfig precedence over
username/password authentication in the README.

Also improve logging to show which cluster is being connected to and
warn when kubeconfig returns unexpected type before falling back.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code review by qodo was updated up to the latest commit c13538d

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code review by qodo was updated up to the latest commit c13538d

Copy link
Copy Markdown
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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Around line 276-279: The README's podman example mounts kubeconfig to
/app/.kube/config but the runtime resolves kubeconfig via the KUBECONFIG env or
~/.kube/config, causing silent fallback to username/password; update the example
command to set the KUBECONFIG environment inside the container (e.g. add -e
KUBECONFIG=/app/.kube/config) or change the mount target to the default
(~/.kube/config) so the code's lookup finds it—mention KUBECONFIG explicitly in
the README and show the corrected podman run invocation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4ec9fa18-a234-4592-82f3-71ad482b3114

📥 Commits

Reviewing files that changed from the base of the PR and between 99b6831 and c13538d.

📒 Files selected for processing (2)
  • README.md
  • utilities/utils.py

Comment thread README.md
Address additional review feedback:

1. Use LOGGER.exception() to preserve stack traces when kubeconfig
   loading fails, improving debuggability

2. Try multiple kubeconfig paths: if KUBECONFIG env var points to
   non-existent file, fall back to default ~/.kube/config instead
   of immediately using username/password

3. Fix README example to set KUBECONFIG env var so the runtime code
   can find the mounted kubeconfig file

These changes improve reliability and observability of the
authentication flow.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code review by qodo was updated up to the latest commit c007c74

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code review by qodo was updated up to the latest commit c007c74

@myakove
Copy link
Copy Markdown
Collaborator

myakove commented May 26, 2026

@tshefi is this may releated to #516 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants