Skip to content

Commit d6ac91f

Browse files
shwstpprshwstppr
authored
minio: fix store user creation (apache#8425)
To prevent errors during multi-user access, use account UUID to create/access user on the provider side. Also, update the existing secret key for a user that already exists.
1 parent 2b28a66 commit d6ac91f

2 files changed

Lines changed: 123 additions & 53 deletions

File tree

‎plugins/storage/object/minio/src/main/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImpl.java‎

Lines changed: 69 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,38 @@
1818
*/
1919
package org.apache.cloudstack.storage.datastore.driver;
2020

21+
import java.io.IOException;
22+
import java.security.InvalidKeyException;
23+
import java.security.NoSuchAlgorithmException;
24+
import java.util.ArrayList;
25+
import java.util.List;
26+
import java.util.Map;
27+
28+
import javax.crypto.KeyGenerator;
29+
import javax.crypto.SecretKey;
30+
import javax.inject.Inject;
31+
32+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
33+
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao;
34+
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao;
35+
import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO;
36+
import org.apache.cloudstack.storage.object.BaseObjectStoreDriverImpl;
37+
import org.apache.cloudstack.storage.object.Bucket;
38+
import org.apache.cloudstack.storage.object.BucketObject;
39+
import org.apache.commons.codec.binary.Base64;
40+
import org.apache.commons.lang3.StringUtils;
41+
import org.apache.log4j.Logger;
42+
2143
import com.amazonaws.services.s3.model.AccessControlList;
2244
import com.amazonaws.services.s3.model.BucketPolicy;
2345
import com.cloud.agent.api.to.DataStoreTO;
24-
import org.apache.cloudstack.storage.object.Bucket;
2546
import com.cloud.storage.BucketVO;
2647
import com.cloud.storage.dao.BucketDao;
2748
import com.cloud.user.Account;
2849
import com.cloud.user.AccountDetailsDao;
2950
import com.cloud.user.dao.AccountDao;
3051
import com.cloud.utils.exception.CloudRuntimeException;
52+
3153
import io.minio.BucketExistsArgs;
3254
import io.minio.DeleteBucketEncryptionArgs;
3355
import io.minio.MakeBucketArgs;
@@ -42,26 +64,10 @@
4264
import io.minio.admin.messages.DataUsageInfo;
4365
import io.minio.messages.SseConfiguration;
4466
import io.minio.messages.VersioningConfiguration;
45-
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
46-
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao;
47-
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao;
48-
import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO;
49-
import org.apache.cloudstack.storage.object.BaseObjectStoreDriverImpl;
50-
import org.apache.cloudstack.storage.object.BucketObject;
51-
import org.apache.commons.codec.binary.Base64;
52-
import org.apache.log4j.Logger;
53-
54-
import javax.crypto.KeyGenerator;
55-
import javax.crypto.SecretKey;
56-
import javax.inject.Inject;
57-
import java.security.NoSuchAlgorithmException;
58-
import java.util.ArrayList;
59-
import java.util.HashMap;
60-
import java.util.List;
61-
import java.util.Map;
6267

6368
public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl {
6469
private static final Logger s_logger = Logger.getLogger(MinIOObjectStoreDriverImpl.class);
70+
protected static final String ACS_PREFIX = "acs";
6571

6672
@Inject
6773
AccountDao _accountDao;
@@ -81,14 +87,18 @@ public class MinIOObjectStoreDriverImpl extends BaseObjectStoreDriverImpl {
8187
private static final String ACCESS_KEY = "accesskey";
8288
private static final String SECRET_KEY = "secretkey";
8389

84-
private static final String MINIO_ACCESS_KEY = "minio-accesskey";
85-
private static final String MINIO_SECRET_KEY = "minio-secretkey";
90+
protected static final String MINIO_ACCESS_KEY = "minio-accesskey";
91+
protected static final String MINIO_SECRET_KEY = "minio-secretkey";
8692

8793
@Override
8894
public DataStoreTO getStoreTO(DataStore store) {
8995
return null;
9096
}
9197

98+
protected String getUserOrAccessKeyForAccount(Account account) {
99+
return String.format("%s-%s", ACS_PREFIX, account.getUuid());
100+
}
101+
92102
@Override
93103
public Bucket createBucket(Bucket bucket, boolean objectLock) {
94104
//ToDo Client pool mgmt
@@ -135,8 +145,8 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) {
135145
" \"Version\": \"2012-10-17\"\n" +
136146
" }";
137147
MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId);
138-
String policyName = "acs-"+account.getAccountName()+"-policy";
139-
String userName = "acs-"+account.getAccountName();
148+
String policyName = getUserOrAccessKeyForAccount(account) + "-policy";
149+
String userName = getUserOrAccessKeyForAccount(account);
140150
try {
141151
minioAdminClient.addCannedPolicy(policyName, policy);
142152
minioAdminClient.setPolicy(userName, false, policyName);
@@ -250,22 +260,53 @@ public void deleteBucketPolicy(String bucketName, long storeId) {
250260

251261
}
252262

263+
protected void updateAccountCredentials(final long accountId, final String accessKey, final String secretKey, final boolean checkIfNotPresent) {
264+
Map<String, String> details = _accountDetailsDao.findDetails(accountId);
265+
boolean updateNeeded = false;
266+
if (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_ACCESS_KEY))) {
267+
details.put(MINIO_ACCESS_KEY, accessKey);
268+
updateNeeded = true;
269+
}
270+
if (StringUtils.isAllBlank(secretKey, details.get(MINIO_SECRET_KEY))) {
271+
s_logger.error(String.format("Failed to retrieve secret key for MinIO user: %s from store and account details", accessKey));
272+
}
273+
if (StringUtils.isNotBlank(secretKey) && (!checkIfNotPresent || StringUtils.isBlank(details.get(MINIO_SECRET_KEY)))) {
274+
details.put(MINIO_SECRET_KEY, secretKey);
275+
updateNeeded = true;
276+
}
277+
if (!updateNeeded) {
278+
return;
279+
}
280+
_accountDetailsDao.persist(accountId, details);
281+
}
282+
253283
@Override
254284
public boolean createUser(long accountId, long storeId) {
255285
Account account = _accountDao.findById(accountId);
256286
MinioAdminClient minioAdminClient = getMinIOAdminClient(storeId);
257-
String accessKey = "acs-"+account.getAccountName();
287+
String accessKey = getUserOrAccessKeyForAccount(account);
258288
// Check user exists
259289
try {
260290
UserInfo userInfo = minioAdminClient.getUserInfo(accessKey);
261291
if(userInfo != null) {
262-
s_logger.debug("User already exists in MinIO store: "+accessKey);
292+
if (s_logger.isDebugEnabled()) {
293+
s_logger.debug(String.format("Skipping user creation as the user already exists in MinIO store: %s", accessKey));
294+
}
295+
updateAccountCredentials(accountId, accessKey, userInfo.secretKey(), true);
263296
return true;
264297
}
265-
} catch (Exception e) {
266-
s_logger.debug("User does not exist. Creating user: "+accessKey);
298+
} catch (NoSuchAlgorithmException | IOException | InvalidKeyException e) {
299+
s_logger.error(String.format("Error encountered while retrieving user: %s for existing MinIO store user check", accessKey), e);
300+
return false;
301+
} catch (RuntimeException e) { // MinIO lib may throw RuntimeException with code: XMinioAdminNoSuchUser
302+
if (s_logger.isDebugEnabled()) {
303+
s_logger.debug(String.format("Ignoring error encountered while retrieving user: %s for existing MinIO store user check", accessKey));
304+
}
305+
s_logger.trace("Exception during MinIO user check", e);
306+
}
307+
if (s_logger.isDebugEnabled()) {
308+
s_logger.debug(String.format("MinIO store user does not exist. Creating user: %s", accessKey));
267309
}
268-
269310
KeyGenerator generator = null;
270311
try {
271312
generator = KeyGenerator.getInstance("HmacSHA1");
@@ -280,10 +321,7 @@ public boolean createUser(long accountId, long storeId) {
280321
throw new CloudRuntimeException(e);
281322
}
282323
// Store user credentials
283-
Map<String, String> details = new HashMap<>();
284-
details.put(MINIO_ACCESS_KEY, accessKey);
285-
details.put(MINIO_SECRET_KEY, secretKey);
286-
_accountDetailsDao.persist(accountId, details);
324+
updateAccountCredentials(accountId, accessKey, secretKey, false);
287325
return true;
288326
}
289327

‎plugins/storage/object/minio/src/test/java/org/apache/cloudstack/storage/datastore/driver/MinIOObjectStoreDriverImplTest.java‎

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,23 @@
1616
// under the License.
1717
package org.apache.cloudstack.storage.datastore.driver;
1818

19-
import com.cloud.storage.BucketVO;
20-
import com.cloud.storage.dao.BucketDao;
21-
import com.cloud.user.AccountDetailVO;
22-
import com.cloud.user.AccountDetailsDao;
23-
import com.cloud.user.AccountVO;
24-
import com.cloud.user.dao.AccountDao;
25-
import io.minio.BucketExistsArgs;
26-
import io.minio.MinioClient;
27-
import io.minio.RemoveBucketArgs;
28-
import io.minio.admin.MinioAdminClient;
19+
import static org.junit.Assert.assertEquals;
20+
import static org.junit.Assert.assertTrue;
21+
import static org.mockito.Mockito.any;
22+
import static org.mockito.Mockito.anyLong;
23+
import static org.mockito.Mockito.anyString;
24+
import static org.mockito.Mockito.doNothing;
25+
import static org.mockito.Mockito.doReturn;
26+
import static org.mockito.Mockito.mock;
27+
import static org.mockito.Mockito.times;
28+
import static org.mockito.Mockito.verify;
29+
import static org.mockito.Mockito.when;
30+
31+
import java.util.ArrayList;
32+
import java.util.HashMap;
33+
import java.util.Map;
34+
import java.util.UUID;
35+
2936
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao;
3037
import org.apache.cloudstack.storage.datastore.db.ObjectStoreDetailsDao;
3138
import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO;
@@ -34,22 +41,24 @@
3441
import org.junit.Test;
3542
import org.junit.runner.RunWith;
3643
import org.mockito.Mock;
44+
import org.mockito.Mockito;
3745
import org.mockito.MockitoAnnotations;
3846
import org.mockito.Spy;
3947
import org.mockito.junit.MockitoJUnitRunner;
48+
import org.mockito.stubbing.Answer;
4049

41-
import java.util.ArrayList;
50+
import com.cloud.storage.BucketVO;
51+
import com.cloud.storage.dao.BucketDao;
52+
import com.cloud.user.AccountDetailVO;
53+
import com.cloud.user.AccountDetailsDao;
54+
import com.cloud.user.AccountVO;
55+
import com.cloud.user.dao.AccountDao;
4256

43-
import static org.junit.Assert.assertEquals;
44-
import static org.junit.Assert.assertTrue;
45-
import static org.mockito.Mockito.any;
46-
import static org.mockito.Mockito.anyLong;
47-
import static org.mockito.Mockito.anyString;
48-
import static org.mockito.Mockito.doNothing;
49-
import static org.mockito.Mockito.doReturn;
50-
import static org.mockito.Mockito.times;
51-
import static org.mockito.Mockito.verify;
52-
import static org.mockito.Mockito.when;
57+
import io.minio.BucketExistsArgs;
58+
import io.minio.MinioClient;
59+
import io.minio.RemoveBucketArgs;
60+
import io.minio.admin.MinioAdminClient;
61+
import io.minio.admin.UserInfo;
5362

5463
@RunWith(MockitoJUnitRunner.class)
5564
public class MinIOObjectStoreDriverImplTest {
@@ -97,7 +106,7 @@ public void testCreateBucket() throws Exception {
97106
doReturn(minioClient).when(minioObjectStoreDriverImpl).getMinIOClient(anyLong());
98107
doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong());
99108
when(bucketDao.listByObjectStoreIdAndAccountId(anyLong(), anyLong())).thenReturn(new ArrayList<BucketVO>());
100-
when(account.getAccountName()).thenReturn("admin");
109+
when(account.getUuid()).thenReturn(UUID.randomUUID().toString());
101110
when(accountDao.findById(anyLong())).thenReturn(account);
102111
when(accountDetailsDao.findDetail(anyLong(),anyString())).
103112
thenReturn(new AccountDetailVO(1L, "abc","def"));
@@ -119,4 +128,27 @@ public void testDeleteBucket() throws Exception {
119128
verify(minioClient, times(1)).bucketExists(any());
120129
verify(minioClient, times(1)).removeBucket(any());
121130
}
131+
132+
@Test
133+
public void testCreateUserExisting() throws Exception {
134+
String uuid = "uuid";
135+
String accessKey = MinIOObjectStoreDriverImpl.ACS_PREFIX + "-" + uuid;
136+
String secretKey = "secret";
137+
138+
doReturn(minioAdminClient).when(minioObjectStoreDriverImpl).getMinIOAdminClient(anyLong());
139+
when(accountDao.findById(anyLong())).thenReturn(account);
140+
when(account.getUuid()).thenReturn(uuid);
141+
UserInfo info = mock(UserInfo.class);
142+
when(info.secretKey()).thenReturn(secretKey);
143+
when(minioAdminClient.getUserInfo(accessKey)).thenReturn(info);
144+
final Map<String, String> persistedMap = new HashMap<>();
145+
Mockito.doAnswer((Answer<Void>) invocation -> {
146+
persistedMap.putAll((Map<String, String>)invocation.getArguments()[1]);
147+
return null;
148+
}).when(accountDetailsDao).persist(Mockito.anyLong(), Mockito.anyMap());
149+
boolean result = minioObjectStoreDriverImpl.createUser(1L, 1L);
150+
assertTrue(result);
151+
assertEquals(accessKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_ACCESS_KEY));
152+
assertEquals(secretKey, persistedMap.get(MinIOObjectStoreDriverImpl.MINIO_SECRET_KEY));
153+
}
122154
}

0 commit comments

Comments
 (0)