Deleting the default VNI instead of saying a VNI like this doesn't exist for 'no vni <VNI>' vtysh command

[piotrsuchy]

Description

root@499e3c061293:~# vtysh -c' show run'
Building configuration...

Current configuration:
!
frr version 8.4.2
frr defaults traditional
hostname 499e3c061293
log syslog informational
vni 1
no ipv6 forwarding
service integrated-vtysh-config
!
end
root@499e3c061293:~# vtysh -c 'configure' -c 'no vni 12'
VNI 12 doesn't exist in VRF: default
root@499e3c061293:~# vtysh -c 'show run' | grep vni
vni 1
root@499e3c061293:~#

Version

FRRouting 10.3-dev (682554b49f7a) on Linux(5.15.0-119-generic).
Copyright 1996-2005 Kunihiro Ishiguro, et al.
configured with:
    '--build=x86_64-linux-gnu' '--prefix=/usr' '--includedir=${prefix}/include' '--mandir=${prefix}/share/man' '--infodir=${prefix}/share/info' '--sysconfdir=/etc' '--localstatedir=/var' '--disable-option-checking' '--disable-silent-rules' '--libdir=${prefix}/lib/x86_64-linux-gnu' '--libexecdir=${prefix}/lib/x86_64-linux-gnu' '--disable-maintainer-mode' '--sbindir=/usr/lib/frr' '--with-vtysh-pager=/usr/bin/pager' '--libdir=/usr/lib/x86_64-linux-gnu/frr' '--with-moduledir=/usr/lib/x86_64-linux-gnu/frr/modules' '--disable-dependency-tracking' '--disable-rpki' '--disable-scripting' '--enable-pim6d' '--disable-grpc' '--with-libpam' '--enable-doc' '--enable-doc-html' '--enable-snmp' '--enable-fpm' '--disable-protobuf' '--disable-zeromq' '--enable-ospfapi' '--enable-bgp-vnc' '--enable-multipath=256' '--enable-pcre2posix' '--enable-user=frr' '--enable-group=frr' '--enable-vty-group=frrvty' '--enable-configfile-mask=0640' '--enable-logfile-mask=0640' 'build_alias=x86_64-linux-gnu' 'PYTHON=python3'

How to reproduce

1. Install FRR
2. Add vni (let's say VNI 1) to the config
3. verify that it's there in the config vtysh -c 'show run'| grep vni
4. Run no vni 2 or some other number aside from the one actually there
5. verify that it's no longer in the running config vtysh -c 'show run' | grep vni

Expected behavior

If the VNI doesn't exist it's just returns an error.
Similar to how it looked in FRR-8.4.2:

root@499e3c061293:~# vtysh -c' show run'
Building configuration...

Current configuration:
!
frr version 8.4.2
frr defaults traditional
hostname 499e3c061293
log syslog informational
vni 1
no ipv6 forwarding
service integrated-vtysh-config
!
end
root@499e3c061293:~# vtysh -c 'configure' -c 'no vni 12'
VNI 12 doesn't exist in VRF: default
root@499e3c061293:~# vtysh -c 'show run' | grep vni
vni 1

Actual behavior

It deletes the default VNI instead.

Additional context

Seems to be the result of the frr10 change to the new northbound interface removing this guards here:

https://github.com/FRRouting/frr/pull/15181/files#diff-4a26c99e786bb1ba28c15d6f0330985b4f71bbe6f36ec74215ca6221895827b3L2780-L2856

Which was guarding this kind of behavior before. But it still should be picked up by the lines here:

 	vni = strtoul(argv[2]->arg, NULL, 10);

	/* Check if we should disallow. */
	vpn = bgp_evpn_lookup_vni(bgp, vni);
	if (!vpn) {
		vty_out(vty, "%% Specified VNI does not exist\n");
		return CMD_WARNING;
	}
	if (!is_vni_configured(vpn)) {
		vty_out(vty, "%% Specified VNI is not configured\n");
		return CMD_WARNING;
	}

But it seems it does not for some reason. I will keep on debugging this.

Checklist

• I have searched the open issues for this bug.
• I have not included sensitive information in this report.

[piotrsuchy]

After some more digging - those are the logs we should focus on:

2025/01/30 15:14:51 MGMTD: [TDP2M-R6CWH] TXN: mgmt_txn_create_new: Added new 'CONFIG' txn-id: 8
2025/01/30 15:14:51 MGMTD: [QV0CF-4ET0W] TXN: mgmt_txn_lock: ../mgmtd/mgmt_txn.c:1827 --> Lock CONFIG txn-id: 8 refcnt: 1
2025/01/30 15:14:51 MGMTD: [GWNS4-S94AF] TXN: mgmt_txn_req_alloc: Added a new SETCFG req-id: 3 txn-id: 8, session-id: 10
2025/01/30 15:14:51 MGMTD: [QV0CF-4ET0W] TXN: mgmt_txn_lock: ../mgmtd/mgmt_txn.c:448 --> Lock CONFIG txn-id: 8 refcnt: 2
2025/01/30 15:14:51 MGMTD: [WJNQK-ZCA5S] TXN: mgmt_txn_send_set_config_req: XPath: '/frr-vrf:lib/vrf[name='default']/frr-zebra:zebra/l3vni-id', Value: 'NULL'
2025/01/30 15:14:51 MGMTD: [WJNQK-ZCA5S] TXN: mgmt_txn_send_set_config_req: XPath: '/frr-vrf:lib/vrf[name='default']/frr-zebra:zebra/prefix-only', Value: 'NULL'
2025/01/30 15:14:51 MGMTD: [PYR9H-051KG] TXN: mgmt_txn_process_set_cfg: Processing 1 SET_CONFIG requests txn-id:8 session-id: 10
2025/01/30 15:14:51 MGMTD: [Q5TEJ-V4JJA] TXN: mgmt_txn_req_alloc: Added a new COMMITCFG req-id: 3 txn-id: 8 session-id: 10
2025/01/30 15:14:51 MGMTD: [QV0CF-4ET0W] TXN: mgmt_txn_lock: ../mgmtd/mgmt_txn.c:448 --> Lock CONFIG txn-id: 8 refcnt: 3
2025/01/30 15:14:51 MGMTD: [MZMN7-7BHZT] TXN: mgmt_txn_req_free: Deleting SETCFG req-id: 3 txn-id: 8
2025/01/30 15:14:51 MGMTD: [TRA5R-4HR77] TXN: mgmt_txn_req_free: Removed req-id: 3 from request-list (left:0)
2025/01/30 15:14:51 MGMTD: [NGTCT-QMWE7] TXN: mgmt_txn_unlock: ../mgmtd/mgmt_txn.c:553 --> Unlock CONFIG txn-id: 8 refcnt: 2
2025/01/30 15:14:51 MGMTD: [K2X00-2H682] TXN: mgmt_txn_process_commit_cfg: Processing COMMIT_CONFIG for txn-id: 8 session-id: 10 Phase 'PREP-CFG'
2025/01/30 15:14:51 MGMTD: [W7XQT-PM3RC] nb_config_diff: {
  "frr-vrf:lib": {
    "@": {
      "yang:operation": "none"
    },
    "vrf": [
      {
        "name": "default",
        "frr-zebra:zebra": {
          "l3vni-id": 1,
          "@l3vni-id": {
            "yang:operation": "delete"
          }
        }
      }
    ]
  }
}

2025/01/30 15:14:51 MGMTD: [HQ1C9-SE3Y2] TXN: mgmt_txn_create_config_batches: XPATH: /frr-vrf:lib/vrf[name='default']/frr-zebra:zebra/l3vni-id, Value: '1'
2025/01/30 15:14:51 MGMTD: [JRWMK-2T50D] BE-ADAPTER: mgmt_be_interested_clients: XPATH: '/frr-vrf:lib/vrf[name='default']/frr-zebra:zebra/l3vni-id'

So the vni_mapping_cmd:

DEFPY_YANG (vni_mapping,
       vni_mapping_cmd,
       "[no] vni ![" CMD_VNI_RANGE "[prefix-routes-only$filter]]",
       NO_STR
       "VNI corresponding to tenant VRF\n"
       "VNI-ID\n"
       "prefix-routes-only\n")
{
	if (!no)
		nb_cli_enqueue_change(vty, "./frr-zebra:zebra/l3vni-id", NB_OP_MODIFY,
			      vni_str);
	else
		nb_cli_enqueue_change(vty, "./frr-zebra:zebra/l3vni-id", NB_OP_DESTROY,
			      NULL);

	if (filter)
		nb_cli_enqueue_change(vty, "./frr-zebra:zebra/prefix-only",
				      NB_OP_MODIFY, "true");
	else
		nb_cli_enqueue_change(vty, "./frr-zebra:zebra/prefix-only",
				      NB_OP_DESTROY, NULL);

	if (vty->node == CONFIG_NODE)
		return nb_cli_apply_changes(vty, "/frr-vrf:lib/vrf[name='%s']",
					    VRF_DEFAULT_NAME);

	return nb_cli_apply_changes(vty, NULL);
}

Passes NULL instead of vni_str for the destroy operation. And later on it probably uses this part in lib/northbound.c:

		/* If the value is not set, get the default if it exists. */
		value = change->value;
		if (value == NULL)
			value = yang_snode_get_default(nb_node->snode);
		data = yang_data_new(xpath, value);

To get the default value of l3vni_id, which is '1' for my config that has vni 1 running.

I'm not familiar with NB etc., but I think there should be a guard against something like that, either in the vni_mapping_cmd itself or the vni_str should be passed to NB_OP_DESTROY and later on it should be checked. That was the case earlier on, but was deleted here:

commit from @idryzhov

with the description:

"zebra: fix vni NB conversion

• unnecessary command duplication
• usage of oper data during validation
• unnecessary checks for things that can't happen"

But i think it can happen, right - just like with this very simple reproduction? Or am i missing something? Could you take a look @idryzhov? Thanks in advance!

[idryzhov]

I'd say there's no uniform rule for things like this in FRR - in many cases we don't check the values on "no" commands, but there are cases where we check.

If necessary, the check can be added. It should be done inside the CLI handler, by checking the value of existing field in vty->candidate_config. It's not possible to implement the check inside NB code, because the value is never passed to any callbacks on "destroy" command.