Only manage DNS for the nsupdate provider

When using any other provider than nsupdate, it doesn't make sense to include a managed bind and can only cause problems. The nsupdate package manage is needed for nsupdate so moved to the installation part.
theforeman · Nov 27, 2019 · 54067fe · 54067fe
1 parent adc9f3d
commit 54067fe
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 84 deletions.
diff --git a/README.md b/README.md
@@ -46,7 +46,6 @@ To use the PowerDNS plugin, the following variables need to be set on the main
 ```puppet
 class{'::foreman_proxy':
   dns          => true,
-  dns_managed  => false,
   dns_provider => 'powerdns',
 }
 ```

diff --git a/manifests/config.pp b/manifests/config.pp
@@ -19,7 +19,7 @@
   # Somehow, calling these DHCP and DNS seems to conflict. So, they get a prefix...
   if $foreman_proxy::dhcp and $foreman_proxy::dhcp_managed { include ::foreman_proxy::proxydhcp }
 
-  if $foreman_proxy::dns and $foreman_proxy::dns_managed {
+  if $foreman_proxy::dns and $foreman_proxy::dns_provider in ['nsupdate', 'nsupdate_gss'] and $foreman_proxy::dns_managed {
     include ::foreman_proxy::proxydns
     $dns_groups = [$foreman_proxy::proxydns::user_group]
   } else {

diff --git a/manifests/init.pp b/manifests/init.pp
@@ -200,7 +200,7 @@
 #
 # $dns_listen_on::              DNS proxy to listen on https, http, or both
 #
-# $dns_managed::                The DNS daemon is managed by this module
+# $dns_managed::                The DNS daemon is managed by this module. Only supported for the nsupdate and nsupdate_gss DNS providers.
 #
 # $dns_provider::               DNS provider
 #

diff --git a/manifests/install.pp b/manifests/install.pp
@@ -31,4 +31,8 @@
   if $foreman_proxy::bmc and $foreman_proxy::bmc_default_provider != 'shell' {
     ensure_packages([$foreman_proxy::bmc_default_provider], { ensure => $foreman_proxy::ensure_packages_version, })
   }
+
+  if $foreman_proxy::dns and $foreman_proxy::dns_provider in ['nsupdate', 'nsupdate_gss'] {
+    ensure_packages([$foreman_proxy::nsupdate], { ensure => $foreman_proxy::ensure_packages_version })
+  }
 }
diff --git a/manifests/proxydns.pp b/manifests/proxydns.pp
@@ -1,9 +1,5 @@
 # Configure the DNS component
 #
-# $nsupdate:: The nsupdate package name
-#
-# $ensure_packages_version:: The ensure to use on the nsupdate package
-#
 # $forwarders:: The DNS forwarders to use
 #
 # $interface:: The interface to use for fact determination. By default the IP
@@ -18,8 +14,6 @@
 #        DNS entry.
 #
 class foreman_proxy::proxydns(
-  $nsupdate = $::foreman_proxy::nsupdate,
-  $ensure_packages_version = $::foreman_proxy::ensure_packages_version,
   $forwarders = $::foreman_proxy::dns_forwarders,
   $interface = $::foreman_proxy::dns_interface,
   $forward_zone = $::foreman_proxy::dns_zone,
@@ -32,8 +26,6 @@
 
   $user_group = $dns::group
 
-  ensure_packages([$nsupdate], { ensure => $ensure_packages_version, })
-
   # puppet fact names are converted from ethX.X and ethX:X to ethX_X
   # so for alias and vlan interfaces we have to modify the name accordingly
   $interface_fact_name = regsubst($interface, '[.:]', '_')

diff --git a/spec/classes/foreman_proxy__proxydns__spec.rb b/spec/classes/foreman_proxy__proxydns__spec.rb
@@ -15,23 +15,10 @@
           'include ::foreman_proxy'
         end
 
-        nsupdate_pkg = case facts[:osfamily]
-                       when 'RedHat'
-                         'bind-utils'
-                       when 'FreeBSD', 'DragonFly'
-                         'bind910'
-                       when 'Archlinux'
-                         'bind-tools'
-                       else
-                         'dnsutils'
-                       end
-
         it { should compile.with_all_deps }
 
         it 'should inherit the correct parameters' do
           should contain_class('foreman_proxy::proxydns')
-            .with_nsupdate(nsupdate_pkg)
-            .with_ensure_packages_version('present')
             .with_forwarders([])
             .with_interface('eth0')
             .with_forward_zone('example.com')
@@ -43,12 +30,10 @@
       context 'with explicit parameters' do
         let :base_params do
           {
-            :nsupdate                => 'nsupdate',
-            :ensure_packages_version => 'installed',
-            :forwarders              => [],
-            :interface               => 'eth0',
-            :forward_zone            => 'example.com',
-            :reverse_zone            => false,
+            forwarders: [],
+            interface: 'eth0',
+            forward_zone: 'example.com',
+            reverse_zone: false,
           }
         end
 
@@ -70,10 +55,6 @@
             should contain_class('dns').with_forwarders([])
           end
 
-          it 'should install nsupdate' do
-            should contain_package('nsupdate').with_ensure('present')
-          end
-
           it 'should include the forward zone' do
             should contain_dns__zone('example.com')
               .with_soa('foo.example.com')

diff --git a/spec/classes/foreman_proxy__spec.rb b/spec/classes/foreman_proxy__spec.rb
@@ -656,72 +656,102 @@
         end
       end
 
-      context 'with dns => true and dns_managed => true' do
+      context 'with dns => true' do
+        let(:params) { super().merge(dns: true) }
         let(:facts) { super().merge(ipaddress_eth0: '192.168.0.2', netmask_eth0: '255.255.255.0') }
-        let(:params) do
-          super().merge(
-            dns: true,
-            dns_managed: false,
-          )
+
+        let(:nsupdate_pkg) do
+          case facts[:osfamily]
+          when 'RedHat'
+            'bind-utils'
+          when 'FreeBSD', 'DragonFly'
+            'bind910'
+          when 'Archlinux'
+            'bind-tools'
+          else
+            'dnsutils'
+          end
         end
 
-        it { should compile.with_all_deps }
-        it { should_not contain_class('foreman_proxy::proxydns') }
-        it { should_not contain_class('dns') }
-      end
+        it { is_expected.to contain_class('foreman_proxy').with_dns_provider('nsupdate').with_dns_managed(true) }
 
-      context 'empty keyfile' do
-        let(:params) { super().merge(keyfile: '') }
+        context 'dns_provider => nsupdate' do
+          let(:params) { super().merge(dns_provider: 'nsupdate') }
 
-        it 'should not output dns_key' do
-          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_nsupdate.yml", [
-            '---',
-            ':dns_server: 127.0.0.1',
-          ])
+          it { is_expected.to compile.with_all_deps }
+          it { is_expected.to contain_package(nsupdate_pkg).with_ensure('present') }
+          it { is_expected.to contain_class('foreman_proxy::proxydns') }
+
+          context 'dns_managed => false' do
+            let(:params) { super().merge(dns_managed: false) }
+
+            it { is_expected.to compile.with_all_deps }
+            it { is_expected.to contain_package(nsupdate_pkg).with_ensure('present') }
+            it { is_expected.not_to contain_class('foreman_proxy::proxydns') }
+          end
         end
-      end
 
-      context 'when dns_provider => libvirt' do
-        let(:params) do
-          super().merge(
-            dns_provider: 'libvirt',
-            libvirt_network: 'mynet',
-            libvirt_connection: 'http://myvirt',
-          )
+        context 'when dns_provider => nsupdate_gss' do
+          let(:params) { super().merge(dns_provider: 'nsupdate_gss') }
+
+          it { is_expected.to compile.with_all_deps }
+          it { is_expected.to contain_package(nsupdate_pkg).with_ensure('present') }
+          it { is_expected.to contain_class('foreman_proxy::proxydns') }
+
+          it 'should contain dns_tsig_* settings' do
+            verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns.yml", [
+              '---',
+              ':enabled: https',
+              ':use_provider: dns_nsupdate_gss',
+              ':dns_ttl: 86400',
+            ])
+
+            verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_nsupdate_gss.yml", [
+              '---',
+              ':dns_server: 127.0.0.1',
+              ":dns_tsig_keytab: #{etc_dir}/foreman-proxy/dns.keytab",
+              ":dns_tsig_principal: foremanproxy/#{facts[:fqdn]}@EXAMPLE.COM",
+            ])
+          end
         end
 
-        it 'should set the provider correctly' do
-          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns.yml", [
-            '---',
-            ':enabled: false',
-            ':use_provider: dns_libvirt',
-            ':dns_ttl: 86400',
-          ])
+        context 'when dns_provider => libvirt' do
+          let(:params) do
+            super().merge(
+              dns_provider: 'libvirt',
+              libvirt_network: 'mynet',
+              libvirt_connection: 'http://myvirt',
+            )
+          end
 
-          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_libvirt.yml", [
-            '---',
-            ':network: mynet',
-            ':url: http://myvirt',
-          ])
+          it { is_expected.to compile.with_all_deps }
+          it { is_expected.not_to contain_package(nsupdate_pkg) }
+          it { is_expected.not_to contain_class('foreman_proxy::proxydns') }
+
+          it 'should generate the correct configs' do
+            verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns.yml", [
+              '---',
+              ':enabled: https',
+              ':use_provider: dns_libvirt',
+              ':dns_ttl: 86400',
+            ])
+
+            verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_libvirt.yml", [
+              '---',
+              ':network: mynet',
+              ':url: http://myvirt',
+            ])
+          end
         end
       end
 
-      context 'when dns_provider => nsupdate_gss' do
-        let(:params) { super().merge(dns_provider: 'nsupdate_gss') }
-
-        it 'should contain dns_tsig_* settings' do
-          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns.yml", [
-            '---',
-            ':enabled: false',
-            ':use_provider: dns_nsupdate_gss',
-            ':dns_ttl: 86400',
-          ])
+      context 'empty keyfile' do
+        let(:params) { super().merge(keyfile: '') }
 
-          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_nsupdate_gss.yml", [
+        it 'should not output dns_key' do
+          verify_exact_contents(catalogue, "#{etc_dir}/foreman-proxy/settings.d/dns_nsupdate.yml", [
             '---',
             ':dns_server: 127.0.0.1',
-            ":dns_tsig_keytab: #{etc_dir}/foreman-proxy/dns.keytab",
-            ":dns_tsig_principal: foremanproxy/#{facts[:fqdn]}@EXAMPLE.COM",
           ])
         end
       end