:bug: Fix interface detection when MAC/IP matches multiple interfaces by elfosardo · Pull Request #935 · metal3-io/ironic-image

elfosardo · 2026-03-06T10:04:17Z

When a physical interface is enslaved to a bridge (e.g. OVN-Kubernetes), both the physical interface and the bridge share the same MAC address. The bash text-parsing pipelines in get_provisioning_interface() and get_interface_of_ip() would return multi-line values like "eno12399\nbr-ex", which is not a valid interface name and causes ironic to fail to start.

Replace both functions with a Python script (detect_interface.py) that uses ip -json -detail for structured output and correctly selects a single interface when multiple match. For MAC-based detection, the selection prefers the interface that already carries an IP address (important for dnsmasq binding), then non-bridge interfaces. For IP-based detection, only the first match is returned.

The script is invoked via two subcommands:

default (no args): MAC-based detection from PROVISIONING_MACS
interface-of-ip [4|6]: IP-based detection

Unit tests are added in tests/test_detect_interface.py covering all selection branches, and a GitHub Actions workflow runs them on every PR.

Assisted-By: Claude 4.6 Opus High (Commercial License)

elfosardo · 2026-03-06T10:05:53Z

/cc @dtantsur @tuminoid @Rozzii

elfosardo · 2026-03-06T10:06:11Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid · 2026-03-06T10:14:50Z

This will supersede #788 as well.

Copilot

Pull request overview

This PR fixes incorrect interface detection when the same MAC/IP matches multiple interfaces (e.g., a physical NIC enslaved to a bridge sharing its MAC), replacing fragile bash parsing with a structured ip -json -detail based Python implementation.

Changes:

Replace bash text-parsing in get_provisioning_interface() / get_interface_of_ip() with calls to a new Python helper script.
Add scripts/detect_interface.py to select a single “best” interface for MAC-based detection and the first match for IP-based detection.
Add unit tests for the detection logic and a GitHub Actions workflow to run them on PRs.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`scripts/ironic-common.sh`	Switches interface detection helpers to invoke `detect_interface.py`.
`scripts/detect_interface.py`	Implements JSON-based interface matching and selection logic using `ip -json -detail`.
`tests/test_detect_interface.py`	Adds unit tests covering helper functions, selection logic, and CLI behavior.
`.github/workflows/unit-tests.yml`	Adds a PR workflow to run the Python unit tests.
`.gitignore`	Ignores common Python test/cache artifacts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

elfosardo · 2026-03-06T13:33:43Z

This will supersede #788 as well.

yes, sorry about that but I was tired of bash :D

elfosardo · 2026-03-06T13:40:44Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

When a physical interface is enslaved to a bridge (e.g. OVN-Kubernetes), both the physical interface and the bridge share the same MAC address. The bash text-parsing pipelines in get_provisioning_interface() and get_interface_of_ip() would return multi-line values like "eno12399\nbr-ex", which is not a valid interface name and causes ironic to fail to start. Replace both functions with a Python script (detect_interface.py) that uses `ip -json -detail` for structured output and correctly selects a single interface when multiple match. For MAC-based detection, the selection prefers the interface that already carries an IP address (important for dnsmasq binding), then non-bridge interfaces. For IP-based detection, only the first match is returned. The script is invoked via two subcommands: - default / interface-of-mac: MAC-based detection from PROVISIONING_MACS - interface-of-ip <addr> [4|6]: IP-based detection Input validation is enforced at both layers: the bash wrappers reject invalid arguments before invoking Python, and the Python functions validate subcommands and IP version independently. Unit tests are added in tests/test_detect_interface.py covering all selection branches, and a GitHub Actions workflow runs them on every PR. Assisted-By: Claude 4.6 Opus High (Commercial License) Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>

elfosardo · 2026-03-06T13:57:41Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid · 2026-03-06T14:05:04Z

This will supersede #788 as well.

yes, sorry about that but I was tired of bash :D

Thats fine, that Bash was growing out of hand.

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-06T14:09:36Z

+            sys.exit(1)
+        ip_addr: str = sys.argv[2]
+        ip_version: str | None = sys.argv[3] if len(sys.argv) > 3 else None
+        print(find_by_ip(ip_addr, ip_version))
+    else:


The CLI currently accepts and silently ignores extra positional arguments (e.g., interface-of-ip <addr> 4 extra or interface-of-mac <macs> extra). This can mask invocation bugs and makes the tool harder to use/debug. Consider validating the exact argc per subcommand and exiting with a non-zero status plus usage when too many args are provided.

Suggested change

sys.exit(1)

ip_addr: str = sys.argv[2]

ip_version: str | None = sys.argv[3] if len(sys.argv) > 3 else None

print(find_by_ip(ip_addr, ip_version))

else:

sys.exit(1)

if len(sys.argv) > 4:

print(f"ERROR: too many arguments for interface-of-ip\n{_USAGE}",

file=sys.stderr)

sys.exit(1)

ip_addr: str = sys.argv[2]

ip_version: str | None = sys.argv[3] if len(sys.argv) > 3 else None

print(find_by_ip(ip_addr, ip_version))

else:

if len(sys.argv) > 3:

print(f"ERROR: too many arguments for interface-of-mac\n{_USAGE}",

file=sys.stderr)

sys.exit(1)

It starts to smell like argparse :)

heh yeah
I see the point but maybe we just keep this simple for now and migrate to argparse later
it's just used internally anyway

Copilot · 2026-03-06T14:09:36Z

+            sys.exit(1)
+        ip_addr: str = sys.argv[2]
+        ip_version: str | None = sys.argv[3] if len(sys.argv) > 3 else None
+        print(find_by_ip(ip_addr, ip_version))


find_by_ip() can raise ValueError for an invalid IP version, but main() doesn't catch it, so users get a Python traceback instead of a consistent CLI error/usage message. Suggest catching ValueError around the find_by_ip() call, printing a concise error + usage to stderr, and exiting 1.

Suggested change

print(find_by_ip(ip_addr, ip_version))

try:

print(find_by_ip(ip_addr, ip_version))

except ValueError as exc:

print(f"ERROR: {exc}\n{_USAGE}", file=sys.stderr)

sys.exit(1)

Copilot · 2026-03-06T14:09:37Z

+class TestMain(unittest.TestCase):
+
+    @mock.patch.object(detect_interface, "detect_provisioning_interface",
+                       return_value="eth0")
+    def test_default_subcommand(self, _mock):
+        with mock.patch("sys.argv", ["detect_interface.py"]):
+            with mock.patch("builtins.print") as mock_print:
+                detect_interface.main()
+                mock_print.assert_called_once_with("eth0")
+
+    @mock.patch.object(detect_interface, "find_by_ip", return_value="eno1")
+    def test_interface_of_ip_subcommand(self, mock_find):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "interface-of-ip",
+                         "10.0.0.1", "4"]):
+            with mock.patch("builtins.print") as mock_print:
+                detect_interface.main()
+                mock_find.assert_called_once_with("10.0.0.1", "4")
+                mock_print.assert_called_once_with("eno1")
+
+    def test_interface_of_ip_missing_addr_exits(self):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "interface-of-ip"]):
+            with self.assertRaises(SystemExit) as ctx:
+                detect_interface.main()
+            self.assertEqual(ctx.exception.code, 1)
+
+    @mock.patch.object(detect_interface, "detect_provisioning_interface",
+                       return_value="eth0")
+    def test_explicit_interface_of_mac_with_arg(self, mock_detect):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "interface-of-mac",
+                         "aa:bb:cc:dd:ee:ff"]):
+            with mock.patch("builtins.print") as mock_print:
+                detect_interface.main()
+                mock_detect.assert_called_once_with("aa:bb:cc:dd:ee:ff")
+                mock_print.assert_called_once_with("eth0")
+
+    @mock.patch.object(detect_interface, "detect_provisioning_interface",
+                       return_value="eth0")
+    def test_explicit_interface_of_mac_no_arg(self, mock_detect):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "interface-of-mac"]):
+            with mock.patch("builtins.print") as mock_print:
+                detect_interface.main()
+                mock_detect.assert_called_once_with(None)
+                mock_print.assert_called_once_with("eth0")
+
+    def test_unknown_subcommand_exits(self):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "interface-of-Ip"]):
+            with self.assertRaises(SystemExit) as ctx:
+                detect_interface.main()
+            self.assertEqual(ctx.exception.code, 1)
+
+    def test_garbage_argument_exits(self):
+        with mock.patch("sys.argv",
+                        ["detect_interface.py", "foobar"]):
+            with self.assertRaises(SystemExit) as ctx:
+                detect_interface.main()
+            self.assertEqual(ctx.exception.code, 1)
+


CLI tests cover unknown subcommands and missing interface-of-ip address, but there’s no test for the user-facing behavior when an invalid IP version is provided (e.g. interface-of-ip 10.0.0.1 5) or when extra args are passed. Adding tests for these cases would prevent regressions once main() is tightened to validate args / handle ValueError cleanly.

elfosardo · 2026-03-06T14:29:34Z

I'm going to take note of all the copilot comments for a possible follow up, I think I'm ok with this for the time being, unless someone objects

tuminoid

/lgtm

Rozzii

/approve

metal3-io-bot · 2026-03-09T07:45:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rozzii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Rozzii]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

metal3-io-bot requested review from iurygregory, lentzi90 and zaneb March 6, 2026 10:04

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 6, 2026

elfosardo force-pushed the fix-provisioning-interface-detection branch from 9429b46 to f62ce29 Compare March 6, 2026 10:05

metal3-io-bot requested review from Rozzii, dtantsur and tuminoid March 6, 2026 10:05

tuminoid requested a review from Copilot March 6, 2026 10:10

Copilot started reviewing on behalf of tuminoid March 6, 2026 10:10 View session

tuminoid reviewed Mar 6, 2026

View reviewed changes

Comment thread scripts/detect_interface.py

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Comment thread scripts/detect_interface.py Outdated

Comment thread scripts/detect_interface.py Outdated

Comment thread scripts/ironic-common.sh

elfosardo force-pushed the fix-provisioning-interface-detection branch from f62ce29 to 75cceb2 Compare March 6, 2026 13:39

elfosardo requested review from Copilot and tuminoid March 6, 2026 13:40

Copilot started reviewing on behalf of elfosardo March 6, 2026 13:40 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Comment thread scripts/detect_interface.py Outdated

Comment thread scripts/ironic-common.sh

elfosardo force-pushed the fix-provisioning-interface-detection branch from 75cceb2 to bb5a5fa Compare March 6, 2026 13:56

tuminoid requested a review from Copilot March 6, 2026 14:04

Copilot started reviewing on behalf of tuminoid March 6, 2026 14:04 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

tuminoid reviewed Mar 6, 2026

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2026

Rozzii reviewed Mar 9, 2026

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2026

Rozzii added this to Metal3 - Roadmap Mar 9, 2026

Rozzii moved this to Ironic-image WIP in Metal3 - Roadmap Mar 9, 2026

metal3-io-bot merged commit b8a3a11 into metal3-io:main Mar 9, 2026
21 checks passed

metal3-io-bot added this to the ironic-image - v35.0 milestone Mar 9, 2026

github-project-automation Bot moved this from Ironic-image WIP to Done / Closed in Metal3 - Roadmap Mar 9, 2026

mchiappero mentioned this pull request Mar 9, 2026

⚠️ Improve priority and validation of input variables consumed in wait_for_interface_or_ip #788

Open

2 tasks

Conversation

elfosardo commented Mar 6, 2026

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

Uh oh!

tuminoid commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

tuminoid commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

tuminoid Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

elfosardo Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

elfosardo commented Mar 6, 2026

Uh oh!

tuminoid left a comment

Choose a reason for hiding this comment

Uh oh!

Rozzii left a comment

Choose a reason for hiding this comment

Uh oh!

metal3-io-bot commented Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tuminoid commented Mar 6, 2026 •

edited

Loading