From 92b475f7212f098b7ea3e4ef46f1f817b25ca27c Mon Sep 17 00:00:00 2001 From: Dag Wieers Date: Fri, 25 Jan 2019 04:23:59 +0100 Subject: [PATCH] mso_contract_filter: Improve logic (#51315) This PR improves the jsonpatch logic. --- lib/ansible/module_utils/network/aci/mso.py | 4 + .../mso_schema_template_contract_filter.py | 122 +++++++++--------- 2 files changed, 67 insertions(+), 59 deletions(-) diff --git a/lib/ansible/module_utils/network/aci/mso.py b/lib/ansible/module_utils/network/aci/mso.py index aca3dbb257..cc51d292c3 100644 --- a/lib/ansible/module_utils/network/aci/mso.py +++ b/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: diff --git a/lib/ansible/modules/network/aci/mso_schema_template_contract_filter.py b/lib/ansible/modules/network/aci/mso_schema_template_contract_filter.py index c876b69d07..53900e1edf 100644 --- a/lib/ansible/modules/network/aci/mso_schema_template_contract_filter.py +++ b/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 ] - default: both-directions + choices: [ both-way, consumer-to-provider, provider-to-consumer ] + 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']), - filter=dict(type='str', aliases=['name']), + contract_scope=dict(type='str', choices=['application-profile', 'global', 'tenant', 'vrf']), + 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 = [ - 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) + ops.append(dict(op='remove', path=contract_path)) else: - # FIXME: Removing based on index is DANGEROUS # Remove filter mso.existing = {} - operations = [ - dict( - op='remove', - path='/templates/{template}/contracts/{contract}/{filter_key}/{filter_idx}'.format( - template=template, - contract=contract, - filter_key=filter_key, - filter_idx=filter_idx, - ), - ), - ] - if not module.check_mode: - mso.request(path, method='PATCH', data=operations) + ops.append(dict(op='remove', path=filter_path)) + + if not module.check_mode: + mso.request(path, method='PATCH', data=ops) 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 = [ - dict(op='add', path='/templates/{template}/contracts/-'.format(template=template), value=payload), - ] + ops.append(dict(op='add', path='/templates/{0}/contracts/-'.format(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 = [ - dict( - op='add', - path='/templates/{template}/contracts/{contract}/{filter_key}/-'.format( - template=template, - contract=contract, - filter_key=filter_key, - ), - value=mso.sent, - ), - ] + ops.append(dict(op='add', path=filters_path + '/-', value=mso.sent)) else: - # FIXME: Updating based on index is DANGEROUS # Filter exists, we have to update it - operations = [ - 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, - ), - ] + ops.append(dict(op='replace', path=filter_path, 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()