From bd1085ef03ca0188e829f8cce1269d96b235457b Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 23 Mar 2021 12:26:09 -0700 Subject: [PATCH 1/2] Switch to using status codes for Vcn This change adds status codes, and switches the Vcn class to use these to signal safe mode status. Additionally, this allows a distinction between a VCN that is quitting and one that is simply in safe mode, providing potential for an opportunistic safe mode where the VCN will continue to attempt to serve NetworkRequests while not restricting underlying networks. Bug: 183174340 Bug: 181789060 Test: atest FrameworksVcnTests Change-Id: I9f2b2d0d8d7b3ade19ca5adcd48cc920171bf8c3 --- .../android/server/VcnManagementService.java | 27 +++++---- .../core/java/com/android/server/vcn/Vcn.java | 58 ++++++++++--------- .../server/VcnManagementServiceTest.java | 6 +- .../java/com/android/server/vcn/VcnTest.java | 22 +++---- 4 files changed, 64 insertions(+), 49 deletions(-) diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index 434e9241c7424..4fd9373ad1df9 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -40,7 +40,6 @@ import android.net.vcn.IVcnManagementService; import android.net.vcn.IVcnStatusCallback; import android.net.vcn.IVcnUnderlyingNetworkPolicyListener; import android.net.vcn.VcnConfig; -import android.net.vcn.VcnManager; import android.net.vcn.VcnManager.VcnErrorCode; import android.net.vcn.VcnManager.VcnStatusCode; import android.net.vcn.VcnUnderlyingNetworkPolicy; @@ -532,11 +531,12 @@ public class VcnManagementService extends IVcnManagementService.Stub { if (mVcns.containsKey(subscriptionGroup)) { final Vcn vcn = mVcns.get(subscriptionGroup); - final boolean isActive = vcn.isActive(); + final int status = vcn.getStatus(); vcn.updateConfig(config); + // TODO(b/183174340): Remove this once opportunistic-safe-mode is supported // Only notify VcnStatusCallbacks if this VCN was previously in Safe Mode - if (!isActive) { + if (status == VCN_STATUS_CODE_SAFE_MODE) { // TODO(b/181789060): invoke asynchronously after Vcn notifies through VcnCallback notifyAllPermissionedStatusCallbacksLocked( subscriptionGroup, VCN_STATUS_CODE_ACTIVE); @@ -769,7 +769,7 @@ public class VcnManagementService extends IVcnManagementService.Stub { synchronized (mLock) { final Vcn vcn = mVcns.get(subGrp); if (vcn != null) { - if (vcn.isActive()) { + if (vcn.getStatus() == VCN_STATUS_CODE_ACTIVE) { isVcnManagedNetwork = true; } @@ -880,20 +880,23 @@ public class VcnManagementService extends IVcnManagementService.Stub { // now that callback is registered, send it the VCN's current status final VcnConfig vcnConfig = mConfigs.get(subGroup); final Vcn vcn = mVcns.get(subGroup); - final int vcnStatus; + final int vcnStatus = + vcn == null ? VCN_STATUS_CODE_NOT_CONFIGURED : vcn.getStatus(); + final int resultStatus; if (vcnConfig == null || !isCallbackPermissioned(cbInfo, subGroup)) { - vcnStatus = VcnManager.VCN_STATUS_CODE_NOT_CONFIGURED; + resultStatus = VCN_STATUS_CODE_NOT_CONFIGURED; } else if (vcn == null) { - vcnStatus = VcnManager.VCN_STATUS_CODE_INACTIVE; - } else if (vcn.isActive()) { - vcnStatus = VcnManager.VCN_STATUS_CODE_ACTIVE; + resultStatus = VCN_STATUS_CODE_INACTIVE; + } else if (vcnStatus == VCN_STATUS_CODE_ACTIVE + || vcnStatus == VCN_STATUS_CODE_SAFE_MODE) { + resultStatus = vcnStatus; } else { - // TODO(b/181789060): create Vcn.getStatus() and Log.WTF() for unknown status - vcnStatus = VcnManager.VCN_STATUS_CODE_SAFE_MODE; + Slog.wtf(TAG, "Unknown VCN status: " + vcnStatus); + resultStatus = VCN_STATUS_CODE_NOT_CONFIGURED; } try { - cbInfo.mCallback.onVcnStatusChanged(vcnStatus); + cbInfo.mCallback.onVcnStatusChanged(resultStatus); } catch (RemoteException e) { Slog.d(TAG, "VcnStatusCallback threw on VCN status change", e); } diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java index c0130da7ddbca..54689358802f1 100644 --- a/services/core/java/com/android/server/vcn/Vcn.java +++ b/services/core/java/com/android/server/vcn/Vcn.java @@ -17,6 +17,9 @@ package com.android.server.vcn; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_ACTIVE; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_INACTIVE; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_SAFE_MODE; import static com.android.server.VcnManagementService.VDBG; @@ -44,7 +47,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; /** * Represents an single instance of a VCN. @@ -137,17 +139,14 @@ public class Vcn extends Handler { @NonNull private TelephonySubscriptionSnapshot mLastSnapshot; /** - * Whether this Vcn instance is active and running. + * The current status of this Vcn instance * - *

The value will be {@code true} while running. It will be {@code false} if the VCN has been - * shut down or has entered safe mode. - * - *

This AtomicBoolean is required in order to ensure consistency and correctness across - * multiple threads. Unlike the rest of the Vcn, this is queried synchronously on Binder threads - * from VcnManagementService, and therefore cannot rely on guarantees of running on the VCN - * Looper. + *

The value will be {@link VCN_STATUS_CODE_ACTIVE} while all VcnGatewayConnections are in + * good standing, {@link VCN_STATUS_CODE_SAFE_MODE} if any VcnGatewayConnections are in safe + * mode, and {@link VCN_STATUS_CODE_INACTIVE} once a teardown has been commanded. */ - private final AtomicBoolean mIsActive = new AtomicBoolean(true); + // Accessed from different threads, but always under lock in VcnManagementService + private volatile int mCurrentStatus = VCN_STATUS_CODE_ACTIVE; public Vcn( @NonNull VcnContext vcnContext, @@ -199,9 +198,15 @@ public class Vcn extends Handler { sendMessageAtFrontOfQueue(obtainMessage(MSG_CMD_TEARDOWN)); } - /** Synchronously checks whether this Vcn is active. */ - public boolean isActive() { - return mIsActive.get(); + /** Synchronously retrieves the current status code. */ + public int getStatus() { + return mCurrentStatus; + } + + /** Sets the status of this VCN */ + @VisibleForTesting(visibility = Visibility.PRIVATE) + public void setStatus(int status) { + mCurrentStatus = status; } /** Get current Gateways for testing purposes */ @@ -217,12 +222,6 @@ public class Vcn extends Handler { return Collections.unmodifiableMap(new HashMap<>(mVcnGatewayConnections)); } - /** Set whether this Vcn is active for testing purposes */ - @VisibleForTesting(visibility = Visibility.PRIVATE) - public void setIsActive(boolean isActive) { - mIsActive.set(isActive); - } - private class VcnNetworkRequestListener implements VcnNetworkProvider.NetworkRequestListener { @Override public void onNetworkRequested(@NonNull NetworkRequest request, int score, int providerId) { @@ -264,7 +263,8 @@ public class Vcn extends Handler { mConfig = config; - if (mIsActive.getAndSet(true)) { + // TODO(b/183174340): Remove this once opportunistic safe mode is supported. + if (mCurrentStatus == VCN_STATUS_CODE_ACTIVE) { // VCN is already active - teardown any GatewayConnections whose configs have been // removed and get all current requests for (final Entry entry : @@ -288,11 +288,15 @@ public class Vcn extends Handler { // Trigger a re-evaluation of all NetworkRequests (to make sure any that can be // satisfied start a new GatewayConnection) mVcnContext.getVcnNetworkProvider().resendAllRequests(mRequestListener); - } else { + } else if (mCurrentStatus == VCN_STATUS_CODE_SAFE_MODE) { // If this VCN was not previously active, it is exiting Safe Mode. Re-register the // request listener to get NetworkRequests again (and all cached requests). mVcnContext.getVcnNetworkProvider().registerListener(mRequestListener); + } else { + // Ignored; VCN was not active; config updates ignored. + return; } + mCurrentStatus = VCN_STATUS_CODE_ACTIVE; } private void handleTeardown() { @@ -302,18 +306,20 @@ public class Vcn extends Handler { gatewayConnection.teardownAsynchronously(); } - mIsActive.set(false); + mCurrentStatus = VCN_STATUS_CODE_INACTIVE; } private void handleEnterSafeMode() { + // TODO(b/183174340): Remove this once opportunistic-safe-mode is supported handleTeardown(); + mCurrentStatus = VCN_STATUS_CODE_SAFE_MODE; mVcnCallback.onEnteredSafeMode(); } private void handleNetworkRequested( @NonNull NetworkRequest request, int score, int providerId) { - if (!isActive()) { + if (mCurrentStatus != VCN_STATUS_CODE_ACTIVE) { Slog.v(getLogTag(), "Received NetworkRequest while inactive. Ignore for now"); return; } @@ -370,8 +376,8 @@ public class Vcn extends Handler { mVcnGatewayConnections.remove(config); // Trigger a re-evaluation of all NetworkRequests (to make sure any that can be satisfied - // start a new GatewayConnection), but only if the Vcn is still active - if (isActive()) { + // start a new GatewayConnection), but only if the Vcn is still alive + if (mCurrentStatus == VCN_STATUS_CODE_ACTIVE) { mVcnContext.getVcnNetworkProvider().resendAllRequests(mRequestListener); } } @@ -379,7 +385,7 @@ public class Vcn extends Handler { private void handleSubscriptionsChanged(@NonNull TelephonySubscriptionSnapshot snapshot) { mLastSnapshot = snapshot; - if (isActive()) { + if (mCurrentStatus == VCN_STATUS_CODE_ACTIVE) { for (VcnGatewayConnection gatewayConnection : mVcnGatewayConnections.values()) { gatewayConnection.updateSubscriptionSnapshot(mLastSnapshot); } diff --git a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java index c88b0c1547126..4ad7136aabb29 100644 --- a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java +++ b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java @@ -19,6 +19,8 @@ package com.android.server; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_ACTIVE; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_SAFE_MODE; import static android.telephony.TelephonyManager.CARRIER_PRIVILEGE_STATUS_HAS_ACCESS; import static android.telephony.TelephonyManager.CARRIER_PRIVILEGE_STATUS_NO_ACCESS; @@ -695,7 +697,9 @@ public class VcnManagementServiceTest { hasCarrierPrivileges); final Vcn vcn = startAndGetVcnInstance(subGrp); - doReturn(isVcnActive).when(vcn).isActive(); + doReturn(isVcnActive ? VCN_STATUS_CODE_ACTIVE : VCN_STATUS_CODE_SAFE_MODE) + .when(vcn) + .getStatus(); doReturn(true) .when(mLocationPermissionChecker) diff --git a/tests/vcn/java/com/android/server/vcn/VcnTest.java b/tests/vcn/java/com/android/server/vcn/VcnTest.java index 530d8348bfff1..540be38ed7988 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnTest.java @@ -19,10 +19,12 @@ package com.android.server.vcn; import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; import static android.net.NetworkCapabilities.NET_CAPABILITY_MMS; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_ACTIVE; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_INACTIVE; +import static android.net.vcn.VcnManager.VCN_STATUS_CODE_SAFE_MODE; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.argThat; import static org.mockito.Matchers.eq; @@ -142,7 +144,7 @@ public class VcnTest { mTestLooper.dispatchAll(); } - private void verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(boolean isActive) { + private void verifyUpdateSubscriptionSnapshotNotifiesGatewayConnections(int status) { final NetworkRequestListener requestListener = verifyAndGetRequestListener(); startVcnGatewayWithCapabilities(requestListener, TEST_CAPS[0]); @@ -152,25 +154,25 @@ public class VcnTest { final TelephonySubscriptionSnapshot updatedSnapshot = mock(TelephonySubscriptionSnapshot.class); - mVcn.setIsActive(isActive); + mVcn.setStatus(status); mVcn.updateSubscriptionSnapshot(updatedSnapshot); mTestLooper.dispatchAll(); for (final VcnGatewayConnection gateway : gatewayConnections) { - verify(gateway, isActive ? times(1) : never()) + verify(gateway, status == VCN_STATUS_CODE_ACTIVE ? times(1) : never()) .updateSubscriptionSnapshot(eq(updatedSnapshot)); } } @Test public void testSubscriptionSnapshotUpdatesVcnGatewayConnections() { - verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(true /* isActive */); + verifyUpdateSubscriptionSnapshotNotifiesGatewayConnections(VCN_STATUS_CODE_ACTIVE); } @Test - public void testSubscriptionSnapshotUpdatesVcnGatewayConnectionsWhileInactive() { - verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(false /* isActive */); + public void testSubscriptionSnapshotUpdatesVcnGatewayConnectionsInSafeMode() { + verifyUpdateSubscriptionSnapshotNotifiesGatewayConnections(VCN_STATUS_CODE_SAFE_MODE); } private void triggerVcnRequestListeners(NetworkRequestListener requestListener) { @@ -201,7 +203,7 @@ public class VcnTest { private void verifySafeMode( NetworkRequestListener requestListener, Set expectedGatewaysTornDown) { - assertFalse(mVcn.isActive()); + assertEquals(VCN_STATUS_CODE_SAFE_MODE, mVcn.getStatus()); for (final VcnGatewayConnection gatewayConnection : expectedGatewaysTornDown) { verify(gatewayConnection).teardownAsynchronously(); } @@ -319,7 +321,7 @@ public class VcnTest { // Registered on start, then re-registered with new configs verify(mVcnNetworkProvider, times(2)).registerListener(eq(requestListener)); - assertTrue(mVcn.isActive()); + assertEquals(VCN_STATUS_CODE_ACTIVE, mVcn.getStatus()); for (final int[] caps : TEST_CAPS) { // Expect each gateway connection created only on initial startup verify(mDeps) @@ -334,7 +336,7 @@ public class VcnTest { @Test public void testIgnoreNetworkRequestWhileInactive() { - mVcn.setIsActive(false /* isActive */); + mVcn.setStatus(VCN_STATUS_CODE_INACTIVE); final NetworkRequestListener requestListener = verifyAndGetRequestListener(); triggerVcnRequestListeners(requestListener); From 8124f47d3f15af84f6b401c4bd66233ad077645d Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 31 Mar 2021 13:16:07 -0700 Subject: [PATCH 2/2] Remove subIds from Telephony NetworkRequests. Since NetworkProviders do not at present support requests with subIds (they always provide an empty list, therefore never have an overlap), the TelephonyBringupRequests must never present subIds as part of the requested capabilities. Bug: 183174340 Test: atest FrameworksVcnTests Change-Id: I2e52ff67748c0ca226a648682b37dfe0996cb850 --- .../server/vcn/UnderlyingNetworkTracker.java | 51 +++++++++++++++---- .../vcn/UnderlyingNetworkTrackerTest.java | 16 +++--- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java b/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java index 069c5c3fc7144..ab9de77005b30 100644 --- a/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java +++ b/services/core/java/com/android/server/vcn/UnderlyingNetworkTracker.java @@ -118,7 +118,7 @@ public class UnderlyingNetworkTracker { if (!mIsQuitting) { mRouteSelectionCallback = new RouteSelectionCallback(); mConnectivityManager.requestBackgroundNetwork( - getBaseNetworkRequestBuilder().build(), mHandler, mRouteSelectionCallback); + getRouteSelectionRequest(), mHandler, mRouteSelectionCallback); mWifiBringupCallback = new NetworkBringupCallback(); mConnectivityManager.requestBackgroundNetwork( @@ -149,12 +149,48 @@ public class UnderlyingNetworkTracker { } } - private NetworkRequest getWifiNetworkRequest() { + /** + * Builds the Route selection request + * + *

This request is guaranteed to select carrier-owned, non-VCN underlying networks by virtue + * of a populated set of subIds as expressed in NetworkCapabilities#getSubIds(). Only carrier + * owned networks may be selected, as the request specifies only subIds in the VCN's + * subscription group, while the VCN networks are excluded by virtue of not having subIds set on + * the VCN-exposed networks. + */ + private NetworkRequest getRouteSelectionRequest() { return getBaseNetworkRequestBuilder() - .addTransportType(NetworkCapabilities.TRANSPORT_WIFI) + .setSubIds(mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup)) .build(); } + /** + * Builds the WiFi bringup request + * + *

This request is built specifically to match only carrier-owned WiFi networks, but is also + * built to ONLY keep Carrier WiFi Networks alive (but never bring them up). This is a result of + * the WifiNetworkFactory not advertising a list of subIds, and therefore not accepting this + * request. As such, it will bind to a Carrier WiFi Network that has already been brought up, + * but will NEVER bring up a Carrier WiFi network itself. + */ + private NetworkRequest getWifiNetworkRequest() { + return getBaseNetworkRequestBuilder() + .addTransportType(NetworkCapabilities.TRANSPORT_WIFI) + .setSubIds(mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup)) + .build(); + } + + /** + * Builds a Cellular bringup request for a given subId + * + *

This request is filed in order to ensure that the Telephony stack always has a + * NetworkRequest to bring up a VCN underlying cellular network. It is required in order to + * ensure that even when a VCN (appears as Cellular) satisfies the default request, Telephony + * will bring up additional underlying Cellular networks. + * + *

Since this request MUST make it to the TelephonyNetworkFactory, subIds are not specified + * in the NetworkCapabilities, but rather in the TelephonyNetworkSpecifier. + */ private NetworkRequest getCellNetworkRequestForSubId(int subId) { return getBaseNetworkRequestBuilder() .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR) @@ -164,20 +200,13 @@ public class UnderlyingNetworkTracker { /** * Builds and returns a NetworkRequest builder common to all Underlying Network requests - * - *

This request is guaranteed to select carrier-owned, non-VCN underlying networks by virtue - * of a populated set of subIds as expressed in NetworkCapabilities#getSubIds(). Only carrier - * owned networks may be selected, as the request specifies only subIds in the VCN's - * subscription group, while the VCN networks are excluded by virtue of not having subIds set on - * the VCN-exposed networks. */ private NetworkRequest.Builder getBaseNetworkRequestBuilder() { return new NetworkRequest.Builder() .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) .removeCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED) .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED) - .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED) - .setSubIds(mLastSnapshot.getAllSubIdsInGroup(mSubscriptionGroup)); + .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED); } /** diff --git a/tests/vcn/java/com/android/server/vcn/UnderlyingNetworkTrackerTest.java b/tests/vcn/java/com/android/server/vcn/UnderlyingNetworkTrackerTest.java index ed2e4d9b9946b..b592000e38f9a 100644 --- a/tests/vcn/java/com/android/server/vcn/UnderlyingNetworkTrackerTest.java +++ b/tests/vcn/java/com/android/server/vcn/UnderlyingNetworkTrackerTest.java @@ -158,7 +158,7 @@ public class UnderlyingNetworkTrackerTest { for (final int subId : expectedSubIds) { verify(mConnectivityManager) .requestBackgroundNetwork( - eq(getCellRequestForSubId(subId, expectedSubIds)), + eq(getCellRequestForSubId(subId)), any(), any(NetworkBringupCallback.class)); } @@ -189,30 +189,30 @@ public class UnderlyingNetworkTrackerTest { } private NetworkRequest getWifiRequest(Set netCapsSubIds) { - return getExpectedRequestBase(netCapsSubIds) + return getExpectedRequestBase() .addTransportType(NetworkCapabilities.TRANSPORT_WIFI) + .setSubIds(netCapsSubIds) .build(); } - private NetworkRequest getCellRequestForSubId(int subId, Set netCapsSubIds) { - return getExpectedRequestBase(netCapsSubIds) + private NetworkRequest getCellRequestForSubId(int subId) { + return getExpectedRequestBase() .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR) .setNetworkSpecifier(new TelephonyNetworkSpecifier(subId)) .build(); } private NetworkRequest getRouteSelectionRequest(Set netCapsSubIds) { - return getExpectedRequestBase(netCapsSubIds).build(); + return getExpectedRequestBase().setSubIds(netCapsSubIds).build(); } - private NetworkRequest.Builder getExpectedRequestBase(Set subIds) { + private NetworkRequest.Builder getExpectedRequestBase() { final NetworkRequest.Builder builder = new NetworkRequest.Builder() .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) .removeCapability(NetworkCapabilities.NET_CAPABILITY_TRUSTED) .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED) - .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED) - .setSubIds(subIds); + .removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED); return builder; }