Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds kubeconfig-generation helpers and a wrapper class, and extends Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_resource.py (2)
312-339: Consider adding an integration test for token resolution.These tests verify the token resolution logic in isolation by duplicating the code from
get_client. While this validates the algorithm, an integration test usingget_clientwith mocked dependencies would provide stronger coverage against regressions if the logic is refactored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_resource.py` around lines 312 - 339, Replace this unit-style duplicate of the token-parsing logic with an integration-style test that calls get_client so the real resolution code is exercised: create a fake or mocked client_configuration where client_configuration.api_key = {"authorization": "Bearer sha256~oauth-resolved-token"}, call get_client (mock out network/cluster calls as needed and/or monkeypatch save_kubeconfig to write to tmp_path), then assert that save_kubeconfig was invoked with the resolved token or that the written kubeconfig contains the expected token; reference get_client, client_configuration.api_key, and save_kubeconfig when updating the test.
265-270: Consider adding a test for write failure handling.The
save_kubeconfigfunction catchesOSErrorand logs an error without re-raising. A test verifying this behavior (e.g., by mockingos.opento raiseOSError) would confirm that write failures are handled gracefully without breaking client creation.♻️ Example test for write failure
def test_save_kubeconfig_write_failure(self, tmp_path, caplog): """Test that write failures are logged but don't raise.""" kubeconfig_path = str(tmp_path / "kubeconfig") with patch("ocp_resources.utils.kubeconfig.os.open", side_effect=OSError("Permission denied")): save_kubeconfig(path=kubeconfig_path, host="https://api.example.com:6443", token="test-token") assert "Failed to save kubeconfig" in caplog.text assert not os.path.exists(kubeconfig_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_resource.py` around lines 265 - 270, Add a new test that verifies save_kubeconfig handles write failures by mocking os.open to raise OSError; specifically, create a test named test_save_kubeconfig_write_failure (in tests/test_resource.py) that patches ocp_resources.utils.kubeconfig.os.open to raise OSError("Permission denied"), calls save_kubeconfig(path=kubeconfig_path, host=..., token=...), asserts the error message "Failed to save kubeconfig" appears in caplog, and asserts the kubeconfig file does not exist after the call.ocp_resources/utils/kubeconfig.py (1)
20-22: Consider handling file read errors explicitly.When
config_fileis provided but the file doesn't exist or is unreadable,FileNotFoundErrororIOErrorwill propagate up. This is inconsistent with the design goal of non-fatal write errors. Consider wrapping in try/except to match the error-handling pattern used for writes.♻️ Optional: Add explicit error handling for config_file read
elif config_file: + try: with open(config_file) as f: _config = yaml.safe_load(f) + except OSError: + LOGGER.error(f"Failed to read config file {config_file}", exc_info=True) + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp_resources/utils/kubeconfig.py` around lines 20 - 22, Wrap the config_file open/read in a try/except to catch FileNotFoundError/OSError/IOError and handle it non-fatally (like the write error pattern) so a missing/unreadable config doesn't propagate; surround the open(config_file) / yaml.safe_load(f) block and on error log a warning (using the module's logger) including the exception details, leave _config as None or empty, and continue execution (do not raise); update the block referencing config_file, _config, and yaml.safe_load to implement this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ocp_resources/utils/kubeconfig.py`:
- Around line 20-22: Wrap the config_file open/read in a try/except to catch
FileNotFoundError/OSError/IOError and handle it non-fatally (like the write
error pattern) so a missing/unreadable config doesn't propagate; surround the
open(config_file) / yaml.safe_load(f) block and on error log a warning (using
the module's logger) including the exception details, leave _config as None or
empty, and continue execution (do not raise); update the block referencing
config_file, _config, and yaml.safe_load to implement this behavior.
In `@tests/test_resource.py`:
- Around line 312-339: Replace this unit-style duplicate of the token-parsing
logic with an integration-style test that calls get_client so the real
resolution code is exercised: create a fake or mocked client_configuration where
client_configuration.api_key = {"authorization": "Bearer
sha256~oauth-resolved-token"}, call get_client (mock out network/cluster calls
as needed and/or monkeypatch save_kubeconfig to write to tmp_path), then assert
that save_kubeconfig was invoked with the resolved token or that the written
kubeconfig contains the expected token; reference get_client,
client_configuration.api_key, and save_kubeconfig when updating the test.
- Around line 265-270: Add a new test that verifies save_kubeconfig handles
write failures by mocking os.open to raise OSError; specifically, create a test
named test_save_kubeconfig_write_failure (in tests/test_resource.py) that
patches ocp_resources.utils.kubeconfig.os.open to raise OSError("Permission
denied"), calls save_kubeconfig(path=kubeconfig_path, host=..., token=...),
asserts the error message "Failed to save kubeconfig" appears in caplog, and
asserts the kubeconfig file does not exist after the call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d081c13-5920-4ed7-916c-69a33de07d53
📒 Files selected for processing (3)
ocp_resources/resource.pyocp_resources/utils/kubeconfig.pytests/test_resource.py
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 `@ocp_resources/utils/kubeconfig.py`:
- Around line 32-34: The code currently uses a truthy check on config_dict (if
config_dict:) which treats empty mappings like {} as "not provided" and lets
them fall through; change the conditional to explicitly test for presence (e.g.,
if config_dict is not None:) so an explicit empty mapping is accepted and
written as-is; update any related branches that assume falsy means missing to
use the same None check and ensure behavior for config_file and other branches
remains unchanged.
- Around line 35-39: Update the exception handling around YAML operations to
also catch yaml.YAMLError (and subclasses) in addition to OSError: where
yaml.safe_load(config_file) is called (the block that sets _config and logs via
LOGGER.error) and the similar block around yaml.safe_dump, catch yaml.YAMLError
so malformed or unserializable YAML is logged with exc_info=True and the
function continues as the docstring promises; reference the calls to
yaml.safe_load, yaml.safe_dump, the local variable _config, config_file, and
LOGGER.error when making the change.
- Around line 62-66: The current write (os.open(..., 0o600) + os.fdopen(...))
can leave an existing kubeconfig with world-readable perms after truncation;
instead write to a secure temporary file and atomically replace the target:
create the parent dir as before, create a temp file in the same directory (e.g.
using tempfile.NamedTemporaryFile(delete=False, dir=os.path.dirname(path)) or
build path + ".tmp"), open/create the temp with explicit 0o600 perms (os.open or
tempfile with os.fchmod), write yaml.safe_dump(_config) to the temp, fsync the
file descriptor, close it, then call os.replace(temp_path, path) to atomically
swap in the secure file; reference variables/functions: path, _config,
yaml.safe_dump, os.replace, os.open, os.fchmod (or tempfile.NamedTemporaryFile)
when making the change.
In `@tests/test_resource.py`:
- Around line 312-367: The tests currently duplicate the token-resolution branch
instead of exercising the real implementation; replace the in-test replication
with calls to the actual get_client(...) code path (passing
kubeconfig_output_path or equivalent parameter) so token resolution occurs in
production code, or refactor the resolution into a single helper (e.g.,
resolve_token_from_api_key) in the resource module and call that helper from
both get_client and the tests; update the two test functions
(test_save_kubeconfig_resolved_token_from_bearer and
test_save_kubeconfig_resolved_token_explicit_takes_precedence) to invoke
get_client(..., kubeconfig_output_path=kubeconfig_path) or the new
resolve_token_from_api_key helper instead of reimplementing the _resolved_token
logic, and keep assertions on the produced kubeconfig token.
🪄 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: CHILL
Plan: Pro
Run ID: 9ae40dfd-f5b3-466c-a881-dfbf6f072c9e
📒 Files selected for processing (2)
ocp_resources/utils/kubeconfig.pytests/test_resource.py
…oken resolution helper
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_resource.py`:
- Line 276: Calls to save_kubeconfig pass literal secret strings (e.g.,
token="test-token"), which triggers Ruff S106; replace these hardcoded token
keyword arguments with a non-literal variable or fixture (for example use a
module-level TEST_TOKEN constant, a pytest fixture like kube_token, or read from
an env var via os.environ.get("KUBE_TOKEN", "test-token")) and update every
save_kubeconfig(...) invocation in the test file (including the other
occurrences) to pass that variable (e.g., token=TEST_TOKEN or token=kube_token)
instead of the literal string.
- Around line 345-346: Test currently patches os.open but save_kubeconfig uses
tempfile.mkstemp, so change the test to patch tempfile.mkstemp to raise an
OSError (e.g. Permission denied) to exercise the write-failure path;
specifically, replace the patch target "ocp_resources.utils.kubeconfig.os.open"
with "tempfile.mkstemp" (or patch
"ocp_resources.utils.kubeconfig.tempfile.mkstemp" if importing tempfile in that
module) so save_kubeconfig hits the exception path and the test asserts the
expected handling.
🪄 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: CHILL
Plan: Pro
Run ID: ff283ab9-5479-4dbc-826e-bcb2e702516d
📒 Files selected for processing (3)
ocp_resources/resource.pyocp_resources/utils/kubeconfig.pytests/test_resource.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ocp_resources/resource.py
- ocp_resources/utils/kubeconfig.py
myakove
requested changes
Mar 18, 2026
ocp_resources/utils/kubeconfig.py
Outdated
| "current-context": "context", | ||
| } | ||
| else: | ||
| LOGGER.warning("kubeconfig_output_path provided but not enough data to build kubeconfig") |
Collaborator
There was a problem hiding this comment.
Please raise an error if someone calls this function and we do nothing. This is an error.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_resource.py (1)
245-265: Strengthenconfig_filetest to assert byte-for-byte copy behavior.Current assertions confirm semantic YAML equivalence, but they won’t detect content rewriting. If this flow should preserve source content as-is, assert raw bytes equality too.
✅ Test enhancement
source_path = str(tmp_path / "source-kubeconfig") with open(source_path, "w") as f: yaml.safe_dump(source_config, f) + with open(source_path, "rb") as f: + source_raw = f.read() @@ with open(output_path) as f: saved_config = yaml.safe_load(f) + with open(output_path, "rb") as f: + output_raw = f.read() assert saved_config == source_config + assert output_raw == source_raw🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_resource.py` around lines 245 - 265, The test test_save_kubeconfig_with_config_file currently only checks YAML semantic equality; update it to also assert a byte-for-byte copy when save_kubeconfig is called with config_file by reading both source_path and output_path in binary mode and asserting their raw bytes are identical (keep or add this alongside the existing yaml.safe_load assertion) so that save_kubeconfig's behavior of preserving exact file content is validated; reference test name test_save_kubeconfig_with_config_file and variables source_path/output_path and function save_kubeconfig to locate where to add the binary-read/assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ocp_resources/utils/kubeconfig.py`:
- Around line 35-38: The code currently reads config_file into _config with
yaml.safe_load and later re-serializes it (yaml.dump), which alters
formatting/comments; instead preserve the file verbatim by copying the original
bytes: remove the yaml.safe_load/yaml.dump round-trip for the config_file branch
and use a binary copy (e.g., shutil.copyfile or open read/write in 'rb'/'wb') to
write the destination kubeconfig file; update the logic around the config_file
variable and eliminate use of yaml.safe_load/_dump for this path so the original
content is preserved exactly.
- Around line 23-24: The docstring in ocp_resources/utils/kubeconfig.py
currently states that errors are "logged but not raised," which is out-of-date;
update the docstring for the kubeconfig read/write function to state that the
function will raise exceptions on read or write failures (and that files are
created with 0o600 permissions) so the documentation matches runtime behavior;
refer to the kubeconfig module and the function that performs the kubeconfig
read/write in the docstring text when making this change.
---
Nitpick comments:
In `@tests/test_resource.py`:
- Around line 245-265: The test test_save_kubeconfig_with_config_file currently
only checks YAML semantic equality; update it to also assert a byte-for-byte
copy when save_kubeconfig is called with config_file by reading both source_path
and output_path in binary mode and asserting their raw bytes are identical (keep
or add this alongside the existing yaml.safe_load assertion) so that
save_kubeconfig's behavior of preserving exact file content is validated;
reference test name test_save_kubeconfig_with_config_file and variables
source_path/output_path and function save_kubeconfig to locate where to add the
binary-read/assert.
🪄 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: CHILL
Plan: Pro
Run ID: 76566eb3-b29d-467e-9ac5-306056ad3afd
📒 Files selected for processing (3)
ocp_resources/resource.pyocp_resources/utils/kubeconfig.pytests/test_resource.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ocp_resources/resource.py
myakove
reviewed
Mar 30, 2026
Collaborator
myakove
left a comment
myakove
left a comment
There was a problem hiding this comment.
please update the PR description to match the new approch.
ocp_resources/resource.py
Outdated
| client = get_client(config_file=config_file, config_dict=config_dict, context=context) | ||
|
|
||
| for _resource in client.resources.search(): | ||
| for _resource in client.resources.search(): # type: ignore[union-attr] |
Collaborator
There was a problem hiding this comment.
why # type: ignore[union-attr] is needed? no code changes here
Collaborator
Author
There was a problem hiding this comment.
Removed in the latest push — no longer needed since we switched from tuple return to DynamicClientWithKubeconfig subclass.
myakove
requested changes
Mar 30, 2026
ocp_resources/resource.py
Outdated
| config_dict=config_dict, | ||
| verify_ssl=verify_ssl, | ||
| ) | ||
| return _dynamic_client, kubeconfig_path |
Collaborator
There was a problem hiding this comment.
Please do not return differtnt types on different path on the same function.
we can empedded the kubeconfig inside the client object
Collaborator
There was a problem hiding this comment.
should be implmented like:
class DynamicClientWithKubeconfig(DynamicClient):
def __init__(self, client: kubernetes.client.ApiClient, kubeconfig: str) -> None:
super().__init__(client=client)
self.kubeconfig = kubeconfig
if generate_kubeconfig:
if config_file:
LOGGER.info(f"`generate_kubeconfig` ignored, kubeconfig already available at {config_file}")
_dynamic_client = DynamicClientWithKubeconfig(client=_dynamic_client.client, kubeconfig=config_file)
else:
_resolved_token = _resolve_bearer_token(token=token, client_configuration=client_configuration)
kubeconfig_path = save_kubeconfig(
host=host or client_configuration.host,
token=_resolved_token,
config_dict=config_dict,
verify_ssl=verify_ssl,
)
_dynamic_client = DynamicClientWithKubeconfig(client=_dynamic_client.client, kubeconfig=kubeconfig_path)
Collaborator
Author
There was a problem hiding this comment.
Done — implemented DynamicClientWithKubeconfig subclass as you suggested. get_client() now always returns DynamicClient (or the subclass with .kubeconfig attribute when generate_kubeconfig=True).
Collaborator
Author
There was a problem hiding this comment.
Implemented as suggested. Also moved DynamicClientWithKubeconfig, resolve_bearer_token, and save_kubeconfig to a new ocp_resources/utils/client_config.py module (renamed from kubeconfig.py).
myakove
requested changes
Mar 30, 2026
ocp_resources/resource.py
Outdated
| config_dict=config_dict, | ||
| verify_ssl=verify_ssl, | ||
| ) | ||
| return _dynamic_client, kubeconfig_path |
Collaborator
There was a problem hiding this comment.
should be implmented like:
class DynamicClientWithKubeconfig(DynamicClient):
def __init__(self, client: kubernetes.client.ApiClient, kubeconfig: str) -> None:
super().__init__(client=client)
self.kubeconfig = kubeconfig
if generate_kubeconfig:
if config_file:
LOGGER.info(f"`generate_kubeconfig` ignored, kubeconfig already available at {config_file}")
_dynamic_client = DynamicClientWithKubeconfig(client=_dynamic_client.client, kubeconfig=config_file)
else:
_resolved_token = _resolve_bearer_token(token=token, client_configuration=client_configuration)
kubeconfig_path = save_kubeconfig(
host=host or client_configuration.host,
token=_resolved_token,
config_dict=config_dict,
verify_ssl=verify_ssl,
)
_dynamic_client = DynamicClientWithKubeconfig(client=_dynamic_client.client, kubeconfig=kubeconfig_path)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ocp_resources/resource.py (1)
319-330:⚠️ Potential issue | 🟠 MajorInconsistent behavior:
config_filepath returnsDynamicClient, but other paths return tuple.When
generate_kubeconfig=Trueandconfig_fileis set, the function logs a message and falls through to return_dynamic_client(line 332). But whenconfig_fileis not set, it returns a tuple(_dynamic_client, kubeconfig_path).This means callers cannot rely on the return type even when
generate_kubeconfig=True- sometimes they get a client, sometimes a tuple. This is confusing and error-prone.🔧 Suggested fix for consistent return
If keeping the tuple approach (though subclass is preferred), always return consistent types:
if generate_kubeconfig: if config_file: LOGGER.info(f"`generate_kubeconfig` ignored, kubeconfig already available at {config_file}") + return _dynamic_client, config_file else: _resolved_token = _resolve_bearer_token(token=token, client_configuration=client_configuration) kubeconfig_path = save_kubeconfig( host=host or client_configuration.host, token=_resolved_token, config_dict=config_dict, verify_ssl=verify_ssl, ) return _dynamic_client, kubeconfig_path return _dynamic_client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp_resources/resource.py` around lines 319 - 330, The function exhibits inconsistent return types when generate_kubeconfig is True: if config_file is provided it currently returns just _dynamic_client, but when it creates a kubeconfig it returns (_dynamic_client, kubeconfig_path); fix this by making the return shape consistent—when generate_kubeconfig is True always return a tuple of ( _dynamic_client, kubeconfig_path ), and in the branch where config_file is set return (_dynamic_client, config_file) instead of falling through; update references to generate_kubeconfig, config_file, _dynamic_client, save_kubeconfig and kubeconfig_path accordingly so callers always receive the same type.
🧹 Nitpick comments (2)
tests/test_resource.py (1)
208-318: Missing integration test forget_client(generate_kubeconfig=True).The tests cover
save_kubeconfig()and_resolve_bearer_token()in isolation, but there's no test that exercises the full integration throughget_client(..., generate_kubeconfig=True). This would catch issues like the inconsistent return type behavior identified inresource.py.📝 Suggested integration test
def test_get_client_with_generate_kubeconfig(self, fake_client): """Test get_client with generate_kubeconfig=True returns tuple.""" from ocp_resources.resource import get_client result = get_client( host="https://api.example.com:6443", token="test-token", # noqa: S105 generate_kubeconfig=True, fake=False, # Need to test real path ) # Verify return type and cleanup assert isinstance(result, tuple) client, kubeconfig_path = result assert hasattr(client, 'resources') assert os.path.exists(kubeconfig_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_resource.py` around lines 208 - 318, Add an integration test that calls get_client(generate_kubeconfig=True) and verifies it returns a tuple (client, kubeconfig_path), the client object has the expected attribute (e.g., resources) and the kubeconfig file exists on disk, then clean up the file; specifically, create a test similar to the suggested snippet invoking ocp_resources.resource.get_client with host, token, generate_kubeconfig=True and fake=False (or appropriate fixture like fake_client), assert isinstance(result, tuple), unpack to (client, kubeconfig_path), assert hasattr(client, "resources"), assert os.path.exists(kubeconfig_path), and finally unlink the kubeconfig_path to avoid leftover files.ocp_resources/resource.py (1)
1490-1492: Address type narrowing with cast or assertion instead of type ignore.These
# type: ignore[union-attr]comments address a real type-checking limitation—the type checker cannot narrow theclient: DynamicClient | Noneunion after reassignment at line 1489, even though the logic guarantees a non-None value. Instead of silencing the warning, consider usingtyping.cast()to explicitly assert the narrowed type, which is more explicit about the intent and preserves type safety:from typing import cast client = cast(DynamicClient, get_client(...))This documents the assumption and allows type checkers to properly understand the type at lines 1490-1492 without suppression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp_resources/resource.py` around lines 1490 - 1492, The type ignores on client.resources.search() and client.get(...) hide a union-narrowing problem for client (DynamicClient | None); replace the "# type: ignore[union-attr]" usage by explicitly narrowing client with typing.cast (or an assert) right after you obtain it (e.g., use cast(DynamicClient, get_client(...)) or assert isinstance(client, DynamicClient)) so downstream calls in the loop (client.resources.search and client.get) have a concrete DynamicClient type and the type checker won't require ignores; update imports to include typing.cast if you use cast and remove the type: ignore comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ocp_resources/resource.py`:
- Around line 226-227: The function currently returns DynamicClient |
FakeDynamicClient | tuple[DynamicClient, str], which breaks isinstance and typed
assignments when generate_kubeconfig=True; implement a subclass (e.g.,
DynamicClientWithKubeconfig that inherits DynamicClient and adds a kubeconfig
attribute via __init__(self, client: kubernetes.client.ApiClient, kubeconfig:
str)) and change the code path that currently returns (client, kubeconfig) to
instead return an instance of DynamicClientWithKubeconfig; update the function
signature to return DynamicClient | FakeDynamicClient and ensure callers
(including assignments to self.client and isinstance checks) continue to treat
the returned object as a DynamicClient while kubeconfig can be accessed on the
subclass when needed.
In `@ocp_resources/utils/kubeconfig.py`:
- Around line 55-64: The code currently creates the kubeconfig with
tempfile.NamedTemporaryFile then calls os.chmod, creating a brief window with
permissive permissions; replace that sequence by creating the temp file
atomically with restrictive permissions (e.g., use os.open or tempfile.mkstemp
with mode 0o600), write the YAML from _config to the returned file descriptor
(or file path), close the descriptor, register the same atexit cleanup using
tmp_path/name, and keep the existing LOGGER.info, yaml.safe_dump, and
atexit.register usage (refer to tmp_file/tmp_path,
tempfile.NamedTemporaryFile/tempfile.mkstemp/os.open, os.chmod, yaml.safe_dump,
atexit.register, LOGGER.info) so the file is never exposed with wider
permissions during creation.
---
Duplicate comments:
In `@ocp_resources/resource.py`:
- Around line 319-330: The function exhibits inconsistent return types when
generate_kubeconfig is True: if config_file is provided it currently returns
just _dynamic_client, but when it creates a kubeconfig it returns
(_dynamic_client, kubeconfig_path); fix this by making the return shape
consistent—when generate_kubeconfig is True always return a tuple of (
_dynamic_client, kubeconfig_path ), and in the branch where config_file is set
return (_dynamic_client, config_file) instead of falling through; update
references to generate_kubeconfig, config_file, _dynamic_client, save_kubeconfig
and kubeconfig_path accordingly so callers always receive the same type.
---
Nitpick comments:
In `@ocp_resources/resource.py`:
- Around line 1490-1492: The type ignores on client.resources.search() and
client.get(...) hide a union-narrowing problem for client (DynamicClient |
None); replace the "# type: ignore[union-attr]" usage by explicitly narrowing
client with typing.cast (or an assert) right after you obtain it (e.g., use
cast(DynamicClient, get_client(...)) or assert isinstance(client,
DynamicClient)) so downstream calls in the loop (client.resources.search and
client.get) have a concrete DynamicClient type and the type checker won't
require ignores; update imports to include typing.cast if you use cast and
remove the type: ignore comments.
In `@tests/test_resource.py`:
- Around line 208-318: Add an integration test that calls
get_client(generate_kubeconfig=True) and verifies it returns a tuple (client,
kubeconfig_path), the client object has the expected attribute (e.g., resources)
and the kubeconfig file exists on disk, then clean up the file; specifically,
create a test similar to the suggested snippet invoking
ocp_resources.resource.get_client with host, token, generate_kubeconfig=True and
fake=False (or appropriate fixture like fake_client), assert isinstance(result,
tuple), unpack to (client, kubeconfig_path), assert hasattr(client,
"resources"), assert os.path.exists(kubeconfig_path), and finally unlink the
kubeconfig_path to avoid leftover files.
🪄 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: CHILL
Plan: Pro
Run ID: 769f73be-63af-42c0-aac1-f9d026028571
📒 Files selected for processing (3)
ocp_resources/resource.pyocp_resources/utils/kubeconfig.pytests/test_resource.py
- Renamed ocp_resources/utils/kubeconfig.py to client_config.py - Moved DynamicClientWithKubeconfig and resolve_bearer_token from resource.py - Updated all imports in resource.py and tests
rnetser
changed the title
redhat-qe-bot1
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ocp_resources/utils/client_config.py (1)
79-88:⚠️ Potential issue | 🟠 MajorRace condition between file creation and chmod.
NamedTemporaryFilecreates the file with the system's default umask (typically0o644), andos.chmodis called afterwards. There's a brief window where the file exists with potentially world-readable permissions containing sensitive tokens.Use
tempfile.mkstempfor atomic secure creation:🔒 Proposed fix using tempfile.mkstemp
- tmp_file = None try: - tmp_file = tempfile.NamedTemporaryFile(delete=False, suffix=".kubeconfig", mode="w") - os.chmod(tmp_file.name, 0o600) - yaml.safe_dump(_config, tmp_file) - tmp_file.close() + fd, tmp_path = tempfile.mkstemp(suffix=".kubeconfig") + os.fchmod(fd, 0o600) + with os.fdopen(fd, "w") as f: + yaml.safe_dump(_config, f) # Ensures the file is cleaned up when the process exits. - atexit.register(lambda p: os.unlink(p) if os.path.exists(p) else None, tmp_file.name) - LOGGER.info(f"kubeconfig saved to {tmp_file.name}") - return tmp_file.name + atexit.register(lambda p: os.unlink(p) if os.path.exists(p) else None, tmp_path) + LOGGER.info(f"kubeconfig saved to {tmp_path}") + return tmp_path - except (OSError, yaml.YAMLError): - if tmp_file is not None and os.path.exists(tmp_file.name): - os.unlink(tmp_file.name) + except (OSError, yaml.YAMLError) as exc: + if 'tmp_path' in locals() and os.path.exists(tmp_path): + os.unlink(tmp_path) LOGGER.error("Failed to save kubeconfig") raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp_resources/utils/client_config.py` around lines 79 - 88, Replace the insecure NamedTemporaryFile flow in the kubeconfig writer: stop using tempfile.NamedTemporaryFile(tmp_file) and instead create the file atomically with tempfile.mkstemp to get (fd, path), immediately set strict permissions with os.fchmod(fd, 0o600), write the YAML via os.fdopen(fd, "w") and yaml.safe_dump, close the file descriptor, register atexit.register to unlink the path, and log/return the path (update references to tmp_file.name to the mkstemp path); this removes the race window between creation and chmod (refer to NamedTemporaryFile, tempfile.mkstemp, os.fchmod, os.fdopen, atexit.register, LOGGER.info).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ocp_resources/utils/client_config.py`:
- Around line 79-88: Replace the insecure NamedTemporaryFile flow in the
kubeconfig writer: stop using tempfile.NamedTemporaryFile(tmp_file) and instead
create the file atomically with tempfile.mkstemp to get (fd, path), immediately
set strict permissions with os.fchmod(fd, 0o600), write the YAML via
os.fdopen(fd, "w") and yaml.safe_dump, close the file descriptor, register
atexit.register to unlink the path, and log/return the path (update references
to tmp_file.name to the mkstemp path); this removes the race window between
creation and chmod (refer to NamedTemporaryFile, tempfile.mkstemp, os.fchmod,
os.fdopen, atexit.register, LOGGER.info).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d3eca28-a7c3-4971-9831-a39a9ef02c5b
📒 Files selected for processing (3)
ocp_resources/resource.pyocp_resources/utils/client_config.pytests/test_resource.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_resource.py
- Replaced NamedTemporaryFile + os.chmod with mkstemp + os.fchmod - Permissions set atomically before content is written
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
generate_kubeconfigparameter toget_client()that optionally generates a kubeconfig file for the client connection.Changes
New module:
ocp_resources/utils/client_config.py(renamed from
kubeconfig.py)DynamicClientWithKubeconfig— subclass ofDynamicClientwith akubeconfigattribute containing the path to the kubeconfig fileresolve_bearer_token()— extracts bearer token from client configurationsave_kubeconfig()— builds and writes kubeconfig to a temp file with0o600permissions, auto-cleaned viaatexitUpdated:
ocp_resources/resource.pygenerate_kubeconfig: bool = Falseparameter toget_client()generate_kubeconfig=True:config_fileis available: returnsDynamicClientWithKubeconfigwithkubeconfigpointing to the existing file (logs info, no temp file created)host+tokenorconfig_dict: generates a temp kubeconfig file, returnsDynamicClientWithKubeconfigwithkubeconfigpointing to the temp filegenerate_kubeconfig=False(default): behavior unchanged, returns plainDynamicClientUsage
Summary by CodeRabbit
New Features
Tests