diff --git a/api/src/main/java/com/cloud/server/ResourceTag.java b/api/src/main/java/com/cloud/server/ResourceTag.java index b3026deceff8..7b49a19d041f 100644 --- a/api/src/main/java/com/cloud/server/ResourceTag.java +++ b/api/src/main/java/com/cloud/server/ResourceTag.java @@ -71,7 +71,7 @@ public enum ResourceObjectType { NetworkOffering(false, true), VpcOffering(true, false), Domain(false, false, true), - ObjectStore(false, false, true); + ObjectStore(false, true, true); ResourceObjectType(boolean resourceTagsSupport, boolean resourceMetadataSupport) { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmd.java index 497179d25ef1..2f0d1cc0b57e 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmd.java @@ -18,6 +18,10 @@ package org.apache.cloudstack.api.command.admin.storage; import org.apache.cloudstack.storage.object.ObjectStore; + +import java.util.HashMap; +import java.util.Map; + import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; @@ -41,9 +45,17 @@ public class UpdateObjectStoragePoolCmd extends BaseCmd { @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "the name for the object store") private String name; + @Parameter(name = ApiConstants.PROVIDER, type = CommandType.STRING, description = "the object store provider name") + private String providerName; + @Parameter(name = ApiConstants.URL, type = CommandType.STRING, description = "the url for the object store") private String url; + @Parameter(name = ApiConstants.DETAILS, + type = CommandType.MAP, + description = "the details for the object store. Example: details[0].key=accesskey&details[0].value=s389ddssaa&details[1].key=secretkey&details[1].value=8dshfsss") + private Map details; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// @@ -61,6 +73,24 @@ public String getUrl() { return url; } + public String getProviderName() { + return providerName; + } + + public Map getDetails() { + Map detailsMap = null; + if (details != null && !details.isEmpty()) { + detailsMap = new HashMap<>(); + for (Object prop : details.values()) { + HashMap detail = (HashMap) prop; + String key = detail.get(ApiConstants.KEY); + String value = detail.get(ApiConstants.VALUE); + detailsMap.put(key, value); + } + } + return detailsMap; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -69,7 +99,7 @@ public String getUrl() { public void execute() { ObjectStore result = _storageService.updateObjectStore(getId(), this); - ObjectStoreResponse storeResponse = null; + ObjectStoreResponse storeResponse; if (result != null) { storeResponse = _responseGenerator.createObjectStoreResponse(result); storeResponse.setResponseName(getCommandName()); diff --git a/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmdTest.java index 307d80aa3f83..1eeb5e1e02cf 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/admin/storage/UpdateObjectStoragePoolCmdTest.java @@ -19,11 +19,13 @@ package org.apache.cloudstack.api.command.admin.storage; import com.cloud.storage.StorageService; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.api.response.ObjectStoreResponse; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.storage.object.ObjectStore; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -32,6 +34,9 @@ import org.mockito.Spy; import org.springframework.test.util.ReflectionTestUtils; +import java.util.HashMap; +import java.util.Map; + import static org.mockito.ArgumentMatchers.any; public class UpdateObjectStoragePoolCmdTest { @@ -83,4 +88,32 @@ public void testUpdateObjectStore() { .updateObjectStore(1L, updateObjectStoragePoolCmd); } + @Test + public void testGetDetailsNone() { + Assert.assertNull(updateObjectStoragePoolCmd.getDetails()); + } + + @Test + public void testGetDetails() { + // test setup + // Build the "details" Map which has the following format: + // {0={value=value0, key=key0}, 1={value=value1, key=key1}, ...} + Map details = new HashMap<>(); + Map map0 = new HashMap<>(); + map0.put(ApiConstants.KEY, "key0"); + map0.put(ApiConstants.VALUE, "value0"); + details.put("0", map0); + Map map1 = new HashMap<>(); + map1.put(ApiConstants.KEY, "key1"); + map1.put(ApiConstants.VALUE, "value1"); + details.put("1", map1); + ReflectionTestUtils.setField(updateObjectStoragePoolCmd, "details", details); + + // Test: getDetails() should return a simple map + Map outDetails = updateObjectStoragePoolCmd.getDetails(); + Assert.assertNotNull(outDetails); + Assert.assertEquals(2, outDetails.size()); + Assert.assertEquals("value0", outDetails.get("key0")); + Assert.assertEquals("value1", outDetails.get("key1")); + } } diff --git a/engine/api/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreEntity.java b/engine/api/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreEntity.java index 7efb72d23b27..0040b1961956 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreEntity.java +++ b/engine/api/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreEntity.java @@ -29,6 +29,8 @@ public interface ObjectStoreEntity extends DataStore, ObjectStore { List listBuckets(); + void verifyServiceConnectivity(); + boolean createUser(long accountId); boolean deleteBucket(BucketTO bucket); diff --git a/engine/storage/object/src/main/java/org/apache/cloudstack/storage/object/store/ObjectStoreImpl.java b/engine/storage/object/src/main/java/org/apache/cloudstack/storage/object/store/ObjectStoreImpl.java index a96d87ada045..ae28756ce43d 100644 --- a/engine/storage/object/src/main/java/org/apache/cloudstack/storage/object/store/ObjectStoreImpl.java +++ b/engine/storage/object/src/main/java/org/apache/cloudstack/storage/object/store/ObjectStoreImpl.java @@ -160,6 +160,11 @@ public List listBuckets() { return driver.listBuckets(objectStoreVO.getId()); } + @Override + public void verifyServiceConnectivity() { + driver.verifyServiceConnectivity(objectStoreVO.getId()); + } + /* Create user if not exists */ diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreDriver.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreDriver.java index 13aaf7c002ef..9b1564e01b68 100644 --- a/engine/storage/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreDriver.java +++ b/engine/storage/src/main/java/org/apache/cloudstack/storage/object/ObjectStoreDriver.java @@ -21,6 +21,7 @@ import com.amazonaws.services.s3.model.AccessControlList; import com.amazonaws.services.s3.model.BucketPolicy; import com.cloud.agent.api.to.BucketTO; +import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreDriver; import java.util.List; @@ -31,6 +32,13 @@ public interface ObjectStoreDriver extends DataStoreDriver { List listBuckets(long storeId); + /** + * Verify Service Connectivity by testing the configuration. + * @param storeId identifies which store's configuration to verify. + * @throws CloudRuntimeException if there are any connectivity issues. + */ + void verifyServiceConnectivity(long storeId); + boolean deleteBucket(BucketTO bucket, long storeId); AccessControlList getBucketAcl(BucketTO bucket, long storeId); diff --git a/plugins/storage/object/ceph/src/main/java/org/apache/cloudstack/storage/datastore/driver/CephObjectStoreDriverImpl.java b/plugins/storage/object/ceph/src/main/java/org/apache/cloudstack/storage/datastore/driver/CephObjectStoreDriverImpl.java index 5b5eaa08dc55..9ff45e81bc13 100644 --- a/plugins/storage/object/ceph/src/main/java/org/apache/cloudstack/storage/datastore/driver/CephObjectStoreDriverImpl.java +++ b/plugins/storage/object/ceph/src/main/java/org/apache/cloudstack/storage/datastore/driver/CephObjectStoreDriverImpl.java @@ -138,6 +138,12 @@ public List listBuckets(long storeId) { return bucketsList; } + @Override + public void verifyServiceConnectivity(long storeId) { + // ideally just ping the storage using credentials. listBuckets() for now. + listBuckets(storeId); + } + @Override public boolean deleteBucket(BucketTO bucket, long storeId) { RgwAdmin rgwAdmin = getRgwAdminClient(storeId); diff --git a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImpl.java b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImpl.java index a0a717f52e42..2df151654d5f 100644 --- a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImpl.java +++ b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImpl.java @@ -123,14 +123,15 @@ protected String getHyperStoreGroupId(Domain domain) { /** * Create the HyperStore user resources matching this account if it doesn't exist. - * + *

* The following resources are created for the account: - * - HyperStore Group to match the CloudStack Domain UUID - * - HyperStore User to match the CloudStack Account UUID - * - HyperStore Root User Credentials to manage Account Buckets etc (kept private to this plugin) - * - HyperStore IAM User with IAM policy granting all S3 actions except create/delete buckets. - * - HyperStore IAM User Credentials (visible to end user as part of Bucket Details) - * + *
    + *
  • HyperStore Group to match the CloudStack Domain UUID
    • + *
  • HyperStore User to match the CloudStack Account UUID
    • + *
  • HyperStore Root User Credentials to manage Account Buckets etc (kept private to this plugin)
    • + *
  • HyperStore IAM User with IAM policy granting all S3 actions except create/delete buckets.
    • + *
  • HyperStore IAM User Credentials (visible to end user as part of Bucket Details)
    • + *
* @param accountId the CloudStack account * @param storeId the object store. * @@ -161,8 +162,8 @@ public boolean createUser(long accountId, long storeId) { logger.error(msg); throw new CloudRuntimeException(msg); } else { - // User exists and is active. We know that the group therefore exists but - // we should ensure that it is active or it will lead to unknown access key errors + // User exists and is active. We know that the group therefore exists, but + // we should ensure that it is active, or it will lead to unknown access key errors // which might confuse the administrator. Checking is clearer. CloudianGroup group = client.listGroup(hsGroupId); if (group != null && ! group.getActive()) { @@ -187,7 +188,7 @@ public boolean createUser(long accountId, long storeId) { /** * Create IAM credentials if required. - * + *

* When the HyperStore user is first created, this method will create an IAM User with an appropriate * permission policy and a set of credentials which will be returned. * After the first run, the IAM resources should already be in place in which case we just ensure @@ -238,7 +239,7 @@ protected AccessKey createIAMCredentials(long storeId, Map detai iamClient.putUserPolicy(new PutUserPolicyRequest(iamUser, CloudianHyperStoreUtil.IAM_USER_POLICY_NAME, CloudianHyperStoreUtil.IAM_USER_POLICY)); if (! createdUser && iamAccessKeyId == null) { - // User already exists but we never saved any access key before. We should try clean up + // User already exists, but we never saved any access key before. We should try clean up logger.debug("Looking for any un-managed IAM credentials for IAM User {}", iamUser); ListAccessKeysResult listRes = iamClient.listAccessKeys(new ListAccessKeysRequest().withUserName(iamUser)); for (AccessKeyMetadata accessKeyMetadata : listRes.getAccessKeyMetadata()) { @@ -379,7 +380,6 @@ private void createHSGroup(CloudianClient client, String hsGroupId, Domain domai } // Group exists and is enabled. Nothing to log. - return; } /** @@ -390,7 +390,7 @@ private void createHSGroup(CloudianClient client, String hsGroupId, Domain domai * @param hsGroupId the group to add him to * @param account the account the user represents * @return user object if successfully created, null otherwise - * @throws ServerAPIException if on other other. + * @throws CloudRuntimeException if any exception occurs */ private CloudianUser createHSUser(CloudianClient client, String hsUserId, String hsGroupId, Account account) { CloudianUser user = new CloudianUser(); @@ -401,7 +401,7 @@ private CloudianUser createHSUser(CloudianClient client, String hsUserId, String user.setFullName(account.getAccountName()); if (! client.addUser(user)) { - // The failure shouldn't be that the user already exists at this point so its something else. + // The failure shouldn't be that the user already exists at this point so it's something else. logger.error("Failed to add user id={} groupId={}", hsUserId, hsGroupId); return null; } else { @@ -426,7 +426,7 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) { long storeId = bucket.getObjectStoreId(); long accountId = bucket.getAccountId(); - // get an s3client using Account Root User Credentials + // get an S3 client using Account Root User Credentials Map storeDetails = _storeDetailsDao.getDetails(storeId); String s3url = storeDetails.get(CloudianHyperStoreUtil.STORE_DETAILS_KEY_S3_URL); Map accountDetails = _accountDetailsDao.findDetails(accountId); @@ -449,14 +449,14 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) { } // Step 2: Any Exception here, we try to delete the bucket. - // If deletion fails, it is not the end of the world as the - // user can try again to create the bucket which if he is - // already the owner, it will succeed. + // If deletion fails, it is not the end of the world. The + // user can try to create the bucket again as it will succeed + // if he is already the owner. try { // Enable a permissive CORS configuration configureBucketCORS(s3client, bucketName); - // Update the Bucket Information (for Bucket details page etc) + // Update the Bucket Information (for Bucket details page etc.) BucketVO bucketVO = _bucketDao.findById(bucket.getId()); bucketVO.setAccessKey(iamAccessKey); bucketVO.setSecretKey(iamSecretKey); @@ -478,15 +478,15 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) { /** * Configure a permissive CrossOrigin setting on the given bucket. - * + *

* Cloudian does not enable CORS by default. The CORS configuration * is required by CloudStack so that the Javascript S3 bucket * browser can function properly. - * + *

* This method does not catch any exceptions which should be caught * by the calling method. * - * @param s3client bucket owner s3client + * @param s3client bucket owner S3 connection * @param bucketName the bucket name. * * @throws AmazonClientException and derivatives @@ -494,7 +494,7 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) { private void configureBucketCORS(AmazonS3 s3client, String bucketName) { logger.debug("Configuring CORS for bucket {}", bucketName); - List corsRules = new ArrayList(); + List corsRules = new ArrayList<>(); CORSRule allowAnyRule = new CORSRule().withId("AllowAny"); allowAnyRule.setAllowedOrigins("*"); allowAnyRule.setAllowedHeaders("*"); @@ -523,7 +523,7 @@ private void configureBucketCORS(AmazonS3 s3client, String bucketName) { @Override public List listBuckets(long storeId) { Map bucketUsage = getAllBucketsUsage(storeId); - List bucketList = new ArrayList(); + List bucketList = new ArrayList<>(); for (String bucketName : bucketUsage.keySet()) { Bucket bucket = new BucketObject(); bucket.setName(bucketName); @@ -532,24 +532,47 @@ public List listBuckets(long storeId) { return bucketList; } + /** + * Verify Service Connectivity by testing the configuration. + * @param storeId identifies which store's configuration to verify. + * @throws CloudRuntimeException if there are any connectivity issues. + */ + @Override + public void verifyServiceConnectivity(long storeId) { + // 1. test Admin Service connectivity. + CloudianClient cloudianClient = getCloudianClientByStoreId(storeId); + String version = cloudianClient.getServerVersion(); + + // 2. test S3 Service connectivity. + Map storeDetails = _storeDetailsDao.getDetails(storeId); + String s3Url = storeDetails.get(CloudianHyperStoreUtil.STORE_DETAILS_KEY_S3_URL); + CloudianHyperStoreUtil.validateS3Url(s3Url); + + // 3. test IAM Service connectivity. + String iamUrl = storeDetails.get(CloudianHyperStoreUtil.STORE_DETAILS_KEY_IAM_URL); + CloudianHyperStoreUtil.validateIAMUrl(iamUrl); + + logger.info("Connectivity confirmed to Cloudian HyperStore {}", version); + } + /** * Delete an empty bucket. * This operation fails if the bucket is not empty. * @param bucket the bucket to delete * @param storeId the store the bucket belongs to. - * @returns true on success or throws an exception. + * @return true on success or throws an exception. * @throws CloudRuntimeException if the bucket deletion fails */ @Override public boolean deleteBucket(BucketTO bucket, long storeId) { AmazonS3 s3client = getS3ClientByBucketAndStore(bucket, storeId); - logger.debug("Deleting bucket {}", bucket.getName()); + logger.debug("Deleting the bucket {}", bucket.getName()); try { s3client.deleteBucket(bucket.getName()); - logger.info("Successfully deleted bucket {}", bucket.getName()); + logger.info("Successfully deleted the bucket {}", bucket.getName()); return true; } catch (AmazonClientException e) { - logger.error("Failed to delete bucket " + bucket.getName(), e); + logger.error("Failed to delete bucket {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -563,7 +586,7 @@ public AccessControlList getBucketAcl(BucketTO bucket, long storeId) { logger.info("Successfully got the bucket ACL for {}", bucket.getName()); return acl; } catch (AmazonClientException e) { - logger.error("Failed to get the bucket ACL for " + bucket.getName(), e); + logger.error("Failed to get the bucket ACL for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -575,9 +598,8 @@ public void setBucketAcl(BucketTO bucket, AccessControlList acl, long storeId) { try { s3client.setBucketAcl(bucket.getName(), acl); logger.info("Successfully set the bucket ACL for {}", bucket.getName()); - return; } catch (AmazonClientException e) { - logger.error("Failed to set the bucket ACL for " + bucket.getName(), e); + logger.error("Failed to set the bucket ACL for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -594,30 +616,28 @@ public void setBucketPolicy(BucketTO bucket, String policy, long storeId) { return; } - StringBuilder sb = new StringBuilder(); - sb.append("{\n"); - sb.append(" \"Version\": \"2012-10-17\",\n"); - sb.append(" \"Statement\": [\n"); - sb.append(" {\n"); - sb.append(" \"Sid\": \"PublicReadForObjects\",\n"); - sb.append(" \"Effect\": \"Allow\",\n"); - sb.append(" \"Principal\": \"*\",\n"); - sb.append(" \"Action\": \"s3:GetObject\",\n"); - sb.append(" \"Resource\": \"arn:aws:s3:::%s/*\"\n"); - sb.append(" }\n"); - sb.append(" ]\n"); - sb.append("}\n"); + String policyFormat = "{\n" + + " \"Version\": \"2012-10-17\",\n" + + " \"Statement\": [\n" + + " {\n" + + " \"Sid\": \"PublicReadForObjects\",\n" + + " \"Effect\": \"Allow\",\n" + + " \"Principal\": \"*\",\n" + + " \"Action\": \"s3:GetObject\",\n" + + " \"Resource\": \"arn:aws:s3:::%s/*\"\n" + + " }\n" + + " ]\n" + + "}\n"; - String jsonPolicy = String.format(sb.toString(), bucket.getName()); + String jsonPolicy = String.format(policyFormat, bucket.getName()); AmazonS3 s3client = getS3ClientByBucketAndStore(bucket, storeId); logger.debug("Setting the bucket policy to {} for {}", policy, bucket.getName()); try { s3client.setBucketPolicy(bucket.getName(), jsonPolicy); logger.info("Successfully set the bucket policy to {} for {}", policy, bucket.getName()); - return; } catch (AmazonClientException e) { - logger.error("Failed to set the bucket policy for " + bucket.getName(), e); + logger.error("Failed to set the bucket policy for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -631,7 +651,7 @@ public BucketPolicy getBucketPolicy(BucketTO bucket, long storeId) { logger.info("Successfully got the bucket policy for {}", bucket.getName()); return bp; } catch (AmazonClientException e) { - logger.error("Failed to get the bucket policy for " + bucket.getName(), e); + logger.error("Failed to get the bucket policy for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -643,9 +663,8 @@ public void deleteBucketPolicy(BucketTO bucket, long storeId) { try { s3client.deleteBucketPolicy(bucket.getName()); logger.info("Successfully deleted bucket policy for {}", bucket.getName()); - return; } catch (AmazonClientException e) { - logger.error("Failed to delete bucket policy for " + bucket.getName(), e); + logger.error("Failed to delete bucket policy for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -664,7 +683,7 @@ public boolean setBucketEncryption(BucketTO bucket, long storeId) { ServerSideEncryptionRule sseRule = new ServerSideEncryptionRule(); sseRule.setApplyServerSideEncryptionByDefault(sseByDefault); - List sseRules = new ArrayList(); + List sseRules = new ArrayList<>(); sseRules.add(sseRule); ServerSideEncryptionConfiguration sseConf = new ServerSideEncryptionConfiguration(); @@ -676,7 +695,7 @@ public boolean setBucketEncryption(BucketTO bucket, long storeId) { logger.info("Successfully enabled bucket encryption configuration for {}", bucket.getName()); return true; } catch (AmazonClientException e) { - logger.error("Failed to enable bucket encryption configuration for " + bucket.getName(), e); + logger.error("Failed to enable bucket encryption configuration for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -690,7 +709,7 @@ public boolean deleteBucketEncryption(BucketTO bucket, long storeId) { logger.info("Successfully deleted bucket encryption configuration for {}", bucket.getName()); return true; } catch (AmazonClientException e) { - logger.error("Failed to delete bucket encryption configuration for " + bucket.getName(), e); + logger.error("Failed to delete bucket encryption configuration for {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -706,7 +725,7 @@ public boolean setBucketVersioning(BucketTO bucket, long storeId) { logger.info("Successfully enabled versioning for bucket {}", bucket.getName()); return true; } catch (AmazonClientException e) { - logger.error("Failed to enable versioning for bucket " + bucket.getName(), e); + logger.error("Failed to enable versioning for bucket {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } @@ -722,14 +741,14 @@ public boolean deleteBucketVersioning(BucketTO bucket, long storeId) { logger.info("Successfully suspended versioning for bucket {}", bucket.getName()); return true; } catch (AmazonClientException e) { - logger.error("Failed to suspend versioning for bucket " + bucket.getName(), e); + logger.error("Failed to suspend versioning for bucket {}", bucket.getName(), e); throw new CloudRuntimeException(e); } } /** * Set the bucket quota to a size limit specified in GiB. - * + *

* Cloudian HyperStore does not currently support bucket quota limits. * CloudStack itself requires a quota to be set. HyperStore may add * Bucket Quota support in a future version. Currently, we only support @@ -753,8 +772,8 @@ public void setBucketQuota(BucketTO bucket, long storeId, long size) { /** * Return a map of bucket names managed by this store and their sizes (in bytes). - * - * Note: Bucket Usage Statistics in HyperStore are disabled by default. They + *

+ * Note: Bucket Usage Statistics in HyperStore are disabled by default. They * can be enabled by the HyperStore Administrator by setting of the configuration * 's3.qos.bucketLevel=true'. If this is not enabled, the values returned will * either be 0 or out of date. @@ -763,16 +782,16 @@ public void setBucketQuota(BucketTO bucket, long storeId, long size) { */ @Override public Map getAllBucketsUsage(long storeId) { - Map bucketUsage = new HashMap(); + Map bucketUsage = new HashMap<>(); List bucketList = _bucketDao.listByObjectStoreId(storeId); if (bucketList.isEmpty()) { return bucketUsage; } - // Create an unique list of domains from the bucket list + // Create a unique list of domains from the bucket list // and add all the bucket names to the bucketUsage map with value -1 as a marker // to know which buckets CloudStack cares about. The -1 will be replaced later. - List domainIds = new ArrayList(); + List domainIds = new ArrayList<>(); for (BucketVO bucket : bucketList) { long bucketDomainId = bucket.getDomainId(); if (! domainIds.contains(bucketDomainId)) { @@ -793,8 +812,8 @@ public Map getAllBucketsUsage(long storeId) { // Update the -1 entry to actual byteCount. bucketUsage.replace(cbu.getBucketName(), cbu.getByteCount()); } else { - // Replace with 0 instead of actual value. Race condition can cause this and it - // should be fixed automatically by a repair job. + // Replace with 0 instead of a negative value. A race condition can cause + // on the server. Usage is corrected periodically so it should self-medicate. bucketUsage.replace(cbu.getBucketName(), 0L); logger.info("Ignoring negative bucket usage for \"{}\": {}", cbu.getBucketName(), cbu.getByteCount()); } @@ -803,7 +822,7 @@ public Map getAllBucketsUsage(long storeId) { } // Remove any remaining -1 entries. These would probably be buckets that were - // deleted outside of CloudStack control. A missing entry might be better than + // deleted outside CloudStack control. A missing entry might be better than // returning the bucket name with -1 or 0. bucketUsage.entrySet().removeIf(entry -> entry.getValue() == -1); @@ -832,8 +851,8 @@ protected CloudianClient getCloudianClientByStoreId(long storeId) { * Returns an S3 connection for the store and account identified by the bucket. * NOTE: https connections must use a trusted certificate. * - * @param store the object store of the S3 service to connect to * @param bucket bucket information identifying the account which identifies the credentials to use. + * @param storeId the object store of the S3 service to connect to * @return an S3 connection (never null) * @throws CloudRuntimeException on failure. */ diff --git a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImpl.java b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImpl.java index d2fda09c65d1..81abd2e1e7f4 100644 --- a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImpl.java +++ b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImpl.java @@ -66,7 +66,7 @@ public DataStore initialize(Map dsInfos) { throw new CloudRuntimeException(msg); } - Map objectStoreParameters = new HashMap(); + Map objectStoreParameters = new HashMap<>(); objectStoreParameters.put(CloudianHyperStoreUtil.STORE_KEY_NAME, name); objectStoreParameters.put(CloudianHyperStoreUtil.STORE_KEY_URL, url); objectStoreParameters.put(CloudianHyperStoreUtil.STORE_KEY_PROVIDER_NAME, providerName); diff --git a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/provider/CloudianHyperStoreObjectStoreProviderImpl.java b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/provider/CloudianHyperStoreObjectStoreProviderImpl.java index eb924b3b709b..bd28b7da986e 100644 --- a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/provider/CloudianHyperStoreObjectStoreProviderImpl.java +++ b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/provider/CloudianHyperStoreObjectStoreProviderImpl.java @@ -28,7 +28,6 @@ import org.apache.cloudstack.storage.datastore.lifecycle.CloudianHyperStoreObjectStoreLifeCycleImpl; import org.apache.cloudstack.storage.datastore.util.CloudianHyperStoreUtil; import org.apache.cloudstack.storage.object.ObjectStoreDriver; -import org.apache.cloudstack.storage.object.datastore.ObjectStoreHelper; import org.apache.cloudstack.storage.object.datastore.ObjectStoreProviderManager; import org.apache.cloudstack.storage.object.store.lifecycle.ObjectStoreLifeCycle; import org.springframework.stereotype.Component; @@ -43,10 +42,7 @@ public class CloudianHyperStoreObjectStoreProviderImpl implements ObjectStorePro @Inject ObjectStoreProviderManager storeMgr; - @Inject - ObjectStoreHelper helper; - private final String providerName = CloudianHyperStoreUtil.OBJECT_STORE_PROVIDER_NAME; protected ObjectStoreLifeCycle lifeCycle; protected ObjectStoreDriver driver; @@ -57,7 +53,7 @@ public DataStoreLifeCycle getDataStoreLifeCycle() { @Override public String getName() { - return this.providerName; + return CloudianHyperStoreUtil.OBJECT_STORE_PROVIDER_NAME; } @Override @@ -80,7 +76,7 @@ public HypervisorHostListener getHostListener() { @Override public Set getTypes() { - Set types = new HashSet(); + Set types = new HashSet<>(); types.add(DataStoreProviderType.OBJECT); return types; } diff --git a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtil.java b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtil.java index efa10229428c..1d110b7b1322 100644 --- a/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtil.java +++ b/plugins/storage/object/cloudian/src/main/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtil.java @@ -90,7 +90,7 @@ public class CloudianHyperStoreUtil { /** * This method is solely for test purposes so that we can mock the timeout. * - * @returns the timeout in seconds + * @return the timeout in seconds */ protected static int getAdminTimeoutSeconds() { return DEFAULT_ADMIN_TIMEOUT_SECONDS; @@ -101,7 +101,7 @@ protected static int getAdminTimeoutSeconds() { * @param url the url of the ADMIN API service * @param user the admin username to connect as * @param pass the matching admin password - * @param validateSSL validate the SSL Certificate (when using https://) + * @param validateSSL validate the SSL Certificate (when using https) * @return a connection object (never null) * @throws CloudRuntimeException if the connection fails for any reason */ @@ -166,7 +166,7 @@ public static AmazonIdentityManagement getIAMClient(String url, String accessKey /** * Test the S3Url to confirm it behaves like an S3 Service. - * + *

* The method uses bad credentials and looks for the particular error from S3 * that says InvalidAccessKeyId was used. The method quietly returns if * we connect and get the expected error back. @@ -176,6 +176,9 @@ public static AmazonIdentityManagement getIAMClient(String url, String accessKey * @throws RuntimeException if there is any issue. */ public static void validateS3Url(String s3Url) { + if (StringUtils.isBlank(s3Url)) { + throw new CloudRuntimeException("The S3 URL must not be blank."); + } try { AmazonS3 s3Client = CloudianHyperStoreUtil.getS3Client(s3Url, "unknown", "unknown"); s3Client.listBuckets(); @@ -189,7 +192,7 @@ public static void validateS3Url(String s3Url) { /** * Test the IAMUrl to confirm it behaves like an IAM Service. - * + *

* The method uses bad credentials and looks for the particular error from IAM * that says InvalidAccessKeyId or InvalidClientTokenId was used. The method quietly * returns if we connect and get the expected error back. @@ -199,6 +202,9 @@ public static void validateS3Url(String s3Url) { * @throws RuntimeException if there is any issue. */ public static void validateIAMUrl(String iamUrl) { + if (StringUtils.isBlank(iamUrl)) { + throw new CloudRuntimeException("The IAM URL must not be blank."); + } try { AmazonIdentityManagement iamClient = CloudianHyperStoreUtil.getIAMClient(iamUrl, "unknown", "unknown"); iamClient.listAccessKeys(); diff --git a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImplTest.java b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImplTest.java index 1cf99ca53465..44b385c55dcc 100644 --- a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImplTest.java +++ b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/driver/CloudianHyperStoreObjectStoreDriverImplTest.java @@ -159,7 +159,7 @@ public void setUp() { when(objectStoreVO.getUrl()).thenReturn(TEST_ADMIN_URL); // The StoreDetailMap has Endpoint info and Admin Credentials - StoreDetailsMap = new HashMap(); + StoreDetailsMap = new HashMap<>(); StoreDetailsMap.put(CloudianHyperStoreUtil.STORE_DETAILS_KEY_USER_NAME, TEST_ADMIN_USER_NAME); StoreDetailsMap.put(CloudianHyperStoreUtil.STORE_DETAILS_KEY_PASSWORD, TEST_ADMIN_PASSWORD); StoreDetailsMap.put(CloudianHyperStoreUtil.STORE_DETAILS_KEY_VALIDATE_SSL, TEST_ADMIN_VALIDATE_SSL); @@ -168,7 +168,7 @@ public void setUp() { when(objectStoreDetailsDao.getDetails(TEST_STORE_ID)).thenReturn(StoreDetailsMap); // The AccountDetailsMap has credentials for operating on the account. - AccountDetailsMap = new HashMap(); + AccountDetailsMap = new HashMap<>(); AccountDetailsMap.put(CloudianHyperStoreUtil.KEY_ROOT_ACCESS_KEY, TEST_ROOT_AK); AccountDetailsMap.put(CloudianHyperStoreUtil.KEY_ROOT_SECRET_KEY, TEST_ROOT_SK); AccountDetailsMap.put(CloudianHyperStoreUtil.KEY_IAM_ACCESS_KEY, TEST_IAM_AK); @@ -190,7 +190,7 @@ public void testGetStoreTO() { } @Test - public void testCreateBucket() throws Exception { + public void testCreateBucket() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3Client(anyString(), anyString(), anyString()); when(bucketDao.findById(anyLong())).thenReturn(bucketVo); @@ -213,9 +213,9 @@ public void testCreateBucket() throws Exception { } @Test - public void testCreateHSCredential() throws Exception { + public void testCreateHSCredential() { cloudianClient = mock(CloudianClient.class); - List CredList = new ArrayList(); + List CredList = new ArrayList<>(); CloudianCredential c1 = new CloudianCredential(); c1.setActive(false); c1.setCreateDate(new Date(1L)); // oldest but inactive @@ -241,8 +241,8 @@ public void testCreateHSCredential() throws Exception { } @Test - public void testGetAllBucketsUsageNoBuckets() throws Exception { - when(bucketDao.listByObjectStoreId(TEST_STORE_ID)).thenReturn(new ArrayList()); + public void testGetAllBucketsUsageNoBuckets() { + when(bucketDao.listByObjectStoreId(TEST_STORE_ID)).thenReturn(new ArrayList<>()); Map emptyMap = cloudianHyperStoreObjectStoreDriverImpl.getAllBucketsUsage(TEST_STORE_ID); assertNotNull(emptyMap); assertEquals(0, emptyMap.size()); @@ -255,7 +255,7 @@ public void testGetAllBucketsUsageTwoDomains() { BucketVO b2 = new BucketVO(TEST_ACCOUNT_ID, 1L, TEST_STORE_ID, "b2", null, false, false, false, null); BucketVO b3 = new BucketVO(TEST_ACCOUNT_ID, 2L, TEST_STORE_ID, "b3", null, false, false, false, null); BucketVO b4 = new BucketVO(TEST_ACCOUNT_ID, 2L, TEST_STORE_ID, "b4", null, false, false, false, null); - List BucketList = new ArrayList(); + List BucketList = new ArrayList<>(); BucketList.add(b1); // b1 owned by domain 1, exists BucketList.add(b2); // b2 owned by domain 1, deleted in object store (so no usage info) BucketList.add(b3); // b3 owned by domain 2, exists @@ -287,9 +287,9 @@ public void testGetAllBucketsUsageTwoDomains() { CloudianBucketUsage bu5 = new CloudianBucketUsage(); bu5.setBucketName("b5"); bu5.setByteCount(5L); - List d1bucketList = new ArrayList(); + List d1bucketList = new ArrayList<>(); d1bucketList.add(bu1); - List d2bucketList = new ArrayList(); + List d2bucketList = new ArrayList<>(); d2bucketList.add(bu3); d2bucketList.add(bu4); d2bucketList.add(bu5); @@ -297,9 +297,9 @@ public void testGetAllBucketsUsageTwoDomains() { when(d1U1Usage.getBuckets()).thenReturn(d1bucketList); CloudianUserBucketUsage d2U1Usage = mock(CloudianUserBucketUsage.class); when(d2U1Usage.getBuckets()).thenReturn(d2bucketList); - List d1Usage = new ArrayList(); + List d1Usage = new ArrayList<>(); d1Usage.add(d1U1Usage); - List d2Usage = new ArrayList(); + List d2Usage = new ArrayList<>(); d2Usage.add(d2U1Usage); doReturn(cloudianClient).when(cloudianHyperStoreObjectStoreDriverImpl).getCloudianClientByStoreId(TEST_STORE_ID); @@ -322,10 +322,10 @@ public void testGetAllBucketsUsageTwoDomains() { } @Test - public void testCreateUserNotExists() throws Exception { + public void testCreateUserNotExists() { // ensure no account credentials are returned in the account details for new user. Mockito.reset(accountDetailsDao); - when(accountDetailsDao.findDetails(TEST_ACCOUNT_ID)).thenReturn(new HashMap()); + when(accountDetailsDao.findDetails(TEST_ACCOUNT_ID)).thenReturn(new HashMap<>()); String hsUserId = "user1"; String hsGroupId = "group1"; @@ -337,12 +337,12 @@ public void testCreateUserNotExists() throws Exception { doReturn(cloudianClient).when(cloudianHyperStoreObjectStoreDriverImpl).getCloudianClientByStoreId(TEST_STORE_ID); - // Setup the user and group as not found. + // Set up the user and group as not found. when(cloudianClient.listUser(hsUserId, hsGroupId)).thenReturn(null); when(cloudianClient.listGroup(hsGroupId)).thenReturn(null); when(cloudianClient.addUser(any(CloudianUser.class))).thenReturn(true); - // lets assume no credentials added, so we add new ones. - when(cloudianClient.listCredentials(hsUserId, hsGroupId)).thenReturn(new ArrayList()); + // let's assume no credentials added, so we add new ones. + when(cloudianClient.listCredentials(hsUserId, hsGroupId)).thenReturn(new ArrayList<>()); CloudianCredential credential = new CloudianCredential(); credential.setAccessKey(TEST_ROOT_AK); credential.setSecretKey(TEST_ROOT_SK); @@ -358,11 +358,11 @@ public void testCreateUserNotExists() throws Exception { when(iamClient.createAccessKey(any(CreateAccessKeyRequest.class))).thenReturn(accessKeyResult); // Next Check what will be persisted in DB after everything created. - // Even though its not going to be true for a new user, lets have 1 bucket + // Even though it's not going to be true for a new user, lets have 1 bucket // whose credentials need to be updated. BucketVO bucketToUpdate = mock(BucketVO.class); when(bucketToUpdate.getId()).thenReturn(9L); - List bucketUpdateList = new ArrayList(); + List bucketUpdateList = new ArrayList<>(); bucketUpdateList.add(bucketToUpdate); when(bucketDao.listByObjectStoreIdAndAccountId(TEST_STORE_ID, TEST_ACCOUNT_ID)).thenReturn(bucketUpdateList); @@ -413,7 +413,7 @@ public void testCreateUserExists() { doReturn(cloudianClient).when(cloudianHyperStoreObjectStoreDriverImpl).getCloudianClientByStoreId(TEST_STORE_ID); - // Setup the user/group as existing and active + // Set up the user/group as existing and active CloudianUser user = mock(CloudianUser.class); CloudianGroup group = mock(CloudianGroup.class); when(user.getActive()).thenReturn(true); @@ -421,20 +421,20 @@ public void testCreateUserExists() { when(cloudianClient.listUser(hsUserId, hsGroupId)).thenReturn(user); when(cloudianClient.listGroup(hsGroupId)).thenReturn(group); - // Setup the HS Credential to match known Root credential + // Set up the HS Credential to match known Root credential CloudianCredential credential = new CloudianCredential(); credential.setAccessKey(TEST_ROOT_AK); credential.setSecretKey(TEST_ROOT_SK); credential.setActive(true); credential.setCreateDate(new Date(1L)); - List credentials = new ArrayList(); + List credentials = new ArrayList<>(); credentials.add(credential); when(cloudianClient.listCredentials(hsUserId, hsGroupId)).thenReturn(credentials); // Setup IAM to return 2 credentials, one that matches and one that doesn't doReturn(iamClient).when(cloudianHyperStoreObjectStoreDriverImpl).getIAMClientByStoreId(TEST_STORE_ID, credential); ListAccessKeysResult listAccessKeyResult = mock(ListAccessKeysResult.class); - List listAccessKeyMetadata = new ArrayList(); + List listAccessKeyMetadata = new ArrayList<>(); AccessKeyMetadata accessKeyNoMatch = mock(AccessKeyMetadata.class); when(accessKeyNoMatch.getAccessKeyId()).thenReturn("no_match"); AccessKeyMetadata accessKeyMatch = mock(AccessKeyMetadata.class); @@ -479,7 +479,7 @@ public void testCreateUserDisabledUserExists() { doReturn(cloudianClient).when(cloudianHyperStoreObjectStoreDriverImpl).getCloudianClientByStoreId(TEST_STORE_ID); - // Setup the user to be found but inactive. + // Set up the user to be found but inactive. CloudianUser user = mock(CloudianUser.class); when(user.getActive()).thenReturn(false); when(cloudianClient.listUser(hsUserId, hsGroupId)).thenReturn(user); @@ -501,7 +501,7 @@ public void testCreateUserDisabledGroupExists() { doReturn(cloudianClient).when(cloudianHyperStoreObjectStoreDriverImpl).getCloudianClientByStoreId(TEST_STORE_ID); - // Setup the user to not be found so that we check for a group + // Set up the user to not be found so that we check for a group when(cloudianClient.listUser(hsUserId, hsGroupId)).thenReturn(null); CloudianGroup group = mock(CloudianGroup.class); when(group.getActive()).thenReturn(false); @@ -514,7 +514,7 @@ public void testCreateUserDisabledGroupExists() { @Test public void testListBuckets() { - Map bucketUsageMap = new HashMap(); + Map bucketUsageMap = new HashMap<>(); bucketUsageMap.put("b1", 1L); bucketUsageMap.put("b2", 2L); when(cloudianHyperStoreObjectStoreDriverImpl.getAllBucketsUsage(anyLong())).thenReturn(bucketUsageMap); @@ -525,7 +525,46 @@ public void testListBuckets() { } @Test - public void testDeleteBucket() throws Exception { + public void testVerifyServiceConnectivity() { + when(cloudianHyperStoreObjectStoreDriverImpl.getCloudianClientByStoreId(TEST_STORE_ID)).thenReturn(cloudianClient); + + try (MockedStatic mockStatic = Mockito.mockStatic(CloudianHyperStoreUtil.class)) { + // This allows both validateS3URL and validateIAMUrl to be called. + cloudianHyperStoreObjectStoreDriverImpl.verifyServiceConnectivity(TEST_STORE_ID); + + // getServerVersion() should be called once. + verify(cloudianClient, times(1)).getServerVersion(); + // validate called once on each Url type from the map. + mockStatic.verify(() -> CloudianHyperStoreUtil.validateS3Url(TEST_S3_URL), times(1)); + mockStatic.verify(() -> CloudianHyperStoreUtil.validateIAMUrl(TEST_IAM_URL), times(1)); + } + } + + @Test + public void testVerifyServiceConnectivityMissingS3Url() { + // delete the S3 URL from our map + StoreDetailsMap.remove(CloudianHyperStoreUtil.STORE_DETAILS_KEY_S3_URL); + when(cloudianHyperStoreObjectStoreDriverImpl.getCloudianClientByStoreId(TEST_STORE_ID)).thenReturn(cloudianClient); + + try (MockedStatic mockStatic = Mockito.mockStatic(CloudianHyperStoreUtil.class)) { + // mock so that we can verify what is called, but pass s3 through to real validation. + mockStatic.when(() -> CloudianHyperStoreUtil.validateS3Url(any())).thenCallRealMethod(); + + // Lack of S3 url should mean validate will fail with an exception + CloudRuntimeException thrown = assertThrows(CloudRuntimeException.class, () -> cloudianHyperStoreObjectStoreDriverImpl.verifyServiceConnectivity(TEST_STORE_ID)); + assertNotNull(thrown); + assertTrue(thrown.getMessage().contains("must not be blank")); + + // getServerVersion() should be called once. + verify(cloudianClient, times(1)).getServerVersion(); + // S3 validate will be called once with null, but IAM validate will not. + mockStatic.verify(() -> CloudianHyperStoreUtil.validateS3Url(null), times(1)); + mockStatic.verify(() -> CloudianHyperStoreUtil.validateIAMUrl(TEST_IAM_URL), never()); + } + } + + @Test + public void testDeleteBucket() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -535,7 +574,7 @@ public void testDeleteBucket() throws Exception { } @Test - public void testSetBucketPolicyPrivate() throws Exception { + public void testSetBucketPolicyPrivate() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -546,7 +585,7 @@ public void testSetBucketPolicyPrivate() throws Exception { } @Test - public void testSetBucketPolicyPublic() throws Exception { + public void testSetBucketPolicyPublic() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -587,7 +626,7 @@ public void testSetBucketEncryption() { } @Test - public void testDeleteBucketEncryption() throws Exception { + public void testDeleteBucketEncryption() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -597,7 +636,7 @@ public void testDeleteBucketEncryption() throws Exception { } @Test - public void testSetBucketVersioning() throws Exception { + public void testSetBucketVersioning() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -612,7 +651,7 @@ public void testSetBucketVersioning() throws Exception { } @Test - public void testDeleteBucketVersioning() throws Exception { + public void testDeleteBucketVersioning() { doReturn(s3Client).when(cloudianHyperStoreObjectStoreDriverImpl).getS3ClientByBucketAndStore(any(), anyLong()); BucketTO bucket = mock(BucketTO.class); when(bucket.getName()).thenReturn(TEST_BUCKET_NAME); @@ -643,7 +682,7 @@ public void testGetS3ClientByBucketAndStore() { BucketVO b2 = new BucketVO(TEST_ACCOUNT_ID, 1L, TEST_STORE_ID, "b2", null, false, false, false, null); BucketVO b3 = new BucketVO(TEST_ACCOUNT_ID, 2L, TEST_STORE_ID, TEST_BUCKET_NAME, null, false, false, false, null); BucketVO b4 = new BucketVO(TEST_ACCOUNT_ID, 2L, TEST_STORE_ID, "b4", null, false, false, false, null); - List BucketList = new ArrayList(); + List BucketList = new ArrayList<>(); BucketList.add(b1); // b1 owned by domain 1, exists BucketList.add(b2); // b2 owned by domain 1, exists BucketList.add(b3); // b3 owned by domain 2, exists - our TEST BUCKET @@ -672,7 +711,7 @@ public void testGetS3ClientByBucketAndStore() { public void testGetS3ClientByBucketAndStoreNoMatch() { // Prepare Buckets the store knows about. BucketVO b1 = new BucketVO(TEST_ACCOUNT_ID, 1L, TEST_STORE_ID, "b1", null, false, false, false, null); - List BucketList = new ArrayList(); + List BucketList = new ArrayList<>(); BucketList.add(b1); // b1 owned by domain 1, exists when(bucketDao.listByObjectStoreId(TEST_STORE_ID)).thenReturn(BucketList); diff --git a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImplTest.java b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImplTest.java index fe6dc16f3fc5..20e77ca93ce6 100644 --- a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImplTest.java +++ b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudianHyperStoreObjectStoreLifeCycleImplTest.java @@ -106,13 +106,13 @@ public void setUp() { cloudianHyperStoreObjectStoreLifeCycleImpl.objectStoreHelper = objectStoreHelper; cloudianHyperStoreObjectStoreLifeCycleImpl.objectStoreMgr = objectStoreMgr; - guiDetailMap = new HashMap(); + guiDetailMap = new HashMap<>(); guiDetailMap.put("accesskey", TEST_ADMIN_USERNAME); guiDetailMap.put("secretkey", TEST_ADMIN_PASSWORD); guiDetailMap.put("validateSSL", TEST_VALIDATE_SSL); guiDetailMap.put("s3Url", TEST_S3_URL); guiDetailMap.put("iamUrl", TEST_IAM_URL); - guiDataStoreMap = new HashMap(); + guiDataStoreMap = new HashMap<>(); guiDataStoreMap.put("name", TEST_STORE_NAME); guiDataStoreMap.put("url", TEST_ADMIN_URL); guiDataStoreMap.put("providerName", TEST_PROVIDER_NAME); @@ -130,7 +130,7 @@ public void testInitializeValidation() { mockStatic.when(() -> CloudianHyperStoreUtil.getCloudianClient(anyString(), anyString(), anyString(), anyBoolean())).thenReturn(cloudianClient); mockStatic.when(() -> CloudianHyperStoreUtil.getS3Client(anyString(), anyString(), anyString())).thenReturn(s3Client); mockStatic.when(() -> CloudianHyperStoreUtil.getIAMClient(anyString(), anyString(), anyString())).thenReturn(iamClient); - // Ensure real validation methods are called (as everything was mocked). These ones we need. + // Ensure real validation methods are called (as everything was mocked). These we need. mockStatic.when(() -> CloudianHyperStoreUtil.validateS3Url(anyString())).thenCallRealMethod(); mockStatic.when(() -> CloudianHyperStoreUtil.validateIAMUrl(anyString())).thenCallRealMethod(); diff --git a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtilTest.java b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtilTest.java index c83d5d7fd2f5..9f2312281f46 100644 --- a/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtilTest.java +++ b/plugins/storage/object/cloudian/src/test/java/org/apache/cloudstack/storage/datastore/util/CloudianHyperStoreUtilTest.java @@ -112,34 +112,40 @@ public void testGetCloudianClientShortenedTimeout() { mockStatic.when(() -> CloudianHyperStoreUtil.getAdminTimeoutSeconds()).thenReturn(1); mockStatic.when(() -> CloudianHyperStoreUtil.getCloudianClient(anyString(), anyString(), anyString(), anyBoolean())).thenCallRealMethod(); - // Get a connection and try using it but it should timeout + // Get a connection and try using it but it should time out String url = String.format("http://localhost:%d", port); CloudianClient cc = CloudianHyperStoreUtil.getCloudianClient(url, "u", "p", false); long before = System.currentTimeMillis(); - ServerApiException thrown = assertThrows(ServerApiException.class, () -> cc.getServerVersion()); + ServerApiException thrown = assertThrows(ServerApiException.class, cc::getServerVersion); long after = System.currentTimeMillis(); assertNotNull(thrown); assertEquals(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, thrown.getErrorCode()); - assertTrue((after - before) >= 1000); // should timeout after 1 second. + assertTrue((after - before) >= 1000); // should time out after 1 second. } } + @Test + public void testValidateS3UrlNull() { + CloudRuntimeException thrown = assertThrows(CloudRuntimeException.class, () -> CloudianHyperStoreUtil.validateS3Url(null)); + assertNotNull(thrown); + assertTrue(thrown.getMessage().contains("must not be blank")); + } + @Test public void testValidateS3UrlGood() { // Mock an AWS S3 invalid access key response. - StringBuilder ERR_XML = new StringBuilder("\n"); - ERR_XML.append("\n"); - ERR_XML.append(" InvalidAccessKeyId\n"); - ERR_XML.append(" The AWS Access Key Id you provided does not exist in our records.\n"); - ERR_XML.append(" unknown\n"); - ERR_XML.append(" 12345=\n"); - ERR_XML.append("\n"); + String ERR_XML = "\n" + "\n" + + " InvalidAccessKeyId\n" + + " The AWS Access Key Id you provided does not exist in our records.\n" + + " unknown\n" + + " 12345=\n" + + "\n"; wireMockRule.stubFor(get(urlEqualTo("/")) .willReturn(aResponse() .withStatus(403) .withHeader("content-type", "application/xml") - .withBody(ERR_XML.toString()))); + .withBody(ERR_XML))); // Test: validates the AmazonS3 client returned by CloudianHyperStoreUtil.getS3Client() // which is called indirectly via the validateS3Url() method can connect to the @@ -161,24 +167,30 @@ public void testValidateS3UrlBadRequest() { assertNotNull(thrown); } + @Test + public void testValidateIAMUrlNull() { + CloudRuntimeException thrown = assertThrows(CloudRuntimeException.class, () -> CloudianHyperStoreUtil.validateIAMUrl(null)); + assertNotNull(thrown); + assertTrue(thrown.getMessage().contains("must not be blank")); + } + @Test public void testValidateIAMUrlGoodInvalidClientTokenId() { // Mock an AWS IAM invalid access key response. - StringBuilder ERR_XML = new StringBuilder(); - ERR_XML.append("\n"); - ERR_XML.append(" \n"); - ERR_XML.append(" Sender\n"); - ERR_XML.append(" InvalidClientTokenId\n"); - ERR_XML.append(" The security token included in the request is invalid.\n"); - ERR_XML.append(" \n"); - ERR_XML.append(" a2c47f7e-0196-4b45-af18-a9e99e4d9ed5\n"); - ERR_XML.append("\n"); + String ERR_XML = "\n" + + " \n" + + " Sender\n" + + " InvalidClientTokenId\n" + + " The security token included in the request is invalid.\n" + + " \n" + + " a2c47f7e-0196-4b45-af18-a9e99e4d9ed5\n" + + "\n"; wireMockRule.stubFor(post(urlEqualTo("/")) .willReturn(aResponse() .withStatus(403) .withHeader("content-type", "text/xml") - .withBody(ERR_XML.toString()))); + .withBody(ERR_XML))); // Test: validates the AmazonIdentityManagement client returned by CloudianHyperStoreUtil.getIAMClient() // which is called indirectly via the validateIAMUrl() method can connect to the @@ -190,20 +202,20 @@ public void testValidateIAMUrlGoodInvalidClientTokenId() { @Test public void testValidateIAMUrlGoodInvalidAccessKeyId() { // Mock HyperStore IAM invalid access key current response. - StringBuilder ERR_XML = new StringBuilder("\n"); - ERR_XML.append("\n"); - ERR_XML.append(" \n"); - ERR_XML.append(" InvalidAccessKeyId\n"); - ERR_XML.append(" The Access Key Id you provided does not exist in our records.\n"); - ERR_XML.append(" \n"); - ERR_XML.append(" a2c47f7e-0196-4b45-af18-a9e99e4d9ed5\n"); - ERR_XML.append("\n"); + String ERR_XML = "\n" + + "\n" + + " \n" + + " InvalidAccessKeyId\n" + + " The Access Key Id you provided does not exist in our records.\n" + + " \n" + + " a2c47f7e-0196-4b45-af18-a9e99e4d9ed5\n" + + "\n"; wireMockRule.stubFor(post(urlEqualTo("/")) .willReturn(aResponse() .withStatus(403) .withHeader("content-type", "application/xml;charset=UTF-8") - .withBody(ERR_XML.toString()))); + .withBody(ERR_XML))); // Test: validates the AmazonIdentityManagement client returned by CloudianHyperStoreUtil.getIAMClient() // which is called indirectly via the validateIAMUrl() method can connect to the diff --git a/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java b/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java index 9dc4b30414e6..0a71f79529b1 100644 --- a/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java +++ b/plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java @@ -180,6 +180,12 @@ public List listBuckets(long storeId) { return bucketsList; } + @Override + public void verifyServiceConnectivity(long storeId) { + // ideally ping the service. For now, listBuckets() will do. + listBuckets(storeId); + } + @Override public boolean deleteBucket(BucketTO bucket, long storeId) { String bucketName = bucket.getName(); diff --git a/plugins/storage/object/simulator/src/main/java/org/apache/cloudstack/storage/datastore/driver/SimulatorObjectStoreDriverImpl.java b/plugins/storage/object/simulator/src/main/java/org/apache/cloudstack/storage/datastore/driver/SimulatorObjectStoreDriverImpl.java index 7b9ac59d5b1e..c9f027025098 100644 --- a/plugins/storage/object/simulator/src/main/java/org/apache/cloudstack/storage/datastore/driver/SimulatorObjectStoreDriverImpl.java +++ b/plugins/storage/object/simulator/src/main/java/org/apache/cloudstack/storage/datastore/driver/SimulatorObjectStoreDriverImpl.java @@ -71,6 +71,10 @@ public List listBuckets(long storeId) { return bucketsList; } + @Override + public void verifyServiceConnectivity(long storeId) { + } + @Override public boolean deleteBucket(BucketTO bucket, long storeId) { return true; diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index d601213329f0..850c09fca569 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -5585,6 +5585,12 @@ public List listResourceDetails(ListResourceDetailsCmd c String value = cmd.getValue(); Long resourceId = null; + // Reject requests for Object Store Details to non-RootAdmins + Account account = CallContext.current().getCallingAccount(); + if (! accountMgr.isRootAdmin(account.getAccountId()) && resourceType == ResourceTag.ResourceObjectType.ObjectStore) { + throw new PermissionDeniedException("The " + account + " is not authorized to list object store details."); + } + //Validation - 1.1 - resourceId and value can't be null. if (resourceIdStr == null && value == null) { throw new InvalidParameterValueException("Insufficient parameters passed for listing by resourceId OR key,value pair. Please check your params and try again."); diff --git a/server/src/main/java/com/cloud/metadata/ResourceMetaDataManagerImpl.java b/server/src/main/java/com/cloud/metadata/ResourceMetaDataManagerImpl.java index fc2453286cbb..e728d0defbec 100644 --- a/server/src/main/java/com/cloud/metadata/ResourceMetaDataManagerImpl.java +++ b/server/src/main/java/com/cloud/metadata/ResourceMetaDataManagerImpl.java @@ -45,6 +45,7 @@ import org.apache.cloudstack.resourcedetail.dao.LBStickinessPolicyDetailsDao; import org.apache.cloudstack.resourcedetail.dao.LBHealthCheckPolicyDetailsDao; import org.apache.cloudstack.resourcedetail.dao.GuestOsDetailsDao; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.springframework.stereotype.Component; @@ -127,6 +128,8 @@ public class ResourceMetaDataManagerImpl extends ManagerBase implements Resource @Inject NetworkOfferingDetailsDao _networkOfferingDetailsDao; @Inject + ObjectStoreDetailsDao _objectStoreDetailsDao; + @Inject ResourceManagerUtil resourceManagerUtil; private static Map> s_daoMap = new HashMap>(); @@ -162,6 +165,7 @@ public boolean configure(String name, Map params) throws Configu s_daoMap.put(ResourceObjectType.SnapshotPolicy, _snapshotPolicyDetailsDao); s_daoMap.put(ResourceObjectType.GuestOs, _guestOsDetailsDao); s_daoMap.put(ResourceObjectType.NetworkOffering, _networkOfferingDetailsDao); + s_daoMap.put(ResourceObjectType.ObjectStore, _objectStoreDetailsDao); return true; } diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 5de7ade696ac..11d3d4274f9a 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -4717,34 +4717,72 @@ public ObjectStore updateObjectStore(Long id, UpdateObjectStoragePoolCmd cmd) { if (objectStoreVO == null) { throw new IllegalArgumentException("Unable to find object store with ID: " + id); } + if (! StringUtils.equals(objectStoreVO.getProviderName(), cmd.getProviderName())) { + String msg = String.format("Unexpected providerName %s does not match store's providerName %s", cmd.getProviderName(), objectStoreVO.getProviderName()); + logger.error(msg); + throw new IllegalArgumentException(msg); + } + + boolean updated = false; + boolean detailsUpdated = false; + + String newName = cmd.getName(); + String oldName = objectStoreVO.getName(); + if (newName != null && !newName.equals(oldName)) { + logger.debug("name {} => {}", oldName, newName); + objectStoreVO.setName(newName); + updated = true; + } - if(cmd.getUrl() != null ) { - String url = cmd.getUrl(); + String newUrl = cmd.getUrl(); + String oldUrl = objectStoreVO.getUrl(); + if (newUrl != null && !newUrl.equals(oldUrl)) { try { - // Check URL - UriUtils.validateUrl(url); - } catch (final Exception e) { - throw new InvalidParameterValueException(url + " is not a valid URL"); - } - ObjectStoreEntity objectStore = (ObjectStoreEntity)_dataStoreMgr.getDataStore(objectStoreVO.getId(), DataStoreRole.Object); - String oldUrl = objectStoreVO.getUrl(); - objectStoreVO.setUrl(url); + UriUtils.validateUrl(newUrl); + logger.debug("url {} => {}", oldUrl, newUrl); + objectStoreVO.setUrl(newUrl); + updated = true; + } catch (final IllegalArgumentException e) { + throw new InvalidParameterValueException(newUrl + " is not a valid URL"); + } + } + + if (updated) { + // any update to the store, save now _objectStoreDao.update(id, objectStoreVO); - //Update URL and check access - try { - objectStore.listBuckets(); - } catch (Exception e) { - //Revert to old URL on failure + } + + // If any details are given and if there is any difference, then + // we update with all the new details. + Map oldDetails = _objectStoreDetailsDao.getDetails(id); + Map newDetails = cmd.getDetails(); + if (newDetails != null && !oldDetails.equals(newDetails)) { + logger.debug("details {} => {}", oldDetails, newDetails); + _objectStoreDetailsDao.update(id, newDetails); + detailsUpdated = true; + } + + // Any updates have been saved. Try validate the connection. + // We test the connection even if there were no updates as that's also useful. + try { + ObjectStoreEntity objectStore = (ObjectStoreEntity)_dataStoreMgr.getDataStore(id, DataStoreRole.Object); + objectStore.verifyServiceConnectivity(); + } catch (Exception e) { + String msg = String.format("Unable to access Object Storage with URL: %s", cmd.getUrl()); + logger.warn(msg, e); + + // revert any updates made. + if (updated) { + objectStoreVO.setName(oldName); objectStoreVO.setUrl(oldUrl); _objectStoreDao.update(id, objectStoreVO); - throw new IllegalArgumentException("Unable to access Object Storage with URL: " + cmd.getUrl()); } + if (detailsUpdated) { + _objectStoreDetailsDao.update(id, oldDetails); + } + throw new IllegalArgumentException(msg); } - if(cmd.getName() != null ) { - objectStoreVO.setName(cmd.getName()); - } - _objectStoreDao.update(id, objectStoreVO); logger.debug("Successfully updated object store: {}", objectStoreVO); return objectStoreVO; } diff --git a/server/src/main/java/com/cloud/tags/ResourceManagerUtilImpl.java b/server/src/main/java/com/cloud/tags/ResourceManagerUtilImpl.java index e6d0b737bbe9..12e98c176c08 100644 --- a/server/src/main/java/com/cloud/tags/ResourceManagerUtilImpl.java +++ b/server/src/main/java/com/cloud/tags/ResourceManagerUtilImpl.java @@ -25,6 +25,7 @@ import org.apache.cloudstack.api.Identity; import org.apache.cloudstack.api.InternalIdentity; import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.commons.lang3.StringUtils; @@ -118,6 +119,7 @@ public class ResourceManagerUtilImpl implements ResourceManagerUtil { s_typeMap.put(ResourceTag.ResourceObjectType.NetworkOffering, NetworkOfferingVO.class); s_typeMap.put(ResourceTag.ResourceObjectType.VpcOffering, VpcOfferingVO.class); s_typeMap.put(ResourceTag.ResourceObjectType.Domain, DomainVO.class); + s_typeMap.put(ResourceTag.ResourceObjectType.ObjectStore, ObjectStoreVO.class); s_typeMap.put(ResourceTag.ResourceObjectType.GuestOsCategory, GuestOsCategory.class); } diff --git a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java index fdc277404f60..097656f92c5c 100644 --- a/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java +++ b/server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java @@ -39,7 +39,10 @@ import com.cloud.network.Network; import com.cloud.network.VNF; import com.cloud.network.dao.NetworkVO; +import com.cloud.server.ResourceManagerUtil; +import com.cloud.server.ResourceMetaDataService; import com.cloud.server.ResourceTag; +import com.cloud.server.ResourceTag.ResourceObjectType; import com.cloud.storage.BucketVO; import com.cloud.storage.VMTemplateVO; import com.cloud.storage.ScopeType; @@ -70,6 +73,7 @@ import org.apache.cloudstack.api.command.user.bucket.ListBucketsCmd; import org.apache.cloudstack.api.command.user.event.ListEventsCmd; import org.apache.cloudstack.api.command.user.resource.ListDetailOptionsCmd; +import org.apache.cloudstack.api.command.user.volume.ListResourceDetailsCmd; import org.apache.cloudstack.api.response.DetailOptionsResponse; import org.apache.cloudstack.api.response.EventResponse; import org.apache.cloudstack.api.response.ListResponse; @@ -105,6 +109,7 @@ import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) @@ -137,6 +142,12 @@ public class QueryManagerImplTest { @Mock SearchCriteria searchCriteriaMock; + @Mock + ResourceManagerUtil resourceManagerUtil; + + @Mock + ResourceMetaDataService _resourceMetaDataMgr; + @Mock ObjectStoreDao objectStoreDao; @@ -407,6 +418,32 @@ public void testSearchForBuckets() { queryManagerImplSpy.searchForBuckets(listBucketsCmd); } + @Test + public void testListResourceDetailsObjectStoreNonRootAdminBadParam() { + ListResourceDetailsCmd cmd = mock(ListResourceDetailsCmd.class); + when(cmd.getResourceType()).thenReturn(ResourceObjectType.ObjectStore); + + // Ensure we get a PermissionDenied exception if we are not RootAdmin. + // The setup currently sets isRootAdmin to false. Quick confirmation. + Assert.assertFalse(accountManager.isRootAdmin(ACCOUNT_ID)); + PermissionDeniedException thrown = Assert.assertThrows(PermissionDeniedException.class, () -> queryManagerImplSpy.listResourceDetails(cmd)); + Assert.assertNotNull(thrown); + Assert.assertTrue(thrown.getMessage().contains("is not authorized to list object store details")); + } + + @Test + public void testListResourceDetailsObjectStoreAsRootAdminBadParam() { + ListResourceDetailsCmd cmd = mock(ListResourceDetailsCmd.class); + when(cmd.getResourceType()).thenReturn(ResourceObjectType.ObjectStore); + when(accountManager.isRootAdmin(account.getId())).thenReturn(true); + + // This time, we are RootAdmin so we get passed the permission check but hit a bad parameter. + Assert.assertTrue(accountManager.isRootAdmin(ACCOUNT_ID)); + InvalidParameterValueException thrown = Assert.assertThrows(InvalidParameterValueException.class, () -> queryManagerImplSpy.listResourceDetails(cmd)); + Assert.assertNotNull(thrown); + Assert.assertTrue(thrown.getMessage().contains("Insufficient parameters")); + } + @Test public void testGetHostTagsFromTemplateForServiceOfferingsListingNoTemplateId() { Assert.assertTrue(CollectionUtils.isEmpty(queryManager.getHostTagsFromTemplateForServiceOfferingsListing(Mockito.mock(AccountVO.class), null))); diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 01ab82c913dd..22774e67bcd7 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -31,6 +31,8 @@ import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.command.admin.storage.ChangeStoragePoolScopeCmd; +import org.apache.cloudstack.api.command.admin.storage.UpdateObjectStoragePoolCmd; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.api.command.admin.storage.ConfigureStorageAccessCmd; import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver; import org.apache.cloudstack.framework.config.ConfigDepot; @@ -38,10 +40,14 @@ import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.resourcedetail.dao.DiskOfferingDetailsDao; import org.apache.cloudstack.storage.command.CheckDataStoreStoragePolicyComplainceCommand; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; +import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.object.ObjectStoreEntity; import org.apache.commons.collections.MapUtils; import org.junit.Assert; import org.junit.Test; @@ -118,6 +124,12 @@ public class StorageManagerImplTest { AccountManagerImpl accountMgr; @Mock StoragePoolDetailsDao storagePoolDetailsDao; + @Mock + DataStoreManager _dataStoreMgr; + @Mock + ObjectStoreDao _objectStoreDao; + @Mock + ObjectStoreDetailsDao _objectStoreDetailsDao; @Mock ClusterDao clusterDao; @@ -980,6 +992,261 @@ public void testGetStoragePoolIopsStats_UsedIopsNegative() { Assert.assertNull("Used IOPS should be null when usedIops <= 0", result.second()); } + @Test + public void testUpdateObjectStoreNotFound() { + long storeId = 1L; + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(null); + + IllegalArgumentException thrown = Assert.assertThrows(IllegalArgumentException.class, () -> storageManagerImpl.updateObjectStore(storeId, updateCmd)); + Assert.assertNotNull(thrown); + Assert.assertTrue(thrown.getMessage().contains("Unable to find object store")); + } + + @Test + public void testUpdateObjectStoreNotMe() { + long storeId = 1L; + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + Mockito.when(objectStoreVO.getProviderName()).thenReturn("provider1"); + Mockito.when(updateCmd.getProviderName()).thenReturn("other provider"); + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + IllegalArgumentException thrown = Assert.assertThrows(IllegalArgumentException.class, () -> storageManagerImpl.updateObjectStore(storeId, updateCmd)); + Assert.assertNotNull(thrown); + Assert.assertTrue(thrown.getMessage().contains("Unexpected providerName")); + } + + @Test + public void testUpdateObjectStoreNothingChanged() { + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + final String provider = "provider1"; + Mockito.when(objectStoreVO.getProviderName()).thenReturn(provider); + Mockito.when(updateCmd.getProviderName()).thenReturn(provider); + long storeId = 1L; + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + // Same Name - no change. + final String name = "store name"; + Mockito.when(updateCmd.getName()).thenReturn(name); + Mockito.when(objectStoreVO.getName()).thenReturn(name); + + // Same URL - no change. + final String url = "http://object"; + Mockito.when(updateCmd.getUrl()).thenReturn(url); + Mockito.when(objectStoreVO.getUrl()).thenReturn(url); + + // 2 detail maps but same contents (as nothing updated) + Map mapOld = new HashMap<>(); + Map mapNew = new HashMap<>(); + mapOld.put("key1", "value1"); + mapNew.put("key1", "value1"); + Mockito.when(updateCmd.getDetails()).thenReturn(mapNew); + Mockito.when(_objectStoreDetailsDao.getDetails(Mockito.anyLong())).thenReturn(mapOld); + + ObjectStoreEntity objectStoreEntity = Mockito.mock(ObjectStoreEntity.class); + Mockito.when(_dataStoreMgr.getDataStore(storeId, DataStoreRole.Object)).thenReturn(objectStoreEntity); + + // Test the update. + Assert.assertNotNull(storageManagerImpl.updateObjectStore(storeId, updateCmd)); + + // The object store should never get an update call as no change. + Mockito.verify(_objectStoreDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any()); + Mockito.verify(_objectStoreDetailsDao, Mockito.never()).update(Mockito.anyLong(), Mockito.anyMap()); + // The Object Store connectivity should be confirmed + Mockito.verify(objectStoreEntity, Mockito.atLeastOnce()).verifyServiceConnectivity(); + } + + @Test + public void testUpdateObjectStoreNameChanged() { + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + final String provider = "provider1"; + Mockito.when(objectStoreVO.getProviderName()).thenReturn(provider); + Mockito.when(updateCmd.getProviderName()).thenReturn(provider); + long storeId = 1L; + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + // Name Changed + final String nameOld = "name old"; + final String nameNew = "name new"; + Mockito.when(updateCmd.getName()).thenReturn(nameNew); + Mockito.when(objectStoreVO.getName()).thenReturn(nameOld); + + // Same URL - no change. + final String url = "http://object"; + Mockito.when(updateCmd.getUrl()).thenReturn(url); + Mockito.when(objectStoreVO.getUrl()).thenReturn(url); + + // 2 detail maps but same contents (as nothing updated) + Map mapOld = new HashMap<>(); + Map mapNew = new HashMap<>(); + mapOld.put("key1", "value1"); + mapNew.put("key1", "value1"); + Mockito.when(updateCmd.getDetails()).thenReturn(mapNew); + Mockito.when(_objectStoreDetailsDao.getDetails(Mockito.anyLong())).thenReturn(mapOld); + + ObjectStoreEntity objectStoreEntity = Mockito.mock(ObjectStoreEntity.class); + Mockito.when(_dataStoreMgr.getDataStore(storeId, DataStoreRole.Object)).thenReturn(objectStoreEntity); + + // Test the update. + Assert.assertNotNull(storageManagerImpl.updateObjectStore(storeId, updateCmd)); + + // Only the name should be updated in the VO + Mockito.verify(objectStoreVO, Mockito.times(1)).setName(nameNew); + Mockito.verify(objectStoreVO, Mockito.never()).setUrl(Mockito.anyString()); + // Only the Store should be updated in the Dao, not the details. + Mockito.verify(_objectStoreDao, Mockito.times(1)).update(Mockito.anyLong(), Mockito.any()); + Mockito.verify(_objectStoreDetailsDao, Mockito.never()).update(Mockito.anyLong(), Mockito.anyMap()); + // The Object Store connectivity should be confirmed + Mockito.verify(objectStoreEntity, Mockito.atLeastOnce()).verifyServiceConnectivity(); + } + + @Test + public void testUpdateObjectStoreUrlChanged() { + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + final String provider = "provider1"; + Mockito.when(objectStoreVO.getProviderName()).thenReturn(provider); + Mockito.when(updateCmd.getProviderName()).thenReturn(provider); + long storeId = 1L; + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + // Name unchanged + final String name = "name"; + Mockito.when(updateCmd.getName()).thenReturn(name); + Mockito.when(objectStoreVO.getName()).thenReturn(name); + + // URL changed. + final String urlOld = "http://object"; + final String urlNew = "http://10.10.10.10"; + Mockito.when(updateCmd.getUrl()).thenReturn(urlNew); + Mockito.when(objectStoreVO.getUrl()).thenReturn(urlOld); + + // 2 detail maps but same contents (as nothing updated) + Map mapOld = new HashMap<>(); + Map mapNew = new HashMap<>(); + mapOld.put("key1", "value1"); + mapNew.put("key1", "value1"); + Mockito.when(updateCmd.getDetails()).thenReturn(mapNew); + Mockito.when(_objectStoreDetailsDao.getDetails(Mockito.anyLong())).thenReturn(mapOld); + + ObjectStoreEntity objectStoreEntity = Mockito.mock(ObjectStoreEntity.class); + Mockito.when(_dataStoreMgr.getDataStore(storeId, DataStoreRole.Object)).thenReturn(objectStoreEntity); + + // Test the update. + Assert.assertNotNull(storageManagerImpl.updateObjectStore(storeId, updateCmd)); + + // Only the url should be updated in the VO + Mockito.verify(objectStoreVO, Mockito.never()).setName(Mockito.anyString()); + Mockito.verify(objectStoreVO, Mockito.times(1)).setUrl(urlNew); + // Only the Store should be updated in the Dao, not the details. + Mockito.verify(_objectStoreDao, Mockito.times(1)).update(Mockito.anyLong(), Mockito.any()); + Mockito.verify(_objectStoreDetailsDao, Mockito.never()).update(Mockito.anyLong(), Mockito.anyMap()); + // The Object Store connectivity should be confirmed + Mockito.verify(objectStoreEntity, Mockito.atLeastOnce()).verifyServiceConnectivity(); + } + + @Test + public void testUpdateObjectStoreDetailsChanged() { + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + final String provider = "provider1"; + Mockito.when(objectStoreVO.getProviderName()).thenReturn(provider); + Mockito.when(updateCmd.getProviderName()).thenReturn(provider); + long storeId = 1L; + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + // Name unchanged + final String name = "name"; + Mockito.when(updateCmd.getName()).thenReturn(name); + Mockito.when(objectStoreVO.getName()).thenReturn(name); + + // Same URL - no change. + final String url = "http://object"; + Mockito.when(updateCmd.getUrl()).thenReturn(url); + Mockito.when(objectStoreVO.getUrl()).thenReturn(url); + + // details changed + Map mapOld = new HashMap<>(); + Map mapNew = new HashMap<>(); + mapOld.put("key1", "oldValue"); + mapNew.put("key1", "newValue"); + Mockito.when(updateCmd.getDetails()).thenReturn(mapNew); + Mockito.when(_objectStoreDetailsDao.getDetails(Mockito.anyLong())).thenReturn(mapOld); + + ObjectStoreEntity objectStoreEntity = Mockito.mock(ObjectStoreEntity.class); + Mockito.when(_dataStoreMgr.getDataStore(storeId, DataStoreRole.Object)).thenReturn(objectStoreEntity); + + // Test the update. + Assert.assertNotNull(storageManagerImpl.updateObjectStore(storeId, updateCmd)); + + // Neither Url nor Name updated + Mockito.verify(objectStoreVO, Mockito.never()).setName(Mockito.anyString()); + Mockito.verify(objectStoreVO, Mockito.never()).setUrl(Mockito.anyString()); + // Store not updated, but details should be with the new map. + Mockito.verify(_objectStoreDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any()); + Mockito.verify(_objectStoreDetailsDao, Mockito.times(1)).update(storeId, mapNew); + // The Object Store connectivity should be confirmed + Mockito.verify(objectStoreEntity, Mockito.atLeastOnce()).verifyServiceConnectivity(); + } + + @Test + public void testUpdateObjectStoreRollback() { + UpdateObjectStoragePoolCmd updateCmd = Mockito.mock(UpdateObjectStoragePoolCmd.class); + ObjectStoreVO objectStoreVO = Mockito.mock(ObjectStoreVO.class); + + final String provider = "provider1"; + Mockito.when(objectStoreVO.getProviderName()).thenReturn(provider); + Mockito.when(updateCmd.getProviderName()).thenReturn(provider); + long storeId = 1L; + Mockito.when(_objectStoreDao.findById(Mockito.anyLong())).thenReturn(objectStoreVO); + + // Name Changed + final String nameOld = "name old"; + final String nameNew = "name new"; + Mockito.when(updateCmd.getName()).thenReturn(nameNew); + Mockito.when(objectStoreVO.getName()).thenReturn(nameOld); + + // Same URL - no change. + final String url = "http://object"; + Mockito.when(updateCmd.getUrl()).thenReturn(url); + Mockito.when(objectStoreVO.getUrl()).thenReturn(url); + + // Details updated + Map mapOld = new HashMap<>(); + Map mapNew = new HashMap<>(); + mapOld.put("key1", "oldValue"); + mapNew.put("key1", "newValue"); + Mockito.when(updateCmd.getDetails()).thenReturn(mapNew); + Mockito.when(_objectStoreDetailsDao.getDetails(Mockito.anyLong())).thenReturn(mapOld); + + ObjectStoreEntity objectStoreEntity = Mockito.mock(ObjectStoreEntity.class); + Mockito.when(_dataStoreMgr.getDataStore(storeId, DataStoreRole.Object)).thenReturn(objectStoreEntity); + // Simulate failure in connectivity to trigger rollback + Mockito.doThrow(new CloudRuntimeException("connectivity failure")).when(objectStoreEntity).verifyServiceConnectivity(); + + // Test the update which should fail + IllegalArgumentException thrown = Assert.assertThrows(IllegalArgumentException.class, () -> storageManagerImpl.updateObjectStore(storeId, updateCmd)); + Assert.assertNotNull(thrown); + Assert.assertTrue(thrown.getMessage().contains("Unable to access Object Storage")); + + // Name updated with nameNew, then nameOld on rollback. + // url updated only on rollback (as name was update). + Mockito.verify(objectStoreVO, Mockito.times(2)).setName(Mockito.anyString()); + Mockito.verify(objectStoreVO, Mockito.times(1)).setUrl(url); + // Store updated and then updated again on rollback. + Mockito.verify(_objectStoreDao, Mockito.times(2)).update(Mockito.anyLong(), Mockito.any()); + // details updated and then update again on rollback + Mockito.verify(_objectStoreDetailsDao, Mockito.times(1)).update(storeId, mapNew); + Mockito.verify(_objectStoreDetailsDao, Mockito.times(1)).update(storeId, mapOld); + } @Test public void testNoStorageAccessGroupsOnHostAndStoragePool() { diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index a85ccc97cf0b..2a9fb0fcebc1 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -2433,6 +2433,7 @@ "label.update.ip.range": "Update IP range", "label.update.ipv4.subnet": "Update IPv4 subnet", "label.update.network": "Update Network", +"label.update.object.storage": "Update Object Storage", "label.update.physical.network": "Update physical Network", "label.update.project.role": "Update project role", "label.update.ssl": " SSL certificate", @@ -3653,6 +3654,7 @@ "message.success.update.ipv4.subnet": "Successfully updated IPv4 subnet", "message.success.update.kubeversion": "Successfully updated Kubernetes supported version", "message.success.update.network": "Successfully updated Network", +"message.success.update.object.storage": "Successfully updated Object Storage", "message.success.update.template": "Successfully updated Template", "message.success.update.user": "Successfully updated User", "message.success.upgrade.kubernetes": "Successfully upgraded Kubernetes cluster", diff --git a/ui/src/config/section/infra/objectStorages.js b/ui/src/config/section/infra/objectStorages.js index 821c1b2d9484..00634d1f1813 100644 --- a/ui/src/config/section/infra/objectStorages.js +++ b/ui/src/config/section/infra/objectStorages.js @@ -59,8 +59,9 @@ export default { api: 'updateObjectStoragePool', icon: 'edit-outlined', label: 'label.action.update.object.storage', - args: ['name', 'url'], - dataView: true + dataView: true, + popup: true, + component: shallowRef(defineAsyncComponent(() => import('@/views/infra/UpdateObjectStorage.vue'))) }, { api: 'deleteObjectStoragePool', diff --git a/ui/src/views/infra/UpdateObjectStorage.vue b/ui/src/views/infra/UpdateObjectStorage.vue new file mode 100644 index 000000000000..8536bef6f22e --- /dev/null +++ b/ui/src/views/infra/UpdateObjectStorage.vue @@ -0,0 +1,210 @@ +// 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. + + + +