Skip to content

Commit

Permalink
Updated PR to satisfy comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaireddjawed
jaireddjawed committed Feb 12, 2025
1 parent 6fb9622 commit 299d06f
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 75 deletions.
45 changes: 36 additions & 9 deletions controllers/hcpvaultsecretsapp_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ func (r *HCPVaultSecretsAppReconciler) cleanupOrphanedShadowSecrets(ctx context.
nameLabelKey := hvsaLabelPrefix + "/name"

// filtering only for dynamic secrets, also checking if namespace and name labels are present
secrets := corev1.SecretList{}
secrets := secretsv1beta1.VaultDynamicSecretList{}
if err := r.List(ctx, &secrets, client.InNamespace(common.OperatorNamespace),
client.MatchingLabels{"app.kubernetes.io/component": "hvs-dynamic-secret-cache"},
client.HasLabels{namespaceLabelKey, nameLabelKey}); err != nil {
Expand All @@ -345,16 +345,30 @@ func (r *HCPVaultSecretsAppReconciler) cleanupOrphanedShadowSecrets(ctx context.
continue
}

// if the HCPVaultSecretsApp has been deleted, and the shadow secret belongs to it, delete both
// delete the HCPVaultSecretsApp and all resources associated with it the secret belongs to it
// and the HCPVaultSecretsApp has a deletion timestamp
if o.GetDeletionTimestamp() != nil && o.GetUID() == types.UID(secret.Labels[helpers.LabelOwnerRefUID]) {
if err := r.handleDeletion(ctx, o); err != nil {
errs = errors.Join(errs, fmt.Errorf("failed to handle deletion of HCPVaultSecretsApp %s: %w", o.Spec.AppName, err))
}

logger.Info("Deleted orphaned resources associated with HCPVaultSecretsApp", "app", o.Name)
} else if apierrors.IsNotFound(err) || secret.GetDeletionTimestamp() != nil {
// otherwise, delete the single shadow secret if it has a deletion timestamp
if err := helpers.DeleteSecret(ctx, r.Client, objKey); err != nil {
} else if apierrors.IsNotFound(err) && secret.GetDeletionTimestamp() != nil {
// otherwise, delete the shadow secret if we can't find the HCPVaultSecretsApp it belongs to and
// the shadow secret has a deletion timestamp
if controllerutil.ContainsFinalizer(&secret, vaultDynamicSecretFinalizer) {
logger.Info("Removing finalizer from shadow secret")
if controllerutil.RemoveFinalizer(&secret, vaultDynamicSecretFinalizer) {
if err := r.Update(ctx, &secret); err != nil {
errs = errors.Join(errs, fmt.Errorf("failed to remove the finalizer from shadow secret %s: %w", secret.Name, err))
continue
}

logger.Info("Successfully removed the finalizer from shadow secret")
}
}

if err := r.Delete(ctx, &secret); err != nil {
errs = errors.Join(errs, fmt.Errorf("failed to delete shadow secret %s: %w", secret.Name, err))
}

Expand Down Expand Up @@ -458,10 +472,20 @@ func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o *se
objKey := client.ObjectKeyFromObject(o)
r.referenceCache.Remove(SecretTransformation, objKey)
r.BackOffRegistry.Delete(objKey)
shadowObjKey := makeShadowObjKey(o)
if err := helpers.DeleteSecret(ctx, r.Client, shadowObjKey); err != nil {
logger.Error(err, "Failed to delete shadow secret", "shadow secret", shadowObjKey)
}
// retrieve all shadow secrets that belong to the HCPVaultSecretsApp, remove their finalizers, and delete them
secrets := secretsv1beta1.VaultDynamicSecretList{}
if err := r.List(ctx, &secrets,
client.InNamespace(common.OperatorNamespace),
client.MatchingLabels{helpers.LabelOwnerRefUID: string(o.GetUID())}); err != nil {
return fmt.Errorf("failed to list secrets in namespace %s: %w", o.GetNamespace(), err)
}
removeFinalizers(ctx, r.Client, logger, &secrets)
if err := r.DeleteAllOf(ctx, &secretsv1beta1.VaultDynamicSecret{},
client.InNamespace(common.OperatorNamespace),
client.MatchingLabels{helpers.LabelOwnerRefUID: string(o.GetUID())}); err != nil {
return fmt.Errorf("failed to delete secrets in namespace %s: %w", o.GetNamespace(), err)
}
// then remove the finalizer from the HCPVaultSecretsApp and delete it
if controllerutil.ContainsFinalizer(o, hcpVaultSecretsAppFinalizer) {
logger.Info("Removing finalizer")
if controllerutil.RemoveFinalizer(o, hcpVaultSecretsAppFinalizer) {
Expand All @@ -472,6 +496,9 @@ func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o *se
logger.Info("Successfully removed the finalizer")
}
}
if err := r.Delete(ctx, o); err != nil {
return fmt.Errorf("failed to delete HCPVaultSecretsApp %s: %w", o.Spec.AppName, err)
}
return nil
}

Expand Down
124 changes: 58 additions & 66 deletions controllers/hcpvaultsecretsapp_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/hashicorp/hcp-sdk-go/clients/cloud-vault-secrets/preview/2023-11-28/models"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -1230,111 +1229,103 @@ func Test_makeShadowObjKey(t *testing.T) {
}

func Test_CleanupOrphanedShadowSecrets(t *testing.T) {
deletionTimestamp := metav1.Now()
hvsApp := secretsv1beta1.HCPVaultSecretsApp{
TypeMeta: metav1.TypeMeta{
Kind: HCPVaultSecretsApp.String(),
APIVersion: secretsv1beta1.GroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
UID: "hvsAppUID",
Namespace: "hvsAppNamespace",
Name: "hvsApp",
Finalizers: []string{hcpVaultSecretsAppFinalizer},
},
}

tests := map[string]struct {
o *secretsv1beta1.HCPVaultSecretsApp
secret *corev1.Secret
secret *secretsv1beta1.VaultDynamicSecret
isHCPVaultSecretsAppDeletionExpected bool
isShadowSecretDeletionExpected bool
}{
"deleted-secret-hvsapp-owner": {
o: &secretsv1beta1.HCPVaultSecretsApp{
o: hvsApp.DeepCopy(),
secret: &secretsv1beta1.VaultDynamicSecret{
TypeMeta: metav1.TypeMeta{
Kind: HCPVaultSecretsApp.String(),
Kind: VaultDynamicSecret.String(),
APIVersion: secretsv1beta1.GroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
UID: "hvsApp1UID",
Namespace: "hvsApp1Namespace",
Name: "hvsApp1",
Finalizers: []string{hcpVaultSecretsAppFinalizer},
},
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: common.OperatorNamespace,
Name: "shadowSecret1",
DeletionTimestamp: &deletionTimestamp,
Finalizers: []string{vaultDynamicSecretFinalizer},
Namespace: common.OperatorNamespace,
Name: "shadowSecret",
Finalizers: []string{vaultDynamicSecretFinalizer},
Labels: map[string]string{
hvsaLabelPrefix + "/namespace": "hvsApp1Namespace",
hvsaLabelPrefix + "/name": "hvsApp1",
hvsaLabelPrefix + "/namespace": hvsApp.GetNamespace(),
hvsaLabelPrefix + "/name": hvsApp.GetName(),
helpers.LabelOwnerRefUID: string(hvsApp.GetUID()),
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
helpers.LabelOwnerRefUID: "hvsApp1UID",
},
},
},
isHCPVaultSecretsAppDeletionExpected: true,
isShadowSecretDeletionExpected: true,
},
"deleted-secret-hvsapp-not-owner": {
o: &secretsv1beta1.HCPVaultSecretsApp{
o: hvsApp.DeepCopy(),
secret: &secretsv1beta1.VaultDynamicSecret{
TypeMeta: metav1.TypeMeta{
Kind: HCPVaultSecretsApp.String(),
Kind: VaultDynamicSecret.String(),
APIVersion: secretsv1beta1.GroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
UID: "hvsApp2UID",
Namespace: "hvsApp2Namespace",
Name: "hvsApp2",
Finalizers: []string{hcpVaultSecretsAppFinalizer},
},
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: common.OperatorNamespace,
Name: "shadowSecret2",
DeletionTimestamp: &deletionTimestamp,
Finalizers: []string{vaultDynamicSecretFinalizer},
Namespace: common.OperatorNamespace,
Name: "shadowSecret",
Finalizers: []string{vaultDynamicSecretFinalizer},
Labels: map[string]string{
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
hvsaLabelPrefix + "/namespace": "",
hvsaLabelPrefix + "/name": "",
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
},
},
},
isShadowSecretDeletionExpected: true,
},
"deleted-secret-hvsapp-not-found": {
secret: &corev1.Secret{
secret: &secretsv1beta1.VaultDynamicSecret{
TypeMeta: metav1.TypeMeta{
Kind: VaultDynamicSecret.String(),
APIVersion: secretsv1beta1.GroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
Namespace: common.OperatorNamespace,
Name: "shadowSecret3",
Namespace: common.OperatorNamespace,
Name: "shadowSecret",
Finalizers: []string{vaultDynamicSecretFinalizer},
Labels: map[string]string{
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
hvsaLabelPrefix + "/namespace": "",
hvsaLabelPrefix + "/name": "",
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
},
DeletionTimestamp: &deletionTimestamp,
Finalizers: []string{hcpVaultSecretsAppFinalizer},
},
},
isShadowSecretDeletionExpected: true,
},
"secret-not-dynamic": {
o: &secretsv1beta1.HCPVaultSecretsApp{
"non-deleted-secret-hvsapp-not-found": {
secret: &secretsv1beta1.VaultDynamicSecret{
TypeMeta: metav1.TypeMeta{
Kind: HCPVaultSecretsApp.String(),
Kind: VaultDynamicSecret.String(),
APIVersion: secretsv1beta1.GroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
UID: "hvsApp4UID",
Namespace: "hvsApp4Namespace",
Name: "hvsApp4",
Finalizers: []string{hcpVaultSecretsAppFinalizer},
},
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: common.OperatorNamespace,
Name: "nonShadowSecret",
DeletionTimestamp: &deletionTimestamp,
Finalizers: []string{vaultDynamicSecretFinalizer},
Namespace: common.OperatorNamespace,
Name: "shadowSecret",
Finalizers: []string{vaultDynamicSecretFinalizer},
Labels: map[string]string{
hvsaLabelPrefix + "/namespace": "hvsApp4Namespace",
hvsaLabelPrefix + "/name": "hvsApp4",
helpers.LabelOwnerRefUID: "hvsApp4UID",
hvsaLabelPrefix + "/namespace": "",
hvsaLabelPrefix + "/name": "",
"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
},
},
},
isShadowSecretDeletionExpected: false,
},
}

Expand All @@ -1358,11 +1349,14 @@ func Test_CleanupOrphanedShadowSecrets(t *testing.T) {
// create the secret for the test case
assert.NoError(t, client.Create(ctx, tt.secret))

// DeleteTimestamp is a read-only field, so Delete will need to be called to
// simulate deletion of the HCPVaultSecretsApp
// DeleteTimestamp is a read-only field, so Delete() will need
// to be called to simulate deletion of both the HCPVaultSecretsApp and the secret
if tt.isHCPVaultSecretsAppDeletionExpected {
assert.NoError(t, client.Delete(ctx, tt.o))
}
if tt.isShadowSecretDeletionExpected {
assert.NoError(t, client.Delete(ctx, tt.secret))
}

r.cleanupOrphanedShadowSecrets(ctx)

Expand All @@ -1372,13 +1366,11 @@ func Test_CleanupOrphanedShadowSecrets(t *testing.T) {
assert.True(t, apierrors.IsNotFound(err))
}

secret := &secretsv1beta1.VaultDynamicSecret{}
err := r.Get(ctx, ctrlclient.ObjectKeyFromObject(tt.secret), secret)
if tt.isShadowSecretDeletionExpected {
deletedSecret := &corev1.Secret{}
err := r.Get(ctx, makeShadowObjKey(tt.secret), deletedSecret)
assert.True(t, apierrors.IsNotFound(err))
} else {
secret := &corev1.Secret{}
err := r.Get(ctx, ctrlclient.ObjectKeyFromObject(tt.secret), secret)
assert.False(t, apierrors.IsNotFound(err))
}
})
Expand Down

0 comments on commit 299d06f

Please sign in to comment.