From 68341f4226a61641c6d2f83c063cbb5aeba58711 Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Mon, 15 Jul 2024 17:31:08 +0300
Subject: [PATCH 1/5] fix: state merged for panos_security_rule

* Also fixes panos_template mode xpath error on second run
---
 plugins/module_utils/panos.py          | 82 +++++++++++++++++++----
 plugins/modules/panos_security_rule.py | 93 ++++++++++++--------------
 plugins/modules/panos_template.py      |  5 ++
 3 files changed, 118 insertions(+), 62 deletions(-)

diff --git a/plugins/module_utils/panos.py b/plugins/module_utils/panos.py
index dffb513dd..25b607426 100644
--- a/plugins/module_utils/panos.py
+++ b/plugins/module_utils/panos.py
@@ -139,6 +139,7 @@ def __init__(
         self.parents = ()
         self.sdk_params = {}
         self.extra_params = {}
+        self.preset_values = {}
         self.reference_operations = ()
         self.ansible_to_sdk_param_mapping = {}
         self.with_uuid = False
@@ -489,6 +490,9 @@ def process(self, module):
                 ansible_param, ansible_param
             )
             spec[sdk_param] = module.params.get(ansible_param)
+            if ansible_param in self.preset_values.keys():
+                self.preset_values[sdk_param] = self.preset_values.pop(ansible_param)
+
         if self.with_uuid:
             spec["uuid"] = module.params["uuid"]
         if self.with_target:
@@ -672,6 +676,9 @@ def apply_state(
 
         # Apply the state.
         if module.params["state"] in ("present", "replaced"):
+            # create reference object for defaults
+            obj_cls = obj.__class__
+            obj_default = obj_cls()
             # Apply the config.
             for item in listing:
                 if item.uid != obj.uid:
@@ -690,27 +697,47 @@ def apply_state(
                 if not item.equal(obj, compare_children=True):
                     result["changed"] = True
                     obj.extend(other_children)
-                    result["after"] = self.describe(obj)
-                    result["diff"]["after"] = eltostr(obj)
                     if not module.check_mode:
                         if self.with_update_in_apply_state:
-                            for param in obj.about().keys():
-                                if getattr(item, param) != getattr(obj, param):
+                            for key, obj_value in obj.about().items():
+                                # checking defaults for with_update_in_apply_state doesnot have
+                                # a use for now as template, stack and device group dont have
+                                # defaults in the SDK
+                                # it also breaks panos_template as SDK has `mode` attribute set
+                                # to "normal" by default, but there is no xpath for this.
+                                # if obj_value is None:
+                                #     default_value = getattr(obj_default, key, None)
+                                #     setattr(obj, key, default_value)
+                                if getattr(item, key) != getattr(obj, key):
                                     try:
-                                        obj.update(param)
+                                        obj.update(key)
                                     except PanDeviceError as e:
                                         module.fail_json(
                                             msg="Failed update {0}: {1}".format(
-                                                param, e
+                                                key, e
                                             )
                                         )
+                            result["after"] = self.describe(obj)
+                            result["diff"]["after"] = eltostr(obj)
                         else:
+                            for key, obj_value in obj.about().items():
+                                if obj_value is None:
+                                    default_value = getattr(obj_default, key, None)
+                                    setattr(obj, key, default_value)
+                            result["after"] = self.describe(obj)
+                            result["diff"]["after"] = eltostr(obj)
                             try:
                                 obj.apply()
                             except PanDeviceError as e:
                                 module.fail_json(msg="Failed apply: {0}".format(e))
                 break
             else:
+                # NOTE alternative is to get defaults from sdk (as in merged)
+                for key, obj_value in obj.about().items():
+                    if obj_value is None:
+                        default_value = getattr(obj_default, key, None)
+                        setattr(obj, key, default_value)
+
                 result["changed"] = True
                 result["before"] = None
                 result["after"] = self.describe(obj)
@@ -829,15 +856,25 @@ def apply_state(
                 updated_params = set([])
                 for key, obj_value in obj.about().items():
                     item_value = getattr(item, key, None)
-                    if obj_value is not None:
+                    if obj_value:
                         if isinstance(obj_value, list) or isinstance(item_value, list):
                             if not item_value:
                                 item_value = []
-                            for elm in obj_value:
-                                if elm not in item_value:
-                                    updated_params.add(key)
-                                    item_value.append(elm)
-                                    setattr(item, key, item_value)
+                            # if current config or obj to create is one of the preset values
+                            # (dropdown options in UI) then replace it with the obj value
+                            # since values like "any" can not be in place with other values.
+                            if ((preset_values := self.preset_values.get(key, None)) and
+                                (set(item_value).issubset(preset_values) or
+                                 set(obj_value).issubset(preset_values))):
+                                updated_params.add(key)
+                                setattr(item, key, obj_value)
+                            else:
+                                # NOTE what happens here if obj_value is not a list?
+                                for elm in obj_value:
+                                    if elm not in item_value:
+                                        updated_params.add(key)
+                                        item_value.append(elm)
+                                        setattr(item, key, item_value)
                         elif item_value != obj_value:
                             updated_params.add(key)
                             setattr(item, key, obj_value)
@@ -854,7 +891,23 @@ def apply_state(
                                     msg="Failed update {0}: {1}".format(param, e)
                                 )
                 break
-            else:
+            else:               # create new record with merge
+                # TODO obj._params is not public attribute on SDK which provide default values
+                # put default values from sdk for null values for a newly created object
+                # for _param in obj._params:
+                #     if _param.value is None:
+                #         setattr(obj, _param.name, _param.default)
+
+                # NOTE alternative is to create a temp object with defaults and use values
+                # from this temp object to fetch defaults for None values and set it for the
+                # object to create
+                obj_cls = obj.__class__
+                obj_default = obj_cls()
+                for key, obj_value in obj.about().items():
+                    if obj_value is None:
+                        default_value = getattr(obj_default, key, None)
+                        setattr(obj, key, default_value)
+
                 result["before"] = None
                 result["after"] = self.describe(obj)
                 result["diff"] = {
@@ -1368,6 +1421,7 @@ def get_connection(
     parents=None,
     sdk_params=None,
     extra_params=None,
+    preset_values=None,
     reference_operations=None,
     ansible_to_sdk_param_mapping=None,
     with_uuid=False,
@@ -1745,6 +1799,8 @@ class in the package (e.g. - "VirtualRouter").  If the class is a singleton
                 renames[k] = sdk_name
             spec[k] = sdk_params[k]
         helper.sdk_params = sdk_params
+        if preset_values is not None:
+            helper.preset_values = preset_values
 
     if with_gathered_filter:
         if "gathered_filter" in spec:
diff --git a/plugins/modules/panos_security_rule.py b/plugins/modules/panos_security_rule.py
index 6c72d8412..c7ed418a3 100644
--- a/plugins/modules/panos_security_rule.py
+++ b/plugins/modules/panos_security_rule.py
@@ -59,13 +59,12 @@
         type: str
     source_zone:
         description:
-            - List of source zones.
-        default: ["any"]
+            - List of source zones. Defaults to "any" in SDK.
         type: list
         elements: str
     source_ip:
         description:
-            - List of source addresses.
+            - List of source addresses. Defaults to "any" in SDK.
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -74,13 +73,12 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
-        default: ["any"]
         type: list
         elements: str
     source_user:
-        description:
+        description: >
             - Use users to enforce policy for individual users or a group of users.
-        default: ["any"]
+            Defaults to "any" in SDK.
         type: list
         elements: str
     hip_profiles:
@@ -97,13 +95,12 @@
         elements: str
     destination_zone:
         description:
-            - List of destination zones.
-        default: ["any"]
+            - List of destination zones. Defaults to "any" in SDK.
         type: list
         elements: str
     destination_ip:
         description:
-            - List of destination addresses.
+            - List of destination addresses. Defaults to "any" in SDK.
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -112,34 +109,31 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
-        default: ["any"]
         type: list
         elements: str
     application:
         description:
-            - List of applications, application groups, and/or application filters.
-        default: ["any"]
+            - List of applications, application groups, and/or application filters. Defaults to "any" in SDK.
         type: list
         elements: str
     service:
         description:
-            - List of services and/or service groups.
-        default: ['application-default']
+            - List of services and/or service groups. Defaults to "application-default" in SDK.
         type: list
         elements: str
     category:
         description:
-            - List of destination URL categories.
+            - List of destination URL categories. Defaults to "any" in SDK.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
               request system external-list show type predefined-url name <tab>
                 panw-auth-portal-exclude-list   panw-auth-portal-exclude-list
-        default: ["any"]
         type: list
         elements: str
     action:
-        description:
-            - Action to apply once rules matches.
+        description: >
+            - Action to apply to the rule. No default value in SDK, should be provided when creating
+            a new resource.
         type: str
         choices:
             - allow
@@ -148,20 +142,17 @@
             - reset-client
             - reset-server
             - reset-both
-        default: "allow"
     log_setting:
         description:
             - Log forwarding profile.
         type: str
     log_start:
         description:
-            - Whether to log at session start.
-        default: false
+            - Whether to log at session start. Defaults to PAN-OS behaviour.
         type: bool
     log_end:
         description:
-            - Whether to log at session end.
-        default: true
+            - Whether to log at session end. Defaults to PAN-OS behaviour.
         type: bool
     description:
         description:
@@ -169,13 +160,12 @@
         type: str
     rule_type:
         description:
-            - Type of security rule (version 6.1 of PanOS and above).
+            - Type of security rule (version 6.1 of PanOS and above). Defaults to "universal" in SDK.
         type: str
         choices:
             - universal
             - intrazone
             - interzone
-        default: 'universal'
     tag_name:
         description:
             - List of tags associated with the rule.
@@ -183,18 +173,15 @@
         elements: str
     negate_source:
         description:
-            - Match on the reverse of the 'source_ip' attribute
-        default: false
+            - Match on the reverse of the 'source_ip' attribute. Defaults to PAN-OS behaviour.
         type: bool
     negate_destination:
         description:
-            - Match on the reverse of the 'destination_ip' attribute
-        default: false
+            - Match on the reverse of the 'destination_ip' attribute. Defaults to PAN-OS behaviour.
         type: bool
     disabled:
         description:
-            - Disable this rule.
-        default: false
+            - Disable this rule. Defaults to PAN-OS behaviour.
         type: bool
     schedule:
         description:
@@ -205,9 +192,9 @@
             - Send 'ICMP Unreachable'. Used with 'deny', 'drop', and 'reset' actions.
         type: bool
     disable_server_response_inspection:
-        description:
+        description: >
             - Disables packet inspection from the server to the client. Useful under heavy server load conditions.
-        default: false
+            Defaults to PAN-OS behaviour.
         type: bool
     group_profile:
         description: >
@@ -400,24 +387,23 @@ def main():
         sdk_params=dict(
             rule_name=dict(required=True, sdk_param="name"),
             source_zone=dict(
-                type="list", elements="str", default=["any"], sdk_param="fromzone"
+                type="list", elements="str", sdk_param="fromzone"
             ),
             source_ip=dict(
-                type="list", elements="str", default=["any"], sdk_param="source"
+                type="list", elements="str", sdk_param="source"
             ),
-            source_user=dict(type="list", elements="str", default=["any"]),
+            source_user=dict(type="list", elements="str"),
             hip_profiles=dict(type="list", elements="str"),
             destination_zone=dict(
-                type="list", elements="str", default=["any"], sdk_param="tozone"
+                type="list", elements="str", sdk_param="tozone"
             ),
             destination_ip=dict(
-                type="list", elements="str", default=["any"], sdk_param="destination"
+                type="list", elements="str", sdk_param="destination"
             ),
-            application=dict(type="list", elements="str", default=["any"]),
-            service=dict(type="list", elements="str", default=["application-default"]),
-            category=dict(type="list", elements="str", default=["any"]),
+            application=dict(type="list", elements="str"),
+            service=dict(type="list", elements="str"),
+            category=dict(type="list", elements="str"),
             action=dict(
-                default="allow",
                 choices=[
                     "allow",
                     "deny",
@@ -428,21 +414,20 @@ def main():
                 ],
             ),
             log_setting=dict(),
-            log_start=dict(type="bool", default=False),
-            log_end=dict(type="bool", default=True),
+            log_start=dict(type="bool"),
+            log_end=dict(type="bool"),
             description=dict(),
             rule_type=dict(
-                default="universal",
                 choices=["universal", "intrazone", "interzone"],
                 sdk_param="type",
             ),
             tag_name=dict(type="list", elements="str", sdk_param="tag"),
-            negate_source=dict(type="bool", default=False),
-            negate_destination=dict(type="bool", default=False),
-            disabled=dict(type="bool", default=False),
+            negate_source=dict(type="bool"),
+            negate_destination=dict(type="bool"),
+            disabled=dict(type="bool"),
             schedule=dict(),
             icmp_unreachable=dict(type="bool"),
-            disable_server_response_inspection=dict(type="bool", default=False),
+            disable_server_response_inspection=dict(type="bool"),
             group_profile=dict(sdk_param="group"),
             antivirus=dict(sdk_param="virus"),
             spyware=dict(),
@@ -457,6 +442,16 @@ def main():
             # TODO(gfreeman) - remove this in the next role release.
             devicegroup=dict(),
         ),
+        preset_values=dict(
+            source_zone=["any"],
+            source_ip=["any"],
+            source_user=["any", "pre-logon", "known-user", "unknown"],
+            destination_zone=["any", "multicast"],
+            destination_ip=["any"],
+            application=["any"],
+            service=["application-default", "any"],
+            category=["any"],
+        ),
     )
 
     module = AnsibleModule(
diff --git a/plugins/modules/panos_template.py b/plugins/modules/panos_template.py
index 89ce4670e..e93a34103 100644
--- a/plugins/modules/panos_template.py
+++ b/plugins/modules/panos_template.py
@@ -60,6 +60,10 @@
             - The default vsys in case of a single vsys firewall.
         type: str
         default: vsys1
+    mode:
+        description:
+            - Mode for template.
+        type: str
 """
 
 EXAMPLES = """
@@ -117,6 +121,7 @@ def main():
             description=dict(),
             devices=dict(type="list", elements="str"),
             default_vsys=dict(default="vsys1"),
+            mode=dict(),
         ),
     )
 

From c3fc902f3071bcbd26ba2931caf0f54c12ad41bb Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Wed, 7 Aug 2024 13:22:17 +0300
Subject: [PATCH 2/5] fix: state merged for panos_security_rule

Introduce default_values param in helper function instead of
sdk_params default values to avoid issue with default values
being merged to existing configuration.
---
 plugins/module_utils/panos.py          | 77 +++++++++++++----------
 plugins/modules/panos_security_rule.py | 87 ++++++++++++++++----------
 tests/sanity/ignore-2.13.txt           |  1 +
 tests/sanity/ignore-2.14.txt           |  1 +
 tests/sanity/ignore-2.15.txt           |  1 +
 tests/sanity/ignore-2.16.txt           |  1 +
 6 files changed, 102 insertions(+), 66 deletions(-)

diff --git a/plugins/module_utils/panos.py b/plugins/module_utils/panos.py
index 25b607426..90e5e4aa8 100644
--- a/plugins/module_utils/panos.py
+++ b/plugins/module_utils/panos.py
@@ -139,6 +139,7 @@ def __init__(
         self.parents = ()
         self.sdk_params = {}
         self.extra_params = {}
+        self.default_values = {}
         self.preset_values = {}
         self.reference_operations = ()
         self.ansible_to_sdk_param_mapping = {}
@@ -492,6 +493,8 @@ def process(self, module):
             spec[sdk_param] = module.params.get(ansible_param)
             if ansible_param in self.preset_values.keys():
                 self.preset_values[sdk_param] = self.preset_values.pop(ansible_param)
+            if ansible_param in self.default_values.keys():
+                self.default_values[sdk_param] = self.default_values.pop(ansible_param)
 
         if self.with_uuid:
             spec["uuid"] = module.params["uuid"]
@@ -676,9 +679,6 @@ def apply_state(
 
         # Apply the state.
         if module.params["state"] in ("present", "replaced"):
-            # create reference object for defaults
-            obj_cls = obj.__class__
-            obj_default = obj_cls()
             # Apply the config.
             for item in listing:
                 if item.uid != obj.uid:
@@ -700,30 +700,26 @@ def apply_state(
                     if not module.check_mode:
                         if self.with_update_in_apply_state:
                             for key, obj_value in obj.about().items():
-                                # checking defaults for with_update_in_apply_state doesnot have
+                                # NOTE checking defaults for with_update_in_apply_state doesnot have
                                 # a use for now as template, stack and device group dont have
                                 # defaults in the SDK
                                 # it also breaks panos_template as SDK has `mode` attribute set
                                 # to "normal" by default, but there is no xpath for this.
                                 # if obj_value is None:
-                                #     default_value = getattr(obj_default, key, None)
-                                #     setattr(obj, key, default_value)
+                                #     setattr(obj, key, self._get_default_value(obj, key))
                                 if getattr(item, key) != getattr(obj, key):
                                     try:
                                         obj.update(key)
                                     except PanDeviceError as e:
                                         module.fail_json(
-                                            msg="Failed update {0}: {1}".format(
-                                                key, e
-                                            )
+                                            msg="Failed update {0}: {1}".format(key, e)
                                         )
                             result["after"] = self.describe(obj)
                             result["diff"]["after"] = eltostr(obj)
                         else:
                             for key, obj_value in obj.about().items():
                                 if obj_value is None:
-                                    default_value = getattr(obj_default, key, None)
-                                    setattr(obj, key, default_value)
+                                    setattr(obj, key, self._get_default_value(obj, key))
                             result["after"] = self.describe(obj)
                             result["diff"]["after"] = eltostr(obj)
                             try:
@@ -732,12 +728,9 @@ def apply_state(
                                 module.fail_json(msg="Failed apply: {0}".format(e))
                 break
             else:
-                # NOTE alternative is to get defaults from sdk (as in merged)
                 for key, obj_value in obj.about().items():
                     if obj_value is None:
-                        default_value = getattr(obj_default, key, None)
-                        setattr(obj, key, default_value)
-
+                        setattr(obj, key, self._get_default_value(obj, key))
                 result["changed"] = True
                 result["before"] = None
                 result["after"] = self.describe(obj)
@@ -863,13 +856,15 @@ def apply_state(
                             # if current config or obj to create is one of the preset values
                             # (dropdown options in UI) then replace it with the obj value
                             # since values like "any" can not be in place with other values.
-                            if ((preset_values := self.preset_values.get(key, None)) and
-                                (set(item_value).issubset(preset_values) or
-                                 set(obj_value).issubset(preset_values))):
+                            if (
+                                preset_values := self.preset_values.get(key, None)
+                            ) and (
+                                set(item_value).issubset(preset_values)
+                                or set(obj_value).issubset(preset_values)
+                            ):
                                 updated_params.add(key)
                                 setattr(item, key, obj_value)
                             else:
-                                # NOTE what happens here if obj_value is not a list?
                                 for elm in obj_value:
                                     if elm not in item_value:
                                         updated_params.add(key)
@@ -891,23 +886,10 @@ def apply_state(
                                     msg="Failed update {0}: {1}".format(param, e)
                                 )
                 break
-            else:               # create new record with merge
-                # TODO obj._params is not public attribute on SDK which provide default values
-                # put default values from sdk for null values for a newly created object
-                # for _param in obj._params:
-                #     if _param.value is None:
-                #         setattr(obj, _param.name, _param.default)
-
-                # NOTE alternative is to create a temp object with defaults and use values
-                # from this temp object to fetch defaults for None values and set it for the
-                # object to create
-                obj_cls = obj.__class__
-                obj_default = obj_cls()
+            else:  # create new record with merge
                 for key, obj_value in obj.about().items():
                     if obj_value is None:
-                        default_value = getattr(obj_default, key, None)
-                        setattr(obj, key, default_value)
-
+                        setattr(obj, key, self._get_default_value(obj, key))
                 result["before"] = None
                 result["after"] = self.describe(obj)
                 result["diff"] = {
@@ -1192,6 +1174,30 @@ def _describe(self, elm):
 
         return ans
 
+    def _get_default_value(self, obj, key):
+        """Returns default value for an sdk param in Ansible module.
+
+        Args:
+            obj: The pandevice object to fetch defaults from SDK.
+            key: sdk param name to get default value for.
+
+        Returns:
+            Default value of sdk param if defined in Ansible module default_values or
+            fetch from SDK defaults as a fallback.
+
+        """
+        # TODO get default values from pan-os-python SDK
+        # obj._params is not public attribute on SDK which provide default values
+        # either make it public accessible or provide a method
+        # NOTE create a temp object with defaults and use values from this temp object
+        # to fetch defaults for None values and set it for the object to create
+        obj_default = obj.__class__()
+        if (default_value := self.default_values.get(key, None)) is None:
+            # set default value from SDK if not found in module default_values
+            default_value = getattr(obj_default, key, None)
+
+        return default_value
+
     def matches_gathered_filter(self, item, logic):
         """Returns True if the item and its contents matches the logic given.
 
@@ -1421,6 +1427,7 @@ def get_connection(
     parents=None,
     sdk_params=None,
     extra_params=None,
+    default_values=None,
     preset_values=None,
     reference_operations=None,
     ansible_to_sdk_param_mapping=None,
@@ -1801,6 +1808,8 @@ class in the package (e.g. - "VirtualRouter").  If the class is a singleton
         helper.sdk_params = sdk_params
         if preset_values is not None:
             helper.preset_values = preset_values
+        if default_values is not None:
+            helper.default_values = default_values
 
     if with_gathered_filter:
         if "gathered_filter" in spec:
diff --git a/plugins/modules/panos_security_rule.py b/plugins/modules/panos_security_rule.py
index c7ed418a3..d3525473d 100644
--- a/plugins/modules/panos_security_rule.py
+++ b/plugins/modules/panos_security_rule.py
@@ -59,12 +59,13 @@
         type: str
     source_zone:
         description:
-            - List of source zones. Defaults to "any" in SDK.
+            - List of source zones.
+        default: ["any"]
         type: list
         elements: str
     source_ip:
         description:
-            - List of source addresses. Defaults to "any" in SDK.
+            - List of source addresses.
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -73,12 +74,13 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
+        default: ["any"]
         type: list
         elements: str
     source_user:
-        description: >
+        description:
             - Use users to enforce policy for individual users or a group of users.
-            Defaults to "any" in SDK.
+        default: ["any"]
         type: list
         elements: str
     hip_profiles:
@@ -95,12 +97,13 @@
         elements: str
     destination_zone:
         description:
-            - List of destination zones. Defaults to "any" in SDK.
+            - List of destination zones.
+        default: ["any"]
         type: list
         elements: str
     destination_ip:
         description:
-            - List of destination addresses. Defaults to "any" in SDK.
+            - List of destination addresses.
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -109,31 +112,34 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
+        default: ["any"]
         type: list
         elements: str
     application:
         description:
-            - List of applications, application groups, and/or application filters. Defaults to "any" in SDK.
+            - List of applications, application groups, and/or application filters.
+        default: ["any"]
         type: list
         elements: str
     service:
         description:
-            - List of services and/or service groups. Defaults to "application-default" in SDK.
+            - List of services and/or service groups.
+        default: ['application-default']
         type: list
         elements: str
     category:
         description:
-            - List of destination URL categories. Defaults to "any" in SDK.
+            - List of destination URL categories.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
               request system external-list show type predefined-url name <tab>
                 panw-auth-portal-exclude-list   panw-auth-portal-exclude-list
+        default: ["any"]
         type: list
         elements: str
     action:
-        description: >
-            - Action to apply to the rule. No default value in SDK, should be provided when creating
-            a new resource.
+        description:
+            - Action to apply to the rule.
         type: str
         choices:
             - allow
@@ -142,17 +148,20 @@
             - reset-client
             - reset-server
             - reset-both
+        default: "allow"
     log_setting:
         description:
             - Log forwarding profile.
         type: str
     log_start:
         description:
-            - Whether to log at session start. Defaults to PAN-OS behaviour.
+            - Whether to log at session start.
+        default: false
         type: bool
     log_end:
         description:
-            - Whether to log at session end. Defaults to PAN-OS behaviour.
+            - Whether to log at session end.
+        default: true
         type: bool
     description:
         description:
@@ -160,12 +169,13 @@
         type: str
     rule_type:
         description:
-            - Type of security rule (version 6.1 of PanOS and above). Defaults to "universal" in SDK.
+            - Type of security rule (version 6.1 of PanOS and above).
         type: str
         choices:
             - universal
             - intrazone
             - interzone
+        default: 'universal'
     tag_name:
         description:
             - List of tags associated with the rule.
@@ -173,15 +183,18 @@
         elements: str
     negate_source:
         description:
-            - Match on the reverse of the 'source_ip' attribute. Defaults to PAN-OS behaviour.
+            - Match on the reverse of the 'source_ip' attribute.
+        default: false
         type: bool
     negate_destination:
         description:
-            - Match on the reverse of the 'destination_ip' attribute. Defaults to PAN-OS behaviour.
+            - Match on the reverse of the 'destination_ip' attribute.
+        default: false
         type: bool
     disabled:
         description:
-            - Disable this rule. Defaults to PAN-OS behaviour.
+            - Disable this rule.
+        default: false
         type: bool
     schedule:
         description:
@@ -192,9 +205,9 @@
             - Send 'ICMP Unreachable'. Used with 'deny', 'drop', and 'reset' actions.
         type: bool
     disable_server_response_inspection:
-        description: >
+        description:
             - Disables packet inspection from the server to the client. Useful under heavy server load conditions.
-            Defaults to PAN-OS behaviour.
+        default: false
         type: bool
     group_profile:
         description: >
@@ -386,20 +399,12 @@ def main():
         sdk_cls=("policies", "SecurityRule"),
         sdk_params=dict(
             rule_name=dict(required=True, sdk_param="name"),
-            source_zone=dict(
-                type="list", elements="str", sdk_param="fromzone"
-            ),
-            source_ip=dict(
-                type="list", elements="str", sdk_param="source"
-            ),
+            source_zone=dict(type="list", elements="str", sdk_param="fromzone"),
+            source_ip=dict(type="list", elements="str", sdk_param="source"),
             source_user=dict(type="list", elements="str"),
             hip_profiles=dict(type="list", elements="str"),
-            destination_zone=dict(
-                type="list", elements="str", sdk_param="tozone"
-            ),
-            destination_ip=dict(
-                type="list", elements="str", sdk_param="destination"
-            ),
+            destination_zone=dict(type="list", elements="str", sdk_param="tozone"),
+            destination_ip=dict(type="list", elements="str", sdk_param="destination"),
             application=dict(type="list", elements="str"),
             service=dict(type="list", elements="str"),
             category=dict(type="list", elements="str"),
@@ -442,6 +447,24 @@ def main():
             # TODO(gfreeman) - remove this in the next role release.
             devicegroup=dict(),
         ),
+        default_values=dict(
+            source_zone=["any"],
+            source_ip=["any"],
+            source_user=["any"],
+            destination_zone=["any"],
+            destination_ip=["any"],
+            application=["any"],
+            service=["application-default"],
+            category=["any"],
+            action="allow",
+            log_start=False,
+            log_end=True,
+            rule_type="universal",
+            negate_source=False,
+            negate_destination=False,
+            disabled=False,
+            disable_server_response_inspection=False,
+        ),
         preset_values=dict(
             source_zone=["any"],
             source_ip=["any"],
diff --git a/tests/sanity/ignore-2.13.txt b/tests/sanity/ignore-2.13.txt
index 7a32ca901..e168937c9 100644
--- a/tests/sanity/ignore-2.13.txt
+++ b/tests/sanity/ignore-2.13.txt
@@ -85,6 +85,7 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
+plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.14.txt b/tests/sanity/ignore-2.14.txt
index 7a32ca901..e168937c9 100644
--- a/tests/sanity/ignore-2.14.txt
+++ b/tests/sanity/ignore-2.14.txt
@@ -85,6 +85,7 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
+plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.15.txt b/tests/sanity/ignore-2.15.txt
index 7a32ca901..e168937c9 100644
--- a/tests/sanity/ignore-2.15.txt
+++ b/tests/sanity/ignore-2.15.txt
@@ -85,6 +85,7 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
+plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.16.txt b/tests/sanity/ignore-2.16.txt
index 7a32ca901..e168937c9 100644
--- a/tests/sanity/ignore-2.16.txt
+++ b/tests/sanity/ignore-2.16.txt
@@ -85,6 +85,7 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
+plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license

From 6070430e38f1b2675b5d5ce420c767c06384dcea Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Wed, 7 Aug 2024 16:25:55 +0300
Subject: [PATCH 3/5] chore: apply black code formatting

---
 plugins/module_utils/panos.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/module_utils/panos.py b/plugins/module_utils/panos.py
index 90e5e4aa8..6c7ace01f 100644
--- a/plugins/module_utils/panos.py
+++ b/plugins/module_utils/panos.py
@@ -1886,7 +1886,7 @@ def __init__(
         with_state=False,
         with_enabled_state=False,
         *args,
-        **kwargs
+        **kwargs,
     ):
         spec = {}
 

From e51e6cfd4df320f8ec5d736a12ec48e0982b62c6 Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Thu, 8 Aug 2024 12:11:05 +0300
Subject: [PATCH 4/5] fix: invocation param as str while config is a member
 type

Fixes #563
---
 plugins/module_utils/panos.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/module_utils/panos.py b/plugins/module_utils/panos.py
index 6c7ace01f..8bce488b0 100644
--- a/plugins/module_utils/panos.py
+++ b/plugins/module_utils/panos.py
@@ -853,6 +853,8 @@ def apply_state(
                         if isinstance(obj_value, list) or isinstance(item_value, list):
                             if not item_value:
                                 item_value = []
+                            if isinstance(obj_value, str):
+                                obj_value = [obj_value]
                             # if current config or obj to create is one of the preset values
                             # (dropdown options in UI) then replace it with the obj value
                             # since values like "any" can not be in place with other values.

From ab4967213fa6bc99bec99dd9ee3075d8bc820100 Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Thu, 8 Aug 2024 14:57:44 +0300
Subject: [PATCH 5/5] docs(panos_security_rule): move defaults to description

---
 plugins/modules/panos_security_rule.py | 59 +++++++++++---------------
 tests/sanity/ignore-2.13.txt           |  1 -
 tests/sanity/ignore-2.14.txt           |  1 -
 tests/sanity/ignore-2.15.txt           |  1 -
 tests/sanity/ignore-2.16.txt           |  1 -
 5 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/plugins/modules/panos_security_rule.py b/plugins/modules/panos_security_rule.py
index d3525473d..b79b0f20c 100644
--- a/plugins/modules/panos_security_rule.py
+++ b/plugins/modules/panos_security_rule.py
@@ -25,10 +25,12 @@
 short_description: Manage security rule policy on PAN-OS devices or Panorama management console.
 description: >
     - Security policies allow you to enforce rules and take action, and can be as
-      general or specific as needed.
+    general or specific as needed.
     - The policy rules are compared against the incoming traffic in sequence, and
-      because the first rule that matches the traffic is applied, the more specific
-      rules must precede the more general ones.
+    because the first rule that matches the traffic is applied, the more specific
+    rules must precede the more general ones.
+    - Defaults in spec descriptions apply when I(state=present)/I(state=replaced),
+    or when creating a new resource with I(state=merged).
 author:
     - Ivan Bojer (@ivanbojer)
     - Robert Hagen (@stealthllama)
@@ -59,13 +61,12 @@
         type: str
     source_zone:
         description:
-            - List of source zones.
-        default: ["any"]
+            - List of source zones. Defaults to I(["any"]).
         type: list
         elements: str
     source_ip:
         description:
-            - List of source addresses.
+            - List of source addresses. Defaults to I(["any"]).
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -74,13 +75,12 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
-        default: ["any"]
         type: list
         elements: str
     source_user:
-        description:
+        description: >
             - Use users to enforce policy for individual users or a group of users.
-        default: ["any"]
+            Defaults to I(["any"]).
         type: list
         elements: str
     hip_profiles:
@@ -97,13 +97,12 @@
         elements: str
     destination_zone:
         description:
-            - List of destination zones.
-        default: ["any"]
+            - List of destination zones. Defaults to I(["any"]).
         type: list
         elements: str
     destination_ip:
         description:
-            - List of destination addresses.
+            - List of destination addresses. Defaults to I(["any"]).
             - This can be an IP address, an address object/group, etc.
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
@@ -112,34 +111,31 @@
                 panw-highrisk-ip-list      panw-highrisk-ip-list
                 panw-known-ip-list         panw-known-ip-list
                 panw-torexit-ip-list       panw-torexit-ip-list
-        default: ["any"]
         type: list
         elements: str
     application:
-        description:
+        description: >
             - List of applications, application groups, and/or application filters.
-        default: ["any"]
+            Defaults to I(["any"]).
         type: list
         elements: str
     service:
         description:
-            - List of services and/or service groups.
-        default: ['application-default']
+            - List of services and/or service groups. Defaults to I(["application-default"]).
         type: list
         elements: str
     category:
         description:
-            - List of destination URL categories.
+            - List of destination URL categories. Defaults to I(["any"]).
             - When referencing predefined EDLs, use config names of the EDLS not
               their full names. The config names can be found with the CLI...
               request system external-list show type predefined-url name <tab>
                 panw-auth-portal-exclude-list   panw-auth-portal-exclude-list
-        default: ["any"]
         type: list
         elements: str
     action:
         description:
-            - Action to apply to the rule.
+            - Action to apply to the rule. Defaults to I("allow").
         type: str
         choices:
             - allow
@@ -148,20 +144,17 @@
             - reset-client
             - reset-server
             - reset-both
-        default: "allow"
     log_setting:
         description:
             - Log forwarding profile.
         type: str
     log_start:
         description:
-            - Whether to log at session start.
-        default: false
+            - Whether to log at session start. Defaults to I(false).
         type: bool
     log_end:
         description:
-            - Whether to log at session end.
-        default: true
+            - Whether to log at session end. Defaults to I(true).
         type: bool
     description:
         description:
@@ -169,13 +162,12 @@
         type: str
     rule_type:
         description:
-            - Type of security rule (version 6.1 of PanOS and above).
+            - Type of security rule (version 6.1 of PanOS and above). Defaults to I("universal").
         type: str
         choices:
             - universal
             - intrazone
             - interzone
-        default: 'universal'
     tag_name:
         description:
             - List of tags associated with the rule.
@@ -183,18 +175,15 @@
         elements: str
     negate_source:
         description:
-            - Match on the reverse of the 'source_ip' attribute.
-        default: false
+            - Match on the reverse of the 'source_ip' attribute. Defaults to I(false).
         type: bool
     negate_destination:
         description:
-            - Match on the reverse of the 'destination_ip' attribute.
-        default: false
+            - Match on the reverse of the 'destination_ip' attribute. Defaults to I(false).
         type: bool
     disabled:
         description:
-            - Disable this rule.
-        default: false
+            - Disable this rule. Defaults to I(false).
         type: bool
     schedule:
         description:
@@ -205,9 +194,9 @@
             - Send 'ICMP Unreachable'. Used with 'deny', 'drop', and 'reset' actions.
         type: bool
     disable_server_response_inspection:
-        description:
+        description: >
             - Disables packet inspection from the server to the client. Useful under heavy server load conditions.
-        default: false
+            Defaults to I(false).
         type: bool
     group_profile:
         description: >
diff --git a/tests/sanity/ignore-2.13.txt b/tests/sanity/ignore-2.13.txt
index e168937c9..7a32ca901 100644
--- a/tests/sanity/ignore-2.13.txt
+++ b/tests/sanity/ignore-2.13.txt
@@ -85,7 +85,6 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
-plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.14.txt b/tests/sanity/ignore-2.14.txt
index e168937c9..7a32ca901 100644
--- a/tests/sanity/ignore-2.14.txt
+++ b/tests/sanity/ignore-2.14.txt
@@ -85,7 +85,6 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
-plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.15.txt b/tests/sanity/ignore-2.15.txt
index e168937c9..7a32ca901 100644
--- a/tests/sanity/ignore-2.15.txt
+++ b/tests/sanity/ignore-2.15.txt
@@ -85,7 +85,6 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
-plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license
diff --git a/tests/sanity/ignore-2.16.txt b/tests/sanity/ignore-2.16.txt
index e168937c9..7a32ca901 100644
--- a/tests/sanity/ignore-2.16.txt
+++ b/tests/sanity/ignore-2.16.txt
@@ -85,7 +85,6 @@ plugins/modules/panos_sag.py validate-modules:missing-gplv3-license
 plugins/modules/panos_schedule_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule_facts.py validate-modules:missing-gplv3-license
 plugins/modules/panos_security_rule.py validate-modules:missing-gplv3-license
-plugins/modules/panos_security_rule.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/panos_service_group.py validate-modules:missing-gplv3-license
 plugins/modules/panos_service_object.py validate-modules:missing-gplv3-license
 plugins/modules/panos_snapshot_report.py validate-modules:missing-gplv3-license