From 6eafbcdeec5c675ee4cdcd10920c6ee51edc3348 Mon Sep 17 00:00:00 2001 From: Arun Saravanan Balachandran <52521751+ArunSaravananBalachandran@users.noreply.github.com> Date: Thu, 26 Sep 2024 11:58:33 +0530 Subject: [PATCH] bgp_communities - Update 'aann' option, Fix issues in merged state (#440) * bgp_communities - Update 'aann' option, Fix issues in merged state * Address sanity test failure * Fix ansible-lint failures in bgp_communities * Fix ansible-lint failures in bgp_communities UT fixture * Address review comments --- .../440-bgp-aann-support-merged-fix.yaml | 5 + .../bgp_communities/bgp_communities.py | 55 ++- .../config/bgp_communities/bgp_communities.py | 413 ++++++++++-------- .../facts/bgp_communities/bgp_communities.py | 16 +- plugins/modules/sonic_bgp_communities.py | 50 ++- .../common/tasks/action.facts.report.yaml | 2 +- .../common/tasks/idempotent.facts.report.yaml | 4 +- .../sonic_bgp_communities/defaults/main.yml | 284 ++++++------ .../sonic_bgp_communities/meta/main.yaml | 2 +- .../sonic_bgp_communities/tasks/main.yml | 13 +- .../tasks/preparation_tests.yaml | 7 +- .../tasks/tasks_template.yaml | 19 +- .../sonic/fixtures/sonic_bgp_communities.yaml | 219 +++++++++- 13 files changed, 684 insertions(+), 405 deletions(-) create mode 100644 changelogs/fragments/440-bgp-aann-support-merged-fix.yaml diff --git a/changelogs/fragments/440-bgp-aann-support-merged-fix.yaml b/changelogs/fragments/440-bgp-aann-support-merged-fix.yaml new file mode 100644 index 000000000..54c9d08c7 --- /dev/null +++ b/changelogs/fragments/440-bgp-aann-support-merged-fix.yaml @@ -0,0 +1,5 @@ +--- +breaking_changes: + - sonic_bgp_communities - Change 'aann' option as a suboption of 'members' and update its type from string to list of strings (https://github.com/ansible-collections/dellemc.enterprise_sonic/pull/440). +bugfixes: + - sonic_bgp_communities - Fix issues in merged state for standard community-lists (https://github.com/ansible-collections/dellemc.enterprise_sonic/pull/440). diff --git a/plugins/module_utils/network/sonic/argspec/bgp_communities/bgp_communities.py b/plugins/module_utils/network/sonic/argspec/bgp_communities/bgp_communities.py index c90fab8e9..53f177330 100644 --- a/plugins/module_utils/network/sonic/argspec/bgp_communities/bgp_communities.py +++ b/plugins/module_utils/network/sonic/argspec/bgp_communities/bgp_communities.py @@ -36,24 +36,37 @@ class Bgp_communitiesArgs(object): # pylint: disable=R0903 def __init__(self, **kwargs): pass - argument_spec = {'config': {'elements': 'dict', - 'options': {'aann': {'type': 'str'}, - 'local_as': {'type': 'bool'}, - 'match': {'choices': ['ALL', 'ANY'], - 'default': 'ANY', - 'type': 'str'}, - 'members': {'options': {'regex': {'elements': 'str', - 'type': 'list'}}, - 'type': 'dict'}, - 'name': {'required': True, 'type': 'str'}, - 'no_advertise': {'type': 'bool'}, - 'no_export': {'type': 'bool'}, - 'no_peer': {'type': 'bool'}, - 'permit': {'type': 'bool'}, - 'type': {'choices': ['standard', 'expanded'], - 'default': 'standard', - 'type': 'str'}}, - 'type': 'list'}, - 'state': {'choices': ['merged', 'deleted', 'replaced', 'overridden'], - 'default': 'merged', - 'type': 'str'}} # pylint: disable=C0301 + argument_spec = { + "config": { + "elements": "dict", + "options": { + "local_as": {"type": "bool"}, + "match": { + "choices": ["ALL", "ANY"], + "type": "str" + }, + "members": { + "options": { + "aann": {"elements": "str", "type": "list"}, + "regex": {"elements": "str", "type": "list"} + }, + "type": "dict" + }, + "name": {"required": True, "type": "str"}, + "no_advertise": {"type": "bool"}, + "no_export": {"type": "bool"}, + "no_peer": {"type": "bool"}, + "permit": {"type": "bool"}, + "type": { + "choices": ["standard", "expanded"], + "type": "str" + } + }, + "type": "list" + }, + "state": { + "choices": ["merged", "deleted", "replaced", "overridden"], + "default": "merged", + "type": "str" + } + } # pylint: disable=C0301 diff --git a/plugins/module_utils/network/sonic/config/bgp_communities/bgp_communities.py b/plugins/module_utils/network/sonic/config/bgp_communities/bgp_communities.py index 1f312658b..8827fdff6 100644 --- a/plugins/module_utils/network/sonic/config/bgp_communities/bgp_communities.py +++ b/plugins/module_utils/network/sonic/config/bgp_communities/bgp_communities.py @@ -19,6 +19,7 @@ ) from ansible_collections.ansible.netcommon.plugins.module_utils.network.common.utils import ( to_list, + search_obj_in_list ) from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.facts.facts import Facts from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.utils import ( @@ -36,22 +37,11 @@ sort_config, remove_void_config ) -from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.sonic import to_request from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.sonic import ( to_request, edit_config ) from ansible.module_utils.connection import ConnectionError -import json -from ansible.module_utils._text import to_native -import traceback -try: - import jinja2 - HAS_LIB = True -except Exception as e: - HAS_LIB = False - ERR_MSG = to_native(e) - LIB_IMP_ERR = traceback.format_exc() try: from urllib.parse import urlencode @@ -172,14 +162,10 @@ def set_config(self, existing_bgp_communities_facts): to the desired configuration """ want = self._module.params['config'] + have = existing_bgp_communities_facts if want: - for conf in want: - if conf.get("match", None): - conf["match"] = conf["match"].upper() - if conf.get("members", {}) and conf['members'].get("regex", []): - conf['members']['regex'].sort() + want = self.validate_and_normalize_config(want, have) - have = existing_bgp_communities_facts resp = self.set_state(want, have) return to_list(resp) @@ -195,17 +181,12 @@ def set_state(self, want, have): commands = [] requests = [] state = self._module.params['state'] - diff = get_diff(want, have) - # with open('/root/ansible_log.log', 'a+') as fp: - # fp.write('comm: want: ' + str(want) + '\n') - # fp.write('comm: have: ' + str(have) + '\n') - # fp.write('comm: diff: ' + str(diff) + '\n') if state == 'overridden': commands, requests = self._state_overridden(want, have) elif state == 'deleted': commands, requests = self._state_deleted(want, have) elif state == 'merged': - commands, requests = self._state_merged(want, have, diff) + commands, requests = self._state_merged(want, have) elif state == 'replaced': commands, requests = self._state_replaced(want, have) return commands, requests @@ -238,16 +219,47 @@ def _state_overridden(self, want, have): return commands, requests - def _state_merged(self, want, have, diff): + def _state_merged(self, want, have): """ The command generator when state is merged :rtype: A list :returns: the commands necessary to merge the provided into the current configuration """ - commands = diff - requests = self.get_modify_bgp_community_requests(commands, have, "merged") - if commands and len(requests) > 0: + requests = [] + commands = get_diff(want, have) + + for conf in commands: + have_conf = search_obj_in_list(conf['name'], have, 'name') + if have_conf: + del_attrs = [] + # Matching values will not be available in diff. + # Hence, updating the required fields in diff with the values from have. + for attr in ('type', 'permit', 'match'): + if conf.get(attr) is None: + conf[attr] = have_conf.get(attr) + if conf['type'] == "standard": + for attr in self.standard_communities_map: + if attr in have_conf: + # Delete options that are disabled in want + if conf.get(attr) is False and have_conf[attr]: + del_attrs.append(self.standard_communities_map[attr]) + elif attr not in conf: + conf[attr] = have_conf[attr] + if 'members' not in conf and have_conf.get('members') and have_conf['members'].get('aann'): + conf['members'] = {'aann': have_conf['members']['aann']} + else: + if 'members' not in conf and have_conf.get('members') and have_conf['members'].get('regex'): + conf['members'] = {'regex': have_conf['members']['regex']} + + if del_attrs: + requests.extend(self.get_delete_single_bgp_community_member_requests(conf['name'], del_attrs)) + + new_req = self.get_new_add_request(conf) + if new_req: + requests.append(new_req) + + if len(requests) > 0: commands = update_states(commands, "merged") else: commands = [] @@ -289,17 +301,17 @@ def _state_deleted(self, want, have): def get_delete_single_bgp_community_member_requests(self, name, members): requests = [] + url = ("data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:" + "bgp-defined-sets/community-sets/community-set={name}/config/{members_param}") + method = "DELETE" for member in members: - url = "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:" - url = url + "bgp-defined-sets/community-sets/community-set={name}/config/{members_param}" - method = "DELETE" members_params = {'community-member': member} members_str = urlencode(members_params) request = {"path": url.format(name=name, members_param=members_str), "method": method} requests.append(request) return requests - def get_delete_single_bgp_community_requests(self, name): + def get_delete_single_bgp_community_request(self, name): url = "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set={}" method = "DELETE" request = {"path": url.format(name), "method": method} @@ -322,47 +334,58 @@ def get_delete_bgp_communities(self, commands, have, is_delete_all): for cmd in commands: name = cmd['name'] members = cmd.get('members', None) - cmd_type = cmd['type'] - diff_members = [] + del_options = [] + del_community_list = False for item in have: if item['name'] == name: - if 'permit' not in cmd or cmd['permit'] is None: - cmd['permit'] = item['permit'] + # Delete community-list if only name is specified + if len(cmd.keys()) == 1: + requests.append(self.get_delete_single_bgp_community_request(name)) + break if cmd == item: - requests.append(self.get_delete_single_bgp_community_requests(name)) + requests.append(self.get_delete_single_bgp_community_request(name)) break - if cmd_type == "standard": + member_type = 'aann' if cmd['type'] == 'standard' else 'regex' + if cmd['type'] == "standard": + have_attr = False for attr in self.standard_communities_map: - if cmd.get(attr, None) and item[attr] and cmd[attr] == item[attr]: - diff_members.append(self.standard_communities_map[attr]) + if cmd.get(attr) and item.get(attr) and cmd[attr] == item[attr]: + del_options.append(self.standard_communities_map[attr]) + elif item.get(attr): + have_attr = True if members: - if members.get('regex', []): - for member_want in members['regex']: - if item.get('members', None) and item['members'].get('regex', []): - if str(member_want) in item['members']['regex']: - diff_members.append("REGEX:" + str(member_want)) - else: - requests.append(self.get_delete_single_bgp_community_requests(name)) - - else: - if cmd_type == "standard": - no_attr = True - for attr in self.standard_communities_map: - if cmd.get(attr, None): - no_attr = False - break - if no_attr: - requests.append(self.get_delete_single_bgp_community_requests(name)) - else: - requests.append(self.get_delete_single_bgp_community_requests(name)) - break + if member_type in members: + have_members = set(item['members'][member_type]) if item.get('members') and item['members'].get(member_type) else set() + if members.get(member_type): + del_members = set(members[member_type]).intersection(have_members) + if del_members: + if cmd['type'] == "standard": + del_options.extend(list(del_members)) + else: + del_options = ['REGEX:' + member for member in del_members] + else: + # In case of 'standard' type, if 'members' -> 'aann' is empty + # 1) Delete the whole community-list, if other attributes are also to be deleted (or) not present. + # 2) Delete all 'aann' members otherwise. + # In case of 'expanded' type, if 'members' -> 'regex' is empty then delete the whole community-list. + if cmd['type'] == "standard": + if not have_attr: + del_community_list = True + else: + del_options.extend(list(have_members)) + else: + del_community_list = True + + if del_community_list: + requests.append(self.get_delete_single_bgp_community_request(name)) + elif del_options: + requests.extend(self.get_delete_single_bgp_community_member_requests(name, del_options)) - if diff_members: - requests.extend(self.get_delete_single_bgp_community_member_requests(name, diff_members)) + break return requests @@ -370,92 +393,38 @@ def get_new_add_request(self, conf): url = "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" method = "PATCH" community_members = [] - community_action = "" - - if 'match' not in conf: - conf['match'] = "ANY" if conf['type'] == 'standard': for attr in self.standard_communities_map: if attr in conf and conf[attr]: community_members.append(self.standard_communities_map[attr]) - if 'members' in conf and conf['members'] and conf['members'].get('regex', []): - for i in conf['members']['regex']: + if 'members' in conf and conf['members'] and conf['members'].get('aann', []): + for i in conf['members']['aann']: community_members.extend([str(i)]) - if not community_members: - self._module.fail_json(msg='Cannot create standard community-list {0} without community attributes'.format(conf['name'])) - elif conf['type'] == 'expanded': if 'members' in conf and conf['members'] and conf['members'].get('regex', []): for i in conf['members']['regex']: community_members.extend(["REGEX:" + str(i)]) - if not community_members: - self._module.fail_json(msg='Cannot create expanded community-list {0} without community attributes'.format(conf['name'])) - if conf['permit']: - community_action = "PERMIT" - else: - community_action = "DENY" - - input_data = {'name': conf['name'], 'members_list': community_members, 'match': conf['match'].upper(), 'permit': community_action} - - payload_template = """ - { - "openconfig-bgp-policy:community-sets": { - "community-set": [ - { - "community-set-name": "{{name}}", - "config": { - "community-set-name": "{{name}}", - "community-member": [ - {% for member in members_list %}"{{member}}"{%- if not loop.last -%},{% endif %}{%endfor%} - ], - "openconfig-bgp-policy-ext:action": "{{permit}}", - "match-set-options": "{{match}}" - } - } - ] - } - }""" - env = jinja2.Environment(autoescape=False) - t = env.from_string(payload_template) - intended_payload = t.render(input_data) - ret_payload = json.loads(intended_payload) - request = {"path": url, "method": method, "data": ret_payload} - - return request - - def get_modify_bgp_community_requests(self, commands, have, cur_state): - requests = [] - if not commands: - return requests - - for conf in commands: - if cur_state == "merged": - for item in have: - if item['name'] == conf['name']: - if 'type' not in conf: - conf['type'] = item['type'] - if 'permit' not in conf or conf['permit'] is None: - conf['permit'] = item['permit'] - if 'match' not in conf: - conf['match'] = item['match'] - if conf['type'] == "standard": - for attr in self.standard_communities_map: - if attr not in conf and attr in item: - conf[attr] = item[attr] - else: - if 'members' not in conf: - if item.get('members', {}) and item['members'].get('regex', []): - conf['members'] = {'regex': item['members']['regex']} - else: - conf['members'] = item['members'] - break - - new_req = self.get_new_add_request(conf) - if new_req: - requests.append(new_req) - return requests + if not community_members: + self._module.fail_json(msg='Cannot create {0} community-list {1} without community attributes'.format(conf['type'], conf['name'])) + + payload = { + 'openconfig-bgp-policy:community-sets': { + 'community-set': [ + { + 'community-set-name': conf['name'], + 'config': { + 'community-set-name': conf['name'], + 'community-member': community_members, + 'openconfig-bgp-policy-ext:action': 'PERMIT' if conf['permit'] else 'DENY', + 'match-set-options': conf['match'] + } + } + ] + } + } + return {"path": url, "method": method, "data": payload} def get_replaced_overridden_config(self, want, have, cur_state): commands, requests = [], [] @@ -465,78 +434,136 @@ def get_replaced_overridden_config(self, want, have, cur_state): for conf in want: name = conf['name'] - in_have = False - for have_conf in have: - if have_conf['name'] == name: - in_have = True - if have_conf['type'] != conf['type']: - # If both community list are of same name but different types + have_conf = search_obj_in_list(name, have, 'name') + if have_conf: + is_add = is_delete = False + add_command, del_command = {'name': name}, {'name': name} + del_attrs_members = [] + member_type = 'regex' + for attr in ('type', 'permit', 'match'): + add_command[attr] = conf[attr] + del_command[attr] = have_conf[attr] + if conf[attr] != have_conf[attr]: commands_del.append(have_conf) commands_add.append(conf) - else: - is_change = False - - if have_conf['permit'] != conf['permit']: - is_change = True - - if have_conf['match'] != conf['match']: - is_change = is_delete = True + requests_del.append(self.get_delete_single_bgp_community_request(name)) + requests_add.append(self.get_new_add_request(conf)) + is_delete = True + break - if conf["type"] == "standard": - no_attr = True - for attr in self.standard_communities_map: - if not conf.get(attr, None): - if have_conf.get(attr, None): - is_change = True - else: - no_attr = False - if not have_conf.get(attr, None): - is_change = True - - if no_attr: - # Since standard type needs atleast one attribute to exist - self._module.fail_json(msg='Cannot create standard community-list {0} without community attributes'.format(conf['name'])) - else: - members = conf.get('members', {}) - if members and members.get('regex', []): - if have_conf.get('members', {}) and have_conf['members'].get('regex', []): - if set(have_conf['members']['regex']).symmetric_difference(set(members['regex'])): - is_change = True - else: - # If there are no members in any community list of want, then - # that particular community list request to be ignored since - # expanded type needs community-member to exist - self._module.fail_json(msg='Cannot create expanded community-list {0} without community attributes'.format(conf['name'])) - - if is_change: - commands_add.append(conf) - commands_del.append(have_conf) - break - - if not in_have: + if is_delete: + continue + + if conf['type'] == 'standard': + member_type = 'aann' + for attr in self.standard_communities_map: + if conf.get(attr) and not have_conf.get(attr): + add_command[attr] = conf[attr] + is_add = True + elif not conf.get(attr) and have_conf.get(attr): + del_attrs_members.append(self.standard_communities_map[attr]) + del_command[attr] = have_conf[attr] + is_delete = True + + have_members = set(have_conf['members'][member_type]) if have_conf.get('members') and have_conf['members'].get(member_type) else set() + want_members = set(conf['members'][member_type]) if conf.get('members') and conf['members'].get(member_type) else set() + add_members = want_members - have_members + del_members = have_members - want_members + if add_members: + add_command['members'] = {member_type: list(add_members)} + is_add = True + if del_members: + del_command['members'] = {member_type: list(del_members)} + del_attrs_members.extend(del_command['members'][member_type]) + is_delete = True + + if is_delete: + commands_del.append(del_command) + if is_add: + commands_add.append(add_command) + requests_del.append(self.get_delete_single_bgp_community_request(name)) + requests_add.append(self.get_new_add_request(conf)) + elif is_add: + commands_add.append(add_command) + requests_add.append(self.get_new_add_request(add_command)) + else: commands_add.append(conf) + requests_add.append(self.get_new_add_request(conf)) if cur_state == "overridden": for have_conf in have: in_want = next((conf for conf in want if conf['name'] == have_conf['name']), None) if not in_want: commands_del.append(have_conf) + requests_del.append(self.get_delete_single_bgp_community_request(have_conf['name'])) - if commands_del: - requests_del = self.get_delete_bgp_communities(commands_del, have, False) + if len(requests_del) > 0: + commands.extend(update_states(commands_del, "deleted")) + requests.extend(requests_del) - if len(requests_del) > 0: - commands.extend(update_states(commands_del, "deleted")) - requests.extend(requests_del) + if len(requests_add) > 0: + commands.extend(update_states(commands_add, cur_state)) + requests.extend(requests_add) - if commands_add: - requests_add = self.get_modify_bgp_community_requests(commands_add, have, cur_state) + return commands, requests - if len(requests_add) > 0: - commands.extend(update_states(commands_add, cur_state)) - requests.extend(requests_add) + @staticmethod + def set_default_values(conf): + if conf.get('type') is None: + conf['type'] = 'standard' + if conf.get('permit') is None: + conf['permit'] = False + if conf.get('match') is None: + conf['match'] = 'ANY' - return commands, requests + def validate_and_normalize_config(self, want, have): + state = self._module.params['state'] + if state != 'deleted': + updated_want = remove_empties_from_list(want) + else: + updated_want = want + + for conf in updated_want: + # Empty values for suboptions of member (aann/regex) is supported. + # Hence, remove_empties is not used for deleted state. + delete_name_only = False + if state == 'deleted': + for key, value in list(conf.items()): + if value is None: + del conf[key] + if len(conf.keys()) == 1: + delete_name_only = True + + if state in ('replaced', 'overridden'): + self.set_default_values(conf) + elif state == 'merged' or (state == 'deleted' and not delete_name_only): + have_conf = search_obj_in_list(conf['name'], have, 'name') + if have_conf: + for attr in ('type', 'permit', 'match'): + if conf.get(attr) is None: + conf[attr] = have_conf.get(attr) + if state == 'merged': + self.set_default_values(conf) + + if not delete_name_only: + members = conf.get('members') + if conf.get('type') == 'standard': + if members: + if members.get('regex'): + self._module.fail_json(msg='members -> regex is not applicable for standard community-list {0}'.format(conf['name'])) + if members.get('aann'): + members['aann'].sort() + else: + for attr in self.standard_communities_map: + if conf.get(attr) is not None: + self._module.fail_json(msg='{0} is not applicable for expanded community-list {1}'.format(attr, conf['name'])) + if members: + if members.get('aann'): + self._module.fail_json(msg='members -> aann is not applicable for expanded community-list {0}'.format(conf['name'])) + if members.get('regex'): + members['regex'].sort() + + return updated_want def post_process_generated_config(self, configs): confs = remove_void_config(configs, TEST_KEYS_sort_config) diff --git a/plugins/module_utils/network/sonic/facts/bgp_communities/bgp_communities.py b/plugins/module_utils/network/sonic/facts/bgp_communities/bgp_communities.py index ff4827e61..1bc732bff 100644 --- a/plugins/module_utils/network/sonic/facts/bgp_communities/bgp_communities.py +++ b/plugins/module_utils/network/sonic/facts/bgp_communities/bgp_communities.py @@ -21,6 +21,9 @@ to_request, edit_config ) +from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.utils import ( + remove_empties_from_list +) from ansible.module_utils.connection import ConnectionError @@ -82,10 +85,7 @@ def get_bgp_communities(self): result['type'] = 'standard' if result['type'] == 'standard': - result['local_as'] = None - result['no_advertise'] = None - result['no_export'] = None - result['no_peer'] = None + aann = [] for i in members: if "NO_EXPORT_SUBCONFED" in i: result['local_as'] = True @@ -95,6 +95,12 @@ def get_bgp_communities(self): result['no_export'] = True elif "NOPEER" in i: result['no_peer'] = True + else: + aann.append(i) + + if aann: + aann.sort() + result['members'] = {'aann': aann} bgp_communities_configs.append(result) return bgp_communities_configs @@ -124,7 +130,7 @@ def populate_facts(self, connection, ansible_facts, data=None): facts = {} if objs: params = utils.validate_config(self.argument_spec, {'config': objs}) - facts['bgp_communities'] = params['config'] + facts['bgp_communities'] = remove_empties_from_list(params['config']) ansible_facts['ansible_network_resources'].update(facts) return ansible_facts diff --git a/plugins/modules/sonic_bgp_communities.py b/plugins/modules/sonic_bgp_communities.py index 4ec915ef3..6f24bb697 100644 --- a/plugins/modules/sonic_bgp_communities.py +++ b/plugins/modules/sonic_bgp_communities.py @@ -57,22 +57,17 @@ type: str description: - Whether it is a standard or expanded community-list entry. + - If unspecified, operational default value is C(standard). required: False choices: - standard - expanded - default: standard permit: required: False type: bool description: - Permits or denies this community. - - Default value while adding a new community-list is C(False). - aann: - required: False - type: str - description: - - Community number aa:nn format 0..65535:0..65535; applicable for standard BGP community type. + - If unspecified, operational default value is C(False). local_as: required: False type: bool @@ -97,6 +92,13 @@ required: False type: dict suboptions: + aann: + required: False + type: list + elements: str + version_added: 3.0.0 + description: + - Community number aa:nn format 0..65535:0..65535; applicable for standard BGP community type. regex: type: list elements: str @@ -110,10 +112,10 @@ type: str description: - Matches any/all of the members. + - If unspecified, operational default value is C(ANY). choices: - ALL - ANY - default: ANY state: description: - The state of the configuration after module completion. @@ -147,7 +149,7 @@ permit: false members: regex: - - 302 + - 302 state: deleted # After state: @@ -256,7 +258,7 @@ # permit 101 # permit 302 -- name: Add a new BGP community-list +- name: Add new BGP community-lists dellemc.enterprise_sonic.sonic_bgp_communities: config: - name: test2 @@ -264,7 +266,14 @@ permit: true members: regex: - - 909 + - 909 + - name: test3 + type: standard + permit: true + no_peer: true + members: + aann: + - 1000:10 state: merged # After state: @@ -276,6 +285,9 @@ # permit 302 # Expanded community list test2: match: ANY # permit 909 +# Standard community list test3: match: ANY +# permit 1000:10 +# permit no-peer # Using replaced @@ -298,7 +310,13 @@ type: expanded members: regex: - - 301 + - 301 + - name: test2 + type: standard + members: + aann: + - 1000:10 + - 2000:20 - name: test3 type: standard no_advertise: true @@ -316,6 +334,9 @@ # Expanded community list test1: match: ANY # deny 101 # deny 302 +# Standard community list test2: match: ANY +# deny 1000:10 +# deny 2000:10 # Standard community list test3: match: ALL # deny no-advertise # deny no-peer @@ -341,7 +362,7 @@ type: expanded members: regex: - - 301 + - 301 state: overridden # After state: @@ -350,8 +371,7 @@ # show bgp community-list # Expanded community list test3: match: ANY # deny 301 - - +# """ RETURN = """ before: diff --git a/tests/regression/roles/common/tasks/action.facts.report.yaml b/tests/regression/roles/common/tasks/action.facts.report.yaml index 8c8ee5e51..6b4a65c56 100644 --- a/tests/regression/roles/common/tasks/action.facts.report.yaml +++ b/tests/regression/roles/common/tasks/action.facts.report.yaml @@ -1,6 +1,6 @@ - set_fact: ansible_facts: - test_reports: "{{ ansible_facts['test_reports']| default({})| combine({module_name: {item.name+'.1': { + test_reports: "{{ ansible_facts['test_reports']| default({})| combine({ ansible_parent_role_names[0].split('sonic_')[-1]: {item.name+'.1': { 'status': action_condition, 'module_stderr': action_task_output.module_stderr | default(action_task_output.msg | default('No Error')), 'before': action_task_output.before | default('Not defined'), diff --git a/tests/regression/roles/common/tasks/idempotent.facts.report.yaml b/tests/regression/roles/common/tasks/idempotent.facts.report.yaml index adeec696c..269873482 100644 --- a/tests/regression/roles/common/tasks/idempotent.facts.report.yaml +++ b/tests/regression/roles/common/tasks/idempotent.facts.report.yaml @@ -1,6 +1,6 @@ - set_fact: ansible_facts: - test_reports: "{{ ansible_facts['test_reports']| default({})| combine({module_name: {item.name+'.2': { + test_reports: "{{ ansible_facts['test_reports']| default({})| combine({ ansible_parent_role_names[0].split('sonic_')[-1]: {item.name+'.2': { 'status': idempotnet_condition, 'module_stderr': idempotent_task_output.module_stderr | default(idempotent_task_output.msg | default('No Error')), 'before': idempotent_task_output.before | default('Not defined'), @@ -9,4 +9,4 @@ 'configs': item.input | default('Not defined'), 'msg': idempotent_task_output.msg | default('Not defined'), }}}, recursive=True) }}" - # no_log: true \ No newline at end of file + # no_log: true diff --git a/tests/regression/roles/sonic_bgp_communities/defaults/main.yml b/tests/regression/roles/sonic_bgp_communities/defaults/main.yml index cf2f82d43..c5b60fb1f 100644 --- a/tests/regression/roles/sonic_bgp_communities/defaults/main.yml +++ b/tests/regression/roles/sonic_bgp_communities/defaults/main.yml @@ -1,8 +1,7 @@ --- ansible_connection: httpapi -module_name: bgp_communities -tests: +sonic_bgp_communities_tests: - name: test_case_01 description: BGP Communities properties state: merged @@ -20,182 +19,207 @@ tests: permit: false no_export: true match: ALL + members: + aann: + - 111:222 - name: test_case_02 description: Update created BGP properties state: merged input: - - name: test - type: expanded - permit: true - match: ANY - members: - regex: - - "12" - - "13" - - 14 - - name: test2 - type: standard - permit: false - no_peer: true - match: ALL + - name: test + type: expanded + permit: true + match: ANY + members: + regex: + - "12" + - "13" + - 14 + - name: test2 + type: standard + permit: false + no_peer: true + no_advertise: true + match: ALL + members: + aann: + - 333:444 - name: test_case_03 description: Update1 created BGP properties state: merged input: - - name: test - type: expanded - permit: false - match: ANY - members: - regex: - - "11" - - "12" - - name: test2 - type: standard - permit: true - match: ALL + - name: test + type: expanded + permit: false + match: ANY + members: + regex: + - "11" + - "12" + - name: test2 + type: standard + permit: true + match: ALL - name: test_case_04 description: Delete BGP properties state: deleted input: - - name: test - type: expanded - members: - regex: - - "12" - - "13" - - name: test2 - type: standard - permit: false - match: ALL - no_export: true - no_peer: true + - name: test + type: expanded + members: + regex: + - "12" + - "13" + - name: test2 + type: standard + permit: false + match: ALL + no_export: true + no_peer: true + members: + aann: + - 111:222 - name: test_case_05 description: Delete1 BGP properties state: deleted input: - - name: test - type: expanded - members: - regex: + - name: test + type: expanded + members: + regex: + - name: test2 + type: standard + members: + aann: - name: test_case_06 description: Update2 BGP properties state: merged input: - - name: test - type: expanded - match: ANY - permit: true - members: - regex: - - 201 - - name: test3 - type: expanded - match: ALL - permit: true - members: - regex: - - "110" - - 111 + - name: test + type: expanded + match: ANY + permit: true + members: + regex: + - 201 + - name: test3 + type: expanded + match: ALL + permit: true + members: + regex: + - "110" + - 111 - name: test_case_07 description: Replace BGP properties state: replaced input: - - name: test - type: standard - local_as: true - permit: true - - name: test2 - type: expanded - match: ALL - permit: false - members: - regex: - - "220" - - 222 - - "123" + - name: test + type: standard + local_as: true + permit: true + members: + aann: + - 4500:5500 + - 5500:6500 + - 6500:7500 + - name: test2 + type: expanded + match: ALL + permit: false + members: + regex: + - "220" + - 222 + - "123" - name: test_case_08 description: Replace2 BGP properties state: replaced input: - - name: test4 - type: standard - permit: true - no_peer: true - - name: test5 - type: expanded - members: - regex: - - 113 - permit: true + - name: test4 + type: standard + permit: true + no_peer: true + - name: test5 + type: expanded + members: + regex: + - 113 + permit: true - name: test_case_09 description: Override BGP properties state: overridden input: - - name: test3 - type: standard - local_as: True - permit: false - - name: test2 - type: standard - permit: true - no_export: true + - name: test3 + type: standard + local_as: true + permit: false + - name: test2 + type: standard + permit: true + no_export: true + members: + aann: + - 111:222 - name: test_case_10 description: Override2 BGP properties state: overridden input: - - name: test3 - type: standard - permit: false - no_export: true - - name: test4 - type: expanded - permit: false - members: - regex: - - 113 - - name: test2 - type: standard - permit: true - no_export: true + - name: test3 + type: standard + permit: false + no_export: true + - name: test4 + type: expanded + permit: false + members: + regex: + - 113 + - name: test2 + type: standard + permit: true + no_export: true - name: test_case_11 description: Override3 BGP properties state: overridden input: - - name: test4 - type: expanded - permit: false - members: - regex: - - 113 - - name: test2 - type: standard - local_as: true - no_peer: true - no_advertise: true - permit: true - no_export: true + - name: test4 + type: expanded + permit: false + members: + regex: + - 113 + - name: test2 + type: standard + local_as: true + no_peer: true + no_advertise: true + permit: true + no_export: true + members: + aann: + - 65000:65100 - name: test_case_12 description: Override4 BGP properties state: overridden input: - - name: test4 - type: expanded - permit: false - members: - regex: - - 113 - - name: test2 - type: standard - local_as: true - no_advertise: true - permit: true - no_export: true + - name: test4 + type: expanded + permit: false + members: + regex: + - 113 + - name: test2 + type: standard + local_as: true + no_advertise: true + permit: true + no_export: true - name: test_case_13 description: Delete2 BGP properties state: deleted input: - - name: test4 + - name: test4 - name: test_case_14 description: Delete2 BGP properties state: deleted diff --git a/tests/regression/roles/sonic_bgp_communities/meta/main.yaml b/tests/regression/roles/sonic_bgp_communities/meta/main.yaml index 611fd54d2..d0ceaf6f5 100644 --- a/tests/regression/roles/sonic_bgp_communities/meta/main.yaml +++ b/tests/regression/roles/sonic_bgp_communities/meta/main.yaml @@ -2,4 +2,4 @@ collections: - dellemc.enterprise_sonic dependencies: - - { role: common } \ No newline at end of file + - { role: common } diff --git a/tests/regression/roles/sonic_bgp_communities/tasks/main.yml b/tests/regression/roles/sonic_bgp_communities/tasks/main.yml index 975b757be..a568eaeb1 100644 --- a/tests/regression/roles/sonic_bgp_communities/tasks/main.yml +++ b/tests/regression/roles/sonic_bgp_communities/tasks/main.yml @@ -1,8 +1,7 @@ -- debug: msg="{{ module_name }} Test started ..." +--- +- name: "Preparations for test" + ansible.builtin.include_tasks: preparation_tests.yaml -- name: Preparations test - include_tasks: preparation_tests.yaml - -- name: "Test {{ module_name }} started ..." - include_tasks: tasks_template.yaml - loop: "{{ tests }}" +- name: "Test started ..." + ansible.builtin.include_tasks: tasks_template.yaml + loop: "{{ sonic_bgp_communities_tests }}" diff --git a/tests/regression/roles/sonic_bgp_communities/tasks/preparation_tests.yaml b/tests/regression/roles/sonic_bgp_communities/tasks/preparation_tests.yaml index d204af4eb..0447e1a90 100644 --- a/tests/regression/roles/sonic_bgp_communities/tasks/preparation_tests.yaml +++ b/tests/regression/roles/sonic_bgp_communities/tasks/preparation_tests.yaml @@ -1,5 +1,6 @@ -- name: Deletes old bgp sonic_bgp_communities - sonic_bgp_communities: +--- +- name: Delete old sonic_bgp_communities + dellemc.enterprise_sonic.sonic_bgp_communities: config: [] state: deleted - ignore_errors: yes \ No newline at end of file + failed_when: false diff --git a/tests/regression/roles/sonic_bgp_communities/tasks/tasks_template.yaml b/tests/regression/roles/sonic_bgp_communities/tasks/tasks_template.yaml index e875d051c..38a1b19ad 100644 --- a/tests/regression/roles/sonic_bgp_communities/tasks/tasks_template.yaml +++ b/tests/regression/roles/sonic_bgp_communities/tasks/tasks_template.yaml @@ -1,23 +1,24 @@ -- name: "{{ item.name}} , {{ item.description}}" - sonic_bgp_communities: +--- +- name: "{{ item.name ~ ' , ' ~ item.description }}" + dellemc.enterprise_sonic.sonic_bgp_communities: config: "{{ item.input }}" state: "{{ item.state }}" register: action_task_output ignore_errors: true -- import_role: +- name: "Update test report" + ansible.builtin.import_role: name: common tasks_from: action.facts.report.yaml - -- name: "{{ item.name}} , {{ item.description}} Idempotent" - sonic_bgp_communities: + +- name: "{{ item.name ~ ' , ' ~ item.description ~ ' Idempotent' }}" + dellemc.enterprise_sonic.sonic_bgp_communities: config: "{{ item.input }}" state: "{{ item.state }}" register: idempotent_task_output ignore_errors: true -- import_role: +- name: "Update test report" + ansible.builtin.import_role: name: common tasks_from: idempotent.facts.report.yaml - -- debug: var=action_task_output \ No newline at end of file diff --git a/tests/unit/modules/network/sonic/fixtures/sonic_bgp_communities.yaml b/tests/unit/modules/network/sonic/fixtures/sonic_bgp_communities.yaml index 5caca21b4..572d0d705 100644 --- a/tests/unit/modules/network/sonic/fixtures/sonic_bgp_communities.yaml +++ b/tests/unit/modules/network/sonic/fixtures/sonic_bgp_communities.yaml @@ -4,10 +4,10 @@ merged_01: config: - name: test members: - regex: + regex: - 808.* match: ALL - permit: True + permit: true type: expanded existing_bgp_config: - path: "/data/openconfig-network-instance:network-instances/network-instance=default/protocols/protocol=BGP,bgp/bgp/global" @@ -47,15 +47,23 @@ merged_01: - REGEX:808.* openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + merged_02: module_args: config: - name: test type: standard - permit: True - local_as: False - no_export: True - no_peer: True + permit: true + local_as: false + no_export: true + no_peer: true + - name: test1 + type: standard + permit: true + no_export: false + members: + aann: + - 800:900 existing_bgp_config: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" response: @@ -70,7 +78,17 @@ merged_02: - NO_ADVERTISE openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - NO_ADVERTISE + - NO_EXPORT + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' expected_config_requests: + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test1/config/community-member=NO_EXPORT" + method: "delete" - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" method: "patch" data: @@ -84,16 +102,38 @@ merged_02: - NOPEER - NO_EXPORT openconfig-bgp-policy-ext:action: 'PERMIT' - match-set-options: 'ANY' + match-set-options: 'ALL' + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" + method: "patch" + data: + openconfig-bgp-policy:community-sets: + community-set: + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - NO_ADVERTISE + - 800:900 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' + deleted_01: module_args: config: - name: test type: expanded members: - regex: + regex: - 808.* permit: true + - name: test1 + type: standard + members: + aann: + - 600:700 + - 800:900 + permit: true + - name: test2 state: deleted existing_bgp_config: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" @@ -112,6 +152,23 @@ deleted_01: - REGEX:772.* openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - 400:500 + - 600:700 + - 800:900 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' + - community-set-name: 'test2' + config: + community-set-name: 'test2' + community-member: + - REGEX:818.* + - REGEX:929.* + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' - path: "data/sonic-vrf:sonic-vrf/VRF/VRF_LIST" response: code: 200 @@ -121,20 +178,33 @@ deleted_01: expected_config_requests: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test/config/community-member=REGEX%3A808.%2A" method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test1/config/community-member=600%3A700" + method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test1/config/community-member=800%3A900" + method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test2" + method: "delete" + deleted_02: module_args: config: - name: test type: expanded match: ALL - permit: True + permit: true members: regex: + - name: test1 + type: standard + match: ALL + permit: true + members: + aann: - name: test2 type: standard match: ANY - permit: False - local_as: True + permit: false + local_as: true state: deleted existing_bgp_config: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" @@ -153,6 +223,14 @@ deleted_02: - REGEX:888.* openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - 600:700 + - 800:900 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' - community-set-name: 'test2' config: community-set-name: 'test2' @@ -172,8 +250,11 @@ deleted_02: expected_config_requests: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test" method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test1" + method: "delete" - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test2/config/community-member=NO_EXPORT_SUBCONFED" method: "delete" + deleted_03: module_args: config: @@ -212,8 +293,10 @@ replaced_01: regex: - 808.* match: ALL - permit: True + permit: true type: expanded + - name: test1 + local_as: true state: replaced existing_bgp_config: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" @@ -260,16 +343,36 @@ replaced_01: - REGEX:808.* openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" + method: "patch" + data: + openconfig-bgp-policy:community-sets: + community-set: + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - NO_EXPORT_SUBCONFED + openconfig-bgp-policy-ext:action: 'DENY' + match-set-options: 'ANY' replaced_02: module_args: config: + - name: test1 + type: standard + permit: true + match: ANY + no_peer: true + members: + aann: + - 100:200 - name: test2 members: regex: - 808.* match: ALL - permit: False + permit: false type: expanded state: replaced existing_bgp_config: @@ -286,6 +389,13 @@ replaced_02: - NO_ADVERTISE openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ANY' + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - NO_EXPORT_SUBCONFED + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ANY' - community-set-name: 'test2' config: community-set-name: 'test2' @@ -303,8 +413,23 @@ replaced_02: sonic-vrf:VRF_LIST: - vrf_name: default expected_config_requests: + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test1" + method: "delete" - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test2" method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" + method: "patch" + data: + openconfig-bgp-policy:community-sets: + community-set: + - community-set-name: 'test1' + config: + community-set-name: 'test1' + community-member: + - NOPEER + - 100:200 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ANY' - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" method: "patch" data: @@ -323,14 +448,28 @@ overridden_01: config: - name: test type: standard - permit: True - local_as: True - no_export: True - no_peer: True + permit: true + local_as: true + no_export: true + no_peer: true - name: test1 - no_advertise: True + no_advertise: true permit: true type: standard + - name: test2 + type: expanded + members: + regex: + - 707.* + - 818.* + - 929.* + - name: test4 + match: ALL + permit: true + members: + aann: + - 100:200 + - 500:600 state: overridden existing_bgp_config: - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" @@ -363,6 +502,23 @@ overridden_01: - REGEX:888.* openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ALL' + - community-set-name: 'test2' + config: + community-set-name: 'test2' + community-member: + - REGEX:818.* + - REGEX:929.* + openconfig-bgp-policy-ext:action: 'DENY' + match-set-options: 'ANY' + - community-set-name: 'test4' + config: + community-set-name: 'test4' + community-member: + - 100:200 + - 300:400 + - 500:600 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL' - path: "data/sonic-vrf:sonic-vrf/VRF/VRF_LIST" response: code: 200 @@ -374,6 +530,8 @@ overridden_01: method: "delete" - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test3" method: "delete" + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets/community-set=test4" + method: "delete" - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" method: "patch" data: @@ -388,3 +546,28 @@ overridden_01: - NOPEER openconfig-bgp-policy-ext:action: 'PERMIT' match-set-options: 'ANY' + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" + method: "patch" + data: + openconfig-bgp-policy:community-sets: + community-set: + - community-set-name: 'test2' + config: + community-set-name: 'test2' + community-member: + - REGEX:707.* + openconfig-bgp-policy-ext:action: 'DENY' + match-set-options: 'ANY' + - path: "data/openconfig-routing-policy:routing-policy/defined-sets/openconfig-bgp-policy:bgp-defined-sets/community-sets" + method: "patch" + data: + openconfig-bgp-policy:community-sets: + community-set: + - community-set-name: 'test4' + config: + community-set-name: 'test4' + community-member: + - 100:200 + - 500:600 + openconfig-bgp-policy-ext:action: 'PERMIT' + match-set-options: 'ALL'