sysrepo config change subscription has no way of filtering augmented modules xpaths

[adaraiseh]

Description

Currently frr_sr_subscribe_config subscribes to config changes on all xpaths of the input yang module (the optional xpath filter is set to NULL):

ret = sr_module_change_subscribe(
		session, module->name, NULL, frr_sr_config_change_cb, NULL, 0,
		SR_SUBSCR_DEFAULT | SR_SUBSCR_ENABLED | SR_SUBSCR_NO_THREAD,
		&module->sr_subscription);

This becomes problematic when the yang module is augmented by multiple daemons submodules.
For example: frr-routing yang module is augmented by pimd modules (frr-pim.yang and frr-pim-rp.yang) and also is augmented by staticd modules (frr-staticd.yang).
With this situation, when running pimd daemon, its sysrepo northbound will subscribe and process changes from pimd and staticd.
So both pimd and staticd will subscribe both of the following xpaths changes:

/frr-routing:routing/control-plane-protocols/control-plane-protocol[type='frr-pimd:pimd']/*
/frr-routing:routing/control-plane-protocols/control-plane-protocol[type='frr-staticd:staticd']/*

Due to this, any config changes happening on staticd submdule xpath will have to be processed by pimd and obviously it fails:

~$ sudo /usr/lib/frr/pimd -M sysrepo --log stdout --log-level debug
2024/07/25 12:53:05 PIM: [JBNZN-EVZ5D] VRF Created: default(0)
2024/07/25 12:53:05 PIM: [QF5NQ-EJ8X3] pim_vrf_enable: for default 0
2024/07/25 12:53:05 PIM: [WGYFP-JK6RV] zclient_lookup_sched_now: zclient lookup immediate connection scheduled
2024/07/25 12:53:05 PIM: [PK8MV-NK1RX] zclient_lookup_new: zclient lookup socket initialized
2024/07/25 12:53:05 PIM: [T83RR-8SM5G] pimd 10.0.1-onm-gb3d3e3384 starting: vty@2611

2024/07/25 12:53:05 PIM: [V6124-SV3DH][EC 100663325] frr_sr_process_change: unknown data path: /frr-routing:routing/control-plane-protocols/control-plane-protocol[type='frr-staticd:staticd'][name='test-static'][vrf='default']
2024/07/25 12:53:05 PIM: [GZ4RY-Z8B52][EC 100663352] sr_module_change_subscribe(): User callback failed

Such problem is not limited only to frr-routing yang module, it is expected to happen to any yang module augmented by multiple daemons submodules, like frr-interface.
This will happen only for sysrepo northbound, because the yang modules in its case are complied outside frr (in sysrepo) and typically it will include all submodules augmentations (users would be installing all frr daemons yang modules into sysrepo).

Version

# sh version 
FRRouting 10.0.1-onm-gb3d3e3384-dirty (Ubuntu-23-10) on Linux(6.5.0-44-generic).
Copyright 1996-2005 Kunihiro Ishiguro, et al.
This is a git build of frr-10.0.1-16-gb3d3e3384-dirty
Associated branch(es):
	local:stable/10.0
	github/frrouting/frr.git/stable/10.0

configured with:
    '--localstatedir=/var/opt/frr' '--sbindir=/usr/lib/frr' '--sysconfdir=/etc/frr' '--enable-multipath=64' '--enable-user=frr' '--enable-group=frr' '--enable-vty-group=frrvty' '--enable-configfile-mask=0640' '--enable-logfile-mask=0640' '--enable-fpm' '--enable-sysrepo' '--enable-config-rollbacks' '--enable-grpc' '--with-pkg-git-version' '--with-pkg-extra-version=-onm'

How to reproduce

1- compile frr with sysrepo module.
2- Install the following yang modules on sysrepo:

#frr
sudo sysrepoctl -i ./ietf/ietf-interfaces.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-vrf.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-interface.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-route-types.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-filter.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-route-map.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-bfdd.yang -o frr -g frr -p 660
sudo sysrepoctl -i ./ietf/ietf-routing-types.yang -o frr -g frr -p 660
sudo sysrepoctl -i  frr-nexthop.yang -o frr -g frr -p 660
sudo sysrepoctl -i  frr-if-rmap.yang -o frr -g frr -p 660
sudo sysrepoctl -i  frr-affinity-map.yang -o frr -g frr -p 660
sudo sysrepoctl -i ./ietf/frr-deviations-ietf-interfaces.yang -o frr -g frr -p 660

#pimd:
sudo sysrepoctl -i frr-pim.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-pim-rp.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-gmp.yang  -o frr -g frr -p 660

#staticd:
sudo sysrepoctl -i frr-bfdd.yang -o frr -g frr -p 660
sudo sysrepoctl -i frr-staticd.yang -o frr -g frr -p 660

3- Add the following configuration sample to sysrepo:

{
  "frr-routing:routing": {
    "control-plane-protocols": {
      "control-plane-protocol": [
        {
          "type": "frr-pim:pimd",
          "name": "pim",
          "vrf": "test-vrf",
          "frr-pim:pim": {
            "address-family": [
              {
                "address-family": "frr-routing:ipv4",
                "keep-alive-timer": 3
              }
            ]
          }
        },
        {
          "type": "frr-staticd:staticd",
          "name": "staticd",
          "vrf": "default",
          "frr-staticd:staticd": {
            "route-list": [
              {
                "prefix": "10.10.10.0/24",
                "afi-safi": "frr-routing:ipv4-unicast"
              }
            ]
          }
        }
      ]
    }
  }
}

3- start pimd daemon, sysrepo subscription will fail.

Expected behavior

pimd should subscribe only to the xpaths relevant to it.

Actual behavior

pimd subscribes to changes on both staticd and pimd augmentations xpaths on frr-routing yang module.

Additional context

No response

Checklist

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

[adaraiseh]

A simple workaround would be to just ignore the "unknown data path" event by replacing NB_ERR to NB_OK here:
https://github.com/FRRouting/frr/blob/master/lib/northbound_sysrepo.c#L181
trusting that other daemons NB will pickup the change and process it.

[donaldsharp]

@choppsv1 could you take a look at this problem?

[vjardin]

@adaraiseh : do you have a patch about it with your suggestion ? Any pull request ?

[adaraiseh]

A simple workaround would be to just ignore the "unknown data path" event by replacing NB_ERR to NB_OK here: https://github.com/FRRouting/frr/blob/master/lib/northbound_sysrepo.c#L181 trusting that other daemons NB will pickup the change and process it.

@vjardin would it make sense to create a PR with this as a solution? otherwise we'd need to find a way to allow daemons to subscribe to specific xpaths they need, not whole modules.

[vjardin]

@vjardin would it make sense to create a PR with this as a solution? otherwise we'd need to find a way to allow daemons to subscribe to specific xpaths they need, not whole modules.

I believe it can be ok to emit just a warning, but to return NB_OK per your suggestion.

There is a similar set of behavior, according to me, for vytsh that can emit some unexpected commands but it does not give up, such as:
https://github.com/FRRouting/frr/blob/master/vtysh/vtysh.c#L920

I agree that a pull request would make sense so it can be reviewed accordingly. However, along with such corner cases of pull request, it would require to add a test too : see https://github.com/FRRouting/frr/tree/master/tests in order to make sure that it won't be broken once.

[github-actions]

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]

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

[choppsv1]

Is sysrepo functionality still working in the latest release?

[frrbot]

This issue will no longer be automatically closed.