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

Martin Basti mbasti at redhat.com
Fri Apr 4 13:46:54 UTC 2014


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
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




More information about the Freeipa-devel mailing list