JUNOS: Add and fix VNI/VxLan properties #8542

jeffkala · 2022-09-13T12:52:52Z

Update and fix vni/vxlan l2 and l3 properties.

batfish-bot · 2022-09-13T12:53:00Z

This change is

jeffkala

Working on figuring out how/why the testIntefaceArp test is failing, doesn't appear to be anything change in this PR that would cause that to fail now.

Reviewable status: 0 of 36 files reviewed, all discussions resolved

jeffkala

Got this cleaned up, forgot I removed a snippet of code during testing that I added back. Everything else should be passing test now.

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

codecov · 2022-09-14T01:38:55Z

Codecov Report

Merging #8542 (fd32735) into master (2a1e8ec) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 66.97%.

@@            Coverage Diff             @@
##           master    #8542      +/-   ##
==========================================
- Coverage   72.36%   72.36%   -0.01%     
==========================================
  Files        3296     3297       +1     
  Lines      168843   169020     +177     
  Branches    19799    19814      +15     
==========================================
+ Hits       122184   122303     +119     
- Misses      37554    37609      +55     
- Partials     9105     9108       +3

Files	Coverage Δ
...a/org/batfish/representation/juniper/BgpGroup.java	`84.45% <100.00%> (+0.54%)`	⬆️
.../batfish/representation/juniper/LogicalSystem.java	`95.17% <100.00%> (+0.06%)`	⬆️
...atfish/representation/juniper/RoutingInstance.java	`93.54% <100.00%> (+0.15%)`	⬆️
.../batfish/representation/juniper/SwitchOptions.java	`100.00% <100.00%> (ø)`
.../java/org/batfish/representation/juniper/Vlan.java	`100.00% <ø> (ø)`
...org/batfish/representation/juniper/VniOptions.java	`93.75% <93.75%> (ø)`
...epresentation/juniper/ExtendedCommunityOrAuto.java	`66.66% <66.66%> (ø)`
...fish/grammar/flatjuniper/ConfigurationBuilder.java	`67.72% <89.47%> (+0.31%)`	⬆️
...h/representation/juniper/JuniperConfiguration.java	`84.30% <45.37%> (-1.57%)`	⬇️

... and 11 files with indirect coverage changes

dhalperi

Reviewed 3 of 25 files at r1, all commit messages.
Reviewable status: 3 of 36 files reviewed, 5 unresolved discussions (waiting on @arifogel and @jeffkala)

a discussion (no related file):
Hi Jeff, please remove all the *.orig files that made it in by accident

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3411 at r3 (raw file):

  public void enterS_switch_options(S_switch_optionsContext ctx) {
    todo(ctx);
  }

confirming: is everything under s_switch_options now fully handled, or are there some rules lower down in the hierarchy we should move this todo(ctx) under.

Code quote:

  @Override
  public void enterS_switch_options(S_switch_optionsContext ctx) {
    todo(ctx);
  }

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

          .setVrfTargetCommunityorAuto(
              ExtendedCommunityOrAuto.of(
                  ExtendedCommunity.parse(ctx.extended_community().getText())));

Usually, we would introduce a toExtendedCommunity rule that takes the Extended_communityContext and returns either an ExtendedCommunity or an Optional<ExtendedCommunity>.

We use the former (not optional) when the lexer will only allow things we know that ExtendedCommunity#parse handles. We use the latter (optional) when the lexer is generous, and we use ExtendedCommunity#tryParse instead.

Can you:

Figure out which alternative to use? (Is the lexer strict or generous?)
Introduce the helper?

Btw, if it turns out we should use the optional then we do a lot of (abbrv'ed):

Optional<EC> maybeEc = toEC(ctx.ec);
if (!maybeEc.isPresent()) {
   warn(ctx, ...);
   maybe do some other stuff so inner rules don't crash.
   return;
}
EC ec = maybeEc.get();
handle for real

Code quote:

ExtendedCommunity.parse(ctx.extended_community().getText())));

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4244 at r3 (raw file):

  @Override
  public void enterB_family(B_familyContext ctx) {
    _currentBgpGroup.setEvpnAf(false);

This pattern looks suspicious to me -- what is happening here? What does this boolean mean?

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7085 at r3 (raw file):

      Optional<Integer> vlan = toInteger(ctx, ctx.id);
      vlan.ifPresent(_currentNamedVlan::setVlanId);
    }

What do you do if there's a none instead? Do you not need to clear it?

jeffkala

Reviewable status: 3 of 36 files reviewed, 5 unresolved discussions (waiting on @arifogel, @dhalperi, and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3411 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

confirming: is everything under s_switch_options now fully handled, or are there some rules lower down in the hierarchy we should move this todo(ctx) under.

I'd say everything that has expectations to need support is covered now. The remaining commands at that stanza level are things like mac-limits etc. Which I wouldn't see Batfish ever supporting? I could be wrong. With that being said I'm not overly sure if that means this todo should just be removed completely?

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Usually, we would introduce a toExtendedCommunity rule that takes the Extended_communityContext and returns either an ExtendedCommunity or an Optional<ExtendedCommunity>.

We use the former (not optional) when the lexer will only allow things we know that ExtendedCommunity#parse handles. We use the latter (optional) when the lexer is generous, and we use ExtendedCommunity#tryParse instead.

Can you:

Figure out which alternative to use? (Is the lexer strict or generous?)

Introduce the helper?

Btw, if it turns out we should use the optional then we do a lot of (abbrv'ed):
Optional<EC> maybeEc = toEC(ctx.ec);
if (!maybeEc.isPresent()) {
   warn(ctx, ...);
   maybe do some other stuff so inner rules don't crash.
   return;
}
EC ec = maybeEc.get();
handle for real

Will take a look.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4244 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

This pattern looks suspicious to me -- what is happening here? What does this boolean mean?

Will get back to you on this one, this might have been leftover on my end before Ari helped me figured this out.

jeffkala

Reviewable status: 3 of 36 files reviewed, 5 unresolved discussions (waiting on @arifogel, @dhalperi, and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3411 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

I'd say everything that has expectations to need support is covered now. The remaining commands at that stanza level are things like mac-limits etc. Which I wouldn't see Batfish ever supporting? I could be wrong. With that being said I'm not overly sure if that means this todo should just be removed completely?

think this is accurate relooking at it.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

Will take a look.

I implemented the ExtendedCommunityOrAuto which to me should make the lexer strict. I don't see how/what the helper would do in this case. Very well just be a misunderstanding on my part.

jeffkala

Reviewable status: 3 of 36 files reviewed, 5 unresolved discussions (waiting on @arifogel, @dhalperi, and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7085 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

What do you do if there's a none instead? Do you not need to clear it?

grammar supports NONE, not doing anything with it ATM, don't think clearing it is needed at this time.

dhalperi

Reviewable status: 3 of 36 files reviewed, 4 unresolved discussions (waiting on @arifogel, @dhalperi, and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

I implemented the ExtendedCommunityOrAuto which to me should make the lexer strict. I don't see how/what the helper would do in this case. Very well just be a misunderstanding on my part.

Even if extended_community is perfect (more on that later), we want a single toExtendedCommunity(Extended_communityContext) function that we use everywhere. Much like toLong, it dramatically reduces code reuse (of EC.parse(ctx.getText())). And if we have to evolve the parser rule, we don't have to hunt down all the various versions of it. (E.g., in the past we've had to change from ctx.getText() to a function that has special logic to handle spaces, or maybe there's a new juniper-specific alternative to origin:foo:bar that is not recognized by ExtendedCommunity.parse. Better to have it all in one place.
Hmm, I'm not sure the lexer/parser are strict. Here's the grammar for extended_community:

ec_literal
:
  dec COLON dec COLON dec
;

ec_named
:
  ec_type COLON ec_administrator COLON assigned_number = dec
;

ec_type
:
  ORIGIN
  | TARGET
;

extended_community
:
  ec_literal
  | ec_named
;

dec COLON dec COLON dec means that 1122334455667788990011223344556677889900:0:0 could be treated as ec_literal. But that would crash ExtendedCommunity.parse -- too many bits.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7085 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

grammar supports NONE, not doing anything with it ATM, don't think clearing it is needed at this time.

Remember that Batfish is designed to handle incremental changes. That is when a user has their running config at the top, and then appends their interactive CLI commands.

What is the result if a user has this config:

set ... vlan-id 7

and then types

set ... vlan-id none

?

jeffkala

Reviewable status: 3 of 36 files reviewed, 4 unresolved discussions (waiting on @arifogel, @dhalperi, and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Even if extended_community is perfect (more on that later), we want a single toExtendedCommunity(Extended_communityContext) function that we use everywhere. Much like toLong, it dramatically reduces code reuse (of EC.parse(ctx.getText())). And if we have to evolve the parser rule, we don't have to hunt down all the various versions of it. (E.g., in the past we've had to change from ctx.getText() to a function that has special logic to handle spaces, or maybe there's a new juniper-specific alternative to origin:foo:bar that is not recognized by ExtendedCommunity.parse. Better to have it all in one place.

Hmm, I'm not sure the lexer/parser are strict. Here's the grammar for extended_community:
ec_literal
:
  dec COLON dec COLON dec
;

ec_named
:
  ec_type COLON ec_administrator COLON assigned_number = dec
;

ec_type
:
  ORIGIN
  | TARGET
;

extended_community
:
  ec_literal
  | ec_named
;
dec COLON dec COLON dec means that 1122334455667788990011223344556677889900:0:0 could be treated as ec_literal. But that would crash ExtendedCommunity.parse -- too many bits.

That's fair, I think alot of this is just me still being pretty novice at java and batfish development. What you're saying does indeed make more sense to me now. I'll give writing the helper a try. Thanks for the information and patience.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7085 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Remember that Batfish is designed to handle incremental changes. That is when a user has their running config at the top, and then appends their interactive CLI commands.

What is the result if a user has this config:
set ... vlan-id 7
and then types
set ... vlan-id none
?

That is fair, I forgot about that aspect of it, will take a look at this.

jeffkala

Reviewable status: 3 of 36 files reviewed, 4 unresolved discussions (waiting on @arifogel and @dhalperi)

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

Hi Jeff, please remove all the *.orig files that made it in by accident

Done

jeffkala

Reviewable status: 3 of 36 files reviewed, 4 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Extended_communityContext
Gave it a shot, hopefully this is closer to what your ask was?

jeffkala

Reviewable status: 3 of 36 files reviewed, 4 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4244 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

Will get back to you on this one, this might have been leftover on my end before Ari helped me figured this out.

Defaulting the creation of the EVPN address family to False (off) to avoid the memory of building this out when evpn AF is not used in the configuration. We then build the AF bgp process for these only when necessary. This was setup with help from Ari previously.

jeffkala

Reviewable status: 3 of 36 files reviewed, 3 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7085 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

That is fair, I forgot about that aspect of it, will take a look at this.

done

Code quote:

    if (ctx.id != null) {
      Optional<Integer> vlan = toInteger(ctx, ctx.id);
      vlan.ifPresent(_currentNamedVlan::setVlanId);
    }

jeffkala · 2022-10-29T01:23:46Z

anyway to push this along to get it over the finish line?

arifogel

Reviewed 15 of 25 files at r1, 5 of 17 files at r2, 3 of 3 files at r3, 8 of 9 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @dhalperi and @jeffkala)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2041 at r5 (raw file):

  private static @Nonnull ExtendedCommunity toExtendedCommunity(Vt_communityContext ctx) {
    if (ctx.getText() != null) {
      return ExtendedCommunity.parse(ctx.getText());

I'm not convinced the grammar enforces that a vt_community nor extended_community as guaranteed to be parseable by ExtendedCommunity.parse, as `@dhalperi pointed out.

Either:

update the grammar to make it so (if possible)
OR:

this should be non-static, return an Optional<ExtendedCommunity> via tryParse, and callers will need to handle the case where Optional.empty() is returned gracefully.

Same goes for your other converter.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2042 at r5 (raw file):

    if (ctx.getText() != null) {
      return ExtendedCommunity.parse(ctx.getText());
    } else {

This branch should never trigger, as an invariant of ParserRuleContext. Just get rid of the if and this branch. Same for your other converter.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3464 at r5 (raw file):

  @Override
  public void exitSovt_auto(Sovt_autoContext ctx) {
    if (ctx.AUTO() != null) {

This is guaranteed to be true by the grammar. Ditch the if.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4408 at r5 (raw file):

  @Override
  public void enterE_vni_options(E_vni_optionsContext ctx) {
    _currentVni = toInt(ctx.id);

This is not safe since id is currently a dec. You should follow the converter+optional pattern used in e.g. CiscoNxosControlPlaneExtractor.toInteger(ParserRuleContext messageCtx, Eigrp_asnContext ctx)

See: https://github.com/batfish/batfish/blob/master/docs/extraction/README.md#validating-and-converting-parse-tree-nodes-with-variable-text
And while you're at it, couldn't hurt to check out our new parsing docs: https://github.com/batfish/batfish/blob/master/docs/parsing/README.md

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4409 at r5 (raw file):

  public void enterE_vni_options(E_vni_optionsContext ctx) {
    _currentVni = toInt(ctx.id);
    assert _currentLogicalSystem.getVniOptions() != null;

This assertion is unnecessary. You should instead fix the annotation. See comment in LogicalSystem.java

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4421 at r5 (raw file):

  @Override
  public void exitEvovt_auto(Evovt_autoContext ctx) {
    if (ctx.AUTO() != null) {

This is guaranteed to be true by the grammar. Remove the if.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4424 at r5 (raw file):

      _currentLogicalSystem
          .getVniOptions()
          .get(_currentVni)

If you make _currentVni a VniOptions (and rename appropriately), you can simplify all instances of:

      _currentLogicalSystem
          .getVniOptions()
          .get(_currentVni)

to:

_currentVniOptions

You should then set _currentVniOptions = null for sanity in exitE_vni_options (not enterE_vni_options`).

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 6081 at r5 (raw file):

  @Override
  public void enterRo_route_distinguisher_id(Ro_route_distinguisher_idContext ctx) {
    if (ctx.ip_address() != null) {

Guaranteed to be true by the grammar. Remove the if.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7237 at r5 (raw file):

  public void exitBf_evpn(Bf_evpnContext ctx) {
    _currentBgpGroup.setEvpnAf(false);
    if (ctx.EVPN() != null) {

This is guaranteed to be true by the grammar, so this whole function is effectively:

_currentBgpGroup.setEvpnAf(true);

projects/batfish/src/main/java/org/batfish/representation/juniper/ExtendedCommunityOrAuto.java line 10 at r5 (raw file):

/** Either {@code auto} or an explicit {@link ExtendedCommunity}. */
public final class ExtendedCommunityOrAuto implements Serializable {

Prefer not to use null as a sentinel value. I think overall this class has a confusing API, and its content is largely unnecessary.

I'd suggest just changing this from a class to an interface that extends Serializable.
Then I'd suggest an ExtendedCommunityLiteral and ExtendedCommunityAuto class in this package that implement that interface.
The literal class would just have a single field that is an org.batfish.datamodel.bgp.community.ExtendedCommunity.
The auto class would be a singleton with a private constructor, a private static INSTANCE field that is a new ExtendedCommunityAuto(), and a public static instance() method that returns INSTANCE.

You'll probably need to override hashCode and equals for those classes since you did it here.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 779 at r5 (raw file):

      }
      neighbor.setLocalIp(localIp);
      if (ig.getEvpnAf() != null) {

What if it's false?
This should be exactly Boolean.TRUE.equals(ig.getEvpnAf())

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 784 at r5 (raw file):

            .setPropagateUnmatched(true)
            .setAddressFamilyCapabilities(
                AddressFamilyCapabilities.builder().setAllowRemoteAsOut(ALWAYS).build());

Sanity check: allow remote as out is always true for juniper? Just this address family?

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 785 at r5 (raw file):

            .setAddressFamilyCapabilities(
                AddressFamilyCapabilities.builder().setAllowRemoteAsOut(ALWAYS).build());
        evpnAfBuilder.setL3Vnis(convertL3Evpn().build());

First, setL3Vnis cannot accept a null argument (you annotated return type of convertL3Evpn @Nullable.

Second, just have convertL3Evpn return a (@Nonnull) Set, and build on its return statement(s).

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 2070 at r5 (raw file):

  }

  private @Nullable ImmutableSet.Builder<Layer3VniConfig> convertL3Evpn() {

This function is too long, and I'm having trouble reading it. Please break it up into helper functions for the different cases.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 2170 at r5 (raw file):

        } else {
          // RI is used, but RD is not manually configured. Junos takes route-distinguisher-id +
          // random number to generate unique RD.

This is far from unique. In a typical large network, you are virtually guaranteed to have collisions here.

That aside, you must not create non-deterministic data model elements, because Batfish must produce the same output for the same set of inputs.

This is going to need a more thorough design to avoid collisions and preserve determinism.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 2203 at r5 (raw file):

      } else {
        if (_c.getVrfs()
                .get(getInterfaceOrUnitByName(l3Interface).get().getRoutingInstance().getName())

Is it guaranteed that the named l3 interface actually exists? This will throw an NPE if it doesn't.
Same question for vtepSource, and also in convertL2Vni.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 2214 at r5 (raw file):

                  .setVni(vxlan.getVniId())
                  .setSrcVrf(
                      getInterfaceOrUnitByName(l3Interface).get().getRoutingInstance().getName())

Please Compute getInterfaceOrUnityByName(l3Interface) once, and reuse the value. Apply this pattern generally.

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 3927 at r5 (raw file):

    // convert vxlan.
    convertVxlan();

Any reason the replacement functions were moved so far up?

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 4377 at r5 (raw file):

                        // set IRB VLAN ID if assigned
                        newUnitInterface.setVlan(irbVlanIds.get(name));

Remove unrelated whitespace change

projects/batfish/src/main/java/org/batfish/representation/juniper/LogicalSystem.java line 109 at r5 (raw file):

  @Nullable private SwitchOptions _switchOptions;

  @Nullable private Map<Integer, VniOptions> _vniOptions;

This is guaranteed to be non-null by the assignment in the constructor.

projects/batfish/src/main/java/org/batfish/representation/juniper/VniOptions.java line 7 at r5 (raw file):

import org.batfish.datamodel.bgp.community.ExtendedCommunity;

public class VniOptions implements Serializable {

Please add javadocs for all added non-private classes and methods, with the exception of simple getters and setters, and @Override methods of subtypes.
Also nit: mark any classes that will not be extended as final.
Also, Make getter return types and setter argument types have the same null annotation as the corresponding fields.

For non-null parameters of a new class, can skip the annotation and instead mark the class with @ParametersAreNonnullByDefault. Note this default does not apply to return types.

projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java line 3506 at r5 (raw file):

                    .build())));

    // Ensure a vlan-id can be present and that it can be cleared back to null.

How does this assertion test that vlan-id can be present?

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/juniper-vxlan-l3vni line 4 at r5 (raw file):

set system host-name juniper-vxlan-l3vni
#
set interfaces xe-0/0/0 description to_host-1

Can you explain the changes in this file?

arifogel

Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @dhalperi and @jeffkala)

projects/batfish/src/main/java/org/batfish/representation/juniper/VniOptions.java line 7 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please add javadocs for all added non-private classes and methods, with the exception of simple getters and setters, and @Override methods of subtypes.
Also nit: mark any classes that will not be extended as final.
Also, Make getter return types and setter argument types have the same null annotation as the corresponding fields.

For non-null parameters of a new class, can skip the annotation and instead mark the class with @ParametersAreNonnullByDefault. Note this default does not apply to return types.

This guidance has since been documented here. That content in that link supersedes any prior comments in this PR about what to annotate, javadoc.

jeffkala · 2022-11-24T17:55:50Z

Thanks @arifogel, hoping to take care of all this feedback soon.

jeffkala · 2023-02-22T17:51:20Z

Hoping to take this feedback and implement soon.

dhalperi · 2023-02-22T17:55:49Z

Thanks @jeffkala - let us know!

You should probably start by merging/rebasing as things have continued to change.

jeffkala

Reviewable status: 30 of 38 files reviewed, 24 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2042 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This branch should never trigger, as an invariant of ParserRuleContext. Just get rid of the if and this branch. Same for your other converter.

Done.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4421 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This is guaranteed to be true by the grammar. Remove the if.

Done.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 7237 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This is guaranteed to be true by the grammar, so this whole function is effectively:
_currentBgpGroup.setEvpnAf(true);

Done.

arifogel

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @dhalperi and @jeffkala)

arifogel

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @dhalperi and @jeffkala)

jeffkala

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3441 at r3 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

Extended_communityContext
Gave it a shot, hopefully this is closer to what your ask was?

should be good now in the next commit.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4408 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This is not safe since id is currently a dec. You should follow the converter+optional pattern used in e.g. CiscoNxosControlPlaneExtractor.toInteger(ParserRuleContext messageCtx, Eigrp_asnContext ctx)

See: https://github.com/batfish/batfish/blob/master/docs/extraction/README.md#validating-and-converting-parse-tree-nodes-with-variable-text
And while you're at it, couldn't hurt to check out our new parsing docs: https://github.com/batfish/batfish/blob/master/docs/parsing/README.md

Done.

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4424 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

If you make _currentVni a VniOptions (and rename appropriately), you can simplify all instances of:
      _currentLogicalSystem
          .getVniOptions()
          .get(_currentVni)
to:
_currentVniOptions
You should then set _currentVniOptions = null for sanity in exitE_vni_options (not enterE_vni_options`).

Done.

projects/batfish/src/main/java/org/batfish/representation/juniper/ExtendedCommunityOrAuto.java line 10 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Prefer not to use null as a sentinel value. I think overall this class has a confusing API, and its content is largely unnecessary.

I'd suggest just changing this from a class to an interface that extends Serializable.
Then I'd suggest an ExtendedCommunityLiteral and ExtendedCommunityAuto class in this package that implement that interface.
The literal class would just have a single field that is an org.batfish.datamodel.bgp.community.ExtendedCommunity.
The auto class would be a singleton with a private constructor, a private static INSTANCE field that is a new ExtendedCommunityAuto(), and a public static instance() method that returns INSTANCE.

You'll probably need to override hashCode and equals for those classes since you did it here.

Was following an established pattern on this one from cisco_nxos. Can look into your suggestion though in the meantime.

batfish/projects/batfish/src/main/java/org/batfish/representation/cisco_nxos/ExtendedCommunityOrAuto.java

Line 10 in 2a1e8ec

public final class ExtendedCommunityOrAuto implements Serializable {

projects/batfish/src/main/java/org/batfish/representation/juniper/LogicalSystem.java line 109 at r7 (raw file):

  @Nullable private SwitchOptions _switchOptions;

  @Nullable private Map<Integer, VniOptions> _vniOptions;

fixed in the next commit that will be pushed.

projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java line 3506 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

How does this assertion test that vlan-id can be present?

I updated this and it will be in my next commit.

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/juniper-vxlan-l3vni line 4 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Can you explain the changes in this file?

changes were required to this file to correct the standard l3vni configuration. In initial test file I was missing some of the required fields.

jeffkala

Reviewable status: 25 of 39 files reviewed, 17 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 4424 at r5 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

Done.

Tried this and _currentVniOptions was always null, so I reverted back to this "longer" pattern. I tried a few different ways to get it to work with no luck.

jeffkala

Reviewable status: 25 of 39 files reviewed, 17 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2041 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I'm not convinced the grammar enforces that a vt_community nor extended_community as guaranteed to be parseable by ExtendedCommunity.parse, as `@dhalperi pointed out.

Either:

update the grammar to make it so (if possible)
OR:

this should be non-static, return an Optional<ExtendedCommunity> via tryParse, and callers will need to handle the case where Optional.empty() is returned gracefully.

Same goes for your other converter.

Changed this to tryParse as suggested.

jeffkala

Did a first pass on the first set of recommendations. Will be working on the rest soon. Would love to get this one over the finish line ASAP.

Reviewable status: 25 of 39 files reviewed, 17 unresolved discussions (waiting on @arifogel and @dhalperi)

jeffkala

Reviewable status: 25 of 39 files reviewed, 17 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 779 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

What if it's false?
This should be exactly Boolean.TRUE.equals(ig.getEvpnAf())

I have the below snippet. Thought by creating new Evpn() as a "dummy" would handle it. Mind expanding on this concern?

Code snippet:

  @Override
  public void enterP_evpn(P_evpnContext ctx) {
    if (_currentLogicalSystem.getEvpn() == null) {
      _currentLogicalSystem.setEvpn(new Evpn());
    }
  }

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 784 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Sanity check: allow remote as out is always true for juniper? Just this address family?

will get this fixed, looks like its disabled by default https://www.juniper.net/documentation/us/en/software/junos/bgp/topics/ref/statement/advertise-peer-as-edit-protocols-bgp.html#id-13323108__d67115e166

jeffkala

Reviewable status: 25 of 39 files reviewed, 16 unresolved discussions (waiting on @arifogel and @dhalperi)

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 785 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

First, setL3Vnis cannot accept a null argument (you annotated return type of convertL3Evpn @Nullable.

Second, just have convertL3Evpn return a (@Nonnull) Set, and build on its return statement(s).

reworked it

projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 2070 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This function is too long, and I'm having trouble reading it. Please break it up into helper functions for the different cases.

reworked it

jeffkala · 2023-10-31T21:56:07Z

@dhalperi @arifogel I know its been awhile, but few weeks back I did some updates here. I believe I covered the concerns, is it possible to get a re-review.

arifogel · 2024-04-13T00:23:03Z

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2062 at r9 (raw file):

    // system are as follows:
    //
    //      4-octet AS number: 00000000-22222222

I'm having a hard time understanding this comment.

The text in the quoted line looks like 8 octets, not 4. Or is this a range?

arifogel · 2024-04-13T00:23:05Z

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 2065 at r9 (raw file):

    //      4-octet AS number followed by 4-octet value: 00000000-22222222:11111111-44444444
    //      16-octet value: 11111111-22222222-33333333-44444444
    //      The first two patterns are used for standard extended communities, while the third

What is a "standard" extended community, and what is a "large" extended community?

IIUC there are three broad types of communities:

standard (4 octets)
extended (6 octets) (with many subtypes, like site-of-origin)
large (12 octets)

See https://datatracker.ietf.org/doc/html/rfc8092#section-1

jeffkala added 3 commits September 12, 2022 16:04

JUNOS: vxlan l2vni and l3vni RD and RT logic fixes and additions

4cb9b7e

sync upstream and fix conflicts

8ea91ec

fix auto import mess ups and cleanups

4890fe6

jeffkala added 2 commits September 13, 2022 12:03

update ref files and get tests to pass. cleanup * imports

f47f172

fix l2 and l3vni tests and fix formatting

57e98da

jeffkala marked this pull request as ready for review September 13, 2022 21:38

jeffkala commented Sep 13, 2022

View reviewed changes

dhalperi requested a review from arifogel September 14, 2022 00:16

revert inadvertent config removal

0e877d4

jeffkala commented Sep 14, 2022

View reviewed changes

dhalperi reviewed Sep 19, 2022

View reviewed changes

jeffkala commented Sep 20, 2022

View reviewed changes

dhalperi reviewed Sep 20, 2022

View reviewed changes

jeffkala commented Sep 21, 2022

View reviewed changes

remove .orig files as they shouldn't have been included

c026ffa

jeffkala commented Sep 21, 2022

View reviewed changes

initial toEC implementation

c934bd1

jeffkala commented Sep 22, 2022

View reviewed changes

fix unset vlan

4164886

jeffkala commented Sep 22, 2022

View reviewed changes

arifogel reviewed Oct 29, 2022

View reviewed changes

arifogel reviewed Nov 24, 2022

View reviewed changes

jeffkala added 2 commits February 27, 2023 22:26

sync feature branch with upstream

85f0a4f

inital fixes that are guarenteed by parser

4ae4409

jeffkala commented Mar 3, 2023

View reviewed changes

arifogel reviewed Mar 24, 2023

View reviewed changes

jeffkala added 2 commits August 9, 2023 14:49

sync with upstream

1d3d6a9

work to make ec strict

7376212

arifogel reviewed Aug 11, 2023

View reviewed changes

jeffkala added 2 commits October 11, 2023 09:46

merge with upstream

0298d0d

another run through PR code fixes

d73232f

jeffkala commented Oct 13, 2023

View reviewed changes

checkstyle fixes

5846cef

jeffkala commented Oct 13, 2023

View reviewed changes

refactor convertl3evpn to smaller helpers

fd32735

jeffkala commented Oct 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JUNOS: Add and fix VNI/VxLan properties #8542

JUNOS: Add and fix VNI/VxLan properties #8542

jeffkala commented Sep 13, 2022

batfish-bot commented Sep 13, 2022

jeffkala left a comment

jeffkala left a comment

codecov bot commented Sep 14, 2022 •

edited

dhalperi left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

dhalperi left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala commented Oct 29, 2022

arifogel left a comment

arifogel left a comment

jeffkala commented Nov 24, 2022

jeffkala commented Feb 22, 2023

dhalperi commented Feb 22, 2023

jeffkala left a comment

arifogel left a comment

arifogel left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala left a comment

jeffkala commented Oct 31, 2023

arifogel commented Apr 13, 2024

arifogel commented Apr 13, 2024

JUNOS: Add and fix VNI/VxLan properties #8542

Are you sure you want to change the base?

JUNOS: Add and fix VNI/VxLan properties #8542

Conversation

jeffkala commented Sep 13, 2022

batfish-bot commented Sep 13, 2022

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 14, 2022 • edited

Codecov Report

dhalperi left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala commented Oct 29, 2022

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

jeffkala commented Nov 24, 2022

jeffkala commented Feb 22, 2023

dhalperi commented Feb 22, 2023

jeffkala left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

arifogel left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala left a comment

Choose a reason for hiding this comment

jeffkala commented Oct 31, 2023

arifogel commented Apr 13, 2024

arifogel commented Apr 13, 2024

codecov bot commented Sep 14, 2022 •

edited