T7496 Fix disabling src route

[RubenNL]

Change Summary

Fixed disabling src route.

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)

https://vyos.dev/T7496

Related PR(s)

Component(s) name

firewall(_global)

Proposed changes

Fixed the ability to disable the src_route.

How to test

First:

- vyos.vyos.vyos_firewall_global:
    config:
      route_redirects:
      - afi: ipv6
        ip_src_route: true

Then:

- vyos.vyos.vyos_firewall_global:
    config:
      route_redirects:
      - afi: ipv6
        ip_src_route: false

Observe: ipv6-src-route is still specified in the config as true. (with the current main branch)

Test results

• Sanity tests passed
• Unit tests passed

The test results can be seen here:
failing: https://github.com/RubenNL/vyos.vyos/actions/runs/15301976749
succeeding: https://github.com/RubenNL/vyos.vyos/actions/runs/15302043258

Tested against VyOS versions:

• 1.4.2 (only tested this bugfix, I still haven't received a open-source license)

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• I have run the ansible sanity and unit tests
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added unit tests to cover my changes
• I have added a file to changelogs/fragments to describe the changes

[Copilot]

Pull Request Overview

This PR fixes the behavior for disabling src_route in both IPv4 and IPv6 route-redirect settings.

• Adjust logic so False values now generate disable commands
• Updated unit tests to expect ip_src_route=False and added ipv6-src-route 'disable' assertions
• Added a changelog fragment for this bugfix

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
plugins/module_utils/network/vyos/config/firewall_global/firewall_global.py	Change condition to val is not None so false values are handled
tests/unit/modules/network/vyos/test_vyos_firewall_global14.py	Updated expected ip_src_route=False and added IPv6 disable command
tests/unit/modules/network/vyos/test_vyos_firewall_global.py	Same updates as above for the non-14 version
changelogs/fragments/T7496_firewall_global_fix_disabling_src_route.yml	Added a bugfix entry for disabling src_route

Comments suppressed due to low confidence (2)

plugins/module_utils/network/vyos/config/firewall_global/firewall_global.py:551

• There is a duplicate afi = None assignment—removing the redundant line will simplify the code and avoid confusion.

afi = None

changelogs/fragments/T7496_firewall_global_fix_disabling_src_route.yml:2

• Changelog entries should follow the - module_name: description format. Change the second dash to a colon for valid YAML, e.g. - vyos_firewall_global: Fix disabling src route.

  - vyos_firewall_global - Fix disabling src route

[omnom62]

Hi @RubenNL
Thanks for spotting this and submitting the fix.
I cloned your branch and tested against.
While it all checked out fine, I can see that when you comment the settings as follows:
- name: Testing FW global hosts: vyos_lab gather_facts: false tasks: - name: Apply the provided configuration (base config) vyos.vyos.vyos_firewall_global: config: route_redirects: - afi: ipv6 # ip_src_route: true state: replaced
this does not result in a change, while I think it should
Apparently, your code needs to cater for use case when the setting was removed from the configuration, i.e. if before it was True, and you removed it from configuration it has to be deleted completely, especially if there is ''replaced' state. What is your opinion?

[RubenNL]

Interesting find!
After a bit of testing I found 2 more: "twa-hazards-protection" and "group" aren't removed.

This needs to be fixed, but in my opinion this is a different issue. "not able to disable" vs "not able to remove".

[omnom62]

After looking into for quite a while, with respect to my recent comment, the behaviour I am requesting seems to demand an effort (we do not use templating, and the provisioning is a bit fragile and will require rewriting a number of lines).
Largely, the behaviour may appear as per design, as we also have a 'deleted' state, to remove unwanted configuration, even though, this creates additional step - but it is better than postponing Release 6 because of this. As such, the solution is acceptable. @gaige do you have opinion on this?

[gaige]

I think the fix seems straightforward for the src-route setting at least. There's a longer-term issue about moving to templates that would likely improve this type of problem, but that's a project for another day.

My one question is whether there's a value in writing an additional test to demonstrate being able to change states. It doesn't appear absolutely necessary, if this wouldn't work even in the absence of an existing value.

Otherwise, this seems isolated enough to include in 6.0.0

[gaige]

One request (fixing the bugfix statement) and one question on an additional test.

[RubenNL]

My one question is whether there's a value in writing an additional test to demonstrate being able to change states. It doesn't appear absolutely necessary, if this wouldn't work even in the absence of an existing value.

I modified one of the tests to check this, I think?

[gaige]

My one question is whether there's a value in writing an additional test to demonstrate being able to change states. It doesn't appear absolutely necessary, if this wouldn't work even in the absence of an existing value.

I modified one of the tests to check this, I think?

You modified the test to only test disabled whereas previously it only checked enabled. Given the specifics of the bug (that it was related specifically to the False value being indistinguishable in that situation from None, it's effective for testing disabled, which is not the default value on the router.

You can probably change your reproduce case to just the setting to disabled, since the switch wasn't necessary in order to trigger the bug.

[RubenNL]

From what I can find:

The ipv6-src-route is enabled in the fixture, and disabled in the test. That should cover the "switching" part.

ip-src-route isn't defined in the fixture, but it's enabled in the test.

Those 2 combined shoul catch everything, right? I don't like modifying the fixture, because that might break other tests.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[RubenNL]

That's a very long CLA!

I'm usually working with CLA's as short as this one: https://www.home-assistant.io/developers/cla/.

Before signing, I will have to spend quite some time reading it all.

[omnom62]

I have read the CLA Document and I hereby sign the CLA

[RubenNL]

I have read the CLA Document and I hereby sign the CLA.

[gaige]

I have read the CLA Document and I hereby sign the CLA

[dmbaturin]

I cannot see any issues and I trust @gaige's judgment.

Dismissing a stale review.