Skip to content

App Configuration Bug Fixes #723

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.

Sign up for GitHub

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

Merged
saragluna merged 3 commits into from
Jul 13, 2020
Merged

App Configuration Bug Fixes #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85fb3b2
Fixes Refresh not being disabled when provider is, fixed Muliple prof…
mrm9084 Jul 1, 2020
da2c074
Fixed issue where switching from on/off to conditional didn't work af…
mrm9084 Jul 2, 2020
28fa00a
Updated so profiles had seprate etag stored
mrm9084 Jul 6, 2020
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 ...ava/com/microsoft/azure/spring/cloud/config/web/AppConfigurationWebAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package com.microsoft.azure.spring.cloud.config.web;

import org.springframework.boot.autoconfigure.condition.ConditionalOnBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
import org.springframework.cloud.endpoint.RefreshEndpoint;
import org.springframework.context.annotation.Bean;
Expand All @@ -13,6 +14,7 @@
import com.microsoft.azure.spring.cloud.config.AppConfigurationRefresh;

@Configuration
@ConditionalOnBean(AppConfigurationRefresh.class)
public class AppConfigurationWebAutoConfiguration {

@Configuration
Expand Down
13 changes: 8 additions & 5 deletions ...n/java/com/microsoft/azure/spring/cloud/config/AppConfigurationPropertySourceLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class AppConfigurationPropertySourceLocator implements PropertySourceLoca
private ClientStore clients;

private KeyVaultCredentialProvider keyVaultCredentialProvider;

private SecretClientBuilderSetup keyVaultClientProvider;

private static Boolean startup = true;
Expand Down Expand Up @@ -214,8 +214,8 @@ private List<AppConfigurationPropertySource> create(String context, ConfigStore
List<AppConfigurationPropertySource> sourceList = new ArrayList<>();

try {
putStoreContext(store.getEndpoint(), context, storeContextsMap);
for (String label : store.getLabels()) {
putStoreContext(store.getEndpoint(), context, storeContextsMap);
AppConfigurationPropertySource propertySource = new AppConfigurationPropertySource(context, store,
label, properties, clients, appProperties, keyVaultCredentialProvider, keyVaultClientProvider);

Expand All @@ -227,7 +227,7 @@ private List<AppConfigurationPropertySource> create(String context, ConfigStore
}

// Setting new ETag values for Watch
String watchedKeyNames = clients.watchedKeyNames(store, storeContextsMap);
String watchedKeyNames = clients.watchedKeyNames(store, context);
SettingSelector settingSelector = new SettingSelector().setKeyFilter(watchedKeyNames).setLabelFilter("*");

ConfigurationSetting configurationRevision = clients.getRevison(settingSelector,
Expand All @@ -238,10 +238,13 @@ private List<AppConfigurationPropertySource> create(String context, ConfigStore
ConfigurationSetting featureRevision = clients.getRevison(settingSelector,
store.getEndpoint());

String prefix = "_" + context;

if (configurationRevision != null) {
StateHolder.setEtagState(store.getEndpoint() + CONFIGURATION_SUFFIX, configurationRevision);
StateHolder.setEtagState(store.getEndpoint() + CONFIGURATION_SUFFIX + prefix, configurationRevision);
} else {
StateHolder.setEtagState(store.getEndpoint() + CONFIGURATION_SUFFIX, new ConfigurationSetting());
StateHolder.setEtagState(store.getEndpoint() + CONFIGURATION_SUFFIX + prefix,
new ConfigurationSetting());
}

if (featureRevision != null) {
Expand Down
48 changes: 36 additions & 12 deletions ...config/src/main/java/com/microsoft/azure/spring/cloud/config/AppConfigurationRefresh.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static com.microsoft.azure.spring.cloud.config.Constants.FEATURE_SUFFIX;

import java.time.Duration;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -51,6 +52,8 @@ public class AppConfigurationRefresh implements ApplicationEventPublisherAware {

private String eventDataInfo;

private List<String> featureWatchKey = new ArrayList<String>();

public AppConfigurationRefresh(AppConfigurationProperties properties, Map<String, List<String>> storeContextsMap,
ClientStore clientStore) {
this.configStores = properties.getStores();
Expand All @@ -59,6 +62,7 @@ public AppConfigurationRefresh(AppConfigurationProperties properties, Map<String
this.lastCheckedTime = new Date();
this.clientStore = clientStore;
this.eventDataInfo = "";
featureWatchKey.add(FEATURE_STORE_WATCH_KEY);
}

@Override
Expand Down Expand Up @@ -100,11 +104,10 @@ private boolean refreshStores() {
if (notCachedTime == null || date.after(notCachedTime)) {
for (ConfigStore configStore : configStores) {
if (StateHolder.getLoadState(configStore.getEndpoint())) {
String watchedKeyNames = clientStore.watchedKeyNames(configStore, storeContextsMap);
willRefresh = refresh(configStore, CONFIGURATION_SUFFIX, watchedKeyNames) ? true
willRefresh = refresh_configurations(configStore) ? true
: willRefresh;
// Refresh Feature Flags
willRefresh = refresh(configStore, FEATURE_SUFFIX, FEATURE_STORE_WATCH_KEY) ? true
willRefresh = refreshFeatureFlag(configStore) ? true
: willRefresh;
} else {
LOGGER.debug("Skipping refresh check for " + configStore.getEndpoint()
Expand Down Expand Up @@ -137,15 +140,36 @@ private boolean refreshStores() {
* published.
*
* @param store the {@code store} for which to composite watched key names
* @param storeSuffix Suffix used to distinguish between Settings and Features
* @param watchedKeyNames Key used to check if refresh should occur
* @return Refresh event was triggered. No other sources need to be checked.
*/
private boolean refresh(ConfigStore store, String storeSuffix, String watchedKeyNames) {
String storeNameWithSuffix = store.getEndpoint() + storeSuffix;
SettingSelector settingSelector = new SettingSelector().setKeyFilter(watchedKeyNames)
.setLabelFilter("*");
private boolean refresh_configurations(ConfigStore store) {
for (String context : storeContextsMap.get(store.getEndpoint())) {
// Checking every Profile
String storeNameWithSuffix = store.getEndpoint() + CONFIGURATION_SUFFIX + "_" + context;
String watchedKeyName = clientStore.watchedKeyNames(store, context);

if (checkETagChange(store, storeNameWithSuffix, watchedKeyName)) {
return true;
}
}
return false;
}

/**
* Checks un-cached items for etag changes for feature flags. If they have changed a
* RefreshEventData is published.
*
* @param store the {@code store} for which to composite watched key names
* @return Refresh event was triggered. No other sources need to be checked.
*/
private boolean refreshFeatureFlag(ConfigStore store) {
String storeNameWithSuffix = store.getEndpoint() + FEATURE_SUFFIX;
return checkETagChange(store, storeNameWithSuffix, FEATURE_STORE_WATCH_KEY);
}

private boolean checkETagChange(ConfigStore store, String storeNameWithSuffix, String watchKey) {
SettingSelector settingSelector = new SettingSelector().setKeyFilter(watchKey)
.setLabelFilter("*");
ConfigurationSetting revision = clientStore.getRevison(settingSelector, store.getEndpoint());

String etag = null;
Expand All @@ -166,11 +190,11 @@ private boolean refresh(ConfigStore store, String storeSuffix, String watchedKey

if (etag != null && !etag.equals(StateHolder.getEtagState(storeNameWithSuffix).getETag())) {
LOGGER.trace("Some keys in store [{}] matching [{}] is updated, will send refresh event.",
store.getEndpoint(), watchedKeyNames);
store.getEndpoint(), watchKey);
if (this.eventDataInfo.isEmpty()) {
this.eventDataInfo = watchedKeyNames;
this.eventDataInfo = watchKey;
} else {
this.eventDataInfo += ", " + watchedKeyNames;
this.eventDataInfo += ", " + watchKey;
}

// Don't need to refresh here will be done in Property Source
Expand Down
26 changes: 21 additions & 5 deletions ...tion-config/src/main/java/com/microsoft/azure/spring/cloud/config/stores/ClientStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import java.io.IOException;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
Expand All @@ -25,10 +26,9 @@
import com.azure.data.appconfiguration.models.ConfigurationSetting;
import com.azure.data.appconfiguration.models.SettingSelector;
import com.azure.identity.ManagedIdentityCredentialBuilder;
import com.microsoft.azure.spring.cloud.config.ConfigurationClientBuilderSetup;
import com.microsoft.azure.spring.cloud.config.AppConfigurationCredentialProvider;
import com.microsoft.azure.spring.cloud.config.AppConfigurationProviderProperties;
import com.microsoft.azure.spring.cloud.config.AppConfigurationRefresh;
import com.microsoft.azure.spring.cloud.config.ConfigurationClientBuilderSetup;
import com.microsoft.azure.spring.cloud.config.pipline.policies.BaseAppConfigurationPolicy;
import com.microsoft.azure.spring.cloud.config.resource.Connection;
import com.microsoft.azure.spring.cloud.config.resource.ConnectionPool;
Expand Down Expand Up @@ -157,12 +157,28 @@ public final List<ConfigurationSetting> listSettings(SettingSelector settingSele
* @param storeContextsMap map storing store name and List of context key-value pair
* @return the full name of the key mapping to the configuration store
*/
public String watchedKeyNames(ConfigStore store, Map<String, List<String>> storeContextsMap) {
public List<String> watchedKeyNames(ConfigStore store, Map<String, List<String>> storeContextsMap) {
List<String> watchedKeys = new ArrayList<String>();
String watchedKey = store.getWatchedKey().trim();
List<String> contexts = storeContextsMap.get(store.getEndpoint());

String watchedKeys = contexts.stream().map(ctx -> genKey(ctx, watchedKey))
.collect(Collectors.joining(","));
for (String context : contexts) {
String key = genKey(context, watchedKey);
if (key.contains(",") && key.contains("*")) {
// Multi keys including one or more key patterns is not supported by API,
// will
// watch all keys(*) instead
key = "*";
}
watchedKeys.add(key);
}

return watchedKeys;
}

public String watchedKeyNames(ConfigStore store, String context) {
String watchedKey = store.getWatchedKey().trim();
String watchedKeys = genKey(context, watchedKey);

if (watchedKeys.contains(",") && watchedKeys.contains("*")) {
// Multi keys including one or more key patterns is not supported by API, will
Expand Down
49 changes: 31 additions & 18 deletions ...ig/src/test/java/com/microsoft/azure/spring/cloud/config/AppConfigurationRefreshTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ public class AppConfigurationRefreshTest {
@Mock
private ClientStore clientStoreMock;

private static final String WATCHED_KEYS = "/application/*";
private static final String WATCHED_KEY = "/application/*";

private List<String> watchedKeys = new ArrayList<String>();

@Before
public void setup() {
Expand All @@ -69,7 +71,10 @@ public void setup() {
ConfigStore store = new ConfigStore();
store.setEndpoint(TEST_STORE_NAME);
store.setConnectionString(TEST_CONN_STRING);
store.setWatchedKey(WATCHED_KEYS);
store.setWatchedKey(WATCHED_KEY);

watchedKeys = new ArrayList<String>();
watchedKeys.add(WATCHED_KEY);

properties = new AppConfigurationProperties();
properties.setStores(Arrays.asList(store));
Expand All @@ -88,21 +93,21 @@ public void setup() {
item.setKey("fake-etag/application/test.key");
item.setETag("fake-etag");

when(clientStoreMock.watchedKeyNames(Mockito.any(), Mockito.any())).thenReturn(WATCHED_KEYS);
when(clientStoreMock.watchedKeyNames(Mockito.any(), Mockito.anyString())).thenReturn(WATCHED_KEY);

configRefresh = new AppConfigurationRefresh(properties, contextsMap, clientStoreMock);
StateHolder.setLoadState(TEST_STORE_NAME, true);
}

@After
public void cleanupMethod() {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, new ConfigurationSetting());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, new ConfigurationSetting());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, new ConfigurationSetting());
}

@Test
public void nonUpdatedEtagShouldntPublishEvent() throws Exception {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse());

configRefresh.setApplicationEventPublisher(eventPublisher);
Expand All @@ -115,7 +120,7 @@ public void nonUpdatedEtagShouldntPublishEvent() throws Exception {

@Test
public void updatedEtagShouldPublishEvent() throws Exception {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse());

when(clientStoreMock.getRevison(Mockito.any(), Mockito.anyString())).thenReturn(initialResponse());
Expand All @@ -131,7 +136,7 @@ public void updatedEtagShouldPublishEvent() throws Exception {
assertTrue(configRefresh.refreshConfigurations().get());
verify(eventPublisher, times(1)).publishEvent(any(RefreshEvent.class));

StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, updatedResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, updatedResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, updatedResponse());

HashMap<String, String> map = new HashMap<String, String>();
Expand All @@ -141,7 +146,7 @@ public void updatedEtagShouldPublishEvent() throws Exception {
ConfigurationSetting updated = new ConfigurationSetting();
updated.setETag("fake-etag-updated");

StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, updated);
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, updated);

// If there is no change it shouldn't update
assertFalse(configRefresh.refreshConfigurations().get());
Expand All @@ -150,7 +155,7 @@ public void updatedEtagShouldPublishEvent() throws Exception {

@Test
public void noEtagReturned() throws Exception {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse());

when(clientStoreMock.getRevison(Mockito.any(), Mockito.anyString()))
Expand All @@ -164,7 +169,7 @@ public void noEtagReturned() throws Exception {

@Test
public void nullItemsReturned() throws Exception {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse());

when(clientStoreMock.getRevison(Mockito.any(), Mockito.anyString())).thenReturn(null);
Expand All @@ -180,13 +185,17 @@ public void noInitialStateNoEtag() throws Exception {
ConfigStore store = new ConfigStore();
store.setEndpoint(TEST_STORE_NAME + "_LOST");
store.setConnectionString(TEST_CONN_STRING);
store.setWatchedKey(WATCHED_KEYS);
store.setWatchedKey(WATCHED_KEY);

AppConfigurationProperties propertiesLost = new AppConfigurationProperties();
propertiesLost.setStores(Arrays.asList(store));

propertiesLost.setCacheExpiration(Duration.ofMinutes(-60));
AppConfigurationRefresh configRefreshLost = new AppConfigurationRefresh(propertiesLost, contextsMap,

Map<String, List<String>> contextsMapLost = new ConcurrentHashMap<>();
contextsMapLost.put(TEST_STORE_NAME + "_LOST", Arrays.asList(TEST_ETAG));

AppConfigurationRefresh configRefreshLost = new AppConfigurationRefresh(propertiesLost, contextsMapLost,
clientStoreMock);
StateHolder.setLoadState(TEST_STORE_NAME + "_LOST", true);
when(clientStoreMock.getRevison(Mockito.any(), Mockito.anyString())).thenReturn(null);
Expand All @@ -202,16 +211,20 @@ public void noInitialStateHasEtag() throws Exception {
ConfigStore store = new ConfigStore();
store.setEndpoint(TEST_STORE_NAME + "_LOST");
store.setConnectionString(TEST_CONN_STRING);
store.setWatchedKey(WATCHED_KEYS);
store.setWatchedKey(WATCHED_KEY);

AppConfigurationProperties propertiesLost = new AppConfigurationProperties();
propertiesLost.setStores(Arrays.asList(store));

propertiesLost.setCacheExpiration(Duration.ofMinutes(-60));
AppConfigurationRefresh configRefreshLost = new AppConfigurationRefresh(propertiesLost, contextsMap,

Map<String, List<String>> contextsMapLost = new ConcurrentHashMap<>();
contextsMapLost.put(TEST_STORE_NAME + "_LOST", Arrays.asList(TEST_ETAG));

AppConfigurationRefresh configRefreshLost = new AppConfigurationRefresh(propertiesLost, contextsMapLost,
clientStoreMock);
StateHolder.setLoadState(TEST_STORE_NAME + "_LOST", true);

when(clientStoreMock.getRevison(Mockito.any(), Mockito.anyString())).thenReturn(updatedResponse());
configRefreshLost.setApplicationEventPublisher(eventPublisher);

Expand All @@ -222,7 +235,7 @@ public void noInitialStateHasEtag() throws Exception {

@Test
public void notRefreshTime() throws Exception {
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + CONFIGURATION_SUFFIX + "_" + TEST_ETAG, initialResponse());
StateHolder.setEtagState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse());

properties.setCacheExpiration(Duration.ofMinutes(60));
Expand Down
2 changes: 1 addition & 1 deletion ...-config/src/test/java/com/microsoft/azure/spring/cloud/config/stores/ClientStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public void watchedKeyNamesWildcardTest() {

storeContextsMap.put(TEST_ENDPOINT, contexts);

assertEquals("/application/*", clientStore.watchedKeyNames(store, storeContextsMap));
assertEquals("/application/*", clientStore.watchedKeyNames(store, "/application/"));
}

private PagedFlux<ConfigurationSetting> getConfigurationPagedFlux(int noOfPages) throws MalformedURLException {
Expand Down
8 changes: 8 additions & 0 deletions ...gement/src/main/java/com/microsoft/azure/spring/cloud/feature/manager/FeatureManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,14 @@ public void putAll(Map<? extends String, ? extends Object> m) {
return;
}

// Need to reset or switch between on/off to conditional doesn't work
featureManagement = new HashMap<String, Feature>();
onOff = new HashMap<String, Boolean>();

if (m.size() == 1 && m.containsKey("featureManagement")) {
m = (Map<? extends String, ? extends Object>) m.get("featureManagement");
}

for (String key : m.keySet()) {
addToFeatures(m, key, "");
}
Expand Down