Remove InterfaceController dependency on NMS

Test: atest FrameworksNetTests NetworkStackTests
Bug: 112869080
Change-Id: Ib3773068b087f58f4ac3394291cda132b00b2dcc

6 files changed, 100 insertions, 61 deletions

diff --git a/services/net/java/android/net/ip/InterfaceController.java b/services/net/java/android/net/ip/InterfaceController.java
index 55dfcef81890..b3af67cdbdc3 100644
--- a/services/net/java/android/net/ip/InterfaceController.java
+++ b/services/net/java/android/net/ip/InterfaceController.java
@@ -18,13 +18,14 @@ package android.net.ip;
 
 import android.net.INetd;
 import android.net.InterfaceConfiguration;
+import android.net.InterfaceConfigurationParcel;
 import android.net.LinkAddress;
 import android.net.util.SharedLog;
-import android.os.INetworkManagementService;
 import android.os.RemoteException;
 import android.os.ServiceSpecificException;
 import android.system.OsConstants;
 
+import java.net.Inet4Address;
 import java.net.InetAddress;
 
 
@@ -39,76 +40,96 @@ public class InterfaceController {
     private final static boolean DBG = false;
 
     private final String mIfName;
-    private final INetworkManagementService mNMS;
     private final INetd mNetd;
     private final SharedLog mLog;
 
-    public InterfaceController(String ifname, INetworkManagementService nms, INetd netd,
-            SharedLog log) {
+    public InterfaceController(String ifname, INetd netd, SharedLog log) {
         mIfName = ifname;
-        mNMS = nms;
         mNetd = netd;
         mLog = log;
     }
 
-    public boolean setIPv4Address(LinkAddress address) {
-        final InterfaceConfiguration ifcg = new InterfaceConfiguration();
-        ifcg.setLinkAddress(address);
+    private boolean setInterfaceConfig(InterfaceConfiguration config) {
+        final InterfaceConfigurationParcel cfgParcel = config.toParcel(mIfName);
+
         try {
-            mNMS.setInterfaceConfig(mIfName, ifcg);
-            if (DBG) mLog.log("IPv4 configuration succeeded");
-        } catch (IllegalStateException | RemoteException e) {
-            logError("IPv4 configuration failed: %s", e);
+            mNetd.interfaceSetCfg(cfgParcel);
+        } catch (RemoteException | ServiceSpecificException e) {
+            logError("Setting IPv4 address to %s/%d failed: %s",
+                    cfgParcel.ipv4Addr, cfgParcel.prefixLength, e);
             return false;
         }
         return true;
     }
 
-    public boolean clearIPv4Address() {
-        try {
-            final InterfaceConfiguration ifcg = new InterfaceConfiguration();
-            ifcg.setLinkAddress(new LinkAddress("0.0.0.0/0"));
-            mNMS.setInterfaceConfig(mIfName, ifcg);
-        } catch (IllegalStateException | RemoteException e) {
-            logError("Failed to clear IPv4 address on interface %s: %s", mIfName, e);
+    /**
+     * Set the IPv4 address of the interface.
+     */
+    public boolean setIPv4Address(LinkAddress address) {
+        if (!(address.getAddress() instanceof Inet4Address)) {
             return false;
         }
-        return true;
+        final InterfaceConfiguration ifConfig = new InterfaceConfiguration();
+        ifConfig.setLinkAddress(address);
+        return setInterfaceConfig(ifConfig);
     }
 
-    public boolean enableIPv6() {
+    /**
+     * Clear the IPv4Address of the interface.
+     */
+    public boolean clearIPv4Address() {
+        final InterfaceConfiguration ifConfig = new InterfaceConfiguration();
+        ifConfig.setLinkAddress(new LinkAddress("0.0.0.0/0"));
+        return setInterfaceConfig(ifConfig);
+    }
+
+    private boolean setEnableIPv6(boolean enabled) {
         try {
-            mNMS.enableIpv6(mIfName);
-        } catch (IllegalStateException | RemoteException e) {
-            logError("enabling IPv6 failed: %s", e);
+            mNetd.interfaceSetEnableIPv6(mIfName, enabled);
+        } catch (RemoteException | ServiceSpecificException e) {
+            logError("%s IPv6 failed: %s", (enabled ? "enabling" : "disabling"), e);
             return false;
         }
         return true;
     }
 
+    /**
+     * Enable IPv6 on the interface.
+     */
+    public boolean enableIPv6() {
+        return setEnableIPv6(true);
+    }
+
+    /**
+     * Disable IPv6 on the interface.
+     */
     public boolean disableIPv6() {
-        try {
-            mNMS.disableIpv6(mIfName);
-        } catch (IllegalStateException | RemoteException e) {
-            logError("disabling IPv6 failed: %s", e);
-            return false;
-        }
-        return true;
+        return setEnableIPv6(false);
     }
 
+    /**
+     * Enable or disable IPv6 privacy extensions on the interface.
+     * @param enabled Whether the extensions should be enabled.
+     */
     public boolean setIPv6PrivacyExtensions(boolean enabled) {
         try {
-            mNMS.setInterfaceIpv6PrivacyExtensions(mIfName, enabled);
-        } catch (IllegalStateException | RemoteException e) {
-            logError("error setting IPv6 privacy extensions: %s", e);
+            mNetd.interfaceSetIPv6PrivacyExtensions(mIfName, enabled);
+        } catch (RemoteException | ServiceSpecificException e) {
+            logError("error %s IPv6 privacy extensions: %s",
+                    (enabled ? "enabling" : "disabling"), e);
             return false;
         }
         return true;
     }
 
+    /**
+     * Set IPv6 address generation mode on the interface.
+     *
+     * <p>IPv6 should be disabled before changing the mode.
+     */
     public boolean setIPv6AddrGenModeIfSupported(int mode) {
         try {
-            mNMS.setIPv6AddrGenMode(mIfName, mode);
+            mNetd.setIPv6AddrGenMode(mIfName, mode);
         } catch (RemoteException e) {
             logError("Unable to set IPv6 addrgen mode: %s", e);
             return false;
@@ -121,10 +142,16 @@ public class InterfaceController {
         return true;
     }
 
+    /**
+     * Add an address to the interface.
+     */
     public boolean addAddress(LinkAddress addr) {
         return addAddress(addr.getAddress(), addr.getPrefixLength());
     }
 
+    /**
+     * Add an address to the interface.
+     */
     public boolean addAddress(InetAddress ip, int prefixLen) {
         try {
             mNetd.interfaceAddAddress(mIfName, ip.getHostAddress(), prefixLen);
@@ -135,6 +162,9 @@ public class InterfaceController {
         return true;
     }
 
+    /**
+     * Remove an address from the interface.
+     */
     public boolean removeAddress(InetAddress ip, int prefixLen) {
         try {
             mNetd.interfaceDelAddress(mIfName, ip.getHostAddress(), prefixLen);
@@ -145,9 +175,12 @@ public class InterfaceController {
         return true;
     }
 
+    /**
+     * Remove all addresses from the interface.
+     */
     public boolean clearAllAddresses() {
         try {
-            mNMS.clearInterfaceAddresses(mIfName);
+            mNetd.interfaceClearAddrs(mIfName);
         } catch (Exception e) {
             logError("Failed to clear addresses: %s", e);
             return false;
diff --git a/services/net/java/android/net/ip/IpClient.java b/services/net/java/android/net/ip/IpClient.java
index 9f1557354dc4..62cd2d7cf2c8 100644
--- a/services/net/java/android/net/ip/IpClient.java
+++ b/services/net/java/android/net/ip/IpClient.java
@@ -479,7 +479,7 @@ public class IpClient extends StateMachine {
 
         // TODO: Consider creating, constructing, and passing in some kind of
         // InterfaceController.Dependencies class.
-        mInterfaceCtrl = new InterfaceController(mInterfaceName, mNwService, deps.getNetd(), mLog);
+        mInterfaceCtrl = new InterfaceController(mInterfaceName, deps.getNetd(), mLog);
 
         mNetlinkTracker = new NetlinkTracker(
                 mInterfaceName,
diff --git a/services/net/java/android/net/ip/IpServer.java b/services/net/java/android/net/ip/IpServer.java
index 8b22f68286af..7910c9a69310 100644
--- a/services/net/java/android/net/ip/IpServer.java
+++ b/services/net/java/android/net/ip/IpServer.java
@@ -226,7 +226,7 @@ public class IpServer extends StateMachine {
         mNetd = deps.getNetdService();
         mStatsService = statsService;
         mCallback = callback;
-        mInterfaceCtrl = new InterfaceController(ifaceName, nMService, mNetd, mLog);
+        mInterfaceCtrl = new InterfaceController(ifaceName, mNetd, mLog);
         mIfaceName = ifaceName;
         mInterfaceType = interfaceType;
         mLinkProperties = new LinkProperties();
diff --git a/tests/net/java/android/net/ip/IpClientTest.java b/tests/net/java/android/net/ip/IpClientTest.java
index a2dcfef50a49..7a83757aa262 100644
--- a/tests/net/java/android/net/ip/IpClientTest.java
+++ b/tests/net/java/android/net/ip/IpClientTest.java
@@ -122,13 +122,14 @@ public class IpClientTest {
     private IpClient makeIpClient(String ifname) throws Exception {
         setTestInterfaceParams(ifname);
         final IpClient ipc = new IpClient(mContext, ifname, mCb, mDependecies);
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).disableIpv6(ifname);
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).clearInterfaceAddresses(ifname);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceSetEnableIPv6(ifname, false);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceClearAddrs(ifname);
         ArgumentCaptor<BaseNetworkObserver> arg =
                 ArgumentCaptor.forClass(BaseNetworkObserver.class);
         verify(mNMService, times(1)).registerObserver(arg.capture());
         mObserver = arg.getValue();
         reset(mNMService);
+        reset(mNetd);
         // Verify IpClient doesn't call onLinkPropertiesChange() when it starts.
         verify(mCb, never()).onLinkPropertiesChange(any());
         reset(mCb);
@@ -200,8 +201,8 @@ public class IpClientTest {
         verify(mCb, never()).onProvisioningFailure(any());
 
         ipc.shutdown();
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).disableIpv6(iface);
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).clearInterfaceAddresses(iface);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceSetEnableIPv6(iface, false);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceClearAddrs(iface);
         verify(mCb, timeout(TEST_TIMEOUT_MS).times(1))
                 .onLinkPropertiesChange(eq(makeEmptyLinkProperties(iface)));
     }
@@ -251,8 +252,8 @@ public class IpClientTest {
         verify(mCb, timeout(TEST_TIMEOUT_MS).times(1)).onProvisioningSuccess(eq(want));
 
         ipc.shutdown();
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).disableIpv6(iface);
-        verify(mNMService, timeout(TEST_TIMEOUT_MS).times(1)).clearInterfaceAddresses(iface);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceSetEnableIPv6(iface, false);
+        verify(mNetd, timeout(TEST_TIMEOUT_MS).times(1)).interfaceClearAddrs(iface);
         verify(mCb, timeout(TEST_TIMEOUT_MS).times(1))
                 .onLinkPropertiesChange(eq(makeEmptyLinkProperties(iface)));
     }
diff --git a/tests/net/java/android/net/ip/IpServerTest.java b/tests/net/java/android/net/ip/IpServerTest.java
index c3162af1868d..80aac047a723 100644
--- a/tests/net/java/android/net/ip/IpServerTest.java
+++ b/tests/net/java/android/net/ip/IpServerTest.java
@@ -33,6 +33,7 @@ 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;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
@@ -47,6 +48,7 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
+import android.net.INetd;
 import android.net.INetworkStatsService;
 import android.net.InterfaceConfiguration;
 import android.net.IpPrefix;
@@ -92,6 +94,7 @@ public class IpServerTest {
     private static final int MAKE_DHCPSERVER_TIMEOUT_MS = 1000;
 
     @Mock private INetworkManagementService mNMService;
+    @Mock private INetd mNetd;
     @Mock private INetworkStatsService mStatsService;
     @Mock private IpServer.Callback mCallback;
     @Mock private InterfaceConfiguration mInterfaceConfiguration;
@@ -112,16 +115,6 @@ public class IpServerTest {
     }
 
     private void initStateMachine(int interfaceType, boolean usingLegacyDhcp) throws Exception {
-        mIpServer = new IpServer(
-                IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog,
-                mNMService, mStatsService, mCallback, usingLegacyDhcp, mDependencies);
-        mIpServer.start();
-        // Starting the state machine always puts us in a consistent state and notifies
-        // the rest of the world that we've changed from an unknown to available state.
-        mLooper.dispatchAll();
-        reset(mNMService, mStatsService, mCallback);
-        when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration);
-
         doAnswer(inv -> {
             final IDhcpServerCallbacks cb = inv.getArgument(2);
             new Thread(() -> {
@@ -135,6 +128,17 @@ public class IpServerTest {
         }).when(mDependencies).makeDhcpServer(any(), mDhcpParamsCaptor.capture(), any());
         when(mDependencies.getRouterAdvertisementDaemon(any())).thenReturn(mRaDaemon);
         when(mDependencies.getInterfaceParams(IFACE_NAME)).thenReturn(TEST_IFACE_PARAMS);
+        when(mDependencies.getNetdService()).thenReturn(mNetd);
+
+        mIpServer = new IpServer(
+                IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog,
+                mNMService, mStatsService, mCallback, usingLegacyDhcp, mDependencies);
+        mIpServer.start();
+        // Starting the state machine always puts us in a consistent state and notifies
+        // the rest of the world that we've changed from an unknown to available state.
+        mLooper.dispatchAll();
+        reset(mNMService, mStatsService, mCallback);
+        when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration);
 
         when(mRaDaemon.start()).thenReturn(true);
     }
@@ -223,9 +227,9 @@ public class IpServerTest {
         initTetheredStateMachine(TETHERING_BLUETOOTH, null);
 
         dispatchCommand(IpServer.CMD_TETHER_UNREQUESTED);
-        InOrder inOrder = inOrder(mNMService, mStatsService, mCallback);
+        InOrder inOrder = inOrder(mNMService, mNetd, mStatsService, mCallback);
         inOrder.verify(mNMService).untetherInterface(IFACE_NAME);
-        inOrder.verify(mNMService).setInterfaceConfig(eq(IFACE_NAME), any());
+        inOrder.verify(mNetd).interfaceSetCfg(argThat(cfg -> IFACE_NAME.equals(cfg.ifName)));
         inOrder.verify(mCallback).updateInterfaceState(
                 mIpServer, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR);
         inOrder.verify(mCallback).updateLinkProperties(
@@ -318,12 +322,12 @@ public class IpServerTest {
         initTetheredStateMachine(TETHERING_BLUETOOTH, UPSTREAM_IFACE);
 
         dispatchCommand(IpServer.CMD_TETHER_UNREQUESTED);
-        InOrder inOrder = inOrder(mNMService, mStatsService, mCallback);
+        InOrder inOrder = inOrder(mNMService, mNetd, mStatsService, mCallback);
         inOrder.verify(mStatsService).forceUpdate();
         inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE);
         inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE);
         inOrder.verify(mNMService).untetherInterface(IFACE_NAME);
-        inOrder.verify(mNMService).setInterfaceConfig(eq(IFACE_NAME), any());
+        inOrder.verify(mNetd).interfaceSetCfg(argThat(cfg -> IFACE_NAME.equals(cfg.ifName)));
         inOrder.verify(mCallback).updateInterfaceState(
                 mIpServer, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR);
         inOrder.verify(mCallback).updateLinkProperties(
diff --git a/tests/net/java/com/android/server/connectivity/TetheringTest.java b/tests/net/java/com/android/server/connectivity/TetheringTest.java
index 1ea83c2bbb6b..b6356076db60 100644
--- a/tests/net/java/com/android/server/connectivity/TetheringTest.java
+++ b/tests/net/java/com/android/server/connectivity/TetheringTest.java
@@ -803,13 +803,14 @@ public class TetheringTest {
         sendWifiApStateChanged(WIFI_AP_STATE_ENABLED, TEST_WLAN_IFNAME, IFACE_IP_MODE_TETHERED);
         mLooper.dispatchAll();
 
-        // We verify get/set called thrice here: once for setup and twice during
-        // teardown because all events happen over the course of the single
+        // We verify get/set called thrice here: twice for setup (on NMService) and once during
+        // teardown (on Netd) because all events happen over the course of the single
         // dispatchAll() above. Note that once the IpServer IPv4 address config
         // code is refactored the two calls during shutdown will revert to one.
         verify(mNMService, times(2)).getInterfaceConfig(TEST_WLAN_IFNAME);
-        verify(mNMService, times(3))
+        verify(mNMService, times(2))
                 .setInterfaceConfig(eq(TEST_WLAN_IFNAME), any(InterfaceConfiguration.class));
+        verify(mNetd, times(1)).interfaceSetCfg(argThat(p -> TEST_WLAN_IFNAME.equals(p.ifName)));
         verify(mNMService, times(1)).tetherInterface(TEST_WLAN_IFNAME);
         verify(mWifiManager).updateInterfaceIpState(
                 TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_TETHERED);