Parse rfc1583 compatibility #617

corinaminer · 2017-10-26T20:07:25Z

Changes are grouped by commit.

Let me know if I need to add to any other files to make the RFC compatibility info more accessible.

This change is

corinaminer · 2017-10-26T20:08:47Z

projects/batfish/src/main/java/org/batfish/grammar/cisco/CiscoControlPlaneExtractor.java

+  @Override public void exitRo_rfc1583_compatibility(Ro_rfc1583_compatibilityContext ctx) {
+    _configuration.getCf().setRfc1583Compatible(ctx.NO() == null);
+  }
+


My understanding here is that the method is only called if there's a line about RFC 1583 compatibility, and then ctx.NO() is null iff there's no no in that line.

dhalperi · 2017-10-26T20:39:20Z

Reviewed 4 of 8 files at r1.
Review status: 4 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/vendor_family/cisco/CiscoFamily.java, line 120 at r1 (raw file):

  }

  public Boolean getRfc1583Compatible() { return _rfc1583Compatible; }

Annotate both the return value and the local variable as @Nullable.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/vendor_family/cisco/CiscoFamily.java, line 198 at r1 (raw file):

  }

  public void setRfc1583Compatible(Boolean rfc1583Compatible) { _rfc1583Compatible = rfc1583Compatible; }

It looks like you have not followed the Batfish instructions and set up the auto formatter. Please do so, and then reformat all java files you've modified.

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 261 at r1 (raw file):

     * (see https://www.cisco.com/c/en/us/support/docs/ip/open-shortest-path-first-ospf/7039-1.html#t29)
     */
    //TODO: add parsing logic for "(no) compatible rfc1583" command and propagate it to useMin

Do this, and delete the TODO?

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 294 at r1 (raw file):

        // Compute the metric from any possible contributing routes, use older RFC by default
        // as it seems consistent with the GNS3 simulations
        Boolean useMin = _c.getVendorFamily().getCisco().getRfc1583Compatible();

This doesn't seem like the right part of the model to have this flag -- it's a per-OSPF process flag. E.g., here's a recommended config; can you support this?

router ospf 1
  no compatible rfc1583
!
router ospf 2
  compatible rfc1583
!

projects/batfish/src/test/resources/org/batfish/grammar/cisco/testconfigs/rfc1583Compatible, line 6 at r1 (raw file):

router ospf 1
 compatible rfc1583
!

Make this 1 file with 2 OSPF routers and verify that we can set compatibility separately for the two of them.

projects/batfish/src/test/resources/org/batfish/grammar/cisco/testconfigs/rfc1583NoCompatible, line 5 at r1 (raw file):

!
router ospf 1
 no compatible rfc1583

(can delete this file now)

Comments from Reviewable

progwriter · 2017-10-26T21:09:35Z

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java

@@ -291,13 +291,17 @@ boolean computeInterAreaSummaries() {
        Long metric = null;
        // Compute the metric from any possible contributing routes, use older RFC by default
        // as it seems consistent with the GNS3 simulations
+        Boolean useMin = _c.getVendorFamily().getCisco().getRfc1583Compatible();


At this point we should rely only on vendor-independent config. I believe there should be a toVendorIndependentConfiguration() method that will need to be changed to keep track of this var. Then it can be propagated to the Vrf & Ospf process

dhalperi · 2017-10-27T18:57:18Z

Reviewed 10 of 10 files at r2.
Review status: 3 of 11 files reviewed at latest revision, 5 unresolved discussions.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/OspfProcess.java, line 73 at r2 (raw file):

  }

  public @Nullable Boolean getRfc1583Compatible() { return _rfc1583Compatible; }

reformat file

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 294 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

At this point we should rely only on vendor-independent config. I believe there should be a toVendorIndependentConfiguration() method that will need to be changed to keep track of this var. Then it can be propagated to the Vrf & Ospf process

Done

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 294 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

This doesn't seem like the right part of the model to have this flag -- it's a per-OSPF process flag. E.g., here's a recommended config; can you support this?
router ospf 1
  no compatible rfc1583
!
router ospf 2
  compatible rfc1583
!

Ignore - as Batfish doesn't currently support multiple ospf processes, multiple files is better.

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 271 at r2 (raw file):

    // Determine whether to use min based on RFC 1583 compatibility setting
    Boolean useMin = proc.getRfc1583Compatible();

simpler:

boolean useMin = MoreObjects.firstNonNull(proc.getRfc1583Compatible(), true);

and I'd probably add a comment that contains some of the comments from the other note, perhaps along the lines of:

// Between the original RFC1583 and the newer RFC2328, the OSPF summary metric
// was changed from min to max. Routers (Cisco, Juniper at least) default to RFC1583
// unless explicitly disabled.

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

  }

  @Override public void exitRo_rfc1583_compatibility(Ro_rfc1583_compatibilityContext ctx) {

auto-format here, too.

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

  }

  public @Nullable Boolean getRfc1583Compatible() { return _rfc1583Compatible; }

autoformat

projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 207 at r2 (raw file):

        BatfishTestUtils.getBatfishFromTestrigText(
            configurationTextMap,
            Collections.emptySortedMap(),

autoformat, but LGTM

Comments from Reviewable

corinaminer · 2017-10-27T18:57:25Z

Review status: 3 of 11 files reviewed at latest revision, 5 unresolved discussions.

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 294 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

This doesn't seem like the right part of the model to have this flag -- it's a per-OSPF process flag. E.g., here's a recommended config; can you support this?
router ospf 1
  no compatible rfc1583
!
router ospf 2
  compatible rfc1583
!

No, can't support that. Moved the flag down into OspfProcess, though.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/vendor_family/cisco/CiscoFamily.java, line 120 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Annotate both the return value and the local variable as @Nullable.

Should the argument of the setter be marked @Nullable too?

Comments from Reviewable

dhalperi · 2017-10-27T19:35:05Z

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

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/vendor_family/cisco/CiscoFamily.java, line 120 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Should the argument of the setter be marked @Nullable too?

Yes, probably. Good point.

Comments from Reviewable

dhalperi · 2017-10-27T19:35:17Z

after comments are addressed

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

Comments from Reviewable

corinaminer · 2017-10-27T19:38:01Z

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

projects/batfish/src/main/java/org/batfish/bdp/VirtualRouter.java, line 271 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

simpler:

boolean useMin = MoreObjects.firstNonNull(proc.getRfc1583Compatible(), true);

and I'd probably add a comment that contains some of the comments from the other note, perhaps along the lines of:

// Between the original RFC1583 and the newer RFC2328, the OSPF summary metric
// was changed from min to max. Routers (Cisco, Juniper at least) default to RFC1583
// unless explicitly disabled.

Done.

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

Previously, dhalperi (Dan Halperin) wrote…

auto-format here, too.

Done.

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

Previously, dhalperi (Dan Halperin) wrote…

autoformat

Done.

projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 207 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

autoformat, but LGTM

Done.

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/vendor_family/cisco/CiscoFamily.java, line 120 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Yes, probably. Good point.

Done.

Comments from Reviewable

dhalperi · 2017-10-27T19:44:33Z

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

Comments from Reviewable

corinaminer added 3 commits October 26, 2017 12:08

Add parsing for [no] compatible rfc1583

e320bfd

Add testing for [no] compatible rfc1583 parsing

2bf2bd6

Tentative determination of useMin from rfc1583 compatibility

193db09

corinaminer requested review from dhalperi and progwriter October 26, 2017 20:07

corinaminer commented Oct 26, 2017

View reviewed changes

progwriter reviewed Oct 26, 2017

View reviewed changes

corinaminer added 2 commits October 27, 2017 11:31

Represent RFC 1583 compatibility in vendor independent model

f5c15a3

Fixing formatting

50ab43c

Better comments, simpler useMin determination

5e96cb0

Add @nullable to arguments in Rfc1583 set methods

3a93939

corinaminer merged commit 54bdbf3 into batfish:master Oct 27, 2017

corinaminer deleted the parse-rfc-compatibility branch October 27, 2017 21:00

dhalperi mentioned this pull request Oct 27, 2017

Implement configurable compatibility with RFCs #1583 and #2383 #609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse rfc1583 compatibility #617

Parse rfc1583 compatibility #617

corinaminer commented Oct 26, 2017 •

edited by dhalperi

corinaminer Oct 26, 2017

dhalperi commented Oct 26, 2017

progwriter Oct 26, 2017

dhalperi commented Oct 27, 2017

corinaminer commented Oct 27, 2017

dhalperi commented Oct 27, 2017

dhalperi commented Oct 27, 2017

corinaminer commented Oct 27, 2017

dhalperi commented Oct 27, 2017

Parse rfc1583 compatibility #617

Parse rfc1583 compatibility #617

Conversation

corinaminer commented Oct 26, 2017 • edited by dhalperi

corinaminer Oct 26, 2017

Choose a reason for hiding this comment

dhalperi commented Oct 26, 2017

progwriter Oct 26, 2017

Choose a reason for hiding this comment

dhalperi commented Oct 27, 2017

corinaminer commented Oct 27, 2017

dhalperi commented Oct 27, 2017

dhalperi commented Oct 27, 2017

corinaminer commented Oct 27, 2017

dhalperi commented Oct 27, 2017

corinaminer commented Oct 26, 2017 •

edited by dhalperi