PAN: loopback routes are local #6677
Conversation
|
This change is |
Requires changes to ConnectedRouteMetadata as well.
Codecov Report
@@ Coverage Diff @@
## master #6677 +/- ##
============================================
- Coverage 73.54% 73.54% -0.01%
- Complexity 36686 36697 +11
============================================
Files 2934 2934
Lines 147569 147633 +64
Branches 17788 17797 +9
============================================
+ Hits 108531 108573 +42
- Misses 30502 30514 +12
- Partials 8536 8546 +10 |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi and @sfraint)
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2003 at r1 (raw file):
if (iface.getType() == Interface.Type.LOOPBACK) { // On PAN, loopback addresses are not connected routes, they are only local routes.
Are there no /32s on non-loopback interfaces? or are /32s on loopback interfaces the only ones that count as "loopback addresses"?
more fundamentally: do we need to make customized ConnectedRouteMetadata for /32s on non-loopback interfaces?
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2005 at r1 (raw file):
We have already filtered down to valid addresses, all of which are /32
We didn't actually confirm they're all /32. Should the allValidAddresses stream above have another filter to limit loopback interface addresses to /32s? or is there something earlier in the pipeline that would block any other addresses from being attributed to a loopback iface?
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2010 at r1 (raw file):
.collect( ImmutableMap.toImmutableMap( a -> a,
nit: could replace with Function.identity()
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/ConnectedRouteMetadataTest.java, line 25 at r1 (raw file):
.addEqualityGroup(builder.setTag(1).build()) .addEqualityGroup(builder.setGenerateLocalRoutes(true).build()) .addEqualityGroup(builder.setAdmin(1).setGenerateLocalRoutes(true))
👀
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @corinaminer, @dhalperi, and @sfraint)
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2005 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
We have already filtered down to valid addresses, all of which are /32
We didn't actually confirm they're all /32. Should the
allValidAddressesstream above have another filter to limit loopback interface addresses to /32s? or is there something earlier in the pipeline that would block any other addresses from being attributed to a loopback iface?
I don't think you understood the code? Can you take another look and suggest clarifications?
The filtering only applies to loopback interfaces, and only lets /32 through in that case only.
We did confirm they're all /32, and we only want to handle loopback interfaces, and we don't need connected route metadata for other interfaces.
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2010 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
nit: could replace with
Function.identity()
Following the Java style recommendations here: this is shorter than function identity
|
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/ConnectedRouteMetadataTest.java, line 25 at r1 (raw file): Previously, corinaminer (Corina Miner) wrote…
Not sure what this means. But the old tests were confusing and complicated; since the builder keeps state we just need to change each property once and we've covered all the bases: for each property we have a pair that only differ in that. Additionally I went ahead and made sure that null and false are different, mainly because of paranoia |
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @sfraint)
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2003 at r1 (raw file):
on your other comment you said:
we don't need connected route metadata for other interfaces
so i assume this question has been considered. Still not personally clear on whether other interfaces have /32s, but nonblocking
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2005 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
I don't think you understood the code? Can you take another look and suggest clarifications?
The filtering only applies to loopback interfaces, and only lets /32 through in that case only.
We did confirm they're all /32, and we only want to handle loopback interfaces, and we don't need connected route metadata for other interfaces.
Oh, yeah, never mind. Misread the filtering.
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/ConnectedRouteMetadataTest.java, line 25 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Not sure what this means. But the old tests were confusing and complicated; since the builder keeps state we just need to change each property once and we've covered all the bases: for each property we have a pair that only differ in that.
Additionally I went ahead and made sure that null and false are different, mainly because of paranoia
yeah i was just confused by the old version of the test
|
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 2005 at r1 (raw file): Previously, corinaminer (Corina Miner) wrote…
Thanks. If this code can be cleaner I'm happy to do it! This is complicated. Appreciate the detailed review. |
Requires changes to ConnectedRouteMetadata as well.