Repository navigation
fix: eliminate TOCTOU bug in static IP preservation check - #494
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSingle-file refactor in ChangesStatic IP Preservation Verification Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
Container Operations
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
AI Features
💡 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/post_migration.py (1)
407-425:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLOW: Fix the stale docstring contract.
The signature now defaults to
660, but the docstring still says600, andRaises:still advertises a genericExceptioneven though this path now surfaces concrete failures (TimeoutError,ValueError, assertions). Stale contract docs make triage harder for callers and for future test updates. As per coding guidelines, "All new functions must have docstrings with standard sections:Args(when function has parameters),Returns(only when returning meaningful non-None value),Raises(only when exceptions are actually raised)".🤖 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/post_migration.py` around lines 407 - 425, Update the docstring for the function that verifies static IPs (parameters: vm_name, vm_ssh_connections, source_vm_data, source_provider_data, timeout, retry_delay) so the timeout default reads 660 (not 600), ensure Args reflect those parameters, add a Raises section that lists the concrete exceptions the function may surface (e.g., TimeoutError, ValueError, AssertionError) instead of a generic Exception, and include a Returns section only if the function returns a meaningful non-None value (otherwise omit Returns).
🤖 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/post_migration.py`:
- Around line 456-465: Split verify_all_static_ips into testable helpers:
extract transport setup and command execution into a new
_load_current_interfaces() that returns parsed interface data, and extract the
matching/matching-bookkeeping logic into
_match_expected_static_ips(current_interfaces, expected_static_ips) that returns
the list of matched interface configs or indicates missing entries; keep the
retry closure in verify_all_static_ips only coordinating retries, calling
_load_current_interfaces and _match_expected_static_ips, and mapping any
CommandExecFailed/timeout into the same exceptions; ensure each new helper has a
leading underscore, stays under ~50 lines, and that names referenced here
(verify_all_static_ips, _load_current_interfaces, _match_expected_static_ips)
are used to locate and replace the corresponding blocks in the original
function.
- Around line 493-500: The current loop in the interface matching logic uses a
permissive substring test (interface_name.lower() in iface_name.lower()) and
breaks on first match, which allows fuzzy name matches to beat later exact MAC
matches; refactor the matching in post_migration.py so you first normalize MACs
(source_mac/dest_mac) and scan all current_interfaces for an exact MAC match and
pick that if found, otherwise scan for an exact normalized name match (compare
normalized iface_name == interface_name), and only if neither yields a hit
perform a substring search that must return a single unambiguous candidate;
update the loop around current_interfaces so it collects candidates for each
phase instead of breaking on first hit and then selects the best match in order
(exact MAC -> exact name -> unique substring).
---
Outside diff comments:
In `@utilities/post_migration.py`:
- Around line 407-425: Update the docstring for the function that verifies
static IPs (parameters: vm_name, vm_ssh_connections, source_vm_data,
source_provider_data, timeout, retry_delay) so the timeout default reads 660
(not 600), ensure Args reflect those parameters, add a Raises section that lists
the concrete exceptions the function may surface (e.g., TimeoutError,
ValueError, AssertionError) instead of a generic Exception, and include a
Returns section only if the function returns a meaningful non-None value
(otherwise omit Returns).
🪄 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: 085be161-8b12-4963-9ae0-70197d827f21
📒 Files selected for processing (1)
utilities/post_migration.py
Collaborator
|
/lgtm |
krcmarik
added a commit
to krcmarik/mtv-api-tests
that referenced
this pull request
May 18, 2026
Address CodeRabbit review feedback on PR RedHatQE#494: - Extract _load_current_interfaces() and _match_expected_static_ips() as module-level helpers for independent testability - Implement phased interface matching priority: exact MAC match first, then exact name match, then unique substring name match (prevents false positives from ambiguous substring matches) - Update docstring with correct timeout default (660s) and specific exception types
|
Clean rebase detected — no code changes compared to previous head ( |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/post_migration.py`:
- Around line 432-436: The code currently calls _parse_windows_network_config or
_parse_linux_network_config and can propagate exceptions from malformed/partial
output; change this polling-path return to wrap the parser call in a try/except
that catches parsing errors (Exception) and logs a warning (using
LOGGER.warning) including the exception and the raw stdout, then return None so
the caller treats it like empty output and retries; apply the same defensive
try/except pattern to the other polling site that currently hard-fails (the code
around the _parse_* invocation referenced at lines 603-605) so parse failures do
not abort the wait window.
- Around line 402-407: The parameter annotation for _load_current_interfaces
currently uses ssh_conn: Any which disables static checks; update the signature
to use ssh_conn: VMSSHConnection and import VMSSHConnection from
utilities.ssh_utils (remove or keep Any only if still used elsewhere), ensuring
the function and its docstring references (rrmngmnt_host, rrmngmnt_user,
local_port) match the concrete VMSSHConnection attributes so type checkers can
validate attribute access; update any other occurrences in this file that
reference the old Any annotation for the same conceptual SSH connection to use
VMSSHConnection as well.
🪄 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: c627df4c-7ddf-4d7c-94c9-fd8be80b28ab
📒 Files selected for processing (1)
utilities/post_migration.py
Collaborator
Author
|
/verifed |
|
Cherry-pick requested for PR: |
Collaborator
Author
|
/verified |
Collaborator
|
/lgtm |
|
Cherry-picked PR fix: eliminate TOCTOU bug in static IP preservation check into v2.11: #504 |
|
Cherry-picked PR fix: eliminate TOCTOU bug in static IP preservation check into v2.10: #505 |
redhat-qe-bot
pushed a commit
that referenced
this pull request
May 19, 2026
* fix: eliminate TOCTOU bug in static IP preservation check Refactor check_static_ip_preservation() to verify all static IPs simultaneously instead of sequentially to ensure that all Static IPs are present on the migrated guest at the moment (to catch a possible bug when firstboot script on the guest wrongly overwrites already configured primary Static IP. - Single TimeoutSampler verifies all static IPs on each iteration - One SSH call per retry instead of one per IP - Use dict(matched) shallow copy to prevent mutation - Use zip(..., strict=True) for length safety * refactor: extract helper functions from static IP verification closure Address CodeRabbit review feedback on PR #494: - Extract _load_current_interfaces() and _match_expected_static_ips() as module-level helpers for independent testability - Implement phased interface matching priority: exact MAC match first, then exact name match, then unique substring name match (prevents false positives from ambiguous substring matches) - Update docstring with correct timeout default (660s) and specific exception types * refactor: use VMSSHConnection type instead of Any in _load_current_interfaces Addresses CodeRabbit review comment on PR #494. Replaces the generic Any annotation with the concrete VMSSHConnection type for static type safety and updates the docstring accordingly. Co-authored-by: Marián Krčmárik <mkrcmari@redhat.com>
redhat-qe-bot2
pushed a commit
that referenced
this pull request
May 19, 2026
* fix: eliminate TOCTOU bug in static IP preservation check Refactor check_static_ip_preservation() to verify all static IPs simultaneously instead of sequentially to ensure that all Static IPs are present on the migrated guest at the moment (to catch a possible bug when firstboot script on the guest wrongly overwrites already configured primary Static IP. - Single TimeoutSampler verifies all static IPs on each iteration - One SSH call per retry instead of one per IP - Use dict(matched) shallow copy to prevent mutation - Use zip(..., strict=True) for length safety * refactor: extract helper functions from static IP verification closure Address CodeRabbit review feedback on PR #494: - Extract _load_current_interfaces() and _match_expected_static_ips() as module-level helpers for independent testability - Implement phased interface matching priority: exact MAC match first, then exact name match, then unique substring name match (prevents false positives from ambiguous substring matches) - Update docstring with correct timeout default (660s) and specific exception types * refactor: use VMSSHConnection type instead of Any in _load_current_interfaces Addresses CodeRabbit review comment on PR #494. Replaces the generic Any annotation with the concrete VMSSHConnection type for static type safety and updates the docstring accordingly. Co-authored-by: Marián Krčmárik <mkrcmari@redhat.com>
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
3 tasks done
This was referenced Jul 21, 2026
3 tasks done
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
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.
Refactor check_static_ip_preservation() to verify all static IPs simultaneously instead of sequentially to ensure that all Static IPs are present on the migrated guest at the moment (to catch a possible bug when firstboot script on the guest wrongly overwrites already configured primary Static IP.
Summary by CodeRabbit
Bug Fixes
Chores