Skip to content
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

[OSPP]Support Kubernetes ConfigMap for Apollo java, golang client #79

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions apollo-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,10 @@
<scope>test</scope>
</dependency>
<!-- end of test -->
<dependency>
<groupId>io.kubernetes</groupId>
<artifactId>client-java</artifactId>
<version>18.0.0</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

  1. Manage the version in the parent POM.
  2. I'm considering marking the dependency as optional, as it's not required in every case, similar to the spring dependencies mentioned above.

</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/*
* Copyright 2022 Apollo Authors
*
* Licensed 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 com.ctrip.framework.apollo.Kubernetes;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use lowercase package naming convention.

Java package names should be in lowercase to avoid conflicts with class names and to follow standard conventions. Please rename the package com.ctrip.framework.apollo.Kubernetes to com.ctrip.framework.apollo.kubernetes.

Apply this change:

-package com.ctrip.framework.apollo.Kubernetes;
+package com.ctrip.framework.apollo.kubernetes;
📝 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
package com.ctrip.framework.apollo.Kubernetes;
package com.ctrip.framework.apollo.kubernetes;

Copy link
Member

Choose a reason for hiding this comment

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

package name should use lowercase

Copy link
Contributor

Choose a reason for hiding this comment

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

@shoothzj Thank you for acknowledging the package naming convention.

((\ /)/
(='.'=)
(_(")("))


import io.kubernetes.client.openapi.ApiClient;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.models.*;
import io.kubernetes.client.util.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;

import javax.annotation.PostConstruct;
import java.util.Map;

@Service
public class KubernetesManager {
private ApiClient client;
private CoreV1Api coreV1Api;

private final Logger log = LoggerFactory.getLogger(this.getClass());

@PostConstruct
public void initClient() {
try {
client = Config.defaultClient();
Configuration.setDefaultApiClient(client);
coreV1Api = new CoreV1Api(client);

} catch (Exception e) {
throw new RuntimeException("k8s client init failed");
}
}

public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) {
if (configMapNamespace == null || configMapNamespace == "" || name == null || name == "") {
log.error("create config map failed due to null parameter");
return null;
}
V1ConfigMap configMap = new V1ConfigMap().metadata(new V1ObjectMeta().name(name).namespace(configMapNamespace)).data(data);
try {
coreV1Api.createNamespacedConfigMap(configMapNamespace, configMap, null, null, null,null);
return name;
} catch (Exception e) {
log.error("create config map failed", e);
return null;
}
}

public String loadFromConfigMap(String configMapNamespace, String name) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || name == null || name.isEmpty()) {
log.error("参数不能为空");
return null;
}
try {
V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null);
if (configMap == null) {
log.error("ConfigMap不存在");
return null;
}
Map<String, String> data = configMap.getData();
if (data != null && data.containsKey(name)) {
return data.get(name);
} else {
log.error("在ConfigMap中未找到指定的键: " + name);
return null;
}
} catch (Exception e) {
log.error("get config map failed", e);
return null;
}
}

public String getValueFromConfigMap(String configMapNamespace, String name, String key) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) {
log.error("参数不能为空");
return null;
}
try {
V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null);
if (configMap == null || configMap.getData() == null) {
log.error("ConfigMap不存在或没有数据");
return null;
}
if (!configMap.getData().containsKey(key)) {
log.error("在ConfigMap中未找到指定的键: " + key);
return null;
}
return configMap.getData().get(key);
} catch (Exception e) {
log.error("get config map failed", e);
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid suppressing exceptions and returning null.

In getValueFromConfigMap, returning null when an exception occurs can conceal underlying issues. It's better to handle the exception appropriately or propagate it.

Consider rethrowing the exception or handling it based on the specific case:

-} catch (Exception e) {
-    return null;
+} catch (ApiException e) {
+    log.error("Failed to get value from ConfigMap", e);
+    throw new RuntimeException("Failed to get value from ConfigMap: " + e.getResponseBody(), e);

This ensures that calling methods are aware that an error has occurred.

Committable suggestion was skipped due to low confidence.

}

public String updateConfigMap(String configMapNamespace, String name, Map<String, String> data) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || data == null || data.isEmpty()) {
log.error("参数不能为空");
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure consistent error handling.

Returning null on failure in updateConfigMap may lead to NullPointerException in calling methods. Instead, consider throwing an exception or returning a boolean to indicate success.

Modify the method signature and return type:

-public String updateConfigMap(String configMapNamespace, String name, Map<String, String> data) {
+public boolean updateConfigMap(String configMapNamespace, String name, Map<String, String> data) {

Update the return statements accordingly:

-return name;
+return true;

In the catch block:

-return null;
+throw new RuntimeException("Failed to update ConfigMap: " + e.getMessage(), e);

This approach provides clearer feedback to the caller about the operation's success.

Committable suggestion was skipped due to low confidence.

try {
V1ConfigMap configMap = new V1ConfigMap().metadata(new V1ObjectMeta().name(name).namespace(configMapNamespace)).data(data);
coreV1Api.replaceNamespacedConfigMap(name, configMapNamespace, configMap, null, null, null, "fieldManagerValue");
return name;
} catch (Exception e) {
log.error("update config map failed", e);
return null;
}
}

public boolean checkConfigMapExist(String configMapNamespace, String configMapName) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || configMapName == null || configMapName.isEmpty()) {
log.error("参数不能为空");
return false;
}
try {
coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null);
return true;
} catch (Exception e) {
log.error("check config map failed", e);
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize existence check for ConfigMap.

Using readNamespacedConfigMap to check existence may be inefficient and can generate unnecessary error logs on exceptions. Instead, use listNamespacedConfigMap with field selectors for a cleaner check.

Replace the method's try-catch block with:

try {
    V1ConfigMapList configMapList = coreV1Api.listNamespacedConfigMap(
        configMapNamespace,
        null,
        null,
        null,
        "metadata.name=" + configMapName,
        null,
        null,
        null,
        null,
        null,
        null
    );
    return !configMapList.getItems().isEmpty();
} catch (ApiException e) {
    log.error("Failed to check ConfigMap existence", e);
    throw new RuntimeException("Failed to check ConfigMap existence: " + e.getResponseBody(), e);
}

This method avoids exceptions for non-existent ConfigMaps and doesn't log errors in normal operation.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
* @since 1.1.0
*/
public enum ConfigSourceType {
REMOTE("Loaded from remote config service"), LOCAL("Loaded from local cache"), NONE("Load failed");
REMOTE("Loaded from remote config service"),
LOCAL("Loaded from local cache"),
CONFIGMAP("Loaded from k8s config map"),
NONE("Load failed");

private final String description;

Expand Down
Loading
Loading