Skip to content

Commit f99c57d

Browse files
pkoutsovasilispkoutsovasilis
committed
fix: cleanup orphaned secret mounts when removed from StackConfigPolicy
1 parent 8b21497 commit f99c57d

File tree

2 files changed

+217
-0
lines changed

2 files changed

+217
-0
lines changed

‎pkg/controller/stackconfigpolicy/controller_test.go‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,170 @@ func TestReconcileStackConfigPolicy_Reconcile(t *testing.T) {
709709
wantErr: false,
710710
wantRequeueAfter: true,
711711
},
712+
{
713+
name: "Secret mount is removed when policy is updated to remove it from SecretMounts",
714+
args: args{
715+
client: func() k8s.Client {
716+
// Simulates a policy that previously had 2 SecretMounts (test-secret-mount, another-secret-mount)
717+
// but was updated to only have 1 (test-secret-mount). The copied secrets from the previous
718+
// reconciliation still exist, and we expect the cleanup logic to remove the orphaned one.
719+
policyWithOneMount := policyFixture.DeepCopy()
720+
policyWithOneMount.Spec.Elasticsearch.SecretMounts = []policyv1alpha1.SecretMount{
721+
{
722+
SecretName: "test-secret-mount",
723+
MountPath: "/usr/test",
724+
},
725+
}
726+
727+
// Source secrets in policy namespace (these don't need owner labels, they're just data sources)
728+
sourceSecret1 := &corev1.Secret{
729+
ObjectMeta: metav1.ObjectMeta{
730+
Name: "test-secret-mount",
731+
Namespace: "ns",
732+
},
733+
Data: map[string][]byte{
734+
"idfile.txt": []byte("test id file"),
735+
},
736+
}
737+
sourceSecret2 := &corev1.Secret{
738+
ObjectMeta: metav1.ObjectMeta{
739+
Name: "another-secret-mount",
740+
Namespace: "ns",
741+
},
742+
Data: map[string][]byte{
743+
"another-file.txt": []byte("another test file"),
744+
},
745+
}
746+
747+
// Copied secrets in ES namespace (created by previous reconciliation)
748+
// These have the source secret annotation - should be eligible for cleanup
749+
copiedSecret1 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), "ns", "test-policy", "ns", "delete")
750+
copiedSecret1.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
751+
copiedSecret1.Annotations = map[string]string{
752+
"policy.k8s.elastic.co/source-secret-name": "test-secret-mount",
753+
}
754+
copiedSecret2 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), "ns", "test-policy", "ns", "delete")
755+
copiedSecret2.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
756+
copiedSecret2.Annotations = map[string]string{
757+
"policy.k8s.elastic.co/source-secret-name": "another-secret-mount",
758+
}
759+
760+
// Create a third secret owned by the policy but WITHOUT the source secret annotation
761+
// This should NOT be deleted even though it's not in SecretMounts
762+
secretWithoutAnnotation := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), "ns", "test-policy", "ns", "delete")
763+
secretWithoutAnnotation.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
764+
765+
return k8s.NewFakeClient(policyWithOneMount, &esFixture, &secretFixture, sourceSecret1, sourceSecret2, copiedSecret1, copiedSecret2, secretWithoutAnnotation, esPodFixture)
766+
}(),
767+
licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true},
768+
esClientProvider: fakeClientProvider(clusterStateFileSettingsFixture(42, nil), nil),
769+
},
770+
post: func(r ReconcileStackConfigPolicy, recorder record.FakeRecorder) {
771+
// Verify the first secret exists (reconciled because still in SecretMounts)
772+
var copiedSecret1 corev1.Secret
773+
err := r.Client.Get(context.Background(), types.NamespacedName{
774+
Name: esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"),
775+
Namespace: "ns",
776+
}, &copiedSecret1)
777+
assert.NoError(t, err, "first copied secret should exist (still in SecretMounts)")
778+
779+
// Verify the second secret was deleted (removed from SecretMounts and has source annotation)
780+
var copiedSecret2 corev1.Secret
781+
err = r.Client.Get(context.Background(), types.NamespacedName{
782+
Name: esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"),
783+
Namespace: "ns",
784+
}, &copiedSecret2)
785+
assert.True(t, apierrors.IsNotFound(err), "second copied secret should be deleted (removed from SecretMounts)")
786+
787+
// Verify the secret without annotation was NOT deleted (lacks source secret annotation)
788+
var secretWithoutAnnotation corev1.Secret
789+
err = r.Client.Get(context.Background(), types.NamespacedName{
790+
Name: esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"),
791+
Namespace: "ns",
792+
}, &secretWithoutAnnotation)
793+
assert.NoError(t, err, "secret without source annotation should NOT be deleted")
794+
},
795+
wantErr: false,
796+
},
797+
{
798+
name: "All secret mounts removed from policy - all copied secrets should be deleted",
799+
args: args{
800+
client: func() k8s.Client {
801+
// Simulates a policy that previously had SecretMounts but was updated to have none.
802+
// All previously copied secrets with source annotations should be cleaned up.
803+
policyWithNoMounts := policyFixture.DeepCopy()
804+
policyWithNoMounts.Spec.Elasticsearch.SecretMounts = []policyv1alpha1.SecretMount{}
805+
806+
// Source secrets in policy namespace (left over from before, not currently used)
807+
sourceSecret1 := &corev1.Secret{
808+
ObjectMeta: metav1.ObjectMeta{
809+
Name: "test-secret-mount",
810+
Namespace: "ns",
811+
},
812+
Data: map[string][]byte{
813+
"idfile.txt": []byte("test id file"),
814+
},
815+
}
816+
sourceSecret2 := &corev1.Secret{
817+
ObjectMeta: metav1.ObjectMeta{
818+
Name: "another-secret-mount",
819+
Namespace: "ns",
820+
},
821+
Data: map[string][]byte{
822+
"another-file.txt": []byte("another test file"),
823+
},
824+
}
825+
826+
// Copied secrets in ES namespace (created by previous reconciliation)
827+
// Both have the source secret annotation and should be cleaned up
828+
copiedSecret1 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), "ns", "test-policy", "ns", "delete")
829+
copiedSecret1.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
830+
copiedSecret1.Annotations = map[string]string{
831+
"policy.k8s.elastic.co/source-secret-name": "test-secret-mount",
832+
}
833+
copiedSecret2 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), "ns", "test-policy", "ns", "delete")
834+
copiedSecret2.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
835+
copiedSecret2.Annotations = map[string]string{
836+
"policy.k8s.elastic.co/source-secret-name": "another-secret-mount",
837+
}
838+
839+
// Create a secret owned by the policy but WITHOUT the source secret annotation
840+
// This should NOT be deleted (not a SecretMount-managed secret)
841+
secretWithoutAnnotation := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), "ns", "test-policy", "ns", "delete")
842+
secretWithoutAnnotation.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es"
843+
844+
return k8s.NewFakeClient(policyWithNoMounts, &esFixture, &secretFixture, sourceSecret1, sourceSecret2, copiedSecret1, copiedSecret2, secretWithoutAnnotation, esPodFixture)
845+
}(),
846+
licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true},
847+
esClientProvider: fakeClientProvider(clusterStateFileSettingsFixture(42, nil), nil),
848+
},
849+
post: func(r ReconcileStackConfigPolicy, recorder record.FakeRecorder) {
850+
// Verify both copied secrets with source annotations were deleted
851+
var copiedSecret1 corev1.Secret
852+
err := r.Client.Get(context.Background(), types.NamespacedName{
853+
Name: esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"),
854+
Namespace: "ns",
855+
}, &copiedSecret1)
856+
assert.True(t, apierrors.IsNotFound(err), "first copied secret should be deleted (all SecretMounts removed)")
857+
858+
var copiedSecret2 corev1.Secret
859+
err = r.Client.Get(context.Background(), types.NamespacedName{
860+
Name: esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"),
861+
Namespace: "ns",
862+
}, &copiedSecret2)
863+
assert.True(t, apierrors.IsNotFound(err), "second copied secret should be deleted (all SecretMounts removed)")
864+
865+
// Verify the secret without annotation was NOT deleted
866+
var secretWithoutAnnotation corev1.Secret
867+
err = r.Client.Get(context.Background(), types.NamespacedName{
868+
Name: esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"),
869+
Namespace: "ns",
870+
}, &secretWithoutAnnotation)
871+
assert.NoError(t, err, "secret without source annotation should NOT be deleted")
872+
},
873+
wantErr: false,
874+
wantRequeueAfter: true,
875+
},
712876
}
713877

714878
for _, tt := range tests {

‎pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"context"
99
"encoding/json"
1010

11+
"k8s.io/apimachinery/pkg/util/sets"
1112
"sigs.k8s.io/controller-runtime/pkg/client"
1213

1314
"github.com/elastic/cloud-on-k8s/v3/pkg/controller/common/metadata"
@@ -24,6 +25,7 @@ import (
2425
"github.com/elastic/cloud-on-k8s/v3/pkg/utils/k8s"
2526

2627
corev1 "k8s.io/api/core/v1"
28+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2729
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2830
"k8s.io/apimachinery/pkg/types"
2931
)
@@ -77,7 +79,11 @@ func newElasticsearchConfigSecret(policy policyv1alpha1.StackConfigPolicy, es es
7779
}
7880

7981
// reconcileSecretMounts creates the secrets in SecretMounts to the respective Elasticsearch namespace where they should be mounted to.
82+
// It also removes any previously mounted secrets that are no longer in policy.Spec.Elasticsearch.SecretMounts.
8083
func reconcileSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsearch, policy *policyv1alpha1.StackConfigPolicy, meta metadata.Metadata) error {
84+
// Track which secret mounts are defined in the policy (by their target secret name in ES namespace)
85+
secretMountsInPolicy := make(sets.Set[string], len(policy.Spec.Elasticsearch.SecretMounts))
86+
8187
for _, secretMount := range policy.Spec.Elasticsearch.SecretMounts {
8288
additionalSecret := corev1.Secret{}
8389
namespacedName := types.NamespacedName{
@@ -114,6 +120,53 @@ func reconcileSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsea
114120
if _, err := reconciler.ReconcileSecret(ctx, c, expected, nil); err != nil {
115121
return err
116122
}
123+
124+
secretMountsInPolicy.Insert(secretName)
125+
}
126+
127+
// Clean up secret mounts that are no longer in the policy
128+
return cleanupOrphanedSecretMounts(ctx, c, es, k8s.ExtractNamespacedName(policy), secretMountsInPolicy)
129+
}
130+
131+
// cleanupOrphanedSecretMounts removes copied secrets that are owned by the given policy and do not exist in the given
132+
// secretMountsInPolicy set. This handles the case where a policy is updated in-place and SecretMounts list is affected.
133+
// See https://github.com/elastic/cloud-on-k8s/issues/8921
134+
func cleanupOrphanedSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsearch, policyNsn types.NamespacedName, secretMountsInPolicy sets.Set[string]) error {
135+
// List all secrets in the ES namespace that are soft-owned by this policy.
136+
// The label selector below ensures we only find secrets that: (1) are soft-owned
137+
// by this specific policy, (2) are marked for deletion when the policy is deleted,
138+
// and (3) belong to this specific ES cluster.
139+
var secrets corev1.SecretList
140+
matchLabels := client.MatchingLabels{
141+
reconciler.SoftOwnerKindLabel: policyv1alpha1.Kind,
142+
reconciler.SoftOwnerNameLabel: policyNsn.Name,
143+
reconciler.SoftOwnerNamespaceLabel: policyNsn.Namespace,
144+
commonlabels.StackConfigPolicyOnDeleteLabelName: commonlabels.OrphanSecretDeleteOnPolicyDelete,
145+
eslabel.ClusterNameLabelName: es.Name,
146+
}
147+
148+
if err := c.List(ctx, &secrets, client.InNamespace(es.Namespace), matchLabels); err != nil {
149+
return err
150+
}
151+
152+
for i := range secrets.Items {
153+
secret := &secrets.Items[i]
154+
155+
// Skip secrets that do not have a source secret annotation
156+
if secret.Annotations[commonannotation.SourceSecretAnnotationName] == "" {
157+
continue
158+
}
159+
160+
// Check if this secret is in the expected set (still in SecretMounts)
161+
if secretMountsInPolicy.Has(secret.Name) {
162+
continue
163+
}
164+
165+
// This secret is owned by the policy but no longer in SecretMounts - delete it
166+
// Since these are single-owner secrets (filtered by labels), we can delete directly
167+
if err := c.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) {
168+
return err
169+
}
117170
}
118171
return nil
119172
}

0 commit comments

Comments
 (0)