On 26.11.2015 09:01, Jan Cholasta wrote:
> On 11.11.2015 15:27, Petr Spacek wrote:
>> On 11.11.2015 09:36, Martin Babinsky wrote:
>>> On 11/11/2015 09:32 AM, Jan Cholasta wrote:
>>>> On 11.11.2015 09:27, Martin Babinsky wrote:
>>>>> On 11/11/2015 08:12 AM, Jan Cholasta wrote:
>>>>>> On 10.11.2015 16:58, Petr Spacek wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Patch 64:
>>>>>>> ipa-dns-install offer IP addresses from resolv.conf as default
>>>>>>> forwarders
>>>>>>>
>>>>>>> In non-interactive more option --auto-forwarders can be used to do the
>>>>>>> same. --forward option can be used to supply additional IP addresses.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/5438
>>>>>>
>>>>>> IMO it's perverse to add option which effectively means "use default
>>>>>> value" instead of actually using the value as default. This is
>>>>>> inconsistent with every other option and I don't see what makes
>>>>>> forwarders so special to require this.
>>>>>>
>>>>>> NACK unless you have a strong justification for this.
>>
>> Motivation:
>> /etc/resolv.conf holds nearest DNS servers. On the other hand, you want to
>> have backup forwarder which may not be local but could work even if local 
>> ones
>> fail.
>>
>> Option --default-forwarders reads list of "local" servers from resolv.conf 
>> and
>> --forwarder option allows you to add additional IP addresses to it.
>>
>> So your Ansible script can contain call like:
>> ipa-server-install --setup-dns --default-forwarder
>> --forwarder=<company-wide-fallback>
>> and you do not need to worry about mapping sites to nearest servers etc.
>>
>>>>> Is it possible to use default_getter decorator to fetch defaults for the
>>>>> 'forwarders' knob from the resolver if it is avaliable like so (warning:
>>>>> untested and possibly wrong)?
>>>>
>>>> Yes, this is exactly how it should be used (although the exception
>>>> handling could be better).
>>>>
>>> That was just a quick example off the top of my head without much thought
>>> going into it.
>>>
>>> Anyway, when running in interactive mode the installer can inform the user
>>> that he found these forwarders as defaults and prompt whether they shoud be
>>> used.
>>
>> After discussion in person we decided to not use default_getter decorator
>> because that would change current behavior in an unexpected way.
>>
>> Original option --auto-forwarders was renamed to --default-forwarders because
>> it sounds nicer :-D
> 
> Turns out I misunderstood the intent here and after another discussion in
> person we decided to go with the --auto-forwarders option.
> 
> ACK on the original patch.
> 
> Petr, could you please rebase patch 65 on top of current master?

Sure. I'm sorry for the delay!

-- 
Petr^2 Spacek
From 6e046e7ecd59777f52d9ae703e1a4e1245a3af3a Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 10 Nov 2015 11:22:43 +0100
Subject: [PATCH] ipa-dns-install offer IP addresses from resolv.conf as
 default forwarders

In non-interactive more option --auto-forwarders can be used to do the
same. --forward option can be used to supply additional IP addresses.

https://fedorahosted.org/freeipa/ticket/5438
---
 ipaserver/install/dns.py                   | 12 ++++++++++--
 ipaserver/install/installutils.py          |  7 +++++++
 ipaserver/install/server/common.py         | 14 ++++++++++++++
 ipaserver/install/server/install.py        |  7 ++++---
 ipaserver/install/server/replicainstall.py |  7 ++++---
 5 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py
index 615bd557b2e675c3e5fa362d338a35c6b697932d..6c8e952f4ba44d64875f207c15c6a0f4bfcb05ec 100644
--- a/ipaserver/install/dns.py
+++ b/ipaserver/install/dns.py
@@ -2,8 +2,11 @@
 # Copyright (C) 2015  FreeIPA Contributors see COPYING for license
 #
 
+from __future__ import absolute_import
 from __future__ import print_function
 
+# absolute import is necessary because IPA module dns clashes with python-dns
+from dns import resolver
 import sys
 
 from subprocess import CalledProcessError
@@ -230,8 +233,13 @@ def install_check(standalone, replica, options, hostname):
 
     if options.no_forwarders:
         dns_forwarders = ()
-    elif options.forwarders:
-        dns_forwarders = options.forwarders
+    elif options.forwarders or options.auto_forwarders:
+        if options.forwarders:
+            dns_forwarders = options.forwarders
+        else:
+            dns_forwarders = []
+        if options.auto_forwarders:
+            dns_forwarders += resolver.get_default_resolver().nameservers
     elif standalone or not replica:
         dns_forwarders = read_dns_forwarders()
 
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 489d03bdad0909ffe78736f720e4376206689a6a..156c8a5eb01a2e66f4dccc4b4d71605f09406dba 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -282,6 +282,13 @@ def read_ip_addresses():
 def read_dns_forwarders():
     addrs = []
     if ipautil.user_input("Do you want to configure DNS forwarders?", True):
+        print("Following DNS servers are configured in /etc/resolv.conf: %s" %
+                ", ".join(resolver.get_default_resolver().nameservers))
+        if ipautil.user_input("Do you want to configure these servers as DNS "
+                "forwarders?", True):
+            addrs = resolver.default_resolver.nameservers[:]
+            print("All DNS servers from /etc/resolv.conf were added. You can "
+                  "enter additional addresses now:")
         while True:
             ip = ipautil.user_input("Enter an IP address for a DNS forwarder, "
                                     "or press Enter to skip", allow_empty=True)
diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
index 93c95dd8e8d2b24af193ee19368959188bcd6cb9..82c2c9eac253f82baeffbebfa388718dcc30d14a 100644
--- a/ipaserver/install/server/common.py
+++ b/ipaserver/install/server/common.py
@@ -167,6 +167,11 @@ class BaseServerDNS(common.Installable, core.Group, core.Composite):
         cli_name='forwarder',
     )
 
+    auto_forwarders = Knob(
+        bool, False,
+        description="Use DNS forwarders configured in /etc/resolv.conf",
+    )
+
     no_forwarders = Knob(
         bool, False,
         description="Do not add any DNS forwarders, use root servers instead",
@@ -395,6 +400,10 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
                 raise RuntimeError(
                     "You cannot specify a --forwarder option without the "
                     "--setup-dns option")
+            if self.dns.auto_forwarders:
+                raise RuntimeError(
+                    "You cannot specify a --auto-forwarders option without "
+                    "the --setup-dns option")
             if self.dns.no_forwarders:
                 raise RuntimeError(
                     "You cannot specify a --no-forwarders option without the "
@@ -415,6 +424,10 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
             raise RuntimeError(
                 "You cannot specify a --forwarder option together with "
                 "--no-forwarders")
+        elif self.dns.auto_forwarders and self.dns.no_forwarders:
+            raise RuntimeError(
+                "You cannot specify a --auto-forwarders option together with "
+                "--no-forwarders")
         elif self.dns.reverse_zones and self.dns.no_reverse:
             raise RuntimeError(
                 "You cannot specify a --reverse-zone option together with "
@@ -441,6 +454,7 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
         self.skip_schema_check = self.ca.skip_schema_check
 
         self.forwarders = self.dns.forwarders
+        self.auto_forwarders = self.dns.auto_forwarders
         self.no_forwarders = self.dns.no_forwarders
         self.reverse_zones = self.dns.reverse_zones
         self.no_reverse = self.dns.no_reverse
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 6ecb87ac9b27c78579a08de79c9df8f5ed5f114d..4af70005d45adf3d3fe98e4b77620c1458b6fe07 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -1267,10 +1267,11 @@ class Server(BaseServer):
                     "and -a options")
             if self.setup_dns:
                 #pylint: disable=no-member
-                if not self.dns.forwarders and not self.dns.no_forwarders:
+                if (not self.dns.forwarders and not self.dns.no_forwarders
+                    and not self.dns.auto_forwarders):
                     raise RuntimeError(
-                        "You must specify at least one --forwarder option or "
-                        "--no-forwarders option")
+                        "You must specify at least one of --forwarder, "
+                        "--auto-forwarders, or --no-forwarders options")
 
         if self.idmax < self.idstart:
             raise RuntimeError(
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index e6d96bbe62c6960ebe94c529a8dac9dd0468d734..eac42dab2a3f94c4e9c4f0f2d0d1b84d4a1f0847 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -1199,10 +1199,11 @@ class Replica(BaseServer):
 
         if self.setup_dns:
             #pylint: disable=no-member
-            if not self.dns.forwarders and not self.dns.no_forwarders:
+            if (not self.dns.forwarders and not self.dns.no_forwarders
+                and not self.dns.auto_forwarders):
                 raise RuntimeError(
-                    "You must specify at least one --forwarder option or "
-                    "--no-forwarders option")
+                    "You must specify at least one of --forwarder, "
+                    "--auto-forwarders, or --no-forwarders options")
 
         self.password = self.dm_password
 
-- 
2.4.3

From 08eab73a9a210325fa13e7a57297b41b6ad765f9 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 10 Nov 2015 16:53:10 +0100
Subject: [PATCH] Remove global variable dns_forwarders from
 ipaserver.install.dns

---
 ipaserver/install/dns.py            | 27 ++++++++++++---------------
 ipaserver/install/server/install.py |  5 ++---
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py
index 6c8e952f4ba44d64875f207c15c6a0f4bfcb05ec..258bf5dbe46e2167e07a62127c7fd8fd4be23ee6 100644
--- a/ipaserver/install/dns.py
+++ b/ipaserver/install/dns.py
@@ -32,7 +32,6 @@ from ipaserver.install import odsexporterinstance
 from ipaserver.install import opendnssecinstance
 
 ip_addresses = []
-dns_forwarders = []
 reverse_zones = []
 
 
@@ -100,7 +99,6 @@ def _disable_dnssec():
 
 def install_check(standalone, replica, options, hostname):
     global ip_addresses
-    global dns_forwarders
     global reverse_zones
     fstore = sysrestore.FileStore(paths.SYSRESTORE)
 
@@ -232,25 +230,24 @@ def install_check(standalone, replica, options, hostname):
                                          True, options.ip_addresses)
 
     if options.no_forwarders:
-        dns_forwarders = ()
+        options.forwarders = []
     elif options.forwarders or options.auto_forwarders:
-        if options.forwarders:
-            dns_forwarders = options.forwarders
-        else:
-            dns_forwarders = []
+        if not options.forwarders:
+            options.forwarders = []
         if options.auto_forwarders:
-            dns_forwarders += resolver.get_default_resolver().nameservers
+            options.forwarders += resolver.get_default_resolver().nameservers
     elif standalone or not replica:
-        dns_forwarders = read_dns_forwarders()
+        options.forwarders = read_dns_forwarders()
 
     # test DNSSEC forwarders
-    if dns_forwarders:
-        if (not bindinstance.check_forwarders(dns_forwarders, root_logger) and
-                not options.no_dnssec_validation):
+    if options.forwarders:
+        if (not bindinstance.check_forwarders(options.forwarders,
+                                              root_logger)
+                and not options.no_dnssec_validation):
             options.no_dnssec_validation = True
             print("WARNING: DNSSEC validation will be disabled")
 
-    root_logger.debug("will use dns_forwarders: %s\n", dns_forwarders)
+    root_logger.debug("will use DNS forwarders: %s\n", options.forwarders)
 
     if not standalone:
         search_reverse_zones = False
@@ -273,7 +270,6 @@ def install_check(standalone, replica, options, hostname):
 
 def install(standalone, replica, options, api=api):
     global ip_addresses
-    global dns_forwarders
     global reverse_zones
 
     local_dnskeysyncd_dn = DN(('cn', 'DNSKeySync'), ('cn', api.env.host),
@@ -292,7 +288,8 @@ def install(standalone, replica, options, api=api):
     bind = bindinstance.BindInstance(fstore, ldapi=True, api=api,
                                      autobind=AUTOBIND_ENABLED)
     bind.setup(api.env.host, ip_addresses, api.env.realm, api.env.domain,
-               dns_forwarders, conf_ntp, reverse_zones, zonemgr=options.zonemgr,
+               options.forwarders, conf_ntp, reverse_zones,
+               zonemgr=options.zonemgr,
                no_dnssec_validation=options.no_dnssec_validation,
                ca_configured=options.setup_ca)
 
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 4af70005d45adf3d3fe98e4b77620c1458b6fe07..8b33cd4b878d951949047826227342e09fa1dede 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -628,8 +628,8 @@ def install_check(installer):
     if options.setup_dns:
         print("BIND DNS server will be configured to serve IPA domain with:")
         print("Forwarders:    %s" % (
-            "No forwarders" if not dns.dns_forwarders
-            else ", ".join([str(ip) for ip in dns.dns_forwarders])
+            "No forwarders" if not options.forwarders
+            else ", ".join([str(ip) for ip in options.forwarders])
         ))
         print("Reverse zone(s):  %s" % (
             "No reverse zone" if options.no_reverse or not dns.reverse_zones
@@ -765,7 +765,6 @@ def install(installer):
             options.dm_password = dm_password
             options.admin_password = admin_password
             options.host_name = host_name
-            options.forwarders = dns.dns_forwarders
             options.reverse_zones = dns.reverse_zones
             cache_vars = {n: getattr(options, n) for o, n in installer.knobs()}
             write_cache(cache_vars)
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to