From 625044532a58c313e8a1ffe360e4ae6e7a5087bd Mon Sep 17 00:00:00 2001 From: Thomas O'Dowd Date: Wed, 30 Oct 2024 07:23:57 +0000 Subject: [PATCH] Allow updating Object Storage Details Currently, when an administrator edits an Object Storage Pool, only the name or url can be updated. This change allows the other `details` of the object store to be updated. The object store details contains the credentials used to connect to the object store as well as other information provided when it was added to the system. Only updated information is updated. The connection is tested using a new method verifyServiceConnectivity(). If no information has changed, the existing connection is also tested. --- .../java/com/cloud/server/ResourceTag.java | 2 +- .../storage/UpdateObjectStoragePoolCmd.java | 32 ++- .../UpdateObjectStoragePoolCmdTest.java | 33 +++ .../storage/object/ObjectStoreEntity.java | 2 + .../storage/object/store/ObjectStoreImpl.java | 5 + .../storage/object/ObjectStoreDriver.java | 8 + .../driver/CephObjectStoreDriverImpl.java | 6 + ...oudianHyperStoreObjectStoreDriverImpl.java | 149 +++++----- ...ianHyperStoreObjectStoreLifeCycleImpl.java | 2 +- ...dianHyperStoreObjectStoreProviderImpl.java | 8 +- .../util/CloudianHyperStoreUtil.java | 14 +- ...anHyperStoreObjectStoreDriverImplTest.java | 107 ++++--- ...yperStoreObjectStoreLifeCycleImplTest.java | 6 +- .../util/CloudianHyperStoreUtilTest.java | 72 +++-- .../driver/MinIOObjectStoreDriverImpl.java | 6 + .../SimulatorObjectStoreDriverImpl.java | 4 + .../com/cloud/api/query/QueryManagerImpl.java | 6 + .../metadata/ResourceMetaDataManagerImpl.java | 4 + .../com/cloud/storage/StorageManagerImpl.java | 78 +++-- .../cloud/tags/ResourceManagerUtilImpl.java | 2 + .../cloud/api/query/QueryManagerImplTest.java | 37 +++ .../cloud/storage/StorageManagerImplTest.java | 268 ++++++++++++++++++ ui/public/locales/en.json | 2 + ui/src/config/section/infra/objectStorages.js | 5 +- ui/src/views/infra/UpdateObjectStorage.vue | 210 ++++++++++++++ 25 files changed, 901 insertions(+), 167 deletions(-) create mode 100644 ui/src/views/infra/UpdateObjectStorage.vue diff --git a/api/src/main/java/com/cloud/server/ResourceTag.java b/api/src/main/java/com/cloud/server/ResourceTag.java index 9bbb5d43eaeb..0e6712f427ff 100644 --- a/api/src/main/java/com/cloud/server/ResourceTag.java +++ b/api/src/main/java/com/cloud/server/ResourceTag.java @@ -70,7 +70,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 7eb350a06439..bec6abdd8d0d 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 846fc1921992..a30006b7b4e9 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -5331,6 +5331,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 938ceb2b0928..d8d71d2a4b07 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -4303,34 +4303,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 c02f41368634..862058382663 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; @@ -117,6 +118,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); } @Inject 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 5bfb05530407..f0a4832b7927 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 999bf85907be..a4079ce5fef8 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -25,16 +25,22 @@ 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.engine.subsystem.api.storage.PrimaryDataStoreDriver; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; 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; @@ -105,6 +111,12 @@ public class StorageManagerImplTest { AccountManagerImpl accountMgr; @Mock StoragePoolDetailsDao storagePoolDetailsDao; + @Mock + DataStoreManager _dataStoreMgr; + @Mock + ObjectStoreDao _objectStoreDao; + @Mock + ObjectStoreDetailsDao _objectStoreDetailsDao; @Mock ClusterDao clusterDao; @@ -894,4 +906,260 @@ public void testGetStoragePoolIopsStats_UsedIopsNegative() { Assert.assertEquals("Capacity IOPS should match pool's capacity IOPS", 1000L, result.first().longValue()); 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); + } } diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 2df2ebf5903f..1b97f07e2a3a 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -2408,6 +2408,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", @@ -3610,6 +3611,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. + + + +