Create a generic FortiOS deploy-config task

[a-v-popov]

Introducing a generic FortiOS deploy-config task using config-script API to enable netlab config ....
Based on a comment here #2271 (reply in thread)

FortiOS feature documentation: https://docs.fortinet.com/document/fortigate/7.6.3/administration-guide/780930/configuration-scripts
Ansible module: https://ansible-galaxy-fortios-docs.readthedocs.io/en/latest/fortios_monitor.html

I had to update fortios.yml to stop normalize task from failing, although I am not sure what side effects this change might have.

[ipspace]

I had to update fortios.yml to stop normalize task from failing, although I am not sure what side effects this change might have.

This is not exactly a promising start of a PR 🤔 Whatever the root cause is, it has to be fixed, or we have to agree to migrate all FortiOS functionality to the new way of doing things.

@ssasso @sdargoeuves do you want to look at this? Does it make sense to go down this path? It might make things easier if we ever decide to implement more functionality, but I have no idea how well this would play with older releases (you know, the ones where the licensing was still reasonable).

[a-v-popov]

The initial provisioning is still done as it was, with ansible tasks, and it works for me. No migration was required.
I don't know why fortios was integrated with the two knobs and without device type, and it is probably beyond the scope of the PR. Potentially the answer might be here https://netlab.tools/dev/config/deploy/

My motivation was to enable these two features.
https://netlab.tools/custom-config-templates/
https://netlab.tools/netlab/config/
I couldn't find a way to do it through custom templates so I forked the repository and made the changes.
I might still need to make them part of some play though. But it looked promising, hence the RP.
I know FortiOS is not your primary target, so feel free to reject the PR.

[sdargoeuves]

I tested both the latest ⁠dev branch and the forked branch on two images: a freshly built ⁠7.0.3 (with the 15‑day license) and ⁠7.4.8. Both behave correctly with the OSPF module enabled and with ⁠firewall.zonebased configured.
The only hiccup -- and it’s a licensing tantrum rather than a bug -- was with ⁠7.0.3 when I left the ⁠vdom parameter: the 15‑day license does not allow multi‑VDOM, so Ansible failed when attempting to enable multi‑VDOM mode:

TASK [Enable multi-VDOM mode if a traffic VDOM is defined] ***************************************************************************************************************************************
fatal: [fw2]: FAILED! => changed=false
meta:
build: 237
cli_error: |-
multi-vdom mode cannot be enabled with the current vdom license.
node_check_object fail! for vdom-mode multi-vdom
[...]

I’m not making much use of ⁠config: [] at the moment -- only for Linux hosts. Our goal is to capture the network state at different stages, so we run multiple playbooks after the lab is up. That’s intentional for our use case, although I can see why others would want ⁠netlab to push configuration as part of bringing the lab up.

[ssasso]

IMHO, if @a-v-popov wants (and has time) to go down this path, it would be great to migrate all the existing config "actions" (initial, ospf, firewall) to this new methodology.

It would be great for the following reasons:

• consistency in config methods
• better "extra config", and config backup & restore (if i'm not wrong we still miss the fetch config?)
• allow people with no ansible knowledge to contribute to config templates, or even write additional plugins with less effort

[a-v-popov]

I think I clearly stated my motivation. I am not proposing or advocating in any way template-only configurations. In fact, I believe that the presence of "normalize" task running on all types of the devices just because two of them have stupid defaults clearly shows that the approach is not viable. It would also have saved me a lot of pain if custom config groups could take .yml instead of .j2.
The goal of the PR is to enable functionality mentioned in my previous post without rework of other components.

[ssasso]

I think I clearly stated my motivation.

I understood your motivation, and this PR is ok.

I was just giving my opinion of what I personally think it could be good as future improvement, also enabling other potential contributors to work on fortigate features.

[a-v-popov]

I amended the commit to address linter's complaints, hope it was the right thing to do.
Let me know if something else should have been done.

[ipspace]

I amended the commit to address linter's complaints, hope it was the right thing to do.

Yes it was. Thank you!

Let me know if something else should have been done.

I made a few changes to get back those two variables you removed, and to prevent crashes when a task has no corresponding template. We're losing a bit of a resilience with this setup, but as we have a single device that uses task lists to configure stuff, additional complications are simply not worth it.

Please check whether these changes work for you. If they do, I'll merge the PR.

[a-v-popov]

I made a few changes...

No other deploy-config has similar behavior, as far as I can tell, they will all fail if config_template is missing. Doesn't look like a step into the direction of unification to me.

I will return control to external playbooks in my setup. Sorry for the inconvenience.

[ipspace]

No other deploy-config has similar behavior, as far as I can tell

Because none of the other devices is ever configured though a task list. All of them are configured through configuration templates and (mostly) shared deployment task list.

they will all fail if config_template is missing.

No. We try to generate sane error messages instead of crashing. The "missing template" will be caught before the deployment task list is called.

Doesn't look like a step into the direction of unification to me.

Look, we had two deployment models:

• Configuration templates + template deployment task list (with minor variations like module-specific deployment)
• Deployment through device-specific Ansible modules

You effectively added the hybrid option, but in a way that would break (at least) custom config deployment using task lists but no templates because you removed those two parameters (with "I am not sure what side effects this change might have" comment, which is a red flag in itself). I know what effects that change might have, and breaking what worked before without a really good reason is unacceptable (to me, YMMV).

I tried to implement your idea with the minimum possible impact on what's already out there. An alternative would be to modify the core Ansible playbooks/task lists to support the hybrid option, and that's probably not worth the effort for the minor inconvenience of a slightly different behavior of a single device.

Anyway, let me get this straight: you're not saying "your proposed change does not work" but "I don't like it"?

I will return control to external playbooks in my setup. Sorry for the inconvenience.

I would be OK with merging the final version that just adds the "deploy-config" task list, but it's your choice 🤷‍♂️

[a-v-popov]

You effectively added the hybrid option...

No, I didn't. It is described in your documentation referenced above.

..., but in a way that would break (at least) custom config deployment using task lists but no templates...

This should be expected, read the big red warning in your own documentation.
I haven't seen any complaints on this PR, and I tend to think that anyone following documentation should have no issue.

YMMV

OK. On my mileage, either your 'core Ansible playbooks' or your documentation, or potentially both, are broken.
It was not a red flag, it was an explicit and direct warning to the maintainer, that additional work might be required.

Anyway, let me get this straight: you're not saying "your proposed change does not work" but "I don't like it"?

I am saying that I will be using external tools to achieve what I need without this PR.
It is your choice as the maintainer, if you want the functionality or not in your project.
For me the PR is closed.