Subject: [Gen-art] Gen-ART review of draft-ietf-ippm-storetraceroutes-07

From: <[email-masked]>
Date: Thu, 10 Jan 2008 14:09:40 +0200

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call 
comments you may receive.

Document: draft-ietf-ippm-storetraceroutes-07
Reviewer: Pasi Eronen
Review Date: 2008-01-10
IETF LC End Date: 2008-01-15
IESG Telechat date: (not known yet)

Summary: This draft is on the right track but has open issues,
described in the review.

Comments:

1) A general comment: The document would be much easier to read if
Section 4 included a sample traceroute output, and some appendix
included an example XML file matching the schema.

2) The information model doesn't describe the relationships
between the elements: e.g. the information elements in 5.2.2
are just a list of information elements, without good specification
on how they're grouped, how many times they can occur, etc.
(This information is obviously present in the XML schema)." 

(See e.g. RFC 5070 for an example of an information model
that describes not just the individual elements, but their
relationships. I'm not necessarily looking for anything that
fancy, but something along the lines "The Measurement class
contains one ResultsStartDateAndTime element, and one or more
ResultsProbe elements. Each ResultsProbe element contains..."
Also, information about which elements are always present
and which may be omitted is clearly part of the information
model.)

3) The information model contains elements OSName, OSVersion,
and ToolVersion, but no ToolName element. While clearly many
operating systems ship with a single well-defined traceroute-like
tool, additional tools (beyond those shipping with the OS)
are available. Thus, I'd recommend adding a ToolName element.
(This would also help in interpreting implementation-dependent
information elements, such as CtlMiscOptions.)

4) Section 5.2.2.9 specifies the HopGeoLocation element, but not
its contents, semantics, or methods used to determine it. We do
have IETF standards for representing both geospatial and civil
locations (e.g. RFC 3825 and 4476); presumably they could be
used. Like was done for the AS number, it might be useful to also
specify the method that was used to get the location.

5) Section 5.2.3.1 states that the TestName element is "locally
unique"; without some definition of "locally", this is meaningless.
In RFC 4560, the scope is clear: it's unique "within the scope of a
traceRouteCtlOwnerIndex" (of a specific Management Information
Base, running on particular node -- this part of the scope being
communicated by SNMP). 

6) Section 5.1, InetAddressType: "The allowed values are to be
intended as imported from [RFC4001]; an additional allowed value 
is "asnumber". At least the XML schema later doesn't seem to allow 
all values from RFC 4001 (in particular, ipv4z and ipv6z); perhaps
the intent was to import only some of the values?

7) Section 5.2.1.7 defines the CtlPort as "Specifies the base UDP
port used by the traceroute measurement.  A port that is not in use
at the destination (target) host needs to be specified."

This description needs some clarification, as different traceroute
tools may function slightly differently here.  At least some common
implementations use a range of port numbers (selecting different
port number for each probe), and thus it's not sufficient to specify
"a port that is not in use"; several other ports need to be not in
use as well. Other implementations may (at least optionally)
use the same port number for all probes (e.g. "-c" option in
OpenBSD traceroute). And for TCP measurements, you may want
to use a port that is in use.

8) Section 5.2.2.14: The ResultsHopRawOutputData should contain
some description of what's expected to be contained (beyond just
saying "raw output data"). Appendix C.2 suggests that this
element is similar to traceRouteProbeHistoryLastRC in RFC 4560,
but at least that contained an integer code. For example, is 
this the line printed by traceroute tool (expected to be printable
ASCII useful for humans); the raw ICMP packet received (in which
case it isn't UTF8, and needs to use xs:base64Binary data type
in XML schema), or something else?

(Even "an implementation-dependant printable string, expected 
to be useful for a human interpreting the traceroute results" 
would be much more detailed specification.)

9) Section 5.2.1.18 defines just three fixed values for CtlType
(UDP, TCP, and ICMP); these are also used in the XML schema, with
no possibility for extension. This is in contrast with RFC 4560,
where CtlType is extensible.

The most common cases (UDP packet to unused port, and ICMP Echo
Request) are pretty clear, but other alternatives are possible.
E.g. as described in Appendix A.1, for TCP, you might use SYN
packets or FIN packets -- which would lead to different results in
the presence of stateful firewalls (pretty common). Also, for UDP,
you could use an UDP port which is in use (and try to get the
application to respond, instead of getting ICMP Port Unreachable);
even RFC 4560 supports UDP pings with "echo" protocol and SNMP
query (and I have a vague recollection of once seeing a tool that
used DNS queries, but can't really recall anything certain).

Given this, I'd recommend at least documenting that other
possibilities (beyond the ones described) here exist, and having
some sort of idea how they would fit in the XML.

10) Inconsistency between XML schema and information model: 
the schema's inetAddressTypeWithoutDns type contains a
"noSpecification" element, which is not present in 
inetAddressType type, or in information model.

11) Inconsistency between XML schema and information model:
_ipASNumberMappingType type element (and its semantics) is 
missing from the information model of Section 5.

12) Inconsistency between XML schema and information model:
information model describes CtlByPassRoute and CtlDontFragment 
as truth value (true/false), but the XML schema allows three 
different options (true, false, and omit the whole XML tag). 
The semantics of the last option are not defined in the 
information model.

13) Inconsistency between XML schema and information model: 
similar concerns apply to several other data elements that have
minOccurs="0": the information model should specify what omitting
the element means.

For some elements (e.g. HopGeoLocation) this is pretty obvious; 
for others (at least CtlProbeDataSize, CtlTimeout, CtlProbesPerHop,
CtlPort, CtlMaxTtl, CtlDSField, CtlInitialTtl, and CtlType), some
kind of semantics should be defined

(e.g. for MaxTtl, possible semantics could be "30" (this is 
the definition used in RFC 4560 -- but in that case, making the 
field mandatory to include would be simpler), "implementation 
dependent default", "not known", or "no maximum (except 255)").

14) XML schema, _dateAndTime type: this type seems unnecessary, as
the basic xs:dateTime type (which is used as a component here) can
represent time with millisecond resolution (and if milliseconds
were included twice, that would be confusing).

15) XML schema: Many of the strings (TestName, OSName, OSVersion,
ToolVersion, CtlMiscOptions, CtlDescr, HopGeoLocation) have
maxLength restrictions which seem rather arbitary and small.

For example, for OSVersion, one obvious choice on Linux would be 
the output of "uname -rv" (possibly combined with something like 
contents of /etc/redhat-release), but that's easily over 32 characters.

Also, the maximum lengths don't match the length restrictions in
RFC 4560 (e.g. CtlMiscOptions is max. 255 octets in RFC 4560, but
100 characters here).

(I took a quick look at recent RFCs which contain XML schemas,
and a common practice seems to be omitting maxLength restrictions
for fields that can contain arbitrary text.)

16) The XML schema contains the concept if "request", in addition
to storing measurements of traceroutes that have already happened.
The information model in Section 5 should probably mention that the
Configuration Information Elements can describe not just traceroute
measurements that have already happened, but also configuration to
be used when requesting a measurement to be made (quite different
semantically, even if the individual information elements are the
same).

17) The document uses RFC 2119 key words, but does not include 
RFC 2119 in the references section.

18) Minor nits

5.1, typo in "Unsigned8 - The type "Unsigned32" represents a value"
5.2.1.7, CtlPort should be Unsigned16 instead of Unsigned32
5.2.1.8, CtlMaxTtl should be Unsigned8 instead of Unsigned32
5.2.1.9: CtlDSField should be Unsigned8 instead of Unsigned32
5.2.1.14, CtlMaxFailures should be Unsigned8 instead of Unsigned32
5.2.1.16, CtlInitialTtl should be Unsigned8 instead of Unsigned32
5.2.2.5, HopIndex should be Unsigned8 instead of Unsigned32 
5.2.2.6, IndexPerHop should be Unsigned8 instead of Unsigned32 
XML schema, _inetAddressASNumber type "as a 24 bit number": old BGP
used 16-bit AS numbers, now extended to 32 bits. Typo?

The remaining comments about schema details are, to some extent,
style issues, and thus just my opinion as to what would be "nicer"
way to do this:

19) XML schema: there are several places where it uses a combination
of xs:complexType+xs:sequence+xs:element, where a single
xs:simpleType would have been sufficient (and would lead to 
simpler XML).  For example,

  <ResultsStartDateAndTime>
    <dateAndTime>
      ...something...
    </dateAndTime>
  </ResultsStartDateAndTime>

could be simply

  <ResultsStartDateAndTime>
     ...something....
  </ResultsStartDateAndTime>

I would recommend "flattening" these when possible (at least
_ResultsStartDateAndTime, _ResponseStatus, _Time,
_ResultsEndDateAndTime -- see also the next comment about 
flattening addresses).

20) XML schema: The representation of InetAddresses is particularly
complex (with several levels of nesting), and also encodes the
address type in two different places (giving opportunities for
mismatches, which can't be easily constrained with the schema).  
If I'm reading the schema right, the XML would look like this:

  <CtlTargetAddressType>
     <targetAddressType>ipv4</targetAddressType>
  </CtlTargetAddressType>
  <CtlTargetAddress>
    <targetAddress>
      <inetAddressIpv4>192.0.1.3</inetAddressIpv4>
    </targetAddress>
  </CtlTargetAddress>

But exactly the same information model could also lead to this
much simpler XML:

  <CtlTargetAddress>
    <inetAddressIpv4>192.0.1.3</inetAddressIpv4>
  </CtlTargetAddress>

21) XML schema: _probeRoundTripTimeNotAvailable type uses the 
string "NotAvailable", while 5.2.2.11 uses the string
"RoundTripTimeNotAvailable". Also, the encoding is a bit 
redundant:

  <RoundTripTime>
    <probeRoundTripTimeNotAvailable>
      NotAvailable
    </probeRoundTripTimeNotAvailable>
  </RoundTripTime>

could be simply
  
  <RoundTripTime>
    <probeRoundTripTimeNotAvailable />
  </RoundTripTime>

22) XML schema: The definition of _traceRoute type is pretty complex
and long; it seems that something along this lines would have the
same effect?

  <xs:sequence minOccurs="1">
    <xs:element name="Request"
                type="_Metadata" minOccurs="0" />
    <xs:element name="MeasurementMetadata"
                type="_Metadata" minOccurs="0" />
    <xs:element name="Measurement"
                type="_Measurement" minOccurs="0" />
  </xs:sequence>

Best regards,
Pasi

_______________________________________________
Gen-art mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/gen-art

---

From: Lars Eggert <[email-masked]>
Date: Thu, 10 Jan 2008 14:15:44 +0200

Hi,

thanks for the thorough review, Pasi!

Authors, please let us know how you're planning to address these  
comments. I'm assuming that a revision will be required before this  
document can enter IESG evaluation.

Lars