Skip to content

fix: handle nil network name for bridge/direct interfaces#189

Merged
ekohl merged 1 commit into
fog:masterfrom
ajmeese7:fix/nil-network-name-bridge-interfaces
May 31, 2026
Merged

fix: handle nil network name for bridge/direct interfaces#189
ekohl merged 1 commit into
fog:masterfrom
ajmeese7:fix/nil-network-name-bridge-interfaces

Conversation

@ajmeese7
Copy link
Copy Markdown
Contributor

@ajmeese7 ajmeese7 commented Mar 31, 2026

Problem

When a VM has a bridge or macvtap interface (e.g. from a Vagrant public_network with :dev), libvirt represents it without a source network attribute, so the nic's network name is nil. addresses() took nics.first unconditionally and passed that nil name down to lookup_network_by_name(nil), raising:

TypeError: no implicit conversion of nil into String

This crashed vagrant-libvirt state checks on any VM with a bridge NIC.

Fix

Primary fix is in addresses(): select the first nic that actually has a network instead of blindly taking the first nic. Bridge/direct interfaces have no network name and never carry a libvirt-managed lease, so skipping them both avoids the nil lookup and gives multi-nic VMs a better chance of resolving a real address.

As defense in depth, get_network_by_filter still returns nil early when the name filter is nil, and list_networks compacts the result so Fog::Collection#load never receives a nil entry. This keeps lookup_network_by_name(nil) from raising for any other caller. Happy to drop this layer if you'd rather keep the surface minimal.

Tests

Added minitest coverage for addresses():

  • A bridge-only VM (single nic, nil network) returns no address and never performs a network lookup.
  • A mixed VM (bridge nic first, NAT nic second) resolves the address from the networked nic.

Fixes vagrant-libvirt state checks crashing on VMs with bridge NICs.

Copy link
Copy Markdown
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please address the RuboCop issue too

Comment thread lib/fog/libvirt/version.rb Outdated
@ajmeese7
Copy link
Copy Markdown
Contributor Author

Feedback addressed @ekohl

Copy link
Copy Markdown
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

fog-libvirt's addresses() passes nic.network (nil) to list_networks, which calls lookup_network_by_name(nil), raising:

I'm rereading this now and this is:

# This retrieves the ip address of the mac address using dhcp_leases
# It returns an array of public and private ip addresses
# Currently only one ip address is returned, but in the future this could be multiple
# if the server has multiple network interface
def addresses(service_arg=service, options={})
ip_address = nil
if (nic = self.nics&.first)
net = service.networks.all(:name => nic.network).first
# Assume the lease expiring last is the current IP address
ip_address = net&.dhcp_leases(nic.mac)&.max_by { |lse| lse["expirytime"] }&.dig("ipaddr")
end
return { :public => [ip_address], :private => [ip_address] }
end

In particular:
net = service.networks.all(:name => nic.network).first

Isn't the better fix to try and find the first nic with a network? So:

if (nic = self.nics&.filter { |nic| nic.network }&.first)

That gives a better chance of finding a network that actually has an ip address.

@ajmeese7
Copy link
Copy Markdown
Contributor Author

Good call, that's the better fix. Switched to selecting the first nic that has a network in addresses():

if (nic = self.nics&.find { |n| n.network })

Used find rather than filter { }.first to keep RuboCop happy (Style/Detect), and renamed the block var to n so it doesn't shadow the outer nic. I kept the list_networks nil-guard as defense in depth so lookup_network_by_name(nil) can't raise from any other caller, but I can to drop it if you'd rather keep the surface minimal.

@ekohl
Copy link
Copy Markdown
Contributor

ekohl commented May 30, 2026

Since you took the effort to write commit messages I don't want to squash for you. Would you mind squashing your commits?

When a VM has a bridge or macvtap interface (e.g. a Vagrant public_network
with :dev), libvirt represents it without a source network, so the nic's
network name is nil. #addresses took nics.first unconditionally and passed
that nil into lookup_network_by_name(nil), raising "TypeError: no implicit
conversion of nil into String" and crashing vagrant-libvirt state checks.

Select the first nic that actually has a network instead. Bridge/direct
interfaces have no network name and never carry a libvirt-managed lease, so
skipping them avoids the nil lookup and gives multi-nic VMs a better chance
of resolving a real address.

As defense in depth, get_network_by_filter returns nil early for a nil name
filter and list_networks compacts the result, so lookup_network_by_name(nil)
can't raise from any other caller.

Add minitest coverage for the bridge-only (no address, no raise) and
mixed-nic (resolves the networked nic) cases.
@ajmeese7 ajmeese7 force-pushed the fix/nil-network-name-bridge-interfaces branch from f51f3f1 to 9a9a9c9 Compare May 30, 2026 14:38
@ajmeese7
Copy link
Copy Markdown
Contributor Author

Squashed, thanks for checking!

Copy link
Copy Markdown
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit 53ec15c into fog:master May 31, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants