From 2c8d36ef70f5cd39ec99fd591d7986c88e12aa03 Mon Sep 17 00:00:00 2001 From: Nikolay Edigaryev Date: Thu, 13 Feb 2025 16:35:12 +0400 Subject: [PATCH] Always randomize MAC address (#251) * Always randomize MAC address * Worker: check DHCP lease time and print a warning if it's unconfigured * Further improve the explanation * Add two leases example to the explanation * Add an example of the resulting /var/db/dhcpd_leases --- .../worker/dhcpleasetime/dhcpleasetime.go | 56 +++++++++++++++ internal/worker/vmmanager/vm.go | 68 ++++++++++++++++--- internal/worker/worker.go | 5 ++ 3 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 internal/worker/dhcpleasetime/dhcpleasetime.go diff --git a/internal/worker/dhcpleasetime/dhcpleasetime.go b/internal/worker/dhcpleasetime/dhcpleasetime.go new file mode 100644 index 0000000..b76ebb4 --- /dev/null +++ b/internal/worker/dhcpleasetime/dhcpleasetime.go @@ -0,0 +1,56 @@ +package dhcpleasetime + +import ( + "errors" + "fmt" + "howett.net/plist" + "os" +) + +const ( + internetSharingPath = "/Library/Preferences/SystemConfiguration/com.apple.InternetSharing.default.plist" +) + +type InternetSharing struct { + Bootpd Bootpd `plist:"bootpd"` +} + +type Bootpd struct { + DHCPLeaseTimeSecs int `plist:"DHCPLeaseTimeSecs"` +} + +func Check() error { + errDefault := fmt.Errorf("it seems that you're using a default DHCP lease time of 1 day which may result in " + + "failures communicating with the VMs, please read https://tart.run/faq/#changing-the-default-dhcp-lease-time " + + "for more details and intstructions on how to fix that") + + plistBytes, err := os.ReadFile(internetSharingPath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return errDefault + } + + return fmt.Errorf("failed to check the default DHCP lease time: %w, please read "+ + "https://tart.run/faq/#changing-the-default-dhcp-lease-time for more details", err) + } + + var internetSharing InternetSharing + + _, err = plist.Unmarshal(plistBytes, &internetSharing) + if err != nil { + return fmt.Errorf("failed to check the default DHCP lease time: %w, please read "+ + "https://tart.run/faq/#changing-the-default-dhcp-lease-time for more details", err) + } + + if internetSharing.Bootpd.DHCPLeaseTimeSecs == 0 { + return errDefault + } + + if internetSharing.Bootpd.DHCPLeaseTimeSecs > 3600 { + return fmt.Errorf("it seems that you're using a DHCP lease time larger than 1 hour which may result in " + + "failures communicating with the VMs, please read https://tart.run/faq/#changing-the-default-dhcp-lease-time " + + "for more details and intstructions on how to fix that") + } + + return nil +} diff --git a/internal/worker/vmmanager/vm.go b/internal/worker/vmmanager/vm.go index 1a6155c..010220f 100644 --- a/internal/worker/vmmanager/vm.go +++ b/internal/worker/vmmanager/vm.go @@ -225,15 +225,67 @@ func (vm *VM) cloneAndConfigure(ctx context.Context) error { } } - // Randomize the VM's MAC-address when using bridged networking - // to avoid collisions when cloning from an OCI image on multiple hosts + // Randomize VM's MAC-address, this is important when using shared (NAT) networking + // with full /var/db/dhcpd_leases file (e.g. 256 entries) having an expired entry + // for a MAC address used by some OCI image, for example: // - // See https://github.com/cirruslabs/orchard/issues/181 for more details. - if vm.Resource.NetBridged != "" { - _, _, err = tart.Tart(ctx, vm.logger, "set", "--random-mac", vm.id()) - if err != nil { - return err - } + // { + // name=adminsVlMachine + // ip_address=192.168.64.2 + // hw_address=1,11:11:11:11:11:11 + // identifier=1,11:11:11:11:11:11 + // lease=0x1234 + //} + // + // The next VM to start with a MAC address 22:22:22:22:22:22 will assume that + // 192.168.64.2 is free (because its lease expired a long time ago) and will + // add a new entry using its MAC address and 192.168.64.2 to the + // /var/db/dhcpd_leases and won't delete the old entry: + // + // { + // name=adminsVlMachine + // ip_address=192.168.64.2 + // hw_address=1,11:11:11:11:11:11 + // identifier=1,11:11:11:11:11:11 + // lease=0x1234 + // } + // { + // name=adminsVlMachine + // ip_address=192.168.64.2 + // hw_address=1,22:22:22:22:22:22 + // identifier=1,22:22:22:22:22:22 + // lease=0x67ade532 + // } + // + // Afterward, when an OCI VM with MAC address 11:11:11:11:11:11 is cloned and run, + // it will re-use the 192.168.64.2 entry instead of creating a new one, even through + // its lease had already expired. The resulting /var/db/dhcpd_leases will look like this: + // + // { + // name=adminsVlMachine + // ip_address=192.168.64.2 + // hw_address=1,11:11:11:11:11:11 + // identifier=1,11:11:11:11:11:11 + // lease=0x67ade5c6 + // } + // { + // name=adminsVlMachine + // ip_address=192.168.64.2 + // hw_address=1,22:22:22:22:22:22 + // identifier=1,22:22:22:22:22:22 + // lease=0x67ade532 + // } + // + // As a result, you will see two VMs with different MAC address using an identical + // IP address 192.168.64.2. + // + // Another scenarion when this is important is when using bridged networking + // to avoid collisions when cloning from an OCI image on multiple hosts[1]. + // + // [1]: https://github.com/cirruslabs/orchard/issues/181 + _, _, err = tart.Tart(ctx, vm.logger, "set", "--random-mac", vm.id()) + if err != nil { + return err } if vm.Resource.RandomSerial { diff --git a/internal/worker/worker.go b/internal/worker/worker.go index 8dfde99..45b874f 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/avast/retry-go/v4" "github.com/cirruslabs/orchard/internal/opentelemetry" + "github.com/cirruslabs/orchard/internal/worker/dhcpleasetime" "github.com/cirruslabs/orchard/internal/worker/iokitregistry" "github.com/cirruslabs/orchard/internal/worker/ondiskname" "github.com/cirruslabs/orchard/internal/worker/tart" @@ -88,6 +89,10 @@ func New(client *client.Client, opts ...Option) (*Worker, error) { } func (worker *Worker) Run(ctx context.Context) error { + if err := dhcpleasetime.Check(); err != nil { + worker.logger.Warnf("%v", err) + } + for { if err := worker.runNewSession(ctx); err != nil { return err