From b95083ea6a042c11b5308ff95337384de9a994ed Mon Sep 17 00:00:00 2001 From: junyulai Date: Thu, 6 May 2021 15:42:17 +0800 Subject: [PATCH] No-op Refactoring of startTrackDefaultNetwork While the actual part that track default request is inside UpstreamNetworkMonitor, instead of passing it from Tethering, move it into counter-part CL and use it from UpstreamNetworkMonitor. Since the current code is replaced by registerSystemDefaultCallback in Android S, implement it inside the api30 shim implementation to provide an unified interface to tethering. Test: atest TetheringCoverageTests Bug: 185952829 Change-Id: Iaf21b6b662aa6aba79c2b75379128b8523f81f02 --- Tethering/Android.bp | 1 + .../networkstack/tethering/Tethering.java | 3 +- .../tethering/TetheringDependencies.java | 6 --- .../tethering/TetheringService.java | 15 -------- .../tethering/UpstreamNetworkMonitor.java | 38 +++++++++---------- .../tethering/TestConnectivityManager.java | 22 ++++++++--- .../networkstack/tethering/TetheringTest.java | 31 +++------------ .../tethering/UpstreamNetworkMonitorTest.java | 36 ++++++++++-------- 8 files changed, 63 insertions(+), 89 deletions(-) diff --git a/Tethering/Android.bp b/Tethering/Android.bp index 4eafc2afa9..79c2b9dd17 100644 --- a/Tethering/Android.bp +++ b/Tethering/Android.bp @@ -30,6 +30,7 @@ java_defaults { ":services-tethering-shared-srcs", ], static_libs: [ + "NetworkStackApiStableShims", "androidx.annotation_annotation", "modules-utils-build", "netlink-client", diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 8db4cb99be..ce80b5a961 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -2110,8 +2110,7 @@ public class Tethering { } private void startTrackDefaultNetwork() { - mUpstreamNetworkMonitor.startTrackDefaultNetwork(mDeps.getDefaultNetworkRequest(), - mEntitlementMgr); + mUpstreamNetworkMonitor.startTrackDefaultNetwork(mEntitlementMgr); } /** Get the latest value of the tethering entitlement check. */ diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringDependencies.java b/Tethering/src/com/android/networkstack/tethering/TetheringDependencies.java index 45b914178e..7df9475153 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringDependencies.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringDependencies.java @@ -20,7 +20,6 @@ import android.app.usage.NetworkStatsManager; import android.bluetooth.BluetoothAdapter; import android.content.Context; import android.net.INetd; -import android.net.NetworkRequest; import android.net.ip.IpServer; import android.net.util.SharedLog; import android.os.Handler; @@ -98,11 +97,6 @@ public abstract class TetheringDependencies { return true; } - /** - * Get the NetworkRequest that should be fulfilled by the default network. - */ - public abstract NetworkRequest getDefaultNetworkRequest(); - /** * Get a reference to the EntitlementManager to be used by tethering. */ diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringService.java b/Tethering/src/com/android/networkstack/tethering/TetheringService.java index e36df7fac2..b7fd01ef09 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringService.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringService.java @@ -35,8 +35,6 @@ import android.net.IIntResultListener; import android.net.INetworkStackConnector; import android.net.ITetheringConnector; import android.net.ITetheringEventCallback; -import android.net.NetworkCapabilities; -import android.net.NetworkRequest; import android.net.NetworkStack; import android.net.TetheringRequestParcel; import android.net.dhcp.DhcpServerCallbacks; @@ -308,19 +306,6 @@ public class TetheringService extends Service { @VisibleForTesting public TetheringDependencies makeTetheringDependencies() { return new TetheringDependencies() { - @Override - public NetworkRequest getDefaultNetworkRequest() { - // TODO: b/147280869, add a proper system API to replace this. - final NetworkRequest trackDefaultRequest = new NetworkRequest.Builder() - .clearCapabilities() - .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED) - .addCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED) - .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) - .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) - .build(); - return trackDefaultRequest; - } - @Override public Looper getTetheringLooper() { final HandlerThread tetherThread = new HandlerThread("android.tethering"); diff --git a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java index 9de4c8799f..e615334bbb 100644 --- a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java +++ b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java @@ -47,6 +47,9 @@ import androidx.annotation.Nullable; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.StateMachine; +import com.android.networkstack.apishim.ConnectivityManagerShimImpl; +import com.android.networkstack.apishim.common.ConnectivityManagerShim; +import com.android.networkstack.apishim.common.UnsupportedApiLevelException; import java.util.HashMap; import java.util.HashSet; @@ -142,33 +145,28 @@ public class UpstreamNetworkMonitor { mWhat = what; mLocalPrefixes = new HashSet<>(); mIsDefaultCellularUpstream = false; - } - - @VisibleForTesting - public UpstreamNetworkMonitor( - ConnectivityManager cm, StateMachine tgt, SharedLog log, int what) { - this((Context) null, tgt, log, what); - mCM = cm; + mCM = (ConnectivityManager) ctx.getSystemService(Context.CONNECTIVITY_SERVICE); } /** - * Tracking the system default network. This method should be called when system is ready. + * Tracking the system default network. This method should be only called once when system is + * ready, and the callback is never unregistered. * - * @param defaultNetworkRequest should be the same as ConnectivityService default request * @param entitle a EntitlementManager object to communicate between EntitlementManager and * UpstreamNetworkMonitor */ - public void startTrackDefaultNetwork(NetworkRequest defaultNetworkRequest, - EntitlementManager entitle) { - - // defaultNetworkRequest is not really a "request", just a way of tracking the system - // default network. It's guaranteed not to actually bring up any networks because it's - // the should be the same request as the ConnectivityService default request, and thus - // shares fate with it. We can't use registerDefaultNetworkCallback because it will not - // track the system default network if there is a VPN that applies to our UID. - if (mDefaultNetworkCallback == null) { - mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET); - cm().requestNetwork(defaultNetworkRequest, mDefaultNetworkCallback, mHandler); + public void startTrackDefaultNetwork(EntitlementManager entitle) { + if (mDefaultNetworkCallback != null) { + Log.wtf(TAG, "default network callback is already registered"); + return; + } + ConnectivityManagerShim mCmShim = ConnectivityManagerShimImpl.newInstance(mContext); + mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET); + try { + mCmShim.registerSystemDefaultNetworkCallback(mDefaultNetworkCallback, mHandler); + } catch (UnsupportedApiLevelException e) { + Log.wtf(TAG, "registerSystemDefaultNetworkCallback is not supported"); + return; } if (mEntitlementMgr == null) { mEntitlementMgr = entitle; diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java index df82d7cf22..6090213516 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java @@ -16,6 +16,10 @@ package com.android.networkstack.tethering; +import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; +import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; +import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; + import static org.junit.Assert.assertFalse; import static org.junit.Assert.fail; @@ -64,12 +68,13 @@ public class TestConnectivityManager extends ConnectivityManager { public static final boolean CALLBACKS_FIRST = true; final Map mAllCallbacks = new ArrayMap<>(); + // This contains the callbacks tracking the system default network, whether it's registered + // with registerSystemDefaultNetworkCallback (S+) or with a custom request (R-). final Map mTrackingDefault = new ArrayMap<>(); final Map mListening = new ArrayMap<>(); final Map mRequested = new ArrayMap<>(); final Map mLegacyTypeMap = new ArrayMap<>(); - private final NetworkRequest mDefaultRequest; private final Context mContext; private int mNetworkId = 100; @@ -80,13 +85,10 @@ public class TestConnectivityManager extends ConnectivityManager { * @param ctx the context to use. Must be a fake or a mock because otherwise the test will * attempt to send real broadcasts and resulting in permission denials. * @param svc an IConnectivityManager. Should be a fake or a mock. - * @param defaultRequest the default NetworkRequest that will be used by Tethering. */ - public TestConnectivityManager(Context ctx, IConnectivityManager svc, - NetworkRequest defaultRequest) { + public TestConnectivityManager(Context ctx, IConnectivityManager svc) { super(ctx, svc); mContext = ctx; - mDefaultRequest = defaultRequest; } class NetworkRequestInfo { @@ -181,11 +183,19 @@ public class TestConnectivityManager extends ConnectivityManager { makeDefaultNetwork(agent, BROADCAST_FIRST, null /* inBetween */); } + static boolean looksLikeDefaultRequest(NetworkRequest req) { + return req.hasCapability(NET_CAPABILITY_INTERNET) + && !req.hasCapability(NET_CAPABILITY_DUN) + && !req.hasTransport(TRANSPORT_CELLULAR); + } + @Override public void requestNetwork(NetworkRequest req, NetworkCallback cb, Handler h) { assertFalse(mAllCallbacks.containsKey(cb)); mAllCallbacks.put(cb, new NetworkRequestInfo(req, h)); - if (mDefaultRequest.equals(req)) { + // For R- devices, Tethering will invoke this function in 2 cases, one is to request mobile + // network, the other is to track system default network. + if (looksLikeDefaultRequest(req)) { assertFalse(mTrackingDefault.containsKey(cb)); mTrackingDefault.put(cb, new NetworkRequestInfo(req, h)); } else { diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index faa5ac7039..1dab22b111 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -26,7 +26,6 @@ import static android.net.ConnectivityManager.ACTION_RESTRICT_BACKGROUND_CHANGED import static android.net.ConnectivityManager.RESTRICT_BACKGROUND_STATUS_DISABLED; import static android.net.ConnectivityManager.RESTRICT_BACKGROUND_STATUS_ENABLED; import static android.net.ConnectivityManager.TYPE_MOBILE_DUN; -import static android.net.ConnectivityManager.TYPE_NONE; import static android.net.ConnectivityManager.TYPE_WIFI; import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; @@ -73,7 +72,6 @@ import static com.android.testutils.TestPermissionUtil.runAsShell; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.argThat; @@ -274,8 +272,6 @@ public class TetheringTest { private UpstreamNetworkMonitor mUpstreamNetworkMonitor; private TestConnectivityManager mCm; - private NetworkRequest mNetworkRequest; - private NetworkCallback mDefaultNetworkCallback; private class TestContext extends BroadcastInterceptingContext { TestContext(Context base) { @@ -449,11 +445,6 @@ public class TetheringTest { return mIpServerDependencies; } - @Override - public NetworkRequest getDefaultNetworkRequest() { - return mNetworkRequest; - } - @Override public EntitlementManager getEntitlementManager(Context ctx, Handler h, SharedLog log, Runnable callback) { @@ -645,15 +636,7 @@ public class TetheringTest { mServiceContext.registerReceiver(mBroadcastReceiver, new IntentFilter(ACTION_TETHER_STATE_CHANGED)); - // TODO: add NOT_VCN_MANAGED here, but more importantly in the production code. - // TODO: even better, change TetheringDependencies.getDefaultNetworkRequest() to use - // registerSystemDefaultNetworkCallback() on S and above. - NetworkCapabilities defaultCaps = new NetworkCapabilities() - .addCapability(NET_CAPABILITY_INTERNET); - mNetworkRequest = new NetworkRequest(defaultCaps, TYPE_NONE, 1 /* requestId */, - NetworkRequest.Type.REQUEST); - mCm = spy(new TestConnectivityManager(mServiceContext, mock(IConnectivityManager.class), - mNetworkRequest)); + mCm = spy(new TestConnectivityManager(mServiceContext, mock(IConnectivityManager.class))); mTethering = makeTethering(); verify(mStatsManager, times(1)).registerNetworkStatsProvider(anyString(), any()); @@ -793,15 +776,13 @@ public class TetheringTest { } private void verifyDefaultNetworkRequestFiled() { - ArgumentCaptor captor = ArgumentCaptor.forClass(NetworkCallback.class); - verify(mCm, times(1)).requestNetwork(eq(mNetworkRequest), - captor.capture(), any(Handler.class)); - mDefaultNetworkCallback = captor.getValue(); - assertNotNull(mDefaultNetworkCallback); - + ArgumentCaptor reqCaptor = ArgumentCaptor.forClass(NetworkRequest.class); + verify(mCm, times(1)).requestNetwork(reqCaptor.capture(), + any(NetworkCallback.class), any(Handler.class)); + assertTrue(TestConnectivityManager.looksLikeDefaultRequest(reqCaptor.getValue())); // The default network request is only ever filed once. verifyNoMoreInteractions(mCm); - mUpstreamNetworkMonitor.startTrackDefaultNetwork(mNetworkRequest, mEntitleMgr); + mUpstreamNetworkMonitor.startTrackDefaultNetwork(mEntitleMgr); verifyNoMoreInteractions(mCm); } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java index cee03655db..ce4ba85565 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java @@ -29,10 +29,10 @@ import static com.android.networkstack.tethering.UpstreamNetworkMonitor.TYPE_NON import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.eq; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; @@ -66,6 +66,7 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -83,10 +84,6 @@ public class UpstreamNetworkMonitorTest { private static final boolean INCLUDES = true; private static final boolean EXCLUDES = false; - // Actual contents of the request don't matter for this test. The lack of - // any specific TRANSPORT_* is sufficient to identify this request. - private static final NetworkRequest sDefaultRequest = new NetworkRequest.Builder().build(); - private static final NetworkCapabilities CELL_CAPABILITIES = new NetworkCapabilities.Builder() .addTransportType(TRANSPORT_CELLULAR).addCapability(NET_CAPABILITY_INTERNET).build(); private static final NetworkCapabilities DUN_CAPABILITIES = new NetworkCapabilities.Builder() @@ -113,9 +110,10 @@ public class UpstreamNetworkMonitorTest { when(mLog.forSubComponent(anyString())).thenReturn(mLog); when(mEntitleMgr.isCellularUpstreamPermitted()).thenReturn(true); - mCM = spy(new TestConnectivityManager(mContext, mCS, sDefaultRequest)); + mCM = spy(new TestConnectivityManager(mContext, mCS)); + when(mContext.getSystemService(eq(Context.CONNECTIVITY_SERVICE))).thenReturn(mCM); mSM = new TestStateMachine(mLooper.getLooper()); - mUNM = new UpstreamNetworkMonitor(mCM, mSM, mLog, EVENT_UNM_UPDATE); + mUNM = new UpstreamNetworkMonitor(mContext, mSM, mLog, EVENT_UNM_UPDATE); } @After public void tearDown() throws Exception { @@ -146,7 +144,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testDefaultNetworkIsTracked() throws Exception { assertTrue(mCM.hasNoCallbacks()); - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); assertEquals(1, mCM.mTrackingDefault.size()); @@ -159,7 +157,7 @@ public class UpstreamNetworkMonitorTest { public void testListensForAllNetworks() throws Exception { assertTrue(mCM.mListening.isEmpty()); - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); assertFalse(mCM.mListening.isEmpty()); assertTrue(mCM.isListeningForAll()); @@ -170,9 +168,17 @@ public class UpstreamNetworkMonitorTest { @Test public void testCallbacksRegistered() { - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); + // Verify the fired default request matches expectation. + final ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(NetworkRequest.class); verify(mCM, times(1)).requestNetwork( - eq(sDefaultRequest), any(NetworkCallback.class), any(Handler.class)); + requestCaptor.capture(), any(NetworkCallback.class), any(Handler.class)); + // For R- devices, Tethering will invoke this function in 2 cases, one is to + // request mobile network, the other is to track system default network. Verify + // the request is the one tracks default network. + assertTrue(TestConnectivityManager.looksLikeDefaultRequest(requestCaptor.getValue())); + mUNM.startObserveAllNetworks(); verify(mCM, times(1)).registerNetworkCallback( any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class)); @@ -293,7 +299,7 @@ public class UpstreamNetworkMonitorTest { final Collection preferredTypes = new ArrayList<>(); preferredTypes.add(TYPE_WIFI); - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); // There are no networks, so there is nothing to select. assertSatisfiesLegacyType(TYPE_NONE, mUNM.selectPreferredUpstreamType(preferredTypes)); @@ -366,7 +372,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testGetCurrentPreferredUpstream() throws Exception { - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); mUNM.setUpstreamConfig(true /* autoUpstream */, false /* dunRequired */); mUNM.setTryCell(true); @@ -438,7 +444,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testLocalPrefixes() throws Exception { - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); // [0] Test minimum set of local prefixes. @@ -549,7 +555,7 @@ public class UpstreamNetworkMonitorTest { // Mobile has higher pirority than wifi. preferredTypes.add(TYPE_MOBILE_HIPRI); preferredTypes.add(TYPE_WIFI); - mUNM.startTrackDefaultNetwork(sDefaultRequest, mEntitleMgr); + mUNM.startTrackDefaultNetwork(mEntitleMgr); mUNM.startObserveAllNetworks(); // Setup wifi and make wifi as default network. final TestNetworkAgent wifiAgent = new TestNetworkAgent(mCM, WIFI_CAPABILITIES);