feat(anta): Added testcase to verify the BGP Redistributed Routes #993
Conversation
geetanjalimanegslab
changed the title
geetanjalimanegslab
changed the title
CodSpeed Performance ReportMerging #993 will not alter performanceComparing Summary
|
anta/input_models/routing/bgp.py
Outdated
| """ | ||
| Can be enabled in the `VerifyBGPPeerCount` tests.""" | ||
| redistributed_route_protocol: Redistributed_Protocol | None = None | ||
| """Specify redistributed route protocol.""" |
There was a problem hiding this comment.
| """Specify redistributed route protocol.""" | |
| """Specify redistributed route protocol. Required field in the `VerifyBGPRedistributedRoutes` test.""" |
Also add validator in test case.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
geetanjalimanegslab
force-pushed
the
test_bgp_redistributed_routes
branch
from
16ed8ec to
145dae6
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
anta/custom_types.py
Outdated
| @@ -263,3 +263,4 @@ def validate_regex(value: str) -> str: | |||
| SnmpHashingAlgorithm = Literal["MD5", "SHA", "SHA-224", "SHA-256", "SHA-384", "SHA-512"] | |||
| SnmpEncryptionAlgorithm = Literal["AES-128", "AES-192", "AES-256", "DES"] | |||
| DynamicVlanSource = Literal["dmf", "dot1x", "dynvtep", "evpn", "mlag", "mlagsync", "mvpn", "swfwd", "vccbfd"] | |||
| RedistributedProtocol = Literal["AttachedHost", "Bgp", "Connected", "Dynamic", "IS-IS", "OSPF Internal", "OSPFv3 Internal", "RIP", "Static", "User"] | |||
There was a problem hiding this comment.
We need to add OSPF External, OSPF Nssa-External, same for OSPFv3. We can also redistribute specific IS-IS levels; level-1, level-2, level-1-2 so please double check the options.
anta/tests/routing/bgp.py
Outdated
| @@ -1685,3 +1688,89 @@ def test(self) -> None: | |||
|
|
|||
| if (actual_origin := get_value(route_path, "routeType.origin")) != origin: | |||
| self.result.is_failure(f"{route} {path} - Origin mismatch - Actual: {actual_origin}") | |||
|
|
|||
|
|
|||
| class VerifyBGPRedistributedRoutes(AntaTest): | |||
There was a problem hiding this comment.
I would call this test VerifyBGPRedistribution instead since we are not checking anything related to redistributed routes.
anta/input_models/routing/bgp.py
Outdated
| Can be enabled in the `VerifyBGPPeerCount` tests.""" | ||
| redistributed_route_protocol: RedistributedProtocol | None = None | ||
| """Specify redistributed route protocol. Required field in the `VerifyBGPRedistributedRoutes` test.""" | ||
| route_map: str | None = None | ||
| """Specify redistributed route protocol route map. Required field in the `VerifyBGPRedistributedRoutes` test.""" |
There was a problem hiding this comment.
Let's use separate models for the show bgp instance related tests. Something like this:
class RedistributedRoute(BaseModel):
proto: RedistributedProtocol # Custom type that you already created
include_leaked: bool | None = None # We should also check this for protocols that it applies
route_map: str | None = None # This is optional, we can redistribute without a route-map
class AfiSafiConfig(BaseModel):
name: Literal["v4u", "v4m"] # Here we should also support other formats like `IPv4 Unicast` or `ipv4Unicast`
redistributed_routes: list[RedistributedRoute]
class BgpVrf(BaseModel):
name: str = "default"
address_families: list[AfiSafiConfig]
# With this structure we can easily add more fields in the future for other tests.
# router_id: str
# local_as: intThere was a problem hiding this comment.
Actually AddressFamilyConfig might be more appropriate versus AfiSafiConfig
There was a problem hiding this comment.
Hi @carl-baillargeon.
Thank you for providing the input model. I've implemented the test case based on that. Here's the current input model:
- VerifyBGPRedistribution:
vrfs:
- vrf: default
address_families:
- afi_safi: ipv4Unicast
redistributed_routes:
- proto: Connected
include_leaked: True
route_map: RM-CONN-2-BGP
- proto: Static
include_leaked: True
route_map: RM-CONN-2-BGP
- afi_safi: IPv6 Unicast
redistributed_routes:
- proto: Dynamic
route_map: RM-CONN-2-BGP
- proto: Static
include_leaked: True
route_map: RM-CONN-2-BGP
However, I'm encountering an issue with Cognitive Complexity. To resolve this, I've proposed an alternative input model approach:
- VerifyBGPRedistribution:
address_families:
- afi_safi: ipv4Unicast
vrf: default
redistributed_routes:
- proto: Connected
include_leaked: True
route_map: RM-CONN-2-BGP
- proto: Static
include_leaked: True
route_map: RM-CONN-2-BGP
- afi_safi: IPv4 Unicast
vrf: test
redistributed_routes:
- proto: Dynamic
route_map: RM-CONN-2-BGP
- proto: Static
include_leaked: True
route_map: RM-CONN-2-BGP
- afi_safi: IPv4Unicast
vrf: mgmt
redistributed_routes:
- proto: Dynamic
route_map: RM-CONN-2-BGP
- proto: Static
include_leaked: True
route_map: RM-CONN-2-BGP
Key Reasons for the Proposed Change
- It aligns with the structure already used in other BGP tests, ensuring consistency.
- It eliminates unnecessary looping and conditional logic.
- It reduces code complexity, making the test case easier to maintain.
As discussed with @gmuloc we will revisit this once Carl returns and then finalize the approach, In the meantime, I’m putting this on hold.
Thanks,
Geetanjali
There was a problem hiding this comment.
as discussed we will go with @carl-baillargeon suggested model
anta/tests/routing/bgp.py
Outdated
| if address_family.afi not in ["ipv4", "ipv6"] or address_family.safi not in ["unicast", "multicast"]: | ||
| msg = f"{address_family}; redistributed route protocol is not supported for address family `{address_family.eos_key}`" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
This check could go in the AfiSafiConfig model directly.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| Examples | ||
| -------- | ||
| - User |
There was a problem hiding this comment.
I think the example is incomplete here.
| ], | ||
| AfterValidator(update_bgp_redistributed_proto_user), | ||
| ] | ||
| RedisrbutedAfiSafi = Annotated[str, BeforeValidator(bgp_redistributed_route_proto_abbreviations)] |
There was a problem hiding this comment.
| RedisrbutedAfiSafi = Annotated[str, BeforeValidator(bgp_redistributed_route_proto_abbreviations)] | |
| RedistributedAfiSafi = Annotated[str, BeforeValidator(bgp_redistributed_route_proto_abbreviations)] |
There was a problem hiding this comment.
We should use a Literal here instead to make sure Pydantic catches if it's not v4u, v4m, etc. You can refer to #1021.
The idea is to modify the user input (if we can) before the validation.
| Can be enabled in the `VerifyBGPPeerCount` tests.""" | ||
|
|
||
| route_map: str | None = None | ||
| """Specify redistributed route protocol route map. Required field in the `VerifyBGPRedistribution` test.""" |
There was a problem hiding this comment.
I don't think that's needed right?
| @@ -12,7 +12,7 @@ | |||
| from pydantic import BaseModel, ConfigDict, Field, PositiveInt, model_validator | |||
| from pydantic_extra_types.mac_address import MacAddress | |||
|
|
|||
| from anta.custom_types import Afi, BgpDropStats, BgpUpdateError, MultiProtocolCaps, Safi, Vni | |||
| from anta.custom_types import Afi, BgpDropStats, BgpUpdateError, MultiProtocolCaps, RedisrbutedAfiSafi, RedistributedProtocol, Safi, Vni | |||
There was a problem hiding this comment.
| from anta.custom_types import Afi, BgpDropStats, BgpUpdateError, MultiProtocolCaps, RedisrbutedAfiSafi, RedistributedProtocol, Safi, Vni | |
| from anta.custom_types import Afi, BgpDropStats, BgpUpdateError, MultiProtocolCaps, RedistributedAfiSafi, RedistributedProtocol, Safi, Vni |
|
|
||
|
|
||
| class BgpVrf(BaseModel): | ||
| """Model representing BGP vrfs.""" |
There was a problem hiding this comment.
| """Model representing BGP vrfs.""" | |
| """Model representing a VRF in a BGP instance.""" |
| class VerifyBGPRedistribution(AntaTest): | ||
| """Verifies BGP redistributed routes protocol and route-map. | ||
|
|
||
| This test performs the following checks for each specified route: |
There was a problem hiding this comment.
| This test performs the following checks for each specified route: | |
| This test performs the following checks for each specified VRF in the BGP instance: |
| """Input model for the VerifyBGPRedistribution test.""" | ||
|
|
||
| vrfs: list[BgpVrf] | ||
| """List of address families.""" |
There was a problem hiding this comment.
| """List of address families.""" | |
| """List of VRFs in the BGP instance.""" |
| if not (afi_safi_configs := get_value(command_output, f"vrfs.{vrf_data.vrf}.afiSafiConfig.{address_family.afi_safi}")): | ||
| self.result.is_failure(f"{vrf_data}, {address_family} - Not configured") | ||
| continue |
There was a problem hiding this comment.
We should have 2 checks, 1 for the VRF (VRF not configured - like the other tests) and the other for the AFI/SAFI.
There was a problem hiding this comment.
Don't forget to update the docstring to reflect this :)
| -------- | ||
| - AFI-SAFI: v4u | ||
| """ | ||
| return f"AFI-SAFI: {self.afi_safi}" |
There was a problem hiding this comment.
I think we should create a mapping to have cleaner failure messages to have something like AFI-SAFI: IPv4 Unicast instead of v4u. Thoughts?
|
|
||
| def _validate_redistribute_route_details(self, vrf_data: str, address_family: dict[str, Any], afi_safi_configs: list[dict[str, Any]]) -> None: | ||
| """Validate the redstributed route details for a given address family.""" | ||
| for route_info in address_family.redistributed_routes: |
There was a problem hiding this comment.
Can you move this loop out of this helper method? I think it would be cleaner to have this method just check if a redistributed route config is valid or not. If it's not valid, you could maybe return the issues and call self.result.is_failure in the test method.
Goal is to separate validation logic from test result handling. Right now the helper method does validation AND sets failure status so we are mixing concerns.
Description
Added testcase to verify the BGP Redistributed Routes
Fixes #1009
Note - Added pylint disable for number of line check (C0302) with TODO.
Checklist:
pre-commit run)tox -e testenv)