Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions config/crds/v1/all-crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10564,6 +10564,13 @@ spec:
- secretName
type: object
type: array
weight:
default: 0
description: |-
Weight determines the priority of this policy when multiple policies target the same resource.
Lower weight values take precedence. Defaults to 0.
format: int32
type: integer
type: object
status:
properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,13 @@ spec:
- secretName
type: object
type: array
weight:
default: 0
description: |-
Weight determines the priority of this policy when multiple policies target the same resource.
Lower weight values take precedence. Defaults to 0.
format: int32
type: integer
type: object
status:
properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10634,6 +10634,13 @@ spec:
- secretName
type: object
type: array
weight:
default: 0
description: |-
Weight determines the priority of this policy when multiple policies target the same resource.
Lower weight values take precedence. Defaults to 0.
format: int32
type: integer
type: object
status:
properties:
Expand Down
1 change: 1 addition & 0 deletions docs/reference/api-reference/main.md
Original file line number Diff line number Diff line change
Expand Up @@ -2066,6 +2066,7 @@ StackConfigPolicy represents a StackConfigPolicy resource in a Kubernetes cluste
| Field | Description |
| --- | --- |
| *`resourceSelector`* __[LabelSelector](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#labelselector-v1-meta)__ | |
| *`weight`* __integer__ | Weight determines the priority of this policy when multiple policies target the same resource.<br>Lower weight values take precedence. Defaults to 0. |
| *`secureSettings`* __[SecretSource](#secretsource) array__ | Deprecated: SecureSettings only applies to Elasticsearch and is deprecated. It must be set per application instead. |
| *`elasticsearch`* __[ElasticsearchConfigPolicySpec](#elasticsearchconfigpolicyspec)__ | |
| *`kibana`* __[KibanaConfigPolicySpec](#kibanaconfigpolicyspec)__ | |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ type StackConfigPolicyList struct {

type StackConfigPolicySpec struct {
ResourceSelector metav1.LabelSelector `json:"resourceSelector,omitempty"`
// Weight determines the priority of this policy when multiple policies target the same resource.
// Lower weight values take precedence. Defaults to 0.
// +kubebuilder:default=0
Weight int32 `json:"weight,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic nit, I found it useful to have the weight in a column, feel free to ignore if you think otherwise:

diff --git a/pkg/apis/stackconfigpolicy/v1alpha1/stackconfigpolicy_types.go b/pkg/apis/stackconfigpolicy/v1alpha1/stackconfigpolicy_types.go
index e7d17e200..ec0de8d60 100644
--- a/pkg/apis/stackconfigpolicy/v1alpha1/stackconfigpolicy_types.go
+++ b/pkg/apis/stackconfigpolicy/v1alpha1/stackconfigpolicy_types.go
@@ -34,6 +34,7 @@ func init() {
 // +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.readyCount",description="Resources configured"
 // +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"
 // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
+// +kubebuilder:printcolumn:name="Weight",type="integer",JSONPath=".spec.weight"
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 type StackConfigPolicy struct {

Result:

 k get stackconfigpolicies.stackconfigpolicy.k8s.elastic.co
NAME               READY   PHASE   AGE   WEIGHT
cluster-policy-1   1/1     Ready   20h   10
cluster-policy-2   1/1     Ready   20h   10
kibana-policy-1    1/1     Ready   20h   1
kibana-policy-2    1/1     Ready   20h   0

// Deprecated: SecureSettings only applies to Elasticsearch and is deprecated. It must be set per application instead.
SecureSettings []commonv1.SecretSource `json:"secureSettings,omitempty"`
Elasticsearch ElasticsearchConfigPolicySpec `json:"elasticsearch,omitempty"`
Expand Down
80 changes: 69 additions & 11 deletions pkg/controller/elasticsearch/filesettings/file_settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,43 @@ func newEmptySettingsState() SettingsState {
}
}

// updateStateFromPolicies merges settings from multiple StackConfigPolicies based on their weights.
// Lower weight policies override higher weight policies for conflicting settings.
func (s *Settings) updateStateFromPolicies(es types.NamespacedName, policies []policyv1alpha1.StackConfigPolicy) error {
if len(policies) == 0 {
return nil
}

sortedPolicies := make([]policyv1alpha1.StackConfigPolicy, len(policies))
copy(sortedPolicies, policies)

// Bubble sort by weight (descending order)
for i := 0; i < len(sortedPolicies)-1; i++ {
for j := 0; j < len(sortedPolicies)-i-1; j++ {
if sortedPolicies[j].Spec.Weight < sortedPolicies[j+1].Spec.Weight {
sortedPolicies[j], sortedPolicies[j+1] = sortedPolicies[j+1], sortedPolicies[j]
}
}
}

for _, policy := range sortedPolicies {
if err := s.updateState(es, policy); err != nil {
return err
}
}

return nil
}

// updateState updates the Settings state from a StackConfigPolicy for a given Elasticsearch.
func (s *Settings) updateState(es types.NamespacedName, policy policyv1alpha1.StackConfigPolicy) error {
p := policy.DeepCopy() // be sure to not mutate the original policy
state := newEmptySettingsState()

// Initialize state if not already done
if s.State.ClusterSettings == nil {
s.State = newEmptySettingsState()
}

// mutate Snapshot Repositories
if p.Spec.Elasticsearch.SnapshotRepositories != nil {
for name, untypedDefinition := range p.Spec.Elasticsearch.SnapshotRepositories.Data {
Expand All @@ -97,34 +130,59 @@ func (s *Settings) updateState(es types.NamespacedName, policy policyv1alpha1.St
}
p.Spec.Elasticsearch.SnapshotRepositories.Data[name] = repoSettings
}
state.SnapshotRepositories = p.Spec.Elasticsearch.SnapshotRepositories
s.mergeConfig(s.State.SnapshotRepositories, p.Spec.Elasticsearch.SnapshotRepositories)
}
// just copy other settings
// merge other settings
if p.Spec.Elasticsearch.ClusterSettings != nil {
state.ClusterSettings = p.Spec.Elasticsearch.ClusterSettings
s.mergeConfig(s.State.ClusterSettings, p.Spec.Elasticsearch.ClusterSettings)
}
if p.Spec.Elasticsearch.SnapshotLifecyclePolicies != nil {
state.SLM = p.Spec.Elasticsearch.SnapshotLifecyclePolicies
s.mergeConfig(s.State.SLM, p.Spec.Elasticsearch.SnapshotLifecyclePolicies)
}
if p.Spec.Elasticsearch.SecurityRoleMappings != nil {
state.RoleMappings = p.Spec.Elasticsearch.SecurityRoleMappings
s.mergeConfig(s.State.RoleMappings, p.Spec.Elasticsearch.SecurityRoleMappings)
}
if p.Spec.Elasticsearch.IndexLifecyclePolicies != nil {
state.IndexLifecyclePolicies = p.Spec.Elasticsearch.IndexLifecyclePolicies
s.mergeConfig(s.State.IndexLifecyclePolicies, p.Spec.Elasticsearch.IndexLifecyclePolicies)
}
if p.Spec.Elasticsearch.IngestPipelines != nil {
state.IngestPipelines = p.Spec.Elasticsearch.IngestPipelines
s.mergeConfig(s.State.IngestPipelines, p.Spec.Elasticsearch.IngestPipelines)
}
if p.Spec.Elasticsearch.IndexTemplates.ComposableIndexTemplates != nil {
state.IndexTemplates.ComposableIndexTemplates = p.Spec.Elasticsearch.IndexTemplates.ComposableIndexTemplates
s.mergeConfig(s.State.IndexTemplates.ComposableIndexTemplates, p.Spec.Elasticsearch.IndexTemplates.ComposableIndexTemplates)
}
if p.Spec.Elasticsearch.IndexTemplates.ComponentTemplates != nil {
state.IndexTemplates.ComponentTemplates = p.Spec.Elasticsearch.IndexTemplates.ComponentTemplates
s.mergeConfig(s.State.IndexTemplates.ComponentTemplates, p.Spec.Elasticsearch.IndexTemplates.ComponentTemplates)
}
s.State = state
return nil
}

// mergeConfig merges source config into target config, with source taking precedence
// For map-type values (like snapshot repositories), individual entries are merged rather than replaced
func (s *Settings) mergeConfig(target, source *commonv1.Config) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a receiver method of Settings?

if source == nil || source.Data == nil {
return
Copy link

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mergeConfig function modifies the target parameter but then creates a new Config object that is not assigned back to the original parameter. The line target = &commonv1.Config{Data: make(map[string]interface{})} only modifies the local variable, not the original pointer. This will cause the merge to fail silently.

Suggested change
return
func (s *Settings) mergeConfig(target, source *commonv1.Config) *commonv1.Config {
if source == nil || source.Data == nil {
return target

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a valid copilot observation. Also I would expect unit tests for this kind of functionality. We do have more advanced merging through the CanonicalConfig abstraction in the code base. But there might be some gotchas with the more complex configuration objects that stack config policies support that might not be thinking of right now.

}
if target == nil || target.Data == nil {
target = &commonv1.Config{Data: make(map[string]interface{})}
}

for key, value := range source.Data {
if targetValue, exists := target.Data[key]; exists {
if targetMap, targetIsMap := targetValue.(map[string]interface{}); targetIsMap {
if sourceMap, sourceIsMap := value.(map[string]interface{}); sourceIsMap {
for subKey, subValue := range sourceMap {
targetMap[subKey] = subValue
}
continue
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems too simplistic given that for example cluster settings can both be in nested and flat syntax. Also it only merges one level deep?

// For non-map values or if target doesn't exist, replace entirely
target.Data[key] = value
}
}

// mutateSnapshotRepositorySettings ensures that a snapshot repository can be used across multiple ES clusters.
// The namespace and the Elasticsearch cluster name are injected in the repository settings depending on the type of the repository:
// - "azure", "gcs", "s3": if not provided, the `base_path` property is set to `snapshots/<namespace>-<esName>`
Expand Down
196 changes: 196 additions & 0 deletions pkg/controller/elasticsearch/filesettings/secret.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,85 @@ func NewSettingsSecretWithVersion(es types.NamespacedName, currentSecret *corev1
return newSettingsSecret(newVersion, es, currentSecret, policy, meta)
}

// NewSettingsSecretWithVersionFromPolicies returns a new SettingsSecret for a given Elasticsearch from multiple policies.
// Policies are merged based on their weights, with higher weights taking precedence.
func NewSettingsSecretWithVersionFromPolicies(es types.NamespacedName, currentSecret *corev1.Secret, policies []policyv1alpha1.StackConfigPolicy, meta metadata.Metadata) (corev1.Secret, int64, error) {
newVersion := time.Now().UnixNano()
return newSettingsSecretFromPolicies(newVersion, es, currentSecret, policies, meta)
}

// NewSettingsSecretFromPolicies returns a new SettingsSecret for a given Elasticsearch from multiple StackConfigPolicies.
func newSettingsSecretFromPolicies(version int64, es types.NamespacedName, currentSecret *corev1.Secret, policies []policyv1alpha1.StackConfigPolicy, meta metadata.Metadata) (corev1.Secret, int64, error) {
settings := NewEmptySettings(version)

// update the settings according to the config policies
if len(policies) > 0 {
err := settings.updateStateFromPolicies(es, policies)
if err != nil {
return corev1.Secret{}, 0, err
}
}

// do not update version if hash hasn't changed
if currentSecret != nil && !hasChanged(*currentSecret, settings) {
currentVersion, err := extractVersion(*currentSecret)
if err != nil {
return corev1.Secret{}, 0, err
}

version = currentVersion
settings.Metadata.Version = strconv.FormatInt(currentVersion, 10)
}

// prepare the SettingsSecret
secretMeta := meta.Merge(metadata.Metadata{
Annotations: map[string]string{
commonannotation.SettingsHashAnnotationName: settings.hash(),
},
})
settingsBytes, err := json.Marshal(settings)
if err != nil {
return corev1.Secret{}, 0, err
}
settingsSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: es.Namespace,
Name: esv1.FileSettingsSecretName(es.Name),
Labels: secretMeta.Labels,
Annotations: secretMeta.Annotations,
},
Data: map[string][]byte{
SettingsSecretKey: settingsBytes,
},
}

// Store all policy references in the secret
policyRefs := make([]PolicyRef, 0, len(policies))
for _, policy := range policies {
policyRefs = append(policyRefs, PolicyRef{
Name: policy.Name,
Namespace: policy.Namespace,
Weight: policy.Spec.Weight,
})
}
if err := SetPolicyRefs(settingsSecret, policyRefs); err != nil {
return corev1.Secret{}, 0, err
}

// Add secure settings from all policies
if err := setSecureSettingsFromPolicies(settingsSecret, policies); err != nil {
return corev1.Secret{}, 0, err
}

// Add a label to reset secret on deletion of the stack config policy
if settingsSecret.Labels == nil {
settingsSecret.Labels = make(map[string]string)
}
settingsSecret.Labels[commonlabel.StackConfigPolicyOnDeleteLabelName] = commonlabel.OrphanSecretResetOnPolicyDelete

return *settingsSecret, version, nil
}

// NewSettingsSecret returns a new SettingsSecret for a given Elasticsearch and StackConfigPolicy.
func newSettingsSecret(version int64, es types.NamespacedName, currentSecret *corev1.Secret, policy *policyv1alpha1.StackConfigPolicy, meta metadata.Metadata) (corev1.Secret, int64, error) {
settings := NewEmptySettings(version)
Expand Down Expand Up @@ -160,6 +239,35 @@ func setSecureSettings(settingsSecret *corev1.Secret, policy policyv1alpha1.Stac
return nil
}

// setSecureSettingsFromPolicies sets secure settings from multiple policies into the settings secret
func setSecureSettingsFromPolicies(settingsSecret *corev1.Secret, policies []policyv1alpha1.StackConfigPolicy) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost identical to setSecureSettings can we not generalise into one function?

var allSecretSources []commonv1.NamespacedSecretSource

for _, policy := range policies {
// Common secureSettings field, this is mainly there to maintain backwards compatibility
//nolint:staticcheck
for _, src := range policy.Spec.SecureSettings {
allSecretSources = append(allSecretSources, commonv1.NamespacedSecretSource{Namespace: policy.GetNamespace(), SecretName: src.SecretName, Entries: src.Entries})
}

// SecureSettings field under Elasticsearch in the StackConfigPolicy
for _, src := range policy.Spec.Elasticsearch.SecureSettings {
allSecretSources = append(allSecretSources, commonv1.NamespacedSecretSource{Namespace: policy.GetNamespace(), SecretName: src.SecretName, Entries: src.Entries})
}
}

if len(allSecretSources) == 0 {
return nil
}

bytes, err := json.Marshal(allSecretSources)
if err != nil {
return err
}
settingsSecret.Annotations[commonannotation.SecureSettingsSecretsAnnotationName] = string(bytes)
return nil
}

// CanBeOwnedBy return true if the Settings Secret can be owned by the given StackConfigPolicy, either because the Secret
// belongs to no one or because it already belongs to the given policy.
func CanBeOwnedBy(settingsSecret corev1.Secret, policy policyv1alpha1.StackConfigPolicy) (reconciler.SoftOwnerRef, bool) {
Expand All @@ -173,6 +281,94 @@ func CanBeOwnedBy(settingsSecret corev1.Secret, policy policyv1alpha1.StackConfi
return currentOwner, canBeOwned
}

// PolicyRef represents a reference to a StackConfigPolicy with its weight
type PolicyRef struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it that these refs are intended as some kind of tracking mechanism to know which policy contributed to the effective settings. However what I am missing is a replacement of the owner references that we had in the single policy approach. This implemenation leaks secrets once you have deleted the policy there is no mechanism in place to clean up the secrets.

Name string
Namespace string
Weight int32
}
Comment on lines +285 to +289
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type PolicyRef struct {
Name string
Namespace string
Weight int32
}
type PolicyRef struct {
Name string `json:"name"`
Namespace string `json:"namespace"`
Weight int32 `json:"weight"`
}


// GetPolicyRefs extracts all policy references from a secret's annotations
func GetPolicyRefs(secret corev1.Secret) ([]PolicyRef, error) {
if secret.Annotations == nil {
return nil, nil
}

policiesData, ok := secret.Annotations["stackconfigpolicy.k8s.elastic.co/policies"]
if !ok {
return nil, nil
}

var policies []PolicyRef
if err := json.Unmarshal([]byte(policiesData), &policies); err != nil {
return nil, err
}

return policies, nil
}

// SetPolicyRefs stores policy references in a secret's annotations
func SetPolicyRefs(secret *corev1.Secret, policies []PolicyRef) error {
if secret.Annotations == nil {
secret.Annotations = make(map[string]string)
}

data, err := json.Marshal(policies)
if err != nil {
return err
}

secret.Annotations["stackconfigpolicy.k8s.elastic.co/policies"] = string(data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a constant.

return nil
}

// AddOrUpdatePolicyRef adds or updates a policy reference in the secret
func AddOrUpdatePolicyRef(secret *corev1.Secret, policy policyv1alpha1.StackConfigPolicy) error {
policies, err := GetPolicyRefs(*secret)
if err != nil {
return err
}

policyRef := PolicyRef{
Name: policy.Name,
Namespace: policy.Namespace,
Weight: policy.Spec.Weight,
}

// Update existing policy or add new one
found := false
for i, p := range policies {
if p.Name == policy.Name && p.Namespace == policy.Namespace {
policies[i] = policyRef
found = true
break
}
}

if !found {
policies = append(policies, policyRef)
}

return SetPolicyRefs(secret, policies)
}

// RemovePolicyRef removes a policy reference from the secret
func RemovePolicyRef(secret *corev1.Secret, policyName, policyNamespace string) error {
policies, err := GetPolicyRefs(*secret)
if err != nil {
return err
}

var filtered []PolicyRef
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a map keyed on namespace/name not be more convenient here?

for _, p := range policies {
if !(p.Name == policyName && p.Namespace == policyNamespace) {
filtered = append(filtered, p)
}
}

return SetPolicyRefs(secret, filtered)
}

// getSecureSettings returns the SecureSettings Secret sources stores in an annotation of the given file settings Secret.
func getSecureSettings(settingsSecret corev1.Secret) ([]commonv1.NamespacedSecretSource, error) {
rawString, ok := settingsSecret.Annotations[commonannotation.SecureSettingsSecretsAnnotationName]
Expand Down
Loading