Skip to content

provider/virtualbox: Refactor host-only network settings#7699

Merged
chrisroberts merged 3 commits intohashicorp:masterfrom
legal90:refactor-network
Nov 2, 2018
Merged

provider/virtualbox: Refactor host-only network settings#7699
chrisroberts merged 3 commits intohashicorp:masterfrom
legal90:refactor-network

Conversation

@legal90
Copy link
Contributor

@legal90 legal90 commented Aug 10, 2016

I've refactored VagrantPlugins:: ProviderVirtualBox::Action:: Network#hostonly_config in order to make it more simpler, readable and safer to use:

  • Simplified calculations of IP and DHCP settings. Handle IPs as IPAddr instances, not strings.
    Use IPAddr methods instead of Vagrant::Util::NetforkIP#network_address and string splitting.
  • Added a basic validation of IP settings. A human-readable exception will be raised if the specified IP is invalid or netmask could not be used with that IP family. Currently, it is done only for private_network (aka host-only), but the validation could be moved to the global scope if you think it's reasonable. Examples:
# Vagranttile
config.vm.network "private_network", ip: "invalid-ip", netmask: '255.255.0.0'


# `vagrant up` output:
Network settings specified in your Vagrantfile are invalid:

Network settings: {:auto_config=>true, :mac=>nil, :nic_type=>nil, :type=>:static, :ip=>"invalid-ip", :netmask=>"255.255.0.0", :protocol=>"tcp", :id=>"d05cca65-6cb5-46f1-ba07-c891923e3219"}
Error: invalid address
# Vagranttile
config.vm.network "private_network", ip: "dead:beef::", netmask: '255.255.0.0'


# `vagrant up` output:
Network settings specified in your Vagrantfile are invalid:

Network settings: {:auto_config=>true, :mac=>nil, :nic_type=>nil, :type=>:static6, :ip=>"dead:beef::", :netmask=>"255.255.0.0", :protocol=>"tcp", :id=>"dddf6dea-0ee7-4afc-9d93-7dbd297661f6"}
Error: address family is not same

Existing unit tests for this class are passed. I've also verified it with manual tests on real VMs.

@legal90
Copy link
Contributor Author

legal90 commented Dec 22, 2016

@chrisroberts Could you please review this?

@legal90 legal90 force-pushed the refactor-network branch from eda3633 to 3077ee3 Compare May 2, 2017 09:21
@legal90
Copy link
Contributor Author

legal90 commented May 2, 2017

I've rebased the branch and fixed a merge conflict.

@legal90
Copy link
Contributor Author

legal90 commented Oct 28, 2018

Rebased

@chrisroberts chrisroberts modified the milestones: Secondary, 2.2.1 Oct 29, 2018
@chrisroberts chrisroberts self-requested a review October 29, 2018 15:28
@chrisroberts chrisroberts self-assigned this Oct 29, 2018
Copy link
Member

@chrisroberts chrisroberts left a comment

Choose a reason for hiding this comment

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

Just updated the error output to be formatted instead of the raw hash. Otherwise, this looks great! Thanks so much and thanks for the rebases (and patience 😃) Cheers!

@chrisroberts chrisroberts merged commit be6f683 into hashicorp:master Nov 2, 2018
@legal90 legal90 deleted the refactor-network branch November 3, 2018 05:48
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants