Stop reporting changes for configured alias (#51068) - #63113
Conversation
|
Hey @waynew , just want to check if there's anything missing in that PR for review. |
bedfbdc to
fbf16c4
Compare
Ch3LL
left a comment
Ch3LL
left a comment
There was a problem hiding this comment.
This will also require test coverage
Ch3LL
left a comment
Ch3LL
left a comment
There was a problem hiding this comment.
We actually are migrating all of our tests to pytests in tests/pytests. Any chance you would be willing to add these new tests there instead?
|
@lvinagre looks like there are some failing tests related to your changes. |
|
Hey @twangboy, I've seen movement here a few times now. Please let me know if anything is required from my part for this to be merged. |
|
Looks like you have some failing tests. |
@twangboy, I tried including the management of the packages missing in the integration tests. I did comment in the past that those failures were related to the underlying modules and not to the state itself. Let's see if those test pass now. |
|
@twangboy, I spent a bit more time on this now and was able to find some stuff that was actually broken. I've put all into a single commit, rebased and put it through pre-commit and it looks good. I've also tested the final thing in in the distros that were failing (debian 11/12 and rocky 8/9) and all tests passed. Hopefully now we are good to go, my apologies for the previous/broken stuff. |
|
@twangboy , can you please check the failed tests results when you have a moment? It seems they are unrelated to my change now and got dragged by rebasing. All the "Test Salt" tests completed successfully, which, from what I understood, is where my changes are tested. |
|
We'll need to wait until tests are passing on the master branch, then rebase this and see what happens. |
|
Please resolve the conflicts |
done |
|
@lvinagre Thank you |
twangboy
left a comment
twangboy
left a comment
There was a problem hiding this comment.
This should be created on the earliest supported branch where the bug exists. Probably 3006.x
| interface_status = True | ||
| interface_status = False | ||
|
|
||
| # Handle interface as desired |
There was a problem hiding this comment.
Unconditionally splitting the interface string on a colon breaks support for complex interface naming conventions (like VLAN tags and sub-interfaces) and incorrectly conflates a parent interface's status with its un-validated alias configurations.
By completely deleting that section and substituting the raw split(":"), the code now blindly conflates the parent interface's operational status with the status of the alias itself. If the parent interface (eth0) is up, Salt will now automatically assume the alias (eth0:1) is up and up-to-date—even if the virtual IP allocation for that alias completely failed to bind on the host.
That deleted code was the only mechanism verifying that the specific label (second.get("label") == name) was genuinely active in the system's runtime network state.
Instead of an unconditional split(":"), Salt should use a targeted regex match to separate a true legacy sub-interface/alias label from a standard interface name. Then, it needs to query the actual nested secondary data structure to confirm that the specific virtual label is bound and active.
# Bring up/shutdown interface
try:
# Get Interfaces information
interfaces = salt.utils.network.interfaces()
interface_status = False
# 1. Check if the exact interface name exists directly
if name in interfaces:
interface_status = interfaces[name].get("up", False)
# 2. Safely check for legacy alias structures (e.g., eth0:1, bond0:vlan1)
else:
import re
# Match standard interface naming followed by a numeric or label alias
match = re.match(r"^([a-zA-Z0-9.\-_]+):([a-zA-Z0-9_\-]+)$", name)
if match:
parent_iface = match.group(1)
if parent_iface in interfaces:
# Look for the explicit alias label in the secondary IP data structures
for family in ("secondary", "inet", "inet6"):
if family in interfaces[parent_iface]:
for data in interfaces[parent_iface][family]:
if data.get("label") == name:
interface_status = True
break
if interface_status:
break
interface_status = interface_status or False
# Handle interface as desired
if enabled:4 participants
What does this PR do?
This fixes an issue where Salt is reporting changes for networking aliases when they are already configured as expected and no changes are done.
What issues does this PR fix or reference?
Fixes: #51068
Previous Behavior
New Behavior
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
I am not good at writing tests, but at least the current ones are passing:
Commits signed with GPG?
No