From 8b3af8d775c57c2706838637613594ce0d3adb61 Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Tue, 19 May 2020 15:23:56 +0800 Subject: [PATCH] [BOT.12] Add unit test for disabling BpfCoordinator by config Bug: 150736748 Test: BpfCoordinatorTest Change-Id: Iedb936b7592b6be773d1b84a2498bfc5a440a198 --- .../tethering/BpfCoordinator.java | 39 +++++++--- .../networkstack/tethering/Tethering.java | 5 +- .../tethering/TetheringConfiguration.java | 14 ++-- .../unit/src/android/net/ip/IpServerTest.java | 7 +- .../tethering/BpfCoordinatorTest.java | 74 +++++++++++++++++-- .../tethering/TetheringConfigurationTest.java | 8 +- 6 files changed, 112 insertions(+), 35 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index aa1d59de89..12464e1289 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -94,7 +94,7 @@ public class BpfCoordinator { // in the constructor because it is hard to unwind all existing change once device // configuration is changed. Especially the forwarding rules. Keep the same setting // to make it simpler. See also TetheringConfiguration. - private final boolean mUsingBpf; + private final boolean mIsBpfEnabled; // Tracks whether BPF tethering is started or not. This is set by tethering before it // starts the first IpServer and is cleared by tethering shortly before the last IpServer @@ -184,7 +184,7 @@ public class BpfCoordinator { mHandler = mDeps.getHandler(); mNetd = mDeps.getNetd(); mLog = mDeps.getSharedLog().forSubComponent(TAG); - mUsingBpf = isOffloadEnabled(); + mIsBpfEnabled = isBpfEnabled(); BpfTetherStatsProvider provider = new BpfTetherStatsProvider(); try { mDeps.getNetworkStatsManager().registerNetworkStatsProvider( @@ -207,7 +207,7 @@ public class BpfCoordinator { public void startPolling() { if (mPollingStarted) return; - if (!mUsingBpf) { + if (!mIsBpfEnabled) { mLog.i("Offload disabled"); return; } @@ -246,7 +246,7 @@ public class BpfCoordinator { */ public void tetherOffloadRuleAdd( @NonNull final IpServer ipServer, @NonNull final Ipv6ForwardingRule rule) { - if (!mUsingBpf) return; + if (!mIsBpfEnabled) return; try { // TODO: Perhaps avoid to add a duplicate rule. @@ -287,7 +287,7 @@ public class BpfCoordinator { */ public void tetherOffloadRuleRemove( @NonNull final IpServer ipServer, @NonNull final Ipv6ForwardingRule rule) { - if (!mUsingBpf) return; + if (!mIsBpfEnabled) return; try { // TODO: Perhaps avoid to remove a non-existent rule. @@ -332,7 +332,7 @@ public class BpfCoordinator { * Note that this can be only called on handler thread. */ public void tetherOffloadRuleClear(@NonNull final IpServer ipServer) { - if (!mUsingBpf) return; + if (!mIsBpfEnabled) return; final LinkedHashMap rules = mIpv6ForwardingRules.get( ipServer); @@ -349,7 +349,7 @@ public class BpfCoordinator { * Note that this can be only called on handler thread. */ public void tetherOffloadRuleUpdate(@NonNull final IpServer ipServer, int newUpstreamIfindex) { - if (!mUsingBpf) return; + if (!mIsBpfEnabled) return; final LinkedHashMap rules = mIpv6ForwardingRules.get( ipServer); @@ -373,7 +373,7 @@ public class BpfCoordinator { * Note that this can be only called on handler thread. */ public void addUpstreamNameToLookupTable(int upstreamIfindex, @NonNull String upstreamIface) { - if (!mUsingBpf) return; + if (!mIsBpfEnabled) return; if (upstreamIfindex == 0 || TextUtils.isEmpty(upstreamIface)) return; @@ -398,7 +398,7 @@ public class BpfCoordinator { public void dump(@NonNull IndentingPrintWriter pw) { final ConditionVariable dumpDone = new ConditionVariable(); mHandler.post(() -> { - pw.println("mUsingBpf: " + mUsingBpf); + pw.println("mIsBpfEnabled: " + mIsBpfEnabled); pw.println("Polling " + (mPollingStarted ? "started" : "not started")); pw.println("Stats provider " + (mStatsProvider != null ? "registered" : "not registered")); @@ -589,9 +589,9 @@ public class BpfCoordinator { } } - private boolean isOffloadEnabled() { + private boolean isBpfEnabled() { final TetheringConfiguration config = mDeps.getTetherConfig(); - return (config != null) ? config.enableBpfOffload : true /* default value */; + return (config != null) ? config.isBpfOffloadEnabled() : true /* default value */; } private int getInterfaceIndexFromRules(@NonNull String ifName) { @@ -754,4 +754,21 @@ public class BpfCoordinator { mHandler.postDelayed(mScheduledPollingTask, mDeps.getPerformPollInterval()); } + + // Return forwarding rule map. This is used for testing only. + // Note that this can be only called on handler thread. + @NonNull + @VisibleForTesting + final HashMap> + getForwardingRulesForTesting() { + return mIpv6ForwardingRules; + } + + // Return upstream interface name map. This is used for testing only. + // Note that this can be only called on handler thread. + @NonNull + @VisibleForTesting + final SparseArray getInterfaceNamesForTesting() { + return mInterfaceNames; + } } diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 974f799c50..71ab176764 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -339,8 +339,7 @@ public class Tethering { @NonNull public NetworkStatsManager getNetworkStatsManager() { - return (NetworkStatsManager) mContext.getSystemService( - Context.NETWORK_STATS_SERVICE); + return mContext.getSystemService(NetworkStatsManager.class); } @NonNull @@ -2386,7 +2385,7 @@ public class Tethering { final TetherState tetherState = new TetherState( new IpServer(iface, mLooper, interfaceType, mLog, mNetd, mBpfCoordinator, makeControlCallback(), mConfig.enableLegacyDhcpServer, - mConfig.enableBpfOffload, mPrivateAddressCoordinator, + mConfig.isBpfOffloadEnabled(), mPrivateAddressCoordinator, mDeps.getIpServerDependencies())); mTetherStates.put(iface, tetherState); tetherState.ipServer.start(); diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java b/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java index 48a600dfe6..1e94de12ce 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java @@ -101,8 +101,6 @@ public class TetheringConfiguration { public final String[] legacyDhcpRanges; public final String[] defaultIPv4DNS; public final boolean enableLegacyDhcpServer; - // TODO: Add to TetheringConfigurationParcel if required. - public final boolean enableBpfOffload; public final String[] provisioningApp; public final String provisioningAppNoUi; @@ -111,6 +109,8 @@ public class TetheringConfiguration { public final int activeDataSubId; private final int mOffloadPollInterval; + // TODO: Add to TetheringConfigurationParcel if required. + private final boolean mEnableBpfOffload; public TetheringConfiguration(Context ctx, SharedLog log, int id) { final SharedLog configLog = log.forSubComponent("config"); @@ -137,7 +137,7 @@ public class TetheringConfiguration { legacyDhcpRanges = getLegacyDhcpRanges(res); defaultIPv4DNS = copy(DEFAULT_IPV4_DNS); - enableBpfOffload = getEnableBpfOffload(res); + mEnableBpfOffload = getEnableBpfOffload(res); enableLegacyDhcpServer = getEnableLegacyDhcpServer(res); provisioningApp = getResourceStringArray(res, R.array.config_mobile_hotspot_provision_app); @@ -218,7 +218,7 @@ public class TetheringConfiguration { pw.println(provisioningAppNoUi); pw.print("enableBpfOffload: "); - pw.println(enableBpfOffload); + pw.println(mEnableBpfOffload); pw.print("enableLegacyDhcpServer: "); pw.println(enableLegacyDhcpServer); @@ -240,7 +240,7 @@ public class TetheringConfiguration { toIntArray(preferredUpstreamIfaceTypes))); sj.add(String.format("provisioningApp:%s", makeString(provisioningApp))); sj.add(String.format("provisioningAppNoUi:%s", provisioningAppNoUi)); - sj.add(String.format("enableBpfOffload:%s", enableBpfOffload)); + sj.add(String.format("enableBpfOffload:%s", mEnableBpfOffload)); sj.add(String.format("enableLegacyDhcpServer:%s", enableLegacyDhcpServer)); return String.format("TetheringConfiguration{%s}", sj.toString()); } @@ -279,6 +279,10 @@ public class TetheringConfiguration { return mOffloadPollInterval; } + public boolean isBpfOffloadEnabled() { + return mEnableBpfOffload; + } + private static Collection getUpstreamIfaceTypes(Resources res, boolean dunRequired) { final int[] ifaceTypes = res.getIntArray(R.array.config_tether_upstream_types); final ArrayList upstreamIfaceTypes = new ArrayList<>(ifaceTypes.length); diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index fdfd92617b..4f88605391 100644 --- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -144,6 +144,7 @@ public class IpServerTest { @Mock private IpServer.Dependencies mDependencies; @Mock private PrivateAddressCoordinator mAddressCoordinator; @Mock private NetworkStatsManager mStatsManager; + @Mock private TetheringConfiguration mTetherConfig; @Captor private ArgumentCaptor mDhcpParamsCaptor; @@ -227,6 +228,7 @@ public class IpServerTest { MockitoAnnotations.initMocks(this); when(mSharedLog.forSubComponent(anyString())).thenReturn(mSharedLog); when(mAddressCoordinator.requestDownstreamAddress(any())).thenReturn(mTestAddress); + when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(true /* default value */); mBpfCoordinator = spy(new BpfCoordinator( new BpfCoordinator.Dependencies() { @@ -252,10 +254,7 @@ public class IpServerTest { @Nullable public TetheringConfiguration getTetherConfig() { - // Returning null configuration object is a hack to enable BPF offload. - // See BpfCoordinator#isOffloadEnabled. - // TODO: Mock TetheringConfiguration to test. - return null; + return mTetherConfig; } })); } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java index f63a473f1d..31d98147c7 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java @@ -31,10 +31,11 @@ import static com.android.networkstack.tethering.BpfCoordinator.StatsType; import static com.android.networkstack.tethering.BpfCoordinator.StatsType.STATS_PER_IFACE; import static com.android.networkstack.tethering.BpfCoordinator.StatsType.STATS_PER_UID; -import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertNotNull; -import static junit.framework.Assert.fail; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyString; @@ -78,6 +79,7 @@ import java.net.Inet6Address; import java.net.InetAddress; import java.util.ArrayList; import java.util.Arrays; +import java.util.LinkedHashMap; @RunWith(AndroidJUnit4.class) @SmallTest @@ -92,6 +94,7 @@ public class BpfCoordinatorTest { @Mock private NetworkStatsManager mStatsManager; @Mock private INetd mNetd; @Mock private IpServer mIpServer; + @Mock private TetheringConfiguration mTetherConfig; // Late init since methods must be called by the thread that created this object. private TestableNetworkStatsProviderCbBinder mTetherStatsProviderCb; @@ -128,15 +131,13 @@ public class BpfCoordinatorTest { @Nullable public TetheringConfiguration getTetherConfig() { - // Returning null configuration object is a hack to enable BPF offload. - // See BpfCoordinator#isOffloadEnabled. - // TODO: Mock TetheringConfiguration to test. - return null; + return mTetherConfig; } }; @Before public void setUp() { MockitoAnnotations.initMocks(this); + when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(true /* default value */); } private void waitForIdle() { @@ -501,4 +502,61 @@ public class BpfCoordinatorTest { .addEntry(buildTestEntry(STATS_PER_UID, ethIface, 10, 20, 30, 40)) .addEntry(buildTestEntry(STATS_PER_UID, mobileIface, 50, 60, 70, 80))); } + + @Test + public void testTetheringConfigDisable() throws Exception { + setupFunctioningNetdInterface(); + when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(false); + + final BpfCoordinator coordinator = makeBpfCoordinator(); + coordinator.startPolling(); + + // The tether stats polling task should not be scheduled. + mTestLooper.moveTimeForward(DEFAULT_PERFORM_POLL_INTERVAL_MS); + waitForIdle(); + verify(mNetd, never()).tetherOffloadGetStats(); + + // The interface name lookup table can't be added. + final String iface = "rmnet_data0"; + final Integer ifIndex = 100; + coordinator.addUpstreamNameToLookupTable(ifIndex, iface); + assertEquals(0, coordinator.getInterfaceNamesForTesting().size()); + + // The rule can't be added. + final InetAddress neigh = InetAddresses.parseNumericAddress("2001:db8::1"); + final MacAddress mac = MacAddress.fromString("00:00:00:00:00:0a"); + final Ipv6ForwardingRule rule = buildTestForwardingRule(ifIndex, neigh, mac); + coordinator.tetherOffloadRuleAdd(mIpServer, rule); + verify(mNetd, never()).tetherOffloadRuleAdd(any()); + LinkedHashMap rules = + coordinator.getForwardingRulesForTesting().get(mIpServer); + assertNull(rules); + + // The rule can't be removed. This is not a realistic case because adding rule is not + // allowed. That implies no rule could be removed, cleared or updated. Verify these + // cases just in case. + rules = new LinkedHashMap(); + rules.put(rule.address, rule); + coordinator.getForwardingRulesForTesting().put(mIpServer, rules); + coordinator.tetherOffloadRuleRemove(mIpServer, rule); + verify(mNetd, never()).tetherOffloadRuleRemove(any()); + rules = coordinator.getForwardingRulesForTesting().get(mIpServer); + assertNotNull(rules); + assertEquals(1, rules.size()); + + // The rule can't be cleared. + coordinator.tetherOffloadRuleClear(mIpServer); + verify(mNetd, never()).tetherOffloadRuleRemove(any()); + rules = coordinator.getForwardingRulesForTesting().get(mIpServer); + assertNotNull(rules); + assertEquals(1, rules.size()); + + // The rule can't be updated. + coordinator.tetherOffloadRuleUpdate(mIpServer, rule.upstreamIfindex + 1 /* new */); + verify(mNetd, never()).tetherOffloadRuleRemove(any()); + verify(mNetd, never()).tetherOffloadRuleAdd(any()); + rules = coordinator.getForwardingRulesForTesting().get(mIpServer); + assertNotNull(rules); + assertEquals(1, rules.size()); + } } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java index 1999ad786e..3f4cfe7afb 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java @@ -292,7 +292,7 @@ public class TetheringConfigurationTest { initializeBpfOffloadConfiguration(true, null /* unset */); final TetheringConfiguration enableByRes = new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); - assertTrue(enableByRes.enableBpfOffload); + assertTrue(enableByRes.isBpfOffloadEnabled()); } @Test @@ -301,7 +301,7 @@ public class TetheringConfigurationTest { initializeBpfOffloadConfiguration(res, "true"); final TetheringConfiguration enableByDevConOverride = new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); - assertTrue(enableByDevConOverride.enableBpfOffload); + assertTrue(enableByDevConOverride.isBpfOffloadEnabled()); } } @@ -310,7 +310,7 @@ public class TetheringConfigurationTest { initializeBpfOffloadConfiguration(false, null /* unset */); final TetheringConfiguration disableByRes = new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); - assertFalse(disableByRes.enableBpfOffload); + assertFalse(disableByRes.isBpfOffloadEnabled()); } @Test @@ -319,7 +319,7 @@ public class TetheringConfigurationTest { initializeBpfOffloadConfiguration(res, "false"); final TetheringConfiguration disableByDevConOverride = new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); - assertFalse(disableByDevConOverride.enableBpfOffload); + assertFalse(disableByDevConOverride.isBpfOffloadEnabled()); } }