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

Fri Apr 4 13:51:28 UTC 2014

On Fri, 2014-04-04 at 15:46 +0200, Martin Basti wrote:
> On Fri, 2014-04-04 at 12:59 +0200, Petr Spacek wrote:
> > 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 .>
> 
> Cos we need to keep if user added domain with/without zone, we use
I mean end dot not zone.
> origin=None, and there is allowed to add only '.' as domain name => root
> In [3]: a = dns.name.from_unicode(u'.', origin=None)
> 
> In [4]: a.labels
> Out[4]: ('',)
> 
> That is more clearly to user add . as root domain, instead of empty
> string
> 
> > 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.
> I agree with origin_sing, or zone_record, or origin, not 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.
> I will take look at dns-python3. 
> 
> > > 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.
> Ok I will remove it.
> 
> > > 8)
> > >
> > > +    def is_ip_reverse(self):
> > Please use is_ip4_reverse() instead. That name will always remind developer 
> > not to forgot to IPv6 :-)
> As you wish.
> > > +        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.
> Okay, bytes then. I will reword it.
> > 
> > 
> > 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
> > 
> I'm not sure with that. Now all record data are returned as string, only
> domain names and domain related attributes in zone are returned as
> DNSName. It could be mess if half of records be returned as DNSName and
> second as string.
> > 
> > 
> > > 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.
> I like constants :-). I'm not sure if making new object, when in the most cases want only to compare, is good idea.
> 
> > > 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.
> I know, but in this IPA case is FQDN something like 'example.domain', not 'domain.' .
> I kept behavior of original function.
> 
> > 
> > 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...)
> I was confused too, maybe later, now I'm almost lost in all these
> changes.
> > 
> > 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 :-)
> I used in development phase, but it should be clear that these functions/methods dont work with unicode, some of them are used in other plugins (include 'private' functions)
> 
> > > 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 ...
> As I wrote above, it could be mayhem.
> 
> > >      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.
> I would use zone record instead. Is it okay?
> > 
> > 
> > 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.
> Thank you for make it clear for me. I though the private address is enough.
> 
> > 
> > I'm looking forward to see this is IPA! Good work!
> > 
> 
> Thank you for your review.


-- 
Martin^2 Basti