"Simulated" vyos configuration parsing

[lucaelin]

SUMMARY

I added a new 'smart' match type to the vyos_config module. It aims to provide better configuration diffing and more reliably provisioning of devices to a given state. This is accomplished by using a new VyOS configuration simulation class. It is useful when using templated configurations while still maintaining a minimal list of changes in the Ansible output.

Fixes #179

ISSUE TYPE

• Bugfix Pull Request
• Feature Pull Request

ADDITIONAL INFORMATION

The new diffing algorithm is enabled by setting the match argument in the vyos_config module to smart.
I also changed the encryped password filter to require an exclamation mark at the end. With the new diffing algorithm the value "!" ... has the special meaning "keep all the existing keys as is". Please let me know if this is a reasonable change to make!
I wasn't able to run the full test suit with my changes in place because I have little experience with python and python tooling. Because of this, feel free to call out inaccuracies of potential issues with the code that I didn't spot.

[lucaelin]

Looking at the test failures - I misunderstood the purpose of the ! when setting the encrypted password. I thought of it as a "not shown here for security reasons", but apparently it is used by vyos to indicate "no password" because I can see the hashes when a password is set.
I am not entirely sure how to proceed with this. I re-enabled the original encrypted-password command filter, but maybe the char "!" is not ideal for use as a "keep all the existing keys as is" indicator.

[lucaelin]

I settled with a special value of ... to keep preexisting siblings and values. I will create a separate issue for being unable to set the encrypted-password.

[pabelanger]

recheck

[lucaelin]

recheck

[lucaelin]

I'm sorry, I cannot figure out why the pipeline fails the way it does with my changes, maybe some side-effect that I am completely unaware of. It did pass after pushing my last commit, but apparently not so much anymore...

[GomathiselviS]

@lucaelin Thanks for working on this. Sorry for the late response. We are reviewing the PR and shall update you with our comments next week.

[NilashishC]

recheck

[bjw-s]

It would be really great if this could find its way into the module, it would greatly improve the usefulnesses for us!

[GomathiselviS]

Hi @lucaelin,

Sorry for the very delayed response. I pulled the changes and tried out a playbook. The behavior seems similar to state: overridden of vyos resource modules. Have you tried that? As far as I understand, with match = smart, the configs given in the playbook are configured and the rest of the configs, that come under the service/protocol/resource, that is mentioned in the playbook are removed. Please correct me if I have understood the use case wrongly. Please provide examples in the PR to bring more clarity.

[lucaelin]

Hey @GomathiselviS,

The behavior seems similar to state: overridden

the match=smart can act the way that overridden does for resource modules, yes. This PR covers the missing functionality of state: overridden for vyos_config, but it also can be used beyond that (more on that further down).

Have you tried that?

I think that the resource modules have a very different workflow or approach than the config module does, so using resource modules instead of directly deploying the configuration as a whole is not much of a viable approach for me. I also think that this discussion is beyond the scope of this PR and more of a discussion about the usability of the config module in general.

the rest of the configs, that come under the service/protocol/resource, that is mentioned in the playbook are removed

They are removed, if the provided configuration does not contain a ... entry at the same level as the entries that should be kept.

... # this line keeps all entries like firewall, interface, system
service {
    ... # this line keeps dhcp, dns
    # but the next block ensures that any configuration to the ssh entry is removed and only disable-password-authentication remains
    ssh {
        disable-password-authentication
    }
}

More importantly, the smart option improves idempotency. The order in which commands appear is irrelevant and the implemented algorithm ensures that the minimal amount of changes is being made on the target in order to reach the desired state. This can be helpful in commit histories on the target and in check-mode.
Additionally, the implemented algorithm is agnostic of the configuration options that are available on the target. This ensures that new features/options added to the OS can be used without changes to this module.

Please provide examples in the PR to bring more clarity.

I will do so as soon as I can, I hope my explanations above help to outline my motivation for this PR.

[GomathiselviS]

@lucaelin Thanks for the detailed explanation. Please add examples, so that I can take this PR to my team and get their feedback. Meanwhile, I tried the following with your changes

- vyos.vyos.vyos_config:
    lines:
     - set system host-name vyos-1.1.8
     - set service lldp
    match: smart

The result was

"delete interfaces",
        "delete service ssh",
        "delete system config-management",
        "delete system console",
        "delete system host-name vyos",
        "delete system login",
        "delete system ntp",
        "delete system package",
        "delete system syslog",
        "delete system time-zone",
        "set service lldp",
        "set system host-name vyos-1.1.8"

The hostname config is deleted and set. IMO, the delete operation can be skipped, when we have the same parameter in want and have with different values. What do you think about it?

I also feel that 'smart' is not the proper name when the operation this does is to replace the device configs with given configs.

[lucaelin]

the delete operation can be skipped,

That is correct for the host-name, but not for every property. E.g. the address property for an ethernet interface. While it would certainly be possible to add knowledge about each individual property to the code, this also greatly increases complexity and requires continuous maintenance with versions changing and would also cause incompatibilities with custom extensions that are installed on the target system. IMHO, these downsides outweigh the benefits of an even cleaner diffing.

'smart' is not the proper name

I agree. But this option is not the only option aiming to replace the config. The match: lines option also partially does this. The main difference between lines and smart is the amount of work that is being done by the client to establish idempotency and reproducibility. One alternative that comes to my mind to smart would be parsed.

[cubic3d]

Thank you @lucaelin for extending this to improve idempotency. I have currently to generate delete statements for all branches before the actual set statements to avoid dangling pieces of config, which leads to very long runtimes and would really like to see this merged.

Is there something blocking @GomathiselviS to get this rolling?