Refresh secret when data change #53

linglingye001 · 2024-07-05T07:44:40Z

No description provided.

RichardChen820 · 2024-07-10T02:54:48Z

internal/controller/appconfigurationprovider_controller.go

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


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

internal/controller/appconfigurationprovider_controller_test.go

RichardChen820 · 2024-07-10T03:11:05Z

internal/loader/keyvault_reference_resolver.go

@@ -28,6 +28,7 @@ type KeyVaultSecretUriSegment struct {
 	HostName      string


This is no longer just indicates the UriSegment of the Secret, how about rename this type to KeyVaultSecretMetadata ?

RichardChen820 · 2024-07-10T03:15:46Z

internal/controller/processor.go

@@ -201,6 +210,7 @@ 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)


This variable seems useless, do we really need it?

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.

RichardChen820 · 2024-07-10T03:19:29Z

internal/controller/processor.go

@@ -332,3 +354,28 @@ func (processor *AppConfigurationProviderProcessor) calculateRequeueAfterInterva

 	return requeueAfterInterval
 }
+
+func checkSecretVersion(existingSecretReferences map[string]*loader.TargetSecretReference, latestSecretReferences map[string]*loader.TargetSecretReference) {


This method is not well self-explained, check secret version for what? I can learn what the method do after reading the implementation, but this is not a good design to a method. A good method is that you can know what the method is doing from the method name and input/output params.

RichardChen820 · 2024-07-10T03:27:17Z

internal/controller/processor.go

@@ -70,6 +70,9 @@ func (processor *AppConfigurationProviderProcessor) processFullReconciliation()
 		return err
 	}
 	processor.Settings = updatedSettings
+	for _, reference := range updatedSettings.SecretReferences {
+		reference.UpdateNeeded = true
+	}
 	processor.ReconciliationState.ExistingSecretReferences = updatedSettings.SecretReferences


Since the SecretReferences now involves the information of secret now, we should set it to ExistingSecretReferences in the Finish() step now, otherwise it might mismatch with the actual version in the K8s secret.

RichardChen820 · 2024-07-10T03:27:51Z

internal/controller/processor.go

+		if _, ok := processor.ReconciliationState.ExistingSecretReferences[secretName]; ok {
+			reference.SecretResourceVersion = processor.ReconciliationState.ExistingSecretReferences[secretName].SecretResourceVersion
+		}
+	}
 	reconcileState.ExistingSecretReferences = keyValueRefreshedSettings.SecretReferences


Same with what I comment on the line 73

RichardChen820 · 2024-07-10T03:34:12Z

internal/controller/processor.go

@@ -70,6 +70,9 @@ func (processor *AppConfigurationProviderProcessor) processFullReconciliation()
 		return err
 	}
 	processor.Settings = updatedSettings
+	for _, reference := range updatedSettings.SecretReferences {


Current all the version check are in the processor, result in we need to set the updateNeeded everywhere for the secret references, could we put the version check in the controller right before trying to update the Secret? I think that would be more clear.

RichardChen820 · 2024-07-17T09:01:29Z

internal/controller/processor.go

@@ -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)


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

yes, how about copiedSecretReferences?

RichardChen820 · 2024-07-17T09:06:33Z

internal/loader/configuration_setting_loader.go

@@ -50,7 +50,7 @@ type TargetKeyValueSettings struct {

 type TargetSecretReference struct {
 	Type                  corev1.SecretType
-	UriSegments           map[string]KeyVaultSecretUriSegment
+	UriSegments           map[string]KeyVaultSecretMetadata


The name UriSegments does not make sense now, how about updated to SecretMetadatas

internal/controller/processor.go

RichardChen820 · 2024-07-17T09:22:00Z

internal/controller/utils.go

@@ -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 {


Could we let the method check secretUpdateNeeded one by one?

RichardChen820 · 2024-07-18T00:18:19Z

internal/controller/appconfigurationprovider_controller.go

-			Type: secret.Type,
-		}
+	secretToUpdate := checkAndUpdateSecretRef(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences, processor.Settings.SecretReferences, processor.ShouldReconcile)
+	for secretName, secret := range processor.Settings.SecretSettings {


How about doing this:

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

internal/controller/appconfigurationprovider_controller.go

RichardChen820 · 2024-07-18T05:16:07Z

internal/controller/appconfigurationprovider_controller.go

+			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 its data is up-to-date", secretName, provider.Namespace)


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

RichardChen820 · 2024-07-18T05:17:18Z

internal/controller/appconfigurationprovider_controller.go

-			Type: secret.Type,
-		}
+	for secretName, secret := range processor.Settings.SecretSettings {
+		if processor.ShouldReconcile || shouldCreateOrUpdate(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences, secretName, processor.Settings.SecretReferences[secretName]) {


This line is too long, break it into two lines.

internal/controller/processor.go

RichardChen820 · 2024-07-18T05:35:40Z

internal/controller/appconfigurationprovider_controller.go

-			Type: secret.Type,
-		}
+	for secretName, secret := range processor.Settings.SecretSettings {
+		if processor.ShouldReconcile || shouldCreateOrUpdate(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences, secretName, processor.Settings.SecretReferences[secretName]) {


Nit: I know there's much difference in logic, but I think doing this is better for readability.

check shouldReconcile in the shouldCreateOrUpdate method

Change the logic to

if !shouldCreateOrUpdate(xxx) { // do something continue } // do something

RichardChen820 · 2024-07-18T05:41:59Z

internal/controller/appconfigurationprovider_controller_test.go

@@ -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")))
 		})
+


Nit: extra line

RichardChen820 · 2024-07-18T05:52:43Z

internal/controller/appconfigurationprovider_controller_test.go

+				SecretReferences: mockedSecretReference,
+			}
+
+			mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings, nil)


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?

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.

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

RichardChen820 · 2024-07-18T05:59:58Z

internal/controller/utils.go

@@ -302,3 +302,25 @@ func verifySelectorObject(selector acpv1.Selector) error {

 	return nil
 }
+
+func shouldCreateOrUpdate(existingSecretReferences map[string]*loader.TargetSecretReference, secretName string, secretReference *loader.TargetSecretReference) bool {


Why not pass the existing and target secretMetadata as input?

Target secretMetadata may not exist in existingSecretReferences map

RichardChen820 · 2024-07-18T06:05:04Z

internal/loader/configuration_setting_loader.go

@@ -50,7 +50,7 @@ type TargetKeyValueSettings struct {

 type TargetSecretReference struct {
 	Type                  corev1.SecretType
-	UriSegments           map[string]KeyVaultSecretUriSegment
+	SecretMetadata        map[string]KeyVaultSecretMetadata


SecretMetadatas, should use the plural

MetaData is plural noun, no need s

RichardChen820 · 2024-07-18T09:31:50Z

internal/controller/appconfigurationprovider_controller.go

@@ -425,7 +425,15 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
 		Namespace: provider.Namespace,
 	}

-	for secretName, secret := range settings.SecretSettings {
+	for secretName, secret := range processor.Settings.SecretSettings {
+		if !shouldCreateOrUpdate(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences, secretName, processor.Settings.SecretReferences[secretName], processor.ShouldReconcile) {


How about just pass the processor?

RichardChen820 · 2024-07-18T09:34:47Z

internal/controller/appconfigurationprovider_controller_test.go

+			}, timeout, interval).Should(BeTrue())
+
+			Expect(secret.Namespace).Should(Equal(ProviderNamespace))
+			Expect(string(secret.Data["testSecretKey"])).Should(Equal("newTestSecretValue"))


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

line746-763 include scenario when secretID not changed.

RichardChen820 · 2024-07-19T02:20:26Z

internal/loader/configuration_setting_loader.go

@@ -245,7 +245,7 @@ func (csl *ConfigurationSettingLoader) CreateKeyValueSettings(ctx context.Contex
 			}

 			currentUrl := *setting.Value
-			secretUriSegment, err := parse(currentUrl)
+			secretMetadata, err := parse(currentUrl)


This line and line200 are all using the secretMatadata word, that indeed causes confusion. I would suggest SecretMatadatas(regardless the plural and singular same form) or SecretsMetadata in TargetSecretReference

RichardChen820 · 2024-07-19T02:48:07Z

internal/controller/processor.go

 	for secretName, reference := range reconcileState.ExistingSecretReferences {
-		for key, uriSegment := range reference.UriSegments {
+		for key, uriSegment := range reference.SecretsMetadata {


uriSegment -> secretMetadata

RichardChen820 · 2024-07-19T02:49:46Z

internal/controller/utils.go

+		if _, ok := existingSecretReferences[secretName].SecretsMetadata[key]; !ok {
+			return true
+		}
+		if existingSecretReferences[secretName].SecretsMetadata[key].SecretId != nil && secretMetadata.SecretId != nil &&


Break this long check into multiple lines

internal/loader/configuration_setting_loader.go

RichardChen820 · 2024-07-19T03:35:11Z

internal/loader/configuration_setting_loader.go

@@ -386,7 +387,10 @@ func (csl *ConfigurationSettingLoader) ResolveSecretReferences(
 						}
 						lock.Lock()
 						defer lock.Unlock()
-						resolvedSecretReferences[name].Data[currentKey] = []byte(*resolvedSecret.Value)
+						resolvedSecrets[name].Data[currentKey] = []byte(*resolvedSecret.Value)
+						currentUriSegment := targetSecretReference.SecretMetadata[currentKey]


This name is not accurate now, currentSecretMetadata

* resolve confict * update * resolve comments * add test * update * update * update * rename secretMetadata to secretsMetadata * update

resolve confict

c77306b

linglingye001 force-pushed the user/linglingye/refreshSecret branch from 637f5ae to c77306b Compare July 5, 2024 08:07

update

9b4dc35

linglingye001 marked this pull request as ready for review July 9, 2024 07:39

RichardChen820 reviewed Jul 10, 2024

View reviewed changes

internal/controller/appconfigurationprovider_controller_test.go Show resolved Hide resolved

RichardChen820 reviewed Jul 10, 2024

View reviewed changes

linglingye001 added 2 commits July 15, 2024 09:31

resolve comments

e006f09

add test

6b64225

RichardChen820 reviewed Jul 17, 2024

View reviewed changes

internal/controller/processor.go Show resolved Hide resolved

RichardChen820 reviewed Jul 17, 2024

View reviewed changes

internal/controller/processor.go Show resolved Hide resolved

RichardChen820 reviewed Jul 17, 2024

View reviewed changes

RichardChen820 reviewed Jul 18, 2024

View reviewed changes

update

d5ca9a3

RichardChen820 reviewed Jul 18, 2024

View reviewed changes

internal/controller/appconfigurationprovider_controller.go Show resolved Hide resolved

RichardChen820 reviewed Jul 18, 2024

View reviewed changes

internal/controller/processor.go Show resolved Hide resolved

RichardChen820 reviewed Jul 18, 2024

View reviewed changes

update

7fdc735

RichardChen820 reviewed Jul 18, 2024

View reviewed changes

update

030d112

RichardChen820 reviewed Jul 19, 2024

View reviewed changes

rename secretMetadata to secretsMetadata

4d0ffb2

RichardChen820 reviewed Jul 19, 2024

View reviewed changes

internal/loader/configuration_setting_loader.go Outdated Show resolved Hide resolved

update

8fee3ea

RichardChen820 reviewed Jul 19, 2024

View reviewed changes

linglingye001 changed the base branch from release/v2/preview to release/v2/stable July 22, 2024 08:09

RichardChen820 approved these changes Jul 22, 2024

View reviewed changes

linglingye001 merged commit 0d28b27 into release/v2/stable Jul 22, 2024
3 checks passed

linglingye001 deleted the user/linglingye/refreshSecret branch July 22, 2024 08:20

linglingye001 added a commit that referenced this pull request Nov 8, 2024

Refresh secret when data change (#53)

92e7a79

* resolve confict * update * resolve comments * add test * update * update * update * rename secretMetadata to secretsMetadata * update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh secret when data change #53

Refresh secret when data change #53

linglingye001 commented Jul 5, 2024

RichardChen820 Jul 10, 2024

RichardChen820 Jul 10, 2024 •

edited

Loading

RichardChen820 Jul 10, 2024

linglingye001 Jul 11, 2024

RichardChen820 Jul 10, 2024 •

edited

Loading

RichardChen820 Jul 10, 2024

RichardChen820 Jul 10, 2024

RichardChen820 Jul 10, 2024

RichardChen820 Jul 17, 2024

linglingye001 Jul 18, 2024

RichardChen820 Jul 17, 2024

RichardChen820 Jul 17, 2024

RichardChen820 Jul 18, 2024

RichardChen820 Jul 18, 2024

RichardChen820 Jul 18, 2024

RichardChen820 Jul 18, 2024 •

edited

Loading

RichardChen820 Jul 18, 2024

RichardChen820 Jul 18, 2024

linglingye001 Jul 18, 2024

RichardChen820 Jul 18, 2024

RichardChen820 Jul 18, 2024

linglingye001 Jul 18, 2024

RichardChen820 Jul 18, 2024 •

edited

Loading

linglingye001 Jul 18, 2024

RichardChen820 Jul 18, 2024 •

edited

Loading

RichardChen820 Jul 18, 2024

linglingye001 Jul 19, 2024

RichardChen820 Jul 19, 2024

RichardChen820 Jul 19, 2024

RichardChen820 Jul 19, 2024

RichardChen820 Jul 19, 2024

		@@ -28,6 +28,7 @@ type KeyVaultSecretUriSegment struct {
		HostName string

Refresh secret when data change #53

Refresh secret when data change #53

Conversation

linglingye001 commented Jul 5, 2024

Choose a reason for hiding this comment

RichardChen820 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 10, 2024 •

edited

Loading

RichardChen820 Jul 10, 2024 •

edited

Loading

RichardChen820 Jul 18, 2024 •

edited

Loading

RichardChen820 Jul 18, 2024 •

edited

Loading

RichardChen820 Jul 18, 2024 •

edited

Loading