not all modules are idempotent

[fredericve]

Not all of the modules are idempotent. E.g. when I enable bgp with panos_bgp, then configure a peer group with panos_bgp_peer_group, and then add a bunch of peers with panos_bgp_peer, and then run the same playbook again, the panos_bgp task will remove the peers and peer_group, resulting in a changed state instead of ok

Expected behavior

Run a playbook with these tasks twice:

  - name: Configure BGP on a Virtual Router
    panos_bgp:
      provider: '{{ provider }}'
      state: present
      vr_name: default
      router_id: "{{ router_id }}"
      local_as: "{{ local_as }}"
      install_route: True
      reject_default_route: False
      commit: false

  - name: Configure BGP Peer Group
    panos_bgp_peer_group:
      provider: '{{ provider }}'
      state: present
      name: "{{ peer_group_name }}"
      vr_name: default
      commit: False

  - name: Configure BGP Peer
    panos_bgp_peer:
      provider: '{{ provider }}'
      state: present
      name: "{{ peer_name }}"
      enable: true
      peer_as: "{{ peer_as }}"
      local_interface_ip: "{{ peer_local_if_ip }}"
      local_interface: "{{ peer_local_if }}
      peer_address_ip: "{{ peer_addr_ip }}"
      peer_group: "{{ peer_group_name }}"
      vr_name: default
      commit: False

The second run each task should display ok

Current behavior

When run a second time, all tasks display changed

Possible solution

I believe the source of the problem is in module_utils/network/panos/panos.py apply_state function. In there the generated object only includes child types that exist in the new version of the object instead of leaving the existing children untouched.

Your Environment

• Version used:
• Ubuntu 18.04, with pandevice 0.14.0

[dstogsdill]

I'm seeing this behavior too, however, it looks like this module doesn't use the 'apply_state' function as suggested. Rather it's relying on the pan object 'equal' function from the the base.py pan-os-python package.

            if not item.equal(virtual_router, compare_children=False):
                changed = True
                virtual_router.extend(item.children)

This looks to be doing a straight string comparison of the XML representation of the objects. unfortunately if there is a change to the device configuration outside of this module (say assigning a virtual router to an interface using panos_l3_subinterface) then this will ALWAYS incorrectly detect a change.

edit #1: Additionally the VirtualRouter object has an 'Interface' var that needs to be accounted for otherwise the assigned interfaces get removed upon applying the change. Right now the module only extends for the 'children' var to replicate any existing configurations.

edit #2: I just realized this issue refers to several BPG modules and not the panos_virtual_router module which my comment refers to. I'll open a new new issue to apply a bug fix for the panos_virtual_router module

I'm working on a fix for this but unsure how to assign this issue/bug to myself.

[chancez]

panos_log_forwarding_profile_match_list isn't idempotent either, yet it but it does use apply_state, so apply_state isn't perfect either.

[dstogsdill]

@chancez You are correct. the apply_state function mutates the object during iteration so any module calling this function will pretty much always register a change. Additionally, the function does not account for any objects with interfaces so these objects will also register a change if not accounted for before calling the function.

[dstogsdill]

There is another specific issue with the panos_bgp_peer module in addition to the underlying apply_state function. the PANOS xml api sets defaults to specific fields (i.e. keep-alive-interval and min-route-adv-interval). If these fields are omitted from your playbook the module will continue to detect a change.

You can work around this by explicitly setting all fields in this module.

I believe the permanent fix would be for the module to assign default values to these fields.

[mrichardson03]

The problem with idempotence here is the child objects attached to the virtual router. panos_virtual_router should really write all child objects itself, rather than only doing part of the configuration and then having multiple other modules then modify that VR. One way to do this would be to move all the BGP configuration into panos_virtual_router, which would make some sense because BGP configuration can't exist outside of a VR.

Another way to do this (and a lot of other modules) better in my opinion, is to have an idempotent way to manipulate the XML config. I have code that does this, and I opened #219 to show how it works and can be used.

[shinmog]

Another idempotency fix was added, this should be resolved now in the next release.