Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BGP ASDOT notation handling in ios_bgp_global module #1124

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Girish5tri
Copy link
Member

@Girish5tri Girish5tri commented Oct 9, 2024

SUMMARY
This PR fixes an issue with the ios_bgp_global module where it fails to handle BGP Autonomous System numbers in ASDOT notation, specifically for the local_as command. The module was previously trying to convert ASDOT notation numbers (e.g., "500.65083") to integers, which caused errors when parsing BGP configurations

To resolve this, we implement deprecation of the "number" parameter in the local_as configuration. The number parameter(type int) has been deprecated in favor of asn(type string) in the bgp_global.py argspec file. This modification allows the module to correctly process ASDOT notation by treating these numbers as strings instead of integers.

Fixes : #1096

ISSUE TYPE

  • Bugfix Pull Request

COMPONENT NAME
cisco.ios.bgp_global

ADDITIONAL INFORMATION
The issue occurred when trying to retrieve or configure BGP information with configurations using ASDOT notation in the local_as command. This resulted in the following error:

fatal: [xxxx]: FAILED! => changed=false 
msg: 'argument ''number'' is of type <class ''float''> found in ''config -> neighbors -> local_as''. and we were unable to convert to int: <class ''float''> cannot be converted to an int'

configuration:

router bgp xxxx
 bgp asnotation dot
 bgp log-neighbor-changes
 bgp deterministic-med
 bgp graceful-restart
 no bgp default ipv4-unicast
 neighbor x.x.x.x remote-as 500.65083
 neighbor x.x.x.x local-as 501.65083 no-prepend replace-as

To fix this, the changes were made in the bgp_global.py argspec, rm templates and config files.

"local_as": {
    "type": "dict",
    "options": {
        "set": {"type": "bool"},
         "number": {
                  "type": "int",
                   "removed_at_date": "2027-01-31",
                   "removed_from_collection": "cisco.ios",
         },
         "asn": {"type": "str"},

These change allows the module to accept and correctly process ASDOT notation in the local_as command. unit and integration tests have been added to verify the correct handling of ASDOT notation in this context.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.06%. Comparing base (c9841b5) to head (c05cc8d).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
+ Coverage   86.90%   87.06%   +0.16%     
==========================================
  Files         193      199       +6     
  Lines       11904    12083     +179     
==========================================
+ Hits        10345    10520     +175     
- Misses       1559     1563       +4     

see 81 files with indirect coverage changes

@Ruchip16 Ruchip16 requested a review from roverflow October 9, 2024 09:34
@Ruchip16 Ruchip16 marked this pull request as ready for review October 9, 2024 09:35
@KB-perByte
Copy link
Collaborator

KB-perByte commented Oct 15, 2024

@Girish5tri The local_as.number parameter is updated from int to string that might impact the end user anywhere they have gathered facts and it is safe to consider it as a user-facing change.
The changes looks good. But we must follow a proper deprecation cycle.

local_as:
   number: int <- deprecated - remove at/as of jan 2027
   asn: str

Recommended changes -

facts - never render number always render asn (basically a change in rm_templates, and other required places for facts generation)
config - handle the deprecation if local_as.number is given from playbook (want) update the data under asn and pop off number.

Reference -

def handle_deprecates(self, want, is_nbr=False):
< handle deprecation

- This option is DEPRECATED and is replaced with fan_enable which accepts bool as input
< module deprecation note

CC @roverflow

@roverflow roverflow self-requested a review October 16, 2024 09:29
plugins/action/ios.py Outdated Show resolved Hide resolved
@Girish5tri Girish5tri marked this pull request as draft October 17, 2024 11:11
@Girish5tri Girish5tri marked this pull request as ready for review October 18, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asdot does not support under bgp local_as command
3 participants