isisd: Fix the calculated metric and backup routes for IPv4 local connected routes.

[zhou-run]

The IPv4 directly connected route prefix exists in both the root LSP and the root's neighbor LSP:

1. When generating vertices for directly connected route prefixes with a metric of 0 based on the root LSP, the isis_spf_preload_tent_ip_reach_cb() function only generates vertices of type VTYPE_IPREACH_INTERNAL without distinguishing between area->oldmetric and area->newmetric.
2. When generating vertices for the directly connected route prefix based on the neighbor LSP, the isis_spf_process_lsp() function will generate vertices of type VTYPE_IPREACH_INTERNAL and VTYPE_IPREACH_TE based on area->oldmetric and area->newmetric, where the vertex metric is the sum of the metric from the root IS to the neighbor IS and from the neighbor IS to the root IS, respectively.

If area->newmetric==1, the same directly connected route prefix will have both VTYPE_IPREACH_INTERNAL vertices with a metric of 0 and VTYPE_IPREACH_TE vertices with a non-zero metric. During route generation, the isis_spf_loop() function will prioritize selecting VTYPE_IPREACH_TE vertices, leading to incorrect metrics for the directly connected routes. Of course, the depth of this VTYPE_IPREACH_TE vertex is always greater than 1, which will cause the spf_vertex_check_is_affected() function to fail the vertex->depth == 1 condition check for this local route vertex. As a result, a backup route will be generated for the local route when LFA or TI-LFA is configured.

Supplement topotest modifications. Directly connected routes with incorrect ISIS cost calculations should not appear in the RIB. Similarly, we should not calculate backup routes for local direct routes.

Signed-off-by: zhou-run [email protected]

[Jafaral]

Please use a short and direct title for the PR, as it is stand, from reading the title only, I can't tell what you are trying to do. IS this what you are trying to say?:

isisd: Fix calculated IPv4 local connected route metric

[zhou-run]

Please use a short and direct title for the PR, as it is stand, from reading the title only, I can't tell what you are trying to do. IS this what you are trying to say?:

isisd: Fix calculated IPv4 local connected route metric

Okay. I have updated the PR title.

[zhou-run]

I previously submitted this PR in #16056, and it passed the basic tests. However, it was reverted in #16160 because the basic tests failed when merging into other branches. Upon reviewing the failed topotests, it was found that the cause was incorrect metric calculations for local direct routes and the generation of backup routes for local direct routes. Now I have fixed these topotests. @ton31337 @riw777 @odd22

[zhou-run]

can you explain why removing the check for these networks is the correct fix

The routes I removed are all IPv4 local direct routes. ISIS should calculate these networks the same way as IPv6 local direct routes, with a metric value of 0 instead of 20. This way, when these routes are passed down to Zebra, they will be compared with the kernel's local direct routes, and the ISIS-generated routes with the same metric value will be removed. Additionally, I believe that IPv4 local direct routes should not have backup routes, so I have removed them as well.

whether this is going to reduce coverage in other protocols?

The IPv4 local connected routes calculated by ISIS will never be selected in the FIB, and these IPv4 local connected routes do not need to have backup routes, so they will not affect the coverage of other protocols.

[donaldsharp]

Kernel routes and isis routes have different AD, and as such isis metrics will never be compared to kernel metrics( as if that even means anything) Talking about metric makes no sense to me. Can you try your explanation again? As that I am now more confused than before

[zhou-run]

Kernel routes and isis routes have different AD, and as such isis metrics will never be compared to kernel metrics( as if that even means anything) Talking about metric makes no sense to me. Can you try your explanation again? As that I am now more confused than before

You're right. The IPv4 local route in ISIS is not compared with the kernel route. Instead, in isis_zebra_route_add_route(), the local route is not sent to Zebra because it lacks a next hop. However, the actual generated IPv4 local route comes from the neighbor and the next hop is the neighbor. Although it is not selected by Zebra, it is still dangerous because if it gets selected, it would cause a routing loop. My modification ensures that the IPv4 local routes will never have a next hop, thus they won't be passed down to zebra and displayed. For instance, consider the deleted route below:

"10.0.1.0\/24":[
   {
     "prefix":"10.0.1.0\/24",
     "protocol":"isis",
     "distance":115,
     "metric":20,
     "nexthops":[
       {
         "ip":"10.0.1.2",
         "afi":"ipv4",
         "interfaceName":"eth-sw1"
       },
       {
         "ip":"10.0.1.3",
         "afi":"ipv4",
         "interfaceName":"eth-sw1"
       }
     ]
   }

[riw777]

Although it is not selected by Zebra, it is still dangerous because if it gets selected, it would cause a routing loop. My modification ensures that the IPv4 local routes will never have a next hop, thus they won't be passed down to zebra and displayed. For instance, consider the deleted route below:

Is there any case where it would be selected? I'm really hesitant to remove this much code to solve a problem that may, in theory, only happen sometimes ...

[zhou-run]

Is there any case where it would be selected? I'm really hesitant to remove this much code to solve a problem that may, in theory, only happen sometimes ...

As the primary route, it's indeed unlikely to be selected unless the kernel does not generate a direct route. However, a direct route can generate a backup route, which allows traffic to reroute back to the local prefix through the backup if the connection fails. Just like the route that was deleted below.

"10.0.7.0\/24":[
  {
    "prefix":"10.0.7.0\/24",
    "protocol":"isis",
    "distance":115,
    "metric":20,
    "nexthops":[
      {
        "ip":"10.0.7.4",
        "afi":"ipv4",
        "interfaceName":"eth-rt4",
        "backupIndex":[
          0
        ]
      }
    ],
    "backupNexthops":[
      {
        "ip":"10.0.8.5",
        "afi":"ipv4",
        "interfaceName":"eth-rt5",
        "active":true
      }
    ]
  }
],

[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.

[riw777]

@frrbot autoclose in 1 month