Addition of aci_l4l7_policy_based_redirect and related modules (DCNE-391) #751
Conversation
shrsr
requested review from
akinross,
anvitha-jain,
gmicol,
lhercot,
sajagana and
samiib
as code owners
github-actions
bot
changed the title
| - The APIC defaults to C(false) when unset during creation. | ||
| - Only available when I(destination_type=l3) | ||
| type: bool | ||
| monitor_policy: |
There was a problem hiding this comment.
should we use full name?
| monitor_policy: | |
| ip_sla_monitor_policy: |
| policy_name=dict(type="str"), | ||
| description=dict(type="str"), | ||
| destination_type=dict(type="str", aliases=["dest_type"], choices=["l1", "l2", "l3"]), | ||
| hash_algorithm=dict(type="str", choices=["source_ip", "destination_ip", "ip_and_protocol"]), |
There was a problem hiding this comment.
can we get choices from L4L7_HASH_ALGORITHMS_MAPPING?
| hash_algorithm=dict(type="str", choices=["source_ip", "destination_ip", "ip_and_protocol"]), | |
| hash_algorithm=dict(type="str", choices=list(L4L7_HASH_ALGORITHMS_MAPPING), |
| monitor_policy: | ||
| description: | ||
| - The name of the IP SLA Monitoring Policy to bind to the L4-L7 Redirect Policy. | ||
| - To remove an existing binding to an IP SLA Monitoring Policy, submit a request with I(state=present) and no I(monitor_policy) value. |
There was a problem hiding this comment.
I think this behaviour is deviating from the decision of deleting only when specified explicitly. Should we do delete on empty "" and else not touch the child config?
| #!/usr/bin/python | ||
| # -*- coding: utf-8 -*- | ||
|
|
||
| # Copyright: (c) 2025, Tim Cragg (@timcragg) |
There was a problem hiding this comment.
add your name? all modules
| description: More information about the internal APIC class, B(vns:SvcRedirectPol) | ||
| link: https://developer.cisco.com/docs/apic-mim-ref/ | ||
| author: | ||
| - Tim Cragg (@timcragg) |
There was a problem hiding this comment.
add your name? all modules
| module = AnsibleModule( | ||
| argument_spec=argument_spec, | ||
| supports_check_mode=True, | ||
| required_if=[["state", "absent", ["tenant", "policy"]], ["state", "present", ["tenant", "policy"]]], |
There was a problem hiding this comment.
can you format this on multiple lines? run black after the below change
| required_if=[["state", "absent", ["tenant", "policy"]], ["state", "present", ["tenant", "policy"]]], | |
| required_if=[["state", "absent", ["tenant", "policy"]], ["state", "present", ["tenant", "policy"]],], |
| state = module.params.get("state") | ||
| pod_id = module.params.get("pod_id") | ||
|
|
||
| if not destination_type: |
There was a problem hiding this comment.
why do this and not specify a default in the argument spec for destination_type? or make it required?
| if destination_type == "l1/l2" and additional_ip is not None: | ||
| aci.fail_json(msg="You cannot provide an additional_ip when configuring an l1/l2 destination") | ||
| elif destination_type == "l3" and (logical_device, concrete_device, concrete_interface) != (None, None, None): | ||
| aci.fail_json(msg="You cannot provide a logical_device, concrete_device or concrete_interface when configuring an l3 destination") | ||
| elif destination_type == "l1/l2" and (logical_device, concrete_device, concrete_interface) == (None, None, None): | ||
| aci.fail_json(msg="You must provide a logical_device, concrete_device and concrete_interface when configuring an l1/l2 destination") | ||
| elif destination_type == "l1/l2" and ip is not None: | ||
| aci.fail_json(msg="You cannot provide an ip when configuring an l1/l2 destination") |
There was a problem hiding this comment.
have you considered creating a dictionary input for l1/l2 and l3, then deducing the type from the dictionary with a mutually exclusive in the argument spec? and adding the dicts to required if for present, that one of the two is required
this would simplify all the error handling and containerise the config on input side
| health_group: | ||
| description: | ||
| - The Health Group to bind the Policy Based Redirection Destination to. | ||
| - To remove an existing binding from a Health Group, submit a request with I(state=present) and no I(health_group) value. |
There was a problem hiding this comment.
empty "" for removal else not touch?
| ) | ||
| aci.payload( | ||
| aci_class=aci_class, | ||
| class_config=dict(ip=ip, mac=mac, destName=destination_name, podId=pod_id, ip2=additional_ip), |
There was a problem hiding this comment.
format on different lines? run black after
| class_config=dict(ip=ip, mac=mac, destName=destination_name, podId=pod_id, ip2=additional_ip), | |
| class_config=dict(ip=ip, mac=mac, destName=destination_name, podId=pod_id, ip2=additional_ip,), |
No description provided.