mso_contract_filter: Improve logic (#51315)

This PR improves the jsonpatch logic.

lib/ansible/module_utils/network/aci/mso.py

@@ -180,6 +180,10 @@ class MSOModule(object):
         if method is not None:
             self.method = method
 
+        # If we PATCH with empty operations, return
+        if method == 'PATCH' and not data:
+            return {}
+
         self.url = urljoin(self.baseuri, path)
 
         if qs is not None:

lib/ansible/modules/network/aci/mso_schema_template_contract_filter.py

@@ -37,10 +37,18 @@ options:
   contract_display_name:
     description:
     - The name as displayed on the MSO web interface.
+    - This defaults to the contract name when unset on creation.
     type: str
+  contract_filter_type:
+    description:
+    - The type of filters defined in this contract.
+    - This defaults to C(both-way) when unset on creation.
+    type: str
+    choices: [ both-way, one-way ]
   contract_scope:
     description:
-    - The scope of the contract
+    - The scope of the contract.
+    - This defaults to C(vrf) when unset on creation.
     type: str
     choices: [ application-profile, global, tenant, vrf ]
   filter:
@@ -60,8 +68,8 @@ options:
     description:
     - The type of filter to manage.
     type: str
-    choices: [ both-directions, consumer-to-provider, provider-to-consumer ]
+    choices: [ both-way, consumer-to-provider, provider-to-consumer ]
-    default: both-directions
+    default: both-way
     aliases: [ type ]
   state:
     description:
@@ -132,7 +140,7 @@ from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.network.aci.mso import MSOModule, mso_argument_spec, mso_reference_spec, issubset
 
 FILTER_KEYS = {
-    'both-directions': 'filterRelationships',
+    'both-way': 'filterRelationships',
     'consumer-to-provider': 'filterRelationshipsConsumerToProvider',
     'provider-to-consumer': 'filterRelationshipsProviderToConsumer',
 }
@@ -143,13 +151,14 @@ def main():
     argument_spec.update(
         schema=dict(type='str', required=True),
         template=dict(type='str', required=True),
-        contract=dict(type='str', required=False),  # This parameter is not required for querying all objects
+        contract=dict(type='str', required=True),
         contract_display_name=dict(type='str'),
-        contract_scope=dict(type='str', required=True, choices=['application-profile', 'global', 'tenant', 'vrf']),
+        contract_scope=dict(type='str', choices=['application-profile', 'global', 'tenant', 'vrf']),
-        filter=dict(type='str', aliases=['name']),
+        contract_filter_type=dict(type='str', choices=['both-way', 'one-way']),
+        filter=dict(type='str', required=False, aliases=['name']),  # This parameter is not required for querying all objects
         filter_template=dict(type='str'),
         filter_schema=dict(type='str'),
-        filter_type=dict(type='str', default='both-directions', choices=FILTER_KEYS.keys(), aliases=['type']),
+        filter_type=dict(type='str', default='both-way', choices=FILTER_KEYS.keys(), aliases=['type']),
         state=dict(type='str', default='present', choices=['absent', 'present', 'query']),
     )
 
@@ -166,6 +175,7 @@ def main():
     template = module.params['template']
     contract = module.params['contract']
     contract_display_name = module.params['contract_display_name']
+    contract_filter_type = module.params['contract_filter_type']
     contract_scope = module.params['contract_scope']
     filter_name = module.params['filter']
     filter_template = module.params['filter_template']
@@ -173,6 +183,13 @@ def main():
     filter_type = module.params['filter_type']
     state = module.params['state']
 
+    contract_ftype = 'bothWay' if contract_filter_type == 'both-way' else 'oneWay'
+
+    if contract_filter_type == 'both-way' and filter_type != 'both-way':
+        module.warn("You are adding 'one-way' filters to a 'both-way' contract")
+    elif contract_filter_type != 'both-way' and filter_type == 'both-way':
+        module.warn("You are adding 'both-way' filters to a 'one-way' contract")
+
     if filter_template is None:
         filter_template = template
 
@@ -203,7 +220,7 @@ def main():
     # Get contracts
     mso.existing = {}
     contract_idx = None
-    entry_idx = None
+    filter_idx = None
     contracts = [c['name'] for c in schema_obj['templates'][template_idx]['contracts']]
 
     if contract in contracts:
@@ -216,12 +233,20 @@ def main():
             mso.existing = schema_obj['templates'][template_idx]['contracts'][contract_idx][filter_key][filter_idx]
 
     if state == 'query':
+        if contract_idx is None:
+            mso.fail_json(msg="Provided contract '{0}' does not exist. Existing contracts: {1}".format(contract, ', '.join(contracts)))
+
         if filter_name is None:
-            mso.existing = schema_obj['templates'][template_idx]['contractss'][contract_idx][filter_key]
+            mso.existing = schema_obj['templates'][template_idx]['contracts'][contract_idx][filter_key]
         elif not mso.existing:
             mso.fail_json(msg="FilterRef '{filter_ref}' not found".format(filter_ref=filter_ref))
         mso.exit_json()
 
+    ops = []
+    contract_path = '/templates/{0}/contracts/{1}'.format(template, contract)
+    # FIXME: Removing based on index is DANGEROUS
+    filter_path = '/templates/{0}/contracts/{1}/{2}/{3}'.format(template, contract, filter_key, filter_idx)
+
     mso.previous = mso.existing
     if state == 'absent':
         mso.proposed = mso.sent = {}
@@ -235,28 +260,14 @@ def main():
         elif len(filters) == 1:
             # There is only one filter, remove contract
             mso.existing = {}
-            operations = [
+            ops.append(dict(op='remove', path=contract_path))
-                dict(op='remove', path='/templates/{template}/contracts/{contract}'.format(template=template, contract=contract)),
-            ]
-            if not module.check_mode:
-                mso.request(path, method='PATCH', data=operations)
         else:
-            # FIXME: Removing based on index is DANGEROUS
             # Remove filter
             mso.existing = {}
-            operations = [
+            ops.append(dict(op='remove', path=filter_path))
-                dict(
+
-                    op='remove',
+        if not module.check_mode:
-                    path='/templates/{template}/contracts/{contract}/{filter_key}/{filter_idx}'.format(
+            mso.request(path, method='PATCH', data=ops)
-                        template=template,
-                        contract=contract,
-                        filter_key=filter_key,
-                        filter_idx=filter_idx,
-                    ),
-                ),
-            ]
-            if not module.check_mode:
-                mso.request(path, method='PATCH', data=operations)
 
     elif state == 'present':
 
@@ -271,55 +282,48 @@ def main():
 
         mso.sanitize(payload, collate=True)
         mso.existing = mso.sent
+        ops = []
+        contract_path = '/templates/{0}/contracts/{1}'.format(template, contract)
+        filters_path = '/templates/{0}/contracts/{1}/{2}'.format(template, contract, filter_key)
+        # FIXME: Updating based on index is DANGEROUS
+        filter_path = '/templates/{0}/contracts/{1}/{2}/{3}'.format(template, contract, filter_key, filter_idx)
 
         if contract_idx is None:
             # COntract does not exist, so we have to create it
             if contract_display_name is None:
                 contract_display_name = contract
+            if contract_filter_type is None:
+                contract_ftype = 'bothWay'
+            if contract_scope is None:
+                contract_scope = 'context'
 
             payload = {
                 'name': contract,
                 'displayName': contract_display_name,
+                'filterType': contract_ftype,
                 'scope': contract_scope,
-                filter_key: [mso.sent],
             }
 
-            operations = [
+            ops.append(dict(op='add', path='/templates/{0}/contracts/-'.format(template), value=payload))
-                dict(op='add', path='/templates/{template}/contracts/-'.format(template=template), value=payload),
+        else:
-            ]
+            # Contract exists, but may require an update
+            if contract_display_name is not None:
+                ops.append(dict(op='replace', path=contract_path + '/displayName', value=contract_display_name))
+            if contract_filter_type is not None:
+                ops.append(dict(op='replace', path=contract_path + '/filterType', value=contract_ftype))
+            if contract_scope is not None:
+                ops.append(dict(op='replace', path=contract_path + '/scope', value=contract_scope))
 
-        elif filter_idx is None:
+        if filter_idx is None:
             # Filter does not exist, so we have to add it
-            operations = [
+            ops.append(dict(op='add', path=filters_path + '/-', value=mso.sent))
-                dict(
-                    op='add',
-                    path='/templates/{template}/contracts/{contract}/{filter_key}/-'.format(
-                        template=template,
-                        contract=contract,
-                        filter_key=filter_key,
-                    ),
-                    value=mso.sent,
-                ),
-            ]
 
         else:
-            # FIXME: Updating based on index is DANGEROUS
             # Filter exists, we have to update it
-            operations = [
+            ops.append(dict(op='replace', path=filter_path, value=mso.sent))
-                dict(
-                    op='replace',
-                    path='/templates/{template}/contracts/{contract}/{filter_key}/{filter_idx}'.format(
-                        template=template,
-                        contract=contract,
-                        filter_key=filter_key,
-                        filter_idx=filter_idx,
-                    ),
-                    value=mso.sent,
-                ),
-            ]
 
         if not module.check_mode:
-            mso.request(path, method='PATCH', data=operations)
+            mso.request(path, method='PATCH', data=ops)
 
     mso.exit_json()