patchpanel: simplify traffic account jump rules
This patch simplifies the creation of traffic accounting jump rules by
adding a single check point for returning early if traffic account rules
for a given interface already exist.
BUG=b:161060333
BUG=b:160112868
TEST=Unit tests. Checked GetTrafficCounters still work with:
$ dbus-send --system --dest=org.chromium.PatchPanel --print-reply \
/org/chromium/PatchPanel org.chromium.PatchPanel.GetTrafficCounters
Checked that accounting chains and jump rules for eth0 are created only
once when plugging in and out an eth adapter multiple times.
Change-Id: I60afecf58466177686b815726b7b5f4ff4d7ce93
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2562980
Reviewed-by: Jie Jiang <jiejiang@chromium.org>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Tested-by: Hugo Benichi <hugobenichi@google.com>
diff --git a/patchpanel/counters_service.cc b/patchpanel/counters_service.cc
index 8e30f86..7c631bb 100644
--- a/patchpanel/counters_service.cc
+++ b/patchpanel/counters_service.cc
@@ -170,22 +170,8 @@
}
void CountersService::IptablesNewRule(std::vector<std::string> params) {
- DCHECK_GT(params.size(), 0);
- const std::string action = params[0];
- DCHECK(action == "-I" || action == "-A");
- params.emplace_back("-w");
-
- params[0] = "-C";
- if (runner_->iptables(kMangleTable, params, false /*log_failures*/) != 0) {
- params[0] = action;
- runner_->iptables(kMangleTable, params);
- }
-
- params[0] = "-C";
- if (runner_->ip6tables(kMangleTable, params, false /*log_failures*/) != 0) {
- params[0] = action;
- runner_->ip6tables(kMangleTable, params);
- }
+ runner_->iptables(kMangleTable, params);
+ runner_->ip6tables(kMangleTable, params);
}
void CountersService::SetupChainsAndRules(const std::string& ifname) {
@@ -197,24 +183,35 @@
// Note that the length of a chain name must less than 29 chars and IFNAMSIZ
// is 16 so we can only use at most 12 chars for the prefix.
- // Ingress traffic chain.
const std::string ingress_chain = "rx_" + ifname;
- MakeAccountingChain(ingress_chain);
- IptablesNewRule({"-A", "FORWARD", "-i", ifname, "-j", ingress_chain});
- IptablesNewRule({"-A", "INPUT", "-i", ifname, "-j", ingress_chain});
- SetupAccountingRules(ingress_chain);
-
- // Egress traffic chain.
const std::string egress_chain = "tx_" + ifname;
- MakeAccountingChain(egress_chain);
- IptablesNewRule({"-A", "POSTROUTING", "-o", ifname, "-j", egress_chain});
+
+ // Creates egress and ingress traffic chains, or stops if they already exist.
+ if (!MakeAccountingChain(ingress_chain) ||
+ !MakeAccountingChain(egress_chain)) {
+ LOG(INFO) << "Traffic accounting chains already exist for " << ifname;
+ return;
+ }
+
+ // Add jump rules
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "FORWARD", "-i", ifname, "-j", ingress_chain, "-w"});
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "INPUT", "-i", ifname, "-j", ingress_chain, "-w"});
+ datapath_->ModifyIptables(
+ IpFamily::Dual, kMangleTable,
+ {"-A", "POSTROUTING", "-o", ifname, "-j", egress_chain, "-w"});
+
+ SetupAccountingRules(ingress_chain);
SetupAccountingRules(egress_chain);
}
void CountersService::SetupAccountingRules(const std::string& chain_name) {
// TODO(jiejiang): This function will be extended to matching on fwmark for
// different sources.
- IptablesNewRule({"-A", chain_name});
+ IptablesNewRule({"-A", chain_name, "-w"});
}
} // namespace patchpanel
diff --git a/patchpanel/counters_service_test.cc b/patchpanel/counters_service_test.cc
index b810256..6e6bcc4 100644
--- a/patchpanel/counters_service_test.cc
+++ b/patchpanel/counters_service_test.cc
@@ -170,12 +170,6 @@
};
TEST_F(CountersServiceTest, OnNewDevice) {
- // Makes the check commands return 1 (not found).
- EXPECT_CALL(runner_, iptables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(1));
- EXPECT_CALL(runner_, ip6tables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(1));
-
// The following commands are expected when eth0 comes up.
const std::vector<std::vector<std::string>> expected_calls{
{"-N", "rx_eth0", "-w"},
@@ -199,16 +193,19 @@
}
TEST_F(CountersServiceTest, OnSameDeviceAppearAgain) {
- // Makes the check commands return 0 (we already have these rules).
- EXPECT_CALL(runner_, iptables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(0));
- EXPECT_CALL(runner_, ip6tables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(0));
+ // Makes the chain creation commands return false (we already have these
+ // rules).
+ EXPECT_CALL(runner_, iptables(_, Contains("-N"), _, _))
+ .WillRepeatedly(Return(1));
+ EXPECT_CALL(runner_, ip6tables(_, Contains("-N"), _, _))
+ .WillRepeatedly(Return(1));
// Creating chains commands are expected but no more creating rules command
// (with "-I" or "-A") should come.
- EXPECT_CALL(runner_, iptables(_, Contains("-N"), _, _)).Times(AnyNumber());
- EXPECT_CALL(runner_, ip6tables(_, Contains("-N"), _, _)).Times(AnyNumber());
+ EXPECT_CALL(runner_, iptables(_, Contains("-A"), _, _)).Times(0);
+ EXPECT_CALL(runner_, ip6tables(_, Contains("-A"), _, _)).Times(0);
+ EXPECT_CALL(runner_, iptables(_, Contains("-I"), _, _)).Times(0);
+ EXPECT_CALL(runner_, ip6tables(_, Contains("-I"), _, _)).Times(0);
std::vector<dbus::ObjectPath> devices = {dbus::ObjectPath("/device/eth0")};
fake_shill_client_->NotifyManagerPropertyChange(shill::kDevicesProperty,
@@ -216,12 +213,6 @@
}
TEST_F(CountersServiceTest, ChainNameLength) {
- // Makes the check commands return 1 (not found).
- EXPECT_CALL(runner_, iptables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(1));
- EXPECT_CALL(runner_, ip6tables(_, Contains("-C"), _, _))
- .WillRepeatedly(Return(1));
-
// The name of a new chain must be shorter than 29 characters, otherwise
// iptables will reject the request. Uses Each() here for simplicity since no
// other params could be longer than 29 for now.
diff --git a/patchpanel/datapath.h b/patchpanel/datapath.h
index adde6d0..a638747 100644
--- a/patchpanel/datapath.h
+++ b/patchpanel/datapath.h
@@ -247,6 +247,11 @@
const std::string& op,
const std::string& chain,
bool log_failures = true);
+ // Sends an iptables command for table |table|.
+ bool ModifyIptables(IpFamily family,
+ const std::string& table,
+ const std::vector<std::string>& argv,
+ bool log_failures = true);
MinijailedProcessRunner& runner() const;
@@ -342,10 +347,6 @@
const std::string& iif,
Fwmark mark,
Fwmark mask);
- bool ModifyIptables(IpFamily family,
- const std::string& table,
- const std::vector<std::string>& argv,
- bool log_failures = true);
bool ModifyRtentry(ioctl_req_t op, struct rtentry* route);
int FindIfIndex(const std::string& ifname);