libyang2 change in behavior between 2.1.80 and 2.1.111

[donaldsharp]

When using libyang2 w/ FRR we are seeing a change in behavior between 2.1.80 and 2.1.111

Our libyang model:

          key "condition";
          description
            "Route map match conditions";
          leaf condition {
            type identityref {
              base rmap-match-type;
            }
            description
              "Match condition";
          }

show run output for 2.1.80 from FRR's perspective:

route-map noadv deny 5
 match ip address prefix-list noadv
exit
!
route-map noadv permit 20
exit
!
end
testserver# exit
sharpd@eva:~/libyang/pkg/pkgs/pop-22.04$ dpkg -l | grep yang
ii  libyang2:amd64                          2.1.80-1                                                        amd64        parser toolkit for IETF YANG data modeling - runtime
ii  libyang2-dev:amd64                      2.1.80-1                                                        amd64        parser toolkit for IETF YANG data modeling - development files

From 2.1.111's perspective:

ip prefix-list noadv seq 60 permit 103.18.4.0/25 le 32
!
route-map noadv deny 5
exit
!
route-map noadv permit 20
exit
!
end
eva# exit
sharpd@eva:~/libyang/pkg/pkgs/pop-22.04/libyang2_2.1.111-1$ dpkg -l | grep yang
ii libyang2:amd64 2.1.111-1 amd64 parser toolkit for IETF YANG data modeling - runtime
ii libyang2-dev:amd64 2.1.111-1 amd64 parser toolkit for IETF YANG data modeling - development files

Notice that the match ip address... line is no longer displayed

[donaldsharp]

I did some libyang2 bisecting between 2.1.80 and 2.1.111 and came up with this commit as breaking:

sharpd@eva:~/libyang$ git bisect good
32f067fecd084f07a03f8bf084f0abaf9c142927 is the first bad commit
commit 32f067fecd084f07a03f8bf084f0abaf9c142927
Author: Michal Vasko <mvasko@cesnet.cz>
Date:   Thu Jun 1 12:31:14 2023 +0200

    identityref UPDATE support optional module prefix
    
    Fixes #420

 src/plugins_types.c                 | 40 ++++++++++++++++++++++++-----
 src/plugins_types.h                 |  2 +-
 src/plugins_types/identityref.c     | 14 ++++++++---
 src/plugins_types/instanceid.c      | 12 ++++++++-
 src/plugins_types/instanceid_keys.c | 38 ++++++++++++++++++++--------
 src/plugins_types/node_instanceid.c | 12 ++++++++-
 src/plugins_types/xpath1.0.c        | 26 +++++++++++--------
 src/printer_json.c                  | 14 +++++++----
 src/printer_xml.c                   | 43 +++++++++++++++++++++----------
 tests/utests/types/identityref.c    | 50 ++++++++++++++++++++++++-------------
 tests/utests/types/instanceid.c     |  2 +-
 tests/utests/types/union.c          |  2 +-

[donaldsharp]

what other data is needed here?

[michalvasko]

Sorry, but I have no idea what is the output I am looking at, is it some router plugin reacting to configuration changes? In any case, you have pointed to some identityref issue and I think the change that occurred in this commit is that before identityrefs were always printed as <module-name>:<identity> but after the commit local identities were no longer printed with the prefix.

If I am right, the plugin is expecting the prefix that is not there but it should not be using the string value no matter what, the struct lyd_value should be accessed directly for the identity and its name. There is never a prefix in this case.

[choppsv1]

The output is showing that config that used to be accepted and saved in the libyang datastore

 match ip address prefix-list noadv

is no longer working. we can try and dig more into this; however, can you please look at the relevant yang model pointed at below to see if you see something obvious that could be triggering the bug? It's definitely using identities and identityrefs

https://github.com/FRRouting/frr/blob/master/yang/frr-route-map.yang

[michalvasko]

I am sorry, but I have nothing to go on. I have mentioned the one idea I had and looking more at the changes, I have correctly described the change that I made in the commit.

[choppsv1]

We aren't doing anything with schema here I don't think. Are you saying that something in a dnode has changed?

[choppsv1]

Perhaps could you describe the exact behavior change in a dnode that the commit does? It might be easier for me to locate the code to look for then.

[choppsv1]

For our CLI parse we create xpaths to set the value in the datastore.. this is the code for that line of config above, it is definitely using identityref here that's the condition key

	const char *xpath =
		"./match-condition[condition='frr-route-map:ipv4-prefix-list']";
	char xpath_value[XPATH_MAXLEN];

	nb_cli_enqueue_change(vty, xpath, NB_OP_CREATE, NULL);
	snprintf(xpath_value, sizeof(xpath_value),
		 "%s/rmap-match-condition/list-name", xpath);
	nb_cli_enqueue_change(vty, xpath_value, NB_OP_MODIFY, name);

	return nb_cli_apply_changes(vty, NULL);

[michalvasko]

Okay, that may be the problem here, likely the condition ./match-condition[condition='ipv4-prefix-list'] would work. In these cases the official recommendation is to use the derived-from() (or derived-from-or-self()) XPath function. But I think the current expression you have should work, too, in libyang so I will try to reproduce it somehow.

[michalvasko]

I have tried a few use-cases and it worked fine whether I used the prefix or not. Can you tell me what libyang or sysrepo function is being called with xpath_value in nb_cli_enqueue_change()?

[choppsv1]

nb_cli_enqueue_change saves the xpath and value in a intermediate changes structure, calls nb_cli_apply_changes calls nb_cli_apply_changes_internal which calls nb_candidate_edit_config_changes which does some validation to see if the change is allowed:

https://github.com/FRRouting/frr/blob/master/lib/northbound.c#L841

and finally calls nb_candidate_edit if it is allowed. That function uses libyang and is here:

https://github.com/FRRouting/frr/blob/master/lib/northbound.c#L651

[michalvasko]

Okay, I cannot make much sense of it. I see lyd_new_path() is being called but the XPath is most likely modified before because it would not accept an expression starting with .. But lyd_find_xpath() is also called in a nested function, which is exactly what I briefly tested and it accepted both forms of the value.

[jktjkt]

I'm reading in between the lines a bit here, but a wild guess -- does FRR store the value somewhere, and does it use that stored value for naive string comparison at a later point, maybe? If that's the case, then you would be affected by libyang's change of behavior because you might be comparing namespace-qualified and unqualified name for string equality. That won't work, and the proper fix is to ensure that these values are not compared as plain strings.

I glanced through FRR's code very briefly, but since I don't know your abstractions, this is the best pointer I can give you. I think I saw some code which performs some mapping from "your identifiers" to underlying libyang data nodes, but it's been some time.