Merge branch '4.20' into 4.22

Author: Daan Hoogland

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java

@@ -53,6 +53,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap
 
     List<SnapshotVO> listByStatusNotIn(long volumeId, Snapshot.State... status);
 
+    SnapshotVO findByVolumeIdAndNameNotInStatus(long volumeId, String name, Snapshot.State... status);
+
     /**
      * Retrieves a list of snapshots filtered by ids.
      * @param ids Snapshot ids.

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java

@@ -70,6 +70,7 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
     private SearchBuilder<SnapshotVO> InstanceIdSearch;
     private SearchBuilder<SnapshotVO> StatusSearch;
     private SearchBuilder<SnapshotVO> notInStatusSearch;
+    private SearchBuilder<SnapshotVO> volumeIdNameNotInStatusSearch;
     private GenericSearchBuilder<SnapshotVO, Long> CountSnapshotsByAccount;
 
     private SearchBuilder<SnapshotVO> volumeIdAndTypeNotInSearch;
@@ -166,6 +167,12 @@ protected void init() {
         notInStatusSearch.and("status", notInStatusSearch.entity().getState(), SearchCriteria.Op.NOTIN);
         notInStatusSearch.done();
 
+        volumeIdNameNotInStatusSearch = createSearchBuilder();
+        volumeIdNameNotInStatusSearch.and("volumeId", volumeIdNameNotInStatusSearch.entity().getVolumeId(), SearchCriteria.Op.EQ);
+        volumeIdNameNotInStatusSearch.and("name", volumeIdNameNotInStatusSearch.entity().getName(), SearchCriteria.Op.EQ);
+        volumeIdNameNotInStatusSearch.and("status", volumeIdNameNotInStatusSearch.entity().getState(), SearchCriteria.Op.NOTIN);
+        volumeIdNameNotInStatusSearch.done();
+
         CountSnapshotsByAccount = createSearchBuilder(Long.class);
         CountSnapshotsByAccount.select(null, Func.COUNT, null);
         CountSnapshotsByAccount.and("account", CountSnapshotsByAccount.entity().getAccountId(), SearchCriteria.Op.EQ);
@@ -310,6 +317,15 @@ public List<SnapshotVO> listByStatusNotIn(long volumeId, Snapshot.State... statu
         return listBy(sc, null);
     }
 
+    @Override
+    public SnapshotVO findByVolumeIdAndNameNotInStatus(long volumeId, String name, Snapshot.State... status) {
+        SearchCriteria<SnapshotVO> sc = volumeIdNameNotInStatusSearch.create();
+        sc.setParameters("volumeId", volumeId);
+        sc.setParameters("name", name);
+        sc.setParameters("status", (Object[]) status);
+        return findOneBy(sc);
+    }
+
     @Override
     public List<SnapshotVO> searchByVolumes(List<Long> volumeIds) {
         if (CollectionUtils.isEmpty(volumeIds)) {

engine/schema/src/test/java/com/cloud/storage/dao/SnapshotDaoImplTest.java

@@ -0,0 +1,80 @@
+// 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.storage.dao;
+
+import java.lang.reflect.Field;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.storage.Snapshot;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SnapshotDaoImplTest {
+
+    @Mock
+    SearchBuilder<SnapshotVO> volumeIdNameNotInStatusSearchMock;
+
+    @Mock
+    SearchCriteria<SnapshotVO> searchCriteriaMock;
+
+    @Spy
+    SnapshotDaoImpl snapshotDao = new SnapshotDaoImpl();
+
+    @Before
+    public void setUp() throws Exception {
+        // init() wires up several search builders that join other DAOs, which are not available in a
+        // unit test; inject only the search builder this method uses and stub it to return a criteria.
+        Field searchField = SnapshotDaoImpl.class.getDeclaredField("volumeIdNameNotInStatusSearch");
+        searchField.setAccessible(true);
+        searchField.set(snapshotDao, volumeIdNameNotInStatusSearchMock);
+        Mockito.lenient().doReturn(searchCriteriaMock).when(volumeIdNameNotInStatusSearchMock).create();
+    }
+
+    @Test
+    public void testFindByVolumeIdAndNameNotInStatusReturnsTheMatch() {
+        SnapshotVO expected = Mockito.mock(SnapshotVO.class);
+        Mockito.doReturn(expected).when(snapshotDao).findOneBy(Mockito.any(SearchCriteria.class));
+
+        SnapshotVO result = snapshotDao.findByVolumeIdAndNameNotInStatus(5L, "snap-1", Snapshot.State.Destroyed);
+
+        Assert.assertSame(expected, result);
+        Mockito.verify(searchCriteriaMock).setParameters("volumeId", 5L);
+        Mockito.verify(searchCriteriaMock).setParameters("name", "snap-1");
+        Mockito.verify(snapshotDao).findOneBy(searchCriteriaMock);
+    }
+
+    @Test
+    public void testFindByVolumeIdAndNameNotInStatusReturnsNullWhenNoMatch() {
+        Mockito.doReturn(null).when(snapshotDao).findOneBy(Mockito.any(SearchCriteria.class));
+
+        SnapshotVO result = snapshotDao.findByVolumeIdAndNameNotInStatus(5L, "snap-1");
+
+        Assert.assertNull(result);
+        Mockito.verify(searchCriteriaMock).setParameters("volumeId", 5L);
+        Mockito.verify(snapshotDao).findOneBy(searchCriteriaMock);
+    }
+}

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

@@ -959,26 +959,8 @@ public boolean validateHAProxyLBRule(final LoadBalancingRule rule) {
 
                 }
             } else if (LbStickinessMethod.StickinessMethodType.AppCookieBased.getName().equalsIgnoreCase(stickinessPolicy.getMethodName())) {
-                String length = null; // optional
-                String holdTime = null; // optional
-
-                for (final Pair<String, String> paramKV : paramsList) {
-                    final String key = paramKV.first();
-                    final String value = paramKV.second();
-                    if ("length".equalsIgnoreCase(key)) {
-                        length = value;
-                    }
-                    if ("holdtime".equalsIgnoreCase(key)) {
-                        holdTime = value;
-                    }
-                }
-
-                if (length != null && !containsOnlyNumbers(length, null)) {
-                    throw new InvalidParameterValueException(String.format("Failed LB in validation rule id: %s Cause: length is not a number: %s", rule.getLb(), length));
-                }
-                if (holdTime != null && !containsOnlyNumbers(holdTime, timeEndChar) && !containsOnlyNumbers(holdTime, null)) {
-                    throw new InvalidParameterValueException(String.format("Failed LB in validation rule id: %s Cause: holdtime is not in timeformat: %s", rule.getLb(), holdTime));
-                }
+                throw new InvalidParameterValueException(String.format("Failed LB in validation rule: %s Cause: AppCookie based stickiness "
+                        + "(HAProxy 'appsession') is not supported on a Virtual Router, as this directive was removed in HAProxy 1.6", rule.getLb()));
             }
         }
         return true;

server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java

@@ -2028,9 +2028,14 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
         if (vmInstance != null) {
             vmDisplayName = vmInstance.getHostName();
         }
+        boolean nameProvided = snapshotName != null;
         if (snapshotName == null)
             snapshotName = vmDisplayName + "_" + volume.getName() + "_" + timeString;
 
+        if (nameProvided && _snapshotDao.findByVolumeIdAndNameNotInStatus(volumeId, snapshotName, Snapshot.State.Destroyed, Snapshot.State.Error) != null) {
+            throw new InvalidParameterValueException(String.format("A snapshot with name [%s] already exists for volume %s.", snapshotName, volume));
+        }
+
         HypervisorType hypervisorType = HypervisorType.None;
         StoragePoolVO storagePool = _storagePoolDao.findById(volume.getDataStore().getId());
         if (storagePool.getScope() == ScopeType.ZONE) {

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

@@ -29,7 +29,15 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.cloud.configuration.Config;
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.lb.LoadBalancingRule;
+import com.cloud.network.rules.LbStickinessMethod;
+import com.cloud.network.rules.LoadBalancer;
+import com.cloud.utils.Pair;
+import com.cloud.utils.net.Ip;
 import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.network.router.deployment.RouterDeploymentDefinition;
 import org.junit.Before;
 import org.junit.Test;
@@ -57,6 +65,10 @@
 import com.cloud.vm.dao.DomainRouterDao;
 import com.cloud.vm.dao.NicDao;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
 
 @RunWith(MockitoJUnitRunner.class)
 public class NetworkHelperImplTest {
@@ -91,10 +103,15 @@ public class NetworkHelperImplTest {
     @Mock
     private VMTemplateVO template;
 
+    @Mock
+    private ConfigurationDao configDao;
+
     @Before
     public void setUp() {
         nwHelper._networkDao = networkDao;
         nwHelper._networkModel = networkModel;
+        nwHelper._configDao = configDao;
+        lenient().when(configDao.getValue(Config.NetworkLBHaproxyStatsPort.key())).thenReturn("8081");
         when(template.getId()).thenReturn(1L);
         when(template.isDynamicallyScalable()).thenReturn(true);
         when(virtualProvider.getId()).thenReturn(1L);
@@ -268,4 +285,33 @@ public void testUpdateDomainRouter() {
         assertEquals(Hypervisor.HypervisorType.KVM, result.getHypervisorType());
         assertTrue(result.isDynamicallyScalable());
     }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testValidateHAProxyLBRuleRejectsAppCookieStickiness() {
+        LoadBalancer lb = mock(LoadBalancer.class);
+        when(lb.getSourcePortStart()).thenReturn(80);
+
+        List<Pair<String, String>> params = new ArrayList<>();
+        params.add(new Pair<>("cookie-name", "JSESSIONID"));
+        LoadBalancingRule.LbStickinessPolicy stickinessPolicy =
+                new LoadBalancingRule.LbStickinessPolicy(LbStickinessMethod.StickinessMethodType.AppCookieBased.getName(), params);
+        LoadBalancingRule rule = new LoadBalancingRule(lb, new ArrayList<>(), Collections.singletonList(stickinessPolicy), new ArrayList<>(), mock(Ip.class));
+
+        nwHelper.validateHAProxyLBRule(rule);
+    }
+
+    @Test
+    public void testValidateHAProxyLBRuleAllowsSourceBasedStickiness() {
+        LoadBalancer lb = mock(LoadBalancer.class);
+        when(lb.getSourcePortStart()).thenReturn(80);
+
+        List<Pair<String, String>> params = new ArrayList<>();
+        params.add(new Pair<>("tablesize", "200k"));
+        params.add(new Pair<>("expire", "30m"));
+        LoadBalancingRule.LbStickinessPolicy stickinessPolicy =
+                new LoadBalancingRule.LbStickinessPolicy(LbStickinessMethod.StickinessMethodType.SourceBased.getName(), params);
+        LoadBalancingRule rule = new LoadBalancingRule(lb, new ArrayList<>(), Collections.singletonList(stickinessPolicy), new ArrayList<>(), mock(Ip.class));
+
+        assertTrue(nwHelper.validateHAProxyLBRule(rule));
+    }
 }

server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java

@@ -24,8 +24,10 @@
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.PermissionDeniedException;
 import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.org.Grouping;
 import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.ScopeType;
 import com.cloud.storage.Snapshot;
 import com.cloud.storage.SnapshotPolicyVO;
 import com.cloud.storage.SnapshotVO;
@@ -42,9 +44,10 @@
 import com.cloud.user.User;
 import com.cloud.user.dao.AccountDao;
 import com.cloud.utils.Pair;
-
 import com.cloud.utils.db.SearchBuilder;
 import com.cloud.utils.db.SearchCriteria;
+import com.cloud.vm.dao.UserVmDao;
+
 import org.apache.cloudstack.api.command.user.snapshot.DeleteSnapshotPoliciesCmd;
 import org.apache.cloudstack.api.command.user.snapshot.ListSnapshotPoliciesCmd;
 import org.apache.cloudstack.context.CallContext;
@@ -55,9 +58,13 @@
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
 import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 
 import org.junit.Assert;
 import org.junit.After;
@@ -106,6 +113,13 @@ public class SnapshotManagerImplTest {
     SnapshotScheduler snapshotScheduler;
     @Mock
     TaggedResourceService taggedResourceService;
+    @Mock
+    PrimaryDataStoreDao primaryDataStoreDao;
+    @Mock
+    VolumeDataFactory volFactory;
+    @Mock
+    UserVmDao userVmDao;
+
     @InjectMocks
     SnapshotManagerImpl snapshotManager = new SnapshotManagerImpl();
 
@@ -123,6 +137,69 @@ public void tearDown() {
         CallContext.unregister();
     }
 
+    @Test
+    public void testAllocSnapshotRejectsDuplicateNameForVolume() {
+        long volumeId = 1L;
+        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+        try {
+            VolumeInfo volume = Mockito.mock(VolumeInfo.class);
+            Mockito.when(volFactory.getVolume(volumeId)).thenReturn(volume);
+            DataStore dataStore = Mockito.mock(DataStore.class);
+            Mockito.when(volume.getDataStore()).thenReturn(dataStore);
+            Mockito.when(dataStore.getId()).thenReturn(10L);
+            StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
+            Mockito.when(primaryDataStoreDao.findById(10L)).thenReturn(pool);
+            Mockito.when(pool.getScope()).thenReturn(ScopeType.ZONE);
+            Mockito.when(pool.getHypervisor()).thenReturn(HypervisorType.None);
+            Mockito.when(volume.getInstanceId()).thenReturn(null);
+            Mockito.when(volume.getAccountId()).thenReturn(2L);
+
+            Mockito.when(snapshotDao.findByVolumeIdAndNameNotInStatus(volumeId, "dup", Snapshot.State.Destroyed, Snapshot.State.Error))
+                    .thenReturn(Mockito.mock(SnapshotVO.class));
+
+            Assert.assertThrows(InvalidParameterValueException.class, () ->
+                    snapshotManager.allocSnapshot(volumeId, Snapshot.MANUAL_POLICY_ID, "dup", null));
+        } finally {
+            CallContext.unregister();
+        }
+    }
+
+    @Test
+    public void testAllocSnapshotDoesNotCheckDuplicateForAutoGeneratedName() {
+        long volumeId = 1L;
+        CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
+        try {
+            VolumeInfo volume = Mockito.mock(VolumeInfo.class);
+            Mockito.when(volFactory.getVolume(volumeId)).thenReturn(volume);
+            DataStore dataStore = Mockito.mock(DataStore.class);
+            Mockito.when(volume.getDataStore()).thenReturn(dataStore);
+            Mockito.lenient().when(dataStore.getId()).thenReturn(10L);
+            StoragePoolVO pool = Mockito.mock(StoragePoolVO.class);
+            Mockito.lenient().when(primaryDataStoreDao.findById(10L)).thenReturn(pool);
+            Mockito.lenient().when(pool.getScope()).thenReturn(ScopeType.ZONE);
+            Mockito.lenient().when(pool.getHypervisor()).thenReturn(HypervisorType.None);
+            Mockito.when(volume.getInstanceId()).thenReturn(null);
+            Mockito.when(volume.getAccountId()).thenReturn(2L);
+
+            // A recurring snapshot carries no caller-supplied name, so the auto-generated name must not be checked.
+            Mockito.lenient().when(snapshotDao.findByVolumeIdAndNameNotInStatus(Mockito.eq(volumeId), Mockito.anyString(),
+                    Mockito.eq(Snapshot.State.Destroyed), Mockito.eq(Snapshot.State.Error)))
+                    .thenReturn(Mockito.mock(SnapshotVO.class));
+
+            try {
+                snapshotManager.allocSnapshot(volumeId, Snapshot.MANUAL_POLICY_ID, null, null);
+            } catch (Exception e) {
+                // The rest of allocSnapshot is not stubbed here; this test only asserts the duplicate-name guard is
+                // not applied to an auto-generated name.
+            }
+
+            Mockito.verify(snapshotDao, Mockito.never()).findByVolumeIdAndNameNotInStatus(Mockito.eq(volumeId),
+                    Mockito.anyString(), Mockito.eq(Snapshot.State.Destroyed), Mockito.eq(Snapshot.State.Error));
+        } finally {
+            CallContext.unregister();
+        }
+    }
+
     @Test
     public void testGetSnapshotZoneImageStoreValid() {
         final long snapshotId = 1L;