-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
82f7356
e14fd00
562b325
8705ba1
d8c51dd
435df9d
e2c7cd7
73581e9
c52337c
3a22a38
80f6c59
65ce65a
88b1d75
c3fc604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,176 @@ | ||||||||||||||
/* | ||||||||||||||
* 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; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Apply this change: -package com.ctrip.framework.apollo.Kubernetes;
+package com.ctrip.framework.apollo.kubernetes; 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. package name should use lowercase There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||
String errorMessage = "Failed to initialize Kubernetes client: " + e.getMessage(); | ||||||||||||||
log.error(errorMessage, e); | ||||||||||||||
throw new RuntimeException(errorMessage, e); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Creates a Kubernetes ConfigMap. | ||||||||||||||
* | ||||||||||||||
* @param configMapNamespace the namespace of the ConfigMap | ||||||||||||||
* @param name the name of the ConfigMap | ||||||||||||||
* @param data the data to be stored in the ConfigMap | ||||||||||||||
* @return the name of the created ConfigMap | ||||||||||||||
* @throws RuntimeException if an error occurs while creating the ConfigMap | ||||||||||||||
*/ | ||||||||||||||
public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) { | ||||||||||||||
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) { | ||||||||||||||
log.error("create config map failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name); | ||||||||||||||
throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty"); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Validate input parameters consistently across methods. The input validation logic is duplicated in multiple methods. Consider creating a private helper method to validate parameters to reduce code duplication and potential inconsistencies. Create a helper method for parameter validation: private void validateParams(String configMapNamespace, String name) {
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
}
} Then, update your methods: -public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) {
- if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
- log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
- throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
- }
+public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) {
+ validateParams(configMapNamespace, name); Repeat this change for other methods where parameters are validated. |
||||||||||||||
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) { | ||||||||||||||
throw new RuntimeException("Failed to create ConfigMap: " + e.getMessage(), e); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle specific Kubernetes API exceptions. Catching generic Update the catch block to handle -} catch (Exception e) {
+} catch (ApiException e) { Additionally, you can handle different HTTP status codes within
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* get value from config map | ||||||||||||||
* | ||||||||||||||
* @param configMapNamespace | ||||||||||||||
* @param name config map name (appId) | ||||||||||||||
* @return configMap data(all key-value pairs in config map) | ||||||||||||||
*/ | ||||||||||||||
public String loadFromConfigMap(String configMapNamespace, String name) { | ||||||||||||||
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() ) { | ||||||||||||||
String errorMessage = String.format("Parameters can not be null or empty, configMapNamespace: '%s', name: '%s'", configMapNamespace, name); | ||||||||||||||
log.error(errorMessage); | ||||||||||||||
throw new IllegalArgumentException(errorMessage); | ||||||||||||||
} | ||||||||||||||
try { | ||||||||||||||
log.info("Starting to read ConfigMap: " + name); | ||||||||||||||
V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | ||||||||||||||
if (configMap == null) { | ||||||||||||||
throw new RuntimeException(String.format("ConfigMap does not exist, configMapNamespace: %s, name: %s", configMapNamespace, name)); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unnecessary null check for The Remove the null check: -V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null);
-if (configMap == null) {
- throw new RuntimeException(String.format("ConfigMap does not exist, configMapNamespace: %s, name: %s", configMapNamespace, name));
-} Handle
|
||||||||||||||
Map<String, String> data = configMap.getData(); | ||||||||||||||
if (data != null && data.containsKey(name)) { | ||||||||||||||
return data.get(name); | ||||||||||||||
} else { | ||||||||||||||
throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct the key used to retrieve data from ConfigMap. In If you intend to return all data as a JSON string: -if (data != null && data.containsKey(name)) {
- return data.get(name);
-} else {
- throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
-}
+if (data != null) {
+ return new ObjectMapper().writeValueAsString(data);
+} else {
+ throw new RuntimeException(String.format("No data found in ConfigMap: %s, configMapNamespace: %s", name, configMapNamespace));
+} Ensure you handle JSON conversion and include necessary imports for
|
||||||||||||||
} catch (Exception e) { | ||||||||||||||
throw new RuntimeException(String | ||||||||||||||
.format("get config map failed, configMapNamespace: %s, name: %s", configMapNamespace, name)); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* get value from config map | ||||||||||||||
* | ||||||||||||||
* @param configMapNamespace configMapNamespace | ||||||||||||||
* @param name config map name (appId) | ||||||||||||||
* @param key config map key (cluster+namespace) | ||||||||||||||
* @return value(json string) | ||||||||||||||
*/ | ||||||||||||||
public String getValueFromConfigMap(String configMapNamespace, String name, String key) { | ||||||||||||||
if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) { | ||||||||||||||
log.error("Parameters can not be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); | ||||||||||||||
return null; | ||||||||||||||
} | ||||||||||||||
try { | ||||||||||||||
V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); | ||||||||||||||
if (configMap == null) { | ||||||||||||||
throw new RuntimeException(String.format("ConfigMap does not exist, configMapNamespace: %s, name: %s", configMapNamespace, name)); | ||||||||||||||
} | ||||||||||||||
if (!configMap.getData().containsKey(key)) { | ||||||||||||||
throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name)); | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include the missing key in the exception message. In Update the exception message to include the missing key: -throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
+throw new RuntimeException(String.format("Specified key '%s' not found in ConfigMap '%s' in namespace '%s'", key, name, configMapNamespace)); 📝 Committable suggestion
Suggested change
|
||||||||||||||
return configMap.getData().get(key); | ||||||||||||||
} catch (Exception e) { | ||||||||||||||
return null; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid suppressing exceptions and returning null. In 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.
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* update config map | ||||||||||||||
* | ||||||||||||||
* @param configMapNamespace | ||||||||||||||
* @param name config map name (appId) | ||||||||||||||
* @param data new data | ||||||||||||||
* @return config map name | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update method documentation to reflect correct return type The Javadoc comment states that the method returns the config map name, but the method actually returns a boolean indicating success. Update the - * @return config map name
+ * @return true if the update is successful, false otherwise 📝 Committable suggestion
Suggested change
|
||||||||||||||
*/ | ||||||||||||||
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("Parameters can not be null or empty: configMapNamespace={}, name={}", configMapNamespace, name); | ||||||||||||||
return null; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure consistent error handling. Returning 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.
|
||||||||||||||
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; | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* check config map exist | ||||||||||||||
* | ||||||||||||||
* @param configMapNamespace config map namespace | ||||||||||||||
* @param configMapName config map name | ||||||||||||||
* @return true if config map exist, false otherwise | ||||||||||||||
*/ | ||||||||||||||
public boolean checkConfigMapExist(String configMapNamespace, String configMapName) { | ||||||||||||||
if (configMapNamespace == null || configMapNamespace.isEmpty() || configMapName == null || configMapName.isEmpty()) { | ||||||||||||||
log.error("Parameters can not be null or empty: configMapNamespace={}, configMapName={}", configMapNamespace, configMapName); | ||||||||||||||
return false; | ||||||||||||||
} | ||||||||||||||
try { | ||||||||||||||
coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null); | ||||||||||||||
return true; | ||||||||||||||
} catch (Exception e) { | ||||||||||||||
log.error("check config map failed", e); | ||||||||||||||
return false; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Optimize existence check for ConfigMap. Using 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. |
||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Standardize method return types and error handling. The Consider throwing an exception on failure and returning a boolean for actual existence: - return false;
+ throw new RuntimeException("Failed to check ConfigMap existence: " + e.getMessage(), e); Update the method's documentation to reflect this behavior.
|
||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional
, as it's not required in every case, similar to thespring
dependencies mentioned above.