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

configdep: T6559: fix regression in dependent script error under configd #3813

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Jul 15, 2024

Change Summary

A regression in the redundant dependency removal when running under configd (T5660) can cause a crash of the config daemon leaving the commit session uncompleted. This occurs when the last element of the priority queue succeeds, but a resulting dependency fails. Taras pointed this out in the case of a particularly crafted config file, but a simple reproducer is as follows:

set firewall group address-group AG address '192.168.137.5'
set nat source rule 10 source group address-group 'AG'
set nat source rule 10 translation address 'masquerade'
set nat source rule 10 outbound-interface name 'eth1'
commit
delete firewall
commit

The reproducer is added as a smoketest to check graceful exit and reporting of error.
The PR drops the asynchronous calling of dependent scripts, as a fix there would still lack the ability to map the error to the originating element of the priority queue. Secondly, it simplifies the management of the dependency list when running with or without configd.

Configtest and smoketests pass in current (note that smoketests now default to running with configd in current); configtest and configd smoketests pass in sagitta, and sagitta 'make test' (no configd) shows only the errors recently seen in CI.

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)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_config_dependency.py 
test_configdep_error (__main__.TestConfigDep.test_configdep_error) ... 
firewall: ConfigError('dependent nat: Invalid address-group "AG" on nat rule')

ok

----------------------------------------------------------------------
Ran 1 test in 5.997s

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

@jestabro jestabro self-assigned this Jul 15, 2024
@jestabro jestabro requested a review from a team as a code owner July 15, 2024 05:30
Copy link

github-actions bot commented Jul 15, 2024

👍
No issues in PR Title / Commit Title

Copy link

👍
No issues in unused-imports

@c-po c-po merged commit 2fdcf50 into vyos:current Jul 15, 2024
12 of 13 checks passed
Copy link

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests ❌ failed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed

@jestabro
Copy link
Contributor Author

@Mergifyio backport circinus

Copy link
Contributor

mergify bot commented Jul 17, 2024

backport circinus

✅ Backports have been created

@jestabro
Copy link
Contributor Author

@Mergifyio backport sagitta

Copy link
Contributor

mergify bot commented Jul 17, 2024

backport sagitta

✅ Backports have been created

dmbaturin added a commit that referenced this pull request Jul 18, 2024
configdep: T6559: fix regression in dependent script error under configd (backport #3813)
dmbaturin added a commit that referenced this pull request Jul 18, 2024
configdep: T6559: fix regression in dependent script error under configd (backport #3813)
@jestabro jestabro deleted the configdep-error branch September 18, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants