From 9f8028fcc63c70691577818f1ca03f8602c49e9c Mon Sep 17 00:00:00 2001
From: Sabari Jaganathan <93724860+sajagana@users.noreply.github.com>
Date: Wed, 30 Oct 2024 14:31:53 +0530
Subject: [PATCH] [ignore] Changed the bfd multi-hop attribute structure in
 ndo_l3out_node_group_policy module

---
 .../modules/ndo_l3out_node_group_policy.py    | 147 ++++++++++--------
 .../tasks/main.yml                            |  91 +++++++----
 2 files changed, 147 insertions(+), 91 deletions(-)

diff --git a/plugins/modules/ndo_l3out_node_group_policy.py b/plugins/modules/ndo_l3out_node_group_policy.py
index 7afec1ee..9a1cd819 100644
--- a/plugins/modules/ndo_l3out_node_group_policy.py
+++ b/plugins/modules/ndo_l3out_node_group_policy.py
@@ -48,22 +48,32 @@
     - The name of the L3Out Node Routing Policy.
     - Providing an empty string will remove the O(node_routing_policy="") from L3Out Node Group Policy.
     type: str
-  bfd_multi_hop_authentication:
+  bfd:
     description:
-    - The Bidirectional Forwarding Detection (BFD) multi-hop authentication of the L3Out Node Group Policy.
-    - To enable the O(bfd_multi_hop_authentication) BGP routing protocol must be configured on the L3Out.
-    - To clear the BFD auth configuration, provide the values O(bfd_multi_hop_authentication=""), O(bfd_multi_hop_key=""), and O(bfd_multi_hop_key_id=0).
-    type: str
-    choices: [ enabled, disabled, "" ]
-  bfd_multi_hop_key_id:
-    description:
-    - The BFD multi-hop key ID of the L3Out Node Group Policy.
-    - Providing 0 will remove the O(bfd_multi_hop_key_id=0) from L3Out Node Group Policy.
-    type: int
-  bfd_multi_hop_key:
-    description:
-    - The BFD multi-hop key of the L3Out Node Group Policy.
-    type: str
+    - The Bidirectional Forwarding Detection (BFD) multi-hop configuration of the L3Out Node Group Policy.
+    type: dict
+    suboptions:
+      state:
+        description:
+        - Use C(enabled) to configure the BFD multi-hop.
+        - Use C(disabled) to remove the BFD multi-hop.
+        type: str
+        choices: [ enabled, disabled ]
+      auth:
+        description:
+        - The BFD multi-hop authentication of the L3Out Node Group Policy.
+        type: str
+        choices: [ enabled, disabled ]
+        aliases: [ bfd_multi_hop_authentication ]
+      key_id:
+        description:
+        - The BFD multi-hop key ID of the L3Out Node Group Policy.
+        type: int
+      key:
+        description:
+        - The BFD multi-hop key of the L3Out Node Group Policy.
+        type: str
+    aliases: [ bfd_multi_hop ]
   target_dscp:
     description:
     - The DSCP Level of the L3Out Node Group Policy.
@@ -131,13 +141,14 @@
     name: node_group_policy_1
     description: "Updated description"
     node_routing_policy: ans_node_policy_group_1
-    bfd_multi_hop_authentication: enabled
-    bfd_multi_hop_key_id: 1
-    bfd_multi_hop_key: TestKey
+    bfd:
+      auth: enabled
+      key_id: 1
+      key: TestKey
     target_dscp: af11
     state: present
 
-- name: Query a L3Out node group policy
+- name: Query an existing L3Out node group policy with name
   cisco.mso.ndo_l3out_node_group_policy:
     host: mso_host
     username: admin
@@ -148,6 +159,16 @@
     state: query
   register: query_with_name
 
+- name: Query all L3Out node group policy
+  cisco.mso.ndo_l3out_node_group_policy:
+    host: mso_host
+    username: admin
+    password: SomeSecretPassword
+    template: l3out_template
+    l3out: l3out
+    state: query
+  register: query_all
+
 - name: Delete an existing L3Out node group policy with name
   cisco.mso.ndo_l3out_node_group_policy:
     host: mso_host
@@ -179,9 +200,16 @@ def main():
         name=dict(type="str", aliases=["l3out_node_group_policy"]),
         description=dict(type="str"),
         node_routing_policy=dict(type="str"),
-        bfd_multi_hop_authentication=dict(type="str", choices=["enabled", "disabled", ""]),
-        bfd_multi_hop_key_id=dict(type="int"),
-        bfd_multi_hop_key=dict(type="str", no_log=True),
+        bfd=dict(
+            type="dict",
+            options=dict(
+                state=dict(type="str", choices=["enabled", "disabled"]),
+                auth=dict(type="str", choices=["enabled", "disabled"], aliases=["bfd_multi_hop_authentication"]),
+                key_id=dict(type="int"),
+                key=dict(type="str", no_log=True),
+            ),
+            aliases=["bfd_multi_hop"],
+        ),
         target_dscp=dict(type="str", choices=list(TARGET_DSCP_MAP)),
         state=dict(type="str", default="query", choices=["absent", "query", "present"]),
     )
@@ -202,9 +230,7 @@ def main():
     name = module.params.get("name")
     description = module.params.get("description")
     node_routing_policy = module.params.get("node_routing_policy")
-    bfd_multi_hop_authentication = module.params.get("bfd_multi_hop_authentication")
-    bfd_multi_hop_key_id = module.params.get("bfd_multi_hop_key_id")
-    bfd_multi_hop_key = module.params.get("bfd_multi_hop_key")
+    bfd = module.params.get("bfd")
     target_dscp = TARGET_DSCP_MAP.get(module.params.get("target_dscp"))
     state = module.params.get("state")
 
@@ -258,37 +284,35 @@ def main():
                 ops.append(dict(op="replace", path=node_group_policy_path + "/nodeRoutingPolicyRef", value=node_routing_policy))
                 proposed_payload["nodeRoutingPolicyRef"] = node_routing_policy
 
-            if bfd_multi_hop_authentication or bfd_multi_hop_key or bfd_multi_hop_key_id:
-                if not mso.existing.get("bfdMultiHop"):
-                    ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop", value=dict()))
-                    proposed_payload["bfdMultiHop"] = dict()
-
-                # Ignore when bfd_multi_hop_authentication is None or empty string
-                if bfd_multi_hop_authentication and mso.existing.get("bfdMultiHop", {}).get("authEnabled") is not (
-                    True if bfd_multi_hop_authentication == "enabled" else False
-                ):
-                    ops.append(
-                        dict(
-                            op="replace",
-                            path=node_group_policy_path + "/bfdMultiHop/authEnabled",
-                            value=True if bfd_multi_hop_authentication == "enabled" else False,
+            if bfd:
+                if bfd.get("state") == "disabled" and proposed_payload.get("bfdMultiHop"):
+                    proposed_payload.pop("bfdMultiHop")
+                    ops.append(dict(op="remove", path=node_group_policy_path + "/bfdMultiHop"))
+                else:
+                    if not proposed_payload.get("bfdMultiHop"):
+                        proposed_payload["bfdMultiHop"] = dict()
+                        ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop", value=dict()))
+
+                    if bfd.get("auth") and mso.existing.get("bfdMultiHop", {}).get("authEnabled") is not (True if bfd.get("auth") == "enabled" else False):
+                        ops.append(
+                            dict(
+                                op="replace",
+                                path=node_group_policy_path + "/bfdMultiHop/authEnabled",
+                                value=True if bfd.get("auth") == "enabled" else False,
+                            )
                         )
-                    )
-                    proposed_payload["bfdMultiHop"]["authEnabled"] = True if bfd_multi_hop_authentication == "enabled" else False
+                        proposed_payload["bfdMultiHop"]["authEnabled"] = True if bfd.get("auth") == "enabled" else False
 
-                if bfd_multi_hop_key_id is not None and mso.existing.get("bfdMultiHop", {}).get("keyID") != bfd_multi_hop_key_id:
-                    ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/keyID", value=bfd_multi_hop_key_id))
-                    proposed_payload["bfdMultiHop"]["keyID"] = bfd_multi_hop_key_id
+                    if bfd.get("key_id") is not None and mso.existing.get("bfdMultiHop", {}).get("keyID") != bfd.get("key_id"):
+                        ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/keyID", value=bfd.get("key_id")))
+                        proposed_payload["bfdMultiHop"]["keyID"] = bfd.get("key_id")
 
-                if bfd_multi_hop_key is not None:
-                    ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/key", value=dict()))
-                    ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/key/value", value=bfd_multi_hop_key))
-                    proposed_payload["bfdMultiHop"]["key"] = dict(value=bfd_multi_hop_key)
+                    if bfd.get("key") is not None:
+                        if not isinstance(proposed_payload["bfdMultiHop"].get("key"), dict):
+                            ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/key", value=dict()))
 
-            # Clear the complete BFD multi-hop configuration when all parameters are empty
-            elif bfd_multi_hop_authentication == "" and bfd_multi_hop_key == "" and bfd_multi_hop_key_id == 0 and mso.existing.get("bfdMultiHop"):
-                ops.append(dict(op="remove", path=node_group_policy_path + "/bfdMultiHop"))
-                proposed_payload.pop("bfdMultiHop")
+                        ops.append(dict(op="replace", path=node_group_policy_path + "/bfdMultiHop/key/value", value=bfd.get("key")))
+                        proposed_payload["bfdMultiHop"]["key"] = dict(value=bfd.get("key"))
 
             if target_dscp is not None and mso.existing.get("targetDscp") != target_dscp:
                 ops.append(dict(op="replace", path=node_group_policy_path + "/targetDscp", value=target_dscp))
@@ -304,19 +328,20 @@ def main():
             if l3out_node_routing_policy_object:
                 payload["nodeRoutingPolicyRef"] = l3out_node_routing_policy_object.details.get("uuid")
 
-            bfd_multi_hop = dict()
+            if bfd:
+                bfd_multi_hop = dict()
 
-            if bfd_multi_hop_authentication:
-                bfd_multi_hop["authEnabled"] = True if bfd_multi_hop_authentication == "enabled" else False
+                if bfd.get("auth"):
+                    bfd_multi_hop["authEnabled"] = True if bfd.get("auth") == "enabled" else False
 
-            if bfd_multi_hop_key_id:
-                bfd_multi_hop["keyID"] = bfd_multi_hop_key_id
+                if bfd.get("key_id"):
+                    bfd_multi_hop["keyID"] = bfd.get("key_id")
 
-            if bfd_multi_hop_key:
-                bfd_multi_hop["key"] = dict(value=bfd_multi_hop_key)
+                if bfd.get("key"):
+                    bfd_multi_hop["key"] = dict(value=bfd.get("key"))
 
-            if bfd_multi_hop:
-                payload["bfdMultiHop"] = bfd_multi_hop
+                if bfd_multi_hop:
+                    payload["bfdMultiHop"] = bfd_multi_hop
 
             if target_dscp:
                 payload["targetDscp"] = target_dscp
diff --git a/tests/integration/targets/ndo_l3out_node_group_policy/tasks/main.yml b/tests/integration/targets/ndo_l3out_node_group_policy/tasks/main.yml
index fdecf7cd..1d640e23 100644
--- a/tests/integration/targets/ndo_l3out_node_group_policy/tasks/main.yml
+++ b/tests/integration/targets/ndo_l3out_node_group_policy/tasks/main.yml
@@ -257,9 +257,10 @@
         <<: *nm_node_group_policy_1_present
         description: "Test description"
         node_routing_policy: ans_node_policy_group_1
-        bfd_multi_hop_authentication: enabled
-        bfd_multi_hop_key_id: 1
-        bfd_multi_hop_key: TestKey
+        bfd:
+          auth: enabled
+          key_id: 1
+          key: TestKey
         target_dscp: af11
         state: present
       check_mode: true
@@ -328,9 +329,10 @@
         name: "node_group_policy_1"
         description: "Test description updated"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: enabled
-        bfd_multi_hop_key_id: 2
-        bfd_multi_hop_key: TestKeyUpdated
+        bfd:
+          auth: enabled
+          key_id: 2
+          key: TestKeyUpdated
         target_dscp: af12
         state: present
       register: update_node_group_policy_attrs
@@ -360,7 +362,8 @@
         name: "node_group_policy_1"
         description: ""
         node_routing_policy: ""
-        bfd_multi_hop_authentication: disabled
+        bfd:
+          auth: disabled
         target_dscp: "unspecified"
         state: present
         output_level: debug
@@ -460,9 +463,10 @@
         l3out: "l3out_2"
         name: "node_group_policy_30"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: enabled
-        bfd_multi_hop_key_id: 2
-        bfd_multi_hop_key: TestKeyUpdated
+        bfd:
+          auth: enabled
+          key_id: 2
+          key: TestKeyUpdated
         target_dscp: af12
         state: present
       register: add_ngp_3_auth_disabled
@@ -485,9 +489,10 @@
         l3out: "l3out_2"
         name: "node_group_policy_3"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: disabled
-        bfd_multi_hop_key_id: 2
-        bfd_multi_hop_key: TestKeyUpdated
+        bfd:
+          auth: disabled
+          key_id: 2
+          key: TestKeyUpdated
         target_dscp: af12
         state: present
       register: add_ngp_3_auth_disabled
@@ -509,7 +514,8 @@
         template: '{{ ansible_l3out_template | default("ansible_test") }}'
         l3out: "l3out_2"
         name: "node_group_policy_3"
-        bfd_multi_hop_key_id: 3
+        bfd:
+          key_id: 3
         state: present
       register: update_ngp_3_with_key_id
 
@@ -534,7 +540,8 @@
         template: '{{ ansible_l3out_template | default("ansible_test") }}'
         l3out: "l3out_2"
         name: "node_group_policy_3"
-        bfd_multi_hop_key: TestKeyUpdated1
+        bfd:
+          key: TestKeyUpdated1
         state: present
       register: update_ngp_3_with_key
 
@@ -560,7 +567,8 @@
         l3out: "l3out_2"
         name: "node_group_policy_4"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: disabled
+        bfd:
+          auth: disabled
         target_dscp: af12
         state: present
       register: add_ngp_4_auth_disabled
@@ -581,7 +589,8 @@
         template: '{{ ansible_l3out_template | default("ansible_test") }}'
         l3out: "l3out_2"
         name: "node_group_policy_4"
-        bfd_multi_hop_key_id: 10
+        bfd:
+          key_id: 10
         state: present
       register: add_key_id_ngp_4_auth_disabled
 
@@ -606,8 +615,9 @@
         template: '{{ ansible_l3out_template | default("ansible_test") }}'
         l3out: "l3out_2"
         name: "node_group_policy_4"
-        bfd_multi_hop_authentication: disabled
-        bfd_multi_hop_key_id: 0
+        bfd:
+          auth: disabled
+          key_id: 0
         state: present
         output_level: debug
       register: clear_key_id_ngp_4_auth_disabled
@@ -656,8 +666,9 @@
         l3out: "l3out_2"
         name: "node_group_policy_5"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_key_id: 3
-        bfd_multi_hop_key: TestKeyUpdated1
+        bfd:
+          key_id: 3
+          key: TestKeyUpdated1
         state: present
       register: update_ngp_5_only_id_value
 
@@ -681,7 +692,8 @@
         l3out: "l3out_2"
         name: "node_group_policy_5"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: enabled
+        bfd:
+          auth: enabled
         state: present
       register: enable_ngp_5_auth_without_key
       ignore_errors: true
@@ -699,8 +711,9 @@
         l3out: "l3out_2"
         name: "node_group_policy_5"
         node_routing_policy: ans_node_policy_group_2
-        bfd_multi_hop_authentication: enabled
-        bfd_multi_hop_key: TestKeyUpdated4
+        bfd:
+          auth: enabled
+          key: TestKeyUpdated4
         state: present
       register: update_ngp_5_bfd_config
 
@@ -726,9 +739,8 @@
         l3out: "l3out_2"
         name: "node_group_policy_5"
         node_routing_policy: ""
-        bfd_multi_hop_authentication: ""
-        bfd_multi_hop_key_id: 0
-        bfd_multi_hop_key: ""
+        bfd:
+          state: disabled
         state: present
       register: clear_ngp_5_bfd_config
 
@@ -814,9 +826,10 @@
         l3out: "l3out_1"
         name: "node_group_policy_nt"
         node_routing_policy: ans_node_policy_group_1
-        bfd_multi_hop_authentication: enabled
-        bfd_multi_hop_key_id: 12
-        bfd_multi_hop_key: "test"
+        bfd:
+          auth: enabled
+          key_id: 12
+          key: "test"
         state: present
       register: node_group_policy_nt_with_l3out_1
       ignore_errors: true
@@ -827,6 +840,24 @@
           - node_group_policy_nt_with_l3out_1 is changed
           - node_group_policy_nt_with_l3out_1.msg == "MSO Error 400{{':'}} Invalid configuration in L3Out 'l3out_1'{{':'}} node group 'node_group_policy_nt'{{':'}} BFD Multihop is not supported with non-BGP routing protocols. Current protocol{{':'}} none"
 
+    - name: Create ngp1 with bfd state disabled
+      cisco.mso.ndo_l3out_node_group_policy:
+        <<: *mso_info
+        template: '{{ ansible_l3out_template | default("ansible_test") }}'
+        l3out: "l3out_1"
+        name: "ngp1"
+        bfd:
+          state: disabled
+        state: present
+      register: create_ngp1_bfd_disabled
+
+    - name: Assertion check for create ngp1 with bfd state disabled
+      ansible.builtin.assert:
+        that:
+          - create_ngp1_bfd_disabled is changed
+          - create_ngp1_bfd_disabled.current.name == "ngp1"
+          - create_ngp1_bfd_disabled.current.bfdMultiHop is not defined
+
     # Cleanup Part
     - name: Remove l3out tenant template
       cisco.mso.ndo_template: