Merge branch 'main' into switch_exp

dvaas/BUILD.bazel

@@ -143,16 +143,12 @@ cc_library(
         "//gutil:proto",
         "//gutil:status",
         "//p4_pdpi:ir_cc_proto",
+        "//p4_pdpi/packetlib",
         "//p4_pdpi/packetlib:packetlib_cc_proto",
-        "@com_google_absl//absl/container:flat_hash_set",
+        "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
-        "@com_google_absl//absl/strings:str_format",
-        "@com_google_absl//absl/types:optional",
-        "@com_google_absl//absl/types:span",
-        "@com_google_googletest//:gtest",
-        "@com_google_protobuf//:protobuf",
         "@com_googlesource_code_re2//:re2",
     ],
 )
@@ -251,8 +247,63 @@ cc_test(
     srcs = ["test_vector_test.cc"],
     deps = [
         ":test_vector",
+        ":test_vector_cc_proto",
+        "//gutil:status_matchers",
+        "//gutil:testing",
+        "//p4_pdpi/packetlib",
+        "//p4_pdpi/packetlib:packetlib_cc_proto",
+        "@com_google_absl//absl/status",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
+cc_library(
+    name = "user_provided_packet_test_vector",
+    testonly = True,
+    srcs = ["user_provided_packet_test_vector.cc"],
+    hdrs = ["user_provided_packet_test_vector.h"],
+    deps = [
+        ":test_vector",
+        ":test_vector_cc_proto",
+        "//gutil:proto",
+        "//gutil:status",
+        "//p4_pdpi/packetlib",
+        "@com_google_absl//absl/status",
+        "@com_google_absl//absl/status:statusor",
+        "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/types:span",
+    ],
+)
+
+# go/golden-test-with-coverage
+cc_test(
+    name = "user_provided_packet_test_vector_test",
+    srcs = ["user_provided_packet_test_vector_test.cc"],
+    linkstatic = True,
+    deps = [
+        ":test_vector",
+        ":test_vector_cc_proto",
+        ":user_provided_packet_test_vector",
+        "//gutil:collections",
+        "//gutil:proto",
         "//gutil:status_matchers",
+        "//gutil:testing",
         "//p4_pdpi/packetlib:packetlib_cc_proto",
+        "@com_google_absl//absl/status",
+        "@com_google_absl//absl/status:statusor",
+        "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest_main",
     ],
 )
+
+cmd_diff_test(
+    name = "user_provided_packet_test_vector_diff_test",
+    actual_cmd = " | ".join([
+        "$(execpath :user_provided_packet_test_vector_test)",
+        # Strip unnecessary lines for golden testing.
+        "sed '1,/^\\[ RUN/d'",  # Strip everything up to a line beginning with '[ RUN'.
+        "sed '/^\\[/d'",  # Strip every line beginning with '['.
+    ]),
+    expected = "user_provided_packet_test_vector_test.expected",
+    tools = [":user_provided_packet_test_vector_test"],
+)

dvaas/arriba_test_vector_validation.cc

@@ -73,8 +73,7 @@ absl::Status ValidateAgaistArribaTestVector(
                                        params.max_packets_to_send_per_second));
 
   // Compare the switch output with expected output for each test vector.
-  return ValidateTestRuns(test_runs, params.ignored_fields_for_validation,
-                          params.ignored_metadata_for_validation,
+  return ValidateTestRuns(test_runs, params.switch_output_diff_params,
                           /*write_failures=*/[&](absl::string_view failures) {
                             return artifact_writer.AppendToTestArtifact(
                                 "test_failures.txt", failures);

dvaas/arriba_test_vector_validation.h

@@ -19,6 +19,7 @@
 
 #include "absl/container/flat_hash_set.h"
 #include "absl/status/statusor.h"
+#include "dvaas/test_run_validation.h"
 #include "dvaas/test_vector.pb.h"
 #include "p4_pdpi/p4_runtime_session.h"
 #include "thinkit/mirror_testbed.h"
@@ -27,19 +28,12 @@ namespace dvaas {
 
 // Parameters for validating a testbed against an ArribaTestVector.
 struct ArribaTestVectorValidationParams {
-  // Used to skip packet fields where model and switch are known to have
-  // different behavior, which we don't want to test. All FieldDescriptors
-  // should belong to packetlib::Packet.
-  std::vector<const google::protobuf::FieldDescriptor*>
-      ignored_fields_for_validation = {};
-
-  // Used to skip packet-in metadata where model and switch are known to have
-  // different behavior, which we don't want to test. If a packet-in metadata
-  // field name in the actual or expected packets is equal to one of the entries
-  // in `ignored_metadata_for_validation`, the field is ignored during
-  // comparison.
-  absl::flat_hash_set<std::string> ignored_metadata_for_validation = {
-      "target_egress_port",
+  // Parameters to control the comparison between the actual switch
+  // output and the expected switch output per each input packet.
+  SwitchOutputDiffParams switch_output_diff_params = {
+      // TODO: Remove when it is possible to reliably validate
+      // target egress port.
+      .ignored_packet_in_metadata = {"target_egress_port"},
   };
 
   // Max number of packets to send per second. If no rate is given, the test

dvaas/test_run_validation.cc

@@ -88,11 +88,10 @@ std::optional<pdpi::IrPacketMetadata> GetPacketInMetadataByName(
   return std::nullopt;
 }
 
-bool CompareSwitchOutputs(
-    SwitchOutput actual_output, SwitchOutput expected_output,
-    const absl::flat_hash_set<std::string>& ignored_metadata,
-    const std::vector<const google::protobuf::FieldDescriptor*>& ignored_fields,
-    MatchResultListener* listener) {
+bool CompareSwitchOutputs(SwitchOutput actual_output,
+                          SwitchOutput expected_output,
+                          const SwitchOutputDiffParams& params,
+                          MatchResultListener* listener) {
   if (actual_output.packets_size() != expected_output.packets_size()) {
     *listener << "has mismatched number of packets (actual: "
               << actual_output.packets_size()
@@ -122,7 +121,7 @@ bool CompareSwitchOutputs(
     const Packet& actual_packet = actual_output.packets(i);
     const Packet& expected_packet = expected_output.packets(i);
     MessageDifferencer differ;
-    for (auto* field : ignored_fields) differ.IgnoreField(field);
+    for (auto* field : params.ignored_fields) differ.IgnoreField(field);
     std::string diff;
     differ.ReportDifferencesToString(&diff);
     if (!differ.Compare(expected_packet.parsed(), actual_packet.parsed())) {
@@ -137,7 +136,7 @@ bool CompareSwitchOutputs(
     const PacketIn& expected_packet_in = expected_output.packet_ins(i);
 
     MessageDifferencer differ;
-    for (auto* field : ignored_fields) differ.IgnoreField(field);
+    for (auto* field : params.ignored_fields) differ.IgnoreField(field);
     std::string diff;
     differ.ReportDifferencesToString(&diff);
     if (!differ.Compare(expected_packet_in.parsed(),
@@ -150,7 +149,8 @@ bool CompareSwitchOutputs(
     // Check that received packet in has desired metadata (except for ignored
     // metadata).
     for (const auto& expected_metadata : expected_packet_in.metadata()) {
-      if (ignored_metadata.contains(expected_metadata.name())) continue;
+      if (params.ignored_packet_in_metadata.contains(expected_metadata.name()))
+        continue;
 
       std::optional<pdpi::IrPacketMetadata> actual_metadata =
           GetPacketInMetadataByName(actual_packet_in, expected_metadata.name());
@@ -172,7 +172,8 @@ bool CompareSwitchOutputs(
     // Check that received packet in does not have additional metadata (except
     // for ignored metadata).
     for (const auto& actual_metadata : actual_packet_in.metadata()) {
-      if (ignored_metadata.contains(actual_metadata.name())) continue;
+      if (params.ignored_packet_in_metadata.contains(actual_metadata.name()))
+        continue;
 
       std::optional<pdpi::IrPacketMetadata> expected_metadata =
           GetPacketInMetadataByName(expected_packet_in, actual_metadata.name());
@@ -194,17 +195,14 @@ bool CompareSwitchOutputs(
 // is acceptable, or an explanation of why it is not otherwise.
 absl::optional<std::string> CompareSwitchOutputs(
     const PacketTestVector packet_test_vector,
-    const SwitchOutput& actual_output,
-    const absl::flat_hash_set<std::string>& ignored_metadata,
-    const std::vector<const google::protobuf::FieldDescriptor*>&
-        ignored_fields) {
+    const SwitchOutput& actual_output, const SwitchOutputDiffParams& params) {
   testing::StringMatchResultListener listener;
   for (int i = 0; i < packet_test_vector.acceptable_outputs_size(); ++i) {
     const SwitchOutput& expected_output =
         packet_test_vector.acceptable_outputs(i);
     listener << "- acceptable output #" << (i + 1) << " ";
-    if (CompareSwitchOutputs(actual_output, expected_output, ignored_metadata,
-                             ignored_fields, &listener)) {
+    if (CompareSwitchOutputs(actual_output, expected_output, params,
+                             &listener)) {
       return absl::nullopt;
     }
   }
@@ -330,15 +328,11 @@ absl::StatusOr<int> ExtractTestPacketTag(const packetlib::Packet& packet) {
 }
 
 PacketTestValidationResult ValidateTestRun(
-    const PacketTestRun& test_run,
-    const absl::flat_hash_set<std::string>& ignored_packet_in_metadata,
-    const std::vector<const google::protobuf::FieldDescriptor*>&
-        ignored_fields) {
+    const PacketTestRun& test_run, const SwitchOutputDiffParams& diff_params) {
   PacketTestValidationResult result;
 
-  const absl::optional<std::string> diff =
-      CompareSwitchOutputs(test_run.test_vector(), test_run.actual_output(),
-                           ignored_packet_in_metadata, ignored_fields);
+  const absl::optional<std::string> diff = CompareSwitchOutputs(
+      test_run.test_vector(), test_run.actual_output(), diff_params);
   if (!diff.has_value()) return result;
 
   // To make the diff more digestible, we first give an abstract
@@ -357,10 +351,10 @@ PacketTestValidationResult ValidateTestRun(
 
   std::string expectation = DescribeExpectation(
       test_run.test_vector().input(), acceptable_output_characterizations);
-  if (!ignored_fields.empty()) {
+  if (!diff_params.ignored_fields.empty()) {
     absl::StrAppend(
         &expectation, "\n          (ignoring the field(s) ",
-        absl::StrJoin(ignored_fields, ",",
+        absl::StrJoin(diff_params.ignored_fields, ",",
                       [](std::string* out,
                          const google::protobuf::FieldDescriptor* field) {
                         absl::StrAppend(out, "'", field->name(), "'");
@@ -400,17 +394,14 @@ PacketTestValidationResult ValidateTestRun(
   return result;
 }
 
-absl::Status ValidateTestRuns(
-    const PacketTestRuns& test_runs,
-    std::vector<const google::protobuf::FieldDescriptor*> ignored_fields,
-    const absl::flat_hash_set<std::string>& ignored_metadata,
-    const OutputWriterFunctionType& write_failures) {
+absl::Status ValidateTestRuns(const PacketTestRuns& test_runs,
+                              const SwitchOutputDiffParams& diff_params,
+                              const OutputWriterFunctionType& write_failures) {
   LOG(INFO) << "Validating test runs";
 
   std::vector<std::string> failures;
   for (const PacketTestRun& test_run : test_runs.test_runs()) {
-    PacketTestValidationResult result =
-        ValidateTestRun(test_run, ignored_metadata, ignored_fields);
+    PacketTestValidationResult result = ValidateTestRun(test_run, diff_params);
     if (result.has_failure()) {
       failures.push_back(result.failure().description());
     }

dvaas/test_run_validation.h

@@ -27,24 +27,33 @@
 
 namespace dvaas {
 
-// Validates the given `test_vector` and returns the result.
-// Validation compares the actual output packets against the expected output
-// packets modulo `ignored_fields` and `ignored_metadata, where:
-// * `ignored_fields` must belong to packetlib::Packet, and
-// * `ignored_metadata` must be the names of Packet In metadata fields.
+// Parameters to control the comparison between the actual switch output and the
+// expected switch output per input packet.
+struct SwitchOutputDiffParams {
+  // Used to skip packet fields where model and switch are known to have
+  // different behavior, which we don't want to test. All FieldDescriptors
+  // should belong to packetlib::Packet.
+  std::vector<const google::protobuf::FieldDescriptor*> ignored_fields;
+
+  // Used to skip packet-in metadata where model and switch are known to have
+  // different behavior, which we don't want to test. If a packet-in metadata
+  // field name in the actual or expected packets is equal to one of the entries
+  // in `ignored_metadata_for_validation`, the field is ignored during
+  // comparison.
+  absl::flat_hash_set<std::string> ignored_packet_in_metadata;
+};
+
+// Validates the given `test_vector` using the parameters in `params` and
+// returns the result.
 PacketTestValidationResult ValidateTestRun(
     const PacketTestRun& test_run,
-    const absl::flat_hash_set<std::string>& ignored_packet_in_metadata = {},
-    const std::vector<const google::protobuf::FieldDescriptor*>&
-        ignored_fields = {});
+    const SwitchOutputDiffParams& diff_params = {});
 
 // Like `ValidateTestRun`, but for a collection of `test_runs`. Also
 // writes the results to a test artifact using `write_failures`.
-absl::Status ValidateTestRuns(
-    const PacketTestRuns& test_runs,
-    std::vector<const google::protobuf::FieldDescriptor*> ignored_fields,
-    const absl::flat_hash_set<std::string>& ignored_metadata,
-    const OutputWriterFunctionType& write_failures);
+absl::Status ValidateTestRuns(const PacketTestRuns& test_runs,
+                              const SwitchOutputDiffParams& diff_params,
+                              const OutputWriterFunctionType& write_failures);
 
 }  // namespace dvaas
 

dvaas/test_vector.cc

@@ -17,10 +17,14 @@
 #include <ostream>
 #include <string>
 
+#include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "absl/strings/escaping.h"
 #include "absl/strings/str_cat.h"
+#include "dvaas/test_vector.pb.h"
 #include "gutil/proto.h"
 #include "gutil/status.h"
+#include "p4_pdpi/packetlib/packetlib.h"
 #include "p4_pdpi/packetlib/packetlib.pb.h"
 #include "re2/re2.h"
 
@@ -53,4 +57,64 @@ std::ostream& operator<<(std::ostream& os, const SwitchOutput& output) {
   return os << output.DebugString();
 }
 
+absl::Status UpdateTestTag(packetlib::Packet& packet, int new_tag) {
+  // Make a new input packet with updated payload.
+  std::string new_payload = packet.payload();
+  if (!RE2::Replace(&new_payload, *kTestPacketIdRegexp,
+                    MakeTestPacketTagFromUniqueId(new_tag))) {
+    return gutil::InvalidArgumentErrorBuilder()
+           << "Test packets must contain a tag of the form '"
+           << kTestPacketIdRegexp->pattern()
+           << "' in their payload, but the given packet with payload '"
+           << packet.payload() << "' does not:\n"
+           << gutil::PrintTextProto(packet);
+  }
+  packet.set_payload(new_payload);
+  bool status;
+  ASSIGN_OR_RETURN(status, PadPacketToMinimumSize(packet),
+                   _.LogError() << "Failed to pad packet for tag: " << new_tag);
+  ASSIGN_OR_RETURN(status, UpdateAllComputedFields(packet),
+                   _.LogError()
+                       << "Failed to update payload for tag: " << new_tag);
+
+  return absl::OkStatus();
+}
+
+// Returns a serialization of the given `packet` as a hexstring.
+absl::StatusOr<std::string> SerializeAsHexString(
+    const packetlib::Packet& packet) {
+  ASSIGN_OR_RETURN(std::string serialized_packet,
+                   packetlib::RawSerializePacket(packet),
+                   _ << " where packet = " << packet.DebugString());
+  return absl::BytesToHexString(serialized_packet);
+}
+
+absl::Status UpdateTestTag(PacketTestVector& packet_test_vector, int new_tag) {
+  // Updates the payload of the SwitchInput.
+  dvaas::Packet& input_packet =
+      *packet_test_vector.mutable_input()->mutable_packet();
+  RETURN_IF_ERROR(UpdateTestTag(*input_packet.mutable_parsed(), new_tag));
+  ASSIGN_OR_RETURN(const std::string input_packet_updated_hexstr,
+                   SerializeAsHexString(input_packet.parsed()));
+  input_packet.set_hex(input_packet_updated_hexstr);
+
+  // Update the payload of the SwitchOutput.
+  for (SwitchOutput& output_packet :
+       *packet_test_vector.mutable_acceptable_outputs()) {
+    for (dvaas::Packet& packet_out : *output_packet.mutable_packets()) {
+      RETURN_IF_ERROR(UpdateTestTag(*packet_out.mutable_parsed(), new_tag));
+      ASSIGN_OR_RETURN(const std::string packet_out_updated_hexstr,
+                       SerializeAsHexString(packet_out.parsed()));
+      packet_out.set_hex(packet_out_updated_hexstr);
+    }
+    for (dvaas::PacketIn& packet_in : *output_packet.mutable_packet_ins()) {
+      RETURN_IF_ERROR(UpdateTestTag(*packet_in.mutable_parsed(), new_tag));
+      ASSIGN_OR_RETURN(const std::string packet_in_updated_hexstr,
+                       SerializeAsHexString(packet_in.parsed()));
+      packet_in.set_hex(packet_in_updated_hexstr);
+    }
+  }
+  return absl::OkStatus();
+}
+
 }  // namespace dvaas