Fix expose with bridge and NAT

[frankiejol]

Properly sets the SNAT always to the interface IP of the host. Thanks to @gmiranda

[Copilot]

Pull request overview

This PR adjusts how Ravada chooses the host-side IP for exposed-port NAT rules when the VM host is behind NAT and/or has a configured display_ip. The goal is to ensure SNAT consistently uses the correct host interface IP (selected based on the client/remote IP) rather than the host’s generic resolved IP.

Changes:

• Use the VM’s interface IP (based on remote_ip) when opening exposed ports, so SNAT targets the correct host interface address.
• Add Ravada::VM::interface_ip() as a cached wrapper around _interface_ip().
• Extend bridge/ports tests to cover a NAT + display_ip scenario and optionally enforce a bridge MAC from config.

Reviewed changes

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

File	Description
t/vm/93_ports_bridge.t	Adds NAT + display_ip bridge test coverage and optional MAC override for bridge interfaces.
lib/Ravada/VM.pm	Introduces interface_ip() wrapper (with caching) for selecting the correct interface IP.
lib/Ravada/Domain.pm	Uses interface_ip($remote_ip) for exposed-port rules and propagates remote_ip into the open-exposed-ports request.

Comments suppressed due to low confidence (1)

t/vm/93_ports_bridge.t:60

• _set_mac_address assumes the bridge MAC node exists in the domain XML. If it doesn't (or the XML shape differs), $dev will be undef and setAttribute will die with a non-obvious error. Add an explicit check with a clear failure message.

        my $doc = XML::LibXML->load_xml( string => $domain->xml_description());
        my ($dev) = $doc->findnodes('/domain/devices/interface[@type="bridge"]/mac');
        $dev->setAttribute('address' => $CONFIG_BRIDGE->{mac});
        $domain->reload_config($doc);

---

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

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (10)

t/vm/93_ports_bridge.t:255

• Same run3 argument issue here: \($in, $out, $err) doesn't capture output/error. Pass undef, \$out, \$err (or \$in, \$out, \$err).

    run3(['iptables','-t','nat','-L','POSTROUTING','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:261

• Same run3 argument issue here: stdout/stderr aren't being captured into $out/$err due to the \($in, $out, $err) usage.

    run3(['iptables-save','-t','nat'],\($in, $out, $err));

t/vm/93_ports_bridge.t:267

• Same run3 argument issue here: \($in, $out, $err) doesn't capture output/error, so later parsing/greps can fail incorrectly.

    run3(['iptables','-L','FORWARD','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:272

• Same run3 argument issue here: \($in, $out, $err) doesn't capture output/error. Pass undef, \$out, \$err (or \$in, \$out, \$err).

    run3(['iptables','-L','FORWARD','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:285

• Same run3 argument issue here inside the cleanup loop: output/error aren't captured into $out/$err due to \($in, $out, $err) being passed as a single argument.

        run3(['iptables','-t','nat','-L','PREROUTING','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:296

• Same run3 argument issue here: to make the @out parsing and assertions reliable, capture stdout/stderr explicitly.

    run3(['iptables','-t','nat','-L','PREROUTING','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:303

• Same run3 argument issue here: \($in, $out, $err) doesn't capture stdout/stderr into the variables used below.

    run3(['iptables','-t','nat','-L','POSTROUTING','-n'],\($in, $out, $err));

t/vm/93_ports_bridge.t:55

• _set_mac_address returns early unless bridge.conf has a mac key, but the Void branch actually requires an ip key (and doesn't use mac). This means Void domains won't be configured when only ip is provided.

sub _set_mac_address($domain) {
    return if ! $CONFIG_BRIDGE || !exists $CONFIG_BRIDGE->{mac};
    if ($domain->type eq 'KVM') {

t/vm/93_ports_bridge.t:44

• _wait_ip defines $remote_ip but never uses it; this looks like leftover code and adds noise to a frequently-called helper.

sub _wait_ip($domain) {
    my $remote_ip = '1.2.3.5';
    for ( 1 .. 30 ) {

lib/Ravada/VM.pm:939

• bridge_ip caches results under the _interface_ip_... key, which collides with interface_ip's cache and can return an interface-ip result when a bridge-ip is requested (or vice versa) for the same argument string. Use a distinct cache key prefix for bridge_ip.

    my $key = '_interface_ip_'.($domain_ip // '');
    return $self->{$key} if exists $self->{$key};