Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat[dcnm_policy]: adding the functions to handle use_desc_as_key #285

Merged
merged 10 commits into from
Jun 18, 2024
Merged

feat[dcnm_policy]: adding the functions to handle use_desc_as_key #285

Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5b38684
adding the functions to handle use_desc_as_key
Apr 18, 2024
ddfab82
adding doc for the new parameter
Apr 18, 2024
48efb15
add bulk update API enpoints for dcnm 11
Apr 18, 2024
0d38eb3
fix a pep8 error
Apr 18, 2024
a49a359
address comments and fix a bug when templateNmae is updated, wrong te…
Apr 18, 2024
72fde1b
adding support for the state query, update the document and examples
Apr 19, 2024
7858625
fix pep8 errors
Apr 19, 2024
dfcb67c
fix yamllint error
Apr 19, 2024
28fe6e5
fix yamllint error
Apr 19, 2024
22989ec
address comments and update the doc
May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 127 additions & 16 deletions plugins/modules/dcnm_policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@
- query
default: merged

use_desc_as_key:
description:
- A flag specifying whether using the description as unique key for policies.
dsx1123 marked this conversation as resolved.
Show resolved Hide resolved
type: bool
required: false
default: false

deploy:
description:
- A flag specifying if a policy is to be deployed on the switches
Expand Down Expand Up @@ -363,6 +370,7 @@ class DcnmPolicy:
"POLICY_WITH_ID": "/rest/control/policies/{}",
"POLICY_GET_SWITCHES": "/rest/control/policies/switches?serialNumber={}",
"POLICY_BULK_CREATE": "/rest/control/policies/bulk-create",
"POLICY_BULK_UPDATE": "/rest/control/policies/{}/bulk",
"POLICY_MARK_DELETE": "/rest/control/policies/{}/mark-delete",
"POLICY_DEPLOY": "/rest/control/policies/deploy",
"POLICY_CFG_DEPLOY": "/rest/control/fabrics/{}/config-deploy/",
Expand All @@ -373,6 +381,7 @@ class DcnmPolicy:
"POLICY_WITH_ID": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/{}",
"POLICY_GET_SWITCHES": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/switches?serialNumber={}",
"POLICY_BULK_CREATE": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/bulk-create",
"POLICY_BULK_UPDATE": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/{}/bulk",
"POLICY_MARK_DELETE": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/{}/mark-delete",
"POLICY_DEPLOY": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/policies/deploy",
"POLICY_CFG_DEPLOY": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/control/fabrics/{}/config-deploy/",
Expand All @@ -385,6 +394,7 @@ def __init__(self, module):
self.module = module
self.params = module.params
self.fabric = module.params["fabric"]
self.use_desc_as_key = module.params["use_desc_as_key"]
self.config = copy.deepcopy(module.params.get("config"))
self.deploy = True # Global 'deploy' flag
self.pb_input = []
Expand Down Expand Up @@ -470,6 +480,10 @@ def dcnm_policy_validate_input(self):
cfg["name"], invalid_params
)
self.module.fail_json(msg=mesg)
# Fail the module when use_desc_as_key is True but description is not given or empty
if self.use_desc_as_key and cfg.get("description", "") == "":
mesg = f"description can't be empty when use_desc_as_key is True: {cfg}"
self.module.fail_json(msg=mesg)
self.policy_info.extend(policy_info)

def dcnm_get_policy_payload_with_template_name(self, pelem, sw):
Expand Down Expand Up @@ -637,12 +651,20 @@ def dcnm_policy_get_have(self):
pl
for pl in plist
for wp in self.want
if (pl["templateName"] == wp["templateName"])
if pl.get("source", "") == ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this check for? Not clear. Can you please add a comment for pl.get("source", "")?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add the comment, here is the reason:
exclude the policies that have the source
when the user modifies a policy but has not deployed the policy,
a sub-policy might be created with the same description name, but marked as deleted
The signature of this kind of policy is that it has an original policyId as the source
it should be excluded from the match list as the policy will be deleted once the user deploys the configuration

and (
(pl["templateName"] == wp["templateName"])
or (
not self.use_desc_as_key
or (pl.get("description") == wp.get("description", ""))
)
)
]

# match_pol can be a list of dicts, containing duplicates. Remove the duplicate entries
# also exclude the policies that are marked for deletion
for pol in match_pol:
if pol not in self.have:
if pol not in self.have and not pol.get("deleted", True):
self.have.append(pol)

def dcnm_policy_compare_nvpairs(self, pnv, hnv):
Expand All @@ -658,8 +680,12 @@ def dcnm_policy_compare_nvpairs(self, pnv, hnv):
return "DCNM_POLICY_MATCH"

def dcnm_policy_compare_policies(self, policy):

found = False
# use list to instead of boolean
# need to handle two templates associated with switch have the same description
# when use_desc_as_key is true, raise error
found = []
match = False
template_changed = False

if self.have == []:
return ("DCNM_POLICY_ADD_NEW", None)
Expand All @@ -669,15 +695,25 @@ def dcnm_policy_compare_policies(self, policy):

if policy.get("policyId", None) is not None:
key = "policyId"
elif self.use_desc_as_key:
key = "description"
else:
key = "templateName"

for have in self.have:
if (have[key] == policy[key]) and (
have.get("serialNumber", None) == policy["serialNumber"]
):
found = True
# Have a policy with matching template name. Check for other objects
found.append(have)
# if use description as key, use policyId got from the target
# if templateName is changed, remove the original policy and create a new one
if self.use_desc_as_key:
policy["policyId"] = have.get("policyId")
if have["templateName"] != policy["templateName"]:
template_changed = True
continue

# Have a policy with matching key. Check for other objects
if have.get("description", None) == policy["description"]:
if have.get("priority", None) == policy["priority"]:
if (
Expand All @@ -687,11 +723,22 @@ def dcnm_policy_compare_policies(self, policy):
)
== "DCNM_POLICY_MATCH"
):
return ("DCNM_POLICY_DONT_ADD", have["policyId"])
if found is True:
match = True

if len(found) == 1 and not match and not template_changed:
# Found a matching policy with the given template name, but other objects don't match.
# Go ahead and merge the objects into the existing policy
return ("DCNM_POLICY_MERGE", have["policyId"])
return ("DCNM_POLICY_MERGE", found[0]["policyId"])
elif len(found) == 1 and not match and template_changed:
return ("DCNM_POLICY_TEMPLATE_CHANGED", found[0]["policyId"])
elif len(found) == 1 and match:
return ("DCNM_POLICY_DONT_ADD", found[0]["policyId"])
elif len(found) > 1 and self.use_desc_as_key:
# module will raise error when duplicated description is found
return ("DCNM_POLICY_DUPLICATED", None)
elif len(found) > 1 and not self.use_desc_as_key:
# if not using description as the key, new
return ("DCNM_POLICY_DONT_ADD", found[0]["policyId"])
else:
return ("DCNM_POLICY_ADD_NEW", None)

Expand All @@ -715,7 +762,11 @@ def dcnm_policy_get_diff_merge(self):

rc, policy_id = self.dcnm_policy_compare_policies(policy)

if rc == "DCNM_POLICY_ADD_NEW":
if rc == "DCNM_POLICY_DUPLICATED":
self.module.fail_json(
f"Policies with the same description are found in DCNM/NDFC: {self.use_desc_as_key}, {policy['description']}"
dsx1123 marked this conversation as resolved.
Show resolved Hide resolved
)
elif rc == "DCNM_POLICY_ADD_NEW":
# A policy does not exists, create a new one. Even if one exists, if create_additional_policy
# is specified, then create the policy
if (policy not in self.diff_create) or (
Expand All @@ -730,6 +781,11 @@ def dcnm_policy_get_diff_merge(self):
# will not know which policy the user is referring to. In the case where a user is providing
# a templateName and we are here, ignore the policy.
if policy.get("policyId", None) is not None:
# id is needed for policy update
id = policy["policyId"].split("-")[1]
policy["id"] = id
policy["policy_id_given"] = True

if policy not in self.diff_modify:
self.changed_dict[0]["merged"].append(policy)
self.diff_modify.append(policy)
Expand Down Expand Up @@ -760,6 +816,21 @@ def dcnm_policy_get_diff_merge(self):
self.changed_dict[0]["merged"].append(policy)
self.diff_create.append(policy)
policy_id = None
elif rc == "DCNM_POLICY_TEMPLATE_CHANGED":
# A policy exists and the template name is changed
# Remove the existing policy and create a new one
del_payload = self.dcnm_policy_get_delete_payload(policy)
if del_payload not in self.diff_delete:
self.diff_delete.append(del_payload)
self.changed_dict[0]["deleted"].append(
{
"policy": policy["policyId"],
"templateName": policy["templateName"],
}
)
policy.pop("policyId")
self.changed_dict[0]["merged"].append(policy)
self.diff_create.append(policy)

# Check the 'deploy' flag and decide if this policy is to be deployed
if self.deploy is True:
Expand Down Expand Up @@ -814,13 +885,22 @@ def dcnm_policy_get_diff_deleted(self):
for pl in plist
for wp in self.want
if (
(wp["policy_id_given"] is False)
and (pl["templateName"] == wp["templateName"])
or (wp["policy_id_given"] is True)
and (pl["policyId"] == wp["policyId"])
not pl["deleted"]
and (
(wp["policy_id_given"] is False)
and (pl["templateName"] == wp["templateName"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the template names match even in the case of use_desc_as_key is True. May be in that case we should just compare description alone. Specifically can there be two policies with different template names but same description? If yes, then we should delete policies matching description if use_desc_as_key is True.

Copy link
Author

@dsx1123 dsx1123 May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea of use_desc_as_key is to only use the description as the key, the use case it can support is, that when the user is using a different template for the same description, the module will delete the old one and create a new one with the new template.

and (
# When use_desc_as_key is True, only add the policy match the description
not self.use_desc_as_key
or (pl.get("description", "") == wp.get("description", ""))
)
)
or (
(wp["policy_id_given"] is True)
and (pl["policyId"] == wp["policyId"])
)
)
]

# match_pol contains all the policies which exist and are to be deleted
# Build the delete payloads

Expand Down Expand Up @@ -943,6 +1023,35 @@ def dcnm_policy_create_policy(self, policy, command):

return resp

def dcnm_policy_update_policy(self, policy, command):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary. you could have used create_policy with 'PUT' command instead. Any specific reason for having a new routine for update?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried the PUT method using POLICY_BULK_CREATE URL, it didn't work, besides, it is not documented. So prefer to use the documented endpoint for the update.


path = self.paths["POLICY_BULK_UPDATE"].format(policy["policyId"])

json_payload = json.dumps([policy])

retries = 0
while retries < 3:
resp = dcnm_send(self.module, command, path, json_payload)

if (
(resp.get("DATA", None) is not None)
and (isinstance(resp["DATA"], dict))
and (resp["DATA"].get("failureList", None) is not None)
):
if isinstance(resp["DATA"]["failureList"], list):
fl = resp["DATA"]["failureList"][0]
else:
fl = resp["DATA"]["failureList"]

if "is not unique" in fl.get("message", ""):
retries = retries + 1
continue
break

self.result["response"].append(resp)

return resp

def dcnm_policy_delete_policy(self, policy, mark_del):

if mark_del is True:
Expand Down Expand Up @@ -1137,7 +1246,8 @@ def dcnm_policy_send_message_to_dcnm(self):
for policy in self.diff_modify:
# POP the 'create_additional_policy' object before sending create
policy.pop("create_additional_policy")
resp = self.dcnm_policy_create_policy(policy, "PUT")
policy.pop("policy_id_given", "")
resp = self.dcnm_policy_update_policy(policy, "PUT")
if isinstance(resp, list):
resp = resp[0]
if (
Expand Down Expand Up @@ -1262,6 +1372,7 @@ def main():
element_spec = dict(
fabric=dict(required=True, type="str"),
config=dict(required=False, type="list", elements="dict"),
use_desc_as_key=dict(required=False, type="bool", default=False),
state=dict(
type="str",
default="merged",
Expand Down
Loading