feat(panos-upgrade-assurance): upgrade assurance modules introduction

[FoSix]

Description

Introducing Ansible modules that wrap the PanOS Upgrade Assurance package.

Motivation and Context

The PanOS Upgrade Assurance package is just a set of libs that have to be wrapped with some code. One way of doing this are Ansible modules. This PR introduces 4 modules:

• panos_readiness_checks - runs readiness checks on a device
• panos_state_snapshot - take a snapshot of a device's state
• panos_snapshot_report - produces a report from a comparison of two snapshot in a form of a dictionary
• panos_active_in_ha - a helper module to quickly verify if a given node in an HA pair is active or passive.

Since the package itself is based on pan-os-python, these modules follow the provider block standard used widely in this collection.

How Has This Been Tested?

Already used on production as local modules.

Screenshots (if appropriate)

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist

• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes if appropriate.
• All new and existing tests passed.

[adambaumeister]

@shinmog thoughts on this one? Is there anything else you need the team to look at?

[thomaschristory]

Hello,

When will this be merged into main ?

[shinmog]

Universal feedback for the modules.

• All version_added values inside DOCUMENTATION blocks need to be updated, as multiple Ansible collection versions have been released since this PR was opened (sorry for the late feedback).
• Remove get_firewall_proxy_object() from all of the modules, and instead create the FirewallProxy from helper.device after invoking helper.get_pandevice_parent. helper.get_pandevice_parent() does other useful things such as minimum pan-os-python versions, minimum PAN-OS versions, python libraries, and all kinds of things. Most importantly, having get_firewall_proxy_object() like this that directly accesses authentication credentials like this duplicates the authentication credential retrieval that we need to move away from as a collection. These days, authentication credentials for Ansible collections comes from environment variables, which I think are configured in the inventory file..? Anyways, use the helper object to deal with authentication credentials is the point.
• If you guys decide that, yes, you do in fact want to keep the panos_upgrade_assurance code separate as opposed to just adding it directly in ./plugins/module_utils/ somewhere or in pan-os-python itself, you're going to need to 1) mandate that releases of panos_upgrade_assurance follow semver standards, just like pan-os-python does, and 2) add a new version checking hook for the version of panos_upgrade_assurance to the helper object. To not break backwards compatibility for people who won't be using the code, the version check should default to None and not worry about if panos_upgrade_assurance is an installed library or not. If the version number is defined, then make sure that panos_upgrade_assurance is present, and that the version is at least what is defined (refer to the version checking for pandevice / pan-os-python within the ./plugins/module_utils/panos.py for the current helper logic and build off of that).
• The ./tests/sanity/ignore* files have all been changed in recent versions, so you'll have to redo the new ignores for the files that are present.
• The CI has changed a lot since this PR was opened, so other things may need to be changed to satisfy all the new requirements collections must follow. So just a heads up that there may be CI failures on syntax, and those have to be fixed.

[shinmog]

It's reasons like this why having the upgrade assurance stuff as completely separate from this collection is problematic.

It's a bad user experience to read documentation to be referred to other documentation for potential values. And the waters get even muddier when a user's installed version of the upgrade assurance stuff is out of sync with what is live, and the values have changed.

These are not insurmountable issues, but you'll need to be ready for when it inevitably happens.

[FoSix]

true, but panos-upgrade-assurance is not for Ansible only, XSOAR can use it as well. Or just some pure Python script. Maybe more reasonable would be to keep it as part of the pan-os-python package?

[shinmog]

Ah, ok. Separate it is, then! So you'll want to add a new parameter to ./plugins/module_utils/panos.py > get_connection() that allows you to do version checking against panos_upgrade_assurance.version the same as happens for both PAN-OS itself and pan-os-python / pandevice. Please have the logic of this new check look like this:

if self.min_panos_upgrade_assurance is not None:
    try:
        import panos_upgrade_assurance
    except ImportError:
        module.fail_json(msg='Missing required library "panos_upgrade_assurance".', syspath=sys.path)
    # This code assumes both panos_upgrade_assurance.version and self.min_panos_upgrade_assurance
    # are a tuple of 3 ints.  If panos_upgrade_assurance.version is a string, then you'll have
    # to turn it into a 3 element tuple of ints to do the comparison.
    if panos_upgrade_assurance.version < self.min_panos_upgrade_assurance:
        module.fail_json(msg=MIN_VERSION_ERROR.format(
            "panos_upgrade_assurance",
            _vstr(panos_upgrade_assurance.version),
            _vstr(self.min_panos_upgrade_assurance),
        ))

[adambaumeister]

@shinmog I am thinking we can use a super call in the FirewallProxy class to get the best of both worlds. Subclassing FirewallProxy makes life easy so what I'm thinking is something like (psuedocode here)

firewall = helper.get_pandevice_parent().device
firewall_proxy = FirewallProxy(firewall)

# ... in assurance library ...

Class FirewallProxy(Firewall):
    def __init__(self, firewall_object: Optional[Firewall] = None, *args, **kwargs):
        """
        """
        if firewall_object:
            super(FirewallProxy, self).__init__(
                api_key=firewall_object.xapi.api_key,
                api_password=firewall_object.xapi.api_password,
                api_username=firewall_object.xapi.api_username,
                serial=firewall_object.xapi.serial,
                *args,
                **kwargs
            )
        else:
            super(FirewallProxy, self).__init__()

Can you see any reason this wouldn't work?

[FoSix]

finally FirewallProxy has both options, so it can take the api-* parameters or an existing device object. Changes will be introduced in upgrade assurance with this pr.

This way we can leave the upgrade assurance as a standalone package and still use the helper class inside Ansible modules.

If someone has any objections, please leave a comment in the upgrade assurance PR.

[shinmog]

Remove this file. The existance of a poetry.lock file breaks the CI and prevents us from testing against multiple Ansible versions.

[shinmog]

Are these two changes necessary for the upgrade assurance stuff?

[FoSix]

There was a reason why this was introduced, but it was so long ago....

I've tested it again and we can skip it so probably the issues got resolved along the way (actually the python = "^3.8.1" requirement came from the fact that we used a recent version of flake8, version 6 and up).

[shinmog]

Just to be clear, this PR needs to be rebased against current develop so that the new CI can run all the new requirements RedHat has in place.

[shinmog]

Few more issues:

• There are multiple changes to files outside the scope of this PR (aka - panos_query_rules.py, for example, which is causing a merge conflict). Please undo the changes to unrelated files.
• It seems the upgrade assurance stuff is specific to firewall. If Panorama is not supported, then please 1) add a note to the notes section saying Panorama is not supported and 2) pass in the desired error message
  as the panorama_error flag to the helper.

[alperenkose]

@FoSix I think we should avoid touching files non-related to the PR

[FoSix]

@alperenkose those file were modified by black, not by me. If I wouldn't fix them, the CI would fail. I don't think I had an option here.

[FoSix]

frankly speaking, I do not know how this work, but for example the module_utils/panos.py is missing a trailing new line causing sanity checks to fail. Did the linting rules change? Maybe. But in the end, I'm the one that have to introduce that change, even though it's not directly related to my code.

or we can stop here, do a separate PR, fix all the things that do not pass a sanity test - wait for review - merge - rebase this PR - run sanity.... but I'm affraid by this time something else would change and the PR would never get merged

[alperenkose]

We need to add this in the "Arguments:" section in method docstring.

[alperenkose]

We have added another parameter ignore_non_functional to this method but we can leave it for a separate PR.

[alperenkose]

Why don't we have the PUA_AVAILABLE logic here as in the panos_snapshot_report.py?

[FoSix]

no, it's in the helper class, panos_snapshot_report does not use the helper class, so the logic had to be moved to the module directly

[alperenkose]

Why don't we have the PUA_AVAILABLE logic here as in the panos_snapshot_report.py?

[FoSix]

same here

[alperenkose]

There is a new file for version 2.16 - ignore-2.16.txt we need to update that as well

[FoSix]

I'm closing this PR in favour of a new one, made from a branch, not a fork, with an up to date code (passing sanity tests at least partially)

all the suggestions from this PR were taken into consideration (unless irrelevant)