Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eigrp: multiple Error type: validation #11301

Closed
1 of 2 tasks
c-po opened this issue May 29, 2022 · 6 comments
Closed
1 of 2 tasks

eigrp: multiple Error type: validation #11301

c-po opened this issue May 29, 2022 · 6 comments
Labels
triage Needs further investigation

Comments

@c-po
Copy link
Contributor

c-po commented May 29, 2022

  • Did you check if this is a duplicate issue?
  • Did you test it on the latest FRRouting/frr master branch?

To Reproduce

LR1.wue3(config)# router eigrp 100
LR1.wue3(config-router)# passive-interface eth0
% Configuration failed.

Error type: validation
LR1.wue3(config-router)# passive-interface eth1
% Configuration failed.

Error type: validation
LR1.wue3(config)# interface eth0
LR1.wue3(config-if)# eigrp bandwidth 10
% Configuration failed.

Expected behavior

No config error

Screenshots

Versions

  • OS Version: Debian 11 (VyOS 1.4)
  • Kernel: 5.10.117
  • FRR Version: 8.2.2 commit 56694ea

Additional context

@c-po c-po added the triage Needs further investigation label May 29, 2022
@volodymyrhuti
Copy link
Contributor

I have triaged down this issue:

  1. The error has appeared between frr-7.1 and frr-7.2 when migrating to the northbound
vova | git log -1 f25c244b7
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
commit f25c244b7502f2fa61f143b514eef8f6b9e24355 ┃
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━
Author: Rafael Zalamena <rzalamena@opensourcerouting.org>
Date:   Tue Jul 30 12:44:13 2019 -0300

    eigrpd: migrate old CLI to northbound
    
    Move all configuration commands to the new CLI code (`eigrp_cli.c`),
    implement the northbound and do all the necessary wiring to get it
    working.
    
    Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
  1. The new architecture introduces a validation step that fails
тра 31 02:02:16 Zephyrus eigrpd[935768]: [EC 100663331] nb_callback_configuration: error processing configuration change: error [internal inconsistency] event [validate] operation [create] xpath [/frr-eigrpd:eigrpd/instance[asn='100'][vrf='']/passive-interface[.='eth1']]
тра 31 02:02:16 Zephyrus eigrpd[935768]: [EC 100663337] nb_candidate_commit_prepare: failed to validate candidate configuration

May 31 09:40:32 vyos eigrpd[5758]: [MHYBZ-5A04C][EC 100663334] error processing configuration change: error [internal inconsistency] event [validate] operation [modify] xpath [/frr-interface:lib/interface[name='eth0']/frr-eigrpd:eigrp/bandwidth]
May 31 09:40:32 vyos eigrpd[5758]: [H68KZ-12QEF][EC 100663340] nb_candidate_commit_prepare: failed to validate candidate configuration
frr/eigrpd/eigrpd_northboud.c:
---------------------------------------------------------------------------------------
/*
 * XPath: /frr-eigrpd:eigrpd/instance/passive-interface
 */
static int
eigrpd_instance_passive_interface_create(struct nb_cb_create_args *args)
{
	struct eigrp_interface *eif;
	struct eigrp *eigrp;
	const char *ifname;

	switch (args->event) {
	case NB_EV_VALIDATE:
		eigrp = nb_running_get_entry(args->dnode, NULL, false);
		if (eigrp == NULL) {
			/*
			 * XXX: we can't verify if the interface exists
			 * and is active until EIGRP is up.
			 */
			break;
		}

		ifname = yang_dnode_get_string(args->dnode, NULL);
		eif = eigrp_interface_lookup(eigrp, ifname);       

		#### ERR: failed validation, interface list empty
		if (eif == NULL) 
		        #### ERR: configuration is inconsistent, return error
			return NB_ERR_INCONSISTENCY; 
		break;



---------------------------------------------------------------------------------------
Prior implementation
frr/eigrpd/eigrpd_northboud.c:
---------------------------------------------------------------------------------------
DEFUN (eigrp_passive_interface,
       eigrp_passive_interface_cmd,
       "passive-interface IFNAME",
       "Suppress routing updates on an interface\n"
       "Interface to suppress on\n")
{
	VTY_DECLVAR_CONTEXT(eigrp, eigrp);
	struct eigrp_interface *ei;
	struct listnode *node;
	char *ifname = argv[1]->arg;

	for (ALL_LIST_ELEMENTS_RO(eigrp->eiflist, node, ei)) {
		if (strcmp(ifname, ei->ifp->name) == 0) {
			ei->params.passive_interface = EIGRP_IF_PASSIVE;
			return CMD_SUCCESS;
		}
	}
	return CMD_SUCCESS;                                                                       
}
  1. From the testing, I can see that when executing the provided configuration, the eiflist is empty both for
    pre/post 7.2 versions. Looking at the old logic, nothing is done if the list is empty, but success is still returned.
    eigrp_list_bug

The next step is to look through the interface initialization logic and consider the proper configuration flow.
I will look for a solution once I have a moment (a bit later ;)

@tarunkundu-est
Copy link

Hi @volodymyrhuti - Did you get a chance to look through the interface initialization logic? I am also seeing this "Configuration failed" issue for every eigrp cli command in FRR 7.5.1 release.

@volodymyrhuti
Copy link
Contributor

volodymyrhuti commented Mar 6, 2023

Hi all! Here is a short update on the issue:

  • I have retested on the latest frr build, and the issue is still valid; the steps are following
ZephyrusNG# configure terminal 
ZephyrusNG(config)# router eigrp 1
ZephyrusNG(config-router)# passive-interface 
docker0     enp5s0      lo          mpqemubr0   virbr0      virbr0-nic  wlp4s0      
ZephyrusNG(config-router)# passive-interface wlp4s0 
% Configuration failed.

Error type: validation
  • I have checked the topotest topology: topotests/eigrp_topo1/test_eigrp_topo1.py
    Using passive-interface for an active interface (with converged topology) works without issue
os.environ["PYTHONBREAKPOINT"] = "pudb.set_trace"
def test_converge_protocols():
    "Wait for protocol convergence"

    tgen = get_topogen()
    # Don't run this test if we have any failure.
    if tgen.routers_have_failure():
        pytest.skip(tgen.errors)

    topotest.sleep(5, "Waiting for EIGRP convergence")
    breakpoint()

------------
ZephyrusNG# show interface 
Interface lo is up, line protocol is up
  Link ups:       0    last: (never)
  Link downs:     0    last: (never)
  vrf: default
  index 1 metric 0 mtu 65536 speed 0 
  flags: <UP,LOOPBACK,RUNNING>
  Ignore all v6 routes with linkdown
  Type: Loopback
  inet6 fe80::200:ff:fe00:0/64
  Interface Type Other
  Interface Slave Type None
  protodown: off 
Interface r1-eth0 is up, line protocol is up
  Link ups:       0    last: (never)
  Link downs:     0    last: (never)
  vrf: default
  index 2 metric 0 mtu 1500 speed 10000 
  flags: <UP,BROADCAST,RUNNING,MULTICAST>
  Ignore all v6 routes with linkdown
  Type: Ethernet
  HWaddr: 12:31:9c:1f:c3:99
  inet 192.168.1.1/24
  inet6 fe80::1031:9cff:fe1f:c399/64
  Interface Type VETH
  Interface Slave Type None
  protodown: off 
  Parent interface: r1-eth1
Interface r1-eth1 is up, line protocol detection is disabled
  Link ups:       0    last: (never)
  Link downs:     0    last: (never)
  vrf: default
  Description: to sw2 - RIPv2 interface
  index 3 metric 0 mtu 1500 speed 10000 
  flags: <UP,BROADCAST,RUNNING,MULTICAST>
  Ignore all v6 routes with linkdown
  Type: Ethernet
  HWaddr: ca:5b:e3:89:ff:35
  inet 193.1.1.1/26
  inet6 fe80::c85b:e3ff:fe89:ff35/64
  Interface Type VETH
  Interface Slave Type None
  protodown: off 
  Parent ifindex: 5
ZephyrusNG# configure terminal 
ZephyrusNG(config)# router eigrp 1
ZephyrusNG(config-router)# 
root@ZephyrusNG:/tmp/topotests/eigrp_topo1.test_eigrp_topo1# vtysh

Hello, this is FRRouting (version 9.0-dev).
Copyright 1996-2005 Kunihiro Ishiguro, et al.

ZephyrusNG# show run
Building configuration...

Current configuration:
!
frr version 9.0-dev
frr defaults traditional
hostname r1
log file /tmp/topotests/eigrp_topo1.test_eigrp_topo1/r1/zebra.log
log timestamp precision 3
log commands
log file /tmp/topotests/eigrp_topo1.test_eigrp_topo1/r1/eigrpd.log
log file /tmp/topotests/eigrp_topo1.test_eigrp_topo1/r1/staticd.log
hostname ZephyrusNG
no service integrated-vtysh-config
!
interface r1-eth0
 ip address 192.168.1.1/24
exit
!
interface r1-eth1
 description to sw2 - RIPv2 interface
 ip address 193.1.1.1/26
 no link-detect
exit
!
router eigrp 1
 network 0.0.0.0/0
exit
!
end
ZephyrusNG# configure terminal 
ZephyrusNG(config)# router eigrp 1
ZephyrusNG(config-router)# passive-interface 
lo       r1-eth0  r1-eth1  
ZephyrusNG(config-router)# passive-interface r1-eth0 
ZephyrusNG(config-router)# 
ZephyrusNG(config)# interface 
lo       r1-eth0  r1-eth1  
ZephyrusNG(config)# interface r1-eth0 
ZephyrusNG(config-if)# eigrp bandwidth 10
ZephyrusNG(config-if)# 
  • XXX: Need to fix the handling/validation flow ??

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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

@frrbot
Copy link

frrbot bot commented Sep 7, 2023

This issue will be automatically closed in the specified period unless there is further activity.

@frrbot frrbot bot closed this as completed Sep 14, 2023
@frrbot frrbot bot closed this as completed Sep 14, 2023
@frrbot frrbot bot removed autoclose labels Sep 14, 2023
@volodymyrhuti
Copy link
Contributor

volodymyrhuti commented Oct 9, 2023

Hi all! Sorry for the delay, I couldn't find time for this (
This bug is still relevant, so I have detected two separate issues here:

  • Interfaces are not allocated unless topology converges, assuming a network command was issued.
    Looking at similar daemons, I have noticed that ripd is solving this by maintaining a vector.
    This means a separate mapping [iface_name -> iface_passive] and a couple more N.B. callbacks.
    I have pulled this logic into eigrpd and published it via the following branch:
    https://github.com/volodymyrhuti/frr/tree/EIGRP_BUG_NB_11301

  • Then I noticed the bandwidth failing with the same message, which sucks a lot.
    From my research, I have found a commented-out callback that should initialize the iface structure.
    eigrp_interface.c -> eigrp_if_init() -> // hook_register_prio(if_add, 0, eigrp_if_new);
    Implementing it back should fix the config issue and will likely resolve the prior bug.
    P.S. It doesn't fix the prior issue, assuming you want interface-passive default functionality
    However, from testing, it breaks code semantics - allocation is not expected by the rest of the code.
    I'm going to see if I can find semantics divergence, but I'm not sure how long it may take ;(

volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Nov 9, 2023
Instatiate interface structure once demon is started.
Otherwise it is not possible to set parameters such as bandwidth
untill a `network` configuration was applied.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Nov 9, 2023
Maintain a vector of mappings [if_name -> if_passive].
Otherwise, if topo is not already convereged, configuration
does not apply, since iface is missing from the iface list.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Nov 9, 2023
Maintain a vector of mappings [if_name -> if_passive].
Otherwise, if topo is not already convereged, configuration
does not apply, since iface is missing from the iface list.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Nov 22, 2023
Maintain a vector of mappings [if_name -> if_passive].
Otherwise, if topo is not already convereged, configuration
does not apply, since iface is missing from the iface list.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Mar 20, 2024
Instatiate interface structure once demon is started.
Otherwise it is not possible to set parameters such as bandwidth
untill a `network` configuration was applied.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
volodymyrhuti pushed a commit to volodymyrhuti/frr that referenced this issue Mar 20, 2024
Maintain a vector of mappings [if_name -> if_passive].
Otherwise, if topo is not already convereged, configuration
does not apply, since iface is missing from the iface list.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <v.huti@vyos.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs further investigation
Projects
None yet
Development

No branches or pull requests

3 participants