Skip to content

Commit bc00f66

Browse files
chrislusfchrislusf
committed
fix(seaweedfs): address second round of PR review comments
- add bounded connect (10s) and per-request (30s) timeouts to the S3 extension HttpClient/HttpRequest so a stalled endpoint cannot block management-server API threads indefinitely - only accept 2xx as success for the quota S3 extension request; 3xx was previously treated as success even though HttpClient does not follow redirects by default and the mutation was not applied - omit a bucket from the usage result on S3 listing failure instead of returning 0, so BucketApiServiceImpl does not overwrite stored usage with a false zero on a transient endpoint/permission failure - prefer the current ObjectStoreVO.url over the persisted s3Url detail in getS3Url so bucket operations follow a live endpoint after an administrator updates the store URL - namespace per-account IAM credential keys by store ID so one account using multiple SeaweedFS pools does not have the second pool overwrite the first pool's credentials - update existing BucketVO rows with the new IAM credentials on key rotation (updateAccountBucketCredentials), mirroring the Cloudian driver, so previously created buckets stop handing out the old key - add SeaweedFS to the Add Object Storage UI provider list - fix testSetBucketQuotaNoS3ConfigThrows to actually clear store details so the exception comes from the missing-config check, not a network call - add a deterministic SigV4 signature-verification test that independently signs the same request and asserts the Authorization header, payload hash, and date match exactly - add a test asserting 3xx responses are rejected for quota requests
1 parent 5109b1c commit bc00f66

4 files changed

Lines changed: 201 additions & 30 deletions

File tree

‎plugins/storage/object/seaweedfs/src/main/java/org/apache/cloudstack/storage/datastore/driver/SeaweedFSObjectStoreDriverImpl.java‎

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,9 @@ public boolean createUser(long accountId, long storeId) {
149149
// Reuse the stored access key if it is still present in IAM; only
150150
// create a new one when no usable key exists.
151151
Map<String, String> details = _accountDetailsDao.findDetails(accountId);
152-
String storedAccessKeyId = details.get(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY);
152+
String accessKeyDetailKey = SeaweedFSObjectStoreUtil.keyAccessKey(storeId);
153+
String secretKeyDetailKey = SeaweedFSObjectStoreUtil.keySecretKey(storeId);
154+
String storedAccessKeyId = details.get(accessKeyDetailKey);
153155
if (storedAccessKeyId != null && iamAccessKeyExists(iamClient, userName, storedAccessKeyId)) {
154156
logger.debug("Reusing existing IAM access key {} for user {}", storedAccessKeyId, userName);
155157
return true;
@@ -164,15 +166,35 @@ public boolean createUser(long accountId, long storeId) {
164166
new CreateAccessKeyRequest().withUserName(userName));
165167
AccessKey key = result.getAccessKey();
166168

167-
// Persist the credentials in the account details
168-
details.put(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY, key.getAccessKeyId());
169-
details.put(SeaweedFSObjectStoreUtil.KEY_SECRET_KEY, key.getSecretAccessKey());
169+
// Persist the credentials in the account details (namespaced by storeId)
170+
details.put(accessKeyDetailKey, key.getAccessKeyId());
171+
details.put(secretKeyDetailKey, key.getSecretAccessKey());
170172
_accountDetailsDao.persist(accountId, details);
171173

174+
// Update existing bucket records for this account/store with the new
175+
// credentials so previously created buckets don't keep handing out
176+
// the old (now invalid) key pair.
177+
updateAccountBucketCredentials(storeId, accountId, key);
178+
172179
logger.info("Created IAM credentials {} for user {}", key.getAccessKeyId(), userName);
173180
return true;
174181
}
175182

183+
/**
184+
* Update the IAM credentials on all BucketVO rows for this store/account
185+
* so previously created buckets reflect the new (rotated) key pair.
186+
* Mirrors CloudianHyperStoreObjectStoreDriverImpl.updateAccountBucketCredentials.
187+
*/
188+
private void updateAccountBucketCredentials(long storeId, long accountId, AccessKey iamCredential) {
189+
List<BucketVO> bucketList = _bucketDao.listByObjectStoreIdAndAccountId(storeId, accountId);
190+
for (BucketVO bucketVO : bucketList) {
191+
logger.info("Updating accountId={} bucket {} with new IAM credentials", accountId, bucketVO.getName());
192+
bucketVO.setAccessKey(iamCredential.getAccessKeyId());
193+
bucketVO.setSecretKey(iamCredential.getSecretAccessKey());
194+
_bucketDao.update(bucketVO.getId(), bucketVO);
195+
}
196+
}
197+
176198
/**
177199
* Check whether the given access key id is still listed in IAM for the user.
178200
*/
@@ -249,8 +271,8 @@ public Bucket createBucket(Bucket bucket, boolean objectLock) {
249271

250272
// Update the bucket record with the account's IAM credentials
251273
Map<String, String> accountDetails = _accountDetailsDao.findDetails(accountId);
252-
String accessKey = accountDetails.get(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY);
253-
String secretKey = accountDetails.get(SeaweedFSObjectStoreUtil.KEY_SECRET_KEY);
274+
String accessKey = accountDetails.get(SeaweedFSObjectStoreUtil.keyAccessKey(storeId));
275+
String secretKey = accountDetails.get(SeaweedFSObjectStoreUtil.keySecretKey(storeId));
254276
if (accessKey == null || secretKey == null) {
255277
logger.warn("No IAM credentials found for account {}. Bucket will be created without per-account credentials.", accountId);
256278
}
@@ -465,7 +487,7 @@ public void setBucketQuota(BucketTO bucket, long storeId, long size) {
465487
* touching the network.
466488
*/
467489
protected java.net.http.HttpClient getS3ExtensionHttpClient() {
468-
return java.net.http.HttpClient.newHttpClient();
490+
return SeaweedFSObjectStoreUtil.newS3ExtensionHttpClient();
469491
}
470492

471493
@Override
@@ -502,8 +524,10 @@ public Map<String, Long> getAllBucketsUsage(long storeId) {
502524
} while (result.isTruncated());
503525
bucketUsage.put(bucket.getName(), size);
504526
} catch (AmazonClientException e) {
505-
logger.warn("Failed to get usage for bucket {}: {}", bucket.getName(), e.getMessage());
506-
bucketUsage.put(bucket.getName(), 0L);
527+
// Omit the bucket rather than reporting 0 — returning 0 would
528+
// cause BucketApiServiceImpl to overwrite the stored size with
529+
// a false zero, erasing known usage on a transient failure.
530+
logger.warn("Failed to get usage for bucket {} (omitting from result): {}", bucket.getName(), e.getMessage());
507531
}
508532
}
509533
return bucketUsage;
@@ -512,13 +536,17 @@ public Map<String, Long> getAllBucketsUsage(long storeId) {
512536
// ---- Client builders ----
513537

514538
protected String getS3Url(long storeId) {
515-
Map<String, String> storeDetails = _storeDetailsDao.getDetails(storeId);
516-
String s3Url = storeDetails.get(SeaweedFSObjectStoreUtil.STORE_DETAILS_KEY_S3_URL);
517-
if (s3Url == null || s3Url.isEmpty()) {
518-
ObjectStoreVO store = _storeDao.findById(storeId);
519-
s3Url = store.getUrl();
539+
// Prefer the current store URL (ObjectStoreVO.url) over the persisted
540+
// s3Url detail. initialize() persists a resolved s3Url detail, but if
541+
// an administrator later updates the store URL via updateObjectStore,
542+
// the detail becomes stale. Using the current store URL keeps bucket
543+
// operations pointed at the live endpoint.
544+
ObjectStoreVO store = _storeDao.findById(storeId);
545+
if (store != null && store.getUrl() != null && ! store.getUrl().isEmpty()) {
546+
return store.getUrl();
520547
}
521-
return s3Url;
548+
Map<String, String> storeDetails = _storeDetailsDao.getDetails(storeId);
549+
return storeDetails.get(SeaweedFSObjectStoreUtil.STORE_DETAILS_KEY_S3_URL);
522550
}
523551

524552
protected String getIAMUrl(long storeId) {

‎plugins/storage/object/seaweedfs/src/main/java/org/apache/cloudstack/storage/datastore/util/SeaweedFSObjectStoreUtil.java‎

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,34 @@ public class SeaweedFSObjectStoreUtil {
5353
public static final String STORE_DETAILS_KEY_S3_URL = "s3Url"; // S3 endpoint URL
5454
public static final String STORE_DETAILS_KEY_IAM_URL = "iamUrl"; // IAM endpoint URL
5555

56-
// Account Detail Map key names - credentials created per CloudStack account
57-
public static final String KEY_ACCESS_KEY = "swfs_AccessKey";
58-
public static final String KEY_SECRET_KEY = "swfs_SecretKey";
56+
// Account Detail Map key names - credentials created per CloudStack account.
57+
// Namespaced by store ID so one account can use multiple SeaweedFS pools
58+
// without the second pool overwriting the first pool's credentials.
59+
public static final String KEY_ACCESS_KEY_PREFIX = "swfs_AccessKey_";
60+
public static final String KEY_SECRET_KEY_PREFIX = "swfs_SecretKey_";
61+
62+
/**
63+
* Build the account-detail key for the IAM access key of a given store.
64+
*/
65+
public static String keyAccessKey(long storeId) {
66+
return KEY_ACCESS_KEY_PREFIX + storeId;
67+
}
68+
69+
/**
70+
* Build the account-detail key for the IAM secret key of a given store.
71+
*/
72+
public static String keySecretKey(long storeId) {
73+
return KEY_SECRET_KEY_PREFIX + storeId;
74+
}
75+
76+
/**
77+
* Connect timeout for the S3 extension HTTP client, in seconds.
78+
*/
79+
public static final int S3_EXTENSION_CONNECT_TIMEOUT_SECONDS = 10;
80+
/**
81+
* Per-request timeout for the S3 extension HTTP request, in seconds.
82+
*/
83+
public static final int S3_EXTENSION_REQUEST_TIMEOUT_SECONDS = 30;
5984

6085
/**
6186
* IAM user policy applied to each per-account IAM user. Grants full S3
@@ -201,7 +226,18 @@ public static void validateIAMUrl(String iamUrl) {
201226
* @throws CloudRuntimeException on any failure
202227
*/
203228
public static void setBucketQuotaViaS3Extension(String s3Url, String accessKey, String secretKey, String bucketName, long sizeGiB) {
204-
setBucketQuotaViaS3Extension(s3Url, accessKey, secretKey, bucketName, sizeGiB, java.net.http.HttpClient.newHttpClient());
229+
setBucketQuotaViaS3Extension(s3Url, accessKey, secretKey, bucketName, sizeGiB, newS3ExtensionHttpClient());
230+
}
231+
232+
/**
233+
* Build a bounded HTTP client for SeaweedFS S3 extension requests with a
234+
* connect timeout so a stalled endpoint cannot block the management-server
235+
* API thread indefinitely.
236+
*/
237+
public static java.net.http.HttpClient newS3ExtensionHttpClient() {
238+
return java.net.http.HttpClient.newBuilder()
239+
.connectTimeout(java.time.Duration.ofSeconds(S3_EXTENSION_CONNECT_TIMEOUT_SECONDS))
240+
.build();
205241
}
206242

207243
/**
@@ -305,7 +341,8 @@ protected static String executeSignedS3Request(String method, String s3Url, Stri
305341
fullUri = java.net.URI.create(fullUri.toString() + "?" + queryString);
306342
}
307343
java.net.http.HttpRequest.Builder reqBuilder = java.net.http.HttpRequest.newBuilder()
308-
.uri(fullUri);
344+
.uri(fullUri)
345+
.timeout(java.time.Duration.ofSeconds(S3_EXTENSION_REQUEST_TIMEOUT_SECONDS));
309346
for (java.util.Map.Entry<String, String> entry : request.getHeaders().entrySet()) {
310347
String headerName = entry.getKey();
311348
if (headerName == null || entry.getValue() == null) {
@@ -325,10 +362,11 @@ protected static String executeSignedS3Request(String method, String s3Url, Stri
325362
java.net.http.HttpResponse<String> response = httpClient.send(reqBuilder.build(),
326363
java.net.http.HttpResponse.BodyHandlers.ofString());
327364

328-
if (response.statusCode() >= 400) {
365+
int statusCode = response.statusCode();
366+
if (statusCode < 200 || statusCode >= 300) {
329367
throw new CloudRuntimeException(String.format(
330368
"S3 extension request %s %s failed with status %d: %s",
331-
method, fullUri, response.statusCode(), response.body()));
369+
method, fullUri, statusCode, response.body()));
332370
}
333371
return response.body();
334372
} catch (CloudRuntimeException e) {

‎plugins/storage/object/seaweedfs/src/test/java/org/apache/cloudstack/storage/datastore/driver/SeaweedFSObjectStoreDriverImplTest.java‎

Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,8 @@ public void setUp() {
154154
lenient().when(objectStoreDetailsDao.getDetails(TEST_STORE_ID)).thenReturn(storeDetailsMap);
155155

156156
accountDetailsMap = new HashMap<>();
157-
accountDetailsMap.put(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY, TEST_AK);
158-
accountDetailsMap.put(SeaweedFSObjectStoreUtil.KEY_SECRET_KEY, TEST_SK);
157+
accountDetailsMap.put(SeaweedFSObjectStoreUtil.keyAccessKey(TEST_STORE_ID), TEST_AK);
158+
accountDetailsMap.put(SeaweedFSObjectStoreUtil.keySecretKey(TEST_STORE_ID), TEST_SK);
159159
lenient().when(accountDetailsDao.findDetails(TEST_ACCOUNT_ID)).thenReturn(accountDetailsMap);
160160

161161
bucketVo = new BucketVO(TEST_ACCOUNT_ID, TEST_DOMAIN_ID, TEST_STORE_ID, TEST_BUCKET_NAME, null, false, false, false, null);
@@ -337,11 +337,116 @@ public void testSetBucketQuotaPropagatesFailure() throws Exception {
337337
assertThrows(CloudRuntimeException.class, () -> driver.setBucketQuota(bucketTO, TEST_STORE_ID, 10));
338338
}
339339

340+
@Test
341+
public void testSetBucketQuotaRejects3xx() throws Exception {
342+
BucketTO bucketTO = mock(BucketTO.class);
343+
when(bucketTO.getName()).thenReturn(TEST_BUCKET_NAME);
344+
doReturn(TEST_S3_URL).when(driver).getS3Url(TEST_STORE_ID);
345+
doReturn("access-key").when(driver).getAccessKey(TEST_STORE_ID);
346+
doReturn("secret-key").when(driver).getSecretKey(TEST_STORE_ID);
347+
348+
HttpClient mockHttpClient = mock(HttpClient.class);
349+
HttpResponse<String> mockResponse = mock(HttpResponse.class);
350+
// 3xx must NOT be treated as success — the mutation was not applied
351+
when(mockResponse.statusCode()).thenReturn(302);
352+
when(mockResponse.body()).thenReturn("redirect");
353+
when(mockHttpClient.send(ArgumentMatchers.<HttpRequest>any(),
354+
ArgumentMatchers.<HttpResponse.BodyHandler<String>>any())).thenReturn(mockResponse);
355+
doReturn(mockHttpClient).when(driver).getS3ExtensionHttpClient();
356+
357+
assertThrows(CloudRuntimeException.class, () -> driver.setBucketQuota(bucketTO, TEST_STORE_ID, 10));
358+
}
359+
360+
/**
361+
* Deterministic SigV4 signature-verification test.
362+
*
363+
* Signs the same request through the AWS SDK v1 AWSS3V4Signer (the same
364+
* signer the production code uses) and asserts that the Authorization
365+
* header, signed headers, x-amz-content-sha256, and x-amz-date produced
366+
* by the driver's request match. This catches signing regressions (e.g.
367+
* the query parameter not being in the canonical query string) that a
368+
* mere "header exists" check would miss.
369+
*/
370+
@Test
371+
public void testSetBucketQuotaSigV4SignatureVerification() throws Exception {
372+
String accessKey = "AKIAIOSFODNN7EXAMPLE";
373+
String secretKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
374+
String bucketName = "quota-sig-test";
375+
String s3Url = "http://s3.example.com:8333";
376+
long quotaGiB = 5;
377+
378+
BucketTO bucketTO = mock(BucketTO.class);
379+
when(bucketTO.getName()).thenReturn(bucketName);
380+
doReturn(s3Url).when(driver).getS3Url(TEST_STORE_ID);
381+
doReturn(accessKey).when(driver).getAccessKey(TEST_STORE_ID);
382+
doReturn(secretKey).when(driver).getSecretKey(TEST_STORE_ID);
383+
384+
HttpClient mockHttpClient = mock(HttpClient.class);
385+
HttpResponse<String> mockResponse = mock(HttpResponse.class);
386+
when(mockResponse.statusCode()).thenReturn(200);
387+
when(mockResponse.body()).thenReturn("");
388+
when(mockHttpClient.send(ArgumentMatchers.<HttpRequest>any(),
389+
ArgumentMatchers.<HttpResponse.BodyHandler<String>>any())).thenReturn(mockResponse);
390+
doReturn(mockHttpClient).when(driver).getS3ExtensionHttpClient();
391+
392+
driver.setBucketQuota(bucketTO, TEST_STORE_ID, quotaGiB);
393+
394+
ArgumentCaptor<HttpRequest> reqCaptor = ArgumentCaptor.forClass(HttpRequest.class);
395+
verify(mockHttpClient, times(1)).send(reqCaptor.capture(),
396+
ArgumentMatchers.<HttpResponse.BodyHandler<String>>any());
397+
HttpRequest sent = reqCaptor.getValue();
398+
399+
// Build the expected signed request the same way the production code does
400+
String expectedBody = String.format("{\"quota_size\":%d,\"quota_unit\":\"GB\",\"quota_enabled\":true}", quotaGiB);
401+
byte[] bodyBytes = expectedBody.getBytes(StandardCharsets.UTF_8);
402+
403+
com.amazonaws.DefaultRequest<?> expectedRequest = new com.amazonaws.DefaultRequest<>("s3");
404+
expectedRequest.setEndpoint(java.net.URI.create(s3Url));
405+
expectedRequest.setHttpMethod(com.amazonaws.http.HttpMethodName.PUT);
406+
expectedRequest.setResourcePath("/" + bucketName);
407+
expectedRequest.addParameter("seaweedfs-quota", "");
408+
expectedRequest.setContent(new java.io.ByteArrayInputStream(bodyBytes));
409+
expectedRequest.getHeaders().put("Content-Length", String.valueOf(bodyBytes.length));
410+
expectedRequest.getHeaders().put("Content-Type", "application/json");
411+
412+
com.amazonaws.auth.AWSCredentials credentials = new com.amazonaws.auth.BasicAWSCredentials(accessKey, secretKey);
413+
com.amazonaws.services.s3.internal.AWSS3V4Signer signer = new com.amazonaws.services.s3.internal.AWSS3V4Signer();
414+
signer.setServiceName("s3");
415+
signer.setRegionName("us-east-1");
416+
signer.sign(expectedRequest, credentials);
417+
418+
// The Authorization header must match exactly — proves the canonical
419+
// query string (including seaweedfs-quota), payload hash, and signed
420+
// headers all match the independently signed reference request.
421+
String expectedAuth = expectedRequest.getHeaders().get("Authorization");
422+
String actualAuth = sent.headers().firstValue("Authorization").orElse(null);
423+
assertNotNull("Authorization header must be present", actualAuth);
424+
assertEquals("SigV4 Authorization header must match the reference signature", expectedAuth, actualAuth);
425+
426+
// The payload hash must be present and match
427+
String expectedContentSha = expectedRequest.getHeaders().get("x-amz-content-sha256");
428+
String actualContentSha = sent.headers().firstValue("x-amz-content-sha256").orElse(null);
429+
assertEquals("x-amz-content-sha256 must match", expectedContentSha, actualContentSha);
430+
431+
// The signed headers list must include the query-signing-relevant headers
432+
String expectedDate = expectedRequest.getHeaders().get("x-amz-date");
433+
String actualDate = sent.headers().firstValue("x-amz-date").orElse(null);
434+
assertEquals("x-amz-date must match", expectedDate, actualDate);
435+
436+
// The query string must carry the subresource
437+
assertNotNull("URI must have a query string", sent.uri().getQuery());
438+
assertTrue("query must carry seaweedfs-quota", sent.uri().getQuery().contains("seaweedfs-quota"));
439+
}
440+
340441
@Test
341442
public void testSetBucketQuotaNoS3ConfigThrows() {
342443
BucketTO bucketTO = mock(BucketTO.class);
343444
when(bucketTO.getName()).thenReturn(TEST_BUCKET_NAME);
344-
// No S3 URL/credentials configured — should throw with a clear message
445+
// Clear store details so no S3 URL/credentials are configured.
446+
// Without this, setUp() stubs valid values and the exception would
447+
// come from a real network call rather than the missing-config check.
448+
storeDetailsMap.clear();
449+
lenient().when(objectStoreDao.findById(TEST_STORE_ID)).thenReturn(null);
345450
assertThrows(CloudRuntimeException.class, () -> driver.setBucketQuota(bucketTO, TEST_STORE_ID, 10));
346451
}
347452

@@ -402,8 +507,8 @@ public void testCreateUserNew() throws Exception {
402507
ArgumentCaptor<Map<String, String>> detailsCaptor = ArgumentCaptor.forClass((Class<Map<String, String>>) (Class<?>) Map.class);
403508
verify(accountDetailsDao, times(1)).persist(anyLong(), detailsCaptor.capture());
404509
Map<String, String> persisted = detailsCaptor.getValue();
405-
assertEquals(TEST_AK, persisted.get(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY));
406-
assertEquals(TEST_SK, persisted.get(SeaweedFSObjectStoreUtil.KEY_SECRET_KEY));
510+
assertEquals(TEST_AK, persisted.get(SeaweedFSObjectStoreUtil.keyAccessKey(TEST_STORE_ID)));
511+
assertEquals(TEST_SK, persisted.get(SeaweedFSObjectStoreUtil.keySecretKey(TEST_STORE_ID)));
407512
}
408513

409514
@Test
@@ -454,8 +559,8 @@ public void testCreateUserStoredKeyMissingCreatesReplacement() throws Exception
454559
ArgumentCaptor<Map<String, String>> detailsCaptor = ArgumentCaptor.forClass((Class<Map<String, String>>) (Class<?>) Map.class);
455560
verify(accountDetailsDao, times(1)).persist(anyLong(), detailsCaptor.capture());
456561
Map<String, String> persisted = detailsCaptor.getValue();
457-
assertEquals("new-ak", persisted.get(SeaweedFSObjectStoreUtil.KEY_ACCESS_KEY));
458-
assertEquals("new-sk", persisted.get(SeaweedFSObjectStoreUtil.KEY_SECRET_KEY));
562+
assertEquals("new-ak", persisted.get(SeaweedFSObjectStoreUtil.keyAccessKey(TEST_STORE_ID)));
563+
assertEquals("new-sk", persisted.get(SeaweedFSObjectStoreUtil.keySecretKey(TEST_STORE_ID)));
459564
}
460565

461566
@Test

‎ui/src/views/infra/AddObjectStorage.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ export default {
127127
inject: ['parentFetchData'],
128128
data () {
129129
return {
130-
providers: ['MinIO', 'Ceph', 'Cloudian HyperStore', 'Simulator'],
130+
providers: ['MinIO', 'Ceph', 'Cloudian HyperStore', 'SeaweedFS', 'Simulator'],
131131
zones: [],
132132
loading: false
133133
}

0 commit comments

Comments
 (0)