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 all 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
20 changes: 14 additions & 6 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,7 +425,15 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
Namespace: provider.Namespace,
}

for secretName, secret := range settings.SecretSettings {
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 !shouldCreateOrUpdate(processor, secretName) {
if _, ok := reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretName]; ok {
processor.Settings.SecretReferences[secretName].SecretResourceVersion = reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretName].SecretResourceVersion
}
klog.V(5).Infof("Skip updating the secret %q in %q namespace since data is not changed", secretName, provider.Namespace)
continue
}

secretObj := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Expand Down Expand Up @@ -453,7 +461,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}
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.


reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretObj.Name].SecretResourceVersion = secretObj.ResourceVersion
processor.Settings.SecretReferences[secretName].SecretResourceVersion = secretObj.ResourceVersion
klog.V(5).Infof("Secret %q in %q namespace is %s", secretObj.Name, secretObj.Namespace, string(operationResult))
}

Expand Down
70 changes: 52 additions & 18 deletions internal/controller/appconfigurationprovider_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

acpv1 "azappconfig/provider/api/v1"

"github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets"
"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -172,12 +173,12 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretTypeTLS,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
Type: corev1.SecretTypeTLS,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
secretName2: {
Type: corev1.SecretTypeOpaque,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
Type: corev1.SecretTypeOpaque,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -265,8 +266,8 @@ var _ = Describe("AppConfiguationProvider controller", func() {
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretType("Opaque"),
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
Type: corev1.SecretType("Opaque"),
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -609,7 +610,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 @@ -618,10 +619,14 @@ var _ = Describe("AppConfiguationProvider controller", func() {

secretResult := make(map[string][]byte)
secretResult["testSecretKey"] = []byte("testSecretValue")
secretResult["testSecretKey2"] = []byte("testSecretValue2")
secretResult["testSecretKey3"] = []byte("testSecretValue3")

secretName := "secret-to-be-refreshed-3"
var fakeId azsecrets.ID = "fakeSecretId"
secretMetadata := make(map[string]loader.KeyVaultSecretMetadata)
secretMetadata["testSecretKey"] = loader.KeyVaultSecretMetadata{
SecretId: &fakeId,
}

allSettings := &loader.TargetKeyValueSettings{
SecretSettings: map[string]corev1.Secret{
secretName: {
Expand All @@ -632,8 +637,8 @@ var _ = Describe("AppConfiguationProvider controller", func() {
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
secretName: {
Type: corev1.SecretType("Opaque"),
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
Type: corev1.SecretType("Opaque"),
SecretsMetadata: secretMetadata,
},
},
}
Expand Down Expand Up @@ -697,14 +702,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {

Expect(secret.Namespace).Should(Equal(ProviderNamespace))
Expect(string(secret.Data["testSecretKey"])).Should(Equal("testSecretValue"))
Expect(string(secret.Data["testSecretKey2"])).Should(Equal("testSecretValue2"))
Expect(string(secret.Data["testSecretKey3"])).Should(Equal("testSecretValue3"))
Expect(secret.Type).Should(Equal(corev1.SecretType("Opaque")))

newSecretResult := make(map[string][]byte)
newSecretResult["testSecretKey"] = []byte("newTestSecretValue")
newSecretResult["testSecretKey2"] = []byte("newTestSecretValue2")
newSecretResult["testSecretKey3"] = []byte("newTestSecretValue3")

newResolvedSecret := map[string]corev1.Secret{
secretName: {
Expand All @@ -713,7 +714,42 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
}

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newResolvedSecret, nil)
var newFakeId azsecrets.ID = "newFakeSecretId"
newSecretMetadata := make(map[string]loader.KeyVaultSecretMetadata)
newSecretMetadata["testSecretKey"] = loader.KeyVaultSecretMetadata{
SecretId: &newFakeId,
}
mockedSecretReference := make(map[string]*loader.TargetSecretReference)
mockedSecretReference[secretName] = &loader.TargetSecretReference{
Type: corev1.SecretType("Opaque"),
SecretsMetadata: 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(secret.Type).Should(Equal(corev1.SecretType("Opaque")))

// Mocked secret refresh scenario when secretMetadata is not changed
newTargetSettings2 := &loader.TargetKeyValueSettings{
SecretSettings: make(map[string]corev1.Secret),
SecretReferences: mockedSecretReference,
}

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

Expand All @@ -724,8 +760,6 @@ var _ = Describe("AppConfiguationProvider controller", func() {

Expect(secret.Namespace).Should(Equal(ProviderNamespace))
Expect(string(secret.Data["testSecretKey"])).Should(Equal("newTestSecretValue"))
Expect(string(secret.Data["testSecretKey2"])).Should(Equal("newTestSecretValue2"))
Expect(string(secret.Data["testSecretKey3"])).Should(Equal("newTestSecretValue3"))
Expect(secret.Type).Should(Equal(corev1.SecretType("Opaque")))
})
})
Expand Down
34 changes: 26 additions & 8 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)
copiedSecretReferences := make(map[string]*loader.TargetSecretReference)
for secretName, reference := range reconcileState.ExistingSecretReferences {
for key, uriSegment := range reference.UriSegments {
if uriSegment.SecretVersion == "" {
for key, secretMetadata := range reference.SecretsMetadata {
if secretMetadata.SecretVersion == "" {
if secretReferencesToSolve[secretName] == nil {
secretReferencesToSolve[secretName] = &loader.TargetSecretReference{
Type: reference.Type,
UriSegments: make(map[string]loader.KeyVaultSecretUriSegment),
Type: reference.Type,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
}
}
secretReferencesToSolve[secretName].UriSegments[key] = uriSegment
secretReferencesToSolve[secretName].SecretsMetadata[key] = secretMetadata
}

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

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(copiedSecretReferences[secretName].SecretsMetadata, reference.SecretsMetadata)
}

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

// Update next refresh time only if settings updated successfully
Expand Down Expand Up @@ -270,6 +283,11 @@ 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
29 changes: 29 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,32 @@ func verifySelectorObject(selector acpv1.Selector) error {

return nil
}

func shouldCreateOrUpdate(processor *AppConfigurationProviderProcessor, secretName string) bool {
if processor.ShouldReconcile {
return true
}

existingSecretReferences := processor.ReconciliationState.ExistingSecretReferences
if _, ok := existingSecretReferences[secretName]; !ok {
return true
}

secretReference := processor.Settings.SecretReferences[secretName]
if len(existingSecretReferences[secretName].SecretsMetadata) != len(secretReference.SecretsMetadata) {
return true
}

for key, secretMetadata := range secretReference.SecretsMetadata {
if _, ok := existingSecretReferences[secretName].SecretsMetadata[key]; !ok {
return true
}
if secretMetadata.SecretId != nil &&
existingSecretReferences[secretName].SecretsMetadata[key].SecretId != nil &&
*(existingSecretReferences[secretName].SecretsMetadata[key].SecretId) != *(secretMetadata.SecretId) {
return true
}
}

return false
}
Loading
Loading