diff --git a/pkg/controller/elasticsearch/driver/driver.go b/pkg/controller/elasticsearch/driver/driver.go index baf0ebf4dd2..324ce315c62 100644 --- a/pkg/controller/elasticsearch/driver/driver.go +++ b/pkg/controller/elasticsearch/driver/driver.go @@ -22,6 +22,7 @@ import ( commonv1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/common/v1" esv1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/elasticsearch/v1" + policyv1alpha1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/stackconfigpolicy/v1alpha1" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/association" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/common" commondriver "github.com/elastic/cloud-on-k8s/v3/pkg/controller/common/driver" @@ -52,6 +53,7 @@ import ( "github.com/elastic/cloud-on-k8s/v3/pkg/controller/elasticsearch/settings" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/elasticsearch/stackmon" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/elasticsearch/user" + "github.com/elastic/cloud-on-k8s/v3/pkg/controller/stackconfigpolicy" "github.com/elastic/cloud-on-k8s/v3/pkg/dev" "github.com/elastic/cloud-on-k8s/v3/pkg/utils/k8s" ulog "github.com/elastic/cloud-on-k8s/v3/pkg/utils/log" @@ -351,11 +353,17 @@ func (d *defaultDriver) Reconcile(ctx context.Context) *reconciler.Results { return results.WithError(err) } - // reconcile an empty File based settings Secret if it doesn't exist if d.Version.GTE(filesettings.FileBasedSettingsMinPreVersion) { - err = filesettings.ReconcileEmptyFileSettingsSecret(ctx, d.Client, d.ES, true) + requeue, err := maybeReconcileEmptyFileSettingsSecret(ctx, d.Client, d.LicenseChecker, &d.ES, d.OperatorParameters.OperatorNamespace) if err != nil { return results.WithError(err) + } else if requeue { + results.WithReconciliationState( + defaultRequeue.WithReason( + fmt.Sprintf("This cluster is targeted by at least one StackConfigPolicy, expecting Secret %s to be created by StackConfigPolicy controller", + esv1.FileSettingsSecretName(d.ES.Name)), + ), + ) } } @@ -417,6 +425,64 @@ func (d *defaultDriver) Reconcile(ctx context.Context) *reconciler.Results { return results.WithResults(d.reconcileNodeSpecs(ctx, esReachable, esClient, d.ReconcileState, *resourcesState, keystoreResources, meta)) } +// maybeReconcileEmptyFileSettingsSecret reconciles an empty file-settings secret for this ES cluster +// based on license status and StackConfigPolicy targeting. When enterprise features are disabled always +// creates an empty file-settings secret. When enterprise features are enabled and at least one StackConfigPolicy +// targets this cluster returns true to requeue and doesn't create the empty file-settings secret. If no +// StackConfigPolicy targets this cluster it creates an empty file-settings secret. Note: This logic here prevents +// the race condition described in https://github.com/elastic/cloud-on-k8s/issues/8912. +func maybeReconcileEmptyFileSettingsSecret(ctx context.Context, c k8s.Client, licenseChecker commonlicense.Checker, es *esv1.Elasticsearch, operatorNamespace string) (bool, error) { + // Check if file-settings secret already exists + var currentSecret corev1.Secret + if err := c.Get(ctx, types.NamespacedName{Namespace: es.Namespace, Name: esv1.FileSettingsSecretName(es.Name)}, ¤tSecret); err == nil { + // Secret does exist + return false, nil + } else if !k8serrors.IsNotFound(err) { + return false, err + } + + log := ulog.FromContext(ctx) + enabled, err := licenseChecker.EnterpriseFeaturesEnabled(ctx) + if err != nil { + return false, err + } + if !enabled { + // If the license is not enabled, we reconcile the empty file-settings secret + return false, filesettings.ReconcileEmptyFileSettingsSecret(ctx, c, *es, true) + } + + // Get all StackConfigPolicies in the cluster + var policyList policyv1alpha1.StackConfigPolicyList + if err := c.List(ctx, &policyList); err != nil { + return false, err + } + + // Check each policy to see if it targets this ES cluster + for _, policy := range policyList.Items { + // Check if this policy's namespace and label selector match this ES cluster + matches, err := stackconfigpolicy.DoesPolicyMatchObject(&policy, es, operatorNamespace) + if err != nil { + // Do not return an err here as this potentially can block ES reconciliation if any SCP in the cluster + // has an invalid label selector, even if it doesn't target the current elasticsearch cluster. + log.Error(err, "Failed to check if StackConfigPolicy matches object", "scp_name", policy.Name, "scp_namespace", policy.Namespace) + continue + } else if !matches { + continue + } + + // Found a policy that targets this ES cluster but the file-settings secret does not exist. + // Let the SCP controller manage it, however, return requeue true to handle the following edge case: + // 1. SCP exists and targets ES cluster at creation time + // 2. ES controller "defers" to SCP controller (doesn't create secret) + // 3. SCP is deleted before it reconciles and creates the file-settings secret + // 4. Result: ES cluster left without any file-settings secret + return true, nil + } + + // No policies target this cluster, so ES controller should create the empty secret + return false, filesettings.ReconcileEmptyFileSettingsSecret(ctx, c, *es, true) +} + // apiKeyStoreSecretSource returns the Secret that holds the remote API keys, and which should be used as a secure settings source. func apiKeyStoreSecretSource(ctx context.Context, es *esv1.Elasticsearch, c k8s.Client) ([]commonv1.NamespacedSecretSource, error) { // Check if Secret exists diff --git a/pkg/controller/elasticsearch/driver/driver_test.go b/pkg/controller/elasticsearch/driver/driver_test.go index b7212d76ada..f4c26b58331 100644 --- a/pkg/controller/elasticsearch/driver/driver_test.go +++ b/pkg/controller/elasticsearch/driver/driver_test.go @@ -10,11 +10,18 @@ import ( "github.com/stretchr/testify/assert" 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" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + esv1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/elasticsearch/v1" + policyv1alpha1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/stackconfigpolicy/v1alpha1" + commonlicense "github.com/elastic/cloud-on-k8s/v3/pkg/controller/common/license" esclient "github.com/elastic/cloud-on-k8s/v3/pkg/controller/elasticsearch/client" "github.com/elastic/cloud-on-k8s/v3/pkg/controller/elasticsearch/user" + "github.com/elastic/cloud-on-k8s/v3/pkg/utils/k8s" "github.com/elastic/cloud-on-k8s/v3/pkg/utils/set" ) @@ -185,3 +192,313 @@ func (f *fakeSecurityClient) withFileTokens(namespacedService, tokenName string, f.serviceAccountCredentials[namespacedService] = serviceAccountCredential return f } + +func Test_maybeReconcileEmptyFileSettingsSecret(t *testing.T) { + const operatorNamespace = "elastic-system" + + tests := []struct { + name string + es *esv1.Elasticsearch + policies []policyv1alpha1.StackConfigPolicy + existingSecrets []corev1.Secret + licenseChecker commonlicense.Checker + wantSecretCreated bool + wantRequeue bool + wantErr bool + }{ + { + name: "No policies exist - should create empty secret (enterprise enabled)", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: true, + wantRequeue: false, + wantErr: false, + }, + { + name: "No policies exist - should create empty secret (enterprise disabled)", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: false}, + wantSecretCreated: true, + wantRequeue: false, + wantErr: false, + }, + { + name: "Policy targets ES cluster in same namespace - should NOT create empty secret but requeue", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: false, + wantRequeue: true, + wantErr: false, + }, + { + name: "Policy targets ES cluster from operator namespace - should NOT create empty secret but requeue", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "global-policy", + Namespace: "elastic-system", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: false, + wantRequeue: true, + wantErr: false, + }, + { + name: "Policy exists but does not target ES cluster - should create empty secret", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "other-app", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: true, + wantRequeue: false, + wantErr: false, + }, + { + name: "Policy in different namespace (not operator namespace) - should create empty secret", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "other-ns-policy", + Namespace: "other-namespace", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "elasticsearch", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: true, + wantRequeue: false, + wantErr: false, + }, + { + name: "Multiple policies, one targets ES, file-settings secret does not exist - should NOT create empty secret but requeue", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + "team": "platform", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "other-app", + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "matching-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "team": "platform", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: false, + wantRequeue: true, + wantErr: false, + }, + { + name: "Multiple policies, one targets ES, file-settings secret exists - should NOT requeue", + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-es", + Namespace: "default", + Labels: map[string]string{ + "app": "elasticsearch", + "team": "platform", + }, + }, + }, + existingSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: esv1.FileSettingsSecretName("test-es"), + Namespace: "default", + }, + }, + }, + policies: []policyv1alpha1.StackConfigPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "other-app", + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "matching-policy", + Namespace: "default", + }, + Spec: policyv1alpha1.StackConfigPolicySpec{ + ResourceSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "team": "platform", + }, + }, + }, + }, + }, + licenseChecker: commonlicense.MockLicenseChecker{EnterpriseEnabled: true}, + wantSecretCreated: true, + wantRequeue: false, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create fake client with initial objects + var initObjs []client.Object + for i := range tt.policies { + initObjs = append(initObjs, &tt.policies[i]) + } + for i := range tt.existingSecrets { + initObjs = append(initObjs, &tt.existingSecrets[i]) + } + initObjs = append(initObjs, tt.es) + + c := k8s.NewFakeClient(initObjs...) + + requeue, err := maybeReconcileEmptyFileSettingsSecret(t.Context(), c, tt.licenseChecker, tt.es, operatorNamespace) + + // Check error expectation + if tt.wantErr { + assert.Error(t, err, "expected error at maybeReconcileEmptyFileSettingsSecret") + return + } + assert.NoError(t, err, "expected no error at maybeReconcileEmptyFileSettingsSecret") + assert.Equal(t, tt.wantRequeue, requeue, "expected requeue does not match") + + // Check if secret was created + var secret corev1.Secret + secretName := esv1.FileSettingsSecretName(tt.es.Name) + secretErr := c.Get(t.Context(), types.NamespacedName{ + Name: secretName, + Namespace: tt.es.Namespace, + }, &secret) + + if tt.wantSecretCreated { + assert.NoError(t, secretErr, "expected no error at getting file-settings secret") + } else { + assert.True(t, apierrors.IsNotFound(secretErr), "expected IsNotFound error at getting file-settings secret") + } + }) + } +} diff --git a/pkg/controller/stackconfigpolicy/controller.go b/pkg/controller/stackconfigpolicy/controller.go index b2cefc8ea52..02d1e2070bb 100644 --- a/pkg/controller/stackconfigpolicy/controller.go +++ b/pkg/controller/stackconfigpolicy/controller.go @@ -303,14 +303,11 @@ func (r *ReconcileStackConfigPolicy) reconcileElasticsearchResources(ctx context continue } - // the file Settings Secret must exist, if not it will be created empty by the ES controller + // Get the file settings secret. Create it if it doesn't exist. + // This resolves the race condition from https://github.com/elastic/cloud-on-k8s/issues/8912 var actualSettingsSecret corev1.Secret err = r.Client.Get(ctx, types.NamespacedName{Namespace: es.Namespace, Name: esv1.FileSettingsSecretName(es.Name)}, &actualSettingsSecret) - if err != nil && apierrors.IsNotFound(err) { - // requeue if the Secret has not been created yet - return results.WithRequeue(defaultRequeue), status - } - if err != nil { + if err != nil && !apierrors.IsNotFound(err) { return results.WithError(err), status } diff --git a/pkg/controller/stackconfigpolicy/controller_test.go b/pkg/controller/stackconfigpolicy/controller_test.go index 851c861bed7..0ebbfbcc9c5 100644 --- a/pkg/controller/stackconfigpolicy/controller_test.go +++ b/pkg/controller/stackconfigpolicy/controller_test.go @@ -279,10 +279,37 @@ func TestReconcileStackConfigPolicy_Reconcile(t *testing.T) { wantRequeueAfter: false, }, { - name: "Settings secret doesn't exist yet: requeue", + name: "Settings secret doesn't exist: policy controller creates it and re-queues waiting for ES to apply settings", args: args{ - client: k8s.NewFakeClient(&policyFixture, &esFixture), - licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true}, + client: k8s.NewFakeClient(&policyFixture, &esFixture, secretMountsSecretFixture), + licenseChecker: &license.MockLicenseChecker{EnterpriseEnabled: true}, + esClientProvider: fakeClientProvider(clusterStateFileSettingsFixture(0, nil), nil), + }, + post: func(r ReconcileStackConfigPolicy, recorder record.FakeRecorder) { + // Verify that the file settings secret was created by the StackConfigPolicy controller + var secret corev1.Secret + err := r.Client.Get(context.Background(), types.NamespacedName{ + Namespace: "ns", + Name: "test-es-es-file-settings", + }, &secret) + assert.NoError(t, err, "file settings secret should be created by the policy controller") + + // Verify the secret has proper ownership labels pointing to the policy + assert.Equal(t, "StackConfigPolicy", secret.Labels["eck.k8s.elastic.co/owner-kind"]) + assert.Equal(t, "ns", secret.Labels["eck.k8s.elastic.co/owner-namespace"]) + assert.Equal(t, "test-policy", secret.Labels["eck.k8s.elastic.co/owner-name"]) + + // Verify settings are applied in the secret + var settings filesettings.Settings + err = json.Unmarshal(secret.Data[filesettings.SettingsSecretKey], &settings) + assert.NoError(t, err) + assert.Equal(t, "42mb", settings.State.ClusterSettings.Data["indices.recovery.max_bytes_per_sec"]) + + // Verify the policy status shows it's applying changes (waiting for ES to pick up the settings) + policy := r.getPolicy(t, k8s.ExtractNamespacedName(&policyFixture)) + assert.Equal(t, 1, policy.Status.Resources) + assert.Equal(t, 0, policy.Status.Ready) + assert.Equal(t, policyv1alpha1.ApplyingChangesPhase, policy.Status.Phase) }, wantErr: false, wantRequeueAfter: true, diff --git a/pkg/controller/stackconfigpolicy/stackconfigpolicy.go b/pkg/controller/stackconfigpolicy/stackconfigpolicy.go new file mode 100644 index 00000000000..ae032d766b2 --- /dev/null +++ b/pkg/controller/stackconfigpolicy/stackconfigpolicy.go @@ -0,0 +1,42 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. + +package stackconfigpolicy + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + + policyv1alpha1 "github.com/elastic/cloud-on-k8s/v3/pkg/apis/stackconfigpolicy/v1alpha1" +) + +// DoesPolicyMatchObject checks if the given StackConfigPolicy targets the given object (e.g., Elasticsearch or Kibana). +// A policy targets an object if both following conditions are met: +// 1. The policy is in either the operator namespace or the same namespace as the object +// 2. The policy's label selector matches the object's labels +// Returns true if the policy targets the object, false otherwise, and an error if the label selector is invalid. +func DoesPolicyMatchObject(policy *policyv1alpha1.StackConfigPolicy, obj metav1.Object, operatorNamespace string) (bool, error) { + // Check namespace restrictions; the policy must be in operator namespace or same namespace as the target object. + // This enforces the scoping rules: policies in the operator namespace are global, + // policies in other namespaces can only target resources in their own namespace. + if policy.Namespace != operatorNamespace && policy.Namespace != obj.GetNamespace() { + return false, nil + } + + // Convert the label selector from the policy spec into a labels.Selector that can be used for matching + selector, err := metav1.LabelSelectorAsSelector(&policy.Spec.ResourceSelector) + if err != nil { + // Return error if the label selector syntax is invalid (e.g., malformed expressions) + return false, err + } + + // Check if the label selector matches the object's labels. + // This is the actual matching logic - does this policy's selector match this object's labels? + if !selector.Matches(labels.Set(obj.GetLabels())) { + return false, nil + } + + // Both conditions met: namespace is valid and labels match + return true, nil +}