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

T6815:Fix compatibility with 1.3-1.5 of VyOS (Step 1) #352

Merged
merged 43 commits into from
Nov 10, 2024

Conversation

gaige
Copy link
Contributor

@gaige gaige commented Jul 28, 2024

Change Summary

This set of changes enables compatibility with a variety of parameters that were previously only compatible with version 1.2 of VyOS. Wherever possible, configuration is backward compatible.

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)
  • Other (please describe): Breaking change for certain configurations

Related Task(s)

https://vyos.dev/T6815

Related PR(s)

Proposed changes

There are a couple of things I’m interested in comments on:

  1. There are multiple changes in here for support of 1.3 and 1.4 of vyos (and 1.5 effectively), as such it’s sizeable. I can theoretically break it up by command if we desire, although it’ll be additional work.
  2. There were configuration parameters that were moved in substantial ways between versions. I’m looking to see if we should add a version parameter to override situations where you need to do things like operate on non-live configurations (and because the ResourceModule doesn’t really deal easily with multiple overlapping templates). I don’t want to put these as new modules, as the configs are nearly identical.
  3. From the previous paragraph, there are a couple of small breaking changes. I tried to heed the configuration as much as possible, but there were versions that I could make backward-compatible, but not effectively forward-compatible. I have done so in these cases, and the firewall rules have changes around the tcp flags that are not backward-compatible, which is noted.

How to test

Tested against 1.3, and 1.5 of VyOS manually using --dry-run for both ingestion and change.
Updated and tested unit tests
Ran Sanity tests

Checklist:

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

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, I'm looking forward to the final version.

Copy link
Contributor

@sdwilsh sdwilsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 439 of plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py, you'll want to incorporate the change I made in #354 to the _add_state method and include tests for it, as it does not work correctly at the moment.

@sdwilsh
Copy link
Contributor

sdwilsh commented Sep 15, 2024

Another two things that doesn't work well with 1.5-rolling right now is pretty much every part of vyos_firewall_global (the only thing that I'm seeing work is group, alas) and I'm having issues with vyos_ntp_global as well.

@gaige
Copy link
Contributor Author

gaige commented Sep 17, 2024

Another two things that doesn't work well with 1.5-rolling right now is pretty much every part of vyos_firewall_global (the only thing that I'm seeing work is group, alas) and I'm having issues with vyos_ntp_global as well.

appreciate the heads up. More detail on what commands you are using and seeing problems with would be helpful. Right now I’m testing on 3 virtual and 3 hardware vyos systems using a real-world configuration. However, my production systems don’t use all commands or capabilities.

thanks

@sdwilsh
Copy link
Contributor

sdwilsh commented Sep 17, 2024

For vyos_firewall_global: https://github.com/sdwilsh/ansible-playbooks/blob/main/plays/vyos.yml#L53-L76 (group is fine, but the default there would work for testing)
For vyos_ntp_global: https://github.com/sdwilsh/ansible-playbooks/blob/main/plays/vyos.yml#L30-L37

@andamasov andamasov changed the title Fix compatibility with 1.3-1.5 of VyOS (Step 1) T6815:Fix compatibility with 1.3-1.5 of VyOS (Step 1) Oct 25, 2024
Copy link
Contributor

@omnom62 omnom62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
I have managed to review the commit related to firewall_rules and ospf_interfaces and test them against my local 1.3.4 and 1.5 vyos VMs and ansible and base and clutetrust fork.
How do we want to tackle the whole lot, by the way?
Also, are the changes/commits worth being tagged by corresponding and specific feature from https://docs.vyos.io/en/stable/changelog/1.4.html and https://docs.vyos.io/en/latest/changelog/1.5.html to track them more effeciently?

Copy link
Contributor

@omnom62 omnom62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per comment, some 1.5 implementation seems to require new details

@omnom62
Copy link
Contributor

omnom62 commented Nov 3, 2024

I finished reviewing the PR and also did some regression and progression tests against live 1.3, 1.4 and 1.5. using this PR.
It generally looks okay to early field testsing (though I could not familiarised against all the changes in Ansible and features of VyOS).
The only outstanding issue from what I saw, was VyOS v.1.5 match-*-in/out support in firewall, which did not work with either original nor cluetrust collections.
My other remarks were about the implementation that can potentially break in future and require a review

@gaige gaige marked this pull request as ready for review November 9, 2024 13:13
Copy link
Contributor

@omnom62 omnom62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me

Copy link
Member

@andamasov andamasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@gaige gaige merged commit a7ac0fd into vyos:main Nov 10, 2024
50 checks passed
@gaige gaige deleted the fix-banners branch November 10, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants