Re: [spring] Roman Danyliw's Discuss on draft-ietf-spring-srv6-network-programming-19: (with DISCUSS and COMMENT)

[Roman Danyliw <[e-mail]>]

Hi Pablo!

Thanks for the updates in -20.  A few more comments inline ...

> -----Original Message-----
> From: Pablo Camarillo (pcamaril) <[e-mail]>
> Sent: Tuesday, September 22, 2020 3:59 PM
> To: Roman Danyliw <[e-mail]>; The IESG <[e-mail]>
> Cc: [e-mail]; spring-
> [e-mail]; [e-mail]; Bruno Decraene
> <[e-mail]>; Joel Halpern <[e-mail]>
> Subject: RE: Roman Danyliw's Discuss on draft-ietf-spring-srv6-network-
> programming-19: (with DISCUSS and COMMENT)
> 
> Hi Roman,
> 
> Many thanks for your thorough review. Please see inline with [PC].
> 
> Thanks,
> Pablo.
> 
> -----Original Message-----
> From: Roman Danyliw via Datatracker <[e-mail]>
> Sent: lunes, 21 de septiembre de 2020 20:24
> To: The IESG <[e-mail]>
> Cc: [e-mail]; spring-
> [e-mail]; [e-mail]; Bruno Decraene
> <[e-mail]>; Joel Halpern <[e-mail]>;
> [e-mail]
> Subject: Roman Danyliw's Discuss on draft-ietf-spring-srv6-network-
> programming-19: (with DISCUSS and COMMENT)
> 
> Roman Danyliw has entered the following ballot position for
> draft-ietf-spring-srv6-network-programming-19: Discuss
> 
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this introductory
> paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-spring-srv6-network-programming/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Section 3.1.  (In case I missed it, please provide the obvious reference) Per “In
> such a case, the semantics and format of the ARG bits are defined as part of the
> SRv6 endpoint behavior specification”, is “endpoint behavior specification”
> Section 4 or another document?  If the former, I don’t see any references to
> argument bits in the pseudo-code of the Section 4.* subsections.
> If the latter, what document?  Can the behaviors be polymorphic (i.e., same
> network behavior accepting different arguments)?
> 
> [PC] The End.DT2M behavior in section 4.12 has an argument called “Arg.FE2”.
> The behavior specification explains the semantics of the argument and the
> pseudocode describes how it is handled. The arguments to the function can
> indeed vary.

Thanks for pointing this out.  What I still don't understand is whether End.DT2M the only behavior that uses the arguments?

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Thanks for responding to the SECDIR review (and thank you to Brian Weis for
> doing it)
> 
> ** Section 3.2.  Per “An SR Source Node cannot infer the behavior by
> examination of the FUNCT value of a SID”, isn’t there at least some hint
> provided by the use of the FUNCT from the IANA registry?
> [PC] Recall that Section 3.1 says the following (there is no hint):
>    The FUNCT is an opaque identification of a local behavior bound to
>    the SID.
>    The term "function" refers to the bit-string in the SRv6 SID.  The
>    term "behavior" identifies the behavior bound to the SID.  The
>    behaviors are defined in Section 4 of this document.
> 
> [PC] The FUNCT is part of the SID value assigned by the operator. This SID is
> associated with a behavior (defined in the IANA registry). The control plane is
> going to advertise the association of the SID (including the FUNCT) and the
> particular behavior (using the IANA codepoint).
> An intermediate node that only has access to the SRv6 user traffic, only by
> looking at the FUNCT value (and with no control plane information), cannot
> know what the behavior is associated to such SID. Looking ahead at your next
> comment, I believe the confusion might arise from the values used in the
> example in this section.
> 
> ** Section 3.2.
> Per “Function 11 associated with the behavior End.X …” AND
> 
> Per “Function 12 associated with the behavior End.X …”
> 
> The initial registrations for Endpoint Behavior per Section 10.2.1 suggest that
> Function 11 and 12 are End.T.
> [PC] Please see response to previous comment. Indeed, the example can be
> improved.  I’ll replace 11 and 12 for 100 and 101 that are unassigned on the
> IANA registry to more clearly differentiate between FUNCT and Behavior.

Understood.

> ** Section 4.1.  Pseudo-code without formal definition might be open to
> interpretation.  My nits (which likely apply to other sections) are that: -- it
> might not be clear that S03, S06 and S10 are effectively returns/exits from this
> “behavior”, and if they are run, S12-S15 should never be reached (otherwise
> Segments Left or Hop Limit = -1) [PC] Indeed, it would be clearer if we call out
> where there is a return/exit.
> <OLD>
>    S03.      Proceed to process the next header in the packet,
>                 whose type is identified by the Next Header field in
>                 the routing header.
> </OLD>
> <NEW>
>    S03.      Stop processing the SRH, proceed to process the next
>                 header in the packet, whose type is identified by the
>                 Next Header field in the routing header.
> </NEW>
> 
> [PC] For lines S06 and S10 currently include “Interrupt packet processing and
> discard the packet”. Do you think this is sufficient to indicate there is no more
> SRH processing done?

I'd recommend using consistent language.  Since you're using "stop" in S03, I'd recommend using the same thing in S06 and S10

> -- (for consistency) the IF … ELSEIF … END construct is used in Section 4.8, but
> not here.
> [PC] Indeed, pseudocodes should be consistent.
> 
> -- What does it mean to indent the code?  For example S06 is:
> S06.      Send an ICMP Time Exceeded message to the Source Address,
>                 Code 0 (Hop limit exceeded in transit),
>                 Interrupt packet processing and discard the packet.
> 
> I can see why “Code 0 …” is indented under “Send an … “ as this is a line wrap,
> but why is “Interrupt packet processing?”
> [PC] Yes. The indentation of interrupt packet processing is incorrect. It should
> be part of the same statement as above (the period should not be there). I’ll
> review the indentation of all pseudocodes.
>
>
> ** A few other psuedo code nits:
> -- Section 4.1.  S05 uses “IPv6 Hop Limit” but S12 uses “Hop Limit”.
> Recommend consistency.
> [PC] Ack. Fixed for next next rev.
>
>
> -- Section 4.4.  Per S05.  Is “next header” purposely lower case?  I asked because
> it looks like explicit field names are in upper case (e.g., S03 says “Segments Left
> field”).
> [PC] Ack. Fixed for next next rev.
> 
> -- Section 4.16.3.  As a style/consistency note, the other sections which check
> the upper header type seem use of a construct of != 41 or 4 to “Process per
> Section 4.1.1”. (e.g., Section 4.4, 4.5, 4.6, etc.) [PC] Ack. Fixed for next next rev.
> 
> ** Section 8.  Given how meticulously the Section 4 guidance was described, it
> would be helpful to say that the HMAC TLV processing, if present, happens as a
> notional Step “S0” per guidance in Section 2.1.2.1 of RFC8754.
> [PC] Ack. I would propose the following addition at the end of the security
> considerations.
> <NEW>
> When enabled by local configuration, HMAC processing occurs at the beginning
> of SRH processing as defined in Sec 2.1.2.1 of RFC8754.
> </NEW>
> 
> ** Editorial Nits:
> -- Section 3.2. Typo. s/an network/a network/
> -- Section 3.2. Typo. s/is minimum/is minimal/ [PC] Acking both. Fixed for next
> next rev.

Thanks for all of the changes described above.

Regards,
Roman