Add test to verify invalid P4 constraint behavior & Make fake_sonic_d…

…b_table thread-safe. (sonic-net#206) Co-authored-by: rhalstea <[email protected]> Co-authored-by: kishanps <[email protected]>
rkavitha-hcl · Jun 19, 2024 · 7711691 · 7711691
1 parent 7374587
commit 7711691
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 19 deletions.
diff --git a/p4rt_app/sonic/adapters/BUILD.bazel b/p4rt_app/sonic/adapters/BUILD.bazel
@@ -169,11 +169,13 @@ cc_library(
     hdrs = ["fake_sonic_db_table.h"],
     deps = [
         "@com_github_google_glog//:glog",
+        "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/container:flat_hash_map",
         "@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/synchronization",
     ],
 )
 

diff --git a/p4rt_app/sonic/adapters/fake_sonic_db_table.cc b/p4rt_app/sonic/adapters/fake_sonic_db_table.cc
@@ -25,6 +25,7 @@
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_join.h"
+#include "absl/synchronization/mutex.h"
 #include "glog/logging.h"
 
 namespace p4rt_app {
@@ -34,6 +35,7 @@ void FakeSonicDbTable::InsertTableEntry(const std::string &key,
                                         const SonicDbEntryList &values) {
   VLOG(1) << absl::StreamFormat("'%s' insert table entry: %s",
                                 debug_table_name_, key);
+  absl::WriterMutexLock lock(&entries_mutex_);
   auto &entry = entries_[key];
   for (const auto &[field, data] : values) {
     entry.insert_or_assign(field, data);
@@ -43,6 +45,7 @@ void FakeSonicDbTable::InsertTableEntry(const std::string &key,
 void FakeSonicDbTable::DeleteTableEntry(const std::string &key) {
   VLOG(1) << absl::StreamFormat("'%s' delete table entry: %s",
                                 debug_table_name_, key);
+  absl::WriterMutexLock lock(&entries_mutex_);
   if (auto iter = entries_.find(key); iter != entries_.end()) {
     entries_.erase(iter);
   }
@@ -67,6 +70,7 @@ bool FakeSonicDbTable::PushNotification(const std::string &key) {
   }
 
   // If the key exists Insert into the StateDb, otherwise delete.
+  absl::WriterMutexLock lock(&entries_mutex_);
   auto entry_iter = entries_.find(key);
   if (entry_iter != entries_.end()) {
     InsertStateDbTableEntry(key, entry_iter->second);
@@ -125,8 +129,11 @@ absl::StatusOr<SonicDbEntryMap> FakeSonicDbTable::ReadTableEntry(
     const std::string &key) const {
   VLOG(1) << absl::StreamFormat("'%s' read table entry: %s", debug_table_name_,
                                 key);
-  if (auto entry = entries_.find(key); entry != entries_.end()) {
-    return entry->second;
+  {
+    absl::ReaderMutexLock lock(&entries_mutex_);
+    if (auto entry = entries_.find(key); entry != entries_.end()) {
+      return entry->second;
+    }
   }
   return absl::Status(absl::StatusCode::kNotFound,
                       absl::StrCat("AppDb missing: ", key));
@@ -135,15 +142,19 @@ absl::StatusOr<SonicDbEntryMap> FakeSonicDbTable::ReadTableEntry(
 std::vector<std::string> FakeSonicDbTable::GetAllKeys() const {
   std::vector<std::string> result;
   VLOG(1) << absl::StreamFormat("'%s' get all keys.", debug_table_name_);
-  for (const auto &entry : entries_) {
-    result.push_back(entry.first);
+  {
+    absl::ReaderMutexLock lock(&entries_mutex_);
+    for (const auto &entry : entries_) {
+      result.push_back(entry.first);
+    }
   }
   VLOG(2) << absl::StreamFormat("'%s' found  keys: %s", debug_table_name_,
                                 absl::StrJoin(result, ", "));
   return result;
 }
 
 void FakeSonicDbTable::DebugState() const {
+  absl::ReaderMutexLock lock(&entries_mutex_);
   for (const auto &[key, values] : entries_) {
     LOG(INFO) << "AppDb entry: " << key;
     for (const auto &[field, data] : values) {

diff --git a/p4rt_app/sonic/adapters/fake_sonic_db_table.h b/p4rt_app/sonic/adapters/fake_sonic_db_table.h
@@ -20,8 +20,10 @@
 #include <utility>
 #include <vector>
 
+#include "absl/base/thread_annotations.h"
 #include "absl/container/flat_hash_map.h"
 #include "absl/status/statusor.h"
+#include "absl/synchronization/mutex.h"
 
 namespace p4rt_app {
 namespace sonic {
@@ -41,8 +43,6 @@ using SonicDbEntryMap = std::unordered_map<std::string, std::string>;
 
 // Fakes how the OrchAgent updates AppDb tables. When an entry is inserted the
 // Orchagent will respond with a notification of success or failure.
-//
-// This class is NOT thread-safe.
 class FakeSonicDbTable {
  public:
   FakeSonicDbTable(const std::string &table_name = "SonicDb:TABLE")
@@ -54,8 +54,10 @@ class FakeSonicDbTable {
   FakeSonicDbTable(const std::string &table_name, FakeSonicDbTable *state_db)
       : debug_table_name_(table_name), state_db_(state_db) {}
 
-  void InsertTableEntry(const std::string &key, const SonicDbEntryList &values);
-  void DeleteTableEntry(const std::string &key);
+  void InsertTableEntry(const std::string &key, const SonicDbEntryList &values)
+      ABSL_LOCKS_EXCLUDED(entries_mutex_);
+  void DeleteTableEntry(const std::string &key)
+      ABSL_LOCKS_EXCLUDED(entries_mutex_);
 
   void SetResponseForKey(const std::string &key, const std::string &code,
                          const std::string &message);
@@ -68,9 +70,11 @@ class FakeSonicDbTable {
   void GetNextNotification(std::string &op, std::string &data,
                            SonicDbEntryList &values);
 
-  absl::StatusOr<SonicDbEntryMap> ReadTableEntry(const std::string &key) const;
+  absl::StatusOr<SonicDbEntryMap> ReadTableEntry(const std::string &key) const
+      ABSL_LOCKS_EXCLUDED(entries_mutex_);
 
-  std::vector<std::string> GetAllKeys() const;
+  std::vector<std::string> GetAllKeys() const
+      ABSL_LOCKS_EXCLUDED(entries_mutex_);
 
   // Method should only be used for debug purposes.
   void DebugState() const;
@@ -87,11 +91,15 @@ class FakeSonicDbTable {
 
   bool UpdateAppStateDb(const std::string &key);
 
+  // Mutex to protect synchronization of entries_.
+  mutable absl::Mutex entries_mutex_;
+
   // Debug table name is used in log messages to help distinguish messages.
-  std::string debug_table_name_;
+  const std::string debug_table_name_;
 
   // Current list of DB entries stored in the table.
-  absl::flat_hash_map<std::string, SonicDbEntryMap> entries_;
+  absl::flat_hash_map<std::string, SonicDbEntryMap> entries_
+      ABSL_GUARDED_BY(entries_mutex_);
 
   // List of notifications the OrchAgent would have generated. One notification
   // is created per insert, and one is removed per notification check.

diff --git a/p4rt_app/tests/BUILD.bazel b/p4rt_app/tests/BUILD.bazel
@@ -155,20 +155,18 @@ cc_test(
         "//p4rt_app/p4runtime:p4runtime_impl",
         "//p4rt_app/tests/lib:app_db_entry_builder",
         "//p4rt_app/tests/lib:p4runtime_grpc_service",
-        "//p4rt_app/tests/lib:p4runtime_request_helpers",
+        "//sai_p4/instantiations/google:clos_stage",
         "//sai_p4/instantiations/google:instantiations",
         "//sai_p4/instantiations/google:sai_p4info_cc",
         "//sai_p4/instantiations/google:sai_p4info_fetcher_cc",
         "@com_github_google_glog//:glog",
         "@com_github_p4lang_p4runtime//:p4info_cc_proto",
-        "@com_github_p4lang_p4runtime//:p4runtime_cc_grpc",
         "@com_github_p4lang_p4runtime//:p4runtime_cc_proto",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/time",
         "@com_google_googletest//:gtest_main",
-        "@com_google_protobuf//:protobuf",
         "@sonic_swss_common//:common",
     ],
 )

diff --git a/p4rt_app/tests/forwarding_pipeline_config_test.cc b/p4rt_app/tests/forwarding_pipeline_config_test.cc
@@ -11,22 +11,25 @@
 // 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.
+#include <cstdint>
 #include <cstdlib>
 #include <memory>
 #include <optional>
 #include <string>
+#include <tuple>
 #include <utility>
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/ascii.h"
+#include "absl/strings/match.h"
 #include "absl/strings/numbers.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
+#include "absl/time/clock.h"
 #include "absl/time/time.h"
 #include "glog/logging.h"
 #include "gmock/gmock.h"
-#include "google/protobuf/repeated_field.h"
-#include "grpcpp/client_context.h"
 #include "grpcpp/security/credentials.h"
 #include "grpcpp/support/status.h"
 #include "gtest/gtest.h"
@@ -35,13 +38,12 @@
 #include "gutil/status.h"
 #include "gutil/status_matchers.h"
 #include "p4/config/v1/p4info.pb.h"
-#include "p4/v1/p4runtime.grpc.pb.h"
 #include "p4/v1/p4runtime.pb.h"
 #include "p4_pdpi/p4_runtime_session.h"
 #include "p4rt_app/p4runtime/p4runtime_impl.h"
 #include "p4rt_app/tests/lib/app_db_entry_builder.h"
 #include "p4rt_app/tests/lib/p4runtime_grpc_service.h"
-#include "p4rt_app/tests/lib/p4runtime_request_helpers.h"
+#include "sai_p4/instantiations/google/clos_stage.h"
 #include "sai_p4/instantiations/google/instantiations.h"
 #include "sai_p4/instantiations/google/sai_p4info.h"
 #include "sai_p4/instantiations/google/sai_p4info_fetcher.h"
@@ -653,6 +655,28 @@ TEST_F(ForwardingPipelineConfigTest,
       StatusIs(absl::StatusCode::kInternal));
 }
 
+TEST_F(ForwardingPipelineConfigTest, InvalidP4ConstraintDoesNotGoCritical) {
+  constexpr absl::string_view kEntryRestrictionStr = "@entry_restriction";
+
+  // Go through all the tables an replace any P4 constraint annotation with an
+  // invalid one.
+  auto p4_info = sai::GetP4Info(sai::Instantiation::kMiddleblock);
+  for (p4::config::v1::Table& table : *p4_info.mutable_tables()) {
+    for (std::string& annon :
+         *table.mutable_preamble()->mutable_annotations()) {
+      if (absl::StartsWith(annon, kEntryRestrictionStr)) {
+        annon = absl::StrCat(kEntryRestrictionStr, "(invalid constraint)");
+      }
+    }
+  }
+
+  ASSERT_THAT(
+      pdpi::SetMetadataAndSetForwardingPipelineConfig(
+          p4rt_session_.get(),
+          SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT, p4_info),
+      StatusIs(absl::StatusCode::kInvalidArgument));
+}
+
 class PerConfigTest : public ForwardingPipelineConfigTest,
                       public testing::WithParamInterface<
                           std::tuple<sai::Instantiation, sai::ClosStage> > {};