fix: Fix "helpers" service option

Cause: The query check was inverted, and query/add used the wrong API (module,
not helper).

Consequence: Trying to set the "helpers:" option of a created service
had no effect.

Fix: Use the correct API and fix the check. Add an integration test
which actually validates the created service in firewalld.

The service helper API does not yet exist on RHEL 7, so skip the
corresponding tests on RHEL < 8.

Fixes #276

Author: martinpitt

library/firewall_lib.py

@@ -488,9 +488,9 @@ def set_service(
                             fw_service_settings.addSourcePort(_port, _protocol)
                         self.changed = True
                 for _module in helper_module:
-                    if fw_service_settings.queryModule(_module):
+                    if not fw_service_settings.queryHelper(_module):
                         if not self.module.check_mode:
-                            fw_service_settings.addModule(_module)
+                            fw_service_settings.addHelper(_module)
                         self.changed = True
                 if destination_ipv4:
                     if not fw_service_settings.queryDestination(

tests/tests_ansible.yml

@@ -587,25 +587,49 @@
 
         - name: Add custom service with all the elements at once
           firewall_lib:
+            permanent: true
+            state: present
             service: customservice
             short: Custom service
             port: 333/tcp
             source_port: 333/tcp
             protocol: ipv6
-            helper_module: nf_conntrack_ftp
             description: Custom Service that does not do a single thing
             destination:
               - 123.45.6.78
               - "aaaa:aaaa:aaaa:aaa:aaaa:aaaa:aaaa::"
+            # these two don't exist yet in RHEL 7
+            helper_module: "{{ 'nf_conntrack_ftp'
+              if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+              and ansible_distribution_major_version is version('8', '>=')
+              else omit }}"
             includes: "{{ ['https', 'ldaps']
               if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
               and ansible_distribution_major_version is version('8', '>=')
               else omit }}"
-            permanent: true
-            state: present
           register: result
           failed_when: result is failed or result is not changed
 
+        - name: Verify service settings
+          command: firewall-offline-cmd --info-service customservice
+          register: info
+          changed_when: false
+          failed_when: >
+              info.failed
+              or "ports: 333/tcp" not in info.stdout
+              or "source-ports: 333/tcp" not in info.stdout
+              or "protocols: ipv6" not in info.stdout
+              or "destination: ipv4:123.45.6.78 ipv6:aaaa:aaaa:aaaa:aaa:aaaa:aaaa:aaaa::"
+                 not in info.stdout
+
+        - name: Verify includes and helper service settings
+          assert:
+            that:
+              - '"includes: https ldaps" in info.stdout'
+              - '"helpers: nf_conntrack_ftp" in info.stdout'
+          when: ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                and ansible_distribution_major_version is version('8', '>=')
+
         - name: Add helper module that already is on customservice
           firewall_lib:
             service: customservice
@@ -614,6 +638,8 @@
             state: present
           register: result
           failed_when: result is failed or result is changed
+          when: ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                and ansible_distribution_major_version is version('8', '>=')
 
         - name: Forward port 40 to 0.0.0.0:8080 (string)
           firewall_lib:

tests/tests_service.yml

@@ -124,8 +124,12 @@
                 destination:
                   - 1.1.1.1
                   - 1::1
-                helper_module: ftp
                 protocol: icmp
+                # these two don't exist yet in RHEL 7
+                helper_module: "{{ 'ftp'
+                  if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                  and ansible_distribution_major_version is version('8', '>=')
+                  else omit }}"
                 includes: "{{ ['ssh', 'ldaps']
                   if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
                   and ansible_distribution_major_version is version('8', '>=')
@@ -168,8 +172,12 @@
                 destination:
                   - 1.1.1.1
                   - 1::1
-                helper_module: ftp
                 protocol: icmp
+                # these two don't exist yet in RHEL 7
+                helper_module: "{{ 'ftp'
+                  if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                  and ansible_distribution_major_version is version('8', '>=')
+                  else omit }}"
                 includes: "{{ ['ssh', 'ldaps']
                   if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
                   and ansible_distribution_major_version is version('8', '>=')
@@ -247,8 +255,11 @@
                 destination:
                   - 1.1.1.1
                   - 1::1
-                helper_module: ftp
                 protocol: icmp
+                helper_module: "{{ 'ftp'
+                  if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                  and ansible_distribution_major_version is version('8', '>=')
+                  else omit }}"
                 includes: "{{ ['ssh', 'ldaps']
                   if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
                   and ansible_distribution_major_version is version('8', '>=')
@@ -274,8 +285,11 @@
                 destination:
                   - 1.1.1.1
                   - 1::1
-                helper_module: ftp
                 protocol: icmp
+                helper_module: "{{ 'ftp'
+                  if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
+                  and ansible_distribution_major_version is version('8', '>=')
+                  else omit }}"
                 includes: "{{ ['ssh', 'ldaps']
                   if ansible_distribution in ['RedHat', 'CentOS', 'Fedora']
                   and ansible_distribution_major_version is version('8', '>=')