-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix(panos_security_rule): state merged with existing values #570
fix(panos_security_rule): state merged with existing values #570
Conversation
* Also fixes panos_template mode xpath error on second run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I understand the intent and functionality of these changes - from what I can see this solution is good and shouldn't add too much additional handling within the Ansible collection. I also like that we don't have to fix the entire thing at once or change the underlying modules considerably.
Introduce default_values param in helper function instead of sdk_params default values to avoid issue with default values being merged to existing configuration.
We need to clarify how to document module default values while they are not in spec but actually applied with present or merged states while creating new resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now and based on our feedback from the Red Hat team it's the only path forward.
## [2.20.0](v2.19.1...v2.20.0) (2024-09-24) ### Features * Add additional error handling to some upgrade assurance modules ([PaloAltoNetworks#561](https://github.com/alperenkose/pan-os-ansible/issues/561)) ([c64cd79](c64cd79)) * Add new option to panos_active_in_ha module ([PaloAltoNetworks#560](https://github.com/alperenkose/pan-os-ansible/issues/560)) ([a2870f5](a2870f5)) ### Bug Fixes * Add 'parent_interface' parameter for l2/l3 subinterface modules ([PaloAltoNetworks#552](https://github.com/alperenkose/pan-os-ansible/issues/552)) ([73c28a8](73c28a8)) * new release for failed ci ([3872708](3872708)) * requirements.txt update python version and remove hashes ([905b1eb](905b1eb)) * **panos_facts.py:** Fixed virtual systems fact name ([PaloAltoNetworks#558](https://github.com/alperenkose/pan-os-ansible/issues/558)) ([0d0fd6d](0d0fd6d)) * **panos_security_rule:** state merged with existing values ([PaloAltoNetworks#570](https://github.com/alperenkose/pan-os-ansible/issues/570)) ([db6c32c](db6c32c))
## [2.20.0](v2.19.1...v2.20.0) (2024-09-25) ### Features * Add additional error handling to some upgrade assurance modules ([PaloAltoNetworks#561](https://github.com/alperenkose/pan-os-ansible/issues/561)) ([c64cd79](c64cd79)) * Add new option to panos_active_in_ha module ([PaloAltoNetworks#560](https://github.com/alperenkose/pan-os-ansible/issues/560)) ([a2870f5](a2870f5)) * ee ci for development ([97c31ba](97c31ba)) ### Bug Fixes * Add 'parent_interface' parameter for l2/l3 subinterface modules ([PaloAltoNetworks#552](https://github.com/alperenkose/pan-os-ansible/issues/552)) ([73c28a8](73c28a8)) * new release for failed ci ([3872708](3872708)) * requirements.txt update python version and remove hashes ([905b1eb](905b1eb)) * **panos_facts.py:** Fixed virtual systems fact name ([PaloAltoNetworks#558](https://github.com/alperenkose/pan-os-ansible/issues/558)) ([0d0fd6d](0d0fd6d)) * **panos_security_rule:** state merged with existing values ([PaloAltoNetworks#570](https://github.com/alperenkose/pan-os-ansible/issues/570)) ([db6c32c](db6c32c))
## [2.20.0](v2.19.1...v2.20.0) (2024-09-25) ### Features * Add additional error handling to some upgrade assurance modules ([PaloAltoNetworks#561](https://github.com/alperenkose/pan-os-ansible/issues/561)) ([c64cd79](c64cd79)) * Add new option to panos_active_in_ha module ([PaloAltoNetworks#560](https://github.com/alperenkose/pan-os-ansible/issues/560)) ([a2870f5](a2870f5)) * ee ci for development ([97c31ba](97c31ba)) * test ee ci for release ([a7605af](a7605af)) ### Bug Fixes * Add 'parent_interface' parameter for l2/l3 subinterface modules ([PaloAltoNetworks#552](https://github.com/alperenkose/pan-os-ansible/issues/552)) ([73c28a8](73c28a8)) * new release for failed ci ([3872708](3872708)) * requirements.txt update python version and remove hashes ([905b1eb](905b1eb)) * **panos_facts.py:** Fixed virtual systems fact name ([PaloAltoNetworks#558](https://github.com/alperenkose/pan-os-ansible/issues/558)) ([0d0fd6d](0d0fd6d)) * **panos_security_rule:** state merged with existing values ([PaloAltoNetworks#570](https://github.com/alperenkose/pan-os-ansible/issues/570)) ([db6c32c](db6c32c))
This seems to have broken the fix from #314 and now all security rules show as if they had changes even when no changes were made. |
Hi @dmurarasu could you please send an example task and output of the issue? |
Hi @alperenkose, sorry, I probably should have raised an issue, details below ansible@a801c38859c8:/ansible$ ansible-galaxy collection install paloaltonetworks.panos I'm just running a few rules in this example, just to see the changed output; the rules are already in place but I'll run it twice to show it's the same every time. ansible@a801c38859c8:/ansible$ ansible-playbook -i local.inventory config_playbook.yml --tags=security_rules TASK [palo_configure : Create security rules] ************************************************************************************* PLAY RECAP ************************************************************************************************************************ ansible@a801c38859c8:/ansible$ ansible-playbook -i local.inventory config_playbook.yml --tags=security_rules TASK [palo_configure : Create security rules] ************************************************************************************* PLAY RECAP ************************************************************************************************************************ Just to show it was ok before ansible@a801c38859c8:/ansible$ ansible-galaxy collection install paloaltonetworks.panos:2.20.0 ansible@a801c38859c8:/ansible$ ansible-playbook -i local.inventory config_playbook.yml --tags=security_rules PLAY [palo] *********************************************************************************************************************** TASK [palo_configure : Create security rules] ************************************************************************************* PLAY RECAP ************************************************************************************************************************ When it detects changes all the time and I run it with verbosity the only changes that I can see in the before and after section: before: after: Commiting and rerunning just shows the same again. Full output below: ansible@a801c38859c8:/ansible$ ansible-playbook -i local.inventory config_playbook.yml --tags=security_rules -vvvv PLAYBOOK: config_playbook.yml ************************************************************************************************************************************************************* PLAY [palo] ******************************************************************************************************************************************************************************* TASK [palo_configure : Create security rules] ********************************************************************************************************************************************* changed: [pa01] => (item={'rule_name': 'GP Connection', 'source_zone': ['External_LAN'], 'source_ip': ['any'], 'destination_zone': ['External_LAN'], 'destination_ip': ['any'], 'application': ['ipsec', 'panos-global-protect', 'ssl'], 'service': ['application-default'], 'action': 'allow', 'description': 'Permit GP Gateway Login', 'group_profile': 'Inbound Network Security', 'tag': ['Global Protect'], 'location': 'after', 'existing_rule': 'Dynamic Block List Outbound'}) => { |
@dmurarasu could you please create an issue for this to be tracked with the |
Description
When
state: merged
is used withpanos_security_rule
for existing entries, it adds default values to existing configuration, like "any" being added to the list of source_zone, or source_ip. And similar issue exist for all the fields with default values in the module.This PR removes "default" values from
sdk_params
in module which results in None (null) values for non-provided arguments, and arguments with null values in the invocation are not updated on the device for updating an existing rule. Whereas if rule doesn't exist a new record is being created with default values provided in the newly introduceddefault_values
helper param. If no default values exist indefault_values
, it fallbacks to SDK default values, and if none found it is set to None.PR also introduces
preset_values
for module parameters, which are available fixed selections for list type parameters like "any" or "application-default" forservice
param. With the addition ofpreset_values
, it fixes updating the list parameters to or from the preset values or user defined entries.Motivation and Context
Fixes issue #564 but we will keep it open for a while when we have the change in development branch.
fixes #563, fixes #467
Also fixes #551 regarding panos_template mode xpath error on second run
How Has This Been Tested?
Tested manually on vmseries.
Similar issue exists for modules using "default" values in the
sdk_params
likepanos_zone
,panos_pbf_rule
or others which causes existing values to revert to default if not provided with the merged state. These will be fixed on a separate PR.Types of changes
Checklist