Skip to content

Commit

Permalink
Fix nil pointer when empty secret refresh (#79)
Browse files Browse the repository at this point in the history 
* fix nil pointer when not loaded KVR

* rename secretReference
  • Loading branch information
@linglingye001
linglingye001 committed Nov 8, 2024
1 parent 41fe55f commit 89ba290
Show file tree
Hide file tree
Showing 8 changed files with 196 additions and 198 deletions.
24 changes: 12 additions & 12 deletions internal/controller/appconfigurationprovider_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ type ReconciliationState struct {
SentinelETags map[acpv1.Sentinel]*azcore.ETag
KeyValueETags map[acpv1.Selector][]*azcore.ETag
FeatureFlagETags map[acpv1.Selector][]*azcore.ETag
ExistingSecretReferences map[string]*loader.TargetSecretReference
ExistingK8sSecrets map[string]*loader.TargetK8sSecretMetadata
NextKeyValueRefreshReconcileTime metav1.Time
NextSecretReferenceRefreshReconcileTime metav1.Time
NextFeatureFlagRefreshReconcileTime metav1.Time
Expand Down Expand Up @@ -158,7 +158,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
}

if reconciler.ProvidersReconcileState[req.NamespacedName] != nil {
for name := range reconciler.ProvidersReconcileState[req.NamespacedName].ExistingSecretReferences {
for name := range reconciler.ProvidersReconcileState[req.NamespacedName].ExistingK8sSecrets {
if _, ok := existingSecrets[name]; !ok {
existingSecret = corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -183,7 +183,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
SentinelETags: make(map[acpv1.Sentinel]*azcore.ETag),
KeyValueETags: make(map[acpv1.Selector][]*azcore.ETag),
FeatureFlagETags: make(map[acpv1.Selector][]*azcore.ETag),
ExistingSecretReferences: make(map[string]*loader.TargetSecretReference),
ExistingK8sSecrets: make(map[string]*loader.TargetK8sSecretMetadata),
ClientManager: nil,
}
}
Expand All @@ -194,11 +194,11 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
}

if provider.Spec.Secret == nil {
reconciler.ProvidersReconcileState[req.NamespacedName].ExistingSecretReferences = make(map[string]*loader.TargetSecretReference)
reconciler.ProvidersReconcileState[req.NamespacedName].ExistingK8sSecrets = make(map[string]*loader.TargetK8sSecretMetadata)
} else {
for name := range reconciler.ProvidersReconcileState[req.NamespacedName].ExistingSecretReferences {
for name := range reconciler.ProvidersReconcileState[req.NamespacedName].ExistingK8sSecrets {
if _, ok := existingSecrets[name]; !ok {
reconciler.ProvidersReconcileState[req.NamespacedName].ExistingSecretReferences[name].SecretResourceVersion = ""
reconciler.ProvidersReconcileState[req.NamespacedName].ExistingK8sSecrets[name].SecretResourceVersion = ""
}
}
}
Expand Down Expand Up @@ -281,7 +281,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context

// 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.Settings.SecretReferences)
result, err := reconciler.expelRemovedSecrets(ctx, provider, existingSecrets, processor.Settings.K8sSecrets)
if err != nil {
return result, nil
}
Expand Down Expand Up @@ -332,7 +332,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) logAndSetFailStatus(
} else if reconcileState != nil &&
reconcileState.ConfigMapResourceVersion != nil &&
(provider.Spec.Secret == nil ||
len(reconcileState.ExistingSecretReferences) == 0) {
len(reconcileState.ExistingK8sSecrets) == 0) {
// If the target ConfigMap or Secret does exists, just show error as warning.
showErrorAsWarning = true
}
Expand Down Expand Up @@ -431,8 +431,8 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets

for secretName, secret := range processor.Settings.SecretSettings {
if !shouldCreateOrUpdate(processor, secretName) {
if _, ok := reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretName]; ok {
processor.Settings.SecretReferences[secretName].SecretResourceVersion = reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences[secretName].SecretResourceVersion
if _, ok := reconciler.ProvidersReconcileState[namespacedName].ExistingK8sSecrets[secretName]; ok {
processor.Settings.K8sSecrets[secretName].SecretResourceVersion = reconciler.ProvidersReconcileState[namespacedName].ExistingK8sSecrets[secretName].SecretResourceVersion
}
klog.V(5).Infof("Skip updating the secret %q in %q namespace since data is not changed", secretName, provider.Namespace)
continue
Expand Down Expand Up @@ -465,7 +465,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) createOrUpdateSecrets
return reconcile.Result{Requeue: true, RequeueAfter: RequeueReconcileAfter}, err
}

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

Expand All @@ -476,7 +476,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) expelRemovedSecrets(
ctx context.Context,
provider *acpv1.AzureAppConfigurationProvider,
existingSecrets map[string]corev1.Secret,
secretReferences map[string]*loader.TargetSecretReference) (reconcile.Result, error) {
secretReferences map[string]*loader.TargetK8sSecretMetadata) (reconcile.Result, error) {
for name := range existingSecrets {
if _, ok := secretReferences[name]; !ok {
err := reconciler.Client.Delete(ctx, &corev1.Secret{
Expand Down
74 changes: 37 additions & 37 deletions internal/controller/appconfigurationprovider_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,14 @@ var _ = Describe("AppConfiguationProvider controller", func() {
Type: corev1.SecretTypeOpaque,
},
},
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretTypeTLS,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
Type: corev1.SecretTypeTLS,
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
secretName2: {
Type: corev1.SecretTypeOpaque,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
Type: corev1.SecretTypeOpaque,
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -262,10 +262,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
Type: corev1.SecretTypeOpaque,
},
},
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretTypeOpaque,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
Type: corev1.SecretTypeOpaque,
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -336,10 +336,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
},
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretType("Opaque"),
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
},
},
}
Expand Down Expand Up @@ -1121,10 +1121,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
},
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretType("Opaque"),
SecretsMetadata: secretMetadata,
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: secretMetadata,
},
},
}
Expand Down Expand Up @@ -1244,10 +1244,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
},
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretType("Opaque"),
SecretsMetadata: secretMetadata,
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: secretMetadata,
},
},
}
Expand Down Expand Up @@ -1370,10 +1370,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
},
ConfigMapSettings: configMapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretType("Opaque"),
SecretsMetadata: secretMetadata,
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: secretMetadata,
},
},
}
Expand Down Expand Up @@ -1454,15 +1454,15 @@ var _ = Describe("AppConfiguationProvider controller", func() {
newSecretMetadata["testSecretKey"] = loader.KeyVaultSecretMetadata{
SecretId: &newFakeId,
}
mockedSecretReference := make(map[string]*loader.TargetSecretReference)
mockedSecretReference[secretName] = &loader.TargetSecretReference{
Type: corev1.SecretType("Opaque"),
SecretsMetadata: newSecretMetadata,
mockedSecretReference := make(map[string]*loader.TargetK8sSecretMetadata)
mockedSecretReference[secretName] = &loader.TargetK8sSecretMetadata{
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: newSecretMetadata,
}

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

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings, nil)
Expand All @@ -1480,8 +1480,8 @@ var _ = Describe("AppConfiguationProvider controller", func() {

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

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings2, nil)
Expand Down Expand Up @@ -1810,10 +1810,10 @@ var _ = Describe("AppConfiguationProvider controller", func() {
},
},
ConfigMapSettings: mapResult,
SecretReferences: map[string]*loader.TargetSecretReference{
K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{
secretName: {
Type: corev1.SecretType("Opaque"),
SecretsMetadata: secretMetadata,
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: secretMetadata,
},
},
}
Expand Down Expand Up @@ -1939,15 +1939,15 @@ var _ = Describe("AppConfiguationProvider controller", func() {
newSecretMetadata["testSecretKey"] = loader.KeyVaultSecretMetadata{
SecretId: &newFakeId,
}
mockedSecretReference := make(map[string]*loader.TargetSecretReference)
mockedSecretReference[secretName] = &loader.TargetSecretReference{
Type: corev1.SecretType("Opaque"),
SecretsMetadata: newSecretMetadata,
mockedSecretReference := make(map[string]*loader.TargetK8sSecretMetadata)
mockedSecretReference[secretName] = &loader.TargetK8sSecretMetadata{
Type: corev1.SecretType("Opaque"),
SecretsKeyVaultMetadata: newSecretMetadata,
}

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

mockConfigurationSettings.EXPECT().ResolveSecretReferences(gomock.Any(), gomock.Any(), gomock.Any()).Return(newTargetSettings, nil)
Expand Down
44 changes: 21 additions & 23 deletions internal/controller/processor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,27 +226,25 @@ 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, secretMetadata := range reference.SecretsMetadata {
secretReferencesToSolve := make(map[string]*loader.TargetK8sSecretMetadata)
copiedSecretReferences := make(map[string]*loader.TargetK8sSecretMetadata)
for secretName, k8sSecret := range reconcileState.ExistingK8sSecrets {
copiedSecretReferences[secretName] = &loader.TargetK8sSecretMetadata{
Type: k8sSecret.Type,
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
}

for key, secretMetadata := range k8sSecret.SecretsKeyVaultMetadata {
if secretMetadata.SecretVersion == "" {
if secretReferencesToSolve[secretName] == nil {
secretReferencesToSolve[secretName] = &loader.TargetSecretReference{
Type: reference.Type,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
secretReferencesToSolve[secretName] = &loader.TargetK8sSecretMetadata{
Type: k8sSecret.Type,
SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata),
}
}
secretReferencesToSolve[secretName].SecretsMetadata[key] = secretMetadata
}

if copiedSecretReferences[secretName] == nil {
copiedSecretReferences[secretName] = &loader.TargetSecretReference{
Type: reference.Type,
SecretsMetadata: make(map[string]loader.KeyVaultSecretMetadata),
}
secretReferencesToSolve[secretName].SecretsKeyVaultMetadata[key] = secretMetadata
}
copiedSecretReferences[secretName].SecretsMetadata[key] = secretMetadata
copiedSecretReferences[secretName].SecretsKeyVaultMetadata[key] = secretMetadata
}
}

Expand All @@ -262,12 +260,12 @@ func (processor *AppConfigurationProviderProcessor) processSecretReferenceRefres
}
}

for secretName, reference := range resolvedSecrets.SecretReferences {
maps.Copy(copiedSecretReferences[secretName].SecretsMetadata, reference.SecretsMetadata)
for secretName, k8sSecret := range resolvedSecrets.K8sSecrets {
maps.Copy(copiedSecretReferences[secretName].SecretsKeyVaultMetadata, k8sSecret.SecretsKeyVaultMetadata)
}

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

// Update next refresh time only if settings updated successfully
Expand Down Expand Up @@ -295,13 +293,13 @@ func (processor *AppConfigurationProviderProcessor) shouldReconcile(
return false
}

if len(processor.ReconciliationState.ExistingSecretReferences) == 0 ||
len(processor.ReconciliationState.ExistingSecretReferences) != len(existingSecrets) {
if len(processor.ReconciliationState.ExistingK8sSecrets) == 0 ||
len(processor.ReconciliationState.ExistingK8sSecrets) != len(existingSecrets) {
return true
}

for name, secret := range existingSecrets {
if processor.ReconciliationState.ExistingSecretReferences[name].SecretResourceVersion != secret.ResourceVersion {
if processor.ReconciliationState.ExistingK8sSecrets[name].SecretResourceVersion != secret.ResourceVersion {
return true
}
}
Expand All @@ -313,7 +311,7 @@ func (processor *AppConfigurationProviderProcessor) Finish() (ctrl.Result, error
processor.ReconciliationState.Generation = processor.Provider.Generation

if processor.RefreshOptions.SecretSettingPopulated {
processor.ReconciliationState.ExistingSecretReferences = processor.Settings.SecretReferences
processor.ReconciliationState.ExistingK8sSecrets = processor.Settings.K8sSecrets
}

if processor.RefreshOptions.updatedKeyValueETags != nil {
Expand Down
Loading

0 comments on commit 89ba290

Please sign in to comment.