TestCompliance/Add_IPv4_Entry_that_references_a_NHG_in_a_different_network_instance submits operations out of order

[jeremyrandnokia]

When running this test, it sends this operation first:

operation {
  id: 1
  network_instance: "NON-DEFAULT-VRF"
  op: ADD
  ipv4 {
    prefix: "1.1.1.1/32"
    ipv4_entry {
      next_hop_group_network_instance {
        value: "DEFAULT"
      }
      next_hop_group {
        value: 1
      }
    }
  }
  election_id {
    low: 4500
  }
}

Note that it is referencing a next_hop_group in network instance DEFAULT which has not been created yet. This operation fails on our gribi server due to that.

After that, the following operation is sent:

operation {
  id: 2
  op: ADD
  next_hop_group {
    id: 1
    next_hop_group {
      next_hop {
        index: 1
        next_hop {
          weight {
            value: 1
          }
        }
      }
    }
  }
  election_id {
    low: 4500
  }
}

Note that there is no network instance specified here. I believe network instance DEFAULT should be specified.

Finally, the following operation is sent:

operation {
  id: 3
  op: ADD
  next_hop {
    index: 1
    next_hop {
      ip_address {
        value: "2.2.2.2"
      }
    }
  }
  election_id {
    low: 4500
  }
}

Again, no network instance is provided in the modify request operation.

I think that the next hop should be created first in the DEFAULT network instance, followed by the next hop group, also in the DEFAULT network instance and finally, the IPv4 entry should be created.

[xw-g]

hi @jeremyrandnokia,

• What test are you running? Or, what is the question? :) Just reading the issue title, I think we do expect the test to fail because of the ordering of rule injections.
• BTW, currently the referenced gRIBI go implementation specify the default network instance if the field is not set. I have proposed to change this behavior via https://github.com/openconfig/gribi/pull/34

[jeremyrandnokia]

The test is AddIPv4EntryDifferentNINHG() in compliance/compliance.go. It currently looks like this:

ops := []func(){
        func() {
            c.Modify().AddEntry(t, fluent.IPv4Entry().
                WithPrefix("1.1.1.1/32").
                WithNetworkInstance(vrfName).
                WithNextHopGroup(1).
                WithNextHopGroupNetworkInstance(defaultNetworkInstanceName))
            c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithID(1).AddNextHop(1, 1))
            c.Modify().AddEntry(t, fluent.NextHopEntry().WithIndex(1).WithIPAddress("2.2.2.2"))
        },
    }

Note that the IPv4 entry is added first, then the NHG and finally the NH. Also, no network instance is specified for the NHG and the NH. If I change the above to:

ops := []func(){
        func() {
            c.Modify().AddEntry(t, fluent.NextHopEntry().WithNetworkInstance(defaultNetworkInstanceName).WithIndex(1).WithIPAddress("2.2.2.2"))
            c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithNetworkInstance(defaultNetworkInstanceName).WithID(1).AddNextHop(1, 1))
            c.Modify().AddEntry(t, fluent.IPv4Entry().
                WithPrefix("1.1.1.1/32").
                WithNetworkInstance(vrfName).
                WithNextHopGroup(1).
                WithNextHopGroupNetworkInstance(defaultNetworkInstanceName))
        },
    }

Then the test will now pass for me. So, I needed to re-order the entries so that they are created in the NH, NHG and IPv4 entry order and ensure that a network instance was specified with the NH and NHG entries.

[robshakir]

Please feel free to open a PR with this change in it. We did agree that AFTOperation messages within a ModifyRequest should be processed in the order that they are specified by the client I believe, so this is a reasonable change.

[jeremyrandnokia]

I am afraid that my organization is unable to sign the Google CLA for some reason so I cannot submit a PR. But if the order of entries in the AddIPv4EntryDifferentNINHG() can be reordered so the the NH is added first, then the NHG and then IPv4 entry, that would solve the problem.

[xw-g]

Filed PR #132.

I will also check why/how the reference implementation passed the test previous.