Re: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

[Cheng Li <[e-mail]>]

Hi Mike,

Thanks for the quick reply. I think my comments are almost addressed. Please see my reply inline.

Thanks,
Cheng


From: Mike Koldychev (mkoldych) <mkoldych=[e-mail]>
Sent: Tuesday, January 16, 2024 6:05 PM
To: Cheng Li <[e-mail]>; Dhruv Dhody <[e-mail]>; [e-mail]
Cc: pce-chairs <[e-mail]>; [e-mail]; [e-mail]
Subject: RE: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi Cheng,

Thanks for your review! Comments inline with <MK></MK>.

Thanks,
Mike.


From: Cheng Li <c.l=[e-mail]<mailto:c.l=[e-mail]>>
Sent: Tuesday, January 16, 2024 11:09 AM
To: Dhruv Dhody <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
Cc: pce-chairs <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
Subject: RE: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi WG,

I read the document and support the WGLC.
However, I also have some minor comments below.


1.
Abstract

A Segment Routing (SR) Policy [RFC9256<https://www.rfc-editor.org/info/rfc9256>] is a non-empty set of SR Candidate Paths, that share the same <headend, color, endpoint> tuple.
1.Nits: s/that/which.
<MK>
Sure.
</MK>

2.share the same <> tuple?
<MK>
Sorry, not clear what your comment is here? The text is referring to the 3-tuple that identifies the SR Policy, from here: https://www.rfc-editor.org/rfc/rfc9256.html#name-identification-of-an-sr-pol
</MK>
<C>well， I see your point. But recommend to rephrase the text. But this is not a big deal, you can ignore it.

3.This document extends [RFC8664<https://www.rfc-editor.org/info/rfc8664>] to fully support the SR Policy construct.
fully? suggest to remove this world. The SR policy is developing, so it can not be fully supported now I guess.
<MK>
Sure, I will remove the “fully”.
</MK>


4.
Introduction

PCEP Extensions for Segment Routing [RFC8664<https://www.rfc-editor.org/info/rfc8664>] specifies extensions that allow PCEP to work with basic SR-TE paths.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-1-1>
s/specifies/specify
<MK>
Sure, I can just use the RFC Number, instead of the full name.
</MK>
<C>it works to me.

PCEP Extensions for Establishing Relationships Between Sets of LSPs [RFC8697<https://www.rfc-editor.org/info/rfc8697>] introduces
s/introduces/introduce because of extensions?  or you only wanto to list the name of the RFC here? anyway, all are nits. Normally, we use RFCXXXX as the subject directly.
<MK>
Sure, I can just use the RFC Number, instead of the full name.
</MK>
<C>great

5. again. Suggest to delete 'fully' in the last paragraph of Introduction.
<MK>
Will do.
</MK>

6.
SR Policy Association. PCEP ASSOCATION that describes the SR Policy. Can refer to the PCEP object or to the group of LSPs that belong to the Association. This should be clear from the context.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-2-2.4>

suggest to rephrase this description to be more formal? , too casual to me.
<MK>
Sure, will rephrase. There was a prior comment about this as well.
</MK>

7.When these rules are not satisfied, the PCE MUST send a PCErr message with Error-Type = 26 "Association Error", Error Value = TBD8

Only the PCE sends? do we have any case that a PCC may send this error?
<MK>
True, it should be “PCEP speaker”, not “PCE”. Thanks.
</MK>
<C>nice
8.

This Association Type is dynamic in nature, thus operator-configured Association Range MUST NOT be set for this Association type and MUST be ignored.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-4-2>

Sorry I do not understand this paragraph. What do you mean this association type is dynamic in nature?
<MK>
It’s referring to this: https://www.rfc-editor.org/rfc/rfc8697.html#section-3.4 . The text is basically saying that this Association Type is fully dynamic. I’m not sure if this is necessary to say, or if we can rephase it to be clearer? Any suggestions?
</MK>
<C> All right, I see your point. If you decided to use 6, then the text works to me. But I will suggest to add a reference to explain that, and also, it is not dynamic in nature. Some of them can be configured so it is dynamic. We chosen 6, so it is not dynamic anymore, isn’t it?
9.
·        Association ID (16-bit): set to "1".¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-4.1-3.3>
why the ID must be set as 1? do you mean a association is identified by association source, color, and endpoint in extended association ID TLV? so we do not need Association ID?
or you like to use This ID 1 to avoid the race case between multiple PCE?
<MK>
Yes, the association is identified by <Source, Color, Endpoint>, so this 16-bit field is not useful. We set it to “1” to avoid using “0”, which is a reserved value for that field. I can put a sentence to clarify this in the text.
</MK>
<C>All right

10.

   0                   1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |             Type              |             Length            |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |                                                               |
  ~                       SR Policy Name                          ~
  |                                                               |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Figure 2<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#figure-2>: The SRPOLICY-POL-NAME TLV format<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#name-the-srpolicy-pol-name-tlv-f>

Though I can not image what may be added to this TLV now. But I think reserving 0 bits for a new TLV is not a good decision.
same for "SRPOLICY-CPATH-NAME" TLV
<MK>
It’s not reserving 0 bits. If you read the description of the TLV, it encodes a policy name string.
</MK>
<C>I see. Even for encoding a name string, it may reserve some bits for future extension? But this is not vital now. I can accept it now, though from a general protocol design angle, I prefer to reserve some bits.


11. section 8

This document defines one new type for association, which do not add any new security concerns beyond those discussed
s/association/ASSOCIATION Object
s/do/does
<MK>
Ack, thanks.
</MK>

Thanks,
Cheng



From: Pce <[e-mail]<mailto:[e-mail]>> On Behalf Of Dhruv Dhody
Sent: Monday, January 8, 2024 11:29 AM
To: [e-mail]<mailto:[e-mail]>
Cc: pce-chairs <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
Subject: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi WG,

This email starts a 2-weeks working group last call for draft-ietf-pce-segment-routing-policy-cp-12.

https://datatracker.ietf.org/doc/draft-ietf-pce-segment-routing-policy-cp/

Please indicate your support or concern for this draft. If you are opposed to the progression of the draft to RFC, please articulate your concern. If you support it, please indicate that you have read the latest version and it is ready for publication in your opinion. As always, review comments and nits are most welcome.

The WG LC will end on Monday 22nd January 2024.

A general reminder to the WG to be more vocal during the last-call/adoption.

Thanks,
Dhruv & Julien