bgpd: permit valid non-labeled nh for labeled path

[zstas]

Hello folks,

There are some cases where we have labeled paths in BGP, but nexthop is plain ipv4/ipv6. For instance, inter-as opt.B is one of them. This change permit non-labeled (but valid) nexthops for those paths.

[zstas]

Hi Mark, Thanks for the feedback!

IMO there shouldn't be such restriction as marking plain nexthops as inaccessible for labeled paths. Of course, the common case when you learn nexthops with IGP and there're transport labels to reach them. Also, I can't recall any restrictions from RFC regarding labeled paths. As the last argument - in 7.5 it worked (only 8.0+ breaks those setups).

If you're still convinced that is wrong behaviour, how I can narrow the scope? Limit to eBGP peers only or introduce a new knob like neighbor x.x.x.x allow-non-labeled-nh?

[NetDEF-CI]

Continuous Integration Result: FAILED

Test incomplete. See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-2632/

This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Incomplete

Addresssanitizer topotests part 4: Incomplete

(check logs for details)

Successful on other platforms/tests

• Topotests Ubuntu 18.04 i386 part 1
• Topotests Ubuntu 18.04 amd64 part 4
• Topotests Ubuntu 18.04 i386 part 6
• Addresssanitizer topotests part 2
• Topotests Ubuntu 18.04 arm8 part 6
• Topotests Ubuntu 18.04 amd64 part 5
• Topotests Ubuntu 18.04 amd64 part 0
• Topotests Ubuntu 18.04 arm8 part 7
• Topotests debian 10 amd64 part 6
• Topotests Ubuntu 18.04 arm8 part 1
• Addresssanitizer topotests part 9
• Topotests Ubuntu 18.04 amd64 part 7
• Fedora 29 rpm pkg check
• Topotests debian 10 amd64 part 0
• Topotests Ubuntu 18.04 arm8 part 2
• Addresssanitizer topotests part 3
• Topotests debian 10 amd64 part 5
• Topotests Ubuntu 18.04 arm8 part 8
• Topotests Ubuntu 18.04 i386 part 7
• CentOS 7 rpm pkg check
• Topotests Ubuntu 18.04 i386 part 2
• Topotests Ubuntu 18.04 amd64 part 3
• Addresssanitizer topotests part 6
• Topotests debian 10 amd64 part 9
• Topotests debian 10 amd64 part 7
• Topotests Ubuntu 18.04 i386 part 5
• Topotests Ubuntu 18.04 i386 part 0
• Topotests debian 10 amd64 part 8
• Topotests Ubuntu 18.04 amd64 part 2
• Debian 10 deb pkg check
• Topotests Ubuntu 18.04 arm8 part 4
• IPv6 protocols on Ubuntu 18.04
• Topotests debian 10 amd64 part 3
• Topotests Ubuntu 18.04 amd64 part 6
• Topotests Ubuntu 18.04 arm8 part 9
• Topotests Ubuntu 18.04 amd64 part 1
• Addresssanitizer topotests part 1
• Ubuntu 20.04 deb pkg check
• Topotests Ubuntu 18.04 i386 part 4
• Ubuntu 18.04 deb pkg check
• Topotests Ubuntu 18.04 i386 part 9
• IPv4 protocols on Ubuntu 18.04
• Topotests Ubuntu 18.04 i386 part 8
• Debian 9 deb pkg check
• Topotests Ubuntu 18.04 amd64 part 9
• Topotests Ubuntu 18.04 i386 part 3
• Addresssanitizer topotests part 8
• Addresssanitizer topotests part 7
• Topotests debian 10 amd64 part 4
• Topotests Ubuntu 18.04 arm8 part 5
• Topotests Ubuntu 18.04 arm8 part 3
• Addresssanitizer topotests part 5
• Topotests Ubuntu 18.04 amd64 part 8
• Topotests debian 10 amd64 part 1
• Topotests Ubuntu 18.04 arm8 part 0
• Static analyzer (clang)
• IPv4 ldp protocol on Ubuntu 18.04
• Ubuntu 16.04 deb pkg check
• Topotests debian 10 amd64 part 2
• Addresssanitizer topotests part 0

[ton31337]

Could you give an exact example of the configuration you use?

[zstas]

sure, here it is:

router bgp 65012
 neighbor 192.168.61.2 remote-as 65011
 !
 address-family ipv4 vpn
  neighbor 192.168.61.2 activate
  neighbor 192.168.61.2 route-map permit-all in
  neighbor 192.168.61.2 route-map permit-all out
 exit-address-family
exit
!
router bgp 65012 vrf test
 !
 address-family ipv4 unicast
  redistribute connected
  redistribute static
  label vpn export 38
  rd vpn export 121:1
  rt vpn both 121:1
  export vpn
  import vpn
 exit-address-family
exit
!
route-map permit-all permit 10
exit
!
end

on the other side just the same configuration (except ip and as).

there is also some cli output in the mentioned issue.

[ton31337]

It looks like that BGP_NEXTHOP_LABELED_VALID is just stripped on the path, we need to figure out this carefully. I would check bgp_process_nexthop_update().

[zstas]

it looks good to me, the flag is stripped but after checking amount of labels the flag is returned. in my case there aren't any labels for the nh, that's why the nexthop isn't marked as BGP_NEXTHOP_LABELED_VALID.

static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc,
				       struct zapi_route *nhr)
{
...
		bnc->flags &= ~BGP_NEXTHOP_LABELED_VALID; /* check below */

		for (i = 0; i < nhr->nexthop_num; i++) {
			int num_labels = 0;

			nexthop = nexthop_from_zapi_nexthop(&nhr->nexthops[i]);
...
			/* There is at least one label-switched path */
			if (nexthop->nh_label &&
				nexthop->nh_label->num_labels) {

				bnc->flags |= BGP_NEXTHOP_LABELED_VALID;
				num_labels = nexthop->nh_label->num_labels;
			}

[ton31337]

I'm not much familiar with the MPLS, but it looks like we can adopt something like:

if (nexthop->nh_label && (nexthop->nh_label->num_labels || nexthop->nh_label_type == ZEBRA_LSP_NONE)) {
    if (nexthop->nh_label->num_labels)
        num_labels = nexthop->nh_label->num_labels;

    bnc->flags |= BGP_NEXTHOP_LABELED_VALID;
}

(If this is the case, didn't test anything. I'm yet strange why it works when you re-enter RT manually)

[zstas]

Hi @pguibert6WIND,

I think the real use case is something wiser, like the next-hop resolution 
does not see any labels and stops, whereas the next-hop at one 
point in time, gets a label, and next-hop resolution is never called back.
no ?

No, not really. Please, have a look at MPLS VPN Inter-AS opt.B [1]. You can imagine that 2 routers from different ASes connected, and they don't have neither IGP (+LDP/or whatever, as they in different AS), nor BGP-LU (it'd be Inter-AS opt. C). So, let's say that 2 connected routers have implicit-null label to reach each other (and only the service label will be presented in MPLS header).

[1] - https://datatracker.ietf.org/doc/html/rfc4364#page-32

[riw777]

I tend to agree with @mjstapp here ... the solution proposed is too broad. It might be useful to have a short meeting with @mjstapp @riw777 @pguibert6WIND @louberger and @ton31337 at least to work out a good solution

[louberger]

Hi @pguibert6WIND,

I think the real use case is something wiser, like the next-hop resolution 
does not see any labels and stops, whereas the next-hop at one 
point in time, gets a label, and next-hop resolution is never called back.
no ?

No, not really. Please, have a look at MPLS VPN Inter-AS opt.B [1]. You can imagine that 2 routers from different ASes connected, and they don't have neither IGP (+LDP/or whatever, as they in different AS), nor BGP-LU (it'd be Inter-AS opt. C). So, let's say that 2 connected routers have implicit-null label to reach each other (and only the service label will be presented in MPLS header).

[1] - https://datatracker.ietf.org/doc/html/rfc4364#page-32

Hi, looking at the quoted text (option b), the last paragraph on the page and 2 1st paragraphs on the next page have all sorts of conditions on when this usage is permitted. These restrictions don't seem to be in the PR, how are you expecting to meetithese conditions?

[zstas]

Hi @louberger, Yes, but opt.B is just one of the cases. You can also find some other cases when there's no label for a nexthop in the RFC, like:

If the backbone supports MPLS, this is done as follows:
...
         * The packet will have an "IGP Next Hop", which is the next hop
           along the IGP route to the BGP Next Hop.

         * If the BGP Next Hop and the IGP Next Hop are the same, and if
           penultimate hop popping is used, the packet is then sent to
           the IGP Next Hop, carrying only the VPN route label.

(not sure if it's properly handled now, but mainly I'm interested only in inter-as opt.B. I have this case in production and also I want to upgrade from 7.5 to 8.1)

I'm not saying that we should accept all the plain next-hops, as I said before I can limit the scope. I'll update my PR correspondingly when we agree on the decision.

[github-actions[bot]]

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

[pguibert6WIND]

Hi @zstas, I am still convinced the change in this commit is wrong behaviour, but the behaviour should be possible by having a config command that permits it. You were proposing that one: neighbor x.x.x.x allow-non-labeled-nh I would be interested in proposing an alternative too:

mpls bgp

@louberger depicted [1] - https://datatracker.ietf.org/doc/html/rfc4364#page-32, and gave multiple examples where this could be authorized. If we assume EBGP is the main exception, then what about going for an option like the first one you propose?

[zstas]

okay, let me try to implement it. do you mean "mpls bgp" in neighbour scope or in global bgp configuration scope?

[pguibert6WIND]

okay, let me try to implement it. do you mean "mpls bgp" in neighbour scope or in global bgp configuration scope?

okay, let me try to implement it. do you mean "mpls bgp" in neighbour scope or in global bgp configuration scope?

Hi @zstas, sorry for delay, I was away from keyboard. I think neighbor scope would be acceptable : neighbor x.x.x.x allow-non-labeled-nh for instance.

VPN-IPv4 routes should only be
         accepted on EBGP connections at private peering points, as part
         of a trusted arrangement between SPs.  VPN-IPv4 routes should
         neither be distributed to nor accepted from the public
         Internet, or from any BGP peers that are not trusted. 

From the above comment, I think the trusted arrangement could be done by adding the config 'neighbor x.x.x.x allow-non-labeled-nh' which would mean: 'accept bgp updates from that ebgp peer'.

addendum: I try to take an interface driver approach by using that one: https://github.com/FRRouting/frr/pull/11823

[zstas]

Hello @pguibert6WIND, Thank you for the patch.