Allow iptables rules from hosts to be merged into routers #338
Conversation
dspicuzzbbn
commented
Aug 17, 2017
dspicuzzbbn
commented
- Writes host vendor configs to a separate directory
- Converts iptables ACLs to router ACLs
- Added tests
- Fixes feature: support for iptables on routers #221
- Writes host vendor configs to a separate directory - Converts iptables ACLs to router ACLs - Added tests - Fixes batfish#221
| @@ -12,6 +14,10 @@ | |||
|
|
|||
| private String _name; | |||
|
|
|||
| private String _inInterfaceName; | |||
There was a problem hiding this comment.
This should not go in IpAccessListLine. This heavily clashes with existing semantics. To filter by input interface, you simply apply the appropriate acl to the appropriate interface in the incoming direction. I understand you are trying to add support for iptables, but this particular implementation will be too problematic. IpAccessListLine is only supposed to filter by header bits (and classifiers).
| byte[] data = fromGzipFile(inputPath); | ||
| logger.debug(" ...OK\n"); | ||
| dataByName.put(name, data); | ||
| if (!Files.isDirectory(inputPath)) { |
There was a problem hiding this comment.
We have an open issue to scan directories recursively for input files. This filtering would then not be necessary, as the inputPath args supplied would be guaranteed not to be directories.
| @@ -1705,8 +1707,15 @@ public TestrigSettings getBaseTestrigSettings() { | |||
| Path serializedVendorConfigPath, ConvertConfigurationAnswerElement answerElement) { | |||
| Map<String, GenericConfigObject> vendorConfigurations = | |||
| deserializeVendorConfigurations(serializedVendorConfigPath); | |||
| Map<String, GenericConfigObject> hostVendorConfigurations = | |||
There was a problem hiding this comment.
This will interfere with resolution of the issue mentioned earlier to scan input directories recursively. Instead, we should have a new top-of-testrig-level folder
| @@ -3169,6 +3178,97 @@ private void populateFlowHistory(FlowHistory flowHistory, String environmentName | |||
| } | |||
| } | |||
|
|
|||
| // this is a hack to deal with router names that are named ethX | |||
There was a problem hiding this comment.
Did you mean interface names instead of router names? If so, you should use normalized name of the interface in the host config to be merged. Alternatively, use the appropriate normalization function for the vendor of the base configuration to be merged into. For instance, see org.batfish.grammar.cisco.CiscoControlPlaneExtractor.getCanonicalInterfaceName
There was a problem hiding this comment.
Yes, that's what I meant.
The problem is that the normalization isn't applied uniformly -- it's applied to the cisco interface names, but not to host interface names. This has caused issues elsewhere (I forget which issue...)
My opinion is that the interface name normalization is a bad idea. Not only does it make it difficult to do matching, but it makes debugging more difficult also (like 'hm... why is my router interface Ethernet0 instead of eth0')
|
|
||
| String dbgName = c.getHostname() + ":" + i.getName(); | ||
|
|
||
| List<IpAccessListLine> newRules = prerouting.getLines().stream() |
There was a problem hiding this comment.
Since you should not add input/output interface to IpAccessListLine, you may be able to accomplish your intended purpose here by creating a host-config-specific IpAccessListLine type class like we have for the other vendors.
| @@ -4003,12 +4103,13 @@ private void serializeHostConfigs( | |||
| // now, serialize | |||
| _logger.info("\n*** SERIALIZING VENDOR CONFIGURATION STRUCTURES ***\n"); | |||
| resetTimer(); | |||
| CommonUtil.createDirectories(outputPath); | |||
| Path hostsOutputPath = outputPath.resolve(BfConsts.RELPATH_HOST_CONFIGS_DIR); | |||
There was a problem hiding this comment.
See earlier comment about where to put host configs meant to be merged.
| @@ -98,4 +98,12 @@ public IpWildcard toIpWildcard() { | |||
|
|
|||
| return subRanges; | |||
| } | |||
|
|
|||
| public String toInterfaceName() { | |||
There was a problem hiding this comment.
This won't be necessary after earlier comments are addressed.
|
@dspicuzzbbn I made edits to the PR review with more context |
