Skip to content

[feature] add alibaba cloud oss support #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions manager/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
<easy-poi.version>4.3.0</easy-poi.version>
<huawei.sdk.version>3.1.37</huawei.sdk.version>
<huawei.obs.version>3.23.5</huawei.obs.version>
<alibaba.oss.version>3.17.4</alibaba.oss.version>
</properties>

<dependencies>
Expand Down Expand Up @@ -198,6 +199,11 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>com.aliyun.oss</groupId>
<artifactId>aliyun-sdk-oss</artifactId>
<version>${alibaba.oss.version}</version>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@ public enum Type {
/**
* <a href="https://support.huaweicloud.com/obs/index.html">Huawei Cloud OBS</a>
*/
OBS
OBS,

/**
* <a href="https://oss.console.aliyun.com/services/tools">Alibaba Cloud OSS</a>
*/
OSS,
}

/**
Expand All @@ -73,4 +78,20 @@ public static class ObsConfig {
private String savePath = "hertzbeat";
}

/**
* file oss storage configuration
*/
@Data
public static class OssConfig {
private String accessKey;
private String secretKey;
private String bucketName;
private String endpoint;

/**
* Save path
*/
private String savePath = "hertzbeat";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.hertzbeat.manager.service.impl;

import com.aliyun.oss.OSSClientBuilder;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.obs.services.ObsClient;
Expand Down Expand Up @@ -83,10 +84,28 @@ public void handler(ObjectStoreDTO<T> config) {
initObs(config);
// case other object store service
}
if (config.getType() == ObjectStoreDTO.Type.OSS) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code structure: Consider using a switch statement instead of multiple if conditions for handling different object store types, which would be more maintainable as more store types are added.

initOss(config);
}
ctx.publishEvent(new ObjectStoreConfigChangeEvent(config));
}
}

private void initOss(ObjectStoreDTO<T> config) {
var ossConfig = objectMapper.convertValue(config.getConfig(), ObjectStoreDTO.OssConfig.class);
Assert.hasText(ossConfig.getAccessKey(), "cannot find oss accessKey");
Assert.hasText(ossConfig.getSecretKey(), "cannot find oss secretKey");
Assert.hasText(ossConfig.getEndpoint(), "cannot find oss endpoint");
Assert.hasText(ossConfig.getBucketName(), "cannot find oss bucket name");

var ossClient = new OSSClientBuilder().build(ossConfig.getEndpoint(), ossConfig.getAccessKey(), ossConfig.getSecretKey());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation: Unlike the initOss method, there's no validation for the OSS client initialization. Consider adding error handling to check if the OSS client was successfully created.


beanFactory.destroySingleton(BEAN_NAME);
beanFactory.registerSingleton(BEAN_NAME, new OssObjectStoreServiceImpl(ossClient, ossConfig.getBucketName(), ossConfig.getSavePath()));

log.info("oss store service init success.");
}

/**
* init Huawei Cloud OBS
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* 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.
*/

package org.apache.hertzbeat.manager.service.impl;

import com.aliyun.oss.OSS;
import com.aliyun.oss.model.ListObjectsRequest;
import com.aliyun.oss.model.OSSObjectSummary;
import java.io.InputStream;
import java.util.List;
import java.util.Objects;
import lombok.extern.slf4j.Slf4j;
import org.apache.hertzbeat.common.constants.SignConstants;
import org.apache.hertzbeat.manager.pojo.dto.FileDTO;
import org.apache.hertzbeat.manager.pojo.dto.ObjectStoreDTO;
import org.apache.hertzbeat.manager.service.ObjectStoreService;

/**
* Alibaba cloud storage service
*/
@Slf4j
public class OssObjectStoreServiceImpl implements ObjectStoreService {

private final OSS ossClient;

private final String bucketName;
private final String rootPath;

public OssObjectStoreServiceImpl(OSS ossClient, String bucketName, String rootPath) {
this.ossClient = ossClient;
this.bucketName = bucketName;
if (rootPath.startsWith(SignConstants.RIGHT_DASH)) {
this.rootPath = rootPath.substring(1);
} else {
this.rootPath = rootPath;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Consider adding a close() method to properly shut down the OSS client when it's no longer needed. The Alibaba OSS client resources should be released when done to prevent resource leaks.

@Override
public boolean upload(String filePath, InputStream is) {
var objectKey = getObjectKey(filePath);
var response = ossClient.putObject(bucketName, objectKey, is);
return Objects.equals(response.getResponse().getStatusCode(), 200);
}
Comment on lines +53 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling to prevent uncaught exceptions.

The upload method doesn't have any exception handling, which could cause unexpected crashes if the OSS client throws an exception during upload.

@Override
public boolean upload(String filePath, InputStream is) {
-    var objectKey = getObjectKey(filePath);
-    var response = ossClient.putObject(bucketName, objectKey, is);
-    return Objects.equals(response.getResponse().getStatusCode(), 200);
+    try {
+        var objectKey = getObjectKey(filePath);
+        var response = ossClient.putObject(bucketName, objectKey, is);
+        return response.getResponse() != null && 
+               Objects.equals(response.getResponse().getStatusCode(), 200);
+    } catch (Exception ex) {
+        log.error("Failed to upload file to OSS: {}", filePath, ex);
+        return false;
+    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public boolean upload(String filePath, InputStream is) {
var objectKey = getObjectKey(filePath);
var response = ossClient.putObject(bucketName, objectKey, is);
return Objects.equals(response.getResponse().getStatusCode(), 200);
}
@Override
public boolean upload(String filePath, InputStream is) {
try {
var objectKey = getObjectKey(filePath);
var response = ossClient.putObject(bucketName, objectKey, is);
return response.getResponse() != null &&
Objects.equals(response.getResponse().getStatusCode(), 200);
} catch (Exception ex) {
log.error("Failed to upload file to OSS: {}", filePath, ex);
return false;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: The response check may not be reliable. The OSS client's putObject method can return a response with null response field in some cases. Consider checking for exceptions instead of status code.


@Override
public FileDTO download(String filePath) {
var objectKey = getObjectKey(filePath);
try {
var ossObject = ossClient.getObject(bucketName, objectKey);
return new FileDTO(filePath, ossObject.getObjectContent());
} catch (Exception ex) {
log.warn("download file from oss error {}", objectKey);
return null;
}
}

@Override
public List<FileDTO> list(String dir) {
var request = new ListObjectsRequest(bucketName);
request.setPrefix(getObjectKey(dir));
return ossClient.listObjects(request).getObjectSummaries().stream()
.map(OSSObjectSummary::getKey).toList().stream()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance concern: This implementation loads all objects in memory at once. For directories with many files, this could lead to memory issues. Consider implementing pagination or streaming the results.

.map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential resource leak: The OSS objects retrieved in the list method aren't being closed. Each getObject() call opens a stream that should be properly closed to avoid resource leaks.

}
Comment on lines +72 to +79
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and performance in list method.

The list method has two issues:

  1. No exception handling, which could lead to unexpected crashes
  2. Each object is fetched individually with a separate API call, which can be inefficient for directories with many files
@Override
public List<FileDTO> list(String dir) {
+    try {
        var request = new ListObjectsRequest(bucketName);
        request.setPrefix(getObjectKey(dir));
-        return ossClient.listObjects(request).getObjectSummaries().stream()
-                .map(OSSObjectSummary::getKey).toList().stream()
-                .map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList();
+        var summaries = ossClient.listObjects(request).getObjectSummaries();
+        return summaries.stream()
+                .map(summary -> {
+                    try {
+                        String key = summary.getKey();
+                        var object = ossClient.getObject(bucketName, key);
+                        return new FileDTO(key, object.getObjectContent());
+                    } catch (Exception e) {
+                        log.warn("Failed to get object content for key: {}", summary.getKey(), e);
+                        return null;
+                    }
+                })
+                .filter(Objects::nonNull)
+                .toList();
+    } catch (Exception ex) {
+        log.error("Failed to list files from OSS directory: {}", dir, ex);
+        return List.of();
+    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public List<FileDTO> list(String dir) {
var request = new ListObjectsRequest(bucketName);
request.setPrefix(getObjectKey(dir));
return ossClient.listObjects(request).getObjectSummaries().stream()
.map(OSSObjectSummary::getKey).toList().stream()
.map(k -> new FileDTO(k, ossClient.getObject(bucketName, k).getObjectContent())).toList();
}
@Override
public List<FileDTO> list(String dir) {
try {
var request = new ListObjectsRequest(bucketName);
request.setPrefix(getObjectKey(dir));
var summaries = ossClient.listObjects(request).getObjectSummaries();
return summaries.stream()
.map(summary -> {
try {
String key = summary.getKey();
var object = ossClient.getObject(bucketName, key);
return new FileDTO(key, object.getObjectContent());
} catch (Exception e) {
log.warn("Failed to get object content for key: {}", summary.getKey(), e);
return null;
}
})
.filter(Objects::nonNull)
.toList();
} catch (Exception ex) {
log.error("Failed to list files from OSS directory: {}", dir, ex);
return List.of();
}
}


@Override
public ObjectStoreDTO.Type type() {
return ObjectStoreDTO.Type.OSS;
}


private String getObjectKey(String filePath) {
return rootPath + SignConstants.RIGHT_DASH + filePath;
}
}
14 changes: 13 additions & 1 deletion web-app/src/app/pojo/ObjectStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ export enum ObjectStoreType {
/**
* <a href="https://support.huaweicloud.com/obs/index.html">华为云OBS</a>
*/
OBS = 'OBS'
OBS = 'OBS',
/**
* <a href="https://oss.console.aliyun.com/services/tools">Alibaba Cloud OSS</a>
*/
OSS = 'OSS'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Consider adding a trailing comma after the OSS enum value for consistency with other enum values and to make future additions cleaner in git diffs.

}

export class ObsConfig {
Expand All @@ -42,3 +46,11 @@ export class ObsConfig {
endpoint!: string;
savePath: string = 'hertzbeat';
}

export class OssConfig {
accessKey!: string;
secretKey!: string;
bucketName!: string;
endpoint!: string;
savePath: string = 'hertzbeat';
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,21 @@
>
<nz-option [nzValue]="ObjectStoreType.FILE" [nzLabel]="'settings.object-store.type.file' | i18n"></nz-option>
<nz-option [nzValue]="ObjectStoreType.OBS" [nzLabel]="'settings.object-store.type.obs' | i18n"></nz-option>
<nz-option [nzValue]="ObjectStoreType.OSS" [nzLabel]="'settings.object-store.type.oss' | i18n"></nz-option>
</nz-select>
</nz-form-item>
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS">
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS || config.type == ObjectStoreType.OSS">
<nz-form-label [nzSpan]="6" nzFor="obs.accessKey" nzRequired="true">{{
'settings.object-store.obs.accessKey' | i18n
}}</nz-form-label>
<nz-form-control [nzErrorTip]="'validation.required' | i18n">
<input
[(ngModel)]="config.config.accessKey"
placeholder="{{ 'settings.object-store.obs.accessKey.placeholder' | i18n }}"
placeholder="{{
config.type == ObjectStoreType.OBS
? ('settings.object-store.obs.accessKey.placeholder' | i18n)
: ('settings.object-store.oss.accessKey.placeholder' | i18n)
}}"
nz-input
required
name="accessKey"
Expand All @@ -50,14 +55,18 @@
/>
</nz-form-control>
</nz-form-item>
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS">
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS || config.type == ObjectStoreType.OSS">
<nz-form-label [nzSpan]="6" nzFor="obs.secretKey" nzRequired="true">{{
'settings.object-store.obs.secretKey' | i18n
}}</nz-form-label>
<nz-form-control [nzErrorTip]="'validation.required' | i18n">
<input
[(ngModel)]="config.config.secretKey"
placeholder="{{ 'settings.object-store.obs.secretKey.placeholder' | i18n }}"
placeholder="{{
config.type == ObjectStoreType.OBS
? ('settings.object-store.obs.secretKey.placeholder' | i18n)
: ('settings.object-store.oss.secretKey.placeholder' | i18n)
}}"
nz-input
required
name="secretKey"
Expand All @@ -66,14 +75,18 @@
/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRY principle: There's significant duplication in the HTML template for handling OBS and OSS configurations. Consider refactoring to use a common template or component for both cloud storage types to improve maintainability.

</nz-form-control>
</nz-form-item>
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS">
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS || config.type == ObjectStoreType.OSS">
<nz-form-label [nzSpan]="6" nzFor="obs.bucketName" nzRequired="true">{{
'settings.object-store.obs.bucketName' | i18n
}}</nz-form-label>
<nz-form-control [nzErrorTip]="'validation.required' | i18n">
<input
[(ngModel)]="config.config.bucketName"
placeholder="{{ 'settings.object-store.obs.bucketName.placeholder' | i18n }}"
placeholder="{{
config.type == ObjectStoreType.OBS
? ('settings.object-store.obs.bucketName.placeholder' | i18n)
: ('settings.object-store.oss.bucketName.placeholder' | i18n)
}}"
nz-input
required
name="bucketName"
Expand All @@ -82,14 +95,18 @@
/>
</nz-form-control>
</nz-form-item>
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS">
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS || config.type == ObjectStoreType.OSS">
<nz-form-label [nzSpan]="6" nzFor="obs.endpoint" nzRequired="true">{{
'settings.object-store.obs.endpoint' | i18n
}}</nz-form-label>
<nz-form-control [nzErrorTip]="'validation.required' | i18n">
<input
[(ngModel)]="config.config.endpoint"
placeholder="{{ 'settings.object-store.obs.endpoint.placeholder' | i18n }}"
placeholder="{{
config.type == ObjectStoreType.OBS
? ('settings.object-store.obs.endpoint.placeholder' | i18n)
: ('settings.object-store.oss.endpoint.placeholder' | i18n)
}}"
nz-input
required
name="endpoint"
Expand All @@ -98,14 +115,18 @@
/>
</nz-form-control>
</nz-form-item>
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS">
<nz-form-item *ngIf="config.type == ObjectStoreType.OBS || config.type == ObjectStoreType.OSS">
<nz-form-label [nzSpan]="6" nzFor="obs.savePath" nzRequired="true">{{
'settings.object-store.obs.savePath' | i18n
}}</nz-form-label>
<nz-form-control [nzErrorTip]="'validation.required' | i18n">
<input
[(ngModel)]="config.config.savePath"
placeholder="{{ 'settings.object-store.obs.savePath.placeholder' | i18n }}"
placeholder="{{
config.type == ObjectStoreType.OBS
? ('settings.object-store.obs.savePath.placeholder' | i18n)
: ('settings.object-store.oss.savePath.placeholder' | i18n)
}}"
nz-input
required
name="savePath"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { ChangeDetectorRef, Component, Inject, OnInit } from '@angular/core';
import { NzNotificationService } from 'ng-zorro-antd/notification';
import { finalize } from 'rxjs/operators';

import { ObjectStore, ObjectStoreType, ObsConfig } from '../../../../pojo/ObjectStore';
import { ObjectStore, ObjectStoreType, ObsConfig, OssConfig } from '../../../../pojo/ObjectStore';
import { GeneralConfigService } from '../../../../service/general-config.service';

const key = 'oss';
Expand Down Expand Up @@ -112,6 +112,9 @@ export class ObjectStoreComponent implements OnInit {
case ObjectStoreType.OBS:
this.config.config = new ObsConfig();
break;
case ObjectStoreType.OSS:
this.config.config = new OssConfig();
break;
}
};

Expand Down
11 changes: 11 additions & 0 deletions web-app/src/assets/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,7 @@
"settings.object-store.type": "File Server Provider",
"settings.object-store.type.file": "Local file (default)",
"settings.object-store.type.obs": "HUAWEI CLOUD OBS",
"settings.object-store.type.oss": "ALIBABA CLOUD OSS",
"settings.object-store.obs.accessKey": "AccessKey",
"settings.object-store.obs.accessKey.placeholder": "Access Key ID of HUAWEI CLOUD",
"settings.object-store.obs.secretKey": "SecretKey",
Expand All @@ -561,6 +562,16 @@
"settings.object-store.obs.endpoint.placeholder": "HUAWEI CLOUD OBS domain name, excluding the bucket name",
"settings.object-store.obs.savePath": "SavePath",
"settings.object-store.obs.savePath.placeholder": "Path to save the backup file, The default value is hertzbeat.",
"settings.object-store.oss.accessKey": "AccessKey",
"settings.object-store.oss.accessKey.placeholder": "Access Key ID of ALIBABA CLOUD",
"settings.object-store.oss.secretKey": "SecretKey",
"settings.object-store.oss.secretKey.placeholder": "Access Key Secret of ALIBABA CLOUD",
"settings.object-store.oss.bucketName": "Bucket",
"settings.object-store.oss.bucketName.placeholder": "The name of the bucket you created in ALIBABA CLOUD OSS",
"settings.object-store.oss.endpoint": "EndPoint",
"settings.object-store.oss.endpoint.placeholder": "ALIBABA CLOUD OSS domain name, excluding the bucket name",
"settings.object-store.oss.savePath": "SavePath",
"settings.object-store.oss.savePath.placeholder": "Path to save the backup file, The default value is hertzbeat.",
"collector": "Collector",
"collector.name": "Collector Name",
"collector.name.placeholder": "Please config the unique name of the collector",
Expand Down
11 changes: 11 additions & 0 deletions web-app/src/assets/i18n/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,7 @@
"settings.object-store.type": "文件服务提供商",
"settings.object-store.type.file": "本地文件(默认)",
"settings.object-store.type.obs": "华为云OBS",
"settings.object-store.type.oss": "阿里云OSS",
"settings.object-store.obs.accessKey": "AccessKey",
"settings.object-store.obs.accessKey.placeholder": "华为云的AccessKeyId",
"settings.object-store.obs.secretKey": "SecretKey",
Expand All @@ -562,6 +563,16 @@
"settings.object-store.obs.endpoint.placeholder": "华为云OBS地域域名,不包括Bucket名",
"settings.object-store.obs.savePath": "保存路径",
"settings.object-store.obs.savePath.placeholder": "备份文件保存路径, 默认是hertzbeat",
"settings.object-store.oss.accessKey": "AccessKey",
"settings.object-store.oss.accessKey.placeholder": "阿里云的AccessKeyId",
"settings.object-store.oss.secretKey": "SecretKey",
"settings.object-store.oss.secretKey.placeholder": "阿里云的AccessKeySecret",
"settings.object-store.oss.bucketName": "Bucket",
"settings.object-store.oss.bucketName.placeholder": "阿里云OSS中您创建的Bucket名称",
"settings.object-store.oss.endpoint": "EndPoint",
"settings.object-store.oss.endpoint.placeholder": "阿里云OSS地域域名,不包括Bucket名",
"settings.object-store.oss.savePath": "保存路径",
"settings.object-store.oss.savePath.placeholder": "备份文件保存路径, 默认是hertzbeat",
"collector": "采集器",
"collector.name": "采集器名称",
"collector.name.placeholder": "请配置待部署的采集器名称,注意需保证唯一性",
Expand Down
Loading
Loading