Subject: [forces] AD review of draft-ietf-forces-model-extension

From: "Adrian Farrel" <[email-masked]>
Date: Wed, 2 Jul 2014 19:05:25 +0100

Hi Evangelos,

Thanks for holding the pen on this.

I have done my usual AD review. Normally the purpose of the review is to catch
issues that might surface in IETF last call and IESG evaluation, but in this
case I feel I am also reviewing in place of ForCES WG last call as there was
complete silence from the WG at that time and, indeed, I rather feel that a
number of the comments I make below should have been caught by an active and
engaged working group.

Although most of my comments are editorial in nature, many of them significantly
impact on the utility and purpose of the document, so I believe they need to be
addressed before the document can advance. So I've place the I-D in "Revised I-D
state" and will wait to see a new version.

Cheers,
Adrian

====

Can you clarify for me whether, after the publication of this document
as an RFC, the extensions it defines are part of the ForCES model. That
is, if you were to implement ForCES from scratch after its publication,
would you be expected to include these features?

If the answer is "yes" then this probably updates 5812. If "no" then we
need to work some language to explain that the extensions are options.

I think, looking at, for example, Section 3.1, that this document is
making specific changes to 5812 as well as potentially defining some
additions. That means it really does update 5812.

To handle this you need:
- An "updates" tag in the metadata at the head of the file
- To make the statement in the Abstract "This document updates RFC 5812
  by defining foo..."
- Add a section (probably Section 1.1 inside the Introduction) that is
  called "Updates to RFC 5812" and provides an overview of the changes
  and additions (you can use forward pointers into the rest of the
  document).

---

The nits noted by the Shepherd in his write-up need to be fixed.

---

Abstract
   RFC5812 has defined
   the ForCES Model provides a formal way
Maybe
   RFC5812 has defined
   the ForCES Model that provides a formal way

---

Abstract
Please expand "FE" on first use.

---

Section 1.2

I am not a not a fan of repeating material from other documents. At best
it means that the reviewer has to cross-check to make sure you haven't
introduced any inconsistencies. At worst it means that an update or fix
to one document has to be reproduced in the other document.

Since I doubt that it makes sense to read this document without a 
thorough understanding of 5812 I think you could replace this section 
with a simple set of pointers such as:

   This document uses the terminology defined in the ForCES Model in
   [RFC5812].  In particular, the reader is expected to be familiar with
   the following terms:

   - FE Model
   - LFB (Logical Functional Block) Class (or type)
   - LFB Instance
   - LFB Model
   - Element
   - Attribute
   - LFB Metadata
   - ForCES Component
   - LFB Class Library

---

The RFC Editor will want to place the Introduction as Section 1 in this
document. It would be good if you could make this change before the 
document reaches them so that you can fix any related issues that may
arise.

---

Please be sure to expand all abbreviations that are not shown as "well
known" in the list at http://www.rfc-editor.org/rfc-style-guide/
abbrev.expansion.txt on first use in the main body of text.

---

In the Introduction, you say...

   Additionally backward
   compatibility is ensured as xml libraries produced with the earlier
   schema are still valid with the new one.

How will an old implementation in the field that uses the old schema
react to an xml library produced with the new schema?

---

Section 3
Is this a "proposal" or does the WG have consensus?

---

Section 3.1

   However there are cases where complex metadata are used in the
   datapath, for example two simple use cases can be seen in the
   OpenFlow switch 1.1 [OpenFlowSpec1.1] and beyond:

   1.  The Action Set metadata follows a packet inside the Flow Tables.
       The Action Set metadata is an array of actions to be performed at
       the end of the pipeline.

   2.  When a packet is received from a controller it may be accompanied
       by a list of actions to be performed on it prior to be sent on
       the flow table pipeline which is also an array.

There are several issues with this text. 

a. Are you saying that the use case is "to be able to do what OpenFlow
   does"? Or is there some specific function that you need to be able to
   perform in the context of ForCES?

b. "The Action Set metadata follows a packet inside the Flow Tables" is
   really hard to parse. It might, I suppose, make sense if I read the
   OpenFlow specification, but perhaps you could use just a few more
   words to explain what a Flow Table is, what a pipeline is, and what
   it means to "follow a packet".

c. Isn't OpenFlow 1.1 rather an old version to be referencing? As you
   are making it a normative reference, it might be best to try to be
   up-to-date.

d. I think s/prior to be sent/prior to being sent/

e. Probably s/flow table/Flow Table/

---

Figure 5

I think there are some indentation issues the fixing of which could make
the schema fragment easier to read.

---

Section 3.2

   Additionally it appends to the declaration...

What is "it"?

---

Section 3.2

I think that I can work out what is meant to happen with Figure 5, but
it would be cleaner if you gave the "before and after" fragments as you
have done in the previous two cases.

---

Section 3.2

Great and really helpful that you have provided an example in Figure 6,
but you need to briefly note it somewhere in the text.

---

Section 3.3
   In the original schema, the access type can be only be defined on
   components of LFB and not on components in structs or arrays.

s/be only/only/
s/LFB/an LFB/

---

Section 3.3

   If by accident an
   access type for a component in a capability is defined, the access
   type MUST NOT be taken into account and MUST always be considered as
   read-only.

It is polite of you, but "by accident" gives the impression that it can
be done "on purpose" with a different result. I think you need:

   The access type for a component in a capability is always read-only 
   per [RFC5812]. If an access type is provided for a component in a
   capability it MUST be ignored.

---

Section 3.3

The example in Figure 9 helpfully shows the higher-level access type
that applies to the whole struct as well as an example of the fine-grain
access type applied to one of the components. However, the schema 
fragments in Figures 7 and 8 do not show the higher-level access type. I
think it would be helpful if they did.

As per Figure 6, it is great to have an example, but the text needs to
make reference to it.

---

Section 3.4

   This event condition is particular useful

s/particular/particularly/

---

Section 3.5

   Experience however has proven valuable at least for debug
   reasons, to have statistics per LFB instance to monitor sent/received
   messages and errors for communication between CE and FE. 

That's a bit garbled. How about...

   Experience has shown that, at least for debug reasons, it would be 
   useful to have statistics per LFB instance to monitor sent/received
   messages and errors in communication between CE and FE. 

---

Section 3.6

   This document augments the derivedFrom part of the LFB class
   definition with a mandatory version attribute when the derivedFrom
   field is used.

I don't see how this is backward compatible as described in the 
Introduction. There you say that XML libraries produced with the earlier
schema are still valid with the new one. But this text and schema 
fragment seems to require that the version attribute is present which
would mean that a user of the new schema would reject an older XML
library.

---

Section 3.6

Again, the example needs to be referred to from the text.

---

Section 3.7

   The following validation rules have been appended in the original
   schema in [RFC5812]:

This is too ambiguous. In most sections when you use the past tense 
and refer to 5812 you are talking about stuff that is already in that
RFC. However, if I take that interpretation of this text then this
section is completely empty. So you must mean something else.

---

Section 4

How am I supposed to read this section compared to Section 4.9 of 5812?
There is no introductory text, and no explanation.

I suspect that this is a complete replacement of Section 4.9 of 5812 in
which case this document really does update 5812 in a fairly significant
way.

Or should you have incremented the model number?

---

6.  IANA Considerations

   This specification requests that LFB Component ID 0 to be reserved.

This statement is almost completely unhelpful to IANA!

Which registry contains the LFB Component IDs?
What information do you want IANA to record?

Section 3.5 says both "reserve" which has specific meaning according to
RFC 5226 and "for LFB properties" which seems to mean you want a 
specific assignment. Although Section 3.5 does talk about "disallowing"
the value zero.

Maybe you want a registry for Component ID values? Or maybe you are 
making a generic statement about the use of Component ID zero. In the
former case you have some IANA work to do. In the latter case, you need
to clarify the statement in 3.5 and change Section 6 to say that no IANA
action is required.

In the latter case, wouldn't this also be a good thing to capture in 
3.7?

---

Of course, what you say in Section 7 is true, but have you considered
how the extensions in this document change the vulnerabilities described
in 5812? I think a little text on each extension/change to say whether
it increases or decreases the risks would be nice.

---

Section 8.2

RFC 2119 should be a normative reference (or at least that is how you 
have used it in section 1.1).

---

From: "Haleplidis Evangelos" <[email-masked]>
Date: Thu, 3 Jul 2014 16:33:52 +0300

Greetings Adrian,

Thank you very much for the review.

Please see inline. I have snipped out all your questions that I agree and
would have answered with simply ok.

I'll try to have it completed by tomorrow before the cut-off date.

Regards,
Evangelos Haleplidis.

>   and additions (you can use forward pointers into the rest of the

[ΕΗ] The extensions are optional (will even change the version attribute as
such) thus it makes no reason for someone to implement them. If I
implemented ForCES from scratch it would be useful to include these features
but not necessary. The goal was to be backwards compatible.

However I agree with your assessment and also consider it is an update,
there are a couple of features, like the new event description, default
values and complex metadata, that I consider very useful.

> react to an xml library produced with the new schema?

[ΕΗ] Assuming that the xml library produced uses none of the new optional
extensions, then there should be no issues. However if they use even one,
then an old implementation will not be able to understand the new xml
library (at least the new parts).

> Is this a "proposal" or does the WG have consensus?

[ΕΗ] It has WG consensus. Will remove the "proposal".

>    perform in the context of ForCES?

[ΕΗ] No. These uses cases simply highlight the need for the complex
metadata.
For example, when we attempted to do an OF library using ForCES
(http://tools.ietf.org/html/draft-haleplidis-forces-openflow-lib-03 - to
which we need to allocate some time and update) we found that the use of
these kinds of metadata are needed.
>    it means to "follow a packet".

[ΕΗ] Thank you for this. Changed to: "The Action Set metadata is an array of
actions descriptors, which traverses the processing pipeline along with the
packet data." While descriptive it also removes the need to add more info
irrelevant to this document.

>    are making it a normative reference, it might be best to try to be

[ΕΗ] Also, I'm moving the OF 1.1 into an informative reference. This is an
example use case. There is no reason for this be a normative reference. In
addition, OF1.1 first introduced the Action Sets, and that's the reason for
mentioning it, as well no changes in the action set has occurred since that
would invalidate the use case. That is the reason for the wording: "OF1.1
and beyond"

>    Additionally it appends to the declaration...

[ΕΗ] Thank you for this. "IT" is the specific extension

> Introduction. There you say that XML libraries produced with the
> earlier schema are still valid with the new one. But this text and
> schema fragment seems to require that the version attribute is present
> which would mean that a user of the new schema would reject an older
> XML library.
> 

[ΕΗ] Thank you for this. You have a very valid point. In order to address
the issue, and still retain backwards compatibility we should probably make
the version attribute optional. That however does not resolve the issue that
Jamal pointed out. However we will include something along the following
lines:
"If the version attribute is not included then it is assumed that the
version imported is always 1.0".
This clearly solves the issue and retains compatibility.

> This is too ambiguous. In most sections when you use the past tense and
> refer to 5812 you are talking about stuff that is already in that RFC.
> However, if I take that interpretation of this text then this section
> is completely empty. So you must mean something else.
> 
> ---

[ΕΗ] The original schema in RFC5812 contains some validation rules. We have
introduced several new to validate the schema against more rules. Would the
following change be more accurate?:
"In the new schema, we have introduced the following validation rules that
did not exist in original schema in [RFC5812]"

> which case this document really does update 5812 in a fairly
> significant way.
> 

[ΕΗ] We will increase the model number.

> to clarify the statement in 3.5 and change Section 6 to say that no
> IANA action is required.
> 

[EH] Thank you for this. We definitely don't need a registry for Component
ID values. We are making a specific statement that Component ID zero MUST
NOT be used. 

I'm not certain, but I think that that does not constitutes an IANA
considerations. I'll remove it from section 6. I will also try to see if I
can capture it in 3.7.

> how the extensions in this document change the vulnerabilities
> described in 5812? I think a little text on each extension/change to
> say whether it increases or decreases the risks would be nice.
> 
> ---

[EH] I'm a little confused. 
The changes have no effect on security, they are simply constructs to write
XML library definitions. They have no effect on security semantics with the
protocol, thus they do not affect security issues as it the case described
in 5812. 
If that is satisfactory, I'll write that down in the security
considerations.