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 17 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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,301 @@
/*
* 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.internals;

import com.ctrip.framework.apollo.kubernetes.KubernetesManager;
import com.ctrip.framework.apollo.build.ApolloInjector;
import com.ctrip.framework.apollo.core.ConfigConsts;
import com.ctrip.framework.apollo.core.utils.DeferredLoggerFactory;
import com.ctrip.framework.apollo.core.utils.StringUtils;
import com.ctrip.framework.apollo.enums.ConfigSourceType;
import com.ctrip.framework.apollo.exceptions.ApolloConfigException;
import com.ctrip.framework.apollo.tracer.Tracer;
import com.ctrip.framework.apollo.tracer.spi.Transaction;
import com.ctrip.framework.apollo.util.ConfigUtil;
import com.ctrip.framework.apollo.util.ExceptionUtil;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
import org.slf4j.Logger;

import java.lang.reflect.Type;
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;

/**
* @author dyx1234
*/
public class K8sConfigMapConfigRepository extends AbstractConfigRepository
implements RepositoryChangeListener {
private static final Logger logger = DeferredLoggerFactory.getLogger(K8sConfigMapConfigRepository.class);
private final String namespace;
private String configMapName;
private String configMapKey;
private final String k8sNamespace;
private final ConfigUtil configUtil;
private final KubernetesManager kubernetesManager;
private volatile Properties configMapProperties;
private volatile ConfigRepository upstream;
private volatile ConfigSourceType sourceType = ConfigSourceType.CONFIGMAP;

/**
* Constructor
*
* @param namespace the namespace
*/
public K8sConfigMapConfigRepository(String namespace) {
Copy link
Member

Choose a reason for hiding this comment

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

This constructor seems unnecessary.

this(namespace, (ConfigRepository) null);
}

public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
Copy link
Member

Choose a reason for hiding this comment

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

It's not a practice to add a constructor only for test scenario. You could use MockInjector.setInstance method to inject the mocked KubernetesManager instance.

this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
k8sNamespace = configUtil.getK8sNamespace();
this.kubernetesManager = kubernetesManager;

this.setConfigMapKey(configUtil.getCluster(), namespace);
this.setConfigMapName(configUtil.getAppId(), false);
this.setUpstreamRepository(upstream);
dyx1234 marked this conversation as resolved.
Show resolved Hide resolved
}

public K8sConfigMapConfigRepository(String namespace, ConfigRepository upstream) {
this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class);
k8sNamespace = configUtil.getK8sNamespace();

this.setConfigMapKey(configUtil.getCluster(), namespace);
this.setConfigMapName(configUtil.getAppId(), false);
this.setUpstreamRepository(upstream);
}

public String getConfigMapKey() {
Copy link
Member

Choose a reason for hiding this comment

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

This method is not used.

return configMapKey;
}

public String getConfigMapName() {
Copy link
Member

Choose a reason for hiding this comment

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

We could make it package private as it's only used in the unit test.

Suggested change
public String getConfigMapName() {
String getConfigMapName() {

return configMapName;
}

void setConfigMapKey(String cluster, String namespace) {
// cluster: 用户定义>idc>default
Copy link
Member

Choose a reason for hiding this comment

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

Use English.

if (StringUtils.isBlank(cluster)) {
configMapKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join("default", namespace);
return;
}
configMapKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(cluster, namespace);
}

void setConfigMapName(String appId, boolean syncImmediately) {
Preconditions.checkNotNull(appId, "AppId cannot be null");
configMapName = ConfigConsts.APOLLO_CONFIG_CACHE + appId;
// 初始化configmap
Copy link
Member

Choose a reason for hiding this comment

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

Use English.

this.checkConfigMapName(configMapName);
if (syncImmediately) {
this.sync();
}
}

private void checkConfigMapName(String configMapName) {
if (StringUtils.isBlank(configMapName)) {
throw new IllegalArgumentException("ConfigMap name cannot be null");
}
if (kubernetesManager.checkConfigMapExist(k8sNamespace, configMapName)) {
return;
}
// Create an empty configmap, write the new value in the update event
Transaction transaction = Tracer.newTransaction("Apollo.ConfigService", "createK8sConfigMap");
transaction.addData("configMapName", configMapName);
try {
kubernetesManager.createConfigMap(k8sNamespace, configMapName, null);
transaction.setStatus(Transaction.SUCCESS);
} catch (Throwable ex) {
Tracer.logEvent("ApolloConfigException", ExceptionUtil.getDetailMessage(ex));
transaction.setStatus(ex);
throw new ApolloConfigException("Create configmap failed!", ex);
} finally {
transaction.complete();
}
}

@Override
public Properties getConfig() {
if (configMapProperties == null) {
sync();
}
Properties result = propertiesFactory.getPropertiesInstance();
result.putAll(configMapProperties);
logger.info("configmap properties: {}", configMapProperties);
Copy link
Member

Choose a reason for hiding this comment

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

It's not a common practice to log the configurations.

Suggested change
logger.info("configmap properties: {}", configMapProperties);

return result;
}

/**
* Update the memory when the configuration center changes
*
* @param upstreamConfigRepository the upstream repo
*/
@Override
public void setUpstreamRepository(ConfigRepository upstreamConfigRepository) {
// 设置上游数据源
Copy link
Member

Choose a reason for hiding this comment

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

Use english comment.

if (upstreamConfigRepository == null) {
return;
}
//clear previous listener
if (upstream != null) {
upstream.removeChangeListener(this);
}
dyx1234 marked this conversation as resolved.
Show resolved Hide resolved
upstream = upstreamConfigRepository;
upstreamConfigRepository.addChangeListener(this);
}

@Override
public ConfigSourceType getSourceType() {
return sourceType;
}

/**
* Sync the configmap
*/
@Override
protected void sync() {
// Chain recovery, first read from upstream data source
boolean syncFromUpstreamResultSuccess = trySyncFromUpstream();

if (syncFromUpstreamResultSuccess) {
return;
}

Transaction transaction = Tracer.newTransaction("Apollo.ConfigService", "syncK8sConfigMap");
Throwable exception = null;
try {
configMapProperties = loadFromK8sConfigMap();
sourceType = ConfigSourceType.CONFIGMAP;
transaction.setStatus(Transaction.SUCCESS);
} catch (Throwable ex) {
Tracer.logEvent("ApolloConfigException", ExceptionUtil.getDetailMessage(ex));
transaction.setStatus(ex);
exception = ex;
throw new ApolloConfigException("Load config from Kubernetes ConfigMap failed!", ex);
Copy link
Member

Choose a reason for hiding this comment

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

This logic seems flawed. If this line executes, then line 193 and lines 199 to 203 are unnecessary.

} finally {
transaction.complete();
}

if (configMapProperties == null) {
sourceType = ConfigSourceType.NONE;
throw new ApolloConfigException(
"Load config from Kubernetes ConfigMap failed!", exception);
}
}

public Properties loadFromK8sConfigMap() {
Preconditions.checkNotNull(configMapName, "ConfigMap name cannot be null");

try {
String jsonConfig = kubernetesManager.getValueFromConfigMap(k8sNamespace, configMapName, configMapKey);
if (jsonConfig == null) {
// TODO 这样重试访问idc,default是否正确
Copy link
Member

Choose a reason for hiding this comment

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

String retryKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join("default", namespace);
jsonConfig = kubernetesManager.getValueFromConfigMap(k8sNamespace, configMapName, retryKey);
}
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

Address the TODO comment regarding retry logic

The code at line 212 contains a TODO comment: "这样重试访问idc,default是否正确" (Is this way of retrying access to idc, default correct?). Please review and confirm if the retry logic when accessing the default cluster is correct.

Would you like assistance in reviewing or improving the retry mechanism?


// Convert jsonConfig to properties
Properties properties = propertiesFactory.getPropertiesInstance();
if (jsonConfig != null && !jsonConfig.isEmpty()) {
Gson gson = new Gson();
Type type = new TypeToken<Map<String, String>>() {}.getType();
Map<String, String> configMap = gson.fromJson(jsonConfig, type);
configMap.forEach(properties::setProperty);
}
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

Reuse Gson instance to improve performance.

Creating multiple Gson instances can be inefficient. Consider defining a single Gson instance at the class level to reuse across methods. This enhances performance and reduces memory footprint.

Apply this diff:

+ private static final Gson GSON = new Gson();

...

public Properties loadFromK8sConfigMap() {
    ...
-   Gson gson = new Gson();
    Type type = new TypeToken<Map<String, String>>() {}.getType();
-   Map<String, String> configMap = gson.fromJson(jsonConfig, type);
+   Map<String, String> configMap = GSON.fromJson(jsonConfig, type);
    ...
}

public void persistConfigMap(Properties properties) {
    ...
-   Gson gson = new Gson();
-   String jsonConfig = gson.toJson(properties);
+   String jsonConfig = GSON.toJson(properties);
    ...
}

Also applies to: 253-254

return properties;
} catch (Exception ex) {
Tracer.logError(ex);
throw new ApolloConfigException(String
.format("Load config from Kubernetes ConfigMap %s failed!", configMapName), ex);
}
}

public boolean trySyncFromUpstream() {
if (upstream == null) {
return false;
}
try {
logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType());
Copy link
Member

Choose a reason for hiding this comment

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

It's not a common practice to log the configurations.

Suggested change
logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType());

updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType());
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(),
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

Handle null properties returned from upstream

In trySyncFromUpstream, if upstream.getConfig() returns null, passing it to updateConfigMapProperties can cause issues. Add a null check to handle this scenario.

Apply this diff to check for null properties:

try {
    Properties upstreamProperties = upstream.getConfig();
    if (upstreamProperties == null) {
        logger.warn("Upstream returned null properties.");
        return false;
    }
    logger.info("Start sync from the upstream data source, upstream.getSourceType(): {}", upstream.getSourceType());
    updateConfigMapProperties(upstreamProperties, upstream.getSourceType());
    return true;
} catch (Throwable ex) {
    Tracer.logError(ex);
    logger.warn("Sync config from upstream repository {} failed, reason: {}",
            upstream.getClass(), ExceptionUtil.getDetailMessage(ex));
}
📝 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
try {
logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType());
updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType());
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(),
try {
Properties upstreamProperties = upstream.getConfig();
if (upstreamProperties == null) {
logger.warn("Upstream returned null properties.");
return false;
}
logger.info("Start sync from the upstream data source, upstream.getSourceType(): {}", upstream.getSourceType());
updateConfigMapProperties(upstreamProperties, upstream.getSourceType());
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}",
upstream.getClass(), ExceptionUtil.getDetailMessage(ex));

ExceptionUtil.getDetailMessage(ex));
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 logging sensitive configuration properties and optimize method calls

At line 238, the logger.info statement logs upstream.getConfig(), which may contain sensitive configuration data. Logging these properties can expose sensitive information in logs. Additionally, upstream.getConfig() and upstream.getSourceType() are called multiple times. To prevent logging sensitive data and to optimize method calls, assign the results to local variables and adjust the log statement accordingly.

Apply this diff to fix the issue:

public boolean trySyncFromUpstream() {
    if (upstream == null) {
        return false;
    }
    try {
+       Properties upstreamProperties = upstream.getConfig();
+       ConfigSourceType upstreamSourceType = upstream.getSourceType();
+       logger.info("Start sync from the upstream data source, sourceType: {}", upstreamSourceType);
+       updateConfigMapProperties(upstreamProperties, upstreamSourceType);
-       logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType());
-       updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType());
        return true;
    } catch (Throwable ex) {
        Tracer.logError(ex);
        logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(),
                ExceptionUtil.getDetailMessage(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
logger.info("Start sync from the upstream data source, upstream.getConfig:{}, upstream.getSourceType():{}", upstream.getConfig(), upstream.getSourceType());
updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType());
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(),
ExceptionUtil.getDetailMessage(ex));
Properties upstreamProperties = upstream.getConfig();
ConfigSourceType upstreamSourceType = upstream.getSourceType();
logger.info("Start sync from the upstream data source, sourceType: {}", upstreamSourceType);
updateConfigMapProperties(upstreamProperties, upstreamSourceType);
return true;
} catch (Throwable ex) {
Tracer.logError(ex);
logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(),
ExceptionUtil.getDetailMessage(ex));

}
return false;
}

private synchronized void updateConfigMapProperties(Properties newProperties, ConfigSourceType sourceType) {
this.sourceType = sourceType;
if (newProperties.equals(configMapProperties)) {
return;
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

Prevent NullPointerException when comparing properties

In updateConfigMapProperties, comparing newProperties.equals(configMapProperties) without null checks can lead to a NullPointerException if newProperties is null.

Apply this diff to safely compare the properties:

private synchronized void updateConfigMapProperties(Properties newProperties, ConfigSourceType sourceType) {
    this.sourceType = sourceType;
+   if (newProperties == null || newProperties.equals(configMapProperties)) {
        return;
    }
    this.configMapProperties = newProperties;
    persistConfigMap(configMapProperties);
}
📝 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
if (newProperties.equals(configMapProperties)) {
return;
if (newProperties == null || newProperties.equals(configMapProperties)) {
return;

}
this.configMapProperties = newProperties;
persistConfigMap(configMapProperties);
}

/**
* Update the memory
*
* @param namespace the namespace of this repository change
* @param newProperties the properties after change
*/
@Override
public void onRepositoryChange(String namespace, Properties newProperties) {
if (newProperties.equals(configMapProperties)) {
return;
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

Add null check to avoid NullPointerException in onRepositoryChange

When newProperties is null, calling newProperties.equals(configMapProperties) will throw a NullPointerException. Ensure that newProperties is not null before comparison.

Apply this diff:

public void onRepositoryChange(String namespace, Properties newProperties) {
+   if (newProperties == null || newProperties.equals(configMapProperties)) {
        return;
    }
    Properties newFileProperties = propertiesFactory.getPropertiesInstance();
    newFileProperties.putAll(newProperties);
    updateConfigMapProperties(newFileProperties, upstream.getSourceType());
    this.fireRepositoryChange(namespace, newProperties);
}
📝 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
if (newProperties.equals(configMapProperties)) {
return;
if (newProperties == null || newProperties.equals(configMapProperties)) {
return;

}
Properties newFileProperties = propertiesFactory.getPropertiesInstance();
newFileProperties.putAll(newProperties);
updateConfigMapProperties(newFileProperties, upstream.getSourceType());
this.fireRepositoryChange(namespace, newProperties);
}

public void persistConfigMap(Properties properties) {
Transaction transaction = Tracer.newTransaction("Apollo.ConfigService", "persistK8sConfigMap");
transaction.addData("configMapName", configMapName);
transaction.addData("k8sNamespace", k8sNamespace);
try {
// Convert properties to a JSON string using Gson
Gson gson = new Gson();
String jsonConfig = gson.toJson(properties);
Map<String, String> data = new HashMap<>();
data.put(configMapKey, jsonConfig);

// update configmap
kubernetesManager.updateConfigMap(k8sNamespace, configMapName, data);
transaction.setStatus(Transaction.SUCCESS);
} catch (Exception ex) {
ApolloConfigException exception =
new ApolloConfigException(
String.format("Persist config to Kubernetes ConfigMap %s failed!", configMapName), ex);
Tracer.logError(exception);
transaction.setStatus(exception);
logger.error("Persist config to Kubernetes ConfigMap failed!", exception);
} finally {
transaction.complete();
}
}

}
Loading
Loading