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

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).

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

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

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,15 +258,22 @@
 # 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
  type: expanded
  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:

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'),

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_log: true