[Freeipa-devel] [PATCH 0029-0046, 0047] Internationalized domain names in DNS plugin
Jan Cholasta
jcholast at redhat.com
Tue Apr 22 09:34:29 UTC 2014
On 11.4.2014 17:23, Martin Basti wrote:
> Updated patches attached.
>
> Patch 0047-1 should be applied between patches 0040-2 and 0041-2
>
> Patch 0043-1 was squashed into 0038-2
> Patch 0044-1 was squashed into 0039-2
> Patch 0034-1 was squashed into 0032-2
> Patch 0034-2 implements bug fix
>
> Patches should be applied in order: 0029-0040, 0047, 0041-0042,
> 0045-0046.
>
Patch 29:
Please keep line length under 80 characters:
- return x[self.obj.primary_key.name][0].lower()
+ return
self.obj.primary_key.sort_key(x[self.obj.primary_key.name][0])
Patch 30:
1)
You can shorten DNSName.ToASCII to:
def ToASCII(self):
return str(self).decode('ascii')
2)
Please keep line length under 80 characters:
+ def choose_relativity(self, origin=None, relativize=True):
+ return DNSName(super(DNSName,
self).choose_relativity(origin=origin, relativize=relativize))
3)
+ def is_idn(self):
+ return any(label.startswith(u'xn--') for label in self.labels)
Labels are str, so drop the u from 'xn--'.
4)
+
+
+#DNS public constants
+DNSName.root = DNSName(dns.name.root) # '.'
+DNSName.empty = DNSName(dns.name.empty) # '@'
+DNSName.ip4_rev_zone = DNSName(('in-addr','arpa',''))
+DNSName.ip6_rev_zone = DNSName(('ip6','arpa',''))
\ No newline at end of file
The extra newline at the beginning is not necessary, the constants are
part of DNSName definition.
Put spaces after commas in tuples.
Add the missing newline at the end of the file.
Patch 31:
1)
+ type = DNSName
Please also add type_error = _('must be DNS name')
2)
+ if(value != normalized_domain_name):
Why the weird parenthesised if?
3)
+ error = _("domain name '%(domain)s' and normalized
domain name "
+ "'%(normalized)s' do not
match. Please use only "
+ "normalized domains") %
{'domain':value,
+ 'normalized':normalized_domain_name}
Too much indentation, keep line length under 80 characters.
4)
+
+
+ def _rule_only_absolute(self, _, value):
+ if self.only_absolute and not value.is_absolute():
+ return _('must be absolute')
The extra new line at the beginning should not be there.
Patch 32:
1)
The patch does not compile:
Capability dns_name_values in ipalib, not in API
2)
The patch needs a rebase.
Patch 33:
Still don't know why is this patch necessary (it should not be).
Patch 34:
1)
The patch does not compile:
Arg in dnsrecord_add doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
Arg in dnsrecord_del doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
Arg in dnsrecord_delentry doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
Arg in dnsrecord_find doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
Arg in dnsrecord_mod doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
Arg in dnsrecord_show doesn't match.
Got Str('dnszoneidnsname', cli_name='dnszone', multivalue=False,
primary_key=True, query=True, required=True)
Expected Str('dnszoneidnsname', cli_name='dnszone', query=True,
required=True)
2)
I think the patch deserves a better commit message.
Patch 35:
No whitespace changes please:
from ipapython.version import API_VERSION
-
Patch 37:
The patch does not compile:
Output in dnszone_add_permission doesn't match. Got Output('value',
<type 'unicode'>, None) Expected PrimaryKey('value', None, None)
Output in dnszone_remove_permission doesn't match. Got Output('value',
<type 'unicode'>, None) Expected PrimaryKey('value', None, None)
Patch 38:
1)
The patch does not compile:
Traceback (most recent call last):
File "./makeapi", line 457, in <module>
sys.exit(main())
File "./makeapi", line 428, in main
api.finalize()
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugable.py", line
705, in finalize
self.__do_if_not_done('load_plugins')
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugable.py", line
481, in __do_if_not_done
getattr(self, name)()
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugable.py", line
642, in load_plugins
self.import_plugins('ipalib')
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugable.py", line
686, in import_plugins
__import__(fullname)
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugins/dns.py",
line 906, in <module>
class AFSDBRecord(DNSRecord):
File "/tmp/freeipa-build.pZlGiFr4U7/build/ipalib/plugins/dns.py",
line 916, in AFSDBRecord
_bind_hostname_validator,
NameError: name '_bind_hostname_validator' is not defined
2)
Please keep line length under 80 characters:
+from ipalib.util import (validate_zonemgr, normalize_zonemgr,
+ get_dns_forward_zone_update_policy,
+ get_dns_reverse_zone_update_policy,
get_reverse_zone_default,
+ REVERSE_DNS_ZONES)
- add_records_for_host_validation('ip_address', host, domain,
+ add_records_for_host_validation('ip_address',
DNSName(host), DNSName(domain),
- add_records_for_host(host, domain,
options['ip_address'],
+ add_records_for_host(DNSName(host),
DNSName(domain), options['ip_address'],
3)
No whitespace changes please:
+
def _create_zone_serial():
+
def normalize_zonemgr(zonemgr):
-
def validate_zonemgr(zonemgr):
4)
Why are relative names supported in _hostname_validator?
Also, you can shorten it a little bit:
def _hostname_validator(ugettext, value):
assert isinstance(value, DNSName)
if len(value.make_absolute().labels) < 3:
return _('invalid domain-name: not fully qualified')
5)
+ if revdns.is_subdomain(zonename.make_absolute()):
+ if revzone is None:
+ revzone = zonename
+ elif zonename.is_subdomain(revzone):
+ revzone = zonename
I would prefer this:
if (revdns.is_subdomain(zonename.make_absolute() and
(revzone is None or zonename.is_subdomain(revzone))):
revzone = zonename
6)
Normalizers should accept only strings and return only strings,
normalize_zonemgr should do that as well, there is no need for
normalize_zonemgr_str. Also some of the checks are redundant. I think it
should look something like this:
def normalize_zonemgr(zonemgr):
if not zonemgr or not isinstance(zonemgr, basestring):
return zonemgr
name, at, domain = zonemgr.partition('@')
name = name.replace('.', '\\.')
return '%s.%s' % (name, domain)
7)
I don't think the DNSName branch in normalize_zone is necessary, you can
call make_absolute on DNSName objects directly. The function should be
for strings only. You can also make it shorter:
def normalize_zone(zone):
return str(DNSName(zone).make_absolute())
8)
validate_zonemgr does too many things at once.
In the dns plugin, after idnssoarname is normalized and converted to
DNSName, you just need to validate that it does not contain any "@" and
has the right number of labels:
def validate_zonemgr(zonemgr):
assert isinstance(zonemgr, DNSName)
assert zonemgr.is_absolute()
if any('@' in label for label in zonemgr.labels):
raise ValueError(_('too many \'@\' characters'))
if len(zonemgr.labels) < 3:
raise ValueError(_('missing address domain'))
if not zonemgr.labels[0]:
raise ValueError(_('missing mail account'))
In bindinstance, you need to normalize and convert the value manually
and then you can do the same check as in the dns plugins:
def validate_zonemgr_str(zonemgr):
zonemgr = normalize_zonemgr(zonemgr)
zonemgr = DNSName(zonemgr).make_absolute()
return validate_zonemgr(zonemgr)
9)
Why strings and not DNSName objects?
REVERSE_DNS_ZONES = {
- '.in-addr.arpa.' : 4,
- '.ip6.arpa.' : 32,
+ u'in-addr.arpa.' : 4,
+ u'ip6.arpa.' : 32,
}
10)
You can shorten zone_is_reverse to:
def zone_is_reverse(zone_name):
return DNSName(zone_name).is_reverse()
Patch 39:
1)
The patch does not compile:
Traceback (most recent call last):
File "./makeapi", line 457, in <module>
sys.exit(main())
File "./makeapi", line 428, in main
api.finalize()
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugable.py", line
705, in finalize
self.__do_if_not_done('load_plugins')
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugable.py", line
481, in __do_if_not_done
getattr(self, name)()
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugable.py", line
642, in load_plugins
self.import_plugins('ipalib')
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugable.py", line
686, in import_plugins
__import__(fullname)
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugins/dns.py",
line 1298, in <module>
class PTRRecord(DNSRecord):
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/plugins/dns.py",
line 1306, in PTRRecord
doc=_('The hostname this reverse record points to'),
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/parameters.py", line
1892, in __init__
super(DNSNameParam, self).__init__(name, *rules, **kw)
File "/tmp/freeipa-build.w7CjNx7hkb/build/ipalib/parameters.py", line
444, in __init__
', '.join(repr(k) for k in sorted(extra))
TypeError: DNSNameParam('hostname'): takes no such kwargs:
'require_absolute'
Traceback (most recent call last):
File "./makeapi", line 457, in <module>
sys.exit(main())
File "./makeapi", line 428, in main
api.finalize()
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugable.py", line
705, in finalize
self.__do_if_not_done('load_plugins')
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugable.py", line
481, in __do_if_not_done
getattr(self, name)()
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugable.py", line
642, in load_plugins
self.import_plugins('ipalib')
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugable.py", line
686, in import_plugins
__import__(fullname)
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugins/dns.py",
line 1545, in <module>
class dnszone(LDAPObject):
File "/tmp/freeipa-build.8geSL2lbho/build/ipalib/plugins/dns.py",
line 1588, in dnszone
normalizer=normalize_zonemgr_idn,
NameError: name 'normalize_zonemgr_idn' is not defined
... and a lot of Str/DNSNameParam mismatches in API.txt
2)
You set only_absolute on some DNSNameParams which previously had
_bind_hostname_validator and on some you don't. I would think it should
be done consistently.
3)
The idnssoarname param of dnszone should have only_absolute=True (in the
original code the normalizer added the trailing dot).
The default_from should be "lambda idnsname:
DNSName(('hostmaster')).derelativize(idnsname)".
4)
Str('hostname',
label=_('Hostname'),
+ #No need IDN validation and normalization
),
The comment seems a little but redundant to me.
Patch 40:
1)
make-lint reports these errors:
************* Module ipalib.plugins.dns
ipalib/plugins/dns.py:408: [E0602(undefined-variable),
_domain_name_validator] Undefined variable 'normalize_zone')
ipalib/plugins/dns.py:409: [E0602(undefined-variable),
_domain_name_validator] Undefined variable 'validate_domain_name')
ipalib/plugins/dns.py:409: [E0602(undefined-variable),
_domain_name_validator] Undefined variable 'zone_is_reverse')
2)
+capability: dns_name_values 2.83
This is already in patch 32.
3)
There already is dnsrecord.postprocess_record, IDN postprocessing should
be done in there instead of _records_idn_postprocess.
4)
- default_from=lambda idnsname: 'hostmaster.%s' %
unicode(idnsname),
- normalizer=normalize_zonemgr_idn,
+ default_from=lambda idnsname: u'hostmaster.%s' %
unicode(idnsname),
+ normalizer=normalize_zonemgr,
This belongs in patch 39.
5)
dnszone.get_dn is unnecessarily complex, keys[-1] will always be an
absolute DNSName, so you can drop the code path for relative names:
def get_dn(self, *keys, **options):
zone = keys[-1]
assert isinstance(zone, DNSName)
assert zone.is_absolute()
zone = zone.ToASCII()
dn = super(dnszone, self).get_dn(zone, **options)
try:
self.backend.get_entry(dn, [''])
except errors.NotFound:
zone = zone[:-1]
dn = super(dnszone, self).get_dn(zone, **options)
return dn
6)
You can shorten dnszone.get_name_in_zone code to:
def get_name_in_zone(self, zone, hostname):
assert isinstance(zone, DNSName)
assert zone.is_absolute()
assert isinstance(hostname, DNSName)
if not hostname.is_absolute():
return hostname
if hostname.is_subdomain(zone):
return hostname.relativize(zone)
7)
Conversions from string to DNSName in interactive_prompt_callbacks may
fail, you should put them in try/except blocks:
+ zone = DNSName(kw['idnsname']).make_absolute()
+ ns = DNSName(kw['idnssoamname'])
8)
idnssoamname from LDAP is converted to DNSName in patch 36, no need to
do it again here:
+ nameserver = DNSName(entry_attrs['idnssoamname'][0])
+ if nameserver.is_absolute():
9)
This should be handled in the realmdomains plugin, by raising a
ValidationError:
+ and not zone.is_idn()): #Realmdomain doesnt support IDN
10)
This should be indented better:
+ if (zone != DNSName(api.env.domain).make_absolute() and
+ not zone.is_idn() and #Realmdomain doesnt support IDN
+ not zone.is_reverse()):
11)
Searching should work for any input, but you make a lot of assumptions
in the IDN search code which may not be true. I think you should not use
DNSName in this code, as it does too much validation. Use something like
this instead:
term = args[-1]
if term:
term = re.split(r'(?<!\\)\.', term)
term = u'.'.join(convert_to_punycode(x) for x in term)
if term != args[-1]:
filter = modify_filter_idn(filter, term)
Also make sure you convert other params to punycode as well, so searches
like this work:
$ ipa dnszone-find --name=<IDN name>
This applies to both dnszone_find and dnsrecord_find.
12)
Make sure to store only lists in LDAP entries:
+ entry_attrs['idnsname'] = keys[-1].relativize(keys[-2])
13)
You don't have to move the "zone = keys[-2]" line here, just delete the
normalize_zone line:
- zone = keys[-2]
if self.is_pkey_zone_record(*keys):
- addr = u''
+ addr = _dns_zone_record
else:
addr = keys[-1]
- zone = normalize_zone(zone)
+ zone = keys[-2]
14)
You can shorten this:
+ addr_len = len(addr.labels) if addr != _dns_zone_record else 0
to:
addr_len = len(addr.labels)
15)
Instead of comparing values to _dns_zone_record:
+ if idnsname == _dns_zone_record or idnsname == zonename:
+ if DNSName(kw['idnsname']) == _dns_zone_record:
I think it would be better to add .is_empty() to DNSName and use it.
16)
dnsrecord.is_pkey_zone_record seems related to dnszone.get_name_in_zone,
maybe you can move the common parts to a single place and use it like this:
def is_pkey_zone_record(self, *keys):
name = get_name_in_zone(keys[-2], keys[-1])
if name is None:
return False
return name.is_empty()
17)
Split this into multiple lines please:
+ keys = keys[:-1] + (keys[-1].relativize(keys[-2]).ToASCII(),)
#Make RR name relative if possible
18)
Please keep line length under 80 characters.
Patch 47:
The patch does not compile:
Traceback (most recent call last):
File "./makeapi", line 457, in <module>
sys.exit(main())
File "./makeapi", line 428, in main
api.finalize()
File "/tmp/freeipa-build.m5sbYb5j0d/build/ipalib/plugable.py", line
705, in finalize
self.__do_if_not_done('load_plugins')
File "/tmp/freeipa-build.m5sbYb5j0d/build/ipalib/plugable.py", line
481, in __do_if_not_done
getattr(self, name)()
File "/tmp/freeipa-build.m5sbYb5j0d/build/ipalib/plugable.py", line
642, in load_plugins
self.import_plugins('ipalib')
File "/tmp/freeipa-build.m5sbYb5j0d/build/ipalib/plugable.py", line
686, in import_plugins
__import__(fullname)
File "/tmp/freeipa-build.m5sbYb5j0d/build/ipalib/plugins/host.py",
line 35, in <module>
from ipalib.plugins.dns import (dns_container_exists, _record_types,
ImportError: cannot import name _hostname_validator
Option in dnsrecord_add doesn't match. Got
DNSNameParam('ptr_part_hostname', attribute=False,
cli_name='ptr_hostname', multivalue=False, option_group=u'PTR Record',
required=False) Expected DNSNameParam('ptr_part_hostname',
attribute=False, cli_name='ptr_hostname', multivalue=False,
only_absolute=True, option_group=u'PTR Record', required=False)
Option in dnsrecord_mod doesn't match. Got
DNSNameParam('ptr_part_hostname', attribute=False, autofill=False,
cli_name='ptr_hostname', multivalue=False, option_group=u'PTR Record',
required=False) Expected DNSNameParam('ptr_part_hostname',
attribute=False, autofill=False, cli_name='ptr_hostname',
multivalue=False, only_absolute=True, option_group=u'PTR Record',
required=False)
I had some test failures, don't know which patch is to blame:
======================================================================
ERROR: test_dns[88]: dnszone_add: Create reverse zone
u'70.16.172.in-addr.arpa.'
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
runTest
self.test(*self.arg)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 301, in <lambda>
func = lambda: self.check(nice, **test)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 319, in check
self.check_output(nice, cmd, args, options, expected, extra_check)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 358, in check_output
got = api.Command[cmd](*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 436,
in __call__
ret = self.run(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 758,
in run
return self.forward(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 779,
in forward
return self.Backend.rpcclient.forward(self.name, *args, **kw)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 854, in
forward
return self._call_command(command, params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 831, in
_call_command
return command(*params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 971, in _call
return self.__request(name, args)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 965, in
__request
raise error_class(message=error['message'])
NotFound: Nameserver 'ns1.zone3.test.' does not have a corresponding
A/AAAA record
======================================================================
ERROR: test_dns[90]: dnsrecord_add: Add NS record to u'128/25' in
revzone u'70.16.172.in-addr.arpa.'
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
runTest
self.test(*self.arg)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 301, in <lambda>
func = lambda: self.check(nice, **test)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 319, in check
self.check_output(nice, cmd, args, options, expected, extra_check)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 358, in check_output
got = api.Command[cmd](*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 436,
in __call__
ret = self.run(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 758,
in run
return self.forward(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 779,
in forward
return self.Backend.rpcclient.forward(self.name, *args, **kw)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 854, in
forward
return self._call_command(command, params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 831, in
_call_command
return command(*params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 971, in _call
return self.__request(name, args)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 965, in
__request
raise error_class(message=error['message'])
NotFound: 70.16.172.in-addr.arpa.: DNS zone not found
======================================================================
ERROR: test_dns[91]: dnsrecord_add: Add CNAME record to u'129' in
revzone u'70.16.172.in-addr.arpa.'
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
runTest
self.test(*self.arg)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 301, in <lambda>
func = lambda: self.check(nice, **test)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 319, in check
self.check_output(nice, cmd, args, options, expected, extra_check)
File
"/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
line 358, in check_output
got = api.Command[cmd](*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 436,
in __call__
ret = self.run(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 758,
in run
return self.forward(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 779,
in forward
return self.Backend.rpcclient.forward(self.name, *args, **kw)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 854, in
forward
return self._call_command(command, params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 831, in
_call_command
return command(*params)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 971, in _call
return self.__request(name, args)
File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 965, in
__request
raise error_class(message=error['message'])
NotFound: 70.16.172.in-addr.arpa.: DNS zone not found
Honza
--
Jan Cholasta
More information about the Freeipa-devel
mailing list