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

Martin Thomson <martin.thomson@gmail.com> Mon, 09 March 2015 22:36 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 11FF11ACDEF for <gen-art@ietfa.amsl.com>; Mon, 9 Mar 2015 15:36:38 -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 MgTjIlrVAKup for <gen-art@ietfa.amsl.com>; Mon, 9 Mar 2015 15:36:24 -0700 (PDT)
Received: from mail-ob0-x22b.google.com (mail-ob0-x22b.google.com [IPv6:2607:f8b0:4003:c01::22b]) (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 A893A1ACDDE for <gen-art@ietf.org>; Mon, 9 Mar 2015 15:36:12 -0700 (PDT)
Received: by obcuz6 with SMTP id uz6so10553389obc.7 for <gen-art@ietf.org>; Mon, 09 Mar 2015 15:36:12 -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=P8Ba3DAPImFKf9OdF1LV5tRWXVayvYy58Xd+/IwYMkw=; b=Wal9wqqW1xukHdRmHbcescDNsjCNs19VXC1AGmSFGdabTrGiLS78oHH7VtA+c/lJrR 0USfq0PViDpIerB38vCNMILbZYE2Po9sFW+8GfkweIkhBaGPxIRjnBFQ5Ny9sJqwAteI yc6fhTaRd6TM1xoN281Y5M/97/ZE7A48m0VQkcLkOnNoL7n6+IRgfSdPH6e+3RkGqpEr M0m0KK5jAxufn6aAXQ3onnRJVZumXkTk0b2nORKlSiw2KYvl3sg9cLvvIRocedWhsM4i 2vbKRVdsXFKPEHiTgCEURz4Plgljf/GaUYsIcAahE7BbHwolrpOC3CBGMZiLUJ6bQ3cy x+GA==
MIME-Version: 1.0
X-Received: by 10.202.7.1 with SMTP id 1mr21961399oih.16.1425940572224; Mon, 09 Mar 2015 15:36:12 -0700 (PDT)
Received: by 10.202.225.135 with HTTP; Mon, 9 Mar 2015 15:36:12 -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 15:36:12 -0700
Message-ID: <CABkgnnW0tk-ZUh7LFps0yC75scqxTOSuKAvNH2wdx8YjOPPKcw@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/ByJ8ky9C6NltbxXPKoOT2va9g3k>
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 22:36:38 -0000

Gotta run, but I hope these responses help.

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.

<> rules, not () rules.  <> is a wildcard.

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

If you aren't fixing it now, fix it later and don't worry about saying
that you will.  Because you might not.

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

Some people get antsy about lowercase uses because it is confusing.  I
don't.  If you've checked I'm happy.  Expect others to notice, that's
all.

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

We get this question all the time.  You don't need to expand, because
RFC 7230 does that for you...

> Finally got to the bottom !
>
> Thanks much for the review.
>
> Francois