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
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)
* [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>
</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,308 @@
/*
* 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 configMapNamespace;
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) {
this(namespace, (ConfigRepository) null);
}

public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class);
Comment on lines +66 to +69
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

Fix the assignment of the kubernetesManager field in the constructor

In the constructor, the parameter kubernetesManager is shadowing the class field kubernetesManager. As a result, the assignment at line 69 does not set the class field but instead reassigns the parameter. This means that the kubernetesManager field remains uninitialized, which can lead to NullPointerException when it is used later in the class.

Apply this diff to correct the assignment:

public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
    this.namespace = namespace;
    configUtil = ApolloInjector.getInstance(ConfigUtil.class);
-   kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class);
+   this.kubernetesManager = kubernetesManager != null ? kubernetesManager : ApolloInjector.getInstance(KubernetesManager.class);
    configMapNamespace = configUtil.getConfigMapNamespace();

    this.setConfigMapKey(configUtil.getCluster(), namespace);
    this.setConfigMapName(configUtil.getAppId(), false);
    // Ensure that the upstream repository is set appropriately if needed
}
📝 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
public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class);
public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) {
this.namespace = namespace;
configUtil = ApolloInjector.getInstance(ConfigUtil.class);
this.kubernetesManager = kubernetesManager != null ? kubernetesManager : ApolloInjector.getInstance(KubernetesManager.class);

configMapNamespace = configUtil.getConfigMapNamespace();
this.kubernetesManager = kubernetesManager;

this.setConfigMapKey(configUtil.getCluster(), namespace);
this.setConfigMapName(configUtil.getAppId(), false);
this.setUpstreamRepository(upstream);
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

Initialize or pass the upstream repository in the constructor

In the constructor, this.setUpstreamRepository(upstream); is called, but the upstream variable is not initialized or passed as a parameter. This may lead to unexpected behavior if upstream is null or not intended.

Consider passing upstream as a parameter to the constructor or initializing it before calling setUpstreamRepository(upstream);.

}

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

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

void setConfigMapKey(String cluster, String namespace) {
// TODO 兜底key怎么设计不会冲突(cluster初始化时已经设置了层级)
// cluster: 用户定义>idc>default,所以已经不需要额外层级设置了
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);
}

public String getConfigMapKey() {
return configMapKey;
}

public String getConfigMapName() {
return configMapName;
}

void setConfigMapName(String appId, boolean syncImmediately) {
configMapName = appId;
// 初始化configmap
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(configMapNamespace, 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(configMapNamespace, 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();
}
}

/**
* TODO 测试点:
* 1. 从上游成功恢复(开启文件存储)
* 2. 从上游成功恢复(没开启文件存储,从remote)
* 3. 从k8s成功恢复
*/
@Override
public Properties getConfig() {
if (configMapProperties == null) {
sync();
}
Properties result = propertiesFactory.getPropertiesInstance();
result.putAll(configMapProperties);
logger.info("configmap值:{}", configMapProperties);
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 potentially sensitive configuration properties

Logging the entire configMapProperties object at INFO level may expose sensitive information in the logs. Consider removing this log statement or adjusting it to log only non-sensitive information.

Apply this diff to remove or modify the log statement:

- logger.info("configmap值:{}", configMapProperties);
+ // logger.info("Configuration properties loaded successfully.");
📝 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("configmap值:{}", configMapProperties);
// logger.info("Configuration properties loaded successfully.");

return result;
}

/**
* Update the memory when the configuration center changes
*
* @param upstreamConfigRepository the upstream repo
*/
@Override
public void setUpstreamRepository(ConfigRepository upstreamConfigRepository) {
// 设置上游数据源
if (upstreamConfigRepository == null) {
return;
}
//clear previous listener
if (upstream != null) {
upstream.removeChangeListener(this);
}
Comment on lines +163 to +169
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 previous listeners are removed when setting upstream to null

In the setUpstreamRepository method, if upstreamConfigRepository is null, the previous listener is not removed, which may lead to unintended behavior or memory leaks.

Apply this diff to always remove the previous listener:

public void setUpstreamRepository(ConfigRepository upstreamConfigRepository) {
    // Clear previous listener if any
    if (upstream != null) {
        upstream.removeChangeListener(this);
    }
    if (upstreamConfigRepository == null) {
        upstream = null;
        return;
    }
    upstream = upstreamConfigRepository;
    upstream.addChangeListener(this);
}

Committable suggestion was skipped due to low confidence.

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);
} 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(configMapNamespace, configUtil.getAppId(), configMapKey);
if (jsonConfig == null) {
// TODO 重试访问idc,default
String fallbackKey = Joiner.on(ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR).join(configUtil, namespace);
jsonConfig = kubernetesManager.getValueFromConfigMap(configMapNamespace, configMapName, fallbackKey);
}

// 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);
}
return properties;
} catch (Exception ex) {
Tracer.logError(ex);
throw new ApolloConfigException(String
.format("Load config from Kubernetes ConfigMap %s failed!", configMapName), ex);
}
}

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

private synchronized void updateConfigMapProperties(Properties newProperties, ConfigSourceType sourceType) {
this.sourceType = sourceType;
if (newProperties.equals(configMapProperties)) {
return;
Comment on lines +258 to +259
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;
Comment on lines +273 to +274
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", configUtil.getAppId());
transaction.addData("configMapNamespace", configUtil.getConfigMapNamespace());
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(configUtil.getConfigMapNamespace(), configUtil.getAppId(), 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