tinkerbell / playground

Example deployments of the Tinkerbell Stack for use as playground environments
Apache License 2.0
127 stars 85 forks source link

[Issue #112] OS X Monterey and vbox 6.1.28 compatibility changes to Vagrantfile #114

Closed ProvenGuilty closed 2 years ago

ProvenGuilty commented 2 years ago

Description

This is will change the default network from 192.168.50.0/24 to 192.168.56.0/24.

Why is this needed

This is suddenly broken in a recent version of Oracle virtualbox version 6.1.28 and greater. This can be observed when the host operating system is Linux, Mac OS X or Solaris follwing changes to allowable IP ranges on the hostonlyif of 192.168.56.0/21

Fixes: #112

How Has This Been Tested?

I have tested using an intel based MacBook Pro 16" 2019 Model running the latest Version of Mac OS X Monterey. It was discovered that the provisioner VM would not provision correctly without these changes.

Upon testing with these changes in the Vagrantfile using vbox, I was able to revert and verify it was no longer working again. There are other suggestions to manually create a networks.conf file in /etc/vbox to specify allowed RFC 1911 space. However, this pull request specifically targets the vbox config in the Vagrant file in a more desirable way, as it will work-around the issue without changing core functionality of the virtualbox installation and other user configurations that may be present.

How are existing users impacted? What migration steps/scripts do we need?

Changes the default network the provisioner and ultimately PXE network runs in from 192.168.50.0/24 to 192.168.56.0/24 for all users (not limited to Mac OS X users). Existing users will likely need to re-provision their existing installations when upgrading to Monterey and VirtualBox 6.1.28

Checklist:

ProvenGuilty commented 2 years ago

Upon successful vagrant up, it appears the following configurations are also dynamically updated. Open to suggestions if this should also be included as a commit to this pull request or if allowing them to dynamically update is more desirable.

cryan@A-1051 sandbox % git status
On branch cryan/sandbox-vagrantvboxpatchosxmonterey
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   deploy/compose/manifests/hardware/hardware.json
    modified:   deploy/compose/manifests/template/ubuntu.yaml
    modified:   deploy/compose/tls/csr.json

Diff of the changed files:

cryan@A-1051 sandbox % git diff
diff --git a/deploy/compose/manifests/hardware/hardware.json b/deploy/compose/manifests/hardware/hardware.json
index 3e89242..0fc4496 100644
--- a/deploy/compose/manifests/hardware/hardware.json
+++ b/deploy/compose/manifests/hardware/hardware.json
@@ -15,7 +15,7 @@
         "dhcp": {
           "arch": "x86_64",
           "ip": {
-            "address": "192.168.50.43",
+            "address": "192.168.56.43",
             "netmask": "255.255.255.0"
           },
           "mac": "08:00:27:9e:f5:3a",
diff --git a/deploy/compose/manifests/template/ubuntu.yaml b/deploy/compose/manifests/template/ubuntu.yaml
index 6cac7d8..65fd757 100644
--- a/deploy/compose/manifests/template/ubuntu.yaml
+++ b/deploy/compose/manifests/template/ubuntu.yaml
@@ -14,7 +14,7 @@ tasks:
         timeout: 600
         environment:
           DEST_DISK: /dev/sda
-          IMG_URL: "http://192.168.50.4:8080/focal-server-cloudimg-amd64.raw.gz"
+          IMG_URL: "http://192.168.56.4:8080/focal-server-cloudimg-amd64.raw.gz"
           COMPRESSED: true
       - name: "install-openssl"
         image: cexec:v1.0.0
diff --git a/deploy/compose/tls/csr.json b/deploy/compose/tls/csr.json
index 2a896d2..c4a135e 100644
--- a/deploy/compose/tls/csr.json
+++ b/deploy/compose/tls/csr.json
@@ -1,6 +1,7 @@
 {
   "CN": "tinkerbell",
   "hosts": [
+    "192.168.56.4",
     "tinkerbell.registry",
     "tinkerbell.tinkerbell",
     "tinkerbell",
mmlb commented 2 years ago

Hey @ProvenGuilty welcome. I've updated the PR description a bit so it doesn't seem to be vbox specific (since you did end up messing with libvirt config). Please do update the modified files in deploy/ directory.

mmlb commented 2 years ago

Maybe only do the v.gui = true if the os is macos?

ProvenGuilty commented 2 years ago

Sorry for the sloppiness I almost never use public GitHub and there are so many things new to me on this platform. :)

nshalman commented 2 years ago

@tstromberg linked me to https://github.com/kubernetes/minikube/pull/12811 which may help in reviewing this.

mmlb commented 2 years ago

Virtualbox 6.1.30 is out with fixes for the gui https://www.virtualbox.org/wiki/Changelog can you try that out @ProvenGuilty ?

ProvenGuilty commented 2 years ago

Verified with 6.1.30 installed on OSX Monterey. This test was performed with commenting out the conditional I added in previous commit: https://gist.github.com/ProvenGuilty/75f7430e74171bcbbd194b4d3c1c0a4a

When re-testing using the conditional, it still ran successfully with a gui window poppping up. I would conclude that virtualbox fixed the introduced bug and the conditional for darwin/macosx is no longer necessary.

mmlb commented 2 years ago

Nice, can you drop the gui stuff from the PR then?

ProvenGuilty commented 2 years ago

Closing PR, this doesn't seem to be an issue any more when switching back to 192.168.50.0/24 either..

ProvenGuilty commented 2 years ago

I lied. It wasn't until I destroyed and re-created the provisioner that I found the error again:

allowed ranges. Please update the address used to be within the allowed
ranges and run the command again.

  Address: 192.168.50.4
  Ranges: 192.168.56.0/21

Valid ranges can be modified in the /etc/vbox/networks.conf file. For
more information including valid format see:

  https://www.virtualbox.org/manual/ch06.html#network_hostonly
ProvenGuilty commented 2 years ago

I don't know why DCO is still unhappy, I tried to fix it like I was able to previously.

ProvenGuilty commented 2 years ago

I dug further into this and found the netmask was embedded in many more files and is still an issue for Linux, OSX and Solaris users running VirtualBox 6.1.28 or higher. Reopening this once more to apply the changes uniform across tf, vbox and libvirtd.

ProvenGuilty commented 2 years ago

@mmlb & @nshalman this still wanted? I recall discussion around if you were wanting to remove vbox support altogether.

mmlb commented 2 years ago

Yes indeed, lgtm. I've rebased and squashed all the intermediate changes to get rid of things we didn't need.

mmlb commented 2 years ago

I just remembered we have a label I can apply to get this tested in ci, so lets see how that goes.

mmlb commented 2 years ago

Which failed, but I very much doubt its because of this PR. Looks like a vagrant issue.

mmlb commented 2 years ago

Good stuff, thanks @ProvenGuilty !

ProvenGuilty commented 2 years ago

Thank you! This is the first project I've ever contributed to!