Page MenuHomeVyOS Platform

get_config_dict() shall always return a list on <multi/> nodes
Open, HighPublicBUG

Description

Depending on the number of values configured on a <multi/> node the return format inside get_config_dict() changes from string to list.

Assume the following:

ssh {
   listen-address 172.18.254.201
   port 22
   port 2222
}

This will yield: {'listen_address': '172.18.254.201', 'port': ['22', '2222']}

ssh {
   listen-address 172.18.254.201
   listen-address 172.18.254.202
   port 22
   port 2222
}

This will yield: {['listen_address': '172.18.254.201', 'listen_address': '172.18.254.201'], 'port': ['22', '2222']}

Thus using this code for templating becomes error prone and will add a lot of redundancy to the templates. It can be coded as:

{% if listen_address %}
# Specifies the local addresses sshd should listen on
{%   if listen_address is string %}
ListenAddress {{ listen_address }}
{%   else %}
{%     for address in listen_address %}
ListenAddress {{ value }}
{%     endfor %}
{%   endif %}
{% endif %}

But is it would always return a list for a multi node (as what happens on conf.return_values()) the template will shrink to (and be consistent to return_values()):

{% if listen_address %}
# Specifies the local addresses sshd should listen on
{%   for address in listen_address %}
ListenAddress {{ value }}
{%   endfor %}
{% endif %}

TagNodes

It turned out that this also becomes an issue on tag nodes.

Interface with one IP address

>>> Config().get_config_dict(['interfaces', 'ethernet', 'eth2', 'address'])
{'address': '2.2.2.2/32'}

Interface with two IP addresses

>>> Config().get_config_dict(['interfaces', 'ethernet', 'eth1', 'address'])
{'address': ['fd00::ffff/64', '1.1.1.1/32']}

In the past IP addresses have always been retrieved using return_values() which always ensured the result is a list. All subsequent code requires a list input here :(

Details

Difficulty level
Easy (less than an hour)
Version
1.3-rolling
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Event Timeline

c-po assigned this task to dmbaturin.Tue, Jun 23, 3:43 PM
c-po triaged this task as High priority.
c-po created this task.
c-po updated the task description. (Show Details)
c-po added a subscriber: jestabro.
pasik added a subscriber: pasik.Wed, Jun 24, 7:26 PM
c-po updated the task description. (Show Details)Fri, Jun 26, 12:01 PM
c-po updated the task description. (Show Details)

The data returned by get_config_dict should always have the same format to prevent any function using it to have to check if one element is a list or a string.

When written T2522, I came across this issue (but the other way round: when trying to render the data on screen from my own configuration class). When there is only one element in the list, it is better for the reader of the configuration of the class if the data is presented as one element.

So I believe showConfig is doing the right thing when generating a configuration for human consumption. It is however not doing what should when parsed to generate a structure to be used for automation.

As the data about which node is now available via the xml code within the repo, it would be quite trivial to iterate the data in get_config_dict and convert any string into a list of one element with that string.
This work would only be required once per call to showConfig and therefore not taxing performance-wise.

Unless someone object I propose to do just that as all the code currently should already work with a list (therefore this should not break anything) and will allow removing the "string" case down the line.

The value vs list is not produced by showConfig, but rather due to matching in configtree. We'll let @dmbaturin consider if this is an issue or not, but I imagine a jinja macro could address the problem where it occurs in template generation.

showConfig produces the following for multi-nodes:

test {
    something one
    something two
}
thomas-mangin added a comment.EditedMon, Jun 29, 1:24 PM

Yes, so there is no way to know at parsing time (unless you have two elements or check the XML) that this is a multi-element. Something "computer-friendly" would generate:

test {
    something [one]
}
test {
    something [one two]
}

which I am sure we all agree is not as good as what we have now.

But you make an excellent point, @thomas-mangin ; it may be best to normalize that within config_dict itself, now that we have the xml processing available ...