search

csmart / ansible-role-virt-infra

Define and manage guests and networks on a KVM host with Ansible
GNU General Public License v3.0
67 stars 48 forks source link

add support for IPv6 with forward mode="route" #34

Closed frgomes closed 3 years ago

frgomes commented 3 years ago

Closes #33

frgomes commented 3 years ago

Since this is my first contact with this code, I'm afraid that I've missed proper defaults or something which should be done. Also, notice that there are other forward modes and other "flavors" for networking not addressed by the current implementation. In other words: Please let me know which fixes to this PR are desirable and I'm happy to fix and submit it once again.

csmart commented 3 years ago

Since this is my first contact with this code, I'm afraid that I've missed proper defaults or something which should be done. Also, notice that there are other forward modes and other "flavors" for networking not addressed by the current implementation. In other words: Please let me know which fixes to this PR are desirable and I'm happy to fix and submit it once again.

Thanks for the contribution! :+1: I have made a few comments, let me know what you think. Cheers!

csmart commented 3 years ago

Thanks, it's looking good! I tested it with my patch for the device and using these two, networks, both worked (just needed to add static route on LAN router):

        - name: "routed"
          domain: routed.test.local
          type: route
          mac: 52:54:00:f9:01:00
          ip_address: 10.249.1.1
          ip_netmask: 255.255.255.0
          dhcp_start: 10.249.1.11
          dhcp_end: 10.249.1.254
          ip6_address: 2001:0db8::f901:1
          ip6_prefix: 64
          dhcp6_start: 2001:0db8::f901:0000
          dhcp6_end: 2001:0db8::f901:00ff
        - name: "routed2"
          type: route
          ip_address: 10.249.2.1
          ip_netmask: 255.255.255.0
          dhcp_start: 10.249.2.11
          dhcp_end: 10.249.2.254
csmart commented 3 years ago

Oh, mind you I did not test IPv6...

frgomes commented 3 years ago

Oh, mind you I did not test IPv6...

I didn't test either using your ansible scripts, but I´ve tested deploying VMs by hand using the very same bridge configuration. So I would expect that it just works, since the XML file I'm generating is identical to the way it was before. If you prefer to hold the PR until I deploy VMs using your ansible scripts and have everything tested end to end, please let me know. When I have everything tested I will inform you, in this case.

frgomes commented 3 years ago

Please notice that the default IPv4 network as per Libvirt documentation is 192.168.122.0/24 and not 192.168.112.0/24. I've fixed that on defaults/main.yml.

frgomes commented 3 years ago

I've made nat_dev the default when host_dev is not present.

Notice that I've not tested it since I could not find documentation for an example of NATed configuration. If you copy/paste an example configuration here, I can add documentation for NATed networking in README.md and I can test it. Also, I would mention that nat_dev is now deprecated and should be replaced. Also, I would add some logic into validations.yml warning that nat_dev is deprecated.

Thanks

csmart commented 3 years ago

I've made nat_dev the default when host_dev is not present.

OK, I'll take a look and see... thanks.

Notice that I've not tested it since I could not find documentation for an example of NATed configuration. If you copy/paste an example configuration here, I can add documentation for NATed networking in README.md and I can test it. Also, I would mention that nat_dev is now deprecated and should be replaced. Also, I would add some logic into validations.yml warning that nat_dev is deprecated.

Sure, although it should be in the README as NAT is the default network type for libvirt networks, where you get a local network that's not routable across the physical network and the KVM host NATs traffic out.

This is the example in the README.

# Networks on kvmhost are a list, you can add more than one
# You can create and remove NAT networks on kvmhost (creating bridges not supported)
# The 'default' network is the standard one shipped with libvirt
# By default we don't remove any networks (empty absent list)
# 'mtu' is optional
virt_infra_host_networks:
  absent: []
  present:
    - name: "default"
      ip_address: "192.168.112.1"
      subnet: "255.255.255.0"
      dhcp_start: "192.168.112.2"
      dhcp_end: "192.168.112.254"
      mtu: 9000

Which translates to something like this XML:

<network>
  <name>default</name>
  <uuid>f7ae42e8-5d02-4e74-9a83-fcb64b127e3e</uuid>
  <forward mode="nat">
    <nat>
      <port start="1024" end="65535"/>
    </nat>
  </forward>
  <bridge name="virbr5" stp="on" delay="0"/>
  <mtu size="9000"/>
  <mac address="52:54:00:6e:ba:f6"/>
  <ip address="192.168.112.1" netmask="255.255.255.0">
    <dhcp>
      <range start="192.168.112.2" end="192.168.112.254"/>
    </dhcp>
  </ip>
</network>

Thanks

I think one thing would be good is to make the bridge supported on all networking types, do you want to add that change in too? I posted an example of the diff above (https://github.com/csmart/ansible-role-virt-infra/pull/34#discussion_r550330056).

Also, I think it's better to not hardcode stp="on" delay="0" in the bridge_dev section, and leave it to libvirt to put in the system defaults. We could add options for stp and delay if you wanted to, but easier to just remove.

Cheers. -c

frgomes commented 3 years ago

I think the implementation is OK now. I've made the bridge block common to all network interfaces. I've tested with the configuration below:

    virt_infra_host_networks:
      #existing:
      #  - name: default
      #absent:
      #  - name: other
      present:
        - name: f901
          type: route
          host_dev: enp5s0
          bridge_dev: virbr10
          bridge_stp: on
          bridge_delay: 0
          mac: 52:54:00:f9:01:00
          domain: f901.example.com
          ip_address: 10.249.1.1
          ip_netmask: 255.255.255.0
          dhcp_start: 10.249.1.11
          dhcp_end: 10.249.1.254
          ip6_address: 2001:cafe:babe:300::1
          ip6_prefix: 64
          dhcp6_start: 2001:cafe:babe:300::f901:0000
          dhcp6_end: 2001:cafe:babe:300::f901:00ff
        - name: "nat_test"
          type: nat
          bridge_dev: virbr20
          ip_address: "192.168.112.1"
          subnet: "255.255.255.0"
          dhcp_start: "192.168.112.2"
          dhcp_end: "192.168.112.254"
          mtu: 9000
csmart commented 3 years ago

I think the implementation is OK now. I've made the bridge block common to all network interfaces. I've tested with the configuration below:

    virt_infra_host_networks:
      #existing:
      #  - name: default
      #absent:
      #  - name: other
      present:
        - name: f901
          type: route
          host_dev: enp5s0
          bridge_dev: virbr10
          bridge_stp: on
          bridge_delay: 0
          mac: 52:54:00:f9:01:00
          domain: f901.example.com
          ip_address: 10.249.1.1
          ip_netmask: 255.255.255.0
          dhcp_start: 10.249.1.11
          dhcp_end: 10.249.1.254
          ip6_address: 2001:cafe:babe:300::1
          ip6_prefix: 64
          dhcp6_start: 2001:cafe:babe:300::f901:0000
          dhcp6_end: 2001:cafe:babe:300::f901:00ff
        - name: "nat_test"
          type: nat
          bridge_dev: virbr20
          ip_address: "192.168.112.1"
          subnet: "255.255.255.0"
          dhcp_start: "192.168.112.2"
          dhcp_end: "192.168.112.254"
          mtu: 9000

Great, thanks for all your work! I think it's looking pretty good to me, I'll test and then accept the merge.

Cheers, -c