Skip to content

Decouple definition of subnet addresses from definition of DHCP range#321

Closed
Bischoff wants to merge 1 commit intodmacvicar:masterfrom
Bischoff:master
Closed

Decouple definition of subnet addresses from definition of DHCP range#321
Bischoff wants to merge 1 commit intodmacvicar:masterfrom
Bischoff:master

Conversation

@Bischoff
Copy link
Copy Markdown
Contributor

@Bischoff Bischoff commented Jul 11, 2018

What does this PR do?

If no addresses are declared for a network, then the bridge to the network does not get an IP address on the real host, making the network (and the VMs using it) unreacheable from the real host.

If addresses are declared, then a DHCP server is started on the network, but this is not always wished.

This PR adds a new dhcp boolean flag, that lets someone, in conjunction with existing addresses list, set up an address for the bridge while not starting a DHCP server. This flag defaults to true, thus keeping compatibility with existing main.tf files.

Example

This setup:

(...)
resource "libvirt_network" "private_network" {
  name = "mynetwork"
  mode = "none"
  addresses = [ "192.168.75.0/24" ]
  dhcp = false
}
(...)

results in:

# virsh net-dumpxml mynetwork
<network connections='1'>
  <name>mynetwork</name>
  <uuid>10de3502-198d-482e-8034-5cc1950905d3</uuid>
  <bridge name='virbr2' stp='on' delay='0'/>
  <mac address='52:54:00:86:59:bd'/>
  <ip family='ipv4' address='192.168.75.1' prefix='24'>
  </ip>
</network>

Please notice the absence of unwanted:

    <dhcp>
      <range start='192.168.75.2' end='192.168.75.254'/>
    </dhcp>

The bridge gets an IP address as expected:

# ip address show
64: virbr2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 52:54:00:86:59:bd brd ff:ff:ff:ff:ff:ff
    inet 192.168.75.1/24 brd 192.168.75.255 scope global virbr2
       valid_lft forever preferred_lft forever

@dmacvicar
Copy link
Copy Markdown
Owner

So what is the equivalent of dhcp=X in the libvirt configuration? Is it also a boolean?

I think the problem is that we are adding dhcp as default. However, the dhcp configuration in libvirt is not done through boolean flags, and this is my main concern.

@Bischoff
Copy link
Copy Markdown
Contributor Author

Bischoff commented Jul 11, 2018

The equivalent of dhcp=X is the absence or presence of the <dhcp> block in libvirt XML.

I have created a separate issue #323 as a place to discuss a full refactoring of the network creation.

@Bischoff
Copy link
Copy Markdown
Contributor Author

Bischoff commented Jul 11, 2018

I just had a look at AWS and OpenStack providers, for comparaison. I do not really understand the AWS approach. For Openstack one reads:

enable_dhcp - (Optional) The administrative state of the network. Acceptable values are "true" and "false".
Changing this value enables or disables the DHCP capabilities of the existing subnet. Defaults to true.

I have renamed dhcp to enable_dhcp for compatibility just right now.

@inercia
Copy link
Copy Markdown
Contributor

inercia commented Jul 19, 2018

I think that maybe this should be part of #288, where disable-dhcp part is already implemented...

@Bischoff
Copy link
Copy Markdown
Contributor Author

Sure, if you want to make it on a wider scale, as discussed in #323, no problem on my side. @inercia, when will #288 be ready?

@inercia
Copy link
Copy Markdown
Contributor

inercia commented Jul 19, 2018

Sure, if you want to make it on a wider scale, as discussed in #323, no problem on my side. @inercia, when will #288 be ready?

@Bischoff Mostly complete, I'm trying to fix the tests right now...

@Bischoff
Copy link
Copy Markdown
Contributor Author

ping @inercia - any progress ?

@MalloZup
Copy link
Copy Markdown
Collaborator

#385

@MalloZup
Copy link
Copy Markdown
Collaborator

MalloZup commented Aug 31, 2018

@Bischoff so the pr #385 is adreessing this PR in more generic way.

I took the intial draft from @inercia and improved it .

This will work for sumaform creating the network. ( i need to fix small issues but the main functionality is already working). feel free to test it with sumaform TIA 🌹 ( let me know. If you want, i can build from src the provider and updating to ix78server the classical one so you can use it)

@MalloZup
Copy link
Copy Markdown
Collaborator

MalloZup commented Sep 3, 2018

@Bischoff i am closing this since i backported your changes to that PR linked above. ( so we can clean up a little the prs)

Thank you for your efforts 🌞

@MalloZup MalloZup closed this Sep 3, 2018
@Bischoff
Copy link
Copy Markdown
Contributor Author

Bischoff commented Sep 3, 2018

Sure. Thanks for your merging efforts @MalloZup. I will keep my branch locally for some time in case we forgot some detail.

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.

4 participants