Re: [mpls] Review of draft-ietf-mpls-ldp-yang-09

[Renato Westphal <[e-mail]>]

Thank you for taking the time to answer my questions Rajiv. Please see my
comments inline.

Em qui., 20 de jan. de 2022 às 20:09, Rajiv Asati (rajiva) <[e-mail]>
escreveu:

> Hi Renato,
>
>
>
> Thanks for the feedback to the WG document. This really helps us to build
> a solid spec.
>
>
>
> 1. You are right about adjacency configuration (RW) and adjacency state
> (RO). The spec well covers all of those.
>
>
>
> The flag you referred to is defined as ReadOnly (RO) for operational
> state, exactly as you pointed out, to capture the neighbor being active or
> passive. Not for config. Hello-adjacencies below is a container, btw, that
> includes both tx and rx adjacencies.
>
>
>
>           |  |     |  |  +--ro hello-adjacencies
>
>           |  |     |  |  |  +--ro hello-adjacency* [adjacent-address]
>
>
>
>           |        |  |     +--ro flag*               identityref
>
> < >
>
>
>
>          container hello-adjacencies {
>
>                …
>
>              uses ldp:adjacency-state-attributes;
>
>
>
> Config (RW) for targeted neighbor covers what you suggested well -
>
>           |  +--rw targeted
>
>           |     +--rw hello-holdtime?     uint16
>
>           |     +--rw hello-interval?     uint16
>
>           |     +--rw hello-accept
>
>           |     |  +--rw enabled?                 boolean
>
>           |     |  +--rw ldp-ext:neighbor-list?   neighbor-list-ref
>

Ack. If I understood correctly, an active adjacency means that hello
messages are both being sent and received, whereas a passive adjacency is
only receiving but not sending hellos (can only happen for targeted
adjacencies).

While a find the adjacency-flag descriptions a bit confusing, the
distinction between active and passive adjacencies makes sense.


> 2.  Targeted neighbor is not bound to an interface (per RFC5036), so that
> interface leaf you referred to is actually not part of targeted discovery
> container, rather basic discovery container (which indeed allows for one or
> more interfaces per neighbor).
>
>
>
>          container hello-adjacencies {
>
>            config false;
>
>            description
>
>              "Containing a list of Hello adjacencies.";
>
>                                 . . . . . .
>
>              leaf interface {
>
>                type if:interface-ref;
>
>                description "Interface for this adjacency.";
>

That leaf lies indirectly under the peer list:
/ietf-routing:routing/control-plane-protocols/control-plane-protocol/ietf-mpls-ldp:mpls-ldp/peers/peer/address-families/ipv4/hello-adjacencies/hello-adjacency/interface

That means it's present for both basic and extended adjacencies as far as I
understand it.


> 3. You are right that a neighbor can be discovered via both basic and
> extended discovery. What you point out is well covered in the tree in two
> distinct places – one for basic and the other for extended/targeted. Pls
> see section 6.1 and figure 5.
>
>
>
>          +--rw discovery
>
>             +--rw interfaces
>
>             |  +--rw interface* [interface]
>
>             |     +--rw address-families
>
>             |        +--rw ipv4
>
>             |           +--ro hello-adjacencies
>
>             |              +--ro hello-adjacencies* [adjacent-address]
>
>             |                 +--ro adjacent-address
>
>             |                    . . . .
>
>             |                    . . . .
>
>             +--rw targeted
>
>                +--rw address-families
>
>                   +--rw ipv4
>
>                      +--ro hello-adjacencies
>
>                         +--ro hello-adjacencies*
>
>                         |                  [local-address adjacent-address]
>
>                         +--ro local-address
>
>                            +--ro adjacent-address
>
>                               . . . .
>
>                               . . . .
>

The problem is that there's also a hello-adjacency list within the peer
list. There interface and targeted adjacencies are displayed together, and
their keys might collide and overlap one another.


> Also, page#52 clarifies the following for local-address -
>
>
>
>                    leaf local-address {
>
>                      type inet:ipv4-address;
>
>                      description
>
>                        "The local address used as the source address to
>
>                         send targeted Hello messages.
>
>                         If the value is not specified, the
>
>                         transport-address is used as the source
>
>                         address.";
>

That's the description of the local-address configuration leaf for targeted
neighbors. In my previous comment, I was referring to the local-address
state leaf from lines 1167 and 1276.

Upon further thought, I think those leafs descriptions are actually fine.
It should only make sense to display the IP source address as the transport
address would be the same for all adjacencies.


> 4. Good point about a good “deployment” practice for different protocols
> to leverage the same router-id. However, LDP protocol (RFC5036) specifies
> LSR-ID and allows it to be different, if/as needed (independent of that,
> one can choose different LSR ID for different VRFs on a node).
>
>
>
> Thankfully, the team agreed to keep the default along the line of what you
> pointed.
>
>
>
Ack, thanks for the clarification.

The fact that RFC5036 doesn't mandate the LSR-ID to use the Router-ID (for
the first four bytes) is a good argument to preserve the lsr-id leaf.
Regarding your second point, I think one should be able to use YANG schema
mount to set different ietf-routing's Router-IDs for different VRFs. If it
wasn't for that, all IGP models would need to have a router-id leaf as well
(which they don't).


> 5. Well, think of IPv6-only node, which unfortunately needs to have IPv4
> for OOB management access. That would result in LDP IPv4 AFI being enabled
> by default, and hence, would need to be explicitly disabled via this
> container.
>
>
>
> I will let my co-authors add further clarification.
>
>
I think that makes sense for the ipv4 presence container within
"global/address-families". For LDP neighbors, I can see no use for an
"ipv4" presence container (instead of a regular container).


> 6. Yes, it does apply.
>
> And  that configuration will allow for the targeted adjacency to be
> created for 1.1.1.1, but peering relationship will be denied.  If the
> intent is to not let the targeted adjacency be created, then policy could
> be created to deny 1.1.1.1.
>

Ack. Thanks for the clarification!


> Wrt custom hello/hold timers per targeted neighbors, AFAIK, RFC5036
> doesn’t specify that, nor do any of the implementations.
>
> Only custom hello/hold timers global config.
>

Ack.


> 7. AFAIK, this is because of RFC5036 that specifies 5s to be the lowest
> hello interval.  😊
>
>
>
> https://datatracker.ietf.org/doc/html/rfc5036#section-3.5.2
>

I couldn't find in that RFC section any reference to a minimum allowed
hello interval. Could you precise which excerpt you're using as a reference?

For what it's worth, I've seen numerous vendors supporting smaller hello
interval and holdtime values.

Also, maybe it would be a good idea to support infinite hold-time values
(0xffff) as they are specified by RFC 5036. It doesn't seem like they are
widely implemented though.


> 8. Thanks for reviewing so diligently and pointing these typos out.
>
> Thankfully, our amazing RFC Editor team has fixed all of them (I just
> checked their version) except interface’s counters (within the statistics
> container), which I believe is correct (since it is about the node, not
> about the peer).
>

I was referring to the statistics container within the peer-state-derived
grouping [1]. There I think "interface’s counters" was a copy and paste
error.

I'm happy to know the other typos were already fixed :)

[1] Full path to be precise:
/ietf-routing:routing/control-plane-protocols/control-plane-protocol/ietf-mpls-ldp:mpls-ldp/peers/peer/statistics

Regards,
-- 
Renato Westphal