Converting RSA pub named key to to VI objects #3972

haverma · 2019-05-29T19:53:58Z

to be reviewed after #3971

PR contents:

addition of VS object for RSA pub named key
Conversion of the above object to IkePhase1Key and IkePhase1Policy

batfish-bot · 2019-05-29T19:54:11Z

This change is

codecov · 2019-05-29T23:28:50Z

Codecov Report

Merging #3972 into master will increase coverage by 0.02%.
The diff coverage is 87.3%.

@@             Coverage Diff              @@
##             master    #3972      +/-   ##
============================================
+ Coverage     74.95%   74.97%   +0.02%     
- Complexity    24272    24306      +34     
============================================
  Files          2031     2033       +2     
  Lines         97295    97409     +114     
  Branches      11591    11598       +7     
============================================
+ Hits          72927    73035     +108     
  Misses        19087    19087              
- Partials       5281     5287       +6

Impacted Files	Coverage Δ	Complexity Δ
...in/java/org/batfish/datamodel/IkePhase1Policy.java	`94.73% <ø> (ø)`	`11 <0> (ø)`	⬇️
...tfish/representation/cisco/CiscoStructureType.java	`100% <100%> (ø)`	`3 <0> (ø)`	⬇️
...tfish/representation/cisco/CiscoConfiguration.java	`83.88% <100%> (-0.05%)`	`532 <2> (+1)`
...fish/representation/cisco/CiscoStructureUsage.java	`100% <100%> (ø)`	`3 <0> (ø)`	⬇️
...fish/grammar/cisco/CiscoControlPlaneExtractor.java	`66.69% <77.77%> (+0.03%)`	`1441 <4> (+4)`	⬆️
...g/batfish/representation/cisco/NamedRsaPubKey.java	`83.33% <83.33%> (ø)`	`6 <6> (?)`
...batfish/representation/cisco/CiscoConversions.java	`90.58% <89.47%> (-0.03%)`	`154 <6> (+6)`
...ava/org/batfish/datamodel/bgp/Layer3VniConfig.java	`84.21% <0%> (-3.29%)`	`24% <0%> (+7%)`
...ava/org/batfish/datamodel/bgp/Layer2VniConfig.java	`85.71% <0%> (-2.86%)`	`15% <0%> (-1%)`
...ain/java/org/batfish/symbolic/IngressLocation.java	`65.78% <0%> (-2.64%)`	`15% <0%> (-1%)`
... and 15 more

haverma

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @progwriter)

a discussion (no related file):
Can be reviewed

progwriter

Reviewed 6 of 12 files at r1, 6 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @haverma)

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_crypto.g4, line 526 at r2 (raw file):

;

ckpn_null

is this rule still necessary? by which I mean, with your change did we full implement the parse subtree under named key? If yes, no need to keep it around. If not, the presence of ADDRESS worries me (antlr will choose longest matching rule, IIRC)

projects/batfish/src/main/java/org/batfish/grammar/cisco/CiscoControlPlaneExtractor.java, line 1960 at r2 (raw file):

  @Override
  public void enterCkp_named_key(Ckp_named_keyContext ctx) {
    _currentCryptoNamedRsaPubKey = new CryptoNamedRsaPubKey(ctx.name.getText());

use computeifAbsent - that's our standard pattern. then you only need to clear _currentX back to null on exit.

projects/batfish/src/main/java/org/batfish/grammar/cisco/CiscoControlPlaneExtractor.java, line 1978 at r2 (raw file):

    _configuration
        .getCryptoNamedRsaPubKeys()
        .put(_currentCryptoNamedRsaPubKey.getName(), _currentCryptoNamedRsaPubKey);

reset _currentCryptoNamedRsaPubKey back to null

projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 3493 at r2 (raw file):

              IkePhase1Key ikePhase1Key = toIkePhase1Key(cryptoNamedRsaPubKey);
              ikePhase1KeysBuilder.put(
                  String.format("~%s_%s~", PREFIX_RSA_PUB, cryptoNamedRsaPubKey.getName()),

Can you factor this out into a static function and use both here and in cisco conversions? This will ensure that key name and map key match.

projects/batfish/src/main/java/org/batfish/representation/cisco/CryptoNamedRsaPubKey.java, line 8 at r2 (raw file):

import org.batfish.datamodel.Ip;

public class CryptoNamedRsaPubKey implements Serializable {

worth javadocing.
also, is the crypto prefix necessary here? I'd just call it NamedRsaPubKey

haverma

addressed comments, and also added calls to define and self reference the new NamedRsaPubKey Cisco structure

Reviewable status: 7 of 14 files reviewed, all discussions resolved (waiting on @progwriter)

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_crypto.g4, line 526 at r2 (raw file):