From d3a30a01e198c6ed9af75ce6de07133b9a1f23d0 Mon Sep 17 00:00:00 2001
From: David Moreau Simard <dmsimard@redhat.com>
Date: Mon, 27 Nov 2017 16:14:39 -0500
Subject: [PATCH] General improvements to configure-unbound role

- Improve README
- Improve integration test coverage
- Prefix variable names with "unbound_" to avoid clashes between
  different roles
- Unbound will very likely be installed but don't assume it is.
  Add an assertion to fail early if it isn't.

Change-Id: Ib8ca105938afdc0ccb1f524df3cae916c845efb8
---
 roles/configure-unbound/README.rst            | 18 +++++----
 roles/configure-unbound/defaults/main.yaml    |  8 ++--
 roles/configure-unbound/handlers/main.yaml    |  5 +++
 roles/configure-unbound/tasks/main.yaml       | 37 ++++++++++--------
 .../templates/forwarding.conf.j2              |  4 +-
 tests/configure-unbound.yaml                  | 39 ++++++++++++++++---
 6 files changed, 76 insertions(+), 35 deletions(-)
 create mode 100644 roles/configure-unbound/handlers/main.yaml

diff --git a/roles/configure-unbound/README.rst b/roles/configure-unbound/README.rst
index 039533e3..ed986d50 100644
--- a/roles/configure-unbound/README.rst
+++ b/roles/configure-unbound/README.rst
@@ -10,18 +10,22 @@ usable IPv6 default route, otherwise IPv4.
 
 **Role Variables**
 
-.. zuul:rolevar:: primary_nameserver_v4
+.. zuul:rolevar:: unbound_primary_nameserver_v4
+   :default: 208.67.222.222 (OpenDNS)
 
    The primary IPv4 nameserver for fowarding requests
 
-.. zuul:rolevar:: primary_nameserver_v6
-
-   The primary IPv6 nameserver for fowarding requests
-
-.. zuul:rolevar:: secondary_nameserver_v4
+.. zuul:rolevar:: unbound_secondary_nameserver_v4
+   :default: 8.8.8.8 (Google)
 
    The secondary IPv4 nameserver for fowarding requests
 
-.. zuul:rolevar:: secondary_nameserver_v6
+.. zuul:rolevar:: unbound_primary_nameserver_v6
+   :default: 2620:0:ccc::2 (OpenDNS)
+
+   The primary IPv6 nameserver for fowarding requests
+
+.. zuul:rolevar:: unbound_secondary_nameserver_v6
+   :default: 2001:4860:4860::8888 (Google)
 
    The seconary IPv6 nameserver for fowarding requests
diff --git a/roles/configure-unbound/defaults/main.yaml b/roles/configure-unbound/defaults/main.yaml
index d0228620..2574c60c 100644
--- a/roles/configure-unbound/defaults/main.yaml
+++ b/roles/configure-unbound/defaults/main.yaml
@@ -1,7 +1,7 @@
 # OpenDNS
-primary_nameserver_v6: "2620:0:ccc::2"
-primary_nameserver_v4: "208.67.222.222"
+unbound_primary_nameserver_v6: "2620:0:ccc::2"
+unbound_primary_nameserver_v4: "208.67.222.222"
 
 # Google
-secondary_nameserver_v6: "2001:4860:4860::8888"
-secondary_nameserver_v4: "8.8.8.8"
+unbound_secondary_nameserver_v6: "2001:4860:4860::8888"
+unbound_secondary_nameserver_v4: "8.8.8.8"
diff --git a/roles/configure-unbound/handlers/main.yaml b/roles/configure-unbound/handlers/main.yaml
new file mode 100644
index 00000000..7199e29b
--- /dev/null
+++ b/roles/configure-unbound/handlers/main.yaml
@@ -0,0 +1,5 @@
+- name: Restart unbound
+  become: yes
+  service:
+    name: unbound
+    state: restarted
diff --git a/roles/configure-unbound/tasks/main.yaml b/roles/configure-unbound/tasks/main.yaml
index 985983ee..65c60a5d 100644
--- a/roles/configure-unbound/tasks/main.yaml
+++ b/roles/configure-unbound/tasks/main.yaml
@@ -1,11 +1,13 @@
-- name: Ensure /etc/unbound exists
-  become: yes
-  file:
+# This role assumes that Unbound is already installed, fail early if it isn't.
+- name: Check that Unbound is installed
+  stat:
     path: /etc/unbound
-    state: directory
-    owner: root
-    group: root
-    mode: 0755
+  register: unbound_config
+
+- name: Ensure that Unbound is installed
+  assert:
+    that:
+      - unbound_config.stat.exists
 
 # ansible_default_ipv6 can either be undefined (no ipv6) or blank (no
 # routable address).  We only want to use ipv6 if it's available &
@@ -24,9 +26,8 @@
   when:
     - unbound_use_ipv6 is defined
   set_fact:
-    primary_nameserver: '{{ primary_nameserver_v6 }}'
-    secondary_nameserver: '{{ secondary_nameserver_v6 }}'
-
+    unbound_primary_nameserver: '{{ unbound_primary_nameserver_v6 }}'
+    unbound_secondary_nameserver: '{{ unbound_secondary_nameserver_v6 }}'
 
 # Fallback to default ipv4 if there is no ipv6 available as this
 # causes timeouts and failovers that are unnecesary.
@@ -34,21 +35,25 @@
   when:
     - unbound_use_ipv6 is not defined
   set_fact:
-    primary_nameserver: '{{ primary_nameserver_v4 }}'
-    secondary_nameserver: '{{ secondary_nameserver_v4 }}'
+    unbound_primary_nameserver: '{{ unbound_primary_nameserver_v4 }}'
+    unbound_secondary_nameserver: '{{ unbound_secondary_nameserver_v4 }}'
 
-- name: Configure unbound fowarding
+# TODO: Move this to /etc/unbound/conf.d ?
+- name: Configure unbound forwarding
   become: yes
   template:
-    dest: '/etc/unbound/forwarding.conf'
+    dest: /etc/unbound/forwarding.conf
     owner: root
     group: root
     mode: 0644
     src: forwarding.conf.j2
+  register: forwarding_config
+  notify:
+    - Restart unbound
 
-- name: restart unbound
+- name: Start unbound
   become: yes
   service:
     name: unbound
-    state: restarted
+    state: started
     enabled: yes
diff --git a/roles/configure-unbound/templates/forwarding.conf.j2 b/roles/configure-unbound/templates/forwarding.conf.j2
index 9b0a1717..3b52571e 100644
--- a/roles/configure-unbound/templates/forwarding.conf.j2
+++ b/roles/configure-unbound/templates/forwarding.conf.j2
@@ -2,5 +2,5 @@
 
 forward-zone:
   name: "."
-  forward-addr: {{ primary_nameserver }}
-  forward-addr: {{ secondary_nameserver }}
+  forward-addr: {{ unbound_primary_nameserver }}
+  forward-addr: {{ unbound_secondary_nameserver }}
diff --git a/tests/configure-unbound.yaml b/tests/configure-unbound.yaml
index 86fb5303..154c598b 100644
--- a/tests/configure-unbound.yaml
+++ b/tests/configure-unbound.yaml
@@ -3,11 +3,38 @@
   roles:
     - role: configure-unbound
   post_tasks:
-    - name: Check for /etc/unbound/forwarding.conf
-      stat: path=/etc/unbound/forwarding.conf
-      register: f
-    - name: Check forwarding file
+    - name: Check that unbound is started
+      become: yes
+      service:
+        name: unbound
+        state: started
+      register: unbound_service
+
+    - name: Ensure that unbound is started
       assert:
         that:
-          - f.stat.exists
-          - f.stat.isreg
+          - not unbound_service | changed
+
+    # Until nodepool no longer embeds a forwarding.conf in the image, it is
+    # safe to assume that we'll be changing the forwarding configuration
+    # because the role has logic to use v6 *or* v4 nameservers while nodepool
+    # puts all four nameservers.
+    - name: Ensure that configuration was installed
+      assert:
+        that:
+          - forwarding_config | changed
+
+    - name: Check if /etc/unbound/forwarding.conf exists
+      stat:
+        path: /etc/unbound/forwarding.conf
+      register: forwarding_file
+
+    - name: Ensure that configuration file exists
+      assert:
+        that:
+          - forwarding_file.stat.exists
+
+    # This is self-tested, no need to assert
+    - name: Do a host lookup (sanity check)
+      command: host openstack.org
+      changed_when: false