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

vyos.configdict: T5308: Remove workarounds for incorrect defaults in get_interface_dict #2052

Merged
merged 2 commits into from Jun 24, 2023

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Jun 23, 2023

Change Summary

Note: this depends on PR:
#2010

Applying the revised vyos.xml lib (T5218) to get_config_dict, one can remove the workarounds needed in get_interface_dict due to incorrect defaults on tag nodes (bug T2665).

The original implementation of the vyos.xml lib adds defaults below a tag node name, irrespective of the existence of a corresponding tag node value, and at the wrong level; this required workarounds to delete and/or shift defaults before merging dicts.

That bug is corrected in the revision of the vyos.xml lib, merged in T5218.

As a first step, we remove the workarounds in vyos.configdict.get_interface_dict; this provides the same results without the workaround overhead, passes all smoketests and config tests. As get_interface_dict is a critical part of the vyos lib infrastructure, it is addressed first and as a separate task. Removing all other cases of T2665 workarounds will follow directly.

Note that for compatibility this maintains the practice of merging 'recursive' defaults in get_interface_dict --- namely, those leaf nodes with default values that have an ancestor node in the config are merged into the result. The revisions to the xml lib and get_config_dict also support providing 'non-recursive' defaults: those that have a parent in the config; cf. the discussion of invariants in PR #2010, above. We can consider maintaining this invariant going forward.

Example, exhibiting the bug corrected in the second commit, below:

vyos@vyos# show interfaces ethernet eth1
+address dhcpv6
+dhcpv6-options {
+    parameters-only
+}
 hw-id 52:54:00:7b:3e:08
>>> from vyos.config import Config
>>> from vyos.orig_configdict import get_interface_dict as orig_get_interface_dict
>>> from vyos.configdict import get_interface_dict
>>> from pprint import pprint
>>> conf = Config()
>>> base = ['interfaces', 'ethernet']
>>> _, orig = orig_get_interface_dict(conf, base, ifname='eth1')
>>> _, rev = get_interface_dict(conf, base, ifname='eth1')
>>> _, rev_local = get_interface_dict(conf, base, ifname='eth1', recursive_defaults=False)
>>> pprint(dict(sorted(orig.items())))
{'address': ['dhcpv6'],
 'dhcpv6_options': {'parameters_only': {}, 'pd': {}},
 'duplex': 'auto',
 'hw_id': '52:54:00:7b:3e:08',
 'ifname': 'eth1',
 'ip': {'arp_cache_timeout': '30'},
 'mtu': '1500',
 'speed': 'auto'}
>>> pprint(dict(sorted(rev.items())))
{'address': ['dhcpv6'],
 'dhcpv6_options': {'parameters_only': {}},
 'duplex': 'auto',
 'hw_id': '52:54:00:7b:3e:08',
 'ifname': 'eth1',
 'ip': {'arp_cache_timeout': '30'},
 'mtu': '1500',
 'speed': 'auto'}
>>> pprint(dict(sorted(rev_local.items())))
{'address': ['dhcpv6'],
 'dhcpv6_options': {'parameters_only': {}},
 'duplex': 'auto',
 'hw_id': '52:54:00:7b:3e:08',
 'ifname': 'eth1',
 'mtu': '1500',
 'speed': 'auto'}

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Component(s) name

Proposed changes

How to test

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

The original implementation of defaults, and workaround required, would
leave an entry {'dhcpv6_options': {'pd': {}}} in the interface_dict.
@jestabro jestabro self-assigned this Jun 23, 2023
@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, sever-sever and c-po and removed request for a team June 23, 2023 16:54
@c-po c-po merged commit ce37b95 into vyos:current Jun 24, 2023
6 of 7 checks passed
@c-po
Copy link
Member

c-po commented Jun 24, 2023

Yeah! always looked forward to it! Thanks @jestabro

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