RE: [Policy] FW: I-D ACTION:draft-reyes-policy-core-ext-schema-04 .txt

["John Strassner" <[e-mail]>]

Hi Mircea,
 
thanks for addressing these issues, and apologies for the delay in
response. Please see inline (<js>..</js>) for clarifications.
 

regards,
John

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] 
Sent: Thursday, March 25, 2004 7:59 PM
To: John Strassner; [e-mail]
Subject: RE: [Policy] FW: I-D
ACTION:draft-reyes-policy-core-ext-schema-04 .txt



John, 

I have reviewed your comments in detail and made several changes to the
PCELS text (to be submitted in a few days) to address these issues.
While for the most part I understand your concerns, there are a few
items that I would like to discuss in more detail. See my comments below
marked <mircea></mircea>.

Thank You, 
Mircea. 

-----Original Message----- 
From: John Strassner [mailto:[e-mail]] 
Sent: Friday, February 13, 2004 9:32 PM 
To: '[e-mail]'; '[e-mail]' 
Subject: RE: [Policy] FW: I-D
ACTION:draft-reyes-policy-core-ext-schema-04 .txt 


First, I support Kurt's comments on LDAP, and will reply to those in a
separate email. 
<mircea>Kurt's recommendations will be addressed in the next revision. 
</mircea> 

Second, I list below a set of additional comments on this draft. 

Third, the lack of an overall diagram makes it very difficult to
evaluate the correctness of this model. This draft is not complete
enough to construct such a model.

<mircea>Can you be more specific. The document includes several diagrams
and tables. What is it missing? 
</mircea>  

<js> True, there are several diagrams and tables. However, the draft
lacks an overall conceptual model. For example, if you look at RFC3060,
Figure 1 shows an overview of all of the classes and their
relationships.  Note that there is no need to show attributes in such a
picture - I'm just looking for a **visual** overview of how the
different classes fit together. </js>

Fourth, a cursory scan revealed that there is no pcelsPolicyGroup class.
This is strange, since PolicyGroup is listed as a subclass of PolicySet
in RFC 3460. Why is this?

<mircea>pcelsGroup will be added in the new revision. 
</mircea> 

Fifth, why is there a pcelsRule and a pcimRule class? 
<mircea>I do not understand the issue. 
</mircea>  

<js> Sorry for not being clearer. I understand that you wanted to create
your own class (pcelsRule) because the semantics of RFC3460 were
different (for PolicyRules) than those of RFC3460. I support your
mentioning both in the draft, since there was feedback (e.g., from Ryan)
that some implementations were still using pcimRule. However, I think
that given this feedback, this draft needs some guidelines as to when
one would use pcelsRule and one would use pcimRule, and what the
implications of doing this are (e.g., how priority is implemented).
</js>

Sixth, why is there no pcelsRuleValidityAssociation subclass? At this
point, <mircea>I do not understand the issue. PCELS reuses
pcimRuleValidityAssociation that is defined in PCLS

</mircea>  

<js> True, this is addressed in Note 1 in page 27 of the draft. Looking
at your class structure, since you subclassed other associations, I was
surprised that you didn't subclass this one as well. This is because
pcelsRule and  pcimRule are siblings, and pcimRuleValidityPeriod (in
PCIM) is defined to exist between pcimRule and policyConditionTimePeriod
only. So, how do pcelsRule instances use a policyConditionTimePeriod?
</js>

I started to go through the document in detail with my developers to try
and implement it. We couldn't. We give you inconsistencies that we
noticed (grammatical and otherwise) through page 31).

Finally, I was surprised to see a lack of an Acknowledgments section,
especially given the amount of feedback that several people on this list
gave the authors. That's in poor form.

<mircea>Acknowledgments will be added in the new revision. 
</mircea> 

Comments are as follows: 

  - s/RFC zzzz/RFC 3703 
<mircea>Fixed. 
</mircea> 
  
  - page 3. You write: "...the combined class hierarchy for the LDAP 
    object classes defined in [PCLS] and in this document". You should 
    include concepts from 3460 that you mapped into new classes, and 
    add that you defined new classes not in 3460 or 3703. 
<mircea>Fixed. 
</mircea> 

  - page 4-7, class diagram - this diagram has no caption. Please add 
    one. In addition, I find the diagram inpenetrable, in that the 
    reader has no idea where these classes came from. I think you need 
    a simpler introduction saying 3060 provided this, 3460 did this, and

    thus we came up with this. Take this key and show, for any class 
    that isn't new in this document, where it came from. 
<mircea>Fixed. 
</mircea> 

  - page 4 - why is your class named pcelsFilerEntry, when 3460 names 
    its class FilterEntryBase? 
  - page 4 - why is your class named pcelsIPHeaders, when 3460 names 
    its class IPHeadersFilter? The Filter part is important! 
  - page 4 - why is your class named pcels8021Headers, when 3460 names 
    its class 8021Filter? The Filter part is important! 
  - page 4 - why is your class named pcelsCompoundFilterAuxClass, when 
    a more consistent name would be
pcelsCompoundFilterConditionAuxClass? 
    The Condition part is important! 
<mircea>All renamed. 
</mircea> 

   - general reflections on the class diagram: part of the problem is
that 
    you are building a schema from three different sources: (1) RFC
3703, 
    (2) RFC 3460, and (3) your own additions. I see no discussion on how

    these relate to each other, which would have been helpful. 
<mircea>The new revision will indicate all these sources explicitly. 
</mircea> 

   - page 7 - you didn't state whether this is for all associations.
This 
    is exacerbated by you saying: "...might need to implement the 
    association..." - which implies a single association. In addition, 
    this is a terse description - the naive reader won't understand why 
    aux classes are being used - you need a reference or a couple of 
    sentences explaining this. 
<mircea>Added example in support of the generic text. Please note that
the reader is not going to be that naive. Section 2. ("Relationship to
other Policy Framework Documents") will also indicate that "These three
documents ([PCIM], [PCIM_EXT] and [PCLS]) are a prerequisite for reading
and understanding this document."

</mircea> 

  - page 7 - you state: "The LDAP object classes defined in this
document 
    are a direct mapping from the corresponding classes and, in some 
    cases, the associations defined in [PCIM_EXT] ". Not strictly true, 
    as you are also seeking to update RFC 3703 (e.g., where is 
    pcimSubtreesPtrAuxClass defined in RFC 3460?). 
<mircea>The text in section 4.1 will be revised for a better description
of the mapping techniques utilised by PCELS. However, I do not
understand 

your reference to pcimSubtreesPtrAuxClass. That class is not defined in
PCELS. 
<mircea>  

<js> If you look at RFC3703, we defined two aux classes
(pcimElementAuxClass and pcimSubtreesPtrAuxClass) to simplify navigation
through the DIT, as well as retrieval of entries found more efficient. I
think that you should take another look at the rationale behind these
classes, and consider again whether they should be included in this
draft. </js>

  - pages 8-11: your table has no caption 
<mircea>Fixed. 
</mircea> 

  - pages 8-11: Where are classes like pcimSubtreesPtrAuxClass? They 
    aren't listed in this table, and better be, if you are "updating" 
    RFC 3703. 
<mircea>The two tables list PCIM_EXT classes mapped by PCELS. Why should
the tables include PCLS classes? 
</mircea> 

<js> Because this draft is supposed to be updating RFC3703, which means
that you need to deal with classes defined in that RFC (such as
pcimSubtreesPtrAuxClass) as well as your own classes. Or, at the very
least, state why these classes do not need to be defined. </js>  

  - once again, I see lots of irksome naming issues. The LDAP schema 
    shouldn't change the name of a class defined in another RFC. Why 
    have you done this? 
<mircea>All are going to be renamed to follow the *exact* PCIM_EXT
names, but I fail to see where is the problem with the old names.

</mircea>  

<js> It's all about implementation ease. If an earlier RFC exists, the
naming in that RFC should be respected and not changed. </js> 

  - page 8, 4th row. How can you give two different mappings to a single

    object class? And how can a RULE (i.e., pcelsRule) map to a GROUP? 
<mircea>pcelsGroup will be added in the new revision. 
</mircea> 

  - page 10, 1st row. How can a single info model association map to 
    two different associations? And do you mean "and" in this row? This 
    would mean that I would have to instantiate both pcelsPolicySet 
    and pcelsPolicySetAssociation, which is clearly wrong. This comment 
    also applies for the other rows on this page where you have "and". 
<mircea> ...means that the PolicySetComponent aggregation is realised by
a pcelsPolicySetComponentList value in the aggregating pcelsPolicySet.
This attribute value is a DN reference to a pcelsPolicySetAsociation
entry. The pcelsPolicySetAsociation entry includes a pcelsPolicySetDN
attribute value that is a reference to the aggregated pcelsPolicySet.
The details are in section 5. The table only gives an overview of the
mapping.

</mircea>  

<js> OK, that makes sense, but I suggest you add a note saying "See
section 5.x" so the impatient reader won't get frustrated. ;-) </js> 

  - page 10 - it is of no help to say "see PolicySetInSystem" in this 
    table for the 3rd and 4th rows - that only confuses the reader. 
    Please spell out what you mean here. 
<mircea>Fixed. Details are in section 5. 
</mircea> 

  - Page 11 - the reader will wonder why ReusablePolicy and 
    PolicyRoleCollectionInSystem are only implementable via DIT 
    containment, when every other association has an association defined

    (independent of whether DIT containment could be used). 
<mircea>I fail to see the issue. 
</mircea>  

<js> Good schemata are consistent. Why are these two associations only
implementable via DIT containment? </js> 

  - Section 4.2, line 3, you write: "The concept of an ordered set of 
    policies...". LDAP doesn't have ordered sets. How are you going to 
    implement this? 
<mircea>replaced "ordered" with "coherent" (from PCIM_EXT) 
</mircea>  

<js> That's certainly better than incoherent ;-) but I don't see how
this solves the problem, as the two aren't synonymous. </js> 

  - Page 13, Section 4.3, second paragraph - s/deprecates/deprecate 
<mircea>Fixed. 
</mircea> 

  - Page 13, Note - actually, PCLS does NOT have anything to do with 
    PCIMe, so this note needs to be reworded 
<mircea>Fixed. 
</mircea> 

  - Page 14, Section 4.5 
    - s/an other/another (and other places) 
    - s/"rule /group"/"rule/group" (5 places) (and other places) 
<mircea>Fixed. 
</mircea> 

  - Page 16, section 4.6, 
    - s/CompoundPolicyActionclasses/CompoundPolicyAction classes 
    - conditions /actions/"conditions/actions" (and other places) 
<mircea>Fixed. 
</mircea> 

  - Page 22. How are you going to enforce an "ordered set of 
    rules /groups"? That is, how can you guarantee that the DSA stores 
    your rules/groups [sic] in the order that you want, and where is 
    that order specified? What if a DSA doesn't have ordering controls? 
  - Same section as above - you say that the "association entries enable

    relative ordering of the aggregated pcelsPolicySet instances within 
    the scope of the aggregating pcelsPolicySet" - how is this 
    accomplished with a plain, vanilla LDAP server with no controls? 
<mircea>Text revised and note added to indicate that applications must
not expect the LDAP data store to implement sorting and ordering.

</mircea> 

  - Page 23 - the DESC for pcelsPolicySetList should say that it 
    contains an UNORDERED list of DN references. 
<mircea>Fixed. 
</mircea> 

  - Page 23, note above Section 5.2, is slightly incorrect. Only those 
    implementations that WANT TO BE COMPATIBLE WITH PCELS should use 
    this aggregation mechanism instead of those defined by PCLS. Not 
    every implementation mechanism is going to want to change. 
<mircea>Revised. The section defining pcelsRule for example will include
the following compatibility note: 

   "Note 2: PCELS implementations SHOULD support pcelsRule and its two 
   subclasses and MAY also support pcimRule and its two subclasses 
   [PCLS]. Applications that choose to support pcelsRule and its two 
   subclasses MUST use the aggregation mechanism provided by 
   pcelsPolicySetAssociation for aggregating policy groups or policy 
   rules in policy rules represented as instances of pcelsRule. 
   Applications that intend to be compatible with [PCIM_EXT] MUST 
   support pcelsRule and its two subclasses." 

</mircea>  

<js> The last MUST contradicts the first SHOULD in the above statement;
please change it to SHOULD. The first MUSt in the above statement is OK.
</js> 

  - Page 23, Section 5.2, says "The pcelsPolicySetAssociation class is 
    used to aggregate instances of pcelsPolicySet into other entries." 
    This is incorrect, as pcelsPolicySet is abstract and thus cannot be 
    instantiated. 
<mircea> I fail to see a problem with "instance of <abstract_class>". It
is obvious that it means "instance of non-abstract subclass of
<abstract_class>". The "non-abstract subclass of" is superfluous and has
been omitted in order to improve the text readabilitiy. PCLS, for
instance, uses such expressions on several occasions. E.g.: (PCLS page
50 first paragraph) "instances of pcimRules". Note that "pcimRules" is
not a class name.

</mircea>  

<js> I still disagree (and with PCLS page 50 as well) - it is imprecise.
</js> 

  - Same section, you write: "...realizes a (subclass of) 
    PolicySetComponent aggregation [sic]. When subordinated to (subclass

    of) dlm1System...realizes a PolicySetInSystem association [sic]". 
    How can the same element realize an aggregation in one usage and an 
    association in another usage? This is semantically inconsistent. 
<mircea>I fail to see the issue. The semantics of
pcelsPolicySetAssociation are context sensitive. 
</mircea>  

<js> The point is that there is a pronounced difference between an
aggregation and an association. Although it is sadly commonplace to call
everything an association. ;-( I would suggest changing your text to
either only use "association" or only use "aggregation" in the same
paragraph. </js> 

  - Next paragraph says: "A non-reusable instance of (subclass of) 
    pcelsPolicySet is attached as auxiliary class directly to the 
    pcelsPolicySetAssociation entry." Subclasses of pcelsPolicySet that 
    are not abstract are pcelsRuleAuxClass and pcelsRuleInstance. The 
    above sentence only makes sense for pcelsRuleAuxClass. 
<mircea>The new specification will include pcelsGroup as well. As
result, the current text will make more sense. 
</mircea> 

  - Next paragraph doesn't make sense. First, you clearly mean a non- 
    abstract subclass of pcelsPolicySet. Second, you are recommending 
    that an ERROR be ignored? Why don't you stop operation? 
<mircea>Revised text: 

   "When reading a pcelsPolicySetAssociation instance that has a 
   pcelsPolicySet attached, the attribute pcelsPolicySetDN MUST 
   be ignored. Applications SHOULD remove the pcelsPolicySetDN value 
   from a pcelsPolicySetAssociation upon attachment of a pcelsPolicySet 
   to the entry." 

This gives applications some flexibility. 
</mircea>  

<js> That's much better </js> 

  - Page 24, DESC of pcelsPriority is insufficient, as "0" has special 
    semantics that you haven't mentioned. This should, of course, also 
    be present in accompanying prose, as Kurt points out. 
<mircea>The PCIM_EXT property and the attribute value restrictions going
to be described in more detail (in prose). However I fail to find the
meaning of "0" in PCIM_EXT. Can you help me locate the text?

</mircea>  

<js> My mistake, the RFC states that this is simply the default value.
</js> 

  - Page 24, DESC of pcelsPolicySetDN should state that this is an 
    UNORDERED list of DNs. 
<mircea>Fixed. 
</mircea> 

  - Page 24, Section 5.3, s/The Three Classes pcelsRule/The pcelsRule 
    Class and Its Subclasses 
<mircea>Fixed. 
</mircea> 

<note: at this point I'm not going to correct any remaining grammar 
 errors, such as the next line ("The pcelsRule is...") because there 
 are too many of them.> 
  - Page 24, Section 5.3, you say: "The pcelsRule is the base class 
    representing policy rules." Does this mean that an implementation 
    can NOT use the subclasses of pcimRule anymore? 
<mircea>I fail to see the issue. 
</mircea> 

  - Page 24, next paragraph, you say: "This class shares the 
    Condition/Action aggregation methods with the 
    pcelsCompoundConditionAuxClass and pcelsCompoundActionAuxClass 
    object classes.". Why does it also not share the 
    pcelsSimpleConditionAuxClass and pcelsSimpleActionAuxClass 
    object classes as well? 
<mircea>Revised text. It was actually trying to say that: 

"   Like pcelsRule, instances of pcelsCompoundConditionAuxClass use 
   pcelsConditionList values and subordinated pcelsConditionAssociation 
   entries to aggregate policy conditions." 
and 
"   Like pcelsRule, instances of pcelsCompoundActionAuxClass use 
   pcelsActionList values and subordinated pcelsActionAssociation 
   entries to aggregate policy actions." 
</mircea> 

  - Page 25, top paragraph, again says that the implementer should
ignore 
    an error condition. This isn't a good idea. 
  - Page 25, next paragraph has the same problem. 
<mircea>Already discussed 
</mircea> 

  - Page 26, the pcelsConditionListType attribute has a constraint. No 
    text is provided that instructs the implementer what to do, aside 
    from Note 5 on page 21, which says: "Text has been added to instruct

    servers and applications what to do if a value outside of this range

    is encountered" - which is exactly the problem - no text is here. 
Note that this is a systemic problem with any constrained attribute 
defined in this draft. Thus, I will only mention this once. 
<mircea>All Fixed. 
</mircea> 

  - Page 27, WHY isn't a PolicyGroup class implemented? You give no 
    reason for not doing this. Note also that Note 2 talks about ORDERED

    policy rules - I don't see how you can construct those. 
<mircea>Already discussed 
</mircea> 

  - Page 27, section 5.4, again you say "pcelsRule" instead of "non- 
    abstract subclasses of pcelsRule". 
<mircea>Already discussed 
</mircea> 

  - Page 28, top paragraph, another error that you are recommending 
    should be ignored 
<mircea>Already discussed 
</mircea> 

  - Page 28, DESC for pcelsConditionAssociation is wrong; you say that 
    it can be used for a pcelsRule instead of a non-abstract subclass of

    pcelsRule 
<mircea>Already discussed 
</mircea> 

  - Page 28, section 5.5, again you say "pcelsRule" instead of "non- 
    abstract subclasses of pcelsRule". 
<mircea>Already discussed 
</mircea> 

   - Page 28, last paragraph, another error that you are recommending 
    should be ignored. 
<mircea>Already discussed 
</mircea> 

   - Page 29, DESC for pcelsActionAssociation is wrong; you say that 
    it can be used for a pcelsRule instead of a non-abstract subclass of

    pcelsRule 
<mircea>Already discussed 
</mircea> 

   - Page 29, last paragraph above Section 5.6, another error that you
are 
    recommending should be ignored. 
<mircea>Already discussed 
</mircea> 

   - Page 29, Section 5.6, last two paragraphs are errors that you are 
    recommending should be ignored. 
<At this point, I'm going to stop listing these, as it is a systemic
problem that should be fixed in the next release> 
<mircea>Already discussed 
</mircea> 

   - Page 29, last paragraph above Section 5.6, another error that you
are 
    recommending should be ignored. 
<mircea>Already discussed 
</mircea> 

   - Page 30, the DESC for pcelsVariableDN is wrong. You say that it is
a 
    "DN reference to a pcelsVariable entry", when it should be a DN 
    reference to a subclass of either pcelsExplicitVariableAuxClass or 
    pcelsImplicitVariableAuxClass or pcelsVendorVariableAuxClass 
<mircea>Already discussed 
</mircea> 

   - Page 30, the DESC for pcelsValueDN is wrong - it should be a
subclass 
    of pcelsValueDN. 
<mircea>Already discussed 
</mircea> 



regards, 
John