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

Statements given in "load set" are order dependent #287

Closed
pheller opened this issue Nov 10, 2021 · 4 comments
Closed

Statements given in "load set" are order dependent #287

pheller opened this issue Nov 10, 2021 · 4 comments

Comments

@pheller
Copy link
Contributor

pheller commented Nov 10, 2021

Given the openconfig schema and a file comprised of the following set statements:

set network-instances network-instance default fdb config flood-unknown-unicast-supression false
set network-instances network-instance default config type oc-ni-types:DEFAULT_INSTANCE
set network-instances network-instance default config enabled true
set network-instances network-instance default config router-id 1.2.3.4

I receive an error response:

Nov 10 18:29:37: Editing configuration: application unknown-element Node 'fdb' tagged with 'when' condition './config/type = 'oc-ni-types:L2VSI'
                  or ./config/type = 'oc-ni-types:L2P2P'
                  or ./config/type = 'oc-ni-types:L2L3'
                  or ./config/type = 'oc-ni-types:DEFAULT_INSTANCE'' in module 'openconfig-network-instance' evaluates to false in edit-config operation (see RFC 7950 Sec 8.3.2) <bad-element>fdb</bad-element>
Nov 10 18:29:37: process_config_statement: 33: FATAL: system error while processing the config file

FATAL: system error while processing the config file

However, if I move the first statement to the end, I receive no error.

Additionally, load set can take quite some time with even small files. It would seem to me that the load_set_file method is simply iterating the lines within the file and processing them like individual set commands given at the CLI, such that each set statement is an individual edit-config operation complete with backend validation.

Ideally, the contents would be processed as a single unit, perhaps mutating some in-memory copy of the candidate, and at validation time, only a single syntactic and semantic validation is performed.

@olofhagsand
Copy link
Member

Side-note, I assume the yang has been patched as follows:

+++ b/release/models/network-instance/openconfig-network-instance.yang
@@ -246,10 +246,10 @@ module openconfig-network-instance {
         }
 
         uses l2ni-instance {
-            when "./config/type = 'L2VSI'
-                  or ./config/type = 'L2P2P'
-                  or ./config/type = 'L2L3'
-                  or ./config/type = 'DEFAULT_INSTANCE'" {
+            when "./config/type = 'oc-ni-types:L2VSI'
+                  or ./config/type = 'oc-ni-types:L2P2P'
+                  or ./config/type = 'oc-ni-types:L2L3'
+                  or ./config/type = 'oc-ni-types:DEFAULT_INSTANCE'" {
                   description

@pheller
Copy link
Contributor Author

pheller commented Nov 14, 2021

Side-note, I assume the yang has been patched as follows:

+++ b/release/models/network-instance/openconfig-network-instance.yang
@@ -246,10 +246,10 @@ module openconfig-network-instance {
         }
 
         uses l2ni-instance {
-            when "./config/type = 'L2VSI'
-                  or ./config/type = 'L2P2P'
-                  or ./config/type = 'L2L3'
-                  or ./config/type = 'DEFAULT_INSTANCE'" {
+            when "./config/type = 'oc-ni-types:L2VSI'
+                  or ./config/type = 'oc-ni-types:L2P2P'
+                  or ./config/type = 'oc-ni-types:L2L3'
+                  or ./config/type = 'oc-ni-types:DEFAULT_INSTANCE'" {
                   description

Yes, it has.

@olofhagsand
Copy link
Member

olofhagsand commented Nov 15, 2021

This is problematic.
It is a combination of

  1. The order in the openconfig spec under "network-instances network-instance default": fdb vs config
  2. RFC 7950 8.3.2 requirement to check "when" conditions on edit-config input.

First, see openconfig-network-instance.yang:

 container network-instances {
       ...
        uses l2ni-instance {
            when "./config/type = 'oc-ni-types:L2VSI'
                  or ./config/type = 'oc-ni-types:L2P2P'
                  or ./config/type = 'oc-ni-types:L2L3'
                  or ./config/type = 'oc-ni-types:DEFAULT_INSTANCE'" {
                ...
            }
        }
        container config {

Note that "fdb" is before "config" in the network-instances container. Clixon uses the order they are stated in YANG. Therefore, if the statements are given one-by-one as in the issue, the "when" statements will be evaluated before the config statements have been given and therefore fail.

Therefore, a clixon workaround is to switch the order of config and fdb in the spec.

The RFC is vague, the closest seem to be in 7.5.7:

   The container's child nodes are encoded as subelements to the
   container element.  If the container defines RPC or action input or
   output parameters, these subelements are encoded in the same order as
   they are defined within the "container" statement.  Otherwise, the
   sub elements are encoded in any order.

which means clixon could use another order when encoding to xml, ie config before fdb. This may be very difficult in the general case, but could be done for very specific use-cases, such as this one (eg place statements with "when" statements last)

Second,
Most validation checks are at validation time. If this was the case for when in this case it would have worked as well.
However, RFC 7950 says in 8.3.2:

   After the incoming data is parsed, the NETCONF server performs the
   <edit-config> operation by applying the data to the configuration
   datastore.  During this processing, the following errors MUST be
   detected:
   ...
   o  Modification requests for nodes tagged with "when", and the "when"
      condition evaluates to "false".  In this case, the server MUST
      reply with an "unknown-element" <error-tag> in the <rpc-error>.

So removing the check at edit-config time and only evaluate "when" conditions at validate time would violate the RFC. I find this requirement somewhat strange though, maybe one should deviate from the RFC in this respect.

Another way would be to not dump single cli statements, ie group them all together in the same edit-config, so that the "config" and "fdb" are in the same edit-config. However this is not the way the generated CLI currently works, since each "CR" generates an edit-config message to the server. I dont know how to change that in an easy way.

Concluding, I see the following solutions/workarounds:

  1. Change the order of config and fdb in openconfig-network-instance.yang
  2. Analyze the container sub-statements and place those with "when" statements last when encoding to XML/JSON.
  3. Deviate from the standard and perform the "when" check only at validation time (not when processing incoming edit-configs)

olofhagsand added a commit that referenced this issue Nov 16, 2021
  * Some openconfig specs seem to have use/when before a "config" which it depends on. This leads to XML encoding being in the "wrong order.
  * When parsing, clixon now sorts container/list statements so that sub-statements with WHEN are put last.
  * See [Statements given in "load set" are order dependent](#287)
* Fixed: [Statements given in "load set" are order dependent](#287)
@olofhagsand
Copy link
Member

Made a change to encoding order by putting sub-elements with "WHEN" conditions last, ie following proposal (2) above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants