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.

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

Jump to bottom

Refresh secret when data change #53

Merged
merged 9 commits into from
Jul 22, 2024
Merged

Refresh secret when data change #53

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c77306b
resolve confict
linglingye001 Jul 5, 2024
9b4dc35
update
linglingye001 Jul 9, 2024
e006f09
resolve comments
linglingye001 Jul 15, 2024
6b64225
add test
linglingye001 Jul 16, 2024
d5ca9a3
update
linglingye001 Jul 18, 2024
7fdc735
update
linglingye001 Jul 18, 2024
030d112
update
linglingye001 Jul 19, 2024
4d0ffb2
rename secretMetadata to secretsMetadata
linglingye001 Jul 19, 2024
8fee3ea
update
linglingye001 Jul 19, 2024
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
65 changes: 35 additions & 30 deletions internal/controller/appconfigurationprovider_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,15 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
}
RichardChen820 marked this conversation as resolved.
Show resolved Hide resolved
}

result, err := reconciler.createOrUpdateSecrets(ctx, provider, processor.Settings)
result, err := reconciler.createOrUpdateSecrets(ctx, provider, processor)
if err != nil {
return result, nil
}
}

// Expel the secrets which are no longer selected by the provider.
if provider.Spec.Secret == nil || processor.RefreshOptions.SecretSettingPopulated {
result, err := reconciler.expelRemovedSecrets(ctx, provider, existingSecrets, processor.ReconciliationState.ExistingSecretReferences)
result, err := reconciler.expelRemovedSecrets(ctx, provider, existingSecrets, processor.Settings.SecretReferences)
if err != nil {
return result, nil
}
Expand Down Expand Up @@ -411,8 +411,8 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateConfigM
func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets(
ctx context.Context,
provider *acpv1.AzureAppConfigurationProvider,
settings *loader.TargetKeyValueSettings) (reconcile.Result, error) {
if len(settings.SecretSettings) == 0 {
processor *AppConfigurationProviderProcessor) (reconcile.Result, error) {
if len(processor.Settings.SecretSettings) == 0 {
klog.V(3).Info("No secret settings are fetched from Azure AppConfiguration")
}

Expand All @@ -425,36 +425,41 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
Namespace: provider.Namespace,
}

for secretName, secret := range settings.SecretSettings {
secretObj := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: provider.Namespace,
},
Type: secret.Type,
}
secretToUpdate := checkAndUpdateSecretRef(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences, processor.Settings.SecretReferences, processor.ShouldReconcile)
for secretName, secret := range processor.Settings.SecretSettings {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about doing this:

for secretName, secret := range processor.Settings.SecretSettings {
   if (!shouldCreateOrUpdate()){
     continue;
   }

    // do create or update
     .....
}

if _, ok := secretToUpdate[secretName]; ok {
secretObj := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: provider.Namespace,
},
Type: secret.Type,
}

// Important: set the ownership of secret
if err := controllerutil.SetControllerReference(provider, secretObj, reconciler.Scheme); err != nil {
reconciler.logAndSetFailStatus(provider, err)
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}
// Important: set the ownership of secret
if err := controllerutil.SetControllerReference(provider, secretObj, reconciler.Scheme); err != nil {
reconciler.logAndSetFailStatus(provider, err)
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}

provider.Annotations[LastReconcileTimeAnnotation] = metav1.Now().UTC().String()
operationResult, err := ctrl.CreateOrUpdate(ctx, reconciler.Client, secretObj, func() error {
secretObj.Data = secret.Data
secretObj.Labels = provider.Labels
secretObj.Annotations = provider.Annotations
provider.Annotations[LastReconcileTimeAnnotation] = metav1.Now().UTC().String()
operationResult, err := ctrl.CreateOrUpdate(ctx, reconciler.Client, secretObj, func() error {
secretObj.Data = secret.Data
secretObj.Labels = provider.Labels
secretObj.Annotations = provider.Annotations

return nil
})
if err != nil {
reconciler.logAndSetFailStatus(provider, err)
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}
return nil
})
if err != nil {
reconciler.logAndSetFailStatus(provider, err)
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}

reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretObj.Name].SecretResourceVersion = secretObj.ResourceVersion
klog.V(5).Infof("Secret %q in %q namespace is %s", secretObj.Name, secretObj.Namespace, string(operationResult))
processor.Settings.SecretReferences[secretName].SecretResourceVersion = secretObj.ResourceVersion
klog.V(5).Infof("Secret %q in %q namespace is %s", secretObj.Name, secretObj.Namespace, string(operationResult))
} else {
klog.V(5).Infof("Skip updating the secret %q in %q namespace since its data is up-to-date", secretName, provider.Namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Skip updating the secret %q in %q namespace since data is not changed.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a v(5) log here if the secret is not updated.

}

return reconcile.Result{}, nil
Expand Down
48 changes: 42 additions & 6 deletions internal/controller/appconfigurationprovider_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,11 @@ var _ = Describe("AppConfiguationProvider controller", func() {
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretTypeTLS,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
UriSegments: make(map[string]loader.KeyVaultSecretMetadata),
},
secretName2: {
Type: corev1.SecretTypeOpaque,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
UriSegments: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -266,7 +266,7 @@ var _ = Describe("AppConfiguationProvider controller", func() {
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretType("Opaque"),
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
UriSegments: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -609,7 +609,7 @@ var _ = Describe("AppConfiguationProvider controller", func() {
_ = k8sClient.Delete(ctx, configProvider)
})

It("Should refresh secret", func() {
RichardChen820 marked this conversation as resolved.
Show resolved Hide resolved
It("Should refresh secret when data change", func() {
By("By enabling refresh on secret")
configMapResult := make(map[string]string)
configMapResult["testKey"] = "testValue"
Expand All @@ -622,6 +622,11 @@ var _ = Describe("AppConfiguationProvider controller", func() {
secretResult["testSecretKey3"] = []byte("testSecretValue3")

secretName := "secret-to-be-refreshed-3"
secretMetadata := make(map[string]loader.KeyVaultSecretMetadata)
secretMetadata["testSecretKey2"] = loader.KeyVaultSecretMetadata{
HostName: "fakeHostName",
}

allSettings := &loader.TargetKeyValueSettings{
SecretSettings: map[string]corev1.Secret{
secretName: {
Expand All @@ -633,7 +638,7 @@ var _ = Describe("AppConfiguationProvider controller", func() {
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretType("Opaque"),
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
UriSegments: secretMetadata,
},
},
}
Expand Down Expand Up @@ -713,7 +718,37 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
}

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newResolvedSecret, nil)
newSecretMetadata := make(map[string]loader.KeyVaultSecretMetadata)
newSecretMetadata["testSecretKey"] = loader.KeyVaultSecretMetadata{
HostName: "fakeHostName",
}
mockedSecretReference := make(map[string]*loader.TargetSecretReference)
mockedSecretReference[secretName] = &loader.TargetSecretReference{
Type: corev1.SecretType("Opaque"),
UriSegments: newSecretMetadata,
}

newTargetSettings := &loader.TargetKeyValueSettings{
SecretSettings: newResolvedSecret,
SecretReferences: mockedSecretReference,
}

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't quite sure why check the refresh two times. And seems you didn't add the mocks for the secretId, how it can be refreshed without checking secretID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test covers 2 refresh scenarios, the first time refresh when secretMetaData change, the second time not refresh when data not change. The first time refresh because mocked secretReferences length changed. Will add secretID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got you, and please add the scenario for the secretId check

// Refresh interval is 1 minute, wait for 65 seconds to make sure the refresh is triggered
time.Sleep(65 * time.Second)

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

Expect(secret.Namespace).Should(Equal(ProviderNamespace))
Expect(string(secret.Data["testSecretKey"])).Should(Equal("newTestSecretValue"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also verify the secret with unchanged ID would not be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line746-763 include scenario when secretID not changed.

Expect(string(secret.Data["testSecretKey2"])).Should(Equal("newTestSecretValue2"))
Expect(string(secret.Data["testSecretKey3"])).Should(Equal("newTestSecretValue3"))
Expect(secret.Type).Should(Equal(corev1.SecretType("Opaque")))

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings, nil)
// Refresh interval is 1 minute, wait for 65 seconds to make sure the refresh is triggered
time.Sleep(65 * time.Second)

Expand All @@ -728,6 +763,7 @@ var _ = Describe("AppConfiguationProvider controller", func() {
Expect(string(secret.Data["testSecretKey3"])).Should(Equal("newTestSecretValue3"))
Expect(secret.Type).Should(Equal(corev1.SecretType("Opaque")))
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra line

})

Context("Verify exist non escaped value in label", func() {
Expand Down
24 changes: 20 additions & 4 deletions internal/controller/processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ func (processor *AppConfigurationProviderProcessor) processFullReconciliation()
return err
}
processor.Settings = updatedSettings
processor.ReconciliationState.ExistingSecretReferences = updatedSettings.SecretReferences
processor.RefreshOptions.ConfigMapSettingPopulated = true
if processor.Provider.Spec.Secret != nil {
processor.RefreshOptions.SecretSettingPopulated = true
Expand Down Expand Up @@ -160,7 +159,6 @@ func (processor *AppConfigurationProviderProcessor) processKeyValueRefresh(exist
}

processor.Settings = keyValueRefreshedSettings
reconcileState.ExistingSecretReferences = keyValueRefreshedSettings.SecretReferences
processor.RefreshOptions.ConfigMapSettingPopulated = true
if processor.Provider.Spec.Secret != nil {
processor.RefreshOptions.SecretSettingPopulated = true
Expand Down Expand Up @@ -201,17 +199,26 @@ func (processor *AppConfigurationProviderProcessor) processSecretReferenceRefres

// Only resolve the secret references that not specified the secret version
secretReferencesToSolve := make(map[string]*loader.TargetSecretReference)
cachedSecretReferences := make(map[string]*loader.TargetSecretReference)
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable seems useless, do we really need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently ResolveSecretReferences doesn't pass/return new parameters for secretID of latest secret, it just update the secretID in kvReferencesToResolve . So need to cache secretRef before resolving.

Copy link
Contributor

Choose a reason for hiding this comment

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

This name is really confusion-leading, what you do here is to deep copy the reconcileState.ExistingSecretReferences, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, how about copiedSecretReferences?

for secretName, reference := range reconcileState.ExistingSecretReferences {
for key, uriSegment := range reference.UriSegments {
if uriSegment.SecretVersion == "" {
if secretReferencesToSolve[secretName] == nil {
secretReferencesToSolve[secretName] = &loader.TargetSecretReference{
Type: reference.Type,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
UriSegments: make(map[string]loader.KeyVaultSecretMetadata),
}
}
secretReferencesToSolve[secretName].UriSegments[key] = uriSegment
}

RichardChen820 marked this conversation as resolved.
Show resolved Hide resolved
if cachedSecretReferences[secretName] == nil {
cachedSecretReferences[secretName] = &loader.TargetSecretReference{
Type: reference.Type,
UriSegments: make(map[string]loader.KeyVaultSecretMetadata),
}
}
cachedSecretReferences[secretName].UriSegments[key] = uriSegment
}
}

Expand All @@ -220,13 +227,19 @@ func (processor *AppConfigurationProviderProcessor) processSecretReferenceRefres
return err
}

for secretName, resolvedSecret := range resolvedSecrets {
for secretName, resolvedSecret := range resolvedSecrets.SecretSettings {
existingSecret, ok := existingSecrets[secretName]
if ok {
maps.Copy(existingSecret.Data, resolvedSecret.Data)
}
}

for secretName, reference := range resolvedSecrets.SecretReferences {
maps.Copy(cachedSecretReferences[secretName].UriSegments, reference.UriSegments)
}

processor.Settings.SecretSettings = existingSecrets
processor.Settings.SecretReferences = cachedSecretReferences
processor.RefreshOptions.SecretSettingPopulated = true

// Update next refresh time only if settings updated successfully
Expand Down Expand Up @@ -270,6 +283,9 @@ func (processor *AppConfigurationProviderProcessor) shouldReconcile(

func (processor *AppConfigurationProviderProcessor) Finish() (ctrl.Result, error) {
processor.ReconciliationState.Generation = processor.Provider.Generation
if processor.RefreshOptions.SecretSettingPopulated {
RichardChen820 marked this conversation as resolved.
Show resolved Hide resolved
RichardChen820 marked this conversation as resolved.
Show resolved Hide resolved
processor.ReconciliationState.ExistingSecretReferences = processor.Settings.SecretReferences
}
if !processor.RefreshOptions.secretReferenceRefreshEnabled &&
!processor.RefreshOptions.sentinelBasedRefreshEnabled &&
!processor.RefreshOptions.featureFlagRefreshEnabled {
Expand Down
38 changes: 38 additions & 0 deletions internal/controller/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,3 +302,41 @@ func verifySelectorObject(selector acpv1.Selector) error {

return nil
}

func checkAndUpdateSecretRef(existingSecretReferences map[string]*loader.TargetSecretReference, latestSecretReferences map[string]*loader.TargetSecretReference, shouldReconcile bool) map[string]bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we let the method check secretUpdateNeeded one by one?

secretUpdateNeeded := make(map[string]bool)
if shouldReconcile {
for secretName := range latestSecretReferences {
secretUpdateNeeded[secretName] = true
}
return secretUpdateNeeded
}

for secretName, secretReference := range latestSecretReferences {
if _, ok := existingSecretReferences[secretName]; !ok {
secretUpdateNeeded[secretName] = true
continue
} else {
latestSecretReferences[secretName].SecretResourceVersion = existingSecretReferences[secretName].SecretResourceVersion
}

if len(existingSecretReferences[secretName].UriSegments) != len(secretReference.UriSegments) {
secretUpdateNeeded[secretName] = true
continue
}

for key, uriSegment := range secretReference.UriSegments {
if _, ok := existingSecretReferences[secretName].UriSegments[key]; !ok {
secretUpdateNeeded[secretName] = true
break
}
if existingSecretReferences[secretName].UriSegments[key].SecretId != nil && uriSegment.SecretId != nil &&
*(existingSecretReferences[secretName].UriSegments[key].SecretId) != *(uriSegment.SecretId) {
secretUpdateNeeded[secretName] = true
break
}
}
}

return secretUpdateNeeded
}
Loading
Loading