Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Nov 2, 2018

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
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

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