Subject: Adam Roach's No Objection on draft-ietf-rtgwg-routing-types-16: (with COMMENT)

From: Adam Roach <[email-masked]>
Date: Wed, 11 Oct 2017 17:44:00 -0700

Adam Roach has entered the following ballot position for
draft-ietf-rtgwg-routing-types-16: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-rtgwg-routing-types/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Section 2:

Are these types in any particular order? If not, you might consider
alphabetizing them to make thing easier to find.

   uint24
      This type defines a 24-bit unsigned integer.  It is used by
      target="I-D.ietf-ospf-yang"/>.

There appears to be some XML damage here.

____

There are several patterns in the YANG definition that perform significant
restriction of numbers (e.g., to ensure they don't fall outside the range that
can be stored in 16 or 32 bits). In many cases, these patterns include the
ability to zero-prefix some (but not all) decimal values. For example, the
production for route-origin would allow leading zeros in "2:0100:0555" but not
in "2:04294967295:065535" (even though "2:4294967295:65535" is okay). I don't
know offhand whether it makes sense to allow leading zeros in these fields, but
I would argue that the production should be consistent in allowing or
disallowing them. This issue arises in various forms in route-target,
ipv6-route-target, route-origin, and ipv6-route-origin.

The definition of bandwidth-ieee-float32 includes the following text:

          The encoding format is the external hexadecimal-significant
          character sequences specified in IEEE 754 and C99. The
          format is restricted to be normalized, non-negative, and
          non-fraction: 0x1.hhhhhhp{+}d or 0X1.HHHHHHP{+}D
          where 'h' and 'H' are hexadecimal digits, 'd' and 'D' are
          integers in the range of [0..127].

Notably, this prose clearly says that values can start with "0x1" and "0X1",
but not "0x0" or "0X0" -- while the pattern production does allow 0x0, and the
examples even include values starting with 0x0. The quoted prose above should
be re-worked so it also allows values starting with 0x0 and 0X0.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 12 Oct 2017 01:16:02 +0000

Hi Adam, 

>https://datatracker.ietf.org/doc/draft-ietf-rtgwg-routing-types/

The YANG types are clustered together based on functional affinity in much
the same way as one would group type definitions in a programming language
header file. 
>      target="I-D.ietf-ospf-yang"/>.

Will fix. 

>ipv6-route-target, route-origin, and ipv6-route-origin.

We’ll look at this and get back to you - a lot of time has already gone
into formulating and testing these patterns.
>be re-worked so it also allows values starting with 0x0 and 0X0.

We’ll either change the pattern or the text.

Thanks,
Acee

---

From: Adam Roach <[email-masked]>
Date: Thu, 12 Oct 2017 11:12:20 -0500

> into formulating and testing these patterns.

Yes, and it would be a shame if that work resulted in publishing 
patterns with known issues.

This flaw arises in three formulations (each of which appear multiple 
times), and would be quite easy to fix. These fixes should be obvious by 
inspection.

32 bits (0-4,294,967,295)
   Replace: [0-3]?[0-9]{0,8}[0-9]
   With:    [1-3][0-9]{9}|[1-9][0-9]{0,8}|0

16 bits (0-65535)
   Replace: [0-5]?[0-9]{0,3}[0-9]
   With:    [1-5][0-9]{4}|[1-9][0-9]{0,3}|0

8 bits (0-255)
   Replace: [01]?[0-9]?[0-9]
   With:    1[0-9]{2}|[1-9]?[0-9]

____

As an aside: replacing "[0-9]" with "\d" everywhere would make these 
patterns easier to read in general, but this is merely a readability 
improvement rather than a bug fix. Compare:

          + '(2:(429496729[0-5]|42949672[0-8][0-9]|'
          +     '4294967[01][0-9]{2}|'
          +     '429496[0-6][0-9]{3}|42949[0-5][0-9]{4}|'
          +     '4294[0-8][0-9]{5}|'
          + '429[0-3][0-9]{6}|42[0-8][0-9]{7}|4[01][0-9]{8}|'
          +     '[1-3][0-9]{9}|[1-9][0-9]{0,8}|0):'
          +     '(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|'
          +     '6[0-4][0-9]{3}|'
          +     '[1-5][0-9]{4}|[1-9][0-9]{0,3}|0))|'

Becomes:

          + '(2:(429496729[0-5]|42949672[0-8]\d|'
          +     '4294967[01]\d{2}|'
          +     '429496[0-6]\d{3}|42949[0-5]\d{4}|'
          +     '4294[0-8]\d{5}|'
          +     '429[0-3]\d{6}|42[0-8]\d{7}|4[01]\d{8}|'
          +     '[1-3]\d{9}|[1-9]\d{0,8}|0):'
          +     '(6553[0-5]|655[0-2]\d|65[0-4]\d{2}|'
          +     '6[0-4]\d{3}|'
          +     '[1-5]\d{4}|[1-9]\d{0,3}|0))|'

/a

---

From: Adam Roach <[email-masked]>
Date: Thu, 12 Oct 2017 11:13:30 -0500

16 bits (0-65535)

8 bits (0-255)

____

Becomes:

/a

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 12 Oct 2017 17:15:20 +0000

Hi Adam, 

On 10/12/17, 12:12 PM, "Adam Roach" <[email-masked]> wrote:

>>> restriction of numbers (e.g., to ensure they don't fall outside the
>>>range
>>> that

Yes - this doesn’t appear to be a complicate fix at all. We’re going to
get more eyes on it and do some tests with https://yangcatalog.org/yangre/
 but we should be able to fix this.

>improvement rather than a bug fix. Compare:

Although this is a somewhat controversial subject, we used “[0-9]" for
portability for implementations using non-standard regular expression
parsers. 

Thanks,
Acee

---

From: Robert Wilton <[email-masked]>
Date: Fri, 13 Oct 2017 11:21:06 +0100

Hi,

>   but we should be able to fix this.
For what its worth, and I'm not proposing this is changed now given that 
this has been discussed previously, but I do think that this is another 
example of the dangers inherent with specifying precise, but more 
complex, regular expressions in YANG for checking numerical values.

E.g. I still prefer "[0-9]{0,10}" instead of 
"429496729[0-5]|42949672[0-8][0-9]|4294967[01][0-9]{2}|429496[0-6][0-9]{3}|42949[0-5][0-9]{4}|4294[0-8][0-9]{5}|429[0-3][0-9]{6}|42[0-8][0-9]{7}|4[01][0-9]{8}|[1-3][0-9]{9}|[1-9][0-9]{0,8}|0".

Yes, it allows some invalid numerical values, which would presumably 
fail when the value is converted to a internal numerical format, but it 
is easier to read/review, and is harder to get wrong.  Besides even if a 
stricter regex validates that it the value is syntactically correct it 
still doesn't mean that the correct intended value has been used ...

> portability for implementations using non-standard regular expression
The XML regex language that is reused by YANG is built for Unicode 
support.  Hence '\d' matches all digit characters in all scripts, i.e. 
it actually matches 500+ characters rather than just the ASCII digits 
0-9, and in most network configuration models that is probably not what 
is wanted.  I'm not sure whether it matters that \d matches a much 
larger class of characters, but at least with [0-9] it is entirely 
unambiguous.

Thanks,
Rob

>   
>
> Thanks,
> Acee
>
>>
>> /a
> _______________________________________________
> rtgwg mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtgwg

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 13 Oct 2017 19:36:06 +0000

Hi Adam, 
I have incorporated your suggested pattern improvements in the -17
version. 
Thanks,
Acee