# Ansible code review

**URL:** <https://forum.ansible.com/t/ansible-code-review/10116>\
**Category:** Get Help\
**Created:** [October 9, 2024, 3:07pm UTC](https://forum.ansible.com/t/ansible-code-review/10116 "2024-10-09T15:07:34Z")\
**Posts on this page:** 5\
**Page:** 1

<div class="post-metadata">

**Author:** ![mroe1234](https://avatars.discourse-cdn.com/v4/letter/m/96bed5/32.png) [@mroe1234](https://forum.ansible.com/u/mroe1234)\
**Post date:** [October 9, 2024, 3:07pm UTC](https://forum.ansible.com/t/ansible-code-review/10116/1 "2024-10-09T15:07:34Z")

</div>

I am looking for some feedback/advice/suggestions for my ansible playbook[s]. I included them below in a hopefully useful format. Here is also a directory structure of the files. These playbooks dynamically manage firewall rules in production, so I’m looking for another set of eyes before I push this into production.

```auto
.
├── ansible.cfg~
├── docs
│ └── fem_ansible.png
├── fwmgmt.yml
├── inventory
├── README.md
├── roles
│ ├── fortinet_fw
│ │ ├── defaults
│ │ │ └── main.yml
│ │ ├── files
│ │ │ └── iana_reversed.csv
│ │ ├── handlers
│ │ │ └── main.yml
│ │ ├── meta
│ │ │ └── main.yml
│ │ ├── README.md
│ │ ├── tasks
│ │ │ ├── address.yml
│ │ │ ├── main.yml
│ │ │ ├── policy.yml
│ │ │ ├── servicegroup.yml
│ │ │ └── service.yml
│ │ ├── templates
│ │ ├── tests
│ │ │ └── inventory
│ │ └── vars
│ │ └── main.yml
│ └── nb_graphql
│ ├── defaults
│ │ └── main.yml
│ ├── files
│ ├── filter_plugins
│ │ ├── custom_filters.py
│ │ └── __pycache__
│ ├── handlers
│ │ └── main.yml
│ ├── meta
│ │ └── main.yml
│ ├── README.md
│ ├── tasks
│ │ └── main.yml
│ ├── templates
│ ├── tests
│ │ └── inventory
│ └── vars
│ └── main.yml

```

````auto
File: ./fwmgmt.yml
---
- name: Get data from nb
  hosts: firewalls
  gather_facts: false
  vars:
    slack_token: "{{ lookup('file', '/opt/firewall/slackkey') }}"
  roles:
    - role: nb_graphql
    - role: fortinet_fw
      policy: "{{ rule.json.data.policy_rules[0] }}"
      before: "{{ rule.json.data.policy_rules[0].policies[0].policy_rules |
                  community.general.json_query('[].index')|
                  find_nearest_elements(target=rule.json.data.policy_rules[0].index) }}"

File: ./roles/nb_graphql/tasks/main.yml
---
# Tasks file for roles/nb_graphql

- name: Policy from Nautobot
  check_mode: false
  diff: false
  ansible.builtin.uri:
    validate_certs: false
    url: "https://{{ policy_data }}"
    method: POST
    follow_redirects: all
    headers:
      Authorization: "Token {{ api_key }}"
      Accept: "application/json; indent=4"
    body_format: json
    body:
      variables:
        policyname: "{{ policy_name }}"
  delegate_to: localhost
  register: rule

- name: To Slack
  check_mode: false
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    msg: "{{ policy_name }}"
  register: slack_response
  delegate_to: localhost

- name: Details to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "Nautobot Object: ```{{ rule.json.data.policy_rules[0] | to_nice_json }}```"
  delegate_to: localhost

File: ./roles/nb_graphql/defaults/main.yml
---
# defaults file for roles/nb_graphql
nautobot_host: nautobot.cc.lehigh.edu
# corresponds with the saved query in nautobot
policy_data: "{{ nautobot_host }}/api/extras/graphql-queries/bee72043-be67-4c58-8930-c08f536c9bd5/run/"
api_key: "{{ lookup('file', '/opt/firewall/nbkey') }}"

File: ./roles/nb_graphql/handlers/main.yml
---
# handlers file for roles/nb_graphql

File: ./roles/nb_graphql/vars/main.yml
---
# vars file for roles/nb_graphql

File: ./roles/fortinet_fw/tasks/servicegroup.yml
---
# Task to bundle services together into a service group

- name: Set firewall service group
  vars:
    members: "{{ item.service_objects | community.general.json_query('[].{name: name}') }}"
  fortinet.fortios.fortios_firewall_service_group:
    vdom: "{{ vdom }}"
    state: "{{ item.status.name | regex_search('Active|Disabled') and 'present' or 'absent' }}"
    access_token: "{{ access_token }}"
    firewall_service_group:
      comment: "{{ item.comment | default('') }}"
      name: "{{ item.name }}"
      member: "{{ members }}"
  with_items: "{{ servicegroup }}"
  register: msg

- name: Details to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "ServiceGroup Update: ``` - {{ msg.msg }}\n - Changed: {{ msg.changed }}```"
  delegate_to: localhost
File: ./roles/fortinet_fw/tasks/service.yml
---
- name: Set firewall service
  vars:
    # Hack to lookup the IP protocol number since nb doesn't provide yet
    # https://github.com/nautobot/nautobot-plugin-firewall-models/issues/169
    ip_protocol_number: "{{ lookup('ansible.builtin.csvfile', item.ip_protocol + ' file=../files/iana_reversed.csv delimiter=,') | default('') }}"
  fortinet.fortios.fortios_firewall_service_custom:
    vdom: "{{ vdom }}"
    state: "{{ item.status.name | regex_search('Active|Disabled') and 'present' or 'absent' }}"
    access_token: "{{ access_token }}"
    firewall_service_custom:
      comment: "{{ item.comment | default('No Comment') }}"
      name: "{{ item.name }}"
      # Weird syntax for matching incoming text and setting protocol field based on it.
      # I don't totally understand how it works to be honest
      protocol: "{{ item.ip_protocol | regex_search('TCP|UDP') and 'TCP/UDP/SCTP' or item.ip_protocol | regex_search('ICMP') and 'ICMP' or 'IP' }}"
      protocol_number: "{{ ip_protocol_number }}"
      tcp_portrange: "{{ item.ip_protocol | replace('TCP', item.port) | default('') }}"
      udp_portrange: "{{ item.ip_protocol | replace('UDP', item.port) | default('') }}"
  with_items: "{{ services }}"
  register: msg

- name: Details to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "Service Update: ``` - {{ msg.msg }}\n - Changed: {{ msg.changed }} ```"
  delegate_to: localhost
File: ./roles/fortinet_fw/tasks/main.yml
---
# Ordered list of playbooks to execute.

- name: Create address task
  collections:
   - fortinet.fortios
  connection: httpapi
  vars:
   addresses:
   # Doesn't support looping multiple addresses... yet
    - "{{ policy.destination_addresses[0] }}"
    - "{{ policy.source_addresses[0] }}"
  ansible.builtin.import_tasks: address.yml

- name: Create services
  collections:
   - fortinet.fortios
  connection: httpapi
  vars:
   # Doeesn't support looping multiple service_groups... yet
   services: "{{ policy.destination_service_groups[0].service_objects }}"
  ansible.builtin.import_tasks: service.yml

- name: Create service groups
  collections:
   - fortinet.fortios
  connection: httpapi
  vars:
   servicegroup: "{{ policy.destination_service_groups }}"
  ansible.builtin.import_tasks: servicegroup.yml

- name: Create policy
  collections:
   - fortinet.fortios
  connection: httpapi
  ansible.builtin.import_tasks: policy.yml

File: ./roles/fortinet_fw/tasks/address.yml
---
# Ansible Playbook to create or delete firewall addresses on a Fortinet device

- name: Address management
  fortinet.fortios.fortios_firewall_address:
    vdom: "{{ vdom }}"
    # If the state of the object is set to anything other than 'Active' or 'Disabled'
    # the object will be deleted.
    state: "{{ item.status.name | regex_search('Active|Disabled') and 'present' or 'absent' }}"
    access_token: "{{ access_token }}"
    # Define the firewall address parameters
    firewall_address:
      # Use the comment field from the item or provide a default if not specified
      comment: "{{ item.comment | default('') }}"
      name: "{{ item.name }}"
      # Set the CIDR for the firewall address using the IP address from the item
      subnet: "{{ item.ip_address.address }}"
      type: ipmask
  # Loop through all of the items in the addresses object.
  with_items: "{{ addresses }}"
  register: msg

- name: Details to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "Address Update: ``` - {{ msg.msg }}\n - Changed: {{msg.changed}}```"
  delegate_to: localhost

File: ./roles/fortinet_fw/tasks/policy.yml
---
# Task to set a new firewall policy rule based on a given data structure

- name: Firewall policy
  vars:
    # Policy ID is based on the index value set in data
    policyid: "{{ policy.index }}"
  fortinet.fortios.fortios_firewall_policy:
    vdom: "{{ vdom }}"
    # If status is either Active or Disabled, state = present, otherwise it is absent
    state: "{{ policy.status.name | regex_search('Active|Disabled') and 'present' or 'absent' }}"
    access_token: "{{ access_token }}"
    firewall_policy:
      policyid: "{{ policyid }}"
      # Mapping ALLOW to accept and DENY to deny
      action: "{{ policy.action | replace('ALLOW', 'accept') | replace('DENY', 'deny') }}"
      comments: "{{ policy.comment | default('') }}"
      name: "{{ policy.name }}"
      # Fun json query to extract the name fields from complex objects
      dstaddr: "{{ policy.destination_addresses | community.general.json_query('[].{name: name}') }}"
      srcaddr: "{{ policy.source_addresses | community.general.json_query('[].{name: name}') }}"
      service: "{{ policy.destination_service_groups | community.general.json_query('[].{name: name}') }}"
      dstintf:
        - "{{ policy.destination_zone.name | community.general.dict_kv('name') }}"
      srcintf:
        - "{{ policy.source_zone.name | community.general.dict_kv('name') }}"
      schedule: "always"
      # Using status field again here to map Disabled to disable
      status: "{{ policy.status.name | regex_search('Disabled|Expired') and 'disable' or 'enable' }}"
  register: msg

- name: Details to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "Policy Invocation: ```Changed: {{ msg.changed }}```"
  delegate_to: localhost

- name: Debug
  ansible.builtin.debug:
    var: policy.action

- name: Move rule
  # Task to move a rule to the top of the list if its action is 'Deny'
  # Only runs when action is set to Deny
  fortinet.fortios.fortios_firewall_policy:
    access_token: "{{ access_token }}"
    vdom: "{{ vdom }}"
    action: move
    self: "{{ policy.index }}"
    before: "{{ before }}"
  when: policy.action == 'DENY'
  register: status

- name: Summary to Slack
  community.general.slack:
    token: "{{ slack_token }}"
    channel: "#netops"
    thread_id: "{{ slack_response['ts'] }}"
    msg: "Move Details: ``` Changed: {{ status.changed }}```"
  delegate_to: localhost

File: ./roles/fortinet_fw/defaults/main.yml
---
# defaults file for roles/fortinet_fw
ansible_network_os: fortinet.fortios.fortios
ansible_httpapi_use_ssl: true
ansible_httpapi_validate_certs: false
ansible_httpapi_port: 443
vdom: FG-traffic

File: ./roles/fortinet_fw/handlers/main.yml
---
# handlers file for roles/fortinet_fw

File: ./roles/fortinet_fw/vars/main.yml
---
# vars file for roles/fortinet_fw
access_token: "{{ lookup('file', policy.policies[0].assigned_devices[0].secrets_group.secrets[0].parameters.path) }}"

````

---

<div class="post-metadata">

**Author:** ![utoddl](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/utoddl/32/249_2.png) [@utoddl](https://forum.ansible.com/u/utoddl)\
**Post date:** [October 10, 2024, 2:22am UTC](https://forum.ansible.com/t/ansible-code-review/10116/2 "2024-10-10T02:22:37Z")

</div>

> [@mroe1234](#):
>
> ```auto
> # Weird syntax for matching incoming text and setting protocol field based on it.
> # I don't totally understand how it works to be honest
> protocol: "{{ item.ip_protocol | regex_search('TCP|UDP') and 'TCP/UDP/SCTP' or item.ip_protocol | regex_search('ICMP') and 'ICMP' or 'IP' }}"
> 
> ```

I had some serious doubts about that line myself. But a little testing made it clear.

```
{{ true and 'WOW' }} ⇒ 'WOW'
{{ false and 'WOW' }} ⇒ False
{{ true or 'WOOPS' }} ⇒ True
{{ false or 'WOOPS' }} ⇒ 'WOOPS'

```

That combined with the fact that “`|`” has higher precedence than “`and`”, and “`and`” has higher precedence than “`or`” lets you work out how it works. (Although, apparently, Jinja operator precedence [is not actually defined](https://github.com/pallets/jinja/issues/379).)

That’s the only thing that jumped out and me, and it turns out to be right. ✅

---

<div class="post-metadata">

**Author:** ![mroe1234](https://avatars.discourse-cdn.com/v4/letter/m/96bed5/32.png) [@mroe1234](https://forum.ansible.com/u/mroe1234)\
**Post date:** [October 10, 2024, 1:34pm UTC](https://forum.ansible.com/t/ansible-code-review/10116/3 "2024-10-10T13:34:49Z")

</div>

Yea, that solution was provided to me by my trusty intern: ChatGPT. The two items that I specifically don’t like about my code is:

1. the duplication of the Details to Slack play in almost every playbook. I kinda would have preferred to call a “function” of some sort rather than copying and pasting the same play all over the place. But I couldn’t figure out how to do that especially since the plays are in different roles
2. in the `Create address task`, I couldn’t figure out how to loop through the array so I had to hardcode the first element and only support a single source and destination address

---

<div class="post-metadata">

**Author:** ![utoddl](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/utoddl/32/249_2.png) [@utoddl](https://forum.ansible.com/u/utoddl)\
**Post date:** [October 10, 2024, 4:22pm UTC](https://forum.ansible.com/t/ansible-code-review/10116/4 "2024-10-10T16:22:21Z")

</div>

Your terminology is, er, rather loose. You keep talking about all these playbooks, but you only have one playbook: `fwmgmt.yml`. Rule of thumb: if it doesn’t have a `hosts:` key at the top level, it isn’t a playbook.

All those other files you’re calling “playbooks” are “task files,” and the things within them aren’t “plays,” they’re “tasks.” I don’t mean to hit you with the pedantic hammer, but you _really_ want to get these terms right. Otherwise you’ll have a hard time interpreting the docs or communicating with the rest of us.

A quote, with edits:

> [@mroe1234](#):
>
> 1. the duplication of the “`Details to Slack`” ~~play~~ **task** in almost every ~~playbook~~ **task file**. I kinda would have preferred to call a “function” of some sort rather than copying and pasting the same ~~play~~ **task** all over the place. But I couldn’t figure out how to do that especially since the ~~plays~~ **tasks** are in different roles

That would be a problem if it were multiple tasks. The fix for that would be to make it into its own role, and you’d invoke it with `ansible.builtin.include_role` with appropriate variables etc. But that’s still a task invocation just to include the role. Since it’s already a single task, there’s no way (that I see anyway) to reduce the duplication or overall footprint of those Slack notifications.

> [@mroe1234](#):
>
> 1. in the `Create address task`, I couldn’t figure out how to loop through the array so I had to hardcode the first element and only support a single source and destination address

Just as a refresher (so folks won’t have to scroll around to follow the discussion, including me), here’s that task:

```yaml
- name: Create address task
  collections:
   - fortinet.fortios
  connection: httpapi
  vars:
   addresses:
   # Doesn't support looping multiple addresses... yet
    - "{{ policy.destination_addresses[0] }}"
    - "{{ policy.source_addresses[0] }}"
  ansible.builtin.import_tasks: address.yml

```

You can loop over `policy.destination_addresses` paired with corresponding `policy.source_addresses` like this:

```yaml
- name: Create address task
  collections:
    - fortinet.fortios
  connection: httpapi
  ansible.builtin.import_tasks: address.yml
  loop: "{{ policy.destination_addresses | zip(policy.source_addresses) }}"
  loop_control:
    loop_var: addresses

```

The `zip()` filter will create paired destination and source addresses. By changing the `loop_var` from the default of `item` to `addresses`, that conveniently creates the variable that your `address.yml` task file expects. Also, by not using `item` as the `loop_var`, it keeps `item` free for use by tasks in the tasks file.

I also changed your 1- and 2-space indentation to a consistent 2-space indentation. You should run `ansible-lint` on your project; it’ll suggest the same thing. And a whole lot more I’m sure. It has _opinions_ about Ansible code.

---

<div class="post-metadata">

**Author:** ![mroe1234](https://avatars.discourse-cdn.com/v4/letter/m/96bed5/32.png) [@mroe1234](https://forum.ansible.com/u/mroe1234)\
**Post date:** [October 10, 2024, 4:44pm UTC](https://forum.ansible.com/t/ansible-code-review/10116/5 "2024-10-10T16:44:37Z")

</div>

Pedantry in our field is very important. So thank you for helping me clarify. Part of wanting to reduce duplication is to make it a little easier to, for instance, change the destination channel. I will look into moving it into its own role, or maybe pulling pertinent information out of the ‘task’ and into variables to be referenced.

And again thank you for your tips with the looping. I am going to have to dissect that a bit to wrap my brain around it.