Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 02.12.2015 17:28, Martin Basti wrote: On 01.12.2015 16:03, Stanislav Laznicka wrote: Sending the patch with renamed function. Standa On 12/01/2015 09:57 AM, Jan Cholasta wrote: On 1.12.2015 09:37, Petr Spacek wrote: On 30.11.2015 20:00, Martin Basti wrote: On 27.11.2015 16:06, Stanislav Laznicka wrote: Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin It works, but I would like to have Honza's opinion if ipalib/util.py is the right place for the new method and if we really need to use exceptions. ipalib/util.py is fine for the function, since it is not used anywhere in ipapython. I would rename the function to verify_host_resolvable() though, since the is_ prefix suggests the function returns a boolean value rather than raises an exception or not. IMO is_record_resolvable() should return only True/False and then it can be located in ipapython module Hmm, I do not see a reason why it should not throw an exception. IMHO the exception should contain the original error/result from resolver, so it is possible to print meaningful error message like "DNS query timed out" instead of "it does not work for some reason, trust us". Pushed to master: 498471e4aed1367b72cd74d15811d0584a6ee268 Of course I forgot to add ACK -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 01.12.2015 16:03, Stanislav Laznicka wrote: Sending the patch with renamed function. Standa On 12/01/2015 09:57 AM, Jan Cholasta wrote: On 1.12.2015 09:37, Petr Spacek wrote: On 30.11.2015 20:00, Martin Basti wrote: On 27.11.2015 16:06, Stanislav Laznicka wrote: Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin It works, but I would like to have Honza's opinion if ipalib/util.py is the right place for the new method and if we really need to use exceptions. ipalib/util.py is fine for the function, since it is not used anywhere in ipapython. I would rename the function to verify_host_resolvable() though, since the is_ prefix suggests the function returns a boolean value rather than raises an exception or not. IMO is_record_resolvable() should return only True/False and then it can be located in ipapython module Hmm, I do not see a reason why it should not throw an exception. IMHO the exception should contain the original error/result from resolver, so it is possible to print meaningful error message like "DNS query timed out" instead of "it does not work for some reason, trust us". Pushed to master: 498471e4aed1367b72cd74d15811d0584a6ee268 -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
Sending the patch with renamed function. Standa On 12/01/2015 09:57 AM, Jan Cholasta wrote: On 1.12.2015 09:37, Petr Spacek wrote: On 30.11.2015 20:00, Martin Basti wrote: On 27.11.2015 16:06, Stanislav Laznicka wrote: Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin It works, but I would like to have Honza's opinion if ipalib/util.py is the right place for the new method and if we really need to use exceptions. ipalib/util.py is fine for the function, since it is not used anywhere in ipapython. I would rename the function to verify_host_resolvable() though, since the is_ prefix suggests the function returns a boolean value rather than raises an exception or not. IMO is_record_resolvable() should return only True/False and then it can be located in ipapython module Hmm, I do not see a reason why it should not throw an exception. IMHO the exception should contain the original error/result from resolver, so it is possible to print meaningful error message like "DNS query timed out" instead of "it does not work for some reason, trust us". From d259e555bf096ff35397de80d273b97d829a45ea Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Wed, 25 Nov 2015 16:38:00 +0100 Subject: [PATCH] Removed duplicate domain name validating function --- ipa-client/ipa-install/ipa-client-install | 9 +--- ipalib/plugins/dns.py | 22 --- ipalib/plugins/host.py| 2 +- ipalib/plugins/service.py | 2 +- ipalib/util.py| 35 +++ ipapython/ipautil.py | 12 --- 6 files changed, 39 insertions(+), 43 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 05a550b11e74db84e46a126798c4db728226865c..974dd1da8bf3f5836170ca67d2f4c298e7ec6844 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -54,6 +54,7 @@ try: from ipapython.config import IPAOptionParser from ipalib import api, errors from ipalib import x509, certstore +from ipalib.util import verify_host_resolvable from ipalib.constants import CACERT from ipapython.dn import DN from ipapython.ssh import SSHPublicKey @@ -1761,11 +1762,13 @@ def get_server_connection_interface(server): def client_dns(server, hostname, options): -dns_ok = ipautil.is_host_resolvable(hostname) - -if not dns_ok: +try: +verify_host_resolvable(hostname, root_logger) +dns_ok = True +except errors.DNSNotARecordError: root_logger.warning("Hostname (%s) does not have A/ record.", hostname) +dns_ok = False if (options.dns_updates or options.all_ip_addresses or options.ip_addresses or not dns_ok): diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 830a70fa5aab7eef5f56f14050921eb2dc160449..67947360eb207de31ed114bb630705c409b2f9a9 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -51,9 +51,10 @@ from ipalib.util import (normalize_zonemgr, DNSSECSignatureMissingError, UnresolvableRecordError, EDNS0UnsupportedError, DNSSECValidationError, validate_dnssec_zone_forwarder_step1, - validate_dnssec_zone_forwarder_step2) + validate_dnssec_zone_forwarder_step2, + verify_host_resolvable) -from ipapython.ipautil import CheckedIPAddress, is_host_resolvable +from ipapython.ipautil import CheckedIPAddress from ipapython.dnsutil import DNSName if six.PY3: @@ -1554,7 +1555,7 @@ _dns_record_options = tuple(__dns_record_options_iter()) _dns_supported_record_types = tuple(record.rrtype for record in _dns_records \ if record.supported) -def check_ns_rec_resolvable(zone, name): +def check_ns_rec_resolvable(zone, name, log): assert isinstance(zone, DNSName) assert isinstance(name, DNSName) @@ -1563,7 +1564,9 @@ def check_ns_rec_resolvable(zone, name): elif not name.is_absolute(): # this is a DNS name relative to the zone name = name.derelativize(zone.make_absolute()) -if not is_host_resolvab
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 1.12.2015 09:37, Petr Spacek wrote: On 30.11.2015 20:00, Martin Basti wrote: On 27.11.2015 16:06, Stanislav Laznicka wrote: Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin It works, but I would like to have Honza's opinion if ipalib/util.py is the right place for the new method and if we really need to use exceptions. ipalib/util.py is fine for the function, since it is not used anywhere in ipapython. I would rename the function to verify_host_resolvable() though, since the is_ prefix suggests the function returns a boolean value rather than raises an exception or not. IMO is_record_resolvable() should return only True/False and then it can be located in ipapython module Hmm, I do not see a reason why it should not throw an exception. IMHO the exception should contain the original error/result from resolver, so it is possible to print meaningful error message like "DNS query timed out" instead of "it does not work for some reason, trust us". -- Jan Cholasta -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 30.11.2015 20:00, Martin Basti wrote: > > > On 27.11.2015 16:06, Stanislav Laznicka wrote: >> Please, see the modified patch attached. >> >> Standa >> >> On 11/27/2015 03:48 PM, Martin Basti wrote: >>> >>> >>> On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: > > On 25.11.2015 17:18, Stanislav Laznicka wrote: >> There were two functions for the same purpose. Removed one. >> >> > Hello, > > I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. >>> So at least I would like to move log param as the last param, in case of >>> need it can be modified to optional parameter. >>> >>> Or log can be default as root_logger, IMO we us only root_logger >>> everywhere, but this need investigation. >>> >>> Martin >> > It works, but I would like to have Honza's opinion if ipalib/util.py is the > right place for the new method and if we really need to use exceptions. > > IMO is_record_resolvable() should return only True/False and then it can be > located in ipapython module Hmm, I do not see a reason why it should not throw an exception. IMHO the exception should contain the original error/result from resolver, so it is possible to print meaningful error message like "DNS query timed out" instead of "it does not work for some reason, trust us". -- Petr^2 Spacek -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 27.11.2015 16:06, Stanislav Laznicka wrote: Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin It works, but I would like to have Honza's opinion if ipalib/util.py is the right place for the new method and if we really need to use exceptions. IMO is_record_resolvable() should return only True/False and then it can be located in ipapython module -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
Please, see the modified patch attached. Standa On 11/27/2015 03:48 PM, Martin Basti wrote: On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin From 93565a54d02ac05b686c18074d95e899610bcdc3 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Wed, 25 Nov 2015 16:38:00 +0100 Subject: [PATCH] Removed duplicate domain name validating function --- ipa-client/ipa-install/ipa-client-install | 9 +--- ipalib/plugins/dns.py | 22 --- ipalib/plugins/host.py| 2 +- ipalib/plugins/service.py | 2 +- ipalib/util.py| 35 +++ ipapython/ipautil.py | 12 --- 6 files changed, 39 insertions(+), 43 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 05a550b..f6d66c7 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -54,6 +54,7 @@ try: from ipapython.config import IPAOptionParser from ipalib import api, errors from ipalib import x509, certstore +from ipalib.util import is_host_resolvable from ipalib.constants import CACERT from ipapython.dn import DN from ipapython.ssh import SSHPublicKey @@ -1761,11 +1762,13 @@ def get_server_connection_interface(server): def client_dns(server, hostname, options): -dns_ok = ipautil.is_host_resolvable(hostname) - -if not dns_ok: +try: +is_host_resolvable(hostname, root_logger) +dns_ok = True +except errors.DNSNotARecordError: root_logger.warning("Hostname (%s) does not have A/ record.", hostname) +dns_ok = False if (options.dns_updates or options.all_ip_addresses or options.ip_addresses or not dns_ok): diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 830a70f..6a63c3c 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -51,9 +51,10 @@ from ipalib.util import (normalize_zonemgr, DNSSECSignatureMissingError, UnresolvableRecordError, EDNS0UnsupportedError, DNSSECValidationError, validate_dnssec_zone_forwarder_step1, - validate_dnssec_zone_forwarder_step2) + validate_dnssec_zone_forwarder_step2, + is_host_resolvable) -from ipapython.ipautil import CheckedIPAddress, is_host_resolvable +from ipapython.ipautil import CheckedIPAddress from ipapython.dnsutil import DNSName if six.PY3: @@ -1554,7 +1555,7 @@ _dns_record_options = tuple(__dns_record_options_iter()) _dns_supported_record_types = tuple(record.rrtype for record in _dns_records \ if record.supported) -def check_ns_rec_resolvable(zone, name): +def check_ns_rec_resolvable(zone, name, log): assert isinstance(zone, DNSName) assert isinstance(name, DNSName) @@ -1563,7 +1564,9 @@ def check_ns_rec_resolvable(zone, name): elif not name.is_absolute(): # this is a DNS name relative to the zone name = name.derelativize(zone.make_absolute()) -if not is_host_resolvable(name): +try: +is_host_resolvable(name, log) +except errors.DNSNotARecordError: raise errors.NotFound( reason=_('Nameserver \'%(host)s\' does not have a corresponding ' 'A/ record') % {'host': name} @@ -2734,7 +2737,8 @@ class dnszone_add(DNSZoneBase_add): # verify if user specified server is resolvable if not options['force']: -check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname']) +check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname'], +self.log) # show warning about --name-server option context.show_warning_nameserver_option = True else: @@ -2833,7 +2837,7 @@ class dnszone_mod(DNSZoneBase_mod): nameserver = entry_attrs['idnssoamname'] if nameserver: if not nameserver.is_empty() and not options['force']: -check_ns_rec_resolvable(keys[0], nameserver) +check_ns_rec_resolvable(keys[0], nameserver, self.log) context.show_warning_nameserver_option = True
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 27.11.2015 15:33, Petr Spacek wrote: On 27.11.2015 15:32, Martin Basti wrote: On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. So at least I would like to move log param as the last param, in case of need it can be modified to optional parameter. Or log can be default as root_logger, IMO we us only root_logger everywhere, but this need investigation. Martin -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 27.11.2015 15:32, Martin Basti wrote: > > > On 25.11.2015 17:18, Stanislav Laznicka wrote: >> There were two functions for the same purpose. Removed one. >> >> > Hello, > > I would like to have "log" param of is_host_resolvable as optional Is there an immediate need for the optional param? If not, I would not clutter the code. -- Petr^2 Spacek -- 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
Re: [Freeipa-devel] [PATCH] Removed duplicate domain name validation function
On 25.11.2015 17:18, Stanislav Laznicka wrote: There were two functions for the same purpose. Removed one. Hello, I would like to have "log" param of is_host_resolvable as optional Otherwise the patch LGTM. Martin -- 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
[Freeipa-devel] [PATCH] Removed duplicate domain name validation function
There were two functions for the same purpose. Removed one. From 15c192fdee0390ca8b6aa923691d66b1081ffae4 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Wed, 25 Nov 2015 16:38:00 +0100 Subject: [PATCH] Removed duplicate domain name validating function --- ipa-client/ipa-install/ipa-client-install | 9 +--- ipalib/plugins/dns.py | 22 --- ipalib/plugins/host.py| 2 +- ipalib/plugins/service.py | 2 +- ipalib/util.py| 35 +++ ipapython/ipautil.py | 12 --- 6 files changed, 39 insertions(+), 43 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 05a550b..837e8bd 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -54,6 +54,7 @@ try: from ipapython.config import IPAOptionParser from ipalib import api, errors from ipalib import x509, certstore +from ipalib.util import is_host_resolvable from ipalib.constants import CACERT from ipapython.dn import DN from ipapython.ssh import SSHPublicKey @@ -1761,11 +1762,13 @@ def get_server_connection_interface(server): def client_dns(server, hostname, options): -dns_ok = ipautil.is_host_resolvable(hostname) - -if not dns_ok: +try: +is_host_resolvable(root_logger, hostname) +dns_ok = True +except errors.DNSNotARecordError: root_logger.warning("Hostname (%s) does not have A/ record.", hostname) +dns_ok = False if (options.dns_updates or options.all_ip_addresses or options.ip_addresses or not dns_ok): diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 830a70f..44c6933 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -51,9 +51,10 @@ from ipalib.util import (normalize_zonemgr, DNSSECSignatureMissingError, UnresolvableRecordError, EDNS0UnsupportedError, DNSSECValidationError, validate_dnssec_zone_forwarder_step1, - validate_dnssec_zone_forwarder_step2) + validate_dnssec_zone_forwarder_step2, + is_host_resolvable) -from ipapython.ipautil import CheckedIPAddress, is_host_resolvable +from ipapython.ipautil import CheckedIPAddress from ipapython.dnsutil import DNSName if six.PY3: @@ -1554,7 +1555,7 @@ _dns_record_options = tuple(__dns_record_options_iter()) _dns_supported_record_types = tuple(record.rrtype for record in _dns_records \ if record.supported) -def check_ns_rec_resolvable(zone, name): +def check_ns_rec_resolvable(zone, name, log): assert isinstance(zone, DNSName) assert isinstance(name, DNSName) @@ -1563,7 +1564,9 @@ def check_ns_rec_resolvable(zone, name): elif not name.is_absolute(): # this is a DNS name relative to the zone name = name.derelativize(zone.make_absolute()) -if not is_host_resolvable(name): +try: +is_host_resolvable(log, name) +except errors.DNSNotARecordError: raise errors.NotFound( reason=_('Nameserver \'%(host)s\' does not have a corresponding ' 'A/ record') % {'host': name} @@ -2734,7 +2737,8 @@ class dnszone_add(DNSZoneBase_add): # verify if user specified server is resolvable if not options['force']: -check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname']) +check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname'], +self.log) # show warning about --name-server option context.show_warning_nameserver_option = True else: @@ -2833,7 +2837,7 @@ class dnszone_mod(DNSZoneBase_mod): nameserver = entry_attrs['idnssoamname'] if nameserver: if not nameserver.is_empty() and not options['force']: -check_ns_rec_resolvable(keys[0], nameserver) +check_ns_rec_resolvable(keys[0], nameserver, self.log) context.show_warning_nameserver_option = True else: # empty value, this option is required by ldap @@ -3004,7 +3008,7 @@ class dnsrecord(LDAPObject): if options.get('force', False) or nsrecords is None: return for nsrecord in nsrecords: -check_ns_rec_resolvable(keys[0], DNSName(nsrecord)) +check_ns_rec_resolvable(keys[0], DNSName(nsrecord), self.log) def _idnsname_pre_callback(self, ldap, dn, entry_attrs, *keys, **options): assert isinstance(dn, DN) @@ -4196,7 +4200,9 @@ class dns_resolve(Command): def execute(self, *args, **options): query=args[0] -if not