Merge "Update the logic of isFeatureEnabled and isFeatureNotChickenedOut" into main am: 241e110c6c

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/2749983

Change-Id: I62002804d1627b4ccae6a5fafe17e78efe45c7db
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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) {
+        public boolean isTetheringFeatureNotChickenedOut(Context context, String feature) {
-            return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(feature);
+            return DeviceConfigUtils.isTetheringFeatureNotChickenedOut(context, feature);
         }
 
         /**

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

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);
         }
 
         /**

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);
         }
     }

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(NAMESPACE_CONNECTIVITY, name, defaultEnabled,
-        return isFeatureEnabled(context, packageVersion, NAMESPACE_CONNECTIVITY, name,
+                () -> getPackageVersion(context));
-                defaultEnabled);
     }
 
     /**
@@ -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(NAMESPACE_TETHERING, name, false /* defaultEnabled */,
-        return isFeatureEnabled(context, packageVersion, NAMESPACE_TETHERING, name,
+                () -> getTetheringModuleVersion(context));
-                false /* defaultEnabled */);
     }
 
-    private static boolean isFeatureEnabled(@NonNull Context context, long packageVersion,
+    private static boolean isFeatureEnabled(@NonNull String namespace,
-            @NonNull String namespace, String name, boolean defaultEnabled) {
+            String name, boolean defaultEnabled, Supplier<Long> packageVersionSupplier) {
-        final int propertyVersion = getDeviceConfigPropertyInt(namespace, name,
+        final int flagValue = getDeviceConfigPropertyInt(namespace, name, 0 /* default value */);
-                0 /* default value */);
+        switch (flagValue) {
-        return (propertyVersion == 0 && defaultEnabled)
+            case 0:
-                || (propertyVersion != 0 && packageVersion >= (long) propertyVersion);
+                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) {
+    public static boolean isTetheringFeatureNotChickenedOut(@NonNull Context context, String name) {
-        return isFeatureNotChickenedOut(NAMESPACE_TETHERING, 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) {
+    public static boolean isNetworkStackFeatureNotChickenedOut(
-        return isFeatureNotChickenedOut(NAMESPACE_CONNECTIVITY, name);
+            @NonNull Context context, String name) {
+        return isFeatureEnabled(NAMESPACE_CONNECTIVITY, name, true /* defaultEnabled */,
+                () -> getPackageVersion(context));
     }
 
     /**

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 {
+    public void testIsFeatureNotChickenedOut() {
-        doReturn("0").when(() -> DeviceConfig.getProperty(
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY,
-                eq(NAMESPACE_TETHERING), eq(TEST_EXPERIMENT_FLAG)));
+                TEST_EXPERIMENT_FLAG));
-        assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG));
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(NAMESPACE_TETHERING,
-
+                TEST_EXPERIMENT_FLAG));
-        doReturn(TEST_FLAG_VALUE_STRING).when(
+        assertTrue(DeviceConfigUtils.isTetheringFeatureNotChickenedOut(
-                () -> DeviceConfig.getProperty(eq(NAMESPACE_TETHERING), eq(TEST_EXPERIMENT_FLAG)));
+                mContext, TEST_EXPERIMENT_FLAG));
-        assertFalse(DeviceConfigUtils.isTetheringFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG));
+        assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut(
+                mContext, TEST_EXPERIMENT_FLAG));
     }
 
     @Test
-    public void testIsNetworkStackFeatureNotChickenedOut() throws Exception {
+    public void testIsFeatureNotChickenedOutFeatureDefaultEnabled() throws Exception {
-        doReturn("0").when(() -> DeviceConfig.getProperty(
+        doReturn(null).when(() -> DeviceConfig.getProperty(NAMESPACE_CONNECTIVITY,
-                eq(NAMESPACE_CONNECTIVITY), eq(TEST_EXPERIMENT_FLAG)));
+                TEST_EXPERIMENT_FLAG));
-        assertTrue(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut(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(
+        // If the flag is unset, package info is not queried
-                () -> DeviceConfig.getProperty(eq(NAMESPACE_CONNECTIVITY),
+        verify(mContext, never()).getPackageManager();
-                                               eq(TEST_EXPERIMENT_FLAG)));
+        verify(mContext, never()).getPackageName();
-        assertFalse(DeviceConfigUtils.isNetworkStackFeatureNotChickenedOut(TEST_EXPERIMENT_FLAG));
+        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());
     }
 }