Skip to content

Commit

Permalink
resolve comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@linglingye001
linglingye001 committed Jan 9, 2025
1 parent f6012f4 commit d4931fd
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 138 deletions.
18 changes: 10 additions & 8 deletions internal/controller/appconfigurationprovider_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,9 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context

/* Create ConfigMap from key-value settings */
if processor.RefreshOptions.ConfigMapSettingPopulated {
if !DataChanged(&existingConfigMap, provider.Spec.Target.ConfigMapData, processor.Settings.ConfigMapSettings) {
klog.V(5).Infof("Skip updating the configMap %q in %q namespace since data is not changed", provider.Spec.Target.ConfigMapName, provider.Namespace)
} else {
result, err := reconciler.createOrUpdateConfigMap(ctx, provider, processor.Settings)
if err != nil {
return result, nil
}
result, err := reconciler.createOrUpdateConfigMap(ctx, &existingConfigMap, provider, processor.Settings)
if err != nil {
return result, nil
}
}

Expand Down Expand Up @@ -373,8 +369,14 @@ func (reconciler *AzureAppConfigurationProviderReconciler) requeueWhenGetSetting

func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateConfigMap(
ctx context.Context,
existingConfigMap *corev1.ConfigMap,
provider *acpv1.AzureAppConfigurationProvider,
settings *loader.TargetKeyValueSettings) (reconcile.Result, error) {
if !shouldCreateOrUpdateConfigMap(existingConfigMap, provider.Spec.Target.ConfigMapData, settings.ConfigMapSettings) {
klog.V(5).Infof("Skip updating the configMap %q in %q namespace since data is not changed", provider.Spec.Target.ConfigMapName, provider.Namespace)
return reconcile.Result{}, nil
}

configMapObj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: provider.Spec.Target.ConfigMapName,
Expand Down Expand Up @@ -435,7 +437,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
}

for secretName, secret := range processor.Settings.SecretSettings {
if !shouldCreateOrUpdate(processor, secretName, existingSecrets) {
if !shouldCreateOrUpdateSecret(processor, secretName, existingSecrets) {
if _, ok := reconciler.ProvidersReconcileState[namespacedName].ExistingK8sSecrets[secretName]; ok {
processor.Settings.K8sSecrets[secretName].SecretResourceVersion = reconciler.ProvidersReconcileState[namespacedName].ExistingK8sSecrets[secretName].SecretResourceVersion
}
Expand Down
120 changes: 0 additions & 120 deletions internal/controller/appconfigurationprovider_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1661,126 +1661,6 @@ var _ = Describe("AppConfiguationProvider controller", func() {
_ = k8sClient.Delete(ctx, configProvider)
})

It("Should refresh configMap when both configuration.refresh and featureFlag.refresh enabled", func() {
By("When selected feattureFlags updated in Azure App Configuration")
mapResult := make(map[string]string)
keyValueEtags := make(map[acpv1.Selector][]*azcore.ETag)
featureFlagEtags := make(map[acpv1.Selector][]*azcore.ETag)
mapResult["filestyle.json"] = "{\"aKey\":\"testValue\",\"feature_management\":{\"feature_flags\":[{\"id\": \"testFeatureFlag\",\"enabled\": true,\"conditions\": {\"client_filters\": []}}]}}"

allSettings := &loader.TargetKeyValueSettings{
ConfigMapSettings: mapResult,
KeyValueETags: keyValueEtags,
FeatureFlagETags: featureFlagEtags,
}

mapResult2 := make(map[string]string)
mapResult2["filestyle.json"] = "{\"aKey\":\"testValue\",\"feature_management\":{\"feature_flags\":[{\"id\": \"testFeatureFlag\",\"enabled\": false,\"conditions\": {\"client_filters\": []}}]}}"

allSettings2 := &loader.TargetKeyValueSettings{
ConfigMapSettings: mapResult2,
KeyValueETags: keyValueEtags,
FeatureFlagETags: featureFlagEtags,
}

mockConfigurationSettings.EXPECT().CreateTargetSettings(gomock.Any(), gomock.Any()).Return(allSettings, nil)

ctx := context.Background()
providerName := "test-appconfigurationprovider-7a"
configMapName := "file-style-configmap-to-be-created-7a"
wildcard := "*"
configProvider := &acpv1.AzureAppConfigurationProvider{
TypeMeta: metav1.TypeMeta{
APIVersion: "appconfig.kubernetes.config/v1",
Kind: "AzureAppConfigurationProvider",
},
ObjectMeta: metav1.ObjectMeta{
Name: providerName,
Namespace: ProviderNamespace,
Labels: map[string]string{"foo": "fooValue", "bar": "barValue"},
},
Spec: acpv1.AzureAppConfigurationProviderSpec{
Endpoint: &EndpointName,
Target: acpv1.ConfigurationGenerationParameters{
ConfigMapName: configMapName,
ConfigMapData: &acpv1.ConfigMapDataOptions{
Type: "json",
Key: "filestyle.json",
},
},
Configuration: acpv1.AzureAppConfigurationKeyValueOptions{
Refresh: &acpv1.DynamicConfigurationRefreshParameters{
Interval: "5s",
Enabled: true,
},
},
FeatureFlag: &acpv1.AzureAppConfigurationFeatureFlagOptions{
Selectors: []acpv1.Selector{
{
KeyFilter: &wildcard,
},
},
Refresh: &acpv1.FeatureFlagRefreshSettings{
Interval: "5s",
Enabled: true,
},
},
},
}
Expect(k8sClient.Create(ctx, configProvider)).Should(Succeed())
time.Sleep(time.Second * 5) //Wait few seconds to wait the second round reconcile complete
configmapLookupKey := types.NamespacedName{Name: configMapName, Namespace: ProviderNamespace}
configmap := &corev1.ConfigMap{}

Eventually(func() bool {
err := k8sClient.Get(ctx, configmapLookupKey, configmap)
return err == nil
}, timeout, interval).Should(BeTrue())

Expect(configmap.Name).Should(Equal(configMapName))
Expect(configmap.Namespace).Should(Equal(ProviderNamespace))
Expect(configmap.Labels["foo"]).Should(Equal("fooValue"))
Expect(configmap.Labels["bar"]).Should(Equal("barValue"))
Expect(configmap.Data["filestyle.json"]).Should(Equal("{\"aKey\":\"testValue\",\"feature_management\":{\"feature_flags\":[{\"id\": \"testFeatureFlag\",\"enabled\": true,\"conditions\": {\"client_filters\": []}}]}}"))
Expect(len(configmap.Data)).Should(Equal(1))

mockConfigurationSettings.EXPECT().CheckPageETags(gomock.Any(), gomock.Any()).Return(false, nil).Times(2)

time.Sleep(6 * time.Second)

Eventually(func() bool {
err := k8sClient.Get(ctx, configmapLookupKey, configmap)
return err == nil
}, timeout, interval).Should(BeTrue())

Expect(configmap.Name).Should(Equal(configMapName))
Expect(configmap.Namespace).Should(Equal(ProviderNamespace))
Expect(configmap.Labels["foo"]).Should(Equal("fooValue"))
Expect(configmap.Labels["bar"]).Should(Equal("barValue"))
Expect(configmap.Data["filestyle.json"]).Should(Equal("{\"aKey\":\"testValue\",\"feature_management\":{\"feature_flags\":[{\"id\": \"testFeatureFlag\",\"enabled\": true,\"conditions\": {\"client_filters\": []}}]}}"))
Expect(len(configmap.Data)).Should(Equal(1))

mockConfigurationSettings.EXPECT().CheckPageETags(gomock.Any(), gomock.Any()).Return(true, nil)
mockConfigurationSettings.EXPECT().RefreshFeatureFlagSettings(gomock.Any(), gomock.Any()).Return(allSettings2, nil)
mockConfigurationSettings.EXPECT().CheckPageETags(gomock.Any(), gomock.Any()).Return(false, nil)

time.Sleep(7 * time.Second)

Eventually(func() bool {
err := k8sClient.Get(ctx, configmapLookupKey, configmap)
return err == nil
}, timeout, interval).Should(BeTrue())

Expect(configmap.Name).Should(Equal(configMapName))
Expect(configmap.Namespace).Should(Equal(ProviderNamespace))
Expect(configmap.Labels["foo"]).Should(Equal("fooValue"))
Expect(configmap.Labels["bar"]).Should(Equal("barValue"))
Expect(configmap.Data["filestyle.json"]).Should(Equal("{\"aKey\":\"testValue\",\"feature_management\":{\"feature_flags\":[{\"id\": \"testFeatureFlag\",\"enabled\": false,\"conditions\": {\"client_filters\": []}}]}}"))
Expect(len(configmap.Data)).Should(Equal(1))

_ = k8sClient.Delete(ctx, configProvider)
})

It("Feature flag refresh can work with secret refresh", func() {
By("By enabling refresh on secret and feature flag")
mapResult := make(map[string]string)
Expand Down
4 changes: 2 additions & 2 deletions internal/controller/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func verifySelectorObject(selector acpv1.Selector) error {
return nil
}

func shouldCreateOrUpdate(processor *AppConfigurationProviderProcessor, secretName string, existingK8sSecrets map[string]corev1.Secret) bool {
func shouldCreateOrUpdateSecret(processor *AppConfigurationProviderProcessor, secretName string, existingK8sSecrets map[string]corev1.Secret) bool {
// If the secret does not exist, create it
if _, ok := existingK8sSecrets[secretName]; !ok {
return true
Expand All @@ -322,7 +322,7 @@ func shouldCreateOrUpdate(processor *AppConfigurationProviderProcessor, secretNa
return !reflect.DeepEqual(processor.Settings.SecretSettings[secretName].Data, existingK8sSecrets[secretName].Data)
}

func DataChanged(existingConfigMap *corev1.ConfigMap, dataOptions *acpv1.ConfigMapDataOptions, latestConfigMapSettings map[string]string) bool {
func shouldCreateOrUpdateConfigMap(existingConfigMap *corev1.ConfigMap, dataOptions *acpv1.ConfigMapDataOptions, latestConfigMapSettings map[string]string) bool {
if existingConfigMap == nil || existingConfigMap.Data == nil {
return true
}
Expand Down
17 changes: 9 additions & 8 deletions internal/loader/configuration_setting_loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,31 +370,32 @@ func (csl *ConfigurationSettingLoader) getFeatureFlagSettings(ctx context.Contex
}

settingsLength := len(settingsResponse.Settings)
featureFlagExisted := make(map[string]bool, settingsLength)
deduplicateFeatureFlags := make([]interface{}, 0)
featureFlagExist := make(map[string]bool, settingsLength)
deduplicatedFeatureFlags := make([]interface{}, 0)

// if settings returned like this: [{"id": "Beta"...}, {"id": "Alpha"...}, {"id": "Beta"...}], we need to deduplicate it to [{"id": "Alpha"...}, {"id": "Beta"...}], the last one wins
for i := settingsLength - 1; i >= 0; i-- {
key := *settingsResponse.Settings[i].Key
if featureFlagExisted[key] {
if featureFlagExist[key] {
continue
}
featureFlagExisted[key] = true
featureFlagExist[key] = true
var out interface{}
err := json.Unmarshal([]byte(*settingsResponse.Settings[i].Value), &out)
if err != nil {
return nil, nil, fmt.Errorf("failed to unmarshal feature flag settings: %s", err.Error())
}
deduplicateFeatureFlags = append(deduplicateFeatureFlags, out)
deduplicatedFeatureFlags = append(deduplicatedFeatureFlags, out)
}

// reverse the deduplicateFeatureFlags to keep the order
for i, j := 0, len(deduplicateFeatureFlags)-1; i < j; i, j = i+1, j-1 {
deduplicateFeatureFlags[i], deduplicateFeatureFlags[j] = deduplicateFeatureFlags[j], deduplicateFeatureFlags[i]
for i, j := 0, len(deduplicatedFeatureFlags)-1; i < j; i, j = i+1, j-1 {
deduplicatedFeatureFlags[i], deduplicatedFeatureFlags[j] = deduplicatedFeatureFlags[j], deduplicatedFeatureFlags[i]
}

// featureFlagSection = {"feature_flags": [{...}, {...}]}
var featureFlagSection = map[string]interface{}{
FeatureFlagSectionName: deduplicateFeatureFlags,
FeatureFlagSectionName: deduplicatedFeatureFlags,
}

return featureFlagSection, settingsResponse.Etags, nil
Expand Down

0 comments on commit d4931fd

Please sign in to comment.