[Freeipa-devel] [PATCH 0029-0046] Internationalized domain names in DNS plugin
Jan Cholasta
jcholast at redhat.com
Thu Apr 3 13:35:23 UTC 2014
On 2.4.2014 14:07, Martin Basti wrote:
> Helo list,
>
> this patchset allows to use internationalized domian in DNS plugin.
> - dns names are stored in ACE form(punycoded) in LDAP
> - raw option shows dns data in ACE form, otherwise dns names are
> converted to unicode
> - plugin allow all characters in domain name, which are valid by IDN
> RFCs (almost everything including non-printable), should be validation
> more restrictive? (there is bug in dnspython with special characters,
> will be fixed soon)
> - TODO update WebUI to support DNSName objects
>
> Required patches:
> freeipa-jcholast-255-Allow-primary-keys-to-use-different-type-than-unicod.patch
> freeipa-jcholast-256-Support-API-version-specific-RPC-marshalling.patch
> freeipa-jcholast-257-Replace-get_syntax-method-of-IPASimpleObject-with-ne.patch
> freeipa-jcholast-258-Use-raw-attribute-values-in-command-result-when-raw-.patch
> freeipa-jcholast-259-Keep-original-name-when-setting-attribute-in-LDAPEnt.patch
>
>
> Patches attached.
>
First batch of comments, so far I have only read the code/patches,
without doing actual testing.
Patch 30:
1)
It might make sense to put all of this into a new module (e.g.
dnsutil.py) rather than ipautil.
2)
+ if isinstance(labels, str):
+ if not labels:
+ raise ValueError('empty string')
...
+ elif isinstance(labels, unicode):
+ if not labels:
+ raise ValueError('empty string')
It might be nicer to:
+ if isinstance(labels, basestring) and not labels:
+ raise ValueError('empty string')
+
+ if isinstance(labels, str):
...
+ elif isinstance(labels, unicode):
3)
+ def __nonzero__(self):
+ return True
It would be nice to include a comment about why DNSName always evaluates
to True (mention "@").
4)
+ @staticmethod
+ def get_root():
+ return DNSName(dns.name.root)
+
+ @staticmethod
+ def get_origin_sign():
+ return DNSName(u'@')
+
+ @staticmethod
+ def get_rev_zone():
+ return DNSName(u'in-addr.arpa.')
+
+ @staticmethod
+ def get_ip6_rev_zone():
+ return DNSName(u'ip6.arpa.')
I think you should either drop the "get_" prefix from the name, or (even
better) make these global constants.
I would shorten "origin_sign" to just "sign".
Can you please use tuples of str objects (i.e. what dns.name.Name uses
internally) instead of unicode objects for the initialization? I think
it should be the preferred style of initializing DNSName objects (DN
objects do the same).
5)
+ def __str__(self):
+ return super(DNSName, self).to_text()
You don't need to use super here.
6)
+ def ToASCII(self, omit_final_dot=False):
+ return super(DNSName,
self).to_text(omit_final_dot=omit_final_dot).decode('ascii')
+
+ def ToUnicode(self, omit_final_dot=False):
+ return super(DNSName,
self).to_unicode(omit_final_dot=omit_final_dot).decode('utf-8')
What was the reason for the unusual naming again? I would prefer PEP-8
compatible names (e.g. "to_ascii" and "to_unicode"), but if the current
names absolutely have to stay, please add a comment with explanation.
I don't like the "omit_final_dot" flag. IMHO it should be dropped and
whether the result includes a final dot or not should depend solely on
whether the name is absolute or relative. You can still use e.g.
"name.derelativize(root).ToUnicode()" to drop the final dot, which is
more explanatory.
In ToUnicode, the call to dns.name.Name.to_unicode already returns a
unicode object, no need to call decode on it.
7)
+ def concatenate(self, other):
+ return DNSName(super(DNSName, self).concatenate(other).labels)
+
+ def relativize(self, origin):
+ return DNSName(super(DNSName, self).relativize(origin).labels)
+
+ def derelativize(self, origin):
+ return DNSName(super(DNSName, self).derelativize(origin).labels)
+
+ def choose_relativity(self, origin=None, relativize=True):
+ return DNSName(super(DNSName,
self).choose_relativity(origin=origin, relativize=relativize).labels)
Why use ".labels" here? The DNSName constructor knows how to deal with
dns.name.Name objects, right?
8)
+ def is_ip_reverse(self):
+ if self.is_subdomain(self.get_rev_zone()):
+ return True
+ return False
+
+ def is_ip6_reverse(self):
+ if self.is_subdomain(self.get_ip6_rev_zone()):
+ return True
+ return False
+
+ def is_reverse(self):
+ if self.is_ip_reverse() or self.is_ip6_reverse():
+ return True
+ return False
The ifs are all redundant. Return the result of the check directly
("return self.is_subdomain ..." etc.)
Patch 31:
1)
+ kwargs = Param.kwargs + (
+ ('require_absolute', bool, False),
+ ('require_relative', bool, False),
+ )
What about renaming these to 'only_absolute' and 'only_relative'? IMO it
better captures the meaning (yes I know we already discussed the naming
in length :-)
2)
+ except (dns.name.LabelTooLong, UnicodeError):
+ #dnspython bug?, punycoded label longer than 63
ASCII-chars returns Unicode Error
+ #instead of LabelTooLong
If that's true, it should be handled in DNSName constructor, not here.
3)
All of the dns.name exceptions inherit from dns.exception.SyntaxError, I
think you should add an except for it as well.
4)
+ #compare if IDN normalized and original domain match
+ normalized_domain_name = encodings.idna.nameprep(value)
+ if(value != normalized_domain_name):
+ raise ValueError( _("domain name '%(domain)s' and
normalized domain name "
+ "'%(normalized)s' do not
match. Please use only "
+ "normalized domains") %
{'domain':value,
+ 'normalized':normalized_domain_name})
Can we instead try to normalize the name in the beginning of
_convert_scalar rather than raise an error later?
5)
+ except Exception as e:
+ raise ValidationError(name=self.name,
+ value=value,
+ index=index,
+ error=unicode(e))
Since this is _convert_scalar, I think this should be ConversionError
(even if the errors are technically validation errors).
Also, use self.get_param_name() instead of self.name.
6)
I'm not a fan of the layout of _convert_scalar, can you please use
something like this instead (includes changes requested in the previous
comments):
def _convert_scalar(self, value, index):
if isinstance(value, unicode):
value = encodings.idna.nameprep(value)
error = None
try:
value = DNSName(value)
except dns.name.BadEscape:
error = _("invalid escape code")
...
except dns.exception.SyntaxError:
error = _("invalid syntax")
if error:
raise ConversionError(
name=self.get_param_name(), index=index, error=error)
return super(DNSNameParam, self)._convert_scalar(value, index)
Patch 33:
Why is this patch necessary? It does not seem right to me.
Patch 34:
This patch should be squashed with patch 32. They don't make sense
without each other.
Patch 37:
1)
-def _rname_validator(ugettext, zonemgr):
What's up with this renaming of everything? Don't do it please, unless
absolutely necessary.
2)
You don't need to redefine has_output for dnszone_add, dnszone_mod,
dnszone_show, dnsrecord_add, dnsrecord_mod and dnsrecord_show. They
already use standard_entry.
3)
+ def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
+ assert isinstance(dn, DN)
+ _ns_zone_post_convert(entry_attrs, **options)
+ return dn
+
Please don't use stuff that is not defined in this or previous patches.
IPA should be at least buildable after each patch is applied.
4)
has_output = output.standard_value
+
msg_summary = _('Disabled DNS zone "%(value)s"')
No new whitespace please (seen 2 times in the patch).
Patch 38:
1)
+_dns_zone_record = DNSName(u'@')
You know you already defined a constant for this in patch 30, right?
2)
No new whitespace please (seen 2 times in the patch).
3)
+ #TODO This is used by realmdomains.py (but it shouldnt allow
classless), not used in DNS
+ #TODO This is used by Host.py
I don't think you should add these comments, since they are resolved in
patches 41 and 42.
4)
+def _hostname_validator_idn(ugettext, value):
+ assert isinstance(value, DNSName)
+
+ req_len = 2
+ if value.is_absolute():
+ req_len = 3
+ if len(value.labels) < req_len:
+ return _('invalid domain-name: not fully qualified')
+
+ return None
Why the "_idn" postfix? It does not seem to be related to IDN.
However, it does seem like it does the same thing as
_hostname_validator, so I think they should be merged.
5)
zonename = zone['idnsname'][0]
- if revdns.endswith(zonename) and len(zonename) > len(revzone):
- revzone = zonename
+ if revdns.is_subdomain(zonename.make_absolute()):
+ if revzone is None:
+ revzone = zonename
+ elif zonename.is_subdomain(revzone):
+ revzone = zonename
The idnsname returned from dnszone_find should already be absolute, so
why "zonename.make_absolute()"?
You can shorten the condition to:
if revdns.is_subdomain(zonename) and (revzone is None or
zonename.is_subdomain(revzone))
6)
- revzone = u'.'.join(items[pos:])
+ revzone = DNSName(u'.'.join(items[pos:]))
You join the items here into string which is split again in DNSName. Use
this instead:
revzone = DNSName(items[pos:])
7)
- return revzone, revname
+ return DNSName(revzone), DNSName(revname)
Aren't both revzone and revname DNSName objects already?
8)
- reason=_('DNS zone %(zone)s not found') % dict(zone=domain)
+ reason=_('DNS zone %(zone)s not found') %
dict(zone=domain.ToUnicode())
Is the ".ToUnicode()" necessary? (seen 4 times in the patch)
9)
IMO normalize_zonemgr_idn and validate_zonemgr_idn should be merged into
normalize_zonemgr and validate_zonemgr, respectively.
10)
def zone_is_reverse(zone_name):
- zone_name = normalize_zone(zone_name)
- if any(zone_name.endswith(name) for name in REVERSE_DNS_ZONES):
- return True
+ if(isinstance(zone_name, DNSName)):
+ if any(zone_name.is_subdomain(DNSName(name)) for name in
REVERSE_DNS_ZONES):
+ return True
+ else:
+ zone_name = normalize_zone(zone_name)
+ if any(zone_name.endswith(name) for name in REVERSE_DNS_ZONES):
+ return True
return False
Something like this would definitely be better:
def zone_is_reverse(zone_name):
if not isinstance(zone_name, DNSName):
zone_name = DNSName(zone_name)
return zone_name.is_reverse()
Patch 39:
1)
- Str('hostname',
- _hostname_validator,
- normalizer=_normalize_hostname,
+ DNSNameParam('hostname',
+ #RFC 2317 section 5.2 -- don't have to be FQDN
It seems you are changing behavior here. Such things belong in separate
patches.
Patch 40:
1)
+ addr = DNSName(u'')
This will always raise ValueError with the current implementation of
DNSName.
2)
No new whitespace please (seen 5 times in the patch).
Patch 43:
I think this patch should be squashed into patch 38.
Patch 44:
This patch should be squashed into patch 39.
1)
+<<<<<<< HEAD
output: PrimaryKey('value', None, None)
+=======
+output: Output('value', <class 'ipapython.ipautil.DNSName'>, None)
+>>>>>>> 5df7ed2... API change
It seems you overlooked this.
This is all for now. Expect more later ;-)
Honza
--
Jan Cholasta
More information about the Freeipa-devel
mailing list