Skip to content

run: add --ip-address option#261

Open
tofugarden wants to merge 1 commit into
nirs:mainfrom
tofugarden:run/static-ip
Open

run: add --ip-address option#261
tofugarden wants to merge 1 commit into
nirs:mainfrom
tofugarden:run/static-ip

Conversation

@tofugarden

Copy link
Copy Markdown

Objective

Allow assigning a static IP to a VM started with run. Closes #260.

Implementation notes

The argument validation is pretty strict, requiring the requested IP to be in the network subnet, but outside the DHCP range. Addresses inside the range route just fine, but this helps avoid conflicts.

Since the same VM can be reused across subnets, the cloud-init image is rebuilt if the contents of the network-config file are not as specified on the command line.

cloud-init supports setting multiple IP addresses, but we only support one command line argument. It wouldn't be that difficult to implement, it's just not needed.

The IP must be on the same subnet as the start address, but outside the
DHCP range. If the IP changes between runs, or --static-ip is no longer
set, the cidata.iso disk is re-provisioned.

@nirs nirs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Tested locally:

Switch from dhcp to ip address

% ./run ubuntu --ip-address 192.168.200.128 --start-address 192.168.200.1 --end-address 192.168.200.127 --subnet-mask 255.255.255.0
[   0.042] INFO Starting vmnet-helper for 'ubuntu' with interface id '6bbc6ce9-5e26-4b39-bc9c-28c88776d519'
[   0.213] INFO Creating cloud-init iso '/Users/nir/.vmnet-helper/vms/ubuntu/cidata.iso'
[   0.235] INFO Starting 'vfkit' virtual machine 'ubuntu' with mac address '82:e9:dd:3d:68:1f'
[   5.080] INFO VM is ready at ubuntu-vmnet-helper.local
% ssh ubuntu -- ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host noprefixroute 
       valid_lft forever preferred_lft forever
2: enp0s1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 82:e9:dd:3d:68:1f brd ff:ff:ff:ff:ff:ff
    altname enx82e9dd3d681f
    inet 192.168.200.128/24 brd 192.168.200.255 scope global enp0s1
       valid_lft forever preferred_lft forever
    inet6 fdd3:28eb:866d:8319:80e9:ddff:fe3d:681f/64 scope global dynamic mngtmpaddr noprefixroute 
       valid_lft 2591905sec preferred_lft 604705sec
    inet6 fe80::80e9:ddff:fe3d:681f/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever

Switch back to dhcp

% ./run ubuntu --start-address 192.168.200.1 --end-address 192.168.200.127 --subnet-mask 255.255.255.0 
[   0.041] INFO Starting vmnet-helper for 'ubuntu' with interface id '6bbc6ce9-5e26-4b39-bc9c-28c88776d519'
[   0.148] INFO Starting 'vfkit' virtual machine 'ubuntu' with mac address '82:e9:dd:3d:68:1f'
[   4.646] INFO VM is ready at ubuntu-vmnet-helper.local

We did not detect the change (ip-address change to None), and did not generate the ciabatta.iso.

% ssh ubuntu -- ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host noprefixroute 
       valid_lft forever preferred_lft forever
2: enp0s1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 82:e9:dd:3d:68:1f brd ff:ff:ff:ff:ff:ff
    altname enx82e9dd3d681f
    inet 192.168.200.128/24 brd 192.168.200.255 scope global enp0s1
       valid_lft forever preferred_lft forever
    inet6 fdd3:28eb:866d:8319:80e9:ddff:fe3d:681f/64 scope global dynamic mngtmpaddr noprefixroute 
       valid_lft 2591992sec preferred_lft 604792sec
    inet6 fe80::80e9:ddff:fe3d:681f/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever

So we got the same IP address in the guest.

We need to generate the network config, read the previous one, and compare. If network-config changed, we to regenerate the iso. The same way will work or user-data later if we want to change packages or mDNS implementation.

Switch back to dhcp by deleting the iso

% rm ~/.vmnet-helper/vms/ubuntu/cidata.iso 

% ./run ubuntu --start-address 192.168.200.1 --end-address 192.168.200.127 --subnet-mask 255.255.255.0
[   0.041] INFO Starting vmnet-helper for 'ubuntu' with interface id '6bbc6ce9-5e26-4b39-bc9c-28c88776d519'
[   0.166] INFO Creating cloud-init iso '/Users/nir/.vmnet-helper/vms/ubuntu/cidata.iso'
[   0.178] INFO Starting 'vfkit' virtual machine 'ubuntu' with mac address '82:e9:dd:3d:68:1f'
[   5.083] INFO VM is ready at ubuntu-vmnet-helper.local
% ssh ubuntu -- ip a                     
Warning: Permanently added 'ubuntu-vmnet-helper.local' (ED25519) to the list of known hosts.
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host noprefixroute 
       valid_lft forever preferred_lft forever
2: enp0s1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 82:e9:dd:3d:68:1f brd ff:ff:ff:ff:ff:ff
    altname enx82e9dd3d681f
    inet 192.168.200.2/24 metric 100 brd 192.168.200.255 scope global dynamic enp0s1
       valid_lft 576sec preferred_lft 576sec
    inet6 fdd3:28eb:866d:8319:80e9:ddff:fe3d:681f/64 scope global dynamic mngtmpaddr noprefixroute 
       valid_lft 2591993sec preferred_lft 604793sec
    inet6 fe80::80e9:ddff:fe3d:681f/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever

Comment thread docs/development.md

```console
./run test
--start-address 192.168.55.1 \

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This network is likely to be used by vment-helper or Apple container (both try to get the next available network and will get 192.168.64.0/24 or 192.168.65.0/24. It is better to show network which is unlikely to be used by the system like 192.168.200.0/24.

Comment thread docs/development.md
cp ~/src/socket_vmnet/test/perf.out/socket_vmnet out/bench/
```

## Static IP addresses in `run`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is nice but we are missing the basic section about using the run scripts without static ip. Do you want to add this?

Comment thread testing/cidata.py
logging.debug("Static IP changed, deleting old cloud-init iso")
os.remove(cidata)
except FileNotFoundError:
pass

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Move this to a helper function like network_config_changed(), so we can have clear flow here without unwanted details:

if network_config_changed(vm):
    silent_remove(cidata)

Or:

if os.path.exists(cidata) and !network_config_changed(vm):
    logging.debug("Reusing cloud-init iso '%s'", cidata)
    return cidata

We don't really need to delete the network config file, creating new file will overwrite it.

Keep exceptions scope narrow as possible - FileNotFoundError can raise only from the open call, so the code should be:

    try:
        with open(network_config_path, "r") as f:
            network_config = yaml.safe_load(f.read(4096))
    except FileNotFoundError:
        pass

This ensures we handle expected exceptions correctly, and we don't handle unexpected exceptions.

Comment thread testing/cidata.py
with open(network_config_path, "r") as f:
network_config = yaml.safe_load(f.read(4096))
addresses = network_config["ethernets"]["eth0"].get("addresses", [])
ip_address = ip_with_prefixlen(vm)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This call is relevant only if vm.ip_address is not None

Comment thread testing/cidata.py
return keys


def ip_with_prefixlen(vm):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's add testing/net.py for networking related functions. We need this for validating command line options and creating cidata.

Comment thread run
)

p.add_argument(
"--ip-address",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We can specify the type to function parsing IPv4 address in the private IP range (RFC 1918), and returning a IPv4 address type that can be used later for more validations. We have the same issue with other addresses. It will be best to add the missing validation to other address options before this change.

The advantage is that the validations of all the address options becomes shorter, and we fail quickly for invalid input.

If we need subnet-mask to validate the address, keep string type and validate after finishing parsing the options.

Comment thread run
ip_address = IPv4Interface(f"{args.ip_address}/{args.subnet_mask}")
if ip_address not in subnet:
p.error("--ip-address must be in the requested subnet")
if ip_address > start_address and ip_address < end_address:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not sure that <= works with IPv4Interface but want to ensure that you ip_address is not start_address or end_address, or in the range between them.

if start_address <= ip_address <= end_address:

Comment thread run
if args.enable_isolation:
p.error("--enable-isolation not compatible with --operation-mode=bridged")

if args.ip_address:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should we do this validation in testing/net.py?

net.validate_network_options(p, args)

The advantage is keeping run script simple, and make it easy to write unit tests for the delicate validations which are easy to get wrong. It is easier to write correct code when you have small function with less context to load into your head.

Comment thread run
"""

import argparse
from ipaddress import IPv4Interface

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We typically import modules, not single types. This makes it easier to tell where the type came from. The code will use ipaddress.IPv4Interface - more verbose but also more clear.

Comment thread run
start_address = IPv4Interface(f"{args.start_address}/{args.subnet_mask}")
end_address = IPv4Interface(f"{args.end_address}/{args.subnet_mask}")
subnet = start_address.network
ip_address = IPv4Interface(f"{args.ip_address}/{args.subnet_mask}")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We are missing validation for private IP range (RFC 1918) for all addresses.

m89sbyw4tk-glitch

This comment was marked as off-topic.

m89sbyw4tk-glitch

This comment was marked as off-topic.

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.

run doesn't support static IP address assignment

3 participants