From aa22ba67140f2e9168bc7abcd3085a4913db1b82 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 2 Jan 2024 14:13:27 +0530 Subject: [PATCH 1/5] minio: fix store user creation To prevent error during multi-user access, use account UUID to create/access user on the procider side. Also, update existing secret key for a user that already exist. Signed-off-by: Abhishek Kumar --- .../driver/MinIOObjectStoreDriverImpl.java | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-) 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 15df5df56dc1..c6e42d0c54e5 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 @@ -18,16 +18,36 @@ */ package org.apache.cloudstack.storage.datastore.driver; +import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; +import javax.inject.Inject; + +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +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.object.BaseObjectStoreDriverImpl; +import org.apache.cloudstack.storage.object.Bucket; +import org.apache.cloudstack.storage.object.BucketObject; +import org.apache.commons.codec.binary.Base64; +import org.apache.log4j.Logger; + import com.amazonaws.services.s3.model.AccessControlList; import com.amazonaws.services.s3.model.BucketPolicy; import com.cloud.agent.api.to.DataStoreTO; -import org.apache.cloudstack.storage.object.Bucket; import com.cloud.storage.BucketVO; import com.cloud.storage.dao.BucketDao; import com.cloud.user.Account; import com.cloud.user.AccountDetailsDao; import com.cloud.user.dao.AccountDao; import com.cloud.utils.exception.CloudRuntimeException; + import io.minio.BucketExistsArgs; import io.minio.DeleteBucketEncryptionArgs; import io.minio.MakeBucketArgs; @@ -42,26 +62,10 @@ import io.minio.admin.messages.DataUsageInfo; import io.minio.messages.SseConfiguration; import io.minio.messages.VersioningConfiguration; -import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; -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.object.BaseObjectStoreDriverImpl; -import org.apache.cloudstack.storage.object.BucketObject; -import org.apache.commons.codec.binary.Base64; -import org.apache.log4j.Logger; - -import javax.crypto.KeyGenerator; -import javax.crypto.SecretKey; -import javax.inject.Inject; -import java.security.NoSuchAlgorithmException; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { private static final Logger s_logger = Logger.getLogger(MinIOObjectStoreDriverImpl.class); + private static final String ACS_PREFIX = "acs"; @Inject AccountDao _accountDao; @@ -89,6 +93,10 @@ public DataStoreTO getStoreTO(DataStore store) { return null; } + protected String getUserOrAccessKeyForAccount(Account account) { + return String.format("%s-%s", ACS_PREFIX, account.getUuid()); + } + @Override public Bucket createBucket(Bucket bucket, boolean objectLock) { //ToDo Client pool mgmt @@ -135,8 +143,8 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) { " \"Version\": \"2012-10-17\"\n" + " }"; MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId); - String policyName = "acs-"+account.getAccountName()+"-policy"; - String userName = "acs-"+account.getAccountName(); + String policyName = getUserOrAccessKeyForAccount(account) + "-policy"; + String userName = getUserOrAccessKeyForAccount(account); try { minioAdminClient.addCannedPolicy(policyName, policy); minioAdminClient.setPolicy(userName, false, policyName); @@ -250,20 +258,28 @@ public void deleteBucketPolicy(String bucketName, long storeId) { } + protected void updateAccountCredentials(final long accountId, final String accessKey, final String secretKey) { + Map details = new HashMap<>(); + details.put(MINIO_ACCESS_KEY, accessKey); + details.put(MINIO_SECRET_KEY, secretKey); + _accountDetailsDao.persist(accountId, details); + } + @Override public boolean createUser(long accountId, long storeId) { Account account = _accountDao.findById(accountId); MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId); - String accessKey = "acs-"+account.getAccountName(); + String accessKey = getUserOrAccessKeyForAccount(account); // Check user exists try { UserInfo userInfo = minioAdminClient.getUserInfo(accessKey); if(userInfo != null) { - s_logger.debug("User already exists in MinIO store: "+accessKey); + s_logger.info(String.format("User already exists in MinIO store: %s, updating credentials", accessKey)); + updateAccountCredentials(accountId, accessKey, userInfo.secretKey()); return true; } } catch (Exception e) { - s_logger.debug("User does not exist. Creating user: "+accessKey); + s_logger.debug("User does not exist. Creating user: " + accessKey); } KeyGenerator generator = null; @@ -280,10 +296,7 @@ public boolean createUser(long accountId, long storeId) { throw new CloudRuntimeException(e); } // Store user credentials - Map details = new HashMap<>(); - details.put(MINIO_ACCESS_KEY, accessKey); - details.put(MINIO_SECRET_KEY, secretKey); - _accountDetailsDao.persist(accountId, details); + updateAccountCredentials(accountId, accessKey, secretKey); return true; } From adb8785b54f3cdaebf8bc6f981db7ac7f987e05d Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 2 Jan 2024 15:29:17 +0530 Subject: [PATCH 2/5] test fix Signed-off-by: Abhishek Kumar --- .../driver/MinIOObjectStoreDriverImpl.java | 6 +- .../MinIOObjectStoreDriverImplTest.java | 76 +++++++++++++------ 2 files changed, 57 insertions(+), 25 deletions(-) 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 c6e42d0c54e5..9845aecda8dd 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 @@ -65,7 +65,7 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { private static final Logger s_logger = Logger.getLogger(MinIOObjectStoreDriverImpl.class); - private static final String ACS_PREFIX = "acs"; + protected static final String ACS_PREFIX = "acs"; @Inject AccountDao _accountDao; @@ -85,8 +85,8 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { private static final String ACCESS_KEY = "accesskey"; private static final String SECRET_KEY = "secretkey"; - private static final String MINIO_ACCESS_KEY = "minio-accesskey"; - private static final String MINIO_SECRET_KEY = "minio-secretkey"; + protected static final String MINIO_ACCESS_KEY = "minio-accesskey"; + protected static final String MINIO_SECRET_KEY = "minio-secretkey"; @Override public DataStoreTO getStoreTO(DataStore store) { diff --git a/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java b/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java index 3041a5b232bf..ac88a0ded398 100644 --- a/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java +++ b/plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java @@ -16,16 +16,23 @@ // under the License. package org.apache.cloudstack.storage.datastore.driver; -import com.cloud.storage.BucketVO; -import com.cloud.storage.dao.BucketDao; -import com.cloud.user.AccountDetailVO; -import com.cloud.user.AccountDetailsDao; -import com.cloud.user.AccountVO; -import com.cloud.user.dao.AccountDao; -import io.minio.BucketExistsArgs; -import io.minio.MinioClient; -import io.minio.RemoveBucketArgs; -import io.minio.admin.MinioAdminClient; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyLong; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao; import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; @@ -34,22 +41,24 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; -import java.util.ArrayList; +import com.cloud.storage.BucketVO; +import com.cloud.storage.dao.BucketDao; +import com.cloud.user.AccountDetailVO; +import com.cloud.user.AccountDetailsDao; +import com.cloud.user.AccountVO; +import com.cloud.user.dao.AccountDao; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyLong; -import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import io.minio.BucketExistsArgs; +import io.minio.MinioClient; +import io.minio.RemoveBucketArgs; +import io.minio.admin.MinioAdminClient; +import io.minio.admin.UserInfo; @RunWith(MockitoJUnitRunner.class) public class MinIOObjectStoreDriverImplTest { @@ -97,7 +106,7 @@ public void testCreateBucket() throws Exception { doReturn(minioClient).when(minioObjectStoreDriverImpl).getMinIOClient(anyLong()); doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong()); when(bucketDao.listByObjectStoreIdAndAccountId(anyLong(), anyLong())).thenReturn(new ArrayList()); - when(account.getAccountName()).thenReturn("admin"); + when(account.getUuid()).thenReturn(UUID.randomUUID().toString()); when(accountDao.findById(anyLong())).thenReturn(account); when(accountDetailsDao.findDetail(anyLong(),anyString())). thenReturn(new AccountDetailVO(1L, "abc","def")); @@ -119,4 +128,27 @@ public void testDeleteBucket() throws Exception { verify(minioClient, times(1)).bucketExists(any()); verify(minioClient, times(1)).removeBucket(any()); } + + @Test + public void testCreateUserExisting() throws Exception { + String uuid = "uuid"; + String accessKey = MinIOObjectStoreDriverImpl.ACS_PREFIX + "-" + uuid; + String secretKey = "secret"; + + doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong()); + when(accountDao.findById(anyLong())).thenReturn(account); + when(account.getUuid()).thenReturn(uuid); + UserInfo info = mock(UserInfo.class); + when(info.secretKey()).thenReturn(secretKey); + when(minioAdminClient.getUserInfo(accessKey)).thenReturn(info); + final Map persistedMap = new HashMap<>(); + Mockito.doAnswer((Answer) invocation -> { + persistedMap.putAll((Map)invocation.getArguments()[1]); + return null; + }).when(accountDetailsDao).persist(Mockito.anyLong(), Mockito.anyMap()); + boolean result = minioObjectStoreDriverImpl.createUser(1L, 1L); + assertTrue(result); + assertEquals(accessKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_ACCESS_KEY)); + assertEquals(secretKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_SECRET_KEY)); + } } From fba6c5610b5074b8c0efd486da2aaa5e96ea5e2c Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 4 Jan 2024 17:39:28 +0530 Subject: [PATCH 3/5] log fix Signed-off-by: Abhishek Kumar --- .../storage/datastore/driver/MinIOObjectStoreDriverImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9845aecda8dd..8e469e8dde46 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 @@ -279,7 +279,7 @@ public boolean createUser(long accountId, long storeId) { return true; } } catch (Exception e) { - s_logger.debug("User does not exist. Creating user: " + accessKey); + s_logger.debug(String.format("User does not exist. Creating user: %s", accessKey)); } KeyGenerator generator = null; From a28b78a1ab078ab4534fb20da75c3691471054ca Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 5 Jan 2024 15:05:25 +0530 Subject: [PATCH 4/5] prevent secret key update when not returned in minio response Signed-off-by: Abhishek Kumar --- .../driver/MinIOObjectStoreDriverImpl.java | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 deletions(-) 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 8e469e8dde46..125f2443927d 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 @@ -18,9 +18,10 @@ */ package org.apache.cloudstack.storage.datastore.driver; +import java.io.IOException; +import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -36,6 +37,7 @@ import org.apache.cloudstack.storage.object.Bucket; import org.apache.cloudstack.storage.object.BucketObject; import org.apache.commons.codec.binary.Base64; +import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import com.amazonaws.services.s3.model.AccessControlList; @@ -258,10 +260,23 @@ public void deleteBucketPolicy(String bucketName, long storeId) { } - protected void updateAccountCredentials(final long accountId, final String accessKey, final String secretKey) { - Map details = new HashMap<>(); - details.put(MINIO_ACCESS_KEY, accessKey); - details.put(MINIO_SECRET_KEY, secretKey); + protected void updateAccountCredentials(final long accountId, final String accessKey, final String secretKey, final boolean checkIfNotPresent) { + Map details = _accountDetailsDao.findDetails(accountId); + boolean updateNeeded = false; + if (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_ACCESS_KEY))) { + details.put(MINIO_ACCESS_KEY, accessKey); + updateNeeded = true; + } + if (StringUtils.isAllBlank(secretKey, details.get(MINIO_SECRET_KEY))) { + s_logger.error(String.format("Failed to retrieve secret key for MinIO user: %s from store and account details", accessKey)); + } + if (StringUtils.isNotBlank(secretKey) && (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_SECRET_KEY)))) { + details.put(MINIO_SECRET_KEY, secretKey); + updateNeeded = true; + } + if (!updateNeeded) { + return; + } _accountDetailsDao.persist(accountId, details); } @@ -274,14 +289,19 @@ public boolean createUser(long accountId, long storeId) { try { UserInfo userInfo = minioAdminClient.getUserInfo(accessKey); if(userInfo != null) { - s_logger.info(String.format("User already exists in MinIO store: %s, updating credentials", accessKey)); - updateAccountCredentials(accountId, accessKey, userInfo.secretKey()); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Skipping user creation as the user already exists in MinIO store: %s", accessKey)); + } + updateAccountCredentials(accountId, accessKey, userInfo.secretKey(), true); return true; } - } catch (Exception e) { - s_logger.debug(String.format("User does not exist. Creating user: %s", accessKey)); + } catch (NoSuchAlgorithmException | IOException | InvalidKeyException e) { + s_logger.error(String.format("Error encountered while retrieving user: %s for existing MinIO store user check", accessKey), e); + return false; + } + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("MinIO store user does not exist. Creating user: %s", accessKey)); } - KeyGenerator generator = null; try { generator = KeyGenerator.getInstance("HmacSHA1"); @@ -296,7 +316,7 @@ public boolean createUser(long accountId, long storeId) { throw new CloudRuntimeException(e); } // Store user credentials - updateAccountCredentials(accountId, accessKey, secretKey); + updateAccountCredentials(accountId, accessKey, secretKey, false); return true; } From 2a6de43cbc7a8591eabb6f9bb48b69fa4144cdb9 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 8 Jan 2024 17:48:30 +0530 Subject: [PATCH 5/5] handle RuntimeException Signed-off-by: Abhishek Kumar --- .../storage/datastore/driver/MinIOObjectStoreDriverImpl.java | 5 +++++ 1 file changed, 5 insertions(+) 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 125f2443927d..b85383a65e83 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 @@ -298,6 +298,11 @@ public boolean createUser(long accountId, long storeId) { } catch (NoSuchAlgorithmException | IOException | InvalidKeyException e) { s_logger.error(String.format("Error encountered while retrieving user: %s for existing MinIO store user check", accessKey), e); return false; + } catch (RuntimeException e) { // MinIO lib may throw RuntimeException with code: XMinioAdminNoSuchUser + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Ignoring error encountered while retrieving user: %s for existing MinIO store user check", accessKey)); + } + s_logger.trace("Exception during MinIO user check", e); } if (s_logger.isDebugEnabled()) { s_logger.debug(String.format("MinIO store user does not exist. Creating user: %s", accessKey));