Merge "No-op Refactoring of startTrackDefaultNetwork"

Tethering/Android.bp

@@ -30,6 +30,7 @@ java_defaults {
         ":services-tethering-shared-srcs",
     ],
     static_libs: [
+        "NetworkStackApiStableShims",
         "androidx.annotation_annotation",
         "modules-utils-build",
         "netlink-client",

Tethering/src/com/android/networkstack/tethering/Tethering.java

@@ -2040,8 +2040,7 @@ public class Tethering {
     }
 
     private void startTrackDefaultNetwork() {
-        mUpstreamNetworkMonitor.startTrackDefaultNetwork(mDeps.getDefaultNetworkRequest(),
+        mUpstreamNetworkMonitor.startTrackDefaultNetwork(mEntitlementMgr);
-                mEntitlementMgr);
     }
 
     /** Get the latest value of the tethering entitlement check. */

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.
      */

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;
@@ -306,19 +304,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");

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;
-    }
+        mCM = (ConnectivityManager) ctx.getSystemService(Context.CONNECTIVITY_SERVICE);
-
-    @VisibleForTesting
-    public UpstreamNetworkMonitor(
-            ConnectivityManager cm, StateMachine tgt, SharedLog log, int what) {
-        this((Context) null, tgt, log, what);
-        mCM = cm;
     }
 
     /**
-     * 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,
+    public void startTrackDefaultNetwork(EntitlementManager entitle) {
-            EntitlementManager entitle) {
+        if (mDefaultNetworkCallback != null) {
-
+            Log.wtf(TAG, "default network callback is already registered");
-        // defaultNetworkRequest is not really a "request", just a way of tracking the system
+            return;
-        // 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
+        ConnectivityManagerShim mCmShim = ConnectivityManagerShimImpl.newInstance(mContext);
-        // shares fate with it. We can't use registerDefaultNetworkCallback because it will not
+        mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET);
-        // track the system default network if there is a VPN that applies to our UID.
+        try {
-        if (mDefaultNetworkCallback == null) {
+            mCmShim.registerSystemDefaultNetworkCallback(mDefaultNetworkCallback, mHandler);
-            mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET);
+        } catch (UnsupportedApiLevelException e) {
-            cm().requestNetwork(defaultNetworkRequest, mDefaultNetworkCallback, mHandler);
+            Log.wtf(TAG, "registerSystemDefaultNetworkCallback is not supported");
+            return;
         }
         if (mEntitlementMgr == null) {
             mEntitlementMgr = entitle;

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<NetworkCallback, NetworkRequestInfo> 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<NetworkCallback, NetworkRequestInfo> mTrackingDefault = new ArrayMap<>();
     final Map<NetworkCallback, NetworkRequestInfo> mListening = new ArrayMap<>();
     final Map<NetworkCallback, NetworkRequestInfo> mRequested = new ArrayMap<>();
     final Map<NetworkCallback, Integer> 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,
+    public TestConnectivityManager(Context ctx, IConnectivityManager svc) {
-            NetworkRequest defaultRequest) {
         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 {

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.
+        mCm = spy(new TestConnectivityManager(mServiceContext, mock(IConnectivityManager.class)));
-        // 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));
 
         mTethering = makeTethering();
         verify(mStatsManager, times(1)).registerNetworkStatsProvider(anyString(), any());
@@ -793,15 +776,13 @@ public class TetheringTest {
     }
 
     private void verifyDefaultNetworkRequestFiled() {
-        ArgumentCaptor<NetworkCallback> captor = ArgumentCaptor.forClass(NetworkCallback.class);
+        ArgumentCaptor<NetworkRequest> reqCaptor = ArgumentCaptor.forClass(NetworkRequest.class);
-        verify(mCm, times(1)).requestNetwork(eq(mNetworkRequest),
+        verify(mCm, times(1)).requestNetwork(reqCaptor.capture(),
-                captor.capture(), any(Handler.class));
+                any(NetworkCallback.class), any(Handler.class));
-        mDefaultNetworkCallback = captor.getValue();
+        assertTrue(TestConnectivityManager.looksLikeDefaultRequest(reqCaptor.getValue()));
-        assertNotNull(mDefaultNetworkCallback);
-
         // The default network request is only ever filed once.
         verifyNoMoreInteractions(mCm);
-        mUpstreamNetworkMonitor.startTrackDefaultNetwork(mNetworkRequest, mEntitleMgr);
+        mUpstreamNetworkMonitor.startTrackDefaultNetwork(mEntitleMgr);
         verifyNoMoreInteractions(mCm);
     }
 

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<NetworkRequest> 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<Integer> 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);