Move dhcp lease timeout to ConfigKey, set it to 'zone' scope, and add some tests.

Author: NVShawn

engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java

@@ -91,6 +91,10 @@ public interface NetworkOrchestrationService {
     ConfigKey<Integer> NetworkThrottlingRate = new ConfigKey<>("Network", Integer.class, NetworkThrottlingRateCK, "200",
             "Default data transfer rate in megabits per second allowed in network.", true, ConfigKey.Scope.Zone);
 
+    ConfigKey<Integer> DhcpLeaseTimeout = new ConfigKey<>("Network", Integer.class, "dhcp.lease.timeout", "0",
+            "DHCP lease time in seconds for VMs. Use 0 for infinite lease time (default). A non-zero value sets the lease duration in seconds.",
+            true, ConfigKey.Scope.Zone, "0-");
+
     ConfigKey<Boolean> PromiscuousMode = new ConfigKey<>("Advanced", Boolean.class, "network.promiscuous.mode", "false",
             "Whether to allow or deny promiscuous mode on NICs for applicable network elements such as for vswitch/dvswitch portgroups.", true);
 

server/src/main/java/com/cloud/configuration/Config.java

@@ -351,15 +351,6 @@ public enum Config {
             ConfigKey.Kind.CSV,
             null),
 
-    DhcpLeaseTimeout(
-            "Network",
-            ManagementServer.class,
-            Integer.class,
-            "dhcp.lease.timeout",
-            "0",
-            "DHCP lease time in seconds for VMs. Use 0 for infinite lease time (default). A non-zero value sets the lease duration in seconds.",
-            null),
-
     //VPN
     RemoteAccessVpnPskLength(
             "Network",

server/src/main/java/com/cloud/network/router/CommandSetupHelper.java

@@ -296,9 +296,8 @@ public void createDhcpEntryCommand(final VirtualRouter router, final UserVm vm,
         dhcpCommand.setDefault(nic.isDefaultNic());
         dhcpCommand.setRemove(remove);
 
-        // Set DHCP lease timeout from global config (0 = infinite)
-        String leaseTimeStr = _configDao.getValue(Config.DhcpLeaseTimeout.key());
-        Long leaseTime = (leaseTimeStr != null) ? Long.parseLong(leaseTimeStr) : 0L;
+        // Set DHCP lease timeout from zone-scoped config (0 = infinite)
+        Long leaseTime = (long) NetworkOrchestrationService.DhcpLeaseTimeout.valueIn(router.getDataCenterId());
         dhcpCommand.setLeaseTime(leaseTime);
 
         dhcpCommand.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));

server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java

@@ -46,6 +46,8 @@
 import com.cloud.vm.NicVO;
 import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.dao.NicDao;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
+import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.network.BgpPeerVO;
 import org.apache.cloudstack.network.dao.BgpPeerDetailsDao;
 import org.junit.Assert;
@@ -271,4 +273,52 @@ public void testCreateBgpPeersCommandsForVpc() {
         Assert.assertTrue(cmd instanceof SetBgpPeersCommand);
         Assert.assertEquals(4, ((SetBgpPeersCommand) cmd).getBpgPeers().length);
     }
+
+    @Test
+    public void testDhcpLeaseTimeoutDefaultValue() {
+        // Test that the default value is 0 (infinite)
+        Integer defaultValue = NetworkOrchestrationService.DhcpLeaseTimeout.value();
+        Assert.assertEquals("Default DHCP lease timeout should be 0 (infinite)", 0, defaultValue.intValue());
+    }
+
+    @Test
+    public void testDhcpLeaseTimeoutAcceptsZero() {
+        // Test that 0 value is accepted (infinite lease)
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        Assert.assertNotNull("ConfigKey should not be null", configKey);
+        Assert.assertEquals("ConfigKey default should be 0", "0", configKey.defaultValue());
+    }
+
+    @Test
+    public void testDhcpLeaseTimeoutAcceptsPositiveValues() {
+        // Test that positive values are accepted
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        Assert.assertNotNull("ConfigKey should not be null", configKey);
+        // Verify the validator allows positive values
+        String validator = configKey.range();
+        Assert.assertNotNull("Validator should not be null", validator);
+        Assert.assertEquals("Validator should be '0-' (>= 0)", "0-", validator);
+    }
+
+    @Test
+    public void testDhcpLeaseTimeoutValidatorRejectsNegative() {
+        // Test that the validator is configured to reject negative values
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        String validator = configKey.range();
+        Assert.assertEquals("Validator should enforce minimum of 0", "0-", validator);
+    }
+
+    @Test
+    public void testDhcpLeaseTimeoutHasZoneScope() {
+        // Test that the ConfigKey has Zone scope
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        Assert.assertEquals("ConfigKey should have Zone scope", ConfigKey.Scope.Zone, configKey.scope());
+    }
+
+    @Test
+    public void testDhcpLeaseTimeoutIsDynamic() {
+        // Test that the ConfigKey is dynamic (can be updated at runtime)
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        Assert.assertTrue("ConfigKey should be dynamic", configKey.isDynamic());
+    }
 }

server/src/test/java/com/cloud/network/router/DhcpLeaseTimeoutIntegrationTest.java

@@ -0,0 +1,190 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.network.router;
+
+import com.cloud.agent.api.routing.DhcpEntryCommand;
+import com.cloud.agent.manager.Commands;
+import com.cloud.dc.DataCenterVO;
+import com.cloud.dc.dao.DataCenterDao;
+import com.cloud.network.NetworkModel;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkDetailsDao;
+import com.cloud.offerings.dao.NetworkOfferingDao;
+import com.cloud.offerings.dao.NetworkOfferingDetailsDao;
+import com.cloud.user.UserVmVO;
+import com.cloud.vm.NicVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.NicDao;
+import com.cloud.vm.dao.UserVmDao;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.when;
+
+/**
+ * Integration tests for DHCP lease timeout functionality.
+ * Tests the end-to-end flow from ConfigKey through DhcpEntryCommand creation.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class DhcpLeaseTimeoutIntegrationTest {
+
+    @InjectMocks
+    protected CommandSetupHelper commandSetupHelper = new CommandSetupHelper();
+
+    @Mock
+    NicDao nicDao;
+    @Mock
+    NetworkDao networkDao;
+    @Mock
+    NetworkModel networkModel;
+    @Mock
+    NetworkOfferingDao networkOfferingDao;
+    @Mock
+    NetworkOfferingDetailsDao networkOfferingDetailsDao;
+    @Mock
+    NetworkDetailsDao networkDetailsDao;
+    @Mock
+    RouterControlHelper routerControlHelper;
+    @Mock
+    DataCenterDao dcDao;
+    @Mock
+    UserVmDao userVmDao;
+
+    private VirtualRouter mockRouter;
+    private UserVmVO mockVm;
+    private NicVO mockNic;
+    private DataCenterVO mockDc;
+
+    @Before
+    public void setUp() {
+        ReflectionTestUtils.setField(commandSetupHelper, "_nicDao", nicDao);
+        ReflectionTestUtils.setField(commandSetupHelper, "_networkDao", networkDao);
+        ReflectionTestUtils.setField(commandSetupHelper, "_networkModel", networkModel);
+        ReflectionTestUtils.setField(commandSetupHelper, "_networkOfferingDao", networkOfferingDao);
+        ReflectionTestUtils.setField(commandSetupHelper, "networkOfferingDetailsDao", networkOfferingDetailsDao);
+        ReflectionTestUtils.setField(commandSetupHelper, "networkDetailsDao", networkDetailsDao);
+        ReflectionTestUtils.setField(commandSetupHelper, "_routerControlHelper", routerControlHelper);
+        ReflectionTestUtils.setField(commandSetupHelper, "_dcDao", dcDao);
+
+        // Create common mocks
+        mockRouter = Mockito.mock(VirtualRouter.class);
+        mockVm = Mockito.mock(UserVmVO.class);
+        mockNic = Mockito.mock(NicVO.class);
+        mockDc = Mockito.mock(DataCenterVO.class);
+
+        // Setup default mock behaviors
+        when(mockRouter.getId()).thenReturn(100L);
+        when(mockRouter.getInstanceName()).thenReturn("r-100-VM");
+        when(mockRouter.getDataCenterId()).thenReturn(1L);
+
+        when(mockVm.getId()).thenReturn(200L);
+        when(mockVm.getHostName()).thenReturn("test-vm");
+
+        when(mockNic.getId()).thenReturn(300L);
+        when(mockNic.getMacAddress()).thenReturn("02:00:0a:0b:0c:0d");
+        when(mockNic.getIPv4Address()).thenReturn("10.1.1.10");
+        when(mockNic.getIPv6Address()).thenReturn(null);
+        when(mockNic.getNetworkId()).thenReturn(400L);
+        when(mockNic.isDefaultNic()).thenReturn(true);
+
+        when(dcDao.findById(anyLong())).thenReturn(mockDc);
+        when(routerControlHelper.getRouterControlIp(anyLong())).thenReturn("10.1.1.1");
+        when(routerControlHelper.getRouterIpInNetwork(anyLong(), anyLong())).thenReturn("10.1.1.1");
+        when(networkModel.getExecuteInSeqNtwkElmtCmd()).thenReturn(false);
+    }
+
+    @Test
+    public void testDhcpEntryCommandContainsLeaseTime() {
+        // Test that DhcpEntryCommand includes the lease time from ConfigKey
+        Commands cmds = new Commands(null);
+        commandSetupHelper.createDhcpEntryCommand(mockRouter, mockVm, mockNic, false, cmds);
+
+        Assert.assertEquals("Should have one DHCP command", 1, cmds.size());
+        DhcpEntryCommand dhcpCmd = (DhcpEntryCommand) cmds.toCommands()[0];
+        Assert.assertNotNull("DHCP command should not be null", dhcpCmd);
+        Assert.assertNotNull("Lease time should not be null", dhcpCmd.getLeaseTime());
+        
+        // Default value should be 0 (infinite)
+        Assert.assertEquals("Default lease time should be 0", Long.valueOf(0L), dhcpCmd.getLeaseTime());
+    }
+
+    @Test
+    public void testDhcpEntryCommandUsesZoneScopedValue() {
+        // Test that the command uses zone-scoped configuration
+        Long zoneId = mockRouter.getDataCenterId();
+        Integer expectedLeaseTime = NetworkOrchestrationService.DhcpLeaseTimeout.valueIn(zoneId);
+
+        Commands cmds = new Commands(null);
+        commandSetupHelper.createDhcpEntryCommand(mockRouter, mockVm, mockNic, false, cmds);
+
+        DhcpEntryCommand dhcpCmd = (DhcpEntryCommand) cmds.toCommands()[0];
+        Assert.assertEquals("Lease time should match zone-scoped config",
+                expectedLeaseTime.longValue(), dhcpCmd.getLeaseTime().longValue());
+    }
+
+    @Test
+    public void testInfiniteLeaseWithZeroValue() {
+        // Test that 0 value represents infinite lease
+        ConfigKey<Integer> configKey = NetworkOrchestrationService.DhcpLeaseTimeout;
+        Assert.assertEquals("Default value should be 0 for infinite lease", "0", configKey.defaultValue());
+
+        Commands cmds = new Commands(null);
+        commandSetupHelper.createDhcpEntryCommand(mockRouter, mockVm, mockNic, false, cmds);
+
+        DhcpEntryCommand dhcpCmd = (DhcpEntryCommand) cmds.toCommands()[0];
+        Assert.assertEquals("Lease time 0 represents infinite lease", Long.valueOf(0L), dhcpCmd.getLeaseTime());
+    }
+
+    @Test
+    public void testDhcpCommandForNonDefaultNic() {
+        // Test DHCP command creation for non-default NIC
+        when(mockNic.isDefaultNic()).thenReturn(false);
+
+        Commands cmds = new Commands(null);
+        commandSetupHelper.createDhcpEntryCommand(mockRouter, mockVm, mockNic, false, cmds);
+
+        DhcpEntryCommand dhcpCmd = (DhcpEntryCommand) cmds.toCommands()[0];
+        Assert.assertNotNull("DHCP command should be created for non-default NIC", dhcpCmd);
+        Assert.assertNotNull("Lease time should be set even for non-default NIC", dhcpCmd.getLeaseTime());
+        Assert.assertFalse("Command should reflect non-default NIC", dhcpCmd.isDefault());
+    }
+
+    @Test
+    public void testDhcpCommandWithRemoveFlag() {
+        // Test DHCP command with remove flag set
+        Commands cmds = new Commands(null);
+        commandSetupHelper.createDhcpEntryCommand(mockRouter, mockVm, mockNic, true, cmds);
+
+        DhcpEntryCommand dhcpCmd = (DhcpEntryCommand) cmds.toCommands()[0];
+        Assert.assertNotNull("DHCP command should be created even with remove flag", dhcpCmd);
+        Assert.assertTrue("Remove flag should be set", dhcpCmd.isRemove());
+        // Lease time should still be included even for removal
+        Assert.assertNotNull("Lease time should be present even for removal", dhcpCmd.getLeaseTime());
+    }
+}