commit ddf00848aff6f2c9ea36b56c59f43e34614fe06d
author Hugo Benichi <hugobenichi@google.com> Fri Nov 20 10:24:08 2020 +0900
committer Commit Bot <commit-bot@chromium.org> Wed Dec 09 13:35:22 2020 +0000
tree 780d926ea0125a139a32ddc08645e3cbf8d6a914
parent b721a80ef01731a173267b863c5ee529d504d701

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>

patchpanel/counters_service_test.cc

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.