Refactor dhcp role (#117)

.github/workflows/test.yaml

@@ -0,0 +1,14 @@
+name: Test
+
+on:
+  push:
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    container: metalstack/metal-deployment-base:latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Test
+        run: |
+          make test

Makefile

@@ -0,0 +1,8 @@
+.PHONY: test
+test:
+	for file in $(shell find . -name test -type d); do python -m unittest discover -v -p '*_test.py' -s $$(dirname $$file); done
+
+.PHONY: test-local
+test-local:
+	docker pull metalstack/metal-deployment-base:latest
+	docker run --rm -it -v $(PWD):/work -w /work metalstack/metal-deployment-base:latest make test

partition/roles/dhcp/README.md

@@ -2,13 +2,17 @@
 
 Configures and starts dhcpd.
 
+This role can deploy on switches running Cumulus Linux or SONiC. It depends on the `switch_facts` module from `ansible-common`, so make sure modules from `ansible-common` are included before executing this role.
+
 ## Variables
 
-| Name             | Mandatory | Description                                                      |
-| ---------------- | --------- | ---------------------------------------------------------------- |
-| dhcp_net         | yes       | The dhcp network address  in which dhcp addresses will be leased |
-| dhcp_netmask     | yes       | The netmask of the dhcp network                                  |
-| dhcp_range_min   | yes       | The smallest address within the dhcp network to offer            |
-| dhcp_range_max   | yes       | The highest address within the dhcp network to offer             |
-| dhcp_server_ip   | yes       |                                                                  |
-| dhcp_dns_servers |           | Defines DNS servers for the machines that use this dhcp server   |
+| Name                     | Mandatory | Description                                                     |
+| ------------------------ | --------- | --------------------------------------------------------------- |
+| dhcp_subnets             |           | An array of subnets for which dhcp should lease addresses from  |
+| dhcp_subnets.comment     |           | The comment for this dhcp subnet                                |
+| dhcp_subnets.network     | yes       | The dhcp network address in which dhcp addresses will be leased |
+| dhcp_subnets.netmask     | yes       | The netmask of the dhcp network                                 |
+| dhcp_subnets.range.begin | yes       | The smallest address within the dhcp network to offer           |
+| dhcp_subnets.range.end   | yes       | The highest address within the dhcp network to offer            |
+| dhcp_subnets.options     |           | The options for a given subnet                                  |
+| dhcp_global_options      |           | The global options                                              |

partition/roles/dhcp/defaults/main.yaml

@@ -1,10 +1,24 @@
 ---
-dhcp_net:
-dhcp_netmask:
-dhcp_range_min:
-dhcp_range_max:
-dhcp_server_ip:
-
-dhcp_dns_servers:
-- 1.1.1.1
-- 8.8.8.8
+dhcp_subnets: []
+# examples:
+# - comment: ""
+#   network:
+#   netmask:
+#   range:
+#     begin:
+#     end:
+#   options: []
+
+dhcp_listening_interfaces:
+ - vlan4000
+
+dhcp_default_lease_time: 172800
+dhcp_max_lease_time: 345600
+
+dhcp_global_options: []
+# examples:
+# - default-url = "http://{{ ansible_host }}/onie-installer"
+# - ztp_provisioning_script_url code 239 = text
+# - ztp_provisioning_script_url "http://{{ ansible_host }}/ztp.sh"
+
+dhcp_service_name: "{{ 'dhcpd' if metal_stack_switch_os_is_cumulus | default(false) else 'isc-dhcp-server' }}"

partition/roles/dhcp/tasks/main.yaml

@@ -1,29 +1,49 @@
 ---
+- name: Gather switch facts
+  switch_facts:
+
 - name: Check mandatory variables for this role are set
   assert:
     fail_msg: "not all mandatory variables given, check role documentation"
     quiet: yes
     that:
-      - dhcp_net is defined
-      - dhcp_netmask is not none
-      - dhcp_server_ip is not none
-      - dhcp_range_min is not none
-      - dhcp_range_max is not none
+      - dhcp_listening_interfaces | type_debug == "list"
+      - item.network is defined
+      - item.netmask is not none
+      - item.range is not none
+      - item.range.begin is not none
+      - item.range.end is not none
+  loop: "{{ dhcp_subnets }}"
+  loop_control:
+    label: "{{ item.network }}"
+
+- name: install isc-dhcp-server
+  apt:
+    name:
+    - isc-dhcp-server
+    update_cache : yes
 
 - name: render dhcpd conf
   template:
     src: dhcpd.conf.j2
     dest: /etc/dhcp/dhcpd.conf
-  notify: dhcpd restart
+  register: _dhcpd_conf
 
-- name: render isc config for
+- name: render isc-dhcp-server config
   template:
     src: isc-dhcp-server.j2
     dest: /etc/default/isc-dhcp-server
-  notify: dhcpd restart
+  register: _isc_dhcp_server
+
+- name: restart isc-dhcp-server on config change
+  service:
+    name: "{{ dhcp_service_name }}"
+    enabled: true
+    state: restarted
+  when: _dhcpd_conf is changed or _isc_dhcp_server is changed
 
-- name: dhcpd enabled
+- name: ensure isc-dhcp-server is running
   service:
-    name: dhcpd
+    name: "{{ dhcp_service_name }}"
     enabled: true
     state: started

partition/roles/dhcp/templates/dhcpd.conf.j2

@@ -1,13 +1,25 @@
-default-lease-time 600;
-max-lease-time 600;
+# indicate that the DHCP server should send DHCPNAK messages to misconfigured client
+authoritative;
+
+default-lease-time {{ dhcp_default_lease_time }};
+max-lease-time {{ dhcp_max_lease_time }};
 
 log-facility local7;
 
-subnet {{ dhcp_net }} netmask {{ dhcp_netmask }} {
-    range {{ dhcp_range_min }} {{ dhcp_range_max }};
-    # Provide routers to set up default gateway for clients.
-    option routers {{ dhcp_server_ip }};
-    # In case of Vagrant DNS must not be resolved via mgmt servers.
-    # This is because the setup to provide e.g. metal images via mgmt servers exceeds resources in Vagrant case.
-    option domain-name-servers {{ dhcp_dns_servers | join(', ') }};
+{% for subnet in dhcp_subnets %}
+{% if subnet.comment | default("") %}
+# {{ subnet.comment }}
+{% endif %}
+subnet {{ subnet.network }} netmask {{ subnet.netmask }} {
+{% if subnet.range is defined %}
+  range {{ subnet.range.begin }} {{ subnet.range.end }};
+{% endif %}
+{% for option in subnet.options | default([]) %}
+  option {{ option }};
+{% endfor %}
 }
+{% endfor %}
+
+{% for option in dhcp_global_options %}
+option {{ option }};
+{% endfor %}

partition/roles/dhcp/templates/isc-dhcp-server.j2

@@ -18,4 +18,8 @@
 
 # On what interfaces should the DHCP server (dhcpd) serve DHCP requests?
 #       Separate multiple interfaces with spaces, e.g. "eth0 eth1".
-INTERFACES="vlan4000"
+{% if metal_stack_switch_os_is_cumulus | default(false) %}
+INTERFACES="{{ dhcp_listening_interfaces | join(' ') }}"
+{% else %}
+INTERFACESv4="{{ dhcp_listening_interfaces | join(' ') }}"
+{% endif %}

partition/roles/dhcp/test/template_test.py

@@ -7,34 +7,39 @@
 
 class DHCPD(unittest.TestCase):
     def test_dhcpd_config_template(self):
-        dhcpd_conf = read_template_file("dhcpd.conf.j2")
+        t = read_template_file("dhcpd.conf.j2")
 
         templar = Templar(loader=None, variables=dict(
-            dhcp_server_net="1.2.3.4",
-            dhcp_range_min="1",
-            dhcp_range_max="2",
-            dhcp_server_ip="2.2.2.2",
+            dhcp_subnets=[
+                dict(
+                    comment="testing",
+                    network="1.2.3.4",
+                    netmask="24",
+                    range=dict(begin=1, end=2),
+                    options=["routers 2.2.2.2", "domain-name-servers 1.1.1.1, 8.8.8.8"],
+                ),
+            ],
+            dhcp_default_lease_time=600,
+            dhcp_max_lease_time=600,
+            dhcp_global_options=[],
             groups=dict(mgmt_servers=["mgmt01", "mgmt02"]),
             hostvars=dict(mgmt01=dict(switch_mgmt_ip="3.3.3.3"), mgmt02=dict(switch_mgmt_ip="4.4.4.4")),
-            dhcp_public_dns_servers_fallback=["1.1.1.1", "8.8.8.8"]
         ))
 
-        res = templar.template(dhcpd_conf)
+        res = templar.template(t)
 
-        self.assertIn("option domain-name-servers 3.3.3.3, 4.4.4.4;", res)
+        self.assertIn("""
+authoritative;
 
-    def test_dhcpd_config_template_fallback(self):
-        dhcpd_conf = read_template_file("dhcpd.conf.j2")
+default-lease-time 600;
+max-lease-time 600;
 
-        templar = Templar(loader=None, variables=dict(
-            dhcp_server_net="1.2.3.4",
-            dhcp_range_min="1",
-            dhcp_range_max="2",
-            dhcp_server_ip="2.2.2.2",
-            groups=dict(),
-            dhcp_public_dns_servers_fallback=["1.1.1.1", "8.8.8.8"]
-        ))
-
-        res = templar.template(dhcpd_conf)
+log-facility local7;
 
-        self.assertIn("option domain-name-servers 1.1.1.1, 8.8.8.8;", res)
+# testing
+subnet 1.2.3.4 netmask 24 {
+  range 1 2;
+  option routers 2.2.2.2;
+  option domain-name-servers 1.1.1.1, 8.8.8.8;
+}
+""".strip(), res.strip())