From 1085e256ff3e3aaa241ac0e36a6f9a241a6c104e Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 07:20:38 -0700 Subject: [PATCH 01/15] Add IPv4 prefix set integration test --- integration_tests/dut_policy_tests/BUILD | 8 + .../dut_policy_tests/prefix_set/BUILD | 19 + .../prefix_set/prefix_set_test.go | 162 ++++++ .../dut_policy_tests/testbed.pb.txt | 106 ++++ .../dut_policy_tests/topology.pb.txt | 91 ++++ policytest/BUILD | 19 + policytest/policytest.go | 464 ++++++++++++++++++ 7 files changed, 869 insertions(+) create mode 100644 integration_tests/dut_policy_tests/BUILD create mode 100644 integration_tests/dut_policy_tests/prefix_set/BUILD create mode 100644 integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go create mode 100644 integration_tests/dut_policy_tests/testbed.pb.txt create mode 100644 integration_tests/dut_policy_tests/topology.pb.txt create mode 100644 policytest/BUILD create mode 100644 policytest/policytest.go diff --git a/integration_tests/dut_policy_tests/BUILD b/integration_tests/dut_policy_tests/BUILD new file mode 100644 index 00000000..870437dd --- /dev/null +++ b/integration_tests/dut_policy_tests/BUILD @@ -0,0 +1,8 @@ +filegroup( + name = "topology_testbed", + srcs = [ + "testbed.pb.txt", + "topology.pb.txt", + ], + visibility = ["//visibility:public"], +) diff --git a/integration_tests/dut_policy_tests/prefix_set/BUILD b/integration_tests/dut_policy_tests/prefix_set/BUILD new file mode 100644 index 00000000..efb1da3e --- /dev/null +++ b/integration_tests/dut_policy_tests/prefix_set/BUILD @@ -0,0 +1,19 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "prefix_set_test", + size = "enormous", + srcs = ["prefix_set_test.go"], + data = ["//integration_tests/dut_policy_tests:topology_testbed"], + tags = [ + "exclusive", + ], + deps = [ + "//bgp/tests/proto/policyval", + "//internal/binding", + "//policytest", + "@com_github_openconfig_ondatra//:ondatra", + "@com_github_openconfig_ondatra//gnmi", + "@com_github_openconfig_ondatra//gnmi/oc", + ], +) diff --git a/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go new file mode 100644 index 00000000..0459c61f --- /dev/null +++ b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go @@ -0,0 +1,162 @@ +/* + Copyright 2022 Google LLC + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package integration_test + +import ( + "testing" + + "github.com/openconfig/lemming/internal/binding" + "github.com/openconfig/lemming/policytest" + "github.com/openconfig/ondatra" + "github.com/openconfig/ondatra/gnmi" + "github.com/openconfig/ondatra/gnmi/oc" + + valpb "github.com/openconfig/lemming/bgp/tests/proto/policyval" +) + +func TestMain(m *testing.M) { + ondatra.RunTests(m, binding.Get("..")) +} + +func TestPrefixSet(t *testing.T) { + installPolicies := func(t *testing.T, dut1, dut2, _, _, _ *policytest.Device, invert bool) { + t.Log("Installing test policies") + prefix1 := "10.33.0.0/16" + prefix2 := "10.34.0.0/16" + + // Policy to reject routes with the given prefix set + policyName := "def1" + + // Create prefix set + prefixSetName := "reject-" + prefix1 + prefix1Path := policytest.RoutingPolicyPath.DefinedSets().PrefixSet(prefixSetName).Prefix(prefix1, "exact").IpPrefix() + gnmi.Replace(t, dut2, prefix1Path.Config(), prefix1) + prefix2Path := policytest.RoutingPolicyPath.DefinedSets().PrefixSet(prefixSetName).Prefix(prefix2, "16..23").IpPrefix() + gnmi.Replace(t, dut2, prefix2Path.Config(), prefix2) + + policy := &oc.RoutingPolicy_PolicyDefinition_Statement_OrderedMap{} + stmt, err := policy.AppendNew("stmt1") + if err != nil { + t.Fatalf("Cannot append new BGP policy statement: %v", err) + } + // Match on prefix set & reject route + stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetPrefixSet(prefixSetName) + if invert { + stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetMatchSetOptions(oc.RoutingPolicy_MatchSetOptionsRestrictedType_INVERT) + } else { + stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetMatchSetOptions(oc.RoutingPolicy_MatchSetOptionsRestrictedType_ANY) + } + stmt.GetOrCreateActions().SetPolicyResult(oc.RoutingPolicy_PolicyResultType_REJECT_ROUTE) + // Install policy + gnmi.Replace(t, dut2, policytest.RoutingPolicyPath.PolicyDefinition(policyName).Config(), &oc.RoutingPolicy_PolicyDefinition{Statement: policy}) + gnmi.Replace(t, dut2, policytest.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().ImportPolicy().Config(), []string{policyName}) + } + + invertResult := func(result valpb.RouteTestResult, invert bool) valpb.RouteTestResult { + if invert { + switch result { + case valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT: + return valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD + case valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD: + return valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT + default: + } + } + return result + } + + getspec := func(invert bool) *valpb.PolicyTestCase { + spec := &valpb.PolicyTestCase{ + Description: "Test that one prefix gets accepted and the other rejected via an ANY prefix-set.", + RouteTests: []*valpb.RouteTestCase{{ + Description: "Exact match", + Input: &valpb.TestRoute{ + ReachPrefix: "10.33.0.0/16", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD, invert), + }, { + Description: "Not exact match", + Input: &valpb.TestRoute{ + ReachPrefix: "10.33.0.0/17", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, invert), + }, { + Description: "No match with any prefix", + Input: &valpb.TestRoute{ + ReachPrefix: "10.3.0.0/16", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, invert), + }, { + Description: "mask length too short", + Input: &valpb.TestRoute{ + ReachPrefix: "10.34.0.0/15", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, invert), + }, { + Description: "Lower end of mask length", + Input: &valpb.TestRoute{ + ReachPrefix: "10.34.0.0/16", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD, invert), + }, { + Description: "Middle of mask length", + Input: &valpb.TestRoute{ + ReachPrefix: "10.34.0.0/20", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD, invert), + }, { + Description: "Upper end of mask length", + Input: &valpb.TestRoute{ + ReachPrefix: "10.34.0.0/23", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_DISCARD, invert), + }, { + Description: "mask length too long", + Input: &valpb.TestRoute{ + ReachPrefix: "10.34.0.0/24", + }, + ExpectedResultBeforePolicy: valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, + ExpectedResult: invertResult(valpb.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT, invert), + }}, + } + return spec + } + + t.Run("ANY", func(t *testing.T) { + policytest.TestPolicy(t, policytest.PolicyTestCase{ + Spec: getspec(false), + InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { + installPolicies(t, dut1, dut2, dut3, dut4, dut5, false) + }, + }) + }) + t.Run("INVERT", func(t *testing.T) { + policytest.TestPolicy(t, policytest.PolicyTestCase{ + Spec: getspec(true), + InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { + installPolicies(t, dut1, dut2, dut3, dut4, dut5, true) + }, + }) + }) +} diff --git a/integration_tests/dut_policy_tests/testbed.pb.txt b/integration_tests/dut_policy_tests/testbed.pb.txt new file mode 100644 index 00000000..d00320cb --- /dev/null +++ b/integration_tests/dut_policy_tests/testbed.pb.txt @@ -0,0 +1,106 @@ +# proto-file: github.com/openconfig/ondatra/blob/main/proto/testbed.proto +# proto-message: ondatra.Testbed + +# PolicyTestCase contains the specifications for a DUT-only policy test. +# +# Topology: +# +# DUT1 (AS 64500) -> DUT2 (AS 64500) -> DUT3 (AS 64501) +# ^ +# | +# DUT4 (AS 64502) -> DUT5 (AS 64500) +# +# Additionally, DUT0 is present as a neighbour for DUT1, DUT4, and DUT5 +# to allow a static route to be resolvable. +# +# Currently by convention, all policies are installed on DUT1 (export), DUT5 +# (export), and DUT2 (import). This is because GoBGP only withdraws routes on +# import policy change after a soft reset: +# https://github.com/osrg/gobgp/blob/master/docs/sources/policy.md#policy-and-soft-reset + +# This DUT connects to DUT1, DUT4, and DUT5 for resolving static routes. +duts { + id: "dut0" + ports { + id: "port1" + } + ports { + id: "port2" + } + ports { + id: "port3" + } +} + +duts { + id: "dut1" + ports { + id: "port0" + } + ports { + id: "port1" + } +} + +duts { + id: "dut2" + ports { + id: "port1" + } + ports { + id: "port2" + } + ports { + id: "port3" + } +} + +duts { + id: "dut3" + ports { + id: "port1" + } +} + +duts { + id: "dut4" + ports { + id: "port0" + } + ports { + id: "port1" + } +} + +duts { + id: "dut5" + ports { + id: "port0" + } + ports { + id: "port1" + } + ports { + id: "port2" + } +} + +links { + a: "dut1:port1" + b: "dut2:port1" +} + +links { + a: "dut2:port2" + b: "dut3:port1" +} + +links { + a: "dut4:port1" + b: "dut5:port1" +} + +links { + a: "dut5:port2" + b: "dut2:port3" +} diff --git a/integration_tests/dut_policy_tests/topology.pb.txt b/integration_tests/dut_policy_tests/topology.pb.txt new file mode 100644 index 00000000..e45c180e --- /dev/null +++ b/integration_tests/dut_policy_tests/topology.pb.txt @@ -0,0 +1,91 @@ +name: "policy" +nodes: { + name: "lemming0" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +nodes: { + name: "lemming1" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +nodes: { + name: "lemming2" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +nodes: { + name: "lemming3" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +nodes: { + name: "lemming4" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +nodes: { + name: "lemming5" + vendor: OPENCONFIG + model: "LEMMING" + config: { + args: "--v=1" + } +} +links: { + a_node: "lemming1" + a_int: "eth1" + z_node: "lemming2" + z_int: "eth1" +} +links: { + a_node: "lemming2" + a_int: "eth2" + z_node: "lemming3" + z_int: "eth1" +} +links: { + a_node: "lemming4" + a_int: "eth1" + z_node: "lemming5" + z_int: "eth1" +} +links: { + a_node: "lemming5" + a_int: "eth2" + z_node: "lemming2" + z_int: "eth3" +} +links: { + a_node: "lemming0" + a_int: "eth1" + z_node: "lemming1" + z_int: "eth9" +} +links: { + a_node: "lemming0" + a_int: "eth2" + z_node: "lemming4" + z_int: "eth9" +} +links: { + a_node: "lemming0" + a_int: "eth3" + z_node: "lemming5" + z_int: "eth9" +} diff --git a/policytest/BUILD b/policytest/BUILD new file mode 100644 index 00000000..04cb3234 --- /dev/null +++ b/policytest/BUILD @@ -0,0 +1,19 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "policytest", + srcs = ["policytest.go"], + importpath = "github.com/openconfig/lemming/policytest", + visibility = ["//visibility:public"], + deps = [ + "//bgp/tests/proto/policyval", + "//gnmi/fakedevice", + "//internal/attrs", + "@com_github_openconfig_ondatra//:ondatra", + "@com_github_openconfig_ondatra//gnmi", + "@com_github_openconfig_ondatra//gnmi/oc", + "@com_github_openconfig_ondatra//gnmi/oc/ocpath", + "@com_github_openconfig_ygnmi//ygnmi", + "@com_github_openconfig_ygot//ygot", + ], +) diff --git a/policytest/policytest.go b/policytest/policytest.go new file mode 100644 index 00000000..c4169c4b --- /dev/null +++ b/policytest/policytest.go @@ -0,0 +1,464 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package policytest + +import ( + "testing" + "time" + + "github.com/openconfig/lemming/bgp/tests/proto/policyval" + "github.com/openconfig/lemming/gnmi/fakedevice" + "github.com/openconfig/lemming/internal/attrs" + "github.com/openconfig/ondatra" + "github.com/openconfig/ondatra/gnmi" + "github.com/openconfig/ondatra/gnmi/oc" + "github.com/openconfig/ondatra/gnmi/oc/ocpath" + "github.com/openconfig/ygnmi/ygnmi" + "github.com/openconfig/ygot/ygot" + + valpb "github.com/openconfig/lemming/bgp/tests/proto/policyval" +) + +// Settings for configuring the baseline testbed with the test +// topology. +// +// The testbed consists of dut:port1 -> dut2:port1 +// +// - dut:port1 -> dut2:port1 subnet 192.0.2.0/30 +const ( + ipv4PrefixLen = 24 + + awaitTimeout = 30 * time.Second + rejectTimeout = 20 * time.Second +) + +var ( + BGPPath = ocpath.Root().NetworkInstance(fakedevice.DefaultNetworkInstance).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, fakedevice.BGPRoutingProtocol).Bgp() + RoutingPolicyPath = ocpath.Root().RoutingPolicy() +) + +type Device struct { + *ondatra.DUTDevice + AS uint32 + RouterID string +} + +// PolicyTestCase contains the specifications for a single policy test. +// +// Topology: +// +// DUT1 (AS 64500) -> DUT2 (AS 64500) -> DUT3 (AS 64501) +// ^ +// | +// DUT4 (AS 64502) -> DUT5 (AS 64500) +// +// Additionally, DUT0 is present as a neighbour for DUT1, DUT4, and DUT5 +// to allow a static route to be resolvable. +// +// Currently by convention, all policies are installed on DUT1 (export), DUT5 +// (export), and DUT2 (import). This is because GoBGP only withdraws routes on +// import policy change after a soft reset: +// https://github.com/osrg/gobgp/blob/master/docs/sources/policy.md#policy-and-soft-reset +type PolicyTestCase struct { + Spec *valpb.PolicyTestCase + InstallPolicies func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *Device) +} + +// TestPolicy is the helper policy integration tests can call to instantiate +// policy tests. +func TestPolicy(t *testing.T, testspec PolicyTestCase) { + t.Helper() + t.Run("installPolicyBeforeRoutes", func(t *testing.T) { + testPolicyAux(t, testspec, false) + }) + + t.Run("installPolicyAfterRoutes", func(t *testing.T) { + // TODO(wenbli): Figure out how to reset and unskip this. + testPolicyAux(t, testspec, true) + }) +} + +var ( + dut0Ports = map[string]*attrs.Attributes{ + "port1": { + Desc: "dut0Port1", + IPv4: "192.0.1.0", + IPv4Len: ipv4PrefixLen, + }, + "port2": { + Desc: "dut0Port2", + IPv4: "192.0.4.0", + IPv4Len: ipv4PrefixLen, + }, + "port3": { + Desc: "dut0Port3", + IPv4: "192.0.5.0", + IPv4Len: ipv4PrefixLen, + }, + } + + dut1Ports = map[string]*attrs.Attributes{ + "port0": { + Desc: "dut1Port1", + IPv4: "192.0.1.1", + IPv4Len: ipv4PrefixLen, + }, + "port1": { + Desc: "dut1Port1", + IPv4: "192.1.0.1", + IPv4Len: ipv4PrefixLen, + }, + } + + dut2Ports = map[string]*attrs.Attributes{ + "port1": { + Desc: "dut2Port1", + IPv4: "192.1.0.2", + IPv4Len: ipv4PrefixLen, + }, + "port2": { + Desc: "dut2Port2", + IPv4: "192.2.0.2", + IPv4Len: ipv4PrefixLen, + }, + "port3": { + Desc: "dut2Port3", + IPv4: "192.3.0.2", + IPv4Len: ipv4PrefixLen, + }, + } + + dut3Ports = map[string]*attrs.Attributes{ + "port1": { + Desc: "dut3Port1", + IPv4: "192.2.0.3", + IPv4Len: ipv4PrefixLen, + }, + } + + dut4Ports = map[string]*attrs.Attributes{ + "port0": { + Desc: "dut1Port1", + IPv4: "192.0.4.4", + IPv4Len: ipv4PrefixLen, + }, + "port1": { + Desc: "dut4Port1", + IPv4: "192.4.0.4", + IPv4Len: ipv4PrefixLen, + }, + } + + dut5Ports = map[string]*attrs.Attributes{ + "port0": { + Desc: "dut1Port1", + IPv4: "192.0.5.5", + IPv4Len: ipv4PrefixLen, + }, + "port1": { + Desc: "dut5Port1", + IPv4: "192.4.0.5", + IPv4Len: ipv4PrefixLen, + }, + "port2": { + Desc: "dut5Port2", + IPv4: "192.3.0.5", + IPv4Len: ipv4PrefixLen, + }, + } +) + +func testPropagation(t *testing.T, routeTest *valpb.RouteTestCase, pair1, pair2 devicePair, filterPoliciesInstalled bool) { + t.Helper() + v4uni := BGPPath.Rib().AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).Ipv4Unicast() + expectedResult := routeTest.GetExpectedResultBeforePolicy() + if filterPoliciesInstalled { + expectedResult = routeTest.GetExpectedResult() + } + + prevDUT, currDUT, nextDUT := pair1.first, pair1.second, pair2.second + port1, port21, port23, port3 := pair1.firstPort, pair1.secondPort, pair2.firstPort, pair2.secondPort + + prefix := routeTest.GetInput().GetReachPrefix() + // Check propagation to AdjRibOutPre for all prefixes. + gnmi.Await(t, prevDUT, v4uni.Neighbor(port21.IPv4).AdjRibOutPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, prevDUT, v4uni.Neighbor(port21.IPv4).AdjRibOutPost().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, currDUT, v4uni.Neighbor(port1.IPv4).AdjRibInPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + switch expectedResult { + case policyval.RouteTestResult_ROUTE_TEST_RESULT_ACCEPT: + t.Logf("Waiting for %s to be propagated", prefix) + gnmi.Await(t, currDUT, v4uni.Neighbor(port1.IPv4).AdjRibInPost().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, currDUT, v4uni.LocRib().Route(prefix, oc.UnionString(port1.IPv4), 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, currDUT, v4uni.Neighbor(port3.IPv4).AdjRibOutPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, currDUT, v4uni.Neighbor(port3.IPv4).AdjRibOutPost().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, nextDUT, v4uni.Neighbor(port23.IPv4).AdjRibInPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + case policyval.RouteTestResult_ROUTE_TEST_RESULT_DISCARD: + w := gnmi.Watch(t, currDUT, v4uni.Neighbor(port1.IPv4).AdjRibInPost().Route(prefix, 0).Prefix().State(), rejectTimeout, func(val *ygnmi.Value[string]) bool { + _, ok := val.Val() + return !ok + }) + if _, ok := w.Await(t); !ok { + t.Errorf("prefix %q (%s) was not rejected from adj-rib-in-post of %v (neighbour %v) within timeout.", prefix, routeTest.GetDescription(), currDUT, prevDUT) + break + } + t.Logf("prefix %q (%s) was successfully rejected from adj-rib-in-post of %v (neighbour %v) within timeout.", prefix, routeTest.GetDescription(), currDUT, prevDUT) + + // Test withdrawal in the case of InstallPolicyAfterRoutes. + w = gnmi.Watch(t, nextDUT, v4uni.Neighbor(port23.IPv4).AdjRibInPre().Route(prefix, 0).Prefix().State(), rejectTimeout, func(val *ygnmi.Value[string]) bool { + _, ok := val.Val() + return !ok + }) + if _, ok := w.Await(t); !ok { + t.Errorf("prefix %q (%s) was not rejected from adj-rib-in-pre of %v (neighbour %v) within timeout.", prefix, routeTest.GetDescription(), nextDUT, currDUT) + break + } + t.Logf("prefix %q (%s) was successfully rejected from adj-rib-in-pre of %v (neighbour %v) within timeout.", prefix, routeTest.GetDescription(), nextDUT, currDUT) + case policyval.RouteTestResult_ROUTE_TEST_RESULT_NOT_PREFERRED: + gnmi.Await(t, currDUT, v4uni.Neighbor(port1.IPv4).AdjRibInPost().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + w := gnmi.Watch(t, currDUT, v4uni.LocRib().Route(prefix, oc.UnionString(port1.IPv4), 0).Prefix().State(), rejectTimeout, func(val *ygnmi.Value[string]) bool { + _, ok := val.Val() + return !ok + }) + if _, ok := w.Await(t); !ok { + t.Errorf("prefix %q with origin %q (%s) was selected into loc-rib of %v.", prefix, prevDUT, routeTest.GetDescription(), currDUT) + break + } + t.Logf("prefix %q with origin %q (%s) was successfully not selected into loc-rib of %v within timeout.", prefix, prevDUT, routeTest.GetDescription(), currDUT) + + gnmi.Await(t, currDUT, v4uni.Neighbor(port3.IPv4).AdjRibOutPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, currDUT, v4uni.Neighbor(port3.IPv4).AdjRibOutPost().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + gnmi.Await(t, nextDUT, v4uni.Neighbor(port23.IPv4).AdjRibInPre().Route(prefix, 0).Prefix().State(), awaitTimeout, prefix) + default: + t.Fatalf("Invalid or unhandled policy result: %v", expectedResult) + } +} + +// configureDUT configures the ports on the DUT. +func configureDUT(t *testing.T, dut *ondatra.DUTDevice, ports map[string]*attrs.Attributes) { + for portName, attr := range ports { + p := dut.Port(t, portName) + gnmi.Replace(t, dut, gnmi.OC().Interface(p.Name()).Config(), attr.NewOCInterface(p.Name(), dut)) + gnmi.Await(t, dut, gnmi.OC().Interface(p.Name()).Subinterface(0).Ipv4().Address(attr.IPv4).Ip().State(), awaitTimeout, attr.IPv4) + } +} + +type devicePair struct { + first *Device + second *Device + firstPort *attrs.Attributes + secondPort *attrs.Attributes +} + +func bgpWithNbr(as uint32, routerID string, nbr *oc.NetworkInstance_Protocol_Bgp_Neighbor) *oc.NetworkInstance_Protocol_Bgp { + bgp := &oc.NetworkInstance_Protocol_Bgp{} + bgp.GetOrCreateGlobal().As = ygot.Uint32(as) + if routerID != "" { + bgp.Global.RouterId = ygot.String(routerID) + } + bgp.AppendNeighbor(nbr) + return bgp +} + +func installStaticRoute(t *testing.T, dut *Device, route *oc.NetworkInstance_Protocol_Static) { + staticp := gnmi.OC().NetworkInstance(fakedevice.DefaultNetworkInstance). + Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_STATIC, fakedevice.StaticRoutingProtocol) + gnmi.Replace(t, dut, staticp.Static(*route.Prefix).Config(), route) + gnmi.Await(t, dut, staticp.Static(*route.Prefix).State(), 30*time.Second, route) +} + +func awaitSessionEstablished(t *testing.T, dutPair devicePair) { + t.Helper() + dut1, dut2 := dutPair.first, dutPair.second + port1, port2 := dutPair.firstPort, dutPair.secondPort + gnmi.Await(t, dut1, BGPPath.Neighbor(port2.IPv4).SessionState().State(), awaitTimeout, oc.Bgp_Neighbor_SessionState_ESTABLISHED) + gnmi.Await(t, dut2, BGPPath.Neighbor(port1.IPv4).SessionState().State(), awaitTimeout, oc.Bgp_Neighbor_SessionState_ESTABLISHED) +} + +func establishSessionPairs(t *testing.T, dutPairs ...devicePair) { + t.Helper() + for _, pair := range dutPairs { + dut1, dut2 := pair.first, pair.second + port1, port2 := pair.firstPort, pair.secondPort + dutConf := bgpWithNbr(dut1.AS, dut1.RouterID, &oc.NetworkInstance_Protocol_Bgp_Neighbor{ + PeerAs: ygot.Uint32(dut2.AS), + NeighborAddress: ygot.String(port2.IPv4), + Transport: &oc.NetworkInstance_Protocol_Bgp_Neighbor_Transport{ + LocalAddress: ygot.String(port1.IPv4), + }, + }) + dut2Conf := bgpWithNbr(dut2.AS, dut2.RouterID, &oc.NetworkInstance_Protocol_Bgp_Neighbor{ + PeerAs: ygot.Uint32(dut1.AS), + NeighborAddress: ygot.String(port1.IPv4), + Transport: &oc.NetworkInstance_Protocol_Bgp_Neighbor_Transport{ + LocalAddress: ygot.String(port2.IPv4), + }, + }) + gnmi.Update(t, dut1, BGPPath.Config(), dutConf) + gnmi.Update(t, dut2, BGPPath.Config(), dut2Conf) + } + + for _, pair := range dutPairs { + awaitSessionEstablished(t, pair) + } +} + +func installDefaultAllowPolicies(t *testing.T, dutPair devicePair) { + t.Helper() + dut1, dut2 := dutPair.first, dutPair.second + port1, port2 := dutPair.firstPort, dutPair.secondPort + gnmi.Replace(t, dut1, BGPPath.Neighbor(port2.IPv4).ApplyPolicy().DefaultExportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE) + gnmi.Replace(t, dut2, BGPPath.Neighbor(port1.IPv4).ApplyPolicy().DefaultImportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE) +} + +func testPolicyAux(t *testing.T, testspec PolicyTestCase, installPolicyAfterRoutes bool) { + dut0 := ondatra.DUT(t, "dut0") + dut1 := &Device{ + DUTDevice: ondatra.DUT(t, "dut1"), + AS: 64500, + RouterID: dut1Ports["port1"].IPv4, + } + dut2 := &Device{ + DUTDevice: ondatra.DUT(t, "dut2"), + AS: 64500, + RouterID: dut2Ports["port1"].IPv4, + } + dut3 := &Device{ + DUTDevice: ondatra.DUT(t, "dut3"), + AS: 64501, + RouterID: dut3Ports["port1"].IPv4, + } + dut4 := &Device{ + DUTDevice: ondatra.DUT(t, "dut4"), + AS: 64502, + RouterID: dut4Ports["port1"].IPv4, + } + dut5 := &Device{ + DUTDevice: ondatra.DUT(t, "dut5"), + AS: 64500, + RouterID: dut5Ports["port1"].IPv4, + } + configureDUT(t, dut0, dut0Ports) + configureDUT(t, dut1.DUTDevice, dut1Ports) + configureDUT(t, dut2.DUTDevice, dut2Ports) + configureDUT(t, dut3.DUTDevice, dut3Ports) + configureDUT(t, dut4.DUTDevice, dut4Ports) + configureDUT(t, dut5.DUTDevice, dut5Ports) + + // Remove any existing BGP config + gnmi.Delete(t, dut1, BGPPath.Config()) + gnmi.Delete(t, dut2, BGPPath.Config()) + gnmi.Delete(t, dut3, BGPPath.Config()) + gnmi.Delete(t, dut4, BGPPath.Config()) + gnmi.Delete(t, dut5, BGPPath.Config()) + gnmi.Delete(t, dut1, RoutingPolicyPath.Config()) + gnmi.Delete(t, dut2, RoutingPolicyPath.Config()) + gnmi.Delete(t, dut3, RoutingPolicyPath.Config()) + gnmi.Delete(t, dut4, RoutingPolicyPath.Config()) + gnmi.Delete(t, dut5, RoutingPolicyPath.Config()) + + pair12 := devicePair{dut1, dut2, dut1Ports["port1"], dut2Ports["port1"]} + pair23 := devicePair{dut2, dut3, dut2Ports["port2"], dut3Ports["port1"]} + pair45 := devicePair{dut4, dut5, dut4Ports["port1"], dut5Ports["port1"]} + pair52 := devicePair{dut5, dut2, dut5Ports["port2"], dut2Ports["port3"]} + + // Clear the path for routes to be propagated. + // DUT1 -> DUT2 -> DUT3 + installDefaultAllowPolicies(t, pair12) + installDefaultAllowPolicies(t, pair23) + // This is an alternate source of routes towards DUT2 and thereby DUT3. + // Note that this path is longer than the above path: + // DUT4 -> DUT5 -> DUT2 (-> DUT3) + installDefaultAllowPolicies(t, pair45) + installDefaultAllowPolicies(t, pair52) + + if testspec.InstallPolicies != nil && !installPolicyAfterRoutes { + testspec.InstallPolicies(t, dut1, dut2, dut3, dut4, dut5) + } + + establishSessionPairs(t, pair12, pair23, pair45, pair52) + + for _, routeTest := range testspec.Spec.RouteTests { + // Install all regular test routes into DUT1. + route := &oc.NetworkInstance_Protocol_Static{ + Prefix: ygot.String(routeTest.GetInput().GetReachPrefix()), + NextHop: map[string]*oc.NetworkInstance_Protocol_Static_NextHop{ + "single": { + Index: ygot.String("single"), + NextHop: oc.UnionString(dut1Ports["port0"].IPv4), + Recurse: ygot.Bool(true), + }, + }, + } + installStaticRoute(t, dut1, route) + } + + for _, routeTest := range testspec.Spec.LongerPathRouteTests { + // Install all longer-path test routes into DUT4. + route := &oc.NetworkInstance_Protocol_Static{ + Prefix: ygot.String(routeTest.GetInput().GetReachPrefix()), + NextHop: map[string]*oc.NetworkInstance_Protocol_Static_NextHop{ + "single": { + Index: ygot.String("single"), + NextHop: oc.UnionString(dut4Ports["port0"].IPv4), + Recurse: ygot.Bool(true), + }, + }, + } + installStaticRoute(t, dut4, route) + } + + for _, routeTest := range testspec.Spec.AlternatePathRouteTests { + // Install all alternate-path test routes into DUT5. + route := &oc.NetworkInstance_Protocol_Static{ + Prefix: ygot.String(routeTest.GetInput().GetReachPrefix()), + NextHop: map[string]*oc.NetworkInstance_Protocol_Static_NextHop{ + "single": { + Index: ygot.String("single"), + NextHop: oc.UnionString(dut5Ports["port0"].IPv4), + Recurse: ygot.Bool(true), + }, + }, + } + installStaticRoute(t, dut5, route) + } + + for _, routeTest := range testspec.Spec.RouteTests { + testPropagation(t, routeTest, pair12, pair23, !installPolicyAfterRoutes) + } + for _, routeTest := range testspec.Spec.LongerPathRouteTests { + testPropagation(t, routeTest, pair52, pair23, !installPolicyAfterRoutes) + } + + if installPolicyAfterRoutes { + if testspec.InstallPolicies != nil { + testspec.InstallPolicies(t, dut1, dut2, dut3, dut4, dut5) + } + // Changing policy currently causes a hard reset of the BGP + // session, which causes routes to disappear from the AdjRIBs, + // so we need to wait for re-establishment first. To do this, + // we sleep for a reasonable amount of time for the sessions to + // be teared down. + time.Sleep(10 * time.Second) + awaitSessionEstablished(t, pair12) + awaitSessionEstablished(t, pair52) + + for _, routeTest := range testspec.Spec.RouteTests { + testPropagation(t, routeTest, pair12, pair23, true) + } + for _, routeTest := range testspec.Spec.LongerPathRouteTests { + testPropagation(t, routeTest, pair52, pair23, true) + } + } +} From 052b6f7f2319acf798358ae5a315747fa2837fec Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 08:04:15 -0700 Subject: [PATCH 02/15] Fix lint --- .../dut_policy_tests/prefix_set/prefix_set_test.go | 4 ++-- policytest/policytest.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go index 0459c61f..861c19ee 100644 --- a/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go +++ b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go @@ -144,7 +144,7 @@ func TestPrefixSet(t *testing.T) { } t.Run("ANY", func(t *testing.T) { - policytest.TestPolicy(t, policytest.PolicyTestCase{ + policytest.TestPolicy(t, policytest.TestCase{ Spec: getspec(false), InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { installPolicies(t, dut1, dut2, dut3, dut4, dut5, false) @@ -152,7 +152,7 @@ func TestPrefixSet(t *testing.T) { }) }) t.Run("INVERT", func(t *testing.T) { - policytest.TestPolicy(t, policytest.PolicyTestCase{ + policytest.TestPolicy(t, policytest.TestCase{ Spec: getspec(true), InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { installPolicies(t, dut1, dut2, dut3, dut4, dut5, true) diff --git a/policytest/policytest.go b/policytest/policytest.go index c4169c4b..a6c8fbf8 100644 --- a/policytest/policytest.go +++ b/policytest/policytest.go @@ -55,7 +55,7 @@ type Device struct { RouterID string } -// PolicyTestCase contains the specifications for a single policy test. +// TestCase contains the specifications for a single policy test. // // Topology: // @@ -71,14 +71,14 @@ type Device struct { // (export), and DUT2 (import). This is because GoBGP only withdraws routes on // import policy change after a soft reset: // https://github.com/osrg/gobgp/blob/master/docs/sources/policy.md#policy-and-soft-reset -type PolicyTestCase struct { +type TestCase struct { Spec *valpb.PolicyTestCase InstallPolicies func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *Device) } // TestPolicy is the helper policy integration tests can call to instantiate // policy tests. -func TestPolicy(t *testing.T, testspec PolicyTestCase) { +func TestPolicy(t *testing.T, testspec TestCase) { t.Helper() t.Run("installPolicyBeforeRoutes", func(t *testing.T) { testPolicyAux(t, testspec, false) @@ -322,7 +322,7 @@ func installDefaultAllowPolicies(t *testing.T, dutPair devicePair) { gnmi.Replace(t, dut2, BGPPath.Neighbor(port1.IPv4).ApplyPolicy().DefaultImportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE) } -func testPolicyAux(t *testing.T, testspec PolicyTestCase, installPolicyAfterRoutes bool) { +func testPolicyAux(t *testing.T, testspec TestCase, installPolicyAfterRoutes bool) { dut0 := ondatra.DUT(t, "dut0") dut1 := &Device{ DUTDevice: ondatra.DUT(t, "dut1"), From 91a9b8cbe8c5346cd02f677136badbf778e7ace7 Mon Sep 17 00:00:00 2001 From: wenovus Date: Thu, 31 Aug 2023 19:37:21 -0700 Subject: [PATCH 03/15] Move unix.Now() calls closer to GnmiUpdate call I suspect the reason for the "update is stale" is due to a race condition between different calls to gNMI.Set that each may provide a different unix.Now() value depending on when it is executed prior to updating the cache. I believe the correct fix is to actually to ignore it. WIP -- isn't Set synchronous? If so how could this happen? clue: is it always happening on default values? --- gnmi/gnmi.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/gnmi/gnmi.go b/gnmi/gnmi.go index ae573d46..ab5c2652 100644 --- a/gnmi/gnmi.go +++ b/gnmi/gnmi.go @@ -127,7 +127,7 @@ func newServer(ctx context.Context, targetName string, enableSet bool, recs ...r if err := ygot.PruneConfigFalse(configSchema.RootSchema(), configSchema.Root); err != nil { return nil, fmt.Errorf("gnmi: %v", err) } - if err := updateCache(c.cache, configSchema.Root, emptySchema.Root, targetName, OpenConfigOrigin, true, time.Now().UnixNano(), "", nil); err != nil { + if err := updateCache(c.cache, configSchema.Root, emptySchema.Root, targetName, OpenConfigOrigin, true, 0, "", nil); err != nil { return nil, fmt.Errorf("gnmi newServer: %v", err) } } @@ -140,7 +140,7 @@ func newServer(ctx context.Context, targetName string, enableSet bool, recs ...r if err := setupSchema(stateSchema, false); err != nil { return nil, err } - if err := updateCache(c.cache, stateSchema.Root, emptySchema.Root, targetName, OpenConfigOrigin, true, time.Now().UnixNano(), "", nil); err != nil { + if err := updateCache(c.cache, stateSchema.Root, emptySchema.Root, targetName, OpenConfigOrigin, true, 0, "", nil); err != nil { return nil, fmt.Errorf("gnmi newServer: %v", err) } } @@ -193,6 +193,9 @@ func setupSchema(schema *ytypes.Schema, config bool) error { func updateCache(cache *cache.Cache, dirtyRoot, root ygot.GoStruct, target, origin string, preferShadowPath bool, timestamp int64, user string, auth PathAuth) error { var nos []*gpb.Notification if root == nil { + if timestamp == 0 { + timestamp = time.Now().UnixNano() + } var err error if nos, err = ygot.TogNMINotifications(dirtyRoot, timestamp, ygot.GNMINotificationsConfig{ UsePathElem: true, @@ -200,14 +203,17 @@ func updateCache(cache *cache.Cache, dirtyRoot, root ygot.GoStruct, target, orig return fmt.Errorf("gnmi: %v", err) } } else { - ns, err := ygot.DiffWithAtomic(root, dirtyRoot, &ygot.DiffPathOpt{PreferShadowPath: preferShadowPath}) - if err != nil { + var err error + if nos, err = ygot.DiffWithAtomic(root, dirtyRoot, &ygot.DiffPathOpt{PreferShadowPath: preferShadowPath}); err != nil { return fmt.Errorf("gnmi: error while creating update notification for Set: %v", err) } - for _, n := range ns { + if timestamp == 0 { + // Set timestamp here in order to minimize latency and reduce change for "update is stale" error. + timestamp = time.Now().UnixNano() + } + for _, n := range nos { n.Timestamp = timestamp } - nos = append(nos, ns...) } if auth != nil && auth.IsInitialized() { @@ -338,7 +344,7 @@ func unmarshalSetRequest(schema *ytypes.Schema, req *gpb.SetRequest, preferShado // set returns a gRPC error with the correct code and shouldn't be wrapped again. // // - timestamp specifies the timestamp of the values that are to be updated in -// the gNMI cache. +// the gNMI cache. If zero, then time.Now().UnixNano() is used. // - auth adds authorization to before writing vals to the cache, if set to nil, not authorization is checked. func set(schema *ytypes.Schema, cache *cache.Cache, target string, req *gpb.SetRequest, preferShadowPath bool, validators []func(*oc.Root) error, timestamp int64, user string, auth PathAuth) error { // skip diffing and deepcopy for performance when handling state update paths. @@ -356,6 +362,9 @@ func set(schema *ytypes.Schema, cache *cache.Cache, target string, req *gpb.SetR Unmarshal: schema.Unmarshal, } unmarshalSetRequest(tempSchema, req, preferShadowPath) + if timestamp == 0 { + timestamp = time.Now().UnixNano() + } notifs, err := ygot.TogNMINotifications(tempSchema.Root, timestamp, ygot.GNMINotificationsConfig{UsePathElem: true}) if err != nil { return err @@ -476,7 +485,7 @@ const ( // is to support cases where the data comes from an externally-timestamped // source. func (s *Server) Set(ctx context.Context, req *gpb.SetRequest) (*gpb.SetResponse, error) { - timestamp := time.Now().UnixNano() + var timestamp int64 // Use ConfigMode by default so that external users don't need to set metadata. gnmiMode := ConfigMode md, ok := metadata.FromIncomingContext(ctx) From 31cf72347b5dfdff6b3d0098230b6e1df4c3cac3 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 11:49:19 -0700 Subject: [PATCH 04/15] Trigger Build to test flakiness From ea36eb741a50fc1d9158ab18d78fb452b1e5dd1e Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 11:51:10 -0700 Subject: [PATCH 05/15] Trigger Build to test flakiness From 32b76d04eb2986d02853fa8995cfe97c647ff23f Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 11:53:23 -0700 Subject: [PATCH 06/15] Trigger Build to test flakiness From 8bd868beea45209b1c235c13d6c5f01dc42e011f Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:16:34 -0700 Subject: [PATCH 07/15] Skip Initial Bad packets in BGP-triggered GUE test --- .../bgp_triggered_gue/bgp_triggered_gue_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go b/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go index 9e6bb752..04748bce 100644 --- a/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go +++ b/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go @@ -520,6 +520,12 @@ func testTrafficAndEncap(t *testing.T, otg *otg.OTG, startingIP string, v6Traffi var expectedPacketCounter int for i := 0; i != 20; i++ { + if expectedPacketCounter == 1 { + // Reset packet index to avoid counting other packet + // types that may be initially in the stream prior to + // the first good packet. + i = 1 + } pkt, err := ps.NextPacket() if err != nil { t.Fatalf("error reading next packet: %v", err) From be90e1a413b5a6f8b97411c72ab76097c46c7b09 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:19:47 -0700 Subject: [PATCH 08/15] Trigger Build to test flakiness From 1a3b1c96777d353d281bbb557be7df346c2401c1 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:21:40 -0700 Subject: [PATCH 09/15] Trigger Build to test flakiness From 84657dd4477862cdd5217cb8431d95f3d17b66c5 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:22:37 -0700 Subject: [PATCH 10/15] Trigger Build to test flakiness From 5922deb28689ad68b62156409fc8e222f67a216e Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:23:32 -0700 Subject: [PATCH 11/15] Trigger Build to test flakiness From 8ec8caa0fcf158b9519b8886f45ed570feb65466 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 12:58:59 -0700 Subject: [PATCH 12/15] Improve logic --- .../bgp_triggered_gue_test.go | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go b/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go index 04748bce..8ae02d09 100644 --- a/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go +++ b/integration_tests/twodut_oneotg_tests/bgp_triggered_gue/bgp_triggered_gue_test.go @@ -519,17 +519,13 @@ func testTrafficAndEncap(t *testing.T, otg *otg.OTG, startingIP string, v6Traffi } var expectedPacketCounter int - for i := 0; i != 20; i++ { - if expectedPacketCounter == 1 { - // Reset packet index to avoid counting other packet - // types that may be initially in the stream prior to - // the first good packet. - i = 1 - } + const wantPacketN = 20 + for i := 0; i != wantPacketN; i++ { pkt, err := ps.NextPacket() if err != nil { t.Fatalf("error reading next packet: %v", err) } + if encapFields == nil { nl := pkt.NetworkLayer() if nl == nil { @@ -543,8 +539,7 @@ func testTrafficAndEncap(t *testing.T, otg *otg.OTG, startingIP string, v6Traffi // TODO(wenbli): What should NextHeader be? if diff := cmp.Diff(wantNL, gotNL, cmpIgnoreOptsNL...); diff != "" { t.Logf("Got unexpected network layer (-want, +got):\n%s\n%s", diff, pkt.Dump()) - } else { - expectedPacketCounter += 1 + continue } } else { nl := pkt.NetworkLayer() @@ -578,15 +573,22 @@ func testTrafficAndEncap(t *testing.T, otg *otg.OTG, startingIP string, v6Traffi } if diff := cmp.Diff(wantTL, gotTL, cmpIgnoreOptsTL...); diff != "" { t.Errorf("Got unexpected transport layer (-want, +got):\n%s\n%s", diff, pkt.Dump()) - } else { - expectedPacketCounter += 1 + continue } // TODO: Check that lower layers is the original packet. } + + if expectedPacketCounter == 0 { + // Reset packet index to avoid counting other packet + // types that may be initially in the stream prior to + // the first good packet. + i = 0 + } + expectedPacketCounter++ } - if expectedPacketCounter < 10 { - t.Errorf("Got less than 10 expected packets: %v", expectedPacketCounter) + if expectedPacketCounter < wantPacketN { + t.Errorf("Got less than %d expected packets: %v", wantPacketN, expectedPacketCounter) } else { t.Logf("Got %d expected packets.", expectedPacketCounter) } From 66f6dbb1876a6524bf832eb20de0262adfeea786 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 13:08:01 -0700 Subject: [PATCH 13/15] Trigger Build to test flakiness From 114878b82e05d4045848ab77cf63abbd62d4f7ad Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 13:08:08 -0700 Subject: [PATCH 14/15] Trigger Build to test flakiness From e4baf5db602e588c017aa8ce20f8f81f186398c9 Mon Sep 17 00:00:00 2001 From: wenovus Date: Fri, 1 Sep 2023 14:35:59 -0700 Subject: [PATCH 15/15] Fix --- .../prefix_set/prefix_set_test.go | 15 +++-- policytest/policytest.go | 62 +++++++++---------- 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go index 861c19ee..bf2c2bfc 100644 --- a/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go +++ b/integration_tests/dut_policy_tests/prefix_set/prefix_set_test.go @@ -33,8 +33,11 @@ func TestMain(m *testing.M) { } func TestPrefixSet(t *testing.T) { - installPolicies := func(t *testing.T, dut1, dut2, _, _, _ *policytest.Device, invert bool) { + installPolicies := func(t *testing.T, pair12, pair52, pair23 *policytest.DevicePair, invert bool) { t.Log("Installing test policies") + dut2 := pair12.Second + port1 := pair12.FirstPort + prefix1 := "10.33.0.0/16" prefix2 := "10.34.0.0/16" @@ -63,7 +66,7 @@ func TestPrefixSet(t *testing.T) { stmt.GetOrCreateActions().SetPolicyResult(oc.RoutingPolicy_PolicyResultType_REJECT_ROUTE) // Install policy gnmi.Replace(t, dut2, policytest.RoutingPolicyPath.PolicyDefinition(policyName).Config(), &oc.RoutingPolicy_PolicyDefinition{Statement: policy}) - gnmi.Replace(t, dut2, policytest.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().ImportPolicy().Config(), []string{policyName}) + gnmi.Replace(t, dut2, policytest.BGPPath.Neighbor(port1.IPv4).ApplyPolicy().ImportPolicy().Config(), []string{policyName}) } invertResult := func(result valpb.RouteTestResult, invert bool) valpb.RouteTestResult { @@ -146,16 +149,16 @@ func TestPrefixSet(t *testing.T) { t.Run("ANY", func(t *testing.T) { policytest.TestPolicy(t, policytest.TestCase{ Spec: getspec(false), - InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { - installPolicies(t, dut1, dut2, dut3, dut4, dut5, false) + InstallPolicies: func(t *testing.T, pair12, pair52, pair23 *policytest.DevicePair) { + installPolicies(t, pair12, pair52, pair23, false) }, }) }) t.Run("INVERT", func(t *testing.T) { policytest.TestPolicy(t, policytest.TestCase{ Spec: getspec(true), - InstallPolicies: func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *policytest.Device) { - installPolicies(t, dut1, dut2, dut3, dut4, dut5, true) + InstallPolicies: func(t *testing.T, pair12, pair52, pair23 *policytest.DevicePair) { + installPolicies(t, pair12, pair52, pair23, true) }, }) }) diff --git a/policytest/policytest.go b/policytest/policytest.go index a6c8fbf8..f0449633 100644 --- a/policytest/policytest.go +++ b/policytest/policytest.go @@ -55,6 +55,13 @@ type Device struct { RouterID string } +type DevicePair struct { + First *Device + Second *Device + FirstPort *attrs.Attributes + SecondPort *attrs.Attributes +} + // TestCase contains the specifications for a single policy test. // // Topology: @@ -73,7 +80,7 @@ type Device struct { // https://github.com/osrg/gobgp/blob/master/docs/sources/policy.md#policy-and-soft-reset type TestCase struct { Spec *valpb.PolicyTestCase - InstallPolicies func(t *testing.T, dut1, dut2, dut3, dut4, dut5 *Device) + InstallPolicies func(t *testing.T, pair12, pair52, pair23 *DevicePair) } // TestPolicy is the helper policy integration tests can call to instantiate @@ -180,7 +187,7 @@ var ( } ) -func testPropagation(t *testing.T, routeTest *valpb.RouteTestCase, pair1, pair2 devicePair, filterPoliciesInstalled bool) { +func testPropagation(t *testing.T, routeTest *valpb.RouteTestCase, pair1, pair2 *DevicePair, filterPoliciesInstalled bool) { t.Helper() v4uni := BGPPath.Rib().AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).Ipv4Unicast() expectedResult := routeTest.GetExpectedResultBeforePolicy() @@ -188,8 +195,8 @@ func testPropagation(t *testing.T, routeTest *valpb.RouteTestCase, pair1, pair2 expectedResult = routeTest.GetExpectedResult() } - prevDUT, currDUT, nextDUT := pair1.first, pair1.second, pair2.second - port1, port21, port23, port3 := pair1.firstPort, pair1.secondPort, pair2.firstPort, pair2.secondPort + prevDUT, currDUT, nextDUT := pair1.First, pair1.Second, pair2.Second + port1, port21, port23, port3 := pair1.FirstPort, pair1.SecondPort, pair2.FirstPort, pair2.SecondPort prefix := routeTest.GetInput().GetReachPrefix() // Check propagation to AdjRibOutPre for all prefixes. @@ -254,13 +261,6 @@ func configureDUT(t *testing.T, dut *ondatra.DUTDevice, ports map[string]*attrs. } } -type devicePair struct { - first *Device - second *Device - firstPort *attrs.Attributes - secondPort *attrs.Attributes -} - func bgpWithNbr(as uint32, routerID string, nbr *oc.NetworkInstance_Protocol_Bgp_Neighbor) *oc.NetworkInstance_Protocol_Bgp { bgp := &oc.NetworkInstance_Protocol_Bgp{} bgp.GetOrCreateGlobal().As = ygot.Uint32(as) @@ -278,19 +278,19 @@ func installStaticRoute(t *testing.T, dut *Device, route *oc.NetworkInstance_Pro gnmi.Await(t, dut, staticp.Static(*route.Prefix).State(), 30*time.Second, route) } -func awaitSessionEstablished(t *testing.T, dutPair devicePair) { +func awaitSessionEstablished(t *testing.T, dutPair *DevicePair) { t.Helper() - dut1, dut2 := dutPair.first, dutPair.second - port1, port2 := dutPair.firstPort, dutPair.secondPort + dut1, dut2 := dutPair.First, dutPair.Second + port1, port2 := dutPair.FirstPort, dutPair.SecondPort gnmi.Await(t, dut1, BGPPath.Neighbor(port2.IPv4).SessionState().State(), awaitTimeout, oc.Bgp_Neighbor_SessionState_ESTABLISHED) gnmi.Await(t, dut2, BGPPath.Neighbor(port1.IPv4).SessionState().State(), awaitTimeout, oc.Bgp_Neighbor_SessionState_ESTABLISHED) } -func establishSessionPairs(t *testing.T, dutPairs ...devicePair) { +func establishSessionPairs(t *testing.T, dutPairs ...*DevicePair) { t.Helper() for _, pair := range dutPairs { - dut1, dut2 := pair.first, pair.second - port1, port2 := pair.firstPort, pair.secondPort + dut1, dut2 := pair.First, pair.Second + port1, port2 := pair.FirstPort, pair.SecondPort dutConf := bgpWithNbr(dut1.AS, dut1.RouterID, &oc.NetworkInstance_Protocol_Bgp_Neighbor{ PeerAs: ygot.Uint32(dut2.AS), NeighborAddress: ygot.String(port2.IPv4), @@ -314,10 +314,10 @@ func establishSessionPairs(t *testing.T, dutPairs ...devicePair) { } } -func installDefaultAllowPolicies(t *testing.T, dutPair devicePair) { +func installDefaultAllowPolicies(t *testing.T, dutPair *DevicePair) { t.Helper() - dut1, dut2 := dutPair.first, dutPair.second - port1, port2 := dutPair.firstPort, dutPair.secondPort + dut1, dut2 := dutPair.First, dutPair.Second + port1, port2 := dutPair.FirstPort, dutPair.SecondPort gnmi.Replace(t, dut1, BGPPath.Neighbor(port2.IPv4).ApplyPolicy().DefaultExportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE) gnmi.Replace(t, dut2, BGPPath.Neighbor(port1.IPv4).ApplyPolicy().DefaultImportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE) } @@ -368,10 +368,10 @@ func testPolicyAux(t *testing.T, testspec TestCase, installPolicyAfterRoutes boo gnmi.Delete(t, dut4, RoutingPolicyPath.Config()) gnmi.Delete(t, dut5, RoutingPolicyPath.Config()) - pair12 := devicePair{dut1, dut2, dut1Ports["port1"], dut2Ports["port1"]} - pair23 := devicePair{dut2, dut3, dut2Ports["port2"], dut3Ports["port1"]} - pair45 := devicePair{dut4, dut5, dut4Ports["port1"], dut5Ports["port1"]} - pair52 := devicePair{dut5, dut2, dut5Ports["port2"], dut2Ports["port3"]} + pair12 := &DevicePair{dut1, dut2, dut1Ports["port1"], dut2Ports["port1"]} + pair23 := &DevicePair{dut2, dut3, dut2Ports["port2"], dut3Ports["port1"]} + pair45 := &DevicePair{dut4, dut5, dut4Ports["port1"], dut5Ports["port1"]} + pair52 := &DevicePair{dut5, dut2, dut5Ports["port2"], dut2Ports["port3"]} // Clear the path for routes to be propagated. // DUT1 -> DUT2 -> DUT3 @@ -384,7 +384,7 @@ func testPolicyAux(t *testing.T, testspec TestCase, installPolicyAfterRoutes boo installDefaultAllowPolicies(t, pair52) if testspec.InstallPolicies != nil && !installPolicyAfterRoutes { - testspec.InstallPolicies(t, dut1, dut2, dut3, dut4, dut5) + testspec.InstallPolicies(t, pair12, pair52, pair23) } establishSessionPairs(t, pair12, pair23, pair45, pair52) @@ -443,14 +443,12 @@ func testPolicyAux(t *testing.T, testspec TestCase, installPolicyAfterRoutes boo if installPolicyAfterRoutes { if testspec.InstallPolicies != nil { - testspec.InstallPolicies(t, dut1, dut2, dut3, dut4, dut5) + testspec.InstallPolicies(t, pair12, pair52, pair23) } - // Changing policy currently causes a hard reset of the BGP - // session, which causes routes to disappear from the AdjRIBs, - // so we need to wait for re-establishment first. To do this, - // we sleep for a reasonable amount of time for the sessions to - // be teared down. - time.Sleep(10 * time.Second) + // Changing policy causes a reset of the BGP session. Wait some + // time to increase confidence that we're detecting routes from + // after the reset. + time.Sleep(5 * time.Second) awaitSessionEstablished(t, pair12) awaitSessionEstablished(t, pair52)