From 985dc6721a12186b5e328382c8389cbfe1eaa868 Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Tue, 1 Sep 2026 12:30:54 -0700 Subject: [PATCH 1/4] Add nftables RAII library + unit tests --- base/cvd/allocd/net/BUILD.bazel | 37 ++++- base/cvd/allocd/net/nft_rule.cc | 74 ++++++++++ base/cvd/allocd/net/nft_rule.h | 55 ++++++++ base/cvd/allocd/net/nft_rule_test.cc | 75 ++++++++++ base/cvd/allocd/net/nftables.h | 55 ++++++++ base/cvd/allocd/net/nftables_nft.cc | 202 +++++++++++++++++++++++++++ base/cvd/allocd/net/nftables_nft.h | 59 ++++++++ base/cvd/allocd/test/BUILD.bazel | 24 ++++ base/cvd/allocd/test/fake_nftables.h | 124 ++++++++++++++++ base/cvd/allocd/test/mock_nftables.h | 54 +++++++ 10 files changed, 758 insertions(+), 1 deletion(-) create mode 100644 base/cvd/allocd/net/nft_rule.cc create mode 100644 base/cvd/allocd/net/nft_rule.h create mode 100644 base/cvd/allocd/net/nft_rule_test.cc create mode 100644 base/cvd/allocd/net/nftables.h create mode 100644 base/cvd/allocd/net/nftables_nft.cc create mode 100644 base/cvd/allocd/net/nftables_nft.h create mode 100644 base/cvd/allocd/test/BUILD.bazel create mode 100644 base/cvd/allocd/test/fake_nftables.h create mode 100644 base/cvd/allocd/test/mock_nftables.h diff --git a/base/cvd/allocd/net/BUILD.bazel b/base/cvd/allocd/net/BUILD.bazel index c189f7d9728..e60c0e21eb2 100644 --- a/base/cvd/allocd/net/BUILD.bazel +++ b/base/cvd/allocd/net/BUILD.bazel @@ -1,4 +1,4 @@ -load("//cuttlefish/bazel:rules.bzl", "cf_cc_library") +load("//cuttlefish/bazel:rules.bzl", "cf_cc_library", "cf_cc_test") package( default_visibility = ["//:android_cuttlefish"], @@ -19,3 +19,38 @@ cf_cc_library( "@abseil-cpp//absl/log", ], ) + +cf_cc_library( + name = "nftables", + srcs = [ + "nft_rule.cc", + "nftables_nft.cc", + ], + hdrs = [ + "nft_rule.h", + "nftables.h", + "nftables_nft.h", + ], + deps = [ + "//cuttlefish/common/libs/utils:files", + "//cuttlefish/common/libs/utils:json", + "//cuttlefish/process:command", + "//cuttlefish/process:managed_stdio", + "//cuttlefish/process:subprocess", + "//cuttlefish/result", + "@abseil-cpp//absl/base:no_destructor", + "@abseil-cpp//absl/log", + "@abseil-cpp//absl/strings", + "@jsoncpp", + ], +) + +cf_cc_test( + name = "nft_rule_test", + srcs = ["nft_rule_test.cc"], + deps = [ + ":nftables", + "//allocd/test:mock_nftables", + "//cuttlefish/result:result_matchers", + ], +) diff --git a/base/cvd/allocd/net/nft_rule.cc b/base/cvd/allocd/net/nft_rule.cc new file mode 100644 index 00000000000..2fbcf44515b --- /dev/null +++ b/base/cvd/allocd/net/nft_rule.cc @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#include "allocd/net/nft_rule.h" + +#include +#include +#include + +#include "absl/log/log.h" +#include "absl/strings/str_cat.h" + +#include "allocd/net/nftables.h" +#include "cuttlefish/result/result.h" + +namespace cuttlefish { +namespace { + +// Namespace prefix applied to every dynamic cvdalloc rule comment. +constexpr std::string_view kCvdallocCommentPrefix = "cvdalloc-"; + +} // namespace + +Result NftRule::Create(Nftables& nft, std::string_view family, + std::string_view table, std::string_view chain, + std::string_view content, + std::string_view tag) { + std::string comment = absl::StrCat(kCvdallocCommentPrefix, tag); + std::string full_content = + absl::StrCat(content, " comment \"", comment, "\""); + CF_EXPECT(nft.AddRule(family, table, chain, full_content)); + return NftRule(&nft, family, table, chain, std::move(comment)); +} + +NftRule::NftRule(Nftables* nft, std::string_view family, std::string_view table, + std::string_view chain, std::string comment) + : nft_(nft), + family_(family), + table_(table), + chain_(chain), + comment_(std::move(comment)) {} + +NftRule::NftRule(NftRule&& r) noexcept + : nft_(std::exchange(r.nft_, nullptr)), + family_(std::move(r.family_)), + table_(std::move(r.table_)), + chain_(std::move(r.chain_)), + comment_(std::move(r.comment_)) {} + +NftRule::~NftRule() { + // nft_ is nulled on move, so a moved-from rule is inert and never deletes. + if (nft_ != nullptr) { + auto res = nft_->DeleteRulesByComment(family_, table_, chain_, comment_); + if (!res.has_value()) { + LOG(ERROR) << "Failed to delete nft rule(s) in NftRule destructor: " + << res.error(); + } + } +} + +} // namespace cuttlefish diff --git a/base/cvd/allocd/net/nft_rule.h b/base/cvd/allocd/net/nft_rule.h new file mode 100644 index 00000000000..10257daf27f --- /dev/null +++ b/base/cvd/allocd/net/nft_rule.h @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#ifndef ALLOCD_NET_NFT_RULE_H_ +#define ALLOCD_NET_NFT_RULE_H_ + +#include +#include + +#include "allocd/net/nftables.h" +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +class NftRule { + public: + // `tag` is a caller-supplied unique identifier (e.g. an interface name). + static Result Create(Nftables& nft, std::string_view family, + std::string_view table, std::string_view chain, + std::string_view content, std::string_view tag); + + NftRule() = delete; + NftRule(Nftables* nft, std::string_view family, std::string_view table, + std::string_view chain, std::string comment); + ~NftRule(); + + NftRule(NftRule&& r) noexcept; + NftRule& operator=(NftRule&& r) = delete; + NftRule(const NftRule& r) = delete; + NftRule& operator=(const NftRule& r) = delete; + + private: + Nftables* nft_ = nullptr; + std::string family_; + std::string table_; + std::string chain_; + std::string comment_; +}; + +} // namespace cuttlefish + +#endif // ALLOCD_NET_NFT_RULE_H_ diff --git a/base/cvd/allocd/net/nft_rule_test.cc b/base/cvd/allocd/net/nft_rule_test.cc new file mode 100644 index 00000000000..89ff053eb6e --- /dev/null +++ b/base/cvd/allocd/net/nft_rule_test.cc @@ -0,0 +1,75 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#include "allocd/net/nft_rule.h" + +#include +#include + +#include +#include + +#include "allocd/test/mock_nftables.h" +#include "cuttlefish/result/result_matchers.h" + +namespace cuttlefish { +namespace { + +using ::testing::Eq; +using ::testing::Return; + +TEST(NftRuleTest, CreateAndAutoDeleteOnDestruction) { + MockNftables mock_nft; + constexpr uint32_t kHandle = 42; + + EXPECT_CALL(mock_nft, AddRule("ip", "table1", "chain1", + "content1 comment \"cvdalloc-tag1\"")) + .WillOnce(Return(kHandle)); + EXPECT_CALL(mock_nft, + DeleteRulesByComment("ip", "table1", "chain1", "cvdalloc-tag1")) + .WillOnce(Return(Result{})); + + { + auto rule = + NftRule::Create(mock_nft, "ip", "table1", "chain1", "content1", "tag1"); + EXPECT_THAT(rule, IsOk()); + } +} + +TEST(NftRuleTest, MoveConstructorTransfersOwnership) { + MockNftables mock_nft; + constexpr uint32_t kHandle = 100; + + EXPECT_CALL(mock_nft, AddRule("ip", "table1", "chain1", + "content1 comment \"cvdalloc-tag1\"")) + .WillOnce(Return(kHandle)); + EXPECT_CALL(mock_nft, + DeleteRulesByComment("ip", "table1", "chain1", "cvdalloc-tag1")) + .WillOnce(Return(Result{})); + + { + auto rule1 = + NftRule::Create(mock_nft, "ip", "table1", "chain1", "content1", "tag1"); + ASSERT_THAT(rule1, IsOk()); + + NftRule rule2(std::move(*rule1)); + // When rule1 leaves scope, it should not call DeleteRule. + // Only rule2 leaving scope will call DeleteRule once. + } +} + +} // namespace +} // namespace cuttlefish diff --git a/base/cvd/allocd/net/nftables.h b/base/cvd/allocd/net/nftables.h new file mode 100644 index 00000000000..5e2b45486cd --- /dev/null +++ b/base/cvd/allocd/net/nftables.h @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#ifndef ALLOCD_NET_NFTABLES_H_ +#define ALLOCD_NET_NFTABLES_H_ + +#include + +#include + +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +class Nftables { + public: + virtual ~Nftables() = default; + + virtual Result EnsureTable(std::string_view family, + std::string_view table) = 0; + virtual Result DeleteTable(std::string_view family, + std::string_view table) = 0; + virtual Result EnsureChain(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view content) = 0; + virtual Result AddRule(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view content) = 0; + virtual Result DeleteRule(std::string_view family, + std::string_view table, + std::string_view chain, uint64_t handle) = 0; + virtual Result DeleteRulesByComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) = 0; +}; + +} // namespace cuttlefish + +#endif // ALLOCD_NET_NFTABLES_H_ diff --git a/base/cvd/allocd/net/nftables_nft.cc b/base/cvd/allocd/net/nftables_nft.cc new file mode 100644 index 00000000000..34db04e4085 --- /dev/null +++ b/base/cvd/allocd/net/nftables_nft.cc @@ -0,0 +1,202 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#include "allocd/net/nftables_nft.h" + +#include + +#include +#include +#include +#include + +#include "absl/base/no_destructor.h" +#include "absl/log/log.h" +#include "json/value.h" + +#include "cuttlefish/common/libs/utils/files.h" +#include "cuttlefish/common/libs/utils/json.h" +#include "cuttlefish/process/command.h" +#include "cuttlefish/process/managed_stdio.h" +#include "cuttlefish/process/subprocess.h" +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +namespace { + +constexpr std::string_view kNftBinary = "nft"; + +// Searches PATH, then the usual sbin locations, for the nft binary. +Result SearchForNft() { + Result p = Search(Path(), std::string(kNftBinary)); + if (p.has_value()) { + return p; + } + return CF_EXPECT(Search({"/usr/sbin", "/sbin"}, std::string(kNftBinary)), + "could not find nft binary"); +} + +} // namespace + +Result NftablesNft::BinaryPath() { + static const absl::NoDestructor path( + SearchForNft().value_or("")); + CF_EXPECT(!path->empty(), "could not find nft binary"); + return *path; +} + +Result NftablesNft::EnsureTable(std::string_view family, + std::string_view table) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("add"); + cmd.AddParameter("table"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + + CF_EXPECTF(cmd.Start().Wait() == 0, + "Failed to ensure nft table: family={}, table={}", family, table); + return {}; +} + +Result NftablesNft::DeleteTable(std::string_view family, + std::string_view table) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("delete"); + cmd.AddParameter("table"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + + CF_EXPECTF(cmd.Start().Wait() == 0, + "Failed to delete nft table: family={}, table={}", family, table); + return {}; +} + +Result NftablesNft::EnsureChain(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view content) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("add"); + cmd.AddParameter("chain"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + cmd.AddParameter(std::string(chain)); + if (!content.empty()) { + cmd.AddParameter(std::string(content)); + } + + CF_EXPECTF( + cmd.Start().Wait() == 0, + "Failed to ensure nft chain: family={}, table={}, chain={}, content={}", + family, table, chain, content); + return {}; +} + +Result NftablesNft::AddRule(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view content) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("-j"); + cmd.AddParameter("-e"); + cmd.AddParameter("add"); + cmd.AddParameter("rule"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + cmd.AddParameter(std::string(chain)); + cmd.AddParameter(std::string(content)); + + std::string stdout_str = CF_EXPECT(RunAndCaptureStdout(std::move(cmd))); + Json::Value json = CF_EXPECT(ParseJson(stdout_str)); + + CF_EXPECT(json.isMember("nftables") && json["nftables"].isArray(), + "Invalid JSON output from nft: " << stdout_str); + + for (const auto& item : json["nftables"]) { + if (item.isMember("add") && item["add"].isMember("rule") && + item["add"]["rule"].isMember("handle")) { + return item["add"]["rule"]["handle"].asUInt64(); + } + } + + return CF_ERR("No rule handle found in nft JSON output: " << stdout_str); +} + +Result NftablesNft::DeleteRule(std::string_view family, + std::string_view table, + std::string_view chain, uint64_t handle) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("delete"); + cmd.AddParameter("rule"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + cmd.AddParameter(std::string(chain)); + cmd.AddParameter("handle"); + cmd.AddParameter(std::to_string(handle)); + + CF_EXPECTF(cmd.Start().Wait() == 0, + "Failed to delete nft rule: family={}, table={}, chain={}, " + "handle={}", + family, table, chain, handle); + return {}; +} + +Result NftablesNft::DeleteRulesByComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) { + Command cmd{CF_EXPECT(BinaryPath())}; + cmd.AddParameter("-j"); + cmd.AddParameter("list"); + cmd.AddParameter("chain"); + cmd.AddParameter(std::string(family)); + cmd.AddParameter(std::string(table)); + cmd.AddParameter(std::string(chain)); + + // If the chain/table no longer exists (e.g. torn down already), there is + // nothing to delete; treat that as success so teardown stays idempotent. + Result stdout_str = RunAndCaptureStdout(std::move(cmd)); + if (!stdout_str.has_value()) { + LOG(INFO) << "nft list chain failed, treating as no-op: family=" << family + << ", table=" << table << ", chain=" << chain; + return {}; + } + + Json::Value json = CF_EXPECT(ParseJson(*stdout_str)); + CF_EXPECT(json.isMember("nftables") && json["nftables"].isArray(), + "Invalid JSON output from nft: " << *stdout_str); + + std::vector handles; + for (const auto& item : json["nftables"]) { + if (!item.isMember("rule")) { + continue; + } + const Json::Value& rule = item["rule"]; + if (rule.isMember("comment") && rule.isMember("handle") && + rule["comment"].asString() == comment) { + handles.push_back(rule["handle"].asUInt64()); + } + } + + for (uint64_t handle : handles) { + CF_EXPECT(DeleteRule(family, table, chain, handle)); + } + + return {}; +} + +} // namespace cuttlefish diff --git a/base/cvd/allocd/net/nftables_nft.h b/base/cvd/allocd/net/nftables_nft.h new file mode 100644 index 00000000000..a3ada334aa2 --- /dev/null +++ b/base/cvd/allocd/net/nftables_nft.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#ifndef ALLOCD_NET_NFTABLES_NFT_H_ +#define ALLOCD_NET_NFTABLES_NFT_H_ + +#include + +#include + +#include "allocd/net/nftables.h" +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +class NftablesNft : public Nftables { + public: + NftablesNft() = default; + ~NftablesNft() override = default; + + // Returns the resolved path to the `nft` binary, or an error if it is not + // available. Static so callers can probe for nft support without having to + // construct an instance. + static Result BinaryPath(); + + Result EnsureTable(std::string_view family, + std::string_view table) override; + Result DeleteTable(std::string_view family, + std::string_view table) override; + Result EnsureChain(std::string_view family, std::string_view table, + std::string_view chain, + std::string_view content) override; + Result AddRule(std::string_view family, std::string_view table, + std::string_view chain, + std::string_view content) override; + Result DeleteRule(std::string_view family, std::string_view table, + std::string_view chain, uint64_t handle) override; + Result DeleteRulesByComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) override; +}; + +} // namespace cuttlefish + +#endif // ALLOCD_NET_NFTABLES_NFT_H_ diff --git a/base/cvd/allocd/test/BUILD.bazel b/base/cvd/allocd/test/BUILD.bazel new file mode 100644 index 00000000000..5c5cd70fe6e --- /dev/null +++ b/base/cvd/allocd/test/BUILD.bazel @@ -0,0 +1,24 @@ +load("//cuttlefish/bazel:rules.bzl", "cf_cc_library") + +package( + default_visibility = ["//:android_cuttlefish"], +) + +cf_cc_library( + name = "mock_nftables", + hdrs = ["mock_nftables.h"], + deps = [ + "//allocd/net:nftables", + "@googletest//:gtest", + ], +) + +cf_cc_library( + name = "fake_nftables", + testonly = True, + hdrs = ["fake_nftables.h"], + deps = [ + "//allocd/net:nftables", + "//cuttlefish/result", + ], +) diff --git a/base/cvd/allocd/test/fake_nftables.h b/base/cvd/allocd/test/fake_nftables.h new file mode 100644 index 00000000000..d19b4e815c0 --- /dev/null +++ b/base/cvd/allocd/test/fake_nftables.h @@ -0,0 +1,124 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#ifndef ALLOCD_TEST_FAKE_NFTABLES_H_ +#define ALLOCD_TEST_FAKE_NFTABLES_H_ + +#include + +#include +#include +#include +#include + +#include "allocd/net/nftables.h" +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +// A minimal, stateful in-memory Nftables test double. +// +// It models a single (family, table, chain) and ignores those arguments. +// Its purpose is to reflect the one behaviour that we need statefulness for +// (handle numbers). +class FakeNftables : public Nftables { + public: + Result EnsureTable(std::string_view /*family*/, + std::string_view /*table*/) override { + return {}; + } + + // Deleting the table drops its rules and restarts the handle counter, exactly + // as nft does when the table is later recreated. + Result DeleteTable(std::string_view /*family*/, + std::string_view /*table*/) override { + rules_.clear(); + next_handle_ = 1; + return {}; + } + + Result EnsureChain(std::string_view /*family*/, + std::string_view /*table*/, + std::string_view /*chain*/, + std::string_view /*content*/) override { + return {}; + } + + Result AddRule(std::string_view /*family*/, + std::string_view /*table*/, + std::string_view /*chain*/, + std::string_view content) override { + uint64_t handle = next_handle_++; + rules_.push_back(Rule{handle, ParseComment(content)}); + return handle; + } + + Result DeleteRule(std::string_view /*family*/, + std::string_view /*table*/, + std::string_view /*chain*/, + uint64_t handle) override { + auto it = std::remove_if(rules_.begin(), rules_.end(), + [&](const Rule& r) { return r.handle == handle; }); + CF_EXPECTF(it != rules_.end(), "no rule with handle {}", handle); + rules_.erase(it, rules_.end()); + return {}; + } + + Result DeleteRulesByComment(std::string_view /*family*/, + std::string_view /*table*/, + std::string_view /*chain*/, + std::string_view comment) override { + // A missing match is a no-op, matching idempotent teardown semantics. + rules_.erase( + std::remove_if(rules_.begin(), rules_.end(), + [&](const Rule& r) { return r.comment == comment; }), + rules_.end()); + return {}; + } + + bool HasRuleWithComment(std::string_view comment) const { + return std::any_of(rules_.begin(), rules_.end(), + [&](const Rule& r) { return r.comment == comment; }); + } + + private: + struct Rule { + uint64_t handle; + std::string comment; + }; + + // Extracts the value of an nft `comment "..."` token, if present. + static std::string ParseComment(std::string_view content) { + constexpr std::string_view kMarker = "comment \""; + auto pos = content.find(kMarker); + if (pos == std::string_view::npos) { + return ""; + } + auto start = pos + kMarker.size(); + auto end = content.find('"', start); + if (end == std::string_view::npos) { + return ""; + } + return std::string(content.substr(start, end - start)); + } + + std::vector rules_; + uint64_t next_handle_ = 1; +}; + +} // namespace cuttlefish + +#endif // ALLOCD_TEST_FAKE_NFTABLES_H_ diff --git a/base/cvd/allocd/test/mock_nftables.h b/base/cvd/allocd/test/mock_nftables.h new file mode 100644 index 00000000000..46d512c534c --- /dev/null +++ b/base/cvd/allocd/test/mock_nftables.h @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#ifndef ALLOCD_TEST_MOCK_NFTABLES_H_ +#define ALLOCD_TEST_MOCK_NFTABLES_H_ + +#include + +#include + +#include "allocd/net/nftables.h" + +namespace cuttlefish { + +class MockNftables : public Nftables { + public: + MOCK_METHOD(Result, EnsureTable, + (std::string_view family, std::string_view table), (override)); + MOCK_METHOD(Result, DeleteTable, + (std::string_view family, std::string_view table), (override)); + MOCK_METHOD(Result, EnsureChain, + (std::string_view family, std::string_view table, + std::string_view chain, std::string_view content), + (override)); + MOCK_METHOD(Result, AddRule, + (std::string_view family, std::string_view table, + std::string_view chain, std::string_view content), + (override)); + MOCK_METHOD(Result, DeleteRule, + (std::string_view family, std::string_view table, + std::string_view chain, uint64_t handle), + (override)); + MOCK_METHOD(Result, DeleteRulesByComment, + (std::string_view family, std::string_view table, + std::string_view chain, std::string_view comment), + (override)); +}; + +} // namespace cuttlefish + +#endif // ALLOCD_TEST_MOCK_NFTABLES_H_ From 791af50880590bebda6917b6a5ab60d906406a5b Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Wed, 2 Sep 2026 14:48:42 -0700 Subject: [PATCH 2/4] switch mock to fake entirely + address several review comments --- base/cvd/allocd/net/BUILD.bazel | 3 +- base/cvd/allocd/net/nft_rule.cc | 4 +- base/cvd/allocd/net/nft_rule.h | 6 +- base/cvd/allocd/net/nft_rule_test.cc | 65 +++++---- base/cvd/allocd/net/nftables.h | 3 +- base/cvd/allocd/net/nftables_nft.cc | 112 ++++++++------- base/cvd/allocd/net/nftables_nft.h | 5 +- base/cvd/allocd/test/BUILD.bazel | 20 +-- base/cvd/allocd/test/fake_nftables.cc | 160 +++++++++++++++++++++ base/cvd/allocd/test/fake_nftables.h | 117 +++++---------- base/cvd/allocd/test/fake_nftables_test.cc | 130 +++++++++++++++++ base/cvd/allocd/test/mock_nftables.h | 54 ------- 12 files changed, 447 insertions(+), 232 deletions(-) create mode 100644 base/cvd/allocd/test/fake_nftables.cc create mode 100644 base/cvd/allocd/test/fake_nftables_test.cc delete mode 100644 base/cvd/allocd/test/mock_nftables.h diff --git a/base/cvd/allocd/net/BUILD.bazel b/base/cvd/allocd/net/BUILD.bazel index e60c0e21eb2..c728ea70fda 100644 --- a/base/cvd/allocd/net/BUILD.bazel +++ b/base/cvd/allocd/net/BUILD.bazel @@ -50,7 +50,8 @@ cf_cc_test( srcs = ["nft_rule_test.cc"], deps = [ ":nftables", - "//allocd/test:mock_nftables", + "//allocd/test:fake_nftables", + "//cuttlefish/result", "//cuttlefish/result:result_matchers", ], ) diff --git a/base/cvd/allocd/net/nft_rule.cc b/base/cvd/allocd/net/nft_rule.cc index 2fbcf44515b..e5292ec7918 100644 --- a/base/cvd/allocd/net/nft_rule.cc +++ b/base/cvd/allocd/net/nft_rule.cc @@ -39,9 +39,7 @@ Result NftRule::Create(Nftables& nft, std::string_view family, std::string_view content, std::string_view tag) { std::string comment = absl::StrCat(kCvdallocCommentPrefix, tag); - std::string full_content = - absl::StrCat(content, " comment \"", comment, "\""); - CF_EXPECT(nft.AddRule(family, table, chain, full_content)); + CF_EXPECT(nft.AddRule(family, table, chain, content, comment)); return NftRule(&nft, family, table, chain, std::move(comment)); } diff --git a/base/cvd/allocd/net/nft_rule.h b/base/cvd/allocd/net/nft_rule.h index 10257daf27f..c2c5030ded7 100644 --- a/base/cvd/allocd/net/nft_rule.h +++ b/base/cvd/allocd/net/nft_rule.h @@ -32,9 +32,6 @@ class NftRule { std::string_view table, std::string_view chain, std::string_view content, std::string_view tag); - NftRule() = delete; - NftRule(Nftables* nft, std::string_view family, std::string_view table, - std::string_view chain, std::string comment); ~NftRule(); NftRule(NftRule&& r) noexcept; @@ -43,6 +40,9 @@ class NftRule { NftRule& operator=(const NftRule& r) = delete; private: + NftRule(Nftables* nft, std::string_view family, std::string_view table, + std::string_view chain, std::string comment); + Nftables* nft_ = nullptr; std::string family_; std::string table_; diff --git a/base/cvd/allocd/net/nft_rule_test.cc b/base/cvd/allocd/net/nft_rule_test.cc index 89ff053eb6e..cd3d5c8a75a 100644 --- a/base/cvd/allocd/net/nft_rule_test.cc +++ b/base/cvd/allocd/net/nft_rule_test.cc @@ -19,56 +19,59 @@ #include #include -#include #include -#include "allocd/test/mock_nftables.h" +#include "allocd/test/fake_nftables.h" +#include "cuttlefish/result/result.h" #include "cuttlefish/result/result_matchers.h" namespace cuttlefish { namespace { -using ::testing::Eq; -using ::testing::Return; +class NftRuleTest : public ::testing::Test { + protected: + void SetUp() override { + ASSERT_THAT(fake_.EnsureTable("ip", "table1"), IsOk()); + ASSERT_THAT(fake_.EnsureChain("ip", "table1", "chain1", ""), IsOk()); + } -TEST(NftRuleTest, CreateAndAutoDeleteOnDestruction) { - MockNftables mock_nft; - constexpr uint32_t kHandle = 42; + FakeNftables fake_; +}; - EXPECT_CALL(mock_nft, AddRule("ip", "table1", "chain1", - "content1 comment \"cvdalloc-tag1\"")) - .WillOnce(Return(kHandle)); - EXPECT_CALL(mock_nft, - DeleteRulesByComment("ip", "table1", "chain1", "cvdalloc-tag1")) - .WillOnce(Return(Result{})); +TEST_F(NftRuleTest, CreateAddsRuleWithPrefixedComment) { + Result rule = + NftRule::Create(fake_, "ip", "table1", "chain1", "content1", "tag1"); + ASSERT_THAT(rule, IsOk()); + EXPECT_TRUE( + fake_.HasRuleWithComment("ip", "table1", "chain1", "cvdalloc-tag1")); +} +TEST_F(NftRuleTest, DeletesRuleOnDestruction) { { - auto rule = - NftRule::Create(mock_nft, "ip", "table1", "chain1", "content1", "tag1"); - EXPECT_THAT(rule, IsOk()); + Result rule = + NftRule::Create(fake_, "ip", "table1", "chain1", "content1", "tag1"); + ASSERT_THAT(rule, IsOk()); + EXPECT_TRUE( + fake_.HasRuleWithComment("ip", "table1", "chain1", "cvdalloc-tag1")); } + EXPECT_FALSE( + fake_.HasRuleWithComment("ip", "table1", "chain1", "cvdalloc-tag1")); + EXPECT_EQ(fake_.RuleCount("ip", "table1", "chain1"), 0); } -TEST(NftRuleTest, MoveConstructorTransfersOwnership) { - MockNftables mock_nft; - constexpr uint32_t kHandle = 100; - - EXPECT_CALL(mock_nft, AddRule("ip", "table1", "chain1", - "content1 comment \"cvdalloc-tag1\"")) - .WillOnce(Return(kHandle)); - EXPECT_CALL(mock_nft, - DeleteRulesByComment("ip", "table1", "chain1", "cvdalloc-tag1")) - .WillOnce(Return(Result{})); - +TEST_F(NftRuleTest, MoveConstructorTransfersOwnership) { { - auto rule1 = - NftRule::Create(mock_nft, "ip", "table1", "chain1", "content1", "tag1"); + Result rule1 = + NftRule::Create(fake_, "ip", "table1", "chain1", "content1", "tag1"); ASSERT_THAT(rule1, IsOk()); NftRule rule2(std::move(*rule1)); - // When rule1 leaves scope, it should not call DeleteRule. - // Only rule2 leaving scope will call DeleteRule once. + EXPECT_TRUE( + fake_.HasRuleWithComment("ip", "table1", "chain1", "cvdalloc-tag1")); } + EXPECT_FALSE( + fake_.HasRuleWithComment("ip", "table1", "chain1", "cvdalloc-tag1")); + EXPECT_EQ(fake_.RuleCount("ip", "table1", "chain1"), 0); } } // namespace diff --git a/base/cvd/allocd/net/nftables.h b/base/cvd/allocd/net/nftables.h index 5e2b45486cd..fc4384495f6 100644 --- a/base/cvd/allocd/net/nftables.h +++ b/base/cvd/allocd/net/nftables.h @@ -40,7 +40,8 @@ class Nftables { virtual Result AddRule(std::string_view family, std::string_view table, std::string_view chain, - std::string_view content) = 0; + std::string_view content, + std::string_view comment) = 0; virtual Result DeleteRule(std::string_view family, std::string_view table, std::string_view chain, uint64_t handle) = 0; diff --git a/base/cvd/allocd/net/nftables_nft.cc b/base/cvd/allocd/net/nftables_nft.cc index 34db04e4085..048878456de 100644 --- a/base/cvd/allocd/net/nftables_nft.cc +++ b/base/cvd/allocd/net/nftables_nft.cc @@ -25,6 +25,7 @@ #include "absl/base/no_destructor.h" #include "absl/log/log.h" +#include "absl/strings/str_cat.h" #include "json/value.h" #include "cuttlefish/common/libs/utils/files.h" @@ -61,11 +62,11 @@ Result NftablesNft::BinaryPath() { Result NftablesNft::EnsureTable(std::string_view family, std::string_view table) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("add"); - cmd.AddParameter("table"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("add") + .AddParameter("table") + .AddParameter(family) + .AddParameter(table); CF_EXPECTF(cmd.Start().Wait() == 0, "Failed to ensure nft table: family={}, table={}", family, table); @@ -74,11 +75,11 @@ Result NftablesNft::EnsureTable(std::string_view family, Result NftablesNft::DeleteTable(std::string_view family, std::string_view table) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("delete"); - cmd.AddParameter("table"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("delete") + .AddParameter("table") + .AddParameter(family) + .AddParameter(table); CF_EXPECTF(cmd.Start().Wait() == 0, "Failed to delete nft table: family={}, table={}", family, table); @@ -89,14 +90,14 @@ Result NftablesNft::EnsureChain(std::string_view family, std::string_view table, std::string_view chain, std::string_view content) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("add"); - cmd.AddParameter("chain"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); - cmd.AddParameter(std::string(chain)); + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("add") + .AddParameter("chain") + .AddParameter(family) + .AddParameter(table) + .AddParameter(chain); if (!content.empty()) { - cmd.AddParameter(std::string(content)); + cmd.AddParameter(content); } CF_EXPECTF( @@ -109,22 +110,27 @@ Result NftablesNft::EnsureChain(std::string_view family, Result NftablesNft::AddRule(std::string_view family, std::string_view table, std::string_view chain, - std::string_view content) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("-j"); - cmd.AddParameter("-e"); - cmd.AddParameter("add"); - cmd.AddParameter("rule"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); - cmd.AddParameter(std::string(chain)); - cmd.AddParameter(std::string(content)); + std::string_view content, + std::string_view comment) { + std::string rule = comment.empty() + ? std::string(content) + : absl::StrCat(content, " comment \"", comment, "\""); + + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("-j") + .AddParameter("-e") + .AddParameter("add") + .AddParameter("rule") + .AddParameter(family) + .AddParameter(table) + .AddParameter(chain) + .AddParameter(rule); std::string stdout_str = CF_EXPECT(RunAndCaptureStdout(std::move(cmd))); Json::Value json = CF_EXPECT(ParseJson(stdout_str)); - CF_EXPECT(json.isMember("nftables") && json["nftables"].isArray(), - "Invalid JSON output from nft: " << stdout_str); + CF_EXPECTF(json.isMember("nftables") && json["nftables"].isArray(), + "Invalid JSON output from nft: {}", stdout_str); for (const auto& item : json["nftables"]) { if (item.isMember("add") && item["add"].isMember("rule") && @@ -133,20 +139,20 @@ Result NftablesNft::AddRule(std::string_view family, } } - return CF_ERR("No rule handle found in nft JSON output: " << stdout_str); + return CF_ERRF("No rule handle found in nft JSON output: {}", stdout_str); } Result NftablesNft::DeleteRule(std::string_view family, std::string_view table, std::string_view chain, uint64_t handle) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("delete"); - cmd.AddParameter("rule"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); - cmd.AddParameter(std::string(chain)); - cmd.AddParameter("handle"); - cmd.AddParameter(std::to_string(handle)); + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("delete") + .AddParameter("rule") + .AddParameter(family) + .AddParameter(table) + .AddParameter(chain) + .AddParameter("handle") + .AddParameter(std::to_string(handle)); CF_EXPECTF(cmd.Start().Wait() == 0, "Failed to delete nft rule: family={}, table={}, chain={}, " @@ -159,13 +165,13 @@ Result NftablesNft::DeleteRulesByComment(std::string_view family, std::string_view table, std::string_view chain, std::string_view comment) { - Command cmd{CF_EXPECT(BinaryPath())}; - cmd.AddParameter("-j"); - cmd.AddParameter("list"); - cmd.AddParameter("chain"); - cmd.AddParameter(std::string(family)); - cmd.AddParameter(std::string(table)); - cmd.AddParameter(std::string(chain)); + Command cmd = Command(CF_EXPECT(BinaryPath())) + .AddParameter("-j") + .AddParameter("list") + .AddParameter("chain") + .AddParameter(family) + .AddParameter(table) + .AddParameter(chain); // If the chain/table no longer exists (e.g. torn down already), there is // nothing to delete; treat that as success so teardown stays idempotent. @@ -177,8 +183,8 @@ Result NftablesNft::DeleteRulesByComment(std::string_view family, } Json::Value json = CF_EXPECT(ParseJson(*stdout_str)); - CF_EXPECT(json.isMember("nftables") && json["nftables"].isArray(), - "Invalid JSON output from nft: " << *stdout_str); + CF_EXPECTF(json.isMember("nftables") && json["nftables"].isArray(), + "Invalid JSON output from nft: {}", *stdout_str); std::vector handles; for (const auto& item : json["nftables"]) { @@ -192,11 +198,19 @@ Result NftablesNft::DeleteRulesByComment(std::string_view family, } } + Result deletion_result = {}; for (uint64_t handle : handles) { - CF_EXPECT(DeleteRule(family, table, chain, handle)); + Result res = DeleteRule(family, table, chain, handle); + if (!res.has_value()) { + LOG(ERROR) << "Failed to delete nft rule, continuing: handle=" << handle + << ": " << res.error(); + if (deletion_result.has_value()) { + deletion_result = std::move(res); + } + } } - return {}; + return deletion_result; } } // namespace cuttlefish diff --git a/base/cvd/allocd/net/nftables_nft.h b/base/cvd/allocd/net/nftables_nft.h index a3ada334aa2..166eb7a1275 100644 --- a/base/cvd/allocd/net/nftables_nft.h +++ b/base/cvd/allocd/net/nftables_nft.h @@ -19,6 +19,7 @@ #include +#include #include #include "allocd/net/nftables.h" @@ -44,8 +45,8 @@ class NftablesNft : public Nftables { std::string_view chain, std::string_view content) override; Result AddRule(std::string_view family, std::string_view table, - std::string_view chain, - std::string_view content) override; + std::string_view chain, std::string_view content, + std::string_view comment) override; Result DeleteRule(std::string_view family, std::string_view table, std::string_view chain, uint64_t handle) override; Result DeleteRulesByComment(std::string_view family, diff --git a/base/cvd/allocd/test/BUILD.bazel b/base/cvd/allocd/test/BUILD.bazel index 5c5cd70fe6e..281da74f348 100644 --- a/base/cvd/allocd/test/BUILD.bazel +++ b/base/cvd/allocd/test/BUILD.bazel @@ -1,24 +1,26 @@ -load("//cuttlefish/bazel:rules.bzl", "cf_cc_library") +load("//cuttlefish/bazel:rules.bzl", "cf_cc_library", "cf_cc_test") package( default_visibility = ["//:android_cuttlefish"], ) cf_cc_library( - name = "mock_nftables", - hdrs = ["mock_nftables.h"], + name = "fake_nftables", + testonly = True, + srcs = ["fake_nftables.cc"], + hdrs = ["fake_nftables.h"], deps = [ "//allocd/net:nftables", - "@googletest//:gtest", + "//cuttlefish/result", ], ) -cf_cc_library( - name = "fake_nftables", - testonly = True, - hdrs = ["fake_nftables.h"], +cf_cc_test( + name = "fake_nftables_test", + srcs = ["fake_nftables_test.cc"], deps = [ - "//allocd/net:nftables", + ":fake_nftables", "//cuttlefish/result", + "//cuttlefish/result:result_matchers", ], ) diff --git a/base/cvd/allocd/test/fake_nftables.cc b/base/cvd/allocd/test/fake_nftables.cc new file mode 100644 index 00000000000..a56a5dd4246 --- /dev/null +++ b/base/cvd/allocd/test/fake_nftables.cc @@ -0,0 +1,160 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#include "allocd/test/fake_nftables.h" + +#include + +#include +#include +#include +#include + +#include "cuttlefish/result/result.h" + +namespace cuttlefish { + +const FakeNftables::Table* FakeNftables::FindTable( + std::string_view family, std::string_view table) const { + auto it = tables_.find({std::string(family), std::string(table)}); + return it == tables_.end() ? nullptr : &it->second; +} + +FakeNftables::Table* FakeNftables::FindTable(std::string_view family, + std::string_view table) { + auto it = tables_.find({std::string(family), std::string(table)}); + return it == tables_.end() ? nullptr : &it->second; +} + +Result FakeNftables::EnsureTable(std::string_view family, + std::string_view table) { + tables_.try_emplace({std::string(family), std::string(table)}); + return {}; +} + +Result FakeNftables::DeleteTable(std::string_view family, + std::string_view table) { + auto it = tables_.find({std::string(family), std::string(table)}); + CF_EXPECTF(it != tables_.end(), "no such table: family={}, table={}", family, + table); + // Erasing the table drops its chains/rules; a later EnsureTable starts a + // fresh handle counter, exactly as nft does when a table is recreated. + tables_.erase(it); + return {}; +} + +Result FakeNftables::EnsureChain(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view /*content*/) { + Table* t = FindTable(family, table); + CF_EXPECTF(t != nullptr, "no such table: family={}, table={}", family, table); + t->chains.try_emplace(std::string(chain)); + return {}; +} + +Result FakeNftables::AddRule(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view content, + std::string_view comment) { + Table* t = FindTable(family, table); + CF_EXPECTF(t != nullptr, "no such table: family={}, table={}", family, table); + auto chain_it = t->chains.find(std::string(chain)); + CF_EXPECTF(chain_it != t->chains.end(), + "no such chain: family={}, table={}, chain={}", family, table, + chain); + uint64_t handle = t->next_handle++; + chain_it->second.push_back( + Rule{handle, std::string(content), std::string(comment)}); + return handle; +} + +Result FakeNftables::DeleteRule(std::string_view family, + std::string_view table, + std::string_view chain, uint64_t handle) { + Table* t = FindTable(family, table); + CF_EXPECTF(t != nullptr, "no such table: family={}, table={}", family, table); + auto chain_it = t->chains.find(std::string(chain)); + CF_EXPECTF(chain_it != t->chains.end(), + "no such chain: family={}, table={}, chain={}", family, table, + chain); + auto removed = std::erase_if( + chain_it->second, [&](const Rule& r) { return r.handle == handle; }); + CF_EXPECTF(removed > 0, "no rule with handle {}", handle); + return {}; +} + +Result FakeNftables::DeleteRulesByComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) { + // A missing table/chain or a comment that matches nothing is a no-op, + // matching the idempotent teardown semantics of the real implementation. + Table* t = FindTable(family, table); + if (t == nullptr) { + return {}; + } + auto chain_it = t->chains.find(std::string(chain)); + if (chain_it == t->chains.end()) { + return {}; + } + std::erase_if(chain_it->second, + [&](const Rule& r) { return r.comment == comment; }); + return {}; +} + +bool FakeNftables::HasTable(std::string_view family, + std::string_view table) const { + return FindTable(family, table) != nullptr; +} + +bool FakeNftables::HasChain(std::string_view family, std::string_view table, + std::string_view chain) const { + const Table* t = FindTable(family, table); + return t != nullptr && t->chains.contains(std::string(chain)); +} + +bool FakeNftables::HasRuleWithComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) const { + const Table* t = FindTable(family, table); + if (t == nullptr) { + return false; + } + auto chain_it = t->chains.find(std::string(chain)); + if (chain_it == t->chains.end()) { + return false; + } + return std::any_of(chain_it->second.begin(), chain_it->second.end(), + [&](const Rule& r) { return r.comment == comment; }); +} + +int FakeNftables::RuleCount(std::string_view family, std::string_view table, + std::string_view chain) const { + const Table* t = FindTable(family, table); + if (t == nullptr) { + return 0; + } + auto chain_it = t->chains.find(std::string(chain)); + if (chain_it == t->chains.end()) { + return 0; + } + return static_cast(chain_it->second.size()); +} + +} // namespace cuttlefish diff --git a/base/cvd/allocd/test/fake_nftables.h b/base/cvd/allocd/test/fake_nftables.h index d19b4e815c0..340b1d5bcb1 100644 --- a/base/cvd/allocd/test/fake_nftables.h +++ b/base/cvd/allocd/test/fake_nftables.h @@ -19,9 +19,10 @@ #include -#include +#include #include #include +#include #include #include "allocd/net/nftables.h" @@ -29,94 +30,52 @@ namespace cuttlefish { -// A minimal, stateful in-memory Nftables test double. -// -// It models a single (family, table, chain) and ignores those arguments. -// Its purpose is to reflect the one behaviour that we need statefulness for -// (handle numbers). +// A stateful, in-memory Nftables test double. +// Statefulness is needed for testing behaviour around handles. class FakeNftables : public Nftables { public: - Result EnsureTable(std::string_view /*family*/, - std::string_view /*table*/) override { - return {}; - } - - // Deleting the table drops its rules and restarts the handle counter, exactly - // as nft does when the table is later recreated. - Result DeleteTable(std::string_view /*family*/, - std::string_view /*table*/) override { - rules_.clear(); - next_handle_ = 1; - return {}; - } - - Result EnsureChain(std::string_view /*family*/, - std::string_view /*table*/, - std::string_view /*chain*/, - std::string_view /*content*/) override { - return {}; - } - - Result AddRule(std::string_view /*family*/, - std::string_view /*table*/, - std::string_view /*chain*/, - std::string_view content) override { - uint64_t handle = next_handle_++; - rules_.push_back(Rule{handle, ParseComment(content)}); - return handle; - } - - Result DeleteRule(std::string_view /*family*/, - std::string_view /*table*/, - std::string_view /*chain*/, - uint64_t handle) override { - auto it = std::remove_if(rules_.begin(), rules_.end(), - [&](const Rule& r) { return r.handle == handle; }); - CF_EXPECTF(it != rules_.end(), "no rule with handle {}", handle); - rules_.erase(it, rules_.end()); - return {}; - } - - Result DeleteRulesByComment(std::string_view /*family*/, - std::string_view /*table*/, - std::string_view /*chain*/, - std::string_view comment) override { - // A missing match is a no-op, matching idempotent teardown semantics. - rules_.erase( - std::remove_if(rules_.begin(), rules_.end(), - [&](const Rule& r) { return r.comment == comment; }), - rules_.end()); - return {}; - } - - bool HasRuleWithComment(std::string_view comment) const { - return std::any_of(rules_.begin(), rules_.end(), - [&](const Rule& r) { return r.comment == comment; }); - } + Result EnsureTable(std::string_view family, + std::string_view table) override; + Result DeleteTable(std::string_view family, + std::string_view table) override; + Result EnsureChain(std::string_view family, std::string_view table, + std::string_view chain, + std::string_view content) override; + Result AddRule(std::string_view family, std::string_view table, + std::string_view chain, std::string_view content, + std::string_view comment) override; + Result DeleteRule(std::string_view family, std::string_view table, + std::string_view chain, uint64_t handle) override; + Result DeleteRulesByComment(std::string_view family, + std::string_view table, + std::string_view chain, + std::string_view comment) override; + + bool HasTable(std::string_view family, std::string_view table) const; + bool HasChain(std::string_view family, std::string_view table, + std::string_view chain) const; + bool HasRuleWithComment(std::string_view family, std::string_view table, + std::string_view chain, + std::string_view comment) const; + int RuleCount(std::string_view family, std::string_view table, + std::string_view chain) const; private: struct Rule { uint64_t handle; + std::string content; std::string comment; }; + struct Table { + std::map> chains; + uint64_t next_handle = 1; + }; + using TableKey = std::pair; - // Extracts the value of an nft `comment "..."` token, if present. - static std::string ParseComment(std::string_view content) { - constexpr std::string_view kMarker = "comment \""; - auto pos = content.find(kMarker); - if (pos == std::string_view::npos) { - return ""; - } - auto start = pos + kMarker.size(); - auto end = content.find('"', start); - if (end == std::string_view::npos) { - return ""; - } - return std::string(content.substr(start, end - start)); - } + const Table* FindTable(std::string_view family, std::string_view table) const; + Table* FindTable(std::string_view family, std::string_view table); - std::vector rules_; - uint64_t next_handle_ = 1; + std::map tables_; }; } // namespace cuttlefish diff --git a/base/cvd/allocd/test/fake_nftables_test.cc b/base/cvd/allocd/test/fake_nftables_test.cc new file mode 100644 index 00000000000..42b6fbb59d5 --- /dev/null +++ b/base/cvd/allocd/test/fake_nftables_test.cc @@ -0,0 +1,130 @@ +/* + * Copyright (C) 2026 The Android Open Source Project + * + * 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. + */ + +#include "allocd/test/fake_nftables.h" + +#include +#include +#include + +#include "cuttlefish/result/result.h" +#include "cuttlefish/result/result_matchers.h" + +namespace cuttlefish { +namespace { + +class FakeNftablesTest : public ::testing::Test { + protected: + void SetUp() override { + ASSERT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); + ASSERT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); + } + + FakeNftables fake_; +}; + +TEST_F(FakeNftablesTest, EnsureTableIsIdempotent) { + EXPECT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); + EXPECT_TRUE(fake_.HasTable("ip", "t")); +} + +TEST_F(FakeNftablesTest, EnsureChainIsIdempotent) { + EXPECT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); + EXPECT_TRUE(fake_.HasChain("ip", "t", "c")); +} + +TEST_F(FakeNftablesTest, EnsureChainRequiresTable) { + EXPECT_THAT(fake_.EnsureChain("ip", "missing", "c", ""), IsError()); +} + +TEST_F(FakeNftablesTest, AddRuleRequiresTableAndChain) { + EXPECT_THAT(fake_.AddRule("ip", "missing", "c", "content", "cmt"), IsError()); + EXPECT_THAT(fake_.AddRule("ip", "t", "missing", "content", "cmt"), IsError()); +} + +TEST_F(FakeNftablesTest, HandlesAreUniqueAndMonotonic) { + Result h1 = fake_.AddRule("ip", "t", "c", "content1", "a"); + Result h2 = fake_.AddRule("ip", "t", "c", "content2", "b"); + ASSERT_THAT(h1, IsOk()); + ASSERT_THAT(h2, IsOk()); + EXPECT_NE(*h1, *h2); + EXPECT_LT(*h1, *h2); +} + +TEST_F(FakeNftablesTest, DeleteRuleByHandleRemovesOnlyThatRule) { + Result h1 = fake_.AddRule("ip", "t", "c", "content1", "a"); + ASSERT_THAT(h1, IsOk()); + ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content2", "b"), IsOk()); + EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 2); + + EXPECT_THAT(fake_.DeleteRule("ip", "t", "c", *h1), IsOk()); + EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); + EXPECT_FALSE(fake_.HasRuleWithComment("ip", "t", "c", "a")); + EXPECT_TRUE(fake_.HasRuleWithComment("ip", "t", "c", "b")); +} + +TEST_F(FakeNftablesTest, DeleteRuleWithUnknownHandleFails) { + EXPECT_THAT(fake_.DeleteRule("ip", "t", "c", 9999), IsError()); +} + +TEST_F(FakeNftablesTest, DeleteRulesByCommentRemovesAllMatching) { + ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content1", "shared"), IsOk()); + ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content2", "shared"), IsOk()); + ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content3", "other"), IsOk()); + + EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "c", "shared"), IsOk()); + EXPECT_FALSE(fake_.HasRuleWithComment("ip", "t", "c", "shared")); + EXPECT_TRUE(fake_.HasRuleWithComment("ip", "t", "c", "other")); + EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); +} + +TEST_F(FakeNftablesTest, DeleteRulesByCommentIsIdempotent) { + // No matching comment, existing chain: no-op success. + EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "c", "nope"), IsOk()); + // Missing chain and missing table: still no-op success. + EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "missing", "x"), IsOk()); + EXPECT_THAT(fake_.DeleteRulesByComment("ip", "missing", "c", "x"), IsOk()); +} + +TEST_F(FakeNftablesTest, DeleteTableFailsWhenMissing) { + EXPECT_THAT(fake_.DeleteTable("ip", "missing"), IsError()); +} + +TEST_F(FakeNftablesTest, RecreatingTableResetsHandleCounter) { + Result first = fake_.AddRule("ip", "t", "c", "content", "a"); + ASSERT_THAT(first, IsOk()); + + ASSERT_THAT(fake_.DeleteTable("ip", "t"), IsOk()); + EXPECT_FALSE(fake_.HasTable("ip", "t")); + + ASSERT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); + ASSERT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); + Result again = fake_.AddRule("ip", "t", "c", "content", "a"); + ASSERT_THAT(again, IsOk()); + EXPECT_EQ(*first, *again); +} + +TEST_F(FakeNftablesTest, TablesAreIsolatedByFamilyAndName) { + ASSERT_THAT(fake_.EnsureTable("bridge", "t"), IsOk()); + ASSERT_THAT(fake_.EnsureChain("bridge", "t", "c", ""), IsOk()); + ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content", "a"), IsOk()); + + EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); + EXPECT_EQ(fake_.RuleCount("bridge", "t", "c"), 0); +} + +} // namespace +} // namespace cuttlefish diff --git a/base/cvd/allocd/test/mock_nftables.h b/base/cvd/allocd/test/mock_nftables.h deleted file mode 100644 index 46d512c534c..00000000000 --- a/base/cvd/allocd/test/mock_nftables.h +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright (C) 2026 The Android Open Source Project - * - * 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. - */ - -#ifndef ALLOCD_TEST_MOCK_NFTABLES_H_ -#define ALLOCD_TEST_MOCK_NFTABLES_H_ - -#include - -#include - -#include "allocd/net/nftables.h" - -namespace cuttlefish { - -class MockNftables : public Nftables { - public: - MOCK_METHOD(Result, EnsureTable, - (std::string_view family, std::string_view table), (override)); - MOCK_METHOD(Result, DeleteTable, - (std::string_view family, std::string_view table), (override)); - MOCK_METHOD(Result, EnsureChain, - (std::string_view family, std::string_view table, - std::string_view chain, std::string_view content), - (override)); - MOCK_METHOD(Result, AddRule, - (std::string_view family, std::string_view table, - std::string_view chain, std::string_view content), - (override)); - MOCK_METHOD(Result, DeleteRule, - (std::string_view family, std::string_view table, - std::string_view chain, uint64_t handle), - (override)); - MOCK_METHOD(Result, DeleteRulesByComment, - (std::string_view family, std::string_view table, - std::string_view chain, std::string_view comment), - (override)); -}; - -} // namespace cuttlefish - -#endif // ALLOCD_TEST_MOCK_NFTABLES_H_ From 74fb59e7a5030c56d7f3a584897a49d494b2aef2 Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Wed, 2 Sep 2026 14:54:40 -0700 Subject: [PATCH 3/4] clarify SearchForNft comment + restrict the nftables RAII lib to linux --- base/cvd/allocd/net/BUILD.bazel | 3 +++ base/cvd/allocd/net/nftables_nft.cc | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/base/cvd/allocd/net/BUILD.bazel b/base/cvd/allocd/net/BUILD.bazel index c728ea70fda..4038407b980 100644 --- a/base/cvd/allocd/net/BUILD.bazel +++ b/base/cvd/allocd/net/BUILD.bazel @@ -31,6 +31,9 @@ cf_cc_library( "nftables.h", "nftables_nft.h", ], + target_compatible_with = [ + "@platforms//os:linux", + ], deps = [ "//cuttlefish/common/libs/utils:files", "//cuttlefish/common/libs/utils:json", diff --git a/base/cvd/allocd/net/nftables_nft.cc b/base/cvd/allocd/net/nftables_nft.cc index 048878456de..03f0c8458d9 100644 --- a/base/cvd/allocd/net/nftables_nft.cc +++ b/base/cvd/allocd/net/nftables_nft.cc @@ -41,7 +41,9 @@ namespace { constexpr std::string_view kNftBinary = "nft"; -// Searches PATH, then the usual sbin locations, for the nft binary. +// nft usually lives in an sbin directory, which is often not on PATH for +// non-root callers, so fall back to the usual sbin locations rather than +// bailing out if we don't find it at the PATH. Result SearchForNft() { Result p = Search(Path(), std::string(kNftBinary)); if (p.has_value()) { From fc8f65aa4554c84eb0612c798a5559bfd84dffa4 Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Thu, 3 Sep 2026 14:45:10 -0700 Subject: [PATCH 4/4] last round of review fixes for fake_nftables --- base/cvd/allocd/test/fake_nftables.cc | 14 +-- base/cvd/allocd/test/fake_nftables.h | 3 +- base/cvd/allocd/test/fake_nftables_test.cc | 117 +++++++++++++-------- 3 files changed, 80 insertions(+), 54 deletions(-) diff --git a/base/cvd/allocd/test/fake_nftables.cc b/base/cvd/allocd/test/fake_nftables.cc index a56a5dd4246..8f41b04167d 100644 --- a/base/cvd/allocd/test/fake_nftables.cc +++ b/base/cvd/allocd/test/fake_nftables.cc @@ -73,7 +73,7 @@ Result FakeNftables::AddRule(std::string_view family, std::string_view comment) { Table* t = FindTable(family, table); CF_EXPECTF(t != nullptr, "no such table: family={}, table={}", family, table); - auto chain_it = t->chains.find(std::string(chain)); + auto chain_it = t->chains.find(chain); CF_EXPECTF(chain_it != t->chains.end(), "no such chain: family={}, table={}, chain={}", family, table, chain); @@ -88,11 +88,11 @@ Result FakeNftables::DeleteRule(std::string_view family, std::string_view chain, uint64_t handle) { Table* t = FindTable(family, table); CF_EXPECTF(t != nullptr, "no such table: family={}, table={}", family, table); - auto chain_it = t->chains.find(std::string(chain)); + auto chain_it = t->chains.find(chain); CF_EXPECTF(chain_it != t->chains.end(), "no such chain: family={}, table={}, chain={}", family, table, chain); - auto removed = std::erase_if( + std::vector::size_type removed = std::erase_if( chain_it->second, [&](const Rule& r) { return r.handle == handle; }); CF_EXPECTF(removed > 0, "no rule with handle {}", handle); return {}; @@ -108,7 +108,7 @@ Result FakeNftables::DeleteRulesByComment(std::string_view family, if (t == nullptr) { return {}; } - auto chain_it = t->chains.find(std::string(chain)); + auto chain_it = t->chains.find(chain); if (chain_it == t->chains.end()) { return {}; } @@ -125,7 +125,7 @@ bool FakeNftables::HasTable(std::string_view family, bool FakeNftables::HasChain(std::string_view family, std::string_view table, std::string_view chain) const { const Table* t = FindTable(family, table); - return t != nullptr && t->chains.contains(std::string(chain)); + return t != nullptr && t->chains.contains(chain); } bool FakeNftables::HasRuleWithComment(std::string_view family, @@ -136,7 +136,7 @@ bool FakeNftables::HasRuleWithComment(std::string_view family, if (t == nullptr) { return false; } - auto chain_it = t->chains.find(std::string(chain)); + auto chain_it = t->chains.find(chain); if (chain_it == t->chains.end()) { return false; } @@ -150,7 +150,7 @@ int FakeNftables::RuleCount(std::string_view family, std::string_view table, if (t == nullptr) { return 0; } - auto chain_it = t->chains.find(std::string(chain)); + auto chain_it = t->chains.find(chain); if (chain_it == t->chains.end()) { return 0; } diff --git a/base/cvd/allocd/test/fake_nftables.h b/base/cvd/allocd/test/fake_nftables.h index 340b1d5bcb1..a79ffe6de7e 100644 --- a/base/cvd/allocd/test/fake_nftables.h +++ b/base/cvd/allocd/test/fake_nftables.h @@ -19,6 +19,7 @@ #include +#include #include #include #include @@ -67,7 +68,7 @@ class FakeNftables : public Nftables { std::string comment; }; struct Table { - std::map> chains; + std::map, std::less<>> chains; uint64_t next_handle = 1; }; using TableKey = std::pair; diff --git a/base/cvd/allocd/test/fake_nftables_test.cc b/base/cvd/allocd/test/fake_nftables_test.cc index 42b6fbb59d5..4295417714d 100644 --- a/base/cvd/allocd/test/fake_nftables_test.cc +++ b/base/cvd/allocd/test/fake_nftables_test.cc @@ -20,110 +20,135 @@ #include #include +#include + #include "cuttlefish/result/result.h" #include "cuttlefish/result/result_matchers.h" namespace cuttlefish { namespace { +constexpr std::string_view kFamily = "ip"; +constexpr std::string_view kTable = "t"; +constexpr std::string_view kChain = "c"; + class FakeNftablesTest : public ::testing::Test { protected: - void SetUp() override { - ASSERT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); - ASSERT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); + void CreateReadyChain() { + ASSERT_THAT(fake_.EnsureTable(kFamily, kTable), IsOk()); + ASSERT_THAT(fake_.EnsureChain(kFamily, kTable, kChain, ""), IsOk()); } FakeNftables fake_; }; TEST_F(FakeNftablesTest, EnsureTableIsIdempotent) { - EXPECT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); - EXPECT_TRUE(fake_.HasTable("ip", "t")); + ASSERT_THAT(fake_.EnsureTable(kFamily, kTable), IsOk()); + EXPECT_THAT(fake_.EnsureTable(kFamily, kTable), IsOk()); + EXPECT_TRUE(fake_.HasTable(kFamily, kTable)); } TEST_F(FakeNftablesTest, EnsureChainIsIdempotent) { - EXPECT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); - EXPECT_TRUE(fake_.HasChain("ip", "t", "c")); + ASSERT_THAT(fake_.EnsureTable(kFamily, kTable), IsOk()); + EXPECT_THAT(fake_.EnsureChain(kFamily, kTable, kChain, ""), IsOk()); + EXPECT_THAT(fake_.EnsureChain(kFamily, kTable, kChain, ""), IsOk()); + EXPECT_TRUE(fake_.HasChain(kFamily, kTable, kChain)); } TEST_F(FakeNftablesTest, EnsureChainRequiresTable) { - EXPECT_THAT(fake_.EnsureChain("ip", "missing", "c", ""), IsError()); + EXPECT_THAT(fake_.EnsureChain(kFamily, "missing", kChain, ""), IsError()); } TEST_F(FakeNftablesTest, AddRuleRequiresTableAndChain) { - EXPECT_THAT(fake_.AddRule("ip", "missing", "c", "content", "cmt"), IsError()); - EXPECT_THAT(fake_.AddRule("ip", "t", "missing", "content", "cmt"), IsError()); + CreateReadyChain(); + EXPECT_THAT(fake_.AddRule(kFamily, "missing", kChain, "content", "cmt"), + IsError()); + EXPECT_THAT(fake_.AddRule(kFamily, kTable, "missing", "content", "cmt"), + IsError()); } TEST_F(FakeNftablesTest, HandlesAreUniqueAndMonotonic) { - Result h1 = fake_.AddRule("ip", "t", "c", "content1", "a"); - Result h2 = fake_.AddRule("ip", "t", "c", "content2", "b"); + CreateReadyChain(); + Result h1 = fake_.AddRule(kFamily, kTable, kChain, "content1", "a"); + Result h2 = fake_.AddRule(kFamily, kTable, kChain, "content2", "b"); ASSERT_THAT(h1, IsOk()); ASSERT_THAT(h2, IsOk()); - EXPECT_NE(*h1, *h2); EXPECT_LT(*h1, *h2); } TEST_F(FakeNftablesTest, DeleteRuleByHandleRemovesOnlyThatRule) { - Result h1 = fake_.AddRule("ip", "t", "c", "content1", "a"); + CreateReadyChain(); + Result h1 = fake_.AddRule(kFamily, kTable, kChain, "content1", "a"); ASSERT_THAT(h1, IsOk()); - ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content2", "b"), IsOk()); - EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 2); + ASSERT_THAT(fake_.AddRule(kFamily, kTable, kChain, "content2", "b"), IsOk()); + EXPECT_EQ(fake_.RuleCount(kFamily, kTable, kChain), 2); - EXPECT_THAT(fake_.DeleteRule("ip", "t", "c", *h1), IsOk()); - EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); - EXPECT_FALSE(fake_.HasRuleWithComment("ip", "t", "c", "a")); - EXPECT_TRUE(fake_.HasRuleWithComment("ip", "t", "c", "b")); + EXPECT_THAT(fake_.DeleteRule(kFamily, kTable, kChain, *h1), IsOk()); + EXPECT_EQ(fake_.RuleCount(kFamily, kTable, kChain), 1); + EXPECT_FALSE(fake_.HasRuleWithComment(kFamily, kTable, kChain, "a")); + EXPECT_TRUE(fake_.HasRuleWithComment(kFamily, kTable, kChain, "b")); } TEST_F(FakeNftablesTest, DeleteRuleWithUnknownHandleFails) { - EXPECT_THAT(fake_.DeleteRule("ip", "t", "c", 9999), IsError()); + CreateReadyChain(); + EXPECT_THAT(fake_.DeleteRule(kFamily, kTable, kChain, 9999), IsError()); } TEST_F(FakeNftablesTest, DeleteRulesByCommentRemovesAllMatching) { - ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content1", "shared"), IsOk()); - ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content2", "shared"), IsOk()); - ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content3", "other"), IsOk()); - - EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "c", "shared"), IsOk()); - EXPECT_FALSE(fake_.HasRuleWithComment("ip", "t", "c", "shared")); - EXPECT_TRUE(fake_.HasRuleWithComment("ip", "t", "c", "other")); - EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); + CreateReadyChain(); + ASSERT_THAT(fake_.AddRule(kFamily, kTable, kChain, "content1", "shared"), + IsOk()); + ASSERT_THAT(fake_.AddRule(kFamily, kTable, kChain, "content2", "shared"), + IsOk()); + ASSERT_THAT(fake_.AddRule(kFamily, kTable, kChain, "content3", "other"), + IsOk()); + + EXPECT_THAT(fake_.DeleteRulesByComment(kFamily, kTable, kChain, "shared"), + IsOk()); + EXPECT_FALSE(fake_.HasRuleWithComment(kFamily, kTable, kChain, "shared")); + EXPECT_TRUE(fake_.HasRuleWithComment(kFamily, kTable, kChain, "other")); + EXPECT_EQ(fake_.RuleCount(kFamily, kTable, kChain), 1); } TEST_F(FakeNftablesTest, DeleteRulesByCommentIsIdempotent) { + CreateReadyChain(); // No matching comment, existing chain: no-op success. - EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "c", "nope"), IsOk()); + EXPECT_THAT(fake_.DeleteRulesByComment(kFamily, kTable, kChain, "nope"), + IsOk()); // Missing chain and missing table: still no-op success. - EXPECT_THAT(fake_.DeleteRulesByComment("ip", "t", "missing", "x"), IsOk()); - EXPECT_THAT(fake_.DeleteRulesByComment("ip", "missing", "c", "x"), IsOk()); + EXPECT_THAT(fake_.DeleteRulesByComment(kFamily, kTable, "missing", "x"), + IsOk()); + EXPECT_THAT(fake_.DeleteRulesByComment(kFamily, "missing", kChain, "x"), + IsOk()); } TEST_F(FakeNftablesTest, DeleteTableFailsWhenMissing) { - EXPECT_THAT(fake_.DeleteTable("ip", "missing"), IsError()); + EXPECT_THAT(fake_.DeleteTable(kFamily, "missing"), IsError()); } TEST_F(FakeNftablesTest, RecreatingTableResetsHandleCounter) { - Result first = fake_.AddRule("ip", "t", "c", "content", "a"); + CreateReadyChain(); + Result first = + fake_.AddRule(kFamily, kTable, kChain, "content", "a"); ASSERT_THAT(first, IsOk()); - ASSERT_THAT(fake_.DeleteTable("ip", "t"), IsOk()); - EXPECT_FALSE(fake_.HasTable("ip", "t")); + ASSERT_THAT(fake_.DeleteTable(kFamily, kTable), IsOk()); + EXPECT_FALSE(fake_.HasTable(kFamily, kTable)); - ASSERT_THAT(fake_.EnsureTable("ip", "t"), IsOk()); - ASSERT_THAT(fake_.EnsureChain("ip", "t", "c", ""), IsOk()); - Result again = fake_.AddRule("ip", "t", "c", "content", "a"); - ASSERT_THAT(again, IsOk()); - EXPECT_EQ(*first, *again); + CreateReadyChain(); + Result again = + fake_.AddRule(kFamily, kTable, kChain, "content", "a"); + EXPECT_THAT(again, IsOkAndValue(*first)); } TEST_F(FakeNftablesTest, TablesAreIsolatedByFamilyAndName) { - ASSERT_THAT(fake_.EnsureTable("bridge", "t"), IsOk()); - ASSERT_THAT(fake_.EnsureChain("bridge", "t", "c", ""), IsOk()); - ASSERT_THAT(fake_.AddRule("ip", "t", "c", "content", "a"), IsOk()); + CreateReadyChain(); + ASSERT_THAT(fake_.EnsureTable("bridge", kTable), IsOk()); + ASSERT_THAT(fake_.EnsureChain("bridge", kTable, kChain, ""), IsOk()); + ASSERT_THAT(fake_.AddRule(kFamily, kTable, kChain, "content", "a"), IsOk()); - EXPECT_EQ(fake_.RuleCount("ip", "t", "c"), 1); - EXPECT_EQ(fake_.RuleCount("bridge", "t", "c"), 0); + EXPECT_EQ(fake_.RuleCount(kFamily, kTable, kChain), 1); + EXPECT_EQ(fake_.RuleCount("bridge", kTable, kChain), 0); } } // namespace