BDP: properly update BGP route attributes based on confederation config #5008
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @corinaminer and @progwriter)
a discussion (no related file):
working to include next hop ip here as well.
Codecov Report
@@ Coverage Diff @@
## master #5008 +/- ##
============================================
- Coverage 76.64% 76.63% -0.01%
+ Complexity 30032 30030 -2
============================================
Files 2399 2399
Lines 115712 115734 +22
Branches 13664 13667 +3
============================================
+ Hits 88683 88690 +7
- Misses 20318 20329 +11
- Partials 6711 6715 +4
|
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/protocols/BgpProtocolHelper.java, line 164 at r1 (raw file):
} // Outgoing metric (MED) is preserved only if advertising to IBGP peer or within a confederation
Comment would be more useful as javadoc to advertiseUnchangedMed(). Same idea for local preference comment below.
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 172 at r1 (raw file):
// Append own as across border resetDefaultRouteBuilders();
why don't you just make all of these separate tests instead of calling this every time? (or possibly putting each test in its own scope would keep the builder clean for you? not sure)
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 200 at r1 (raw file):
transformBgpRoutePostExport( _baseBgpRouteBuilder, false, ConfedSessionType.WITHIN_CONFED, 6, DEST_IP, Ip.ZERO); assertThat(_baseBgpRouteBuilder.getAsPath(), equalTo(AsPath.empty()));
I think _baseBgpRouteBuilder should have an existing AsPath with both a confed and a non-confed AsSet for all of these tests so you can ensure that it is modified (or not) as expected
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpSessionProperties.java, line 264 at r1 (raw file):
} public boolean advertiseUnchanedLocalPref() {
typo Unchaned
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpSessionProperties.java, line 345 at r1 (raw file):
} /** For test use only. */
maybe add a warning "not for use with peers in confederations"
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @corinaminer and @progwriter)
a discussion (no related file):
Previously, progwriter (Victor Heorhiadi) wrote…
working to include next hop ip here as well.
done
projects/batfish/src/main/java/org/batfish/dataplane/protocols/BgpProtocolHelper.java, line 164 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Comment would be more useful as javadoc to
advertiseUnchangedMed(). Same idea for local preference comment below.
done
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 172 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
why don't you just make all of these separate tests instead of calling this every time? (or possibly putting each test in its own scope would keep the builder clean for you? not sure)
too many names
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpSessionProperties.java, line 264 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
typo
Unchaned
unchained
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @progwriter)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpSessionProperties.java, line 264 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
unchained
⛓️ ⚒️
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @corinaminer and @progwriter)
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 200 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
I think
_baseBgpRouteBuildershould have an existing AsPath with both a confed and a non-confed AsSet for all of these tests so you can ensure that it is modified (or not) as expected
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @progwriter)
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 193 at r3 (raw file):
AsPath.of(ImmutableList.<AsSet>builder().add(AsSet.of(2)).add(AsSet.of(777)).build()))); // Clear confederations, prepend own AS
This test now redundant
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
projects/batfish/src/test/java/org/batfish/dataplane/protocols/BgpProtocolHelperTest.java, line 193 at r3 (raw file):
Previously, corinaminer (Corina Miner) wrote…
This test now redundant
done
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpSessionProperties.java, line 345 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
maybe add a warning "not for use with peers in confederations"
done
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:
No description provided.