Cisco IPSec tunnels

[ratulm]

First step of #667

From the config file below:

crypto keyring keyring-vpn-ba2e34a8-0  
  local-address 172.16.1.2
  pre-shared-key address 52.27.166.152 key QMP5tKPP5qk95Y1Eee9c_82.MofNUJQV
!
crypto isakmp policy 200
 encr aes
 authentication pre-share
 group 2
 lifetime 28800
!
crypto isakmp profile isakmp-vpn-ba2e34a8-0
   keyring keyring-vpn-ba2e34a8-0
   match identity address 52.27.166.152 255.255.255.255 
   local-address 172.16.1.2
!
crypto ipsec transform-set ipsec-prop-vpn-ba2e34a8-0 esp-aes esp-sha-hmac 
 mode tunnel
!
!
crypto ipsec profile ipsec-vpn-ba2e34a8-0
 set transform-set ipsec-prop-vpn-ba2e34a8-0 
 set pfs group2
!
interface Tunnel1
 ip address 169.254.15.194 255.255.255.252
 ip tcp adjust-mss 1379
 tunnel source 172.16.1.2
 tunnel mode ipsec ipv4
 tunnel destination 52.27.166.152
 tunnel protection ipsec profile ipsec-vpn-ba2e34a8-0
!

1. We extract into CiscoConfiguration the following concepts: Keyring, IsakmpPolicy, IsakmpProfile, IpsecTransformSet, IpsecProfile, and Tunnel information for interfaces.

2. We then map these concepts to those in Configuration:
   -IkeProposals are derived from IsakmpPolicy

• IkePolicy is derived from IsakmpProfile and Keyring
• IkeGateway is derived from IsakmpProfile and Tunnel using address-based matching
• IpsecProposal is derived from IpsecTransformSet
• IpsecPolicy is derived from IpsecProfile
• IpsecVpn is derived from Tunnel using address-based matching

3. The final output looks something like:

          "ikeGateways" : {
            "isakmp-vpn-ba2e34a8-0" : {
              "name" : "isakmp-vpn-ba2e34a8-0",
              "address" : "52.27.166.152",
              "externalInterface" : "Tunnel1",
              "ikePolicy" : "isakmp-vpn-ba2e34a8-0",
              "localAddress" : "172.16.1.2"
            }
          },
          "ikePolicies" : {
            "isakmp-vpn-ba2e34a8-0" : {
              "name" : "isakmp-vpn-ba2e34a8-0",
              "preSharedKeyHash" : "QMP5tKPP5qk95Y1Eee9c_82.MofNUJQV",
              "proposals" : [
                "200"
              ]
            }
          },
          "ikeProposals" : {
            "200" : {
              "name" : "200",
              "authenticationMethod" : "PRE_SHARED_KEYS",
              "diffieHellmanGroup" : "GROUP2",
              "encryptionAlgorithm" : "AES_128_CBC",
              "lifetimeSeconds" : 28800
            },
          "ipsecPolicies" : {
            "ipsec-vpn-ba2e34a8-0" : {
              "name" : "ipsec-vpn-ba2e34a8-0",
              "pfsKeyGroup" : "GROUP2",
              "pfsKeyGroupDynamicIke" : false,
              "proposals" : [
                "ipsec-prop-vpn-ba2e34a8-0"
              ]
            }
          },
          "ipsecProposals" : {
            "ipsec-prop-vpn-ba2e34a8-0" : {
              "name" : "ipsec-prop-vpn-ba2e34a8-0",
              "authenticationAlgorithm" : "HMAC_SHA1_96",
              "encryptionAlgorithm" : "AES_128_CBC"
            }
          },
          "ipsecVpns" : {
            "Tunnel1" : {
              "name" : "Tunnel1",
              "bindInterface" : "Tunnel1",
              "ikeGateway" : "isakmp-vpn-ba2e34a8-0",
              "ipsecPolicy" : "ipsec-vpn-ba2e34a8-0"
            }
          }

---

This change is 

[arifogel]

Quite a few changes here. They mostly look good. There are some style issues to address and some questions I'd like answered.

---

Reviewed 21 of 21 files at r1.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

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

   TRANSFORM_SET variable ipsec_encryption ipsec_authentication NEWLINE
   (
      cipt_line

Please inline the definition of cipt_line and remove it.

---

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

:
   (
      IKEV2_PROFILE

We don't need to do anything with this? Is it a lot of effort to support ikev2?

---

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

   POLICY name = variable NEWLINE
   (
      cispol_line

Please inline the definition of cispol_line and remove it.

---

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

   PROFILE name = variable NEWLINE
   (
      cisprf_line

Please inline the definition of cisprf_line and remove it.

---

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

   KEYRING name = variable_permissive NEWLINE
   (
      ckr_line

Please inline the definition of ckr_line and remove it.

---

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

  @Override
  public void enterCipprf_set_pfs(Cipprf_set_pfsContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCipprf_set_transform_set(Cipprf_set_transform_setContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCipt_mode(Cipt_modeContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_authentication(Cispol_authenticationContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_encr(Cispol_encrContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_encryption(Cispol_encryptionContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_group(Cispol_groupContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_hash(Cispol_hashContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCispol_lifetime(Cispol_lifetimeContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCisprf_keyring(Cisprf_keyringContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCisprf_match(Cisprf_matchContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCisprf_local_address(Cisprf_local_addressContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCkr_local_address(Ckr_local_addressContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

  @Override
  public void enterCkr_psk(Ckr_pskContext ctx) {

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

---

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

      return IpsecAuthenticationAlgorithm.HMAC_SHA_256_128;
    } else {
      throw new BatfishException("Unknown ipsec authentication algo " + ctx.getText());

Use 'throw convError(...)'

---

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

      return DiffieHellmanGroup.GROUP2;
    } else {
      throw new BatfishException("invalid dh-group");

Use 'throw convError(...)'

---

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

      return EncryptionAlgorithm.THREEDES_CBC;
    } else {
      throw new BatfishException("Unsupported encryption algorithm " + ctx.getText());

Use 'throw convError(...)'

---

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

      return EncryptionAlgorithm.AES_256_CBC;
    } else {
      throw new BatfishException("Unsupported encryption algorithm " + ctx.getText());

Use 'throw convError(...)'

---

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

      return EncryptionAlgorithm.THREEDES_CBC;
    } else {
      throw new BatfishException("Unknown encryption algorithm " + ctx.getText());

Use 'throw convError(...)'

---

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

            profile.getDefinitionLine());
      } else {
        policy.getProposals().put(transformSetName, c.getIpsecProposals().get(transformSetName));

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

---

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

              tunnel.getIpsecProfileNameLine());
        } else {
          ipsecVpn.setIpsecPolicy(c.getIpsecPolicies().get(profileName));

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

---

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

            isakmpProfile.getDefinitionLine());
      } else {
        Keyring keyring = _keyrings.get(keyringName);

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

---

projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoStructureUsage.java, line 48 at r1 (raw file):

  IP_NAT_SOURCE_ACCESS_LIST("ip nat source dynamic access-list"),
  IP_NAT_SOURCE_POOL("ip nat source pool"),
  IPSEC_PROFILE("ipsec profile"),

Should be more descriptive. What's important here is not just the type of the structure being referenced, but the nature of the reference itself, i.e. where in the configuration the reference occurs. The same goes for the below additions.

---

tests/basic/init-example-aws.ref, line 18 at r1 (raw file):

        "lhr-border-02" : {
          "extended ip access-list" : {
            "2001" : [

What is the purpose of this unused list?

---

Comments from Reviewable

[arifogel]

FYI r2 is just a rebase

---

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

Comments from Reviewable

[arifogel]

Done from my end. @dhalperi please take a look.

---

Reviewed 40 of 40 files at r3.
Review status: all files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[arifogel]

Had to resolve conflict in CiscoGrammarTest so I made another revision.

---

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

Comments from Reviewable

[dhalperi]

Reviewed 4 of 21 files at r1, 3 of 8 files at r2, 38 of 40 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 38 unresolved discussions.

---

projects/batfish-common-protocol/src/main/java/org/batfish/common/util/DefinedStructure.java, line 3 at r4 (raw file):

package org.batfish.common.util;

public interface DefinedStructure {

What's the relationship between DefinedStructure and ComparableStructure and ReferenceCountedStructure? Why isn't this an abstract class that extends ComparableStructure, given that all users inherit both?

Why is this refactoring of reference tracking in this PR at all? This is easily separable from the existing VPN work.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IpsecVpn.java, line 101 at r4 (raw file):

    String psk = ikePolicy.getPreSharedKeyHash();
    String remotePsk = remoteIkePolicy.getPreSharedKeyHash();
    return psk != null && remotePsk != null && psk.equals(remotePsk);

can just use Objects.equals here?

I guess that the PSK itself could be null, in which case just drop the remotePsk != null bit? (it's redundant).

---

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

Previously, arifogel (Ari Fogel) wrote…

Please inline the definition of cipt_line and remove it.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

Please inline the definition of cispol_line and remove it.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

Please inline the definition of cisprf_line and remove it.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

Please inline the definition of ckr_line and remove it.

still needed?

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_interface.g4, line 1017 at r4 (raw file):

iftunnel_protection
:
   PROTECTION IPSEC PROFILE variable NEWLINE

do you not normally do some foo = variable here? Do you not need the profile name?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

For style consistency, change to exit rule. Enter rules are used only when context is needed by child nodes.

still needed?

I'm going to stop commenting ont hese, they're all unresolved comments authored by you anyway.

---

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

Previously, arifogel (Ari Fogel) wrote…

Use 'throw convError(...)'

still needed?

---

projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 2117 at r4 (raw file):

          Ip externalAddress = externalPrefix.getAddress();
          Set<IpsecVpn> vpnsUsingExternalAddress =
              externalAddresses.computeIfAbsent(externalAddress, k -> Sets.newIdentityHashSet());

I think there's a shorter-hand for this. Sets::newIdentityHashSet ?

---

projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 2117 at r4 (raw file):

          Ip externalAddress = externalPrefix.getAddress();
          Set<IpsecVpn> vpnsUsingExternalAddress =
              externalAddresses.computeIfAbsent(externalAddress, k -> Sets.newIdentityHashSet());

Also wondering about the motivation for the type change?

---

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

Previously, arifogel (Ari Fogel) wrote…

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

still needed?

---

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

Previously, arifogel (Ari Fogel) wrote…

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

still needed?

---

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

  }

  private Interface getInterfaceByTunnelAddresses(Ip sourceAddress, Prefix destPrefix) {

@Nullable return value

---

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

              boolean exists = maps.stream().anyMatch(map -> map != null && map.containsKey(name));
              if (exists) {
                String msg = usage.getDescription();

seems like you could simplify this code a fair bit by converting exists into a filter. Then `if (matches.isEmpty()) {
mark undefined
} else {
add all matches as referrers.
}

---

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

                String msg = usage.getDescription();
                for (Map<String, ? extends ReferenceCountedStructure> map : maps) {
                  if (map.containsKey(name)) {

this code down here does not handle map == null, which you assume is possible above.

---

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

  private void warnUnusedRouteMaps() {
    warnUnusedStructure(_routeMaps, CiscoStructureType.ROUTE_MAP);

inline all these?

in fact, it might be much cleaner to "mark/warn ; mark/warn; mark/warn;" instead of "mark all;warn all;" like the function is right now.

---

Comments from Reviewable

[dhalperi]

high level: this PR does not move the needle in the right direction w.r.t. java unit tests or documentation.

---

Review status: all files reviewed at latest revision, 38 unresolved discussions.

---

Comments from Reviewable

[arifogel]

Review status: 18 of 48 files reviewed at latest revision, 35 unresolved discussions.

---

projects/batfish-common-protocol/src/main/java/org/batfish/common/util/DefinedStructure.java, line 3 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

What's the relationship between DefinedStructure and ComparableStructure and ReferenceCountedStructure? Why isn't this an abstract class that extends ComparableStructure, given that all users inherit both?

Why is this refactoring of reference tracking in this PR at all? This is easily separable from the existing VPN work.

Hmm initially I thought there was a reason to separate out this interface. But if all inherit both, I will just make it an abstract class as you said.

Refactoring of reference tracking could arguably be separated. The purpose of this PR is to add Cisco structures and logic for VPNs. Part of that is reference tracking for the new structures. In order to do that cleanly, I preferred to do the refactoring now rather than implement it verbosely and defer a clean implementation to a refactoring PR.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IpsecVpn.java, line 101 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

can just use Objects.equals here?

I guess that the PSK itself could be null, in which case just drop the remotePsk != null bit? (it's redundant).

Right good call.

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cisco/Cisco_interface.g4, line 1017 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

do you not normally do some foo = variable here? Do you not need the profile name?

Just a style thing in this case. More generally, it protects against future changing of generated variable() method to returning List<VariableContext> (which is never null) instead of VariableContext. This happens when the variable rule is referenced more than once within the definition of the containing rule (in this case iftunnel_protection, or is referenced inside of ()+ or ()* body.

But yeah, I'll just change it to match.

---

projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 2117 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

I think there's a shorter-hand for this. Sets::newIdentityHashSet ?

No, because it does not use the k arg.

---

projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 2117 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Also wondering about the motivation for the type change?

IpsecVpns with the same name would have pushed each other out of this set. This is beacuse they inherit from ComparableStructure<T>, which overrides equals and hashCode in a way unsuitable for this operation. Frankly, I'm not sure there is any value to these overrides in almost all cases we use this ComparableStructure<T>. We should revisit this after this PR.

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

No. This location turned out to be unsuitable. Instead I added unconditional marking in toVendorIndpendentConfiguration().

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

No. This location turned out to be unsuitable. Instead I added unconditional marking in toVendorIndpendentConfiguration().

---

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

Previously, arifogel (Ari Fogel) wrote…

You need to mark usage here if the structure is not undefined. That is, unless you already marked it somewhere else.

This location turned out to be unsuitable. Instead I added unconditional marking in toVendorIndpendentConfiguration().

---

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

Previously, dhalperi (Dan Halperin) wrote…

inline all these?

in fact, it might be much cleaner to "mark/warn ; mark/warn; mark/warn;" instead of "mark all;warn all;" like the function is right now.

I inlined the warns with a single map. The marks look like crap when inlined, so I skipped for now. I don't feel comfortable re-ordering right now because not all marking happens in this function -- the result would be confusing.

---

projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoStructureUsage.java, line 48 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Should be more descriptive. What's important here is not just the type of the structure being referenced, but the nature of the reference itself, i.e. where in the configuration the reference occurs. The same goes for the below additions.

I did this.

---

Comments from Reviewable

[arifogel]

Fixed a bunch of unaddressed stuff. I believe it is ready. Please re-review.

---

Review status: 18 of 48 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

I'm going to stop commenting ont hese, they're all unresolved comments authored by you anyway.

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

still needed?

done

---

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

Previously, arifogel (Ari Fogel) wrote…

Use 'throw convError(...)'

done

---

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

Previously, arifogel (Ari Fogel) wrote…

Use 'throw convError(...)'

done

---

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

Previously, arifogel (Ari Fogel) wrote…

Use 'throw convError(...)'

done

---

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

Previously, arifogel (Ari Fogel) wrote…

Use 'throw convError(...)'

done

---

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

Previously, dhalperi (Dan Halperin) wrote…

seems like you could simplify this code a fair bit by converting exists into a filter. Then `if (matches.isEmpty()) {
mark undefined
} else {
add all matches as referrers.
}

</blockquote></details>

I cleaned it up a bit.

---

*[projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java, line 1101 at r4](https://reviewable.io:443/reviews/batfish/batfish/684#-L-5dRZDEASJD3wu1mKJ:-L-8TOl816OAVz2fiwxu:bt2vr1b) ([raw file](https://github.com/batfish/batfish/blob/0c9aeeffc48d9429e57cd4ed4631f34dcbafe10c/projects/batfish/src/main/java/org/batfish/representation/cisco/CiscoConfiguration.java#L1101)):*
<details><summary><i>Previously, dhalperi (Dan Halperin) wrote…</i></summary><blockquote>

this code down here does not handle `map == null`, which you assume is possible above.
</blockquote></details>

Fixed.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/batfish/batfish/684#-:-L-8TQwjq9hokVbWAbjz:bss9ox4)*
<!-- Sent from Reviewable.io -->

[dhalperi]

You need to resolve the comments that you fixed before it can be merged.

---

Reviewed 30 of 30 files at r5.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[dhalperi]

---

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

---

Comments from Reviewable

[arifogel]

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

tests/basic/init-example-aws.ref, line 18 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

What is the purpose of this unused list?

Ignoring for now.

---

Comments from Reviewable