[Freeipa-devel] [PATCH 0029-0046] Internationalized domain names in DNS plugin

[Petr Spacek]

On 3.4.2014 15:35, Jan Cholasta wrote:
> On 2.4.2014 14:07, Martin Basti wrote:
> Patch 30:
> 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):
Is it expected that you can't create root name?

I would like to imitate dns-python semantics as much as possible.

In [2]: dns.name.from_text("")
Out[2]: <DNS name .>

In [4]: dns.name.Name([])
Out[4]: <DNS name @>

In [5]: dns.name.Name([""])
Out[5]: <DNS name .>

I would like to see more descriptive error texts if you insist on current 
semantics.

> 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".
Sign of what? Decay? :-) I don't think that sign is descriptive enough, I 
would personally stick with origin_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).
Please don't forget to consult dns-python3 and try to make migration from 
dns-python to dns-python3 easy.

> 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.
Generally, I agree. We can get rid of the relative/absolute name madness by 
using final dot everywhere.

> 8)
>
> +    def is_ip_reverse(self):
Please use is_ip4_reverse() instead. That name will always remind developer 
not to forgot to IPv6 :-)

> +        if self.is_subdomain(self.get_rev_zone()):
> +            return True
> +        return False



> Patch 31:
2b)
+                except dns.name.NameTooLong:
+                    raise ValueError(_('domain name cannot be longer than 255 
characters'))

Personally, I would say '255 bytes' instead of '255 characters'. Characters 
are tricky when IDN is involved etc.



Patch freeipa-mbasti-0036-DNSName-conversion-in-ipaldap.patch:

@@ -255,6 +256,10 @@ class IPASimpleLDAPObject(object):
          'managedtemplate': DN,
          'managedbase':     DN,
          'originscope':     DN,
+        'idnsname':        DNSName,
+        'idnssoamname':    DNSName,
+        'idnssoarname':    DNSName,
+        'dnszoneidnsname': DNSName,
      })

Maybe you can add following attributes too:
CNAMERecord
DNAMERecord
MDRecord
NSRecord
PTRRecord



> Patch 38:
>
> 1)
>
> +_dns_zone_record = DNSName(u'@')
>
> You know you already defined a constant for this in patch 30, right?
It seems that both constants are unnecessary because IMHO
DNSName(u'@')
is more readable and you don't need to dig into code to find out what the 
cryptic name means.

> 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')
This test is not correct because FQDN = an absolute name.

This code tries to guess if the name is FQDN or not but we can directly use 
is_absolute() test.


4b)
  def is_forward_record(zone, str_address):
      addr = netaddr.IPAddress(str_address)
@@ -461,6 +444,7 @@ def is_forward_record(zone, str_address):

  def add_forward_record(zone, name, str_address):
      addr = netaddr.IPAddress(str_address)
+
      try:
          if addr.version == 4:
              api.Command['dnsrecord_add'](zone, name, arecord=str_address)

Personally, I think that 'forward' is confusing (in this context). Could you 
rename *_forward_record() functions to *_ipaddr_record(), please? (Maybe in a 
separate patch...)


4c)
  def add_records_for_host(host, domain, ip_addresses, add_forward=True, 
add_reverse=True):
+    assert isinstance(host, DNSName)
+    assert isinstance(domain, DNSName)

The same applies to all places with:
+    assert isinstance(value, DNSName)
scattered all over the patch.

Is this really necessary? I thought that Python usually uses
http://en.wikipedia.org/wiki/Duck_typing

It seems like leftovers from development phase. (Maybe it is not the case for 
IPA framework, correct me if I'm wrong :-)


> 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.
Note: This is (mostly) related to classless reverse zones.



freeipa-mbasti-0040-Modified-record-and-zone-class-to-support-IDN.patch
> * Records data are always returned as string
> * Attributes idnsname, idnssoamname, idnssoarname are returned as DNSName, with
>   option --raw as string
> * option --raw returns all IDN domains punycoded

I have mentioned
CNAMERecord
DNAMERecord
MDRecord
NSRecord
PTRRecord

above. Their are also pure DNS names ...
>      def get_name_in_zone(self, zone, hostname):
[snip]
> -        be added to a zone
> +        be added to a zone. Returns '@' when the hostname is directly in the zone

I would replace "in the zone" with "at zone apex". Technically, all records 
with suffix = zone are "in" the zone.



freeipa-mbasti-0046-DNS-new-tests.patch
+idnzone1_ip = u'192.168.11.1'
[snip]
+revidnzone1 = u'15.168.192.in-addr.arpa.'

Please use addresses from 172.16/12 prefix so IPA tests don't consume more 
than one prefix.

More prefixes we use => higher probability that it will clash with somebody's 
environment.


I'm looking forward to see this is IPA! Good work!

-- 
Petr^2 Spacek