provide useful error when invalid service name provided add offline mode to firewalld permanent operations

Signed-off-by: Adam Miller <maxamillion@fedoraproject.org>

lib/ansible/modules/extras/system/firewalld.py

@@ -106,7 +106,13 @@ EXAMPLES = '''
 
 from ansible.module_utils.basic import AnsibleModule
 
+import sys
+
+#####################
+# Globals
+#
 fw = None
+module = None
 fw_offline = False
 Rich_Rule = None
 FirewallClientZoneSettings = None
@@ -114,9 +120,37 @@ FirewallClientZoneSettings = None
 module = None
 
 #####################
-# fw_offline helpers
+# exception handling
 #
+def action_handler(action_func, action_func_args):
+    """
+    Function to wrap calls to make actions on firewalld in try/except
+    logic and emit (hopefully) useful error messages
+    """
+
+    msgs = []
+
+    try:
+        return action_func(*action_func_args)
+    except Exception:
+        # Make python 2.4 shippable ci tests happy
+        e = sys.exc_info()[1]
+
+        # If there are any commonly known errors that we should provide more
+        # context for to help the users diagnose what's wrong. Handle that here
+        if "INVALID_SERVICE" in "%s" % e:
+            msgs.append("Services are defined by port/tcp relationship and named as they are in /etc/services (on most systems)")
+
+        if len(msgs) > 0:
+            module.fail_json(
+                msg='ERROR: Exception caught: %s %s' % (e, ', '.join(msgs))
+            )
+        else:
+            module.fail_json(msg='ERROR: Exception caught: %s' % e)
 
+#####################
+# fw_offline helpers
+#
 def get_fw_zone_settings(zone):
     if fw_offline:
         fw_zone = fw.config.get_zone(zone)
@@ -151,7 +185,6 @@ def get_masquerade_enabled_permanent(zone):
     else:
         return False
 
-
 def set_masquerade_enabled(zone):
     fw.addMasquerade(zone)
 
@@ -364,10 +397,12 @@ def set_rich_rule_disabled_permanent(zone, rule):
     fw_settings.removeRichRule(rule)
     update_fw_settings(fw_zone, fw_settings)
 
-
 def main():
     global module
 
+    ## make module global so we don't have to pass it to action_handler every
+    ## function call
+    global module
     module = AnsibleModule(
         argument_spec = dict(
             service=dict(required=False,default=None),
@@ -380,6 +415,7 @@ def main():
             timeout=dict(type='int',required=False,default=0),
             interface=dict(required=False,default=None),
             masquerade=dict(required=False,default=None),
+            offline=dict(type='bool',required=False,default=None),
         ),
         supports_check_mode=True
     )
@@ -397,7 +433,6 @@ def main():
 
         from firewall.client import Rich_Rule
         from firewall.client import FirewallClient
-        HAS_FIREWALLD = True
         fw = None
         fw_offline = False
 
@@ -418,10 +453,9 @@ def main():
             fw_offline = True
 
     except ImportError:
-        HAS_FIREWALLD = False
+        ## Make python 2.4 shippable ci tests happy
-
+        e = sys.exc_info()[1]
-    if not HAS_FIREWALLD:
+        module.fail_json(msg='firewalld and its python 2 module are required for this module, version 2.0.11 or newer required (3.0.9 or newer for offline operations) \n %s' % e)
-        module.fail_json(msg='firewalld and its python 2 module are required for this module, version 2.0.11 or newer required (3.0.9 or newer for offline operations)')
 
     if fw_offline:
         ## Pre-run version checking
@@ -495,8 +529,57 @@ def main():
         module.fail_json(msg='can only operate on port, service, rich_rule or interface at once')
 
     if service != None:
-        if permanent:
+        if immediate and permanent:
-            is_enabled = get_service_enabled_permanent(zone, service)
+            is_enabled_permanent = action_handler(
+                get_service_enabled_permanent,
+                (zone, service)
+            )
+            is_enabled_immediate = action_handler(
+                get_service_enabled,
+                (zone, service)
+            )
+            msgs.append('Permanent and Non-Permanent(immediate) operation')
+
+            if desired_state == "enabled":
+                if not is_enabled_permanent or not is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if not is_enabled_permanent:
+                    action_handler(
+                        set_service_enabled_permanent,
+                        (zone, service)
+                    )
+                    changed=True
+                if not is_enabled_immediate:
+                    action_handler(
+                        set_service_enabled,
+                        (zone, service, timeout)
+                    )
+                    changed=True
+
+
+            elif desired_state == "disabled":
+                if is_enabled_permanent or is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if is_enabled_permanent:
+                    action_handler(
+                        set_service_disabled_permanent,
+                        (zone, service)
+                    )
+                    changed=True
+                if is_enabled_immediate:
+                    action_handler(
+                        set_service_disabled,
+                        (zone, service)
+                    )
+                    changed=True
+
+        elif permanent and not immediate:
+            is_enabled = action_handler(
+                get_service_enabled_permanent,
+                (zone, service)
+            )
             msgs.append('Permanent operation')
 
             if desired_state == "enabled":
@@ -504,17 +587,26 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_service_enabled_permanent(zone, service)
+                    action_handler(
+                        set_service_enabled_permanent,
+                        (zone, service)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_service_disabled_permanent(zone, service)
+                    action_handler(
+                        set_service_disabled_permanent,
+                        (zone, service)
+                    )
                     changed=True
-        if immediate or not permanent:
+        elif immediate and not permanent:
-            is_enabled = get_service_enabled(zone, service)
+            is_enabled = action_handler(
+                get_service_enabled,
+                (zone, service)
+            )
             msgs.append('Non-permanent operation')
 
 
@@ -523,27 +615,35 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_service_enabled(zone, service, timeout)
+                    action_handler(
+                        set_service_enabled,
+                        (zone, service, timeout)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_service_disabled(zone, service)
+                    action_handler(
+                        set_service_disabled,
+                        (zone, service)
+                    )
                     changed=True
 
         if changed == True:
             msgs.append("Changed service %s to %s" % (service, desired_state))
 
+    # FIXME - source type does not handle non-permanent mode, this was an
+    #         oversight in the past.
     if source != None:
-        is_enabled = get_source(zone, source)
+        is_enabled = action_handler(get_source, (zone, source))
         if desired_state == "enabled":
             if is_enabled == False:
                 if module.check_mode:
                     module.exit_json(changed=True)
 
-                add_source(zone, source)
+                action_handler(add_source, (zone, source))
                 changed=True
                 msgs.append("Added %s to zone %s" % (source, zone))
         elif desired_state == "disabled":
@@ -551,13 +651,61 @@ def main():
                 if module.check_mode:
                     module.exit_json(changed=True)
 
-                remove_source(zone, source)
+                action_handler(remove_source, (zone, source))
                 changed=True
                 msgs.append("Removed %s from zone %s" % (source, zone))
 
     if port != None:
-        if permanent:
+        if immediate and permanent:
-            is_enabled = get_port_enabled_permanent(zone, [port, protocol])
+            is_enabled_permanent = action_handler(
+                get_port_enabled_permanent,
+                (zone,[port, protocol])
+            )
+            is_enabled_immediate = action_handler(
+                get_port_enabled,
+                (zone, [port, protocol])
+            )
+            msgs.append('Permanent and Non-Permanent(immediate) operation')
+
+            if desired_state == "enabled":
+                if not is_enabled_permanent or not is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if not is_enabled_permanent:
+                    action_handler(
+                        set_port_enabled_permanent,
+                        (zone, port, protocol)
+                    )
+                    changed=True
+                if not is_enabled_immediate:
+                    action_handler(
+                        set_port_enabled,
+                        (zone, port, protocol, timeout)
+                    )
+                    changed=True
+
+            elif desired_state == "disabled":
+                if is_enabled_permanent or is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if is_enabled_permanent:
+                    action_handler(
+                        set_port_disabled_permanent,
+                        (zone, port, protocol)
+                    )
+                    changed=True
+                if is_enabled_immediate:
+                    action_handler(
+                        set_port_disabled,
+                        (zone, port, protocol)
+                    )
+                    changed=True
+
+        elif permanent and not immediate:
+            is_enabled = action_handler(
+                get_port_enabled_permanent,
+                (zone, [port, protocol])
+            )
             msgs.append('Permanent operation')
 
             if desired_state == "enabled":
@@ -565,17 +713,26 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_port_enabled_permanent(zone, port, protocol)
+                    action_handler(
+                        set_port_enabled_permanent,
+                        (zone, port, protocol)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_port_disabled_permanent(zone, port, protocol)
+                    action_handler(
+                        set_port_disabled_permanent,
+                        (zone, port, protocol)
+                    )
                     changed=True
-        if immediate or not permanent:
+        if immediate and not permanent:
-            is_enabled = get_port_enabled(zone, [port,protocol])
+            is_enabled = action_handler(
+                get_port_enabled,
+                (zone, [port,protocol])
+            )
             msgs.append('Non-permanent operation')
 
             if desired_state == "enabled":
@@ -583,14 +740,20 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_port_enabled(zone, port, protocol, timeout)
+                    action_handler(
+                        set_port_enabled,
+                        (zone, port, protocol, timeout)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_port_disabled(zone, port, protocol)
+                    action_handler(
+                        set_port_disabled,
+                        (zone, port, protocol)
+                    )
                     changed=True
 
         if changed == True:
@@ -598,8 +761,55 @@ def main():
                         desired_state))
 
     if rich_rule != None:
-        if permanent:
+        if immediate and permanent:
-            is_enabled = get_rich_rule_enabled_permanent(zone, rich_rule)
+            is_enabled_permanent = action_handler(
+                get_rich_rule_enabled_permanent,
+                (zone, rich_rule)
+            )
+            is_enabled_immediate = action_handler(
+                get_rich_rule_enabled,
+                (zone, rich_rule)
+            )
+            msgs.append('Permanent and Non-Permanent(immediate) operation')
+
+            if desired_state == "enabled":
+                if not is_enabled_permanent or not is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if not is_enabled_permanent:
+                    action_handler(
+                        set_rich_rule_enabled_permanent,
+                        (zone, rich_rule)
+                    )
+                    changed=True
+                if not is_enabled_immediate:
+                    action_handler(
+                        set_rich_rule_enabled,
+                        (zone, rich_rule, timeout)
+                    )
+                    changed=True
+
+            elif desired_state == "disabled":
+                if is_enabled_permanent or is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if is_enabled_permanent:
+                    action_handler(
+                        set_rich_rule_disabled_permanent,
+                        (zone, rich_rule)
+                    )
+                    changed=True
+                if is_enabled_immediate:
+                    action_handler(
+                        set_rich_rule_disabled,
+                        (zone, rich_rule)
+                    )
+                    changed=True
+        if permanent and not immediate:
+            is_enabled = action_handler(
+                get_rich_rule_enabled_permanent,
+                (zone, rich_rule)
+            )
             msgs.append('Permanent operation')
 
             if desired_state == "enabled":
@@ -607,17 +817,26 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_rich_rule_enabled_permanent(zone, rich_rule)
+                    action_handler(
+                        set_rich_rule_enabled_permanent,
+                        (zone, rich_rule)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_rich_rule_disabled_permanent(zone, rich_rule)
+                    action_handler(
+                        set_rich_rule_disabled_permanent,
+                        (zone, rich_rule)
+                    )
                     changed=True
-        if immediate or not permanent:
+        if immediate and not permanent:
-            is_enabled = get_rich_rule_enabled(zone, rich_rule)
+            is_enabled = action_handler(
+                get_rich_rule_enabled,
+                (zone, rich_rule)
+            )
             msgs.append('Non-permanent operation')
 
             if desired_state == "enabled":
@@ -625,22 +844,68 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_rich_rule_enabled(zone, rich_rule, timeout)
+                    action_handler(
+                        set_rich_rule_enabled,
+                        (zone, rich_rule, timeout)
+                    )
                     changed=True
             elif desired_state == "disabled":
                 if is_enabled == True:
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_rich_rule_disabled(zone, rich_rule)
+                    action_handler(
+                        set_rich_rule_disabled,
+                        (zone, rich_rule)
+                    )
                     changed=True
 
         if changed == True:
             msgs.append("Changed rich_rule %s to %s" % (rich_rule, desired_state))
 
     if interface != None:
-        if permanent:
+        if immediate and permanent:
-            is_enabled = get_interface_permanent(zone, interface)
+            is_enabled_permanent = action_handler(
+                get_interface_permanent,
+                (zone, interface)
+            )
+            is_enabled_immediate = action_handler(
+                get_interface,
+                (zone, interface)
+            )
+            msgs.append('Permanent and Non-Permanent(immediate) operation')
+
+            if desired_state == "enabled":
+                if not is_enabled_permanent or not is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if not is_enabled_permanent:
+                    change_zone_of_interface_permanent(zone, interface)
+                    changed=True
+                if not is_enabled_immediate:
+                    change_zone_of_interface(zone, interface)
+                    changed=True
+                if changed:
+                    msgs.append("Changed %s to zone %s" % (interface, zone))
+
+            elif desired_state == "disabled":
+                if is_enabled_permanent or is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if is_enabled_permanent:
+                    remove_interface_permanent(zone, interface)
+                    changed=True
+                if is_enabled_immediate:
+                    remove_interface(zone, interface)
+                    changed=True
+                if changed:
+                    msgs.append("Removed %s from zone %s" % (interface, zone))
+
+        elif permanent and not immediate:
+            is_enabled = action_handler(
+                get_interface_permanent,
+                (zone, interface)
+            )
             msgs.append('Permanent operation')
             if desired_state == "enabled":
                 if is_enabled == False:
@@ -658,8 +923,11 @@ def main():
                     remove_interface_permanent(zone, interface)
                     changed=True
                     msgs.append("Removed %s from zone %s" % (interface, zone))
-        if immediate or not permanent:
+        elif immediate and not permanent:
-            is_enabled = get_interface(zone, interface)
+            is_enabled = action_handler(
+                get_interface,
+                (zone, interface)
+            )
             msgs.append('Non-permanent operation')
             if desired_state == "enabled":
                 if is_enabled == False:
@@ -680,8 +948,42 @@ def main():
 
     if masquerade != None:
 
-        if permanent:
+        if immediate and permanent:
-            is_enabled = get_masquerade_enabled_permanent(zone)
+            is_enabled_permanent = action_handler(
+                get_masquerade_enabled_permanent,
+                (zone)
+            )
+            is_enabled_immediate = action_handler(get_masquerade_enabled, (zone))
+            msgs.append('Permanent and Non-Permanent(immediate) operation')
+
+            if desired_state == "enabled":
+                if not is_enabled_permanent or not is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if not is_enabled_permanent:
+                    action_handler(set_masquerade_permanent, (zone, True))
+                    changed=True
+                if not is_enabled_immediate:
+                    action_handler(set_masquerade_enabled, (zone))
+                    changed=True
+                if changed:
+                    msgs.append("Added masquerade to zone %s" % (zone))
+
+            elif desired_state == "disabled":
+                if is_enabled_permanent or is_enabled_immediate:
+                    if module.check_mode:
+                        module.exit_json(changed=True)
+                if is_enabled_permanent:
+                    action_handler(set_masquerade_permanent, (zone, False))
+                    changed=True
+                if is_enabled_immediate:
+                    action_handler(set_masquerade_disabled, (zone))
+                    changed=True
+                if changed:
+                    msgs.append("Removed masquerade from zone %s" % (zone))
+
+        elif permanent and not immediate:
+            is_enabled = action_handler(get_masquerade_enabled_permanent, (zone))
             msgs.append('Permanent operation')
 
             if desired_state == "enabled":
@@ -689,7 +991,7 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_masquerade_permanent(zone, True)
+                    action_handler(set_masquerade_permanent, (zone, True))
                     changed=True
                     msgs.append("Added masquerade to zone %s" % (zone))
             elif desired_state == "disabled":
@@ -697,11 +999,11 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_masquerade_permanent(zone, False)
+                    action_handler(set_masquerade_permanent, (zone, False))
                     changed=True
                     msgs.append("Removed masquerade from zone %s" % (zone))
-        if immediate or not permanent:
+        elif immediate and not permanent:
-            is_enabled = get_masquerade_enabled(zone)
+            is_enabled = action_handler(get_masquerade_enabled, (zone))
             msgs.append('Non-permanent operation')
 
             if desired_state == "enabled":
@@ -709,7 +1011,7 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_masquerade_enabled(zone)
+                    action_handler(set_masquerade_enabled, (zone))
                     changed=True
                     msgs.append("Added masquerade to zone %s" % (zone))
             elif desired_state == "disabled":
@@ -717,7 +1019,7 @@ def main():
                     if module.check_mode:
                         module.exit_json(changed=True)
 
-                    set_masquerade_disabled(zone)
+                    action_handler(set_masquerade_disabled, (zone))
                     changed=True
                     msgs.append("Removed masquerade from zone %s" % (zone))