Re: [Gen-art] genART review of draft-ietf-cdni-logging-15

Martin Thomson <martin.thomson@gmail.com> Mon, 09 March 2015 23:24 UTC

Return-Path: <martin.thomson@gmail.com>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1487A1A8AAB for <gen-art@ietfa.amsl.com>; Mon, 9 Mar 2015 16:24:08 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -3.4
X-Spam-Level:
X-Spam-Status: No, score=-3.4 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, GB_I_LETTER=-2, J_CHICKENPOX_31=0.6, SPF_PASS=-0.001] autolearn=ham
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id PkxGqbgbRKrf for <gen-art@ietfa.amsl.com>; Mon, 9 Mar 2015 16:24:05 -0700 (PDT)
Received: from mail-ob0-x232.google.com (mail-ob0-x232.google.com [IPv6:2607:f8b0:4003:c01::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id E99C11ACDDA for <gen-art@ietf.org>; Mon, 9 Mar 2015 16:24:03 -0700 (PDT)
Received: by obcva8 with SMTP id va8so25957140obc.8 for <gen-art@ietf.org>; Mon, 09 Mar 2015 16:24:03 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=fkpImKJSeX+LyQA1er6a7KB2sIQWOUMrAmDYSWxS1Ao=; b=ODZrMxRcaOkbJSdFlv51bH5CO1vOWQLFndl+uZybh/6mCEeGLwRJH1GUAIlE/eg9On L53Oc/H3YhgLNM+bZ+pi2GDQPkRkBWxC+eWubwArauPkth7Sc5cFdw7KiGZr+ytgSupu swb8p97tALdblC1um1n/2WIGTv0RegdUKz4ZV+9zHj07NbHHLuCQYH6WH0yEAUucmFag vanJh/FabEQvAYufJelQPP4zx93ojozW1mjjRBwgDrnykecGrE0s02LS8cWdAZNSuPDu qmlcAQsLX1IcM2N0cbMvvhUtzOOeXlzlBq/jxDbXmwqlzKq+GSLcIJbzwBbK2d/iUMrL nGYQ==
MIME-Version: 1.0
X-Received: by 10.182.68.12 with SMTP id r12mr23702429obt.84.1425943443416; Mon, 09 Mar 2015 16:24:03 -0700 (PDT)
Received: by 10.202.225.135 with HTTP; Mon, 9 Mar 2015 16:24:03 -0700 (PDT)
In-Reply-To: <76ADC0A7-9093-4722-9BE3-E27CFEA88F5A@cisco.com>
References: <CABkgnnU95w0SzS+PVZL0Lh0bCjfKr7DwYp0xajJ8-WBKnmao6w@mail.gmail.com> <76ADC0A7-9093-4722-9BE3-E27CFEA88F5A@cisco.com>
Date: Mon, 09 Mar 2015 16:24:03 -0700
Message-ID: <CABkgnnWS72ik7UsSy4xsNwaAcVNoaWkYzoQdRbnmLZ4-aRVJNg@mail.gmail.com>
From: Martin Thomson <martin.thomson@gmail.com>
To: "Francois Le Faucheur (flefauch)" <flefauch@cisco.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <http://mailarchive.ietf.org/arch/msg/gen-art/jGLvMIW98Il_FXuA74WYZCwZPYc>
Cc: "gen-art@ietf.org" <gen-art@ietf.org>, "draft-ietf-cdni-logging.all@tools.ietf.org" <draft-ietf-cdni-logging.all@tools.ietf.org>
Subject: Re: [Gen-art] genART review of draft-ietf-cdni-logging-15
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 09 Mar 2015 23:24:08 -0000

On 9 March 2015 at 10:13, Francois Le Faucheur (flefauch)
<flefauch@cisco.com> wrote:
> Hi Martin,
>
> Getting onto you first batch of comments:
>
>>
>> Summary:
>>
>> Major issues:
>>
>> Minor issues:
>>
>> S3.1 needs to define the basic atom that is used for the logging file.
>> It appears as though the atom is an octet.
>
> We have the following statement:
> " A CDNI Logging File MUST contain a sequence of lines containing US-
>    ASCII characters [CHAR_SET] terminated by CRLF.”
> I see that as defining the atom as an ASCII character.

I'd prefer it be explicit near the ABNF definitions.

>> S3.2 uses the wildcard ABNF rule (i.e., <text....>) too aggressively.
>
> Can you give me the specifics (unless this refers to what you've expanded below already)

Using <> says to a generic processor : ignore everything that follows.
It breaks automatic validation.  It's a cop-out that you don't need.
You know what letters you want to permit, so build a rule for it.

That does mean that you need to consider whether you want to permit
the ASCII BELL (or BEL) character, but I think that you already have
an answer ready-to-hand.

>> RECLINE (a potentially confusing name) could be more strictly defined.
>
> potentially confusing, but hopefully not really given the consistent structure across DIRLINE/DIRGROUP/RECLINE/RECGROUP.
>
>>
>>    <CDNI Logging File> = 1*<DIRGROUP RECGROUP>
>>
>> ... is not valid and could be:
>>
>>    CDNI-LOG-FILE = 1*(DIRGROUP / RECGROUP)
>
> I have changed it to :
> "
> CDNI Logging File = 1*<DIRGROUP RECGROUP>
> "

Again, <> is not appropriate here, it puts what you have inside it
down as comments, not actionable BNF.  Also, that name is not valid
ABNF.

> This is to avoid defining an extra name (CDNI-LOG-FILE) and have to explain that CDNI-LOG-FILE is actually the CDNI Logging File.
> Let me know if that is not valid.
>
>>
>> That implies that an empty file is not valid.  I'd have thought that *
>> rather than 1* would be better.
>
> A logging file with no Logging Records is considered valid and allowed, but a logging file without any directive is not considered valid.
> This is consistent with the text that I added based on our previous discussion stating that “occurrences” for all directives need to be respected (in other words, some directives are mandatory).

I think that you could express that better than you have.

LoggingFile = 1*(1*DIRGROUP *RECGROUP)

Might work.

>> S3.3 again uses wildcard rules too aggressively.  DIRNAME (not the
>> unix command) should be defined more explicitly, even if it is just to
>> specify what characters are allowed.  Use text to further constrain
>> it.  As it stands, a generic processor cannot know that the name
>> doesn't permit the inclusion of (for instance) ":" or CRLF.  A common
>> method is to define something like this as:
>>
>>    DIRNAME = Version-directive / UUID-directive / ... / extension-directive
>>
>> More commonly, go up a level and define directive using the choice.
>> That way, each directive can define a name and value together using
>> ABNF.  e.g.
>>
>>    Version-directive = "Version" ":" HTAB "CDNI" "/" 1*DIGIT "." 1*DIGIT
>>
>
> I’ll park these ABNF comments since I got many others and will handle them together.
>
>
>> I wouldn't be so prescriptive about the version format.  Unless there
>> are specific semantics you want to extract from each digit.
>
> Fair enough. I have relaxed it and simply made the format NHTABSTRING.
>
> In the corresponding IANA section for the Version registry, I have added:
> “
> Within the registry, Version values are to be allocated by IANA
>    according to the "Specification Required" policy specified in
>    [RFC5226].  Version values are to be allocated by IANA with a format
>    of NAMEFORMAT (see Section 3.1).  Version values corresponding to
>    specifications produced by the IETF CDNI Working Group are to be
>    allocated a name starting with "CDNI".  All other Version values are
>    to be allocated a name that does not start with "CDNI”.
> “

Creating carve-outs in registries like that is inadvisable.  Someone
might attach semantics to them and then you get into trouble.  See the
RFC on x-.


> I have kept CDNI/1.0 as the value allocated by this document.
>
>
>>
>> What is a receiver expected to do if the version *isn't* CDNI/1.0 ?
>> Discard the entire file?  That doesn't seem like a good way to ensure
>> forward compatibility.
>
> This has been discussed and fixed already.
>
>
>>
>> UUID: this a Universally Unique IDentifier (UUID)
>>         from the UUID Uniform Resource Name (URN) namespace specified
>>         in [RFC4122]) for the CDNI Logging File.
>>
>> I don't know how to apply this.  Is it the UUID portion of the URN or a URN?
>
> Good point.
>
> Text has been modified to:
> “
> this a Uniform Resource Name (URN) from the
>          Universally Unique IDentifier (UUID) URN namespace specified in
>          [RFC4122]).  The UUID contained in the URN uniquely identifies
>          the CDNI Logging File.
> "
>
>
>>
>> Please don't use MUST here: If the
>>         two values are equal, then the received CDNI Logging File MUST
>>         be considered non-corrupted.
>
> Done. Replaced MUST by “is to be”.
>
>>
>> MD5 collisions are easy to manufacture.  I'd also drop the MUST from
>> the following sentence.
>
> Done. Replaced MUST by “is to be”.
>
>>
>> S3.4: more wildcards
>
> Sorry. I didn’t get that.
>
>>
>> S3.4.1: I wouldn't worry about HTTP/2 being special here.  All that
>> speculation is painful, especially since HTTP/2 will likely be
>> published before this document.  Better to just cite it and avoid
>> getting caught up in the details.
>
> The thing is that there are additional things in HTTP2 that could be logged and the WG has not yet discussed whether there is real value in doing that, and how. So we do not have specific extensions for those. So we need to be clear that we “only” log the same thing for HTTP/2 as for HTTP/1.1, even if we know that one may potentially want to log more.
> And I’d rather we don;t hold publication of this document until teh WG has worked out exactly waht to do for the few additional things specific to HTTP/2.
> So I think the current text is quite appropriate.
>
>
>>
>>
>> Nits/editorial comments:
>>
>> There are lots of uses of lowercase "may”.
>
> Right. But we’ve been quite careful at using “MAY" as per RFC2119 and “may" where appropriate.
> Do you see specific occurences of “may” that are not appropriate?
>
>>
>> Page 6 has a few instances of missing hyphens in dCDN3/dCDN2 etc…
>
> Fixed.
>
>> S6.2.1: chunck
>>
>
> Fixed
>
>> S3.1 the DATE/TIME rules can/should reference RFC 3339
>
> Good point.
>
> NEW:
> “
> Dates are encoded as "full-date" specified in [RFC3339].
> “
> and
>
> NEW:
> “
> Times are encoded as "partial-time" specified in [RFC3339].
> “
>
>>
>> S3.3 using ":" HTAB as a separator makes it hard to construct these
>> files manually.
>
> This is a recommendation from ELF and commonly used today. Since we want to be as consistent as possible with ELF, I’d rather we stick to HTAB.
>
>>
>> S3.4 use of "c:" in combination with the unordered list is confusing
>> when the prefix is actually "c-".  Maybe use the string "c-" quotes
>> and all to denote the prefix.
>>
>
> Done. For example it says:
> “
> "c-" refers to the User Agent that issues the request (corresponds to the "client" of W3C Extended Log Format)
> "
>
>> S3.4.1 citing RFC 7230 is sufficient for HTTP/1.1; and probably for
>> HTTP in general at this moment.
>
> RFC3270 itself says that HTTP1.1 is specified collectively by RFC3270, RFC3271,… RFC3275. So we may as well be consistent with that.
>
>
> Finally got to the bottom !
>
> Thanks much for the review.
>
> Francois