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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to