From 0ac84b08dd651f7b5ed91bfad8d1052bbdb0d969 Mon Sep 17 00:00:00 2001 From: Shawn Edwards Date: Tue, 13 Jan 2026 11:17:21 -0600 Subject: [PATCH] Move dhcp lease timeout to ConfigKey, set it to 'zone' scope, and add some tests. --- .../service/NetworkOrchestrationService.java | 4 + .../java/com/cloud/configuration/Config.java | 9 - .../network/router/CommandSetupHelper.java | 5 +- .../router/CommandSetupHelperTest.java | 50 +++++ .../DhcpLeaseTimeoutIntegrationTest.java | 190 ++++++++++++++++++ 5 files changed, 246 insertions(+), 12 deletions(-) create mode 100644 server/src/test/java/com/cloud/network/router/DhcpLeaseTimeoutIntegrationTest.java diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java index 947cbd8e618..23bd7aa13b4 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java @@ -91,6 +91,10 @@ public interface NetworkOrchestrationService { ConfigKey NetworkThrottlingRate = new ConfigKey<>("Network", Integer.class, NetworkThrottlingRateCK, "200", "Default data transfer rate in megabits per second allowed in network.", true, ConfigKey.Scope.Zone); + ConfigKey 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 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); diff --git a/server/src/main/java/com/cloud/configuration/Config.java b/server/src/main/java/com/cloud/configuration/Config.java index 518c2168e9c..abae4d3996c 100644 --- a/server/src/main/java/com/cloud/configuration/Config.java +++ b/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", diff --git a/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java b/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java index 78a2be99be5..39775cc08af 100644 --- a/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java +++ b/server/src/main/java/com/cloud/network/router/CommandSetupHelper.java @@ -296,9 +296,8 @@ public class CommandSetupHelper { 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())); diff --git a/server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java b/server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java index bdd905841f9..5abc4ee255f 100644 --- a/server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java +++ b/server/src/test/java/com/cloud/network/router/CommandSetupHelperTest.java @@ -46,6 +46,8 @@ import com.cloud.utils.net.Ip; 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 class CommandSetupHelperTest { 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 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 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 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 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 configKey = NetworkOrchestrationService.DhcpLeaseTimeout; + Assert.assertTrue("ConfigKey should be dynamic", configKey.isDynamic()); + } } diff --git a/server/src/test/java/com/cloud/network/router/DhcpLeaseTimeoutIntegrationTest.java b/server/src/test/java/com/cloud/network/router/DhcpLeaseTimeoutIntegrationTest.java new file mode 100644 index 00000000000..7de699aa4a1 --- /dev/null +++ b/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 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()); + } +}