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

Merged
merged 27 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 13 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
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ Release Notes.
Apollo Java 2.4.0

------------------

* [Feature Support Kubernetes ConfigMap cache for Apollo java, golang client](https://github.com/apolloconfig/apollo-java/pull/79)
Copy link
Member

Choose a reason for hiding this comment

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

Add it to the last

* [Fix the Cannot enhance @Configuration bean definition issue](https://github.com/apolloconfig/apollo-java/pull/82)
* [Feature openapi query namespace support not fill item](https://github.com/apolloconfig/apollo-java/pull/83)
* [Add more observability in apollo config client](https://github.com/apolloconfig/apollo-java/pull/74)
Expand Down
4 changes: 4 additions & 0 deletions apollo-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@
<artifactId>spring-boot-configuration-processor</artifactId>
<optional>true</optional>
</dependency>
<dependency>
<groupId>io.kubernetes</groupId>
<artifactId>client-java</artifactId>
dyx1234 marked this conversation as resolved.
Show resolved Hide resolved
</dependency>
<!-- test -->
<dependency>
<groupId>org.eclipse.jetty</groupId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
/*
* 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.ApiException;
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 java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;

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

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

public KubernetesManager() {
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);
}
}

public KubernetesManager(CoreV1Api coreV1Api) {
this.coreV1Api = coreV1Api;
}

public V1ConfigMap buildConfigMap(String name, String namespace, Map<String, String> data) {
V1ObjectMeta metadata = new V1ObjectMeta()
.name(name)
.namespace(namespace);

return new V1ConfigMap()
.apiVersion("v1")
.kind("ConfigMap")
.metadata(metadata)
.data(data);
}

/**
* 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 configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name);
}
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

Ensure input validation halts execution upon failure

The input validation checks for null or empty parameters log an error but do not halt execution. This can lead to exceptions or unintended behavior later in the code. Consider throwing an exception to prevent the methods from proceeding with invalid input.

Apply the following changes to the methods:

  • In createConfigMap:

    if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
        log.error("create configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name);
    +   throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
    }
  • In loadFromConfigMap:

    if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty()) {
        log.error("load configmap failed due to null or empty parameter: configMapNamespace={}, name={}", configMapNamespace, name);
    +   throw new IllegalArgumentException("ConfigMap namespace and name cannot be null or empty");
    }
  • In getValueFromConfigMap:

    if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) {
        log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
    +   throw new IllegalArgumentException("ConfigMap namespace, name, and key cannot be null or empty");
    }
  • In updateConfigMap:

    if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || data == null || data.isEmpty()) {
        log.error("Parameters cannot be null or empty: configMapNamespace={}, name={}", configMapNamespace, name);
    +   return false;
    }

Also applies to: 99-101, 129-131, 154-156

V1ConfigMap configMap = buildConfigMap(name, configMapNamespace, data);
try {
coreV1Api.createNamespacedConfigMap(configMapNamespace, configMap, null, null, null, null);
log.info("ConfigMap created successfully: name: {}, namespace: {}", name, configMapNamespace);
return name;
} catch (Exception e) {
throw new RuntimeException("Failed to create ConfigMap: " + e.getMessage(), e);
}
}

/**
* 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 (!Objects.requireNonNull(configMap.getData()).containsKey(key)) {
throw new RuntimeException(String.format("Specified key not found in ConfigMap: %s, configMapNamespace: %s, name: %s", name, configMapNamespace, name));
}
return configMap.getData().get(key);
} catch (Exception 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.

}

/**
* update config map
*
* @param configMapNamespace configmap namespace
* @param name config map name (appId)
* @param data new data
* @return config map name
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

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 tag to reflect the correct return type:

-  * @return config map name
+  * @return true if the update is successful, false otherwise
📝 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
* @return config map name
* @return true if the update is successful, false otherwise

*/
// TODO 使用client自带的retry机制,设置重试次数,CAS
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

Address the TODO: Use client's built-in retry mechanism

The TODO comment indicates that the client's built-in retry mechanism should be used instead of custom retry logic. Implementing this can simplify the code and adhere to best practices.

Would you like assistance in implementing the client's built-in retry mechanism for updating the ConfigMap? This can improve reliability and reduce code complexity.

public boolean 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 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

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.


// retry
int maxRetries = 5;
int retryCount = 0;
long waitTime = 100;

while (retryCount < maxRetries) {
try {
V1ConfigMap configmap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null);
configmap.setData(data);
coreV1Api.replaceNamespacedConfigMap(name, configMapNamespace, configmap, null, null, null, null);
return true;
} catch (ApiException e) {
if (e.getCode() == 409) {
retryCount++;
log.warn("Conflict occurred, retrying... (" + retryCount + ")");
try {
TimeUnit.MILLISECONDS.sleep(waitTime);
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
waitTime = Math.min(waitTime * 2, 1000);
} else {
System.err.println("Error updating ConfigMap: " + e.getMessage());
}
}
}
return retryCount < maxRetries;
}

/**
* 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 {
log.info("Check whether ConfigMap exists, configMapName: {}", configMapName);
coreV1Api.readNamespacedConfigMap(configMapName, configMapNamespace, null);
return true;
} catch (Exception e) {
// configmap not exist
return false;
}
}
}
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