diff --git a/pkg/controller/stackconfigpolicy/controller_test.go b/pkg/controller/stackconfigpolicy/controller_test.go index 0ebbfbcc9c..c8804fbc14 100644 --- a/pkg/controller/stackconfigpolicy/controller_test.go +++ b/pkg/controller/stackconfigpolicy/controller_test.go @@ -709,6 +709,170 @@ func TestReconcileStackConfigPolicy_Reconcile(t *testing.T) { wantErr: false, wantRequeueAfter: true, }, + { + name: "Secret mount is removed when policy is updated to remove it from SecretMounts", + args: args{ + client: func() k8s.Client { + // Simulates a policy that previously had 2 SecretMounts (test-secret-mount, another-secret-mount) + // but was updated to only have 1 (test-secret-mount). The copied secrets from the previous + // reconciliation still exist, and we expect the cleanup logic to remove the orphaned one. + policyWithOneMount := policyFixture.DeepCopy() + policyWithOneMount.Spec.Elasticsearch.SecretMounts = []policyv1alpha1.SecretMount{ + { + SecretName: "test-secret-mount", + MountPath: "/usr/test", + }, + } + + // Source secrets in policy namespace (these don't need owner labels, they're just data sources) + sourceSecret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-mount", + Namespace: "ns", + }, + Data: map[string][]byte{ + "idfile.txt": []byte("test id file"), + }, + } + sourceSecret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "another-secret-mount", + Namespace: "ns", + }, + Data: map[string][]byte{ + "another-file.txt": []byte("another test file"), + }, + } + + // Copied secrets in ES namespace (created by previous reconciliation) + // These have the source secret annotation - should be eligible for cleanup + copiedSecret1 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), "ns", "test-policy", "ns", "delete") + copiedSecret1.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + copiedSecret1.Annotations = map[string]string{ + "policy.k8s.elastic.co/source-secret-name": "test-secret-mount", + } + copiedSecret2 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), "ns", "test-policy", "ns", "delete") + copiedSecret2.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + copiedSecret2.Annotations = map[string]string{ + "policy.k8s.elastic.co/source-secret-name": "another-secret-mount", + } + + // Create a third secret owned by the policy but WITHOUT the source secret annotation + // This should NOT be deleted even though it's not in SecretMounts + secretWithoutAnnotation := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), "ns", "test-policy", "ns", "delete") + secretWithoutAnnotation.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + + return k8s.NewFakeClient(policyWithOneMount, &esFixture, &secretFixture, sourceSecret1, sourceSecret2, copiedSecret1, copiedSecret2, secretWithoutAnnotation, esPodFixture) + }(), + licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true}, + esClientProvider: fakeClientProvider(clusterStateFileSettingsFixture(42, nil), nil), + }, + post: func(r ReconcileStackConfigPolicy, recorder record.FakeRecorder) { + // Verify the first secret exists (reconciled because still in SecretMounts) + var copiedSecret1 corev1.Secret + err := r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), + Namespace: "ns", + }, &copiedSecret1) + assert.NoError(t, err, "first copied secret should exist (still in SecretMounts)") + + // Verify the second secret was deleted (removed from SecretMounts and has source annotation) + var copiedSecret2 corev1.Secret + err = r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), + Namespace: "ns", + }, &copiedSecret2) + assert.True(t, apierrors.IsNotFound(err), "second copied secret should be deleted (removed from SecretMounts)") + + // Verify the secret without annotation was NOT deleted (lacks source secret annotation) + var secretWithoutAnnotation corev1.Secret + err = r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), + Namespace: "ns", + }, &secretWithoutAnnotation) + assert.NoError(t, err, "secret without source annotation should NOT be deleted") + }, + wantErr: false, + }, + { + name: "All secret mounts removed from policy - all copied secrets should be deleted", + args: args{ + client: func() k8s.Client { + // Simulates a policy that previously had SecretMounts but was updated to have none. + // All previously copied secrets with source annotations should be cleaned up. + policyWithNoMounts := policyFixture.DeepCopy() + policyWithNoMounts.Spec.Elasticsearch.SecretMounts = []policyv1alpha1.SecretMount{} + + // Source secrets in policy namespace (left over from before, not currently used) + sourceSecret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret-mount", + Namespace: "ns", + }, + Data: map[string][]byte{ + "idfile.txt": []byte("test id file"), + }, + } + sourceSecret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "another-secret-mount", + Namespace: "ns", + }, + Data: map[string][]byte{ + "another-file.txt": []byte("another test file"), + }, + } + + // Copied secrets in ES namespace (created by previous reconciliation) + // Both have the source secret annotation and should be cleaned up + copiedSecret1 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), "ns", "test-policy", "ns", "delete") + copiedSecret1.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + copiedSecret1.Annotations = map[string]string{ + "policy.k8s.elastic.co/source-secret-name": "test-secret-mount", + } + copiedSecret2 := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), "ns", "test-policy", "ns", "delete") + copiedSecret2.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + copiedSecret2.Annotations = map[string]string{ + "policy.k8s.elastic.co/source-secret-name": "another-secret-mount", + } + + // Create a secret owned by the policy but WITHOUT the source secret annotation + // This should NOT be deleted (not a SecretMount-managed secret) + secretWithoutAnnotation := getSecretMountSecret(t, esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), "ns", "test-policy", "ns", "delete") + secretWithoutAnnotation.Labels["elasticsearch.k8s.elastic.co/cluster-name"] = "test-es" + + return k8s.NewFakeClient(policyWithNoMounts, &esFixture, &secretFixture, sourceSecret1, sourceSecret2, copiedSecret1, copiedSecret2, secretWithoutAnnotation, esPodFixture) + }(), + licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true}, + esClientProvider: fakeClientProvider(clusterStateFileSettingsFixture(42, nil), nil), + }, + post: func(r ReconcileStackConfigPolicy, recorder record.FakeRecorder) { + // Verify both copied secrets with source annotations were deleted + var copiedSecret1 corev1.Secret + err := r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "test-secret-mount"), + Namespace: "ns", + }, &copiedSecret1) + assert.True(t, apierrors.IsNotFound(err), "first copied secret should be deleted (all SecretMounts removed)") + + var copiedSecret2 corev1.Secret + err = r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "another-secret-mount"), + Namespace: "ns", + }, &copiedSecret2) + assert.True(t, apierrors.IsNotFound(err), "second copied secret should be deleted (all SecretMounts removed)") + + // Verify the secret without annotation was NOT deleted + var secretWithoutAnnotation corev1.Secret + err = r.Client.Get(context.Background(), types.NamespacedName{ + Name: esv1.StackConfigAdditionalSecretName("test-es", "test-es-other-secret"), + Namespace: "ns", + }, &secretWithoutAnnotation) + assert.NoError(t, err, "secret without source annotation should NOT be deleted") + }, + wantErr: false, + wantRequeueAfter: true, + }, } for _, tt := range tests { diff --git a/pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go b/pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go index f190cb637b..f5c1bc96c1 100644 --- a/pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go +++ b/pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" + "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/common/metadata" @@ -24,6 +25,7 @@ import ( "github.com/elastic/cloud-on-k8s/v3/pkg/utils/k8s" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -77,7 +79,11 @@ func newElasticsearchConfigSecret(policy policyv1alpha1.StackConfigPolicy, es es } // reconcileSecretMounts creates the secrets in SecretMounts to the respective Elasticsearch namespace where they should be mounted to. +// It also removes any previously mounted secrets that are no longer in policy.Spec.Elasticsearch.SecretMounts. func reconcileSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsearch, policy *policyv1alpha1.StackConfigPolicy, meta metadata.Metadata) error { + // Track which secret mounts are defined in the policy (by their target secret name in ES namespace) + secretMountsInPolicy := make(sets.Set[string], len(policy.Spec.Elasticsearch.SecretMounts)) + for _, secretMount := range policy.Spec.Elasticsearch.SecretMounts { additionalSecret := corev1.Secret{} namespacedName := types.NamespacedName{ @@ -114,6 +120,54 @@ func reconcileSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsea if _, err := reconciler.ReconcileSecret(ctx, c, expected, nil); err != nil { return err } + + secretMountsInPolicy.Insert(secretName) + } + + // Clean up secret mounts that are no longer in the policy + return cleanupOrphanedSecretMounts(ctx, c, es, k8s.ExtractNamespacedName(policy), secretMountsInPolicy) +} + +// cleanupOrphanedSecretMounts removes copied secrets that are owned by the given policy and do not exist in the given +// secretMountsInPolicy set. This handles the case where a policy is updated in-place and SecretMounts list is affected. +// See https://github.com/elastic/cloud-on-k8s/issues/8921 +func cleanupOrphanedSecretMounts(ctx context.Context, c k8s.Client, es esv1.Elasticsearch, policyNsn types.NamespacedName, secretMountsInPolicy sets.Set[string]) error { + // List all secrets in the ES namespace that are soft-owned by this policy. + // The label selector below ensures we only find secrets that: (1) are soft-owned + // by this specific policy, (2) are marked for deletion when the policy is deleted, + // and (3) belong to this specific ES cluster. + var secrets corev1.SecretList + matchLabels := client.MatchingLabels{ + reconciler.SoftOwnerKindLabel: policyv1alpha1.Kind, + reconciler.SoftOwnerNameLabel: policyNsn.Name, + reconciler.SoftOwnerNamespaceLabel: policyNsn.Namespace, + commonlabels.StackConfigPolicyOnDeleteLabelName: commonlabels.OrphanSecretDeleteOnPolicyDelete, + eslabel.ClusterNameLabelName: es.Name, + } + + if err := c.List(ctx, &secrets, client.InNamespace(es.Namespace), matchLabels); err != nil { + return err + } + + for i := range secrets.Items { + secret := &secrets.Items[i] + + // Skip secrets that do not have commonannotation.SourceSecretAnnotationName which identifies the ones that + // were reconciled from a secret mount in the owner StackConfigPolicy. See reconcileSecretMounts func + if secret.Annotations[commonannotation.SourceSecretAnnotationName] == "" { + continue + } + + // Check if this secret is in the expected set (still in SecretMounts) + if secretMountsInPolicy.Has(secret.Name) { + continue + } + + // This secret is owned by the policy but no longer in SecretMounts - delete it + // Since these are single-owner secrets (filtered by labels), we can delete directly + if err := c.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) { + return err + } } return nil }