IOS fix aggregate-address implementation #7075
Conversation
arifogel
commented
Jun 17, 2021
•
arifogel
commented
- update parsing and vendor model for aggregate-address
- use BGP process-level aggregates instead of vrf-level following IOS-XR and VI support for BGP aggregates activated from BGP RIB #7067
- fixes Cisco IOS aggregate address null route #6064
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #7075 +/- ##
============================================
- Coverage 71.97% 71.97% -0.01%
- Complexity 37888 37903 +15
============================================
Files 3043 3043
Lines 155068 155101 +33
Branches 18633 18630 -3
============================================
+ Hits 111612 111627 +15
- Misses 34541 34553 +12
- Partials 8915 8921 +6
|
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 3 of 9 files at r2, 5 of 5 files at r3.
Reviewable status: 13 of 19 files reviewed, 2 unresolved discussions (waiting on @arifogel)
projects/batfish/src/main/java/org/batfish/grammar/cisco/CiscoControlPlaneExtractor.java, line 3718 at r4 (raw file):
// Intentional identity comparison if (_currentPeerGroup == proc.getMasterBgpPeerGroup()) { if (ctx.network != null || ctx.prefix != null) {
nit: might be cleaner to check if _currentIpv4Aggregate != null and then else { assert _currentIpv6Aggregate != null; ... }
projects/batfish/src/main/java/org/batfish/representation/cisco/BgpAggregateIpv4Network.java, line 12 at r4 (raw file):
@ParametersAreNonnullByDefault public class BgpAggregateIpv4Network extends BgpAggregateNetwork {
Shouldn't there be analogous changes in BgpAggregateIpv6Network?
projects/batfish/src/main/java/org/batfish/representation/cisco/BgpAggregateIpv4Network.java, line 56 at r4 (raw file):
@Override public String toString() {
not really sure why you generate these in the first place, but should omitNullValues if you do
There was a problem hiding this comment.
Reviewable status: 11 of 19 files reviewed, 1 unresolved discussion (waiting on @corinaminer)
projects/batfish/src/main/java/org/batfish/grammar/cisco/CiscoControlPlaneExtractor.java, line 3718 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
nit: might be cleaner to check
if _currentIpv4Aggregate != nulland thenelse { assert _currentIpv6Aggregate != null; ... }
right, this code was just moved.
done.
projects/batfish/src/main/java/org/batfish/representation/cisco/BgpAggregateIpv4Network.java, line 12 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Shouldn't there be analogous changes in
BgpAggregateIpv6Network?
Done, but probably a waste of time until we implement IPv6 in 2050.
projects/batfish/src/main/java/org/batfish/representation/cisco/BgpAggregateIpv4Network.java, line 56 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
not really sure why you generate these in the first place, but should
omitNullValuesif you do
Because a test failed and I couldn't read the output, of course.
Done.
There was a problem hiding this comment.
Reviewed 5 of 9 files at r2, 1 of 1 files at r4, 1 of 2 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel)
projects/batfish/src/main/java/org/batfish/representation/cisco/BgpAggregateIpv4Network.java, line 12 at r4 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Done, but probably a waste of time until we implement IPv6 in 2050.
yeahh i hear that, but too freaky to write baseEquals/baseHashcode and then not use them in all child classes
projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 6026 at r4 (raw file):
assertThat( aggs, hasEntry(
nit: you don't need all the equalTos; hasEntry(Object, Object) exists
projects/batfish/src/test/resources/org/batfish/grammar/cisco/testrigs/bgp-agg-learned-contributors/configs/c1, line 9 at r4 (raw file):
! ! Next hop for static routes interface GigabitEthernet0/1
unused
projects/batfish/src/test/resources/org/batfish/grammar/cisco/testrigs/bgp-agg-learned-contributors/configs/c1, line 23 at r4 (raw file):
neighbor 10.10.10.2 activate neighbor 10.10.10.2 route-map ALLOW-ALL in neighbor 10.10.10.2 route-map ALLOW-ALL out
Unlike XR, the IOS default is to allow all when no route-map is configured, so you could drop the route-maps.
There was a problem hiding this comment.
Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @corinaminer)
projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 6026 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
nit: you don't need all the
equalTos;hasEntry(Object, Object)exists
hasEntry(Object, Object) uses reference equality IIRC, unfortunately. So it's not always safe.
projects/batfish/src/test/resources/org/batfish/grammar/cisco/testrigs/bgp-agg-learned-contributors/configs/c1, line 9 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
unused
Removed
projects/batfish/src/test/resources/org/batfish/grammar/cisco/testrigs/bgp-agg-learned-contributors/configs/c1, line 23 at r4 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Unlike XR, the IOS default is to allow all when no route-map is configured, so you could drop the route-maps.
Doesn't it depend on eBGP/iBGP? Regardless, just gonna leave it for now, since it reduces uncertainty for the reader.
There was a problem hiding this comment.
Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @corinaminer)
projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 6026 at r4 (raw file):
Previously, arifogel (Ari Fogel) wrote…
hasEntry(Object, Object)uses reference equality IIRC, unfortunately. So it's not always safe.
Or maybe it was that if one arg is a Matcher, it checks if the other arg is equal to the Matcher arg. Regardless, this is fail-safe, so leaving it.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @arifogel)
There was a problem hiding this comment.
Reviewable status:
projects/batfish/src/test/resources/org/batfish/grammar/cisco/testrigs/bgp-agg-learned-contributors/configs/c1, line 23 at r4 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Doesn't it depend on eBGP/iBGP? Regardless, just gonna leave it for now, since it reduces uncertainty for the reader.
👍 just to follow up: XR depends EBGP/IBGP (denies if no route-map in EBGP, permits if no route-map in IBGP); IOS permits in both cases.
