From ed4e7ec954021776fa98b8b41af5b2a5ecd4e09c Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Wed, 13 Sep 2023 14:58:32 +0900 Subject: [PATCH] Update the logic of isFeatureEnabled and isFeatureNotChickenedOut If the flag value is unset or 0, isFeatureEnabled return false and isFeatureNotChickenedOut return true. If the flag value is -1 (force disable), both return false. If the flag value is other values, both query the package version and return `flagValue <= packageVersion` Now the only difference is a default behavior when the flag is not set or 0. So isFeatureEnabled and isFeatureNotChickenedOut can use the same flag. This CL also fixes the issue in rollback. Before this CL, isFeatureNotChickenedOut did not check the module version and could have a issue if there is a rollback. Test: NetworkStaticLibsTests Bug: 279108992 Change-Id: I12d6ebadff3aee7b7c614aca4eb0a34ef0db9857 --- .../src/com/android/server/NsdService.java | 7 +- .../src/com/android/server/BpfNetMaps.java | 2 +- .../AutomaticOnOffKeepaliveTracker.java | 2 +- .../server/connectivity/KeepaliveTracker.java | 2 +- .../net/module/util/DeviceConfigUtils.java | 74 ++++++----- .../module/util/DeviceConfigUtilsTest.java | 118 ++++++++++++++---- 6 files changed, 141 insertions(+), 64 deletions(-) diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java index 468d7bd76f..93ccb85295 100644 --- a/service-t/src/com/android/server/NsdService.java +++ b/service-t/src/com/android/server/NsdService.java @@ -26,6 +26,7 @@ import static android.net.nsd.NsdManager.MDNS_DISCOVERY_MANAGER_EVENT; import static android.net.nsd.NsdManager.MDNS_SERVICE_EVENT; import static android.net.nsd.NsdManager.RESOLVE_SERVICE_SUCCEEDED; import static android.provider.DeviceConfig.NAMESPACE_TETHERING; + import static com.android.modules.utils.build.SdkLevel.isAtLeastU; import static com.android.networkstack.apishim.ConstantsShim.REGISTER_NSD_OFFLOAD_ENGINE; import static com.android.server.connectivity.mdns.MdnsAdvertiser.AdvertiserMetrics; @@ -1709,7 +1710,7 @@ public class NsdService extends INsdManager.Stub { mMdnsSocketClient, LOGGER.forSubComponent("MdnsDiscoveryManager")); handler.post(() -> mMdnsSocketClient.setCallback(mMdnsDiscoveryManager)); MdnsFeatureFlags flags = new MdnsFeatureFlags.Builder().setIsMdnsOffloadFeatureEnabled( - mDeps.isTetheringFeatureNotChickenedOut( + mDeps.isTetheringFeatureNotChickenedOut(mContext, MdnsFeatureFlags.NSD_FORCE_DISABLE_MDNS_OFFLOAD)).build(); mAdvertiser = deps.makeMdnsAdvertiser(handler.getLooper(), mMdnsSocketProvider, new AdvertiserCallback(), LOGGER.forSubComponent("MdnsAdvertiser"), flags); @@ -1763,8 +1764,8 @@ public class NsdService extends INsdManager.Stub { /** * @see DeviceConfigUtils#isTetheringFeatureNotChickenedOut */ - public boolean isTetheringFeatureNotChickenedOut(String feature) { - return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(feature); + public boolean isTetheringFeatureNotChickenedOut(Context context, String feature) { + return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(context, feature); } /** diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java index 4b24aaf850..6a34a242ad 100644 --- a/service/src/com/android/server/BpfNetMaps.java +++ b/service/src/com/android/server/BpfNetMaps.java @@ -254,7 +254,7 @@ public class BpfNetMaps { if (sInitialized) return; if (sEnableJavaBpfMap == null) { sEnableJavaBpfMap = SdkLevel.isAtLeastU() || - DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + DeviceConfigUtils.isTetheringFeatureNotChickenedOut(context, BPF_NET_MAPS_FORCE_DISABLE_JAVA_BPF_MAP); } Log.d(TAG, "BpfNetMaps is initialized with sEnableJavaBpfMap=" + sEnableJavaBpfMap); diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 3befcfa43b..11345d3aa8 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -974,7 +974,7 @@ public class AutomaticOnOffKeepaliveTracker { * @return whether the feature is enabled */ public boolean isTetheringFeatureNotChickenedOut(@NonNull final String name) { - return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(name); + return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(mContext, name); } /** diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index feba82198d..a51f09f968 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java @@ -993,7 +993,7 @@ public class KeepaliveTracker { */ public boolean isAddressTranslationEnabled(@NonNull Context context) { return DeviceConfigUtils.isFeatureSupported(context, FEATURE_CLAT_ADDRESS_TRANSLATE) - && DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + && DeviceConfigUtils.isTetheringFeatureNotChickenedOut(context, CONFIG_DISABLE_CLAT_ADDRESS_TRANSLATE); } } diff --git a/staticlibs/device/com/android/net/module/util/DeviceConfigUtils.java b/staticlibs/device/com/android/net/module/util/DeviceConfigUtils.java index e646f37a6e..5b75699774 100644 --- a/staticlibs/device/com/android/net/module/util/DeviceConfigUtils.java +++ b/staticlibs/device/com/android/net/module/util/DeviceConfigUtils.java @@ -40,6 +40,7 @@ import androidx.annotation.VisibleForTesting; import java.util.ArrayList; import java.util.List; +import java.util.function.Supplier; /** * Utilities for modules to query {@link DeviceConfig} and flags. @@ -70,6 +71,9 @@ public final class DeviceConfigUtils { sNetworkStackModuleVersion = -1; } + private static final int FORCE_ENABLE_FEATURE_FLAG_VALUE = 1; + private static final int FORCE_DISABLE_FEATURE_FLAG_VALUE = -1; + private static volatile long sPackageVersion = -1; private static long getPackageVersion(@NonNull final Context context) { // sPackageVersion may be set by another thread just after this check, but querying the @@ -186,9 +190,8 @@ public final class DeviceConfigUtils { */ public static boolean isNetworkStackFeatureEnabled(@NonNull Context context, @NonNull String name, boolean defaultEnabled) { - final long packageVersion = getPackageVersion(context); - return isFeatureEnabled(context, packageVersion, NAMESPACE_CONNECTIVITY, name, - defaultEnabled); + return isFeatureEnabled(NAMESPACE_CONNECTIVITY, name, defaultEnabled, + () -> getPackageVersion(context)); } /** @@ -202,7 +205,7 @@ public final class DeviceConfigUtils { * * If the feature is disabled by default and enabled by flag push, this method should be used. * If the feature is enabled by default and disabled by flag push (kill switch), - * {@link #isTetheringFeatureNotChickenedOut(String)} should be used. + * {@link #isTetheringFeatureNotChickenedOut(Context, String)} should be used. * * @param context The global context information about an app environment. * @param name The name of the property to look up. @@ -210,17 +213,24 @@ public final class DeviceConfigUtils { */ public static boolean isTetheringFeatureEnabled(@NonNull Context context, @NonNull String name) { - final long packageVersion = getTetheringModuleVersion(context); - return isFeatureEnabled(context, packageVersion, NAMESPACE_TETHERING, name, - false /* defaultEnabled */); + return isFeatureEnabled(NAMESPACE_TETHERING, name, false /* defaultEnabled */, + () -> getTetheringModuleVersion(context)); } - private static boolean isFeatureEnabled(@NonNull Context context, long packageVersion, - @NonNull String namespace, String name, boolean defaultEnabled) { - final int propertyVersion = getDeviceConfigPropertyInt(namespace, name, - 0 /* default value */); - return (propertyVersion == 0 && defaultEnabled) - || (propertyVersion != 0 && packageVersion >= (long) propertyVersion); + private static boolean isFeatureEnabled(@NonNull String namespace, + String name, boolean defaultEnabled, Supplier packageVersionSupplier) { + final int flagValue = getDeviceConfigPropertyInt(namespace, name, 0 /* default value */); + switch (flagValue) { + case 0: + return defaultEnabled; + case FORCE_DISABLE_FEATURE_FLAG_VALUE: + return false; + case FORCE_ENABLE_FEATURE_FLAG_VALUE: + return true; + default: + final long packageVersion = packageVersionSupplier.get(); + return packageVersion >= (long) flagValue; + } } // Guess the tethering module name based on the package prefix of the connectivity resources @@ -330,43 +340,39 @@ public final class DeviceConfigUtils { || moduleVersion >= (featureId & VERSION_MASK); } - /** - * Check whether one specific experimental feature in specific namespace from - * {@link DeviceConfig} is not disabled. Feature can be disabled by setting a non-zero - * value in the property. If the feature is enabled by default and disabled by flag push - * (kill switch), this method should be used. If the feature is disabled by default and - * enabled by flag push, {@link #isFeatureEnabled} should be used. - * - * @param namespace The namespace containing the property to look up. - * @param name The name of the property to look up. - * @return true if this feature is enabled, or false if disabled. - */ - private static boolean isFeatureNotChickenedOut(String namespace, String name) { - final int propertyVersion = getDeviceConfigPropertyInt(namespace, name, - 0 /* default value */); - return propertyVersion == 0; - } - /** * Check whether one specific experimental feature in Tethering module from {@link DeviceConfig} * is not disabled. + * If the feature is enabled by default and disabled by flag push (kill switch), this method + * should be used. + * If the feature is disabled by default and enabled by flag push, + * {@link #isTetheringFeatureEnabled(Context, String)} should be used. * + * @param context The global context information about an app environment. * @param name The name of the property in tethering module to look up. * @return true if this feature is enabled, or false if disabled. */ - public static boolean isTetheringFeatureNotChickenedOut(String name) { - return isFeatureNotChickenedOut(NAMESPACE_TETHERING, name); + public static boolean isTetheringFeatureNotChickenedOut(@NonNull Context context, String name) { + return isFeatureEnabled(NAMESPACE_TETHERING, name, true /* defaultEnabled */, + () -> getTetheringModuleVersion(context)); } /** * Check whether one specific experimental feature in NetworkStack module from * {@link DeviceConfig} is not disabled. + * If the feature is enabled by default and disabled by flag push (kill switch), this method + * should be used. + * If the feature is disabled by default and enabled by flag push, + * {@link #isNetworkStackFeatureEnabled(Context, String)} should be used. * + * @param context The global context information about an app environment. * @param name The name of the property in NetworkStack module to look up. * @return true if this feature is enabled, or false if disabled. */ - public static boolean isNetworkStackFeatureNotChickenedOut(String name) { - return isFeatureNotChickenedOut(NAMESPACE_CONNECTIVITY, name); + public static boolean isNetworkStackFeatureNotChickenedOut( + @NonNull Context context, String name) { + return isFeatureEnabled(NAMESPACE_CONNECTIVITY, name, true /* defaultEnabled */, + () -> getPackageVersion(context)); } /** diff --git a/staticlibs/tests/unit/src/com/android/net/module/util/DeviceConfigUtilsTest.java b/staticlibs/tests/unit/src/com/android/net/module/util/DeviceConfigUtilsTest.java index 5a96bcbb04..9e046765e2 100644 --- a/staticlibs/tests/unit/src/com/android/net/module/util/DeviceConfigUtilsTest.java +++ b/staticlibs/tests/unit/src/com/android/net/module/util/DeviceConfigUtilsTest.java @@ -228,27 +228,57 @@ public class DeviceConfigUtilsTest { } @Test - public void testIsNetworkStackFeatureEnabled() { + public void testIsFeatureEnabled() { doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, TEST_EXPERIMENT_FLAG)); - assertTrue(DeviceConfigUtils.isNetworkStackFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); - } - - @Test - public void testIsTetheringFeatureEnabled() { doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isNetworkStackFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); assertTrue(DeviceConfigUtils.isTetheringFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); } - @Test - public void testFeatureDefaultEnabled() { + public void testIsFeatureEnabledFeatureDefaultDisabled() throws Exception { doReturn(null).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, TEST_EXPERIMENT_FLAG)); doReturn(null).when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, TEST_EXPERIMENT_FLAG)); assertFalse(DeviceConfigUtils.isNetworkStackFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); assertFalse(DeviceConfigUtils.isTetheringFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); + + // If the flag is unset, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); + } + + @Test + public void testIsFeatureEnabledFeatureForceEnabled() throws Exception { + doReturn("1").when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn("1").when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isNetworkStackFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isTetheringFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); + + // If the feature is force enabled, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); + } + + @Test + public void testIsFeatureEnabledFeatureForceDisabled() throws Exception { + doReturn("-1").when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn("-1").when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertFalse(DeviceConfigUtils.isNetworkStackFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); + assertFalse(DeviceConfigUtils.isTetheringFeatureEnabled(mContext, TEST_EXPERIMENT_FLAG)); + + // If the feature is force disabled, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); } @Test @@ -415,25 +445,65 @@ public class DeviceConfigUtilsTest { } @Test - public void testIsTetheringFeatureNotChickenedOut() throws Exception { - doReturn("0").when(() -> DeviceConfig.getProperty( - eq(NAMESPACE_TETHERING), eq(TEST_EXPERIMENT_FLAG))); - assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG)); - - doReturn(TEST_FLAG_VALUE_STRING).when( - () -> DeviceConfig.getProperty(eq(NAMESPACE_TETHERING), eq(TEST_EXPERIMENT_FLAG))); - assertFalse(DeviceConfigUtils.isTetheringFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG)); + public void testIsFeatureNotChickenedOut() { + doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); } @Test - public void testIsNetworkStackFeatureNotChickenedOut() throws Exception { - doReturn("0").when(() -> DeviceConfig.getProperty( - eq(NAMESPACE_CONNECTIVITY), eq(TEST_EXPERIMENT_FLAG))); - assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG)); + public void testIsFeatureNotChickenedOutFeatureDefaultEnabled() throws Exception { + doReturn(null).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn(null).when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); - doReturn(TEST_FLAG_VALUE_STRING).when( - () -> DeviceConfig.getProperty(eq(NAMESPACE_CONNECTIVITY), - eq(TEST_EXPERIMENT_FLAG))); - assertFalse(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG)); + // If the flag is unset, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); + } + + @Test + public void testIsFeatureNotChickenedOutFeatureForceEnabled() throws Exception { + doReturn("1").when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn("1").when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + + // If the feature is force enabled, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); + } + + @Test + public void testIsFeatureNotChickenedOutFeatureForceDisabled() throws Exception { + doReturn("-1").when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY, + TEST_EXPERIMENT_FLAG)); + doReturn("-1").when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING, + TEST_EXPERIMENT_FLAG)); + assertFalse(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + assertFalse(DeviceConfigUtils.isTetheringFeatureNotChickenedOut( + mContext, TEST_EXPERIMENT_FLAG)); + + // If the feature is force disabled, package info is not queried + verify(mContext, never()).getPackageManager(); + verify(mContext, never()).getPackageName(); + verify(mPm, never()).getPackageInfo(anyString(), anyInt()); } }