-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Changes from 14 commits
82f7356
e14fd00
562b325
8705ba1
d8c51dd
435df9d
e2c7cd7
73581e9
c52337c
3a22a38
80f6c59
65ce65a
88b1d75
c3fc604
79ae56a
9252a93
82d61d7
2f5ddf6
f95ca86
7438b07
0c66c59
f7ff6d9
84d10fd
41672e6
03aa3ad
74a6b45
b8392ca
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,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) { | ||||||||||||||||||||||||||||||||||||||||||
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. This constructor seems unnecessary. |
||||||||||||||||||||||||||||||||||||||||||
this(namespace, (ConfigRepository) null); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
public K8sConfigMapConfigRepository(String namespace, KubernetesManager kubernetesManager) { | ||||||||||||||||||||||||||||||||||||||||||
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. It's not a practice to add a constructor only for test scenario. You could use |
||||||||||||||||||||||||||||||||||||||||||
this.namespace = namespace; | ||||||||||||||||||||||||||||||||||||||||||
configUtil = ApolloInjector.getInstance(ConfigUtil.class); | ||||||||||||||||||||||||||||||||||||||||||
kubernetesManager = ApolloInjector.getInstance(KubernetesManager.class); | ||||||||||||||||||||||||||||||||||||||||||
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. Fix the assignment of the In the constructor, the parameter 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
configMapNamespace = configUtil.getConfigMapNamespace(); | ||||||||||||||||||||||||||||||||||||||||||
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. The variable name k8sNamespace may be clearer |
||||||||||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||
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. It may be better to add a prefix to distinguish it from the configmap of the business, such as apollo-configcache-xxx |
||||||||||||||||||||||||||||||||||||||||||
// 初始化configmap | ||||||||||||||||||||||||||||||||||||||||||
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 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(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); | ||||||||||||||||||||||||||||||||||||||||||
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 logging potentially sensitive configuration properties Logging the entire Apply this diff to remove or modify the log statement: - logger.info("configmap值:{}", configMapProperties);
+ // logger.info("Configuration properties loaded successfully."); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
return result; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||
* Update the memory when the configuration center changes | ||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||
* @param upstreamConfigRepository the upstream repo | ||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||||||||||||||||||||
public void setUpstreamRepository(ConfigRepository upstreamConfigRepository) { | ||||||||||||||||||||||||||||||||||||||||||
// 设置上游数据源 | ||||||||||||||||||||||||||||||||||||||||||
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 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); | ||||||||||||||||||||||||||||||||||||||||||
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. 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(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); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
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 Reuse Creating multiple 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); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
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()); | ||||||||||||||||||||||||||||||||||||||||||
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. It's not a common practice to log the configurations.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
updateConfigMapProperties(upstream.getConfig(), upstream.getSourceType()); | ||||||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||||||
} catch (Throwable ex) { | ||||||||||||||||||||||||||||||||||||||||||
Tracer.logError(ex); | ||||||||||||||||||||||||||||||||||||||||||
logger.warn("Sync config from upstream repository {} failed, reason: {}", upstream.getClass(), | ||||||||||||||||||||||||||||||||||||||||||
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. Handle null properties returned from upstream In 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
ExceptionUtil.getDetailMessage(ex)); | ||||||||||||||||||||||||||||||||||||||||||
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 logging sensitive configuration properties and optimize method calls At line 238, the 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
private synchronized void updateConfigMapProperties(Properties newProperties, ConfigSourceType sourceType) { | ||||||||||||||||||||||||||||||||||||||||||
this.sourceType = sourceType; | ||||||||||||||||||||||||||||||||||||||||||
if (newProperties.equals(configMapProperties)) { | ||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||
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. Prevent NullPointerException when comparing properties In 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||
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. Add null check to avoid NullPointerException in When 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
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(); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
} |
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.
Add it to the last