[AutoOps] Allow overriding parts of the agent collector configuration via spec.config/configRef#9507
[AutoOps] Allow overriding parts of the agent collector configuration via spec.config/configRef#9507pkoutsovasilis wants to merge 6 commits into
Conversation
🔍 Preview links for changed docs |
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
|
@pickypg — would appreciate your eyes on the baseline/mandatory config split in this PR. The original monolithic template has been split into two:
The main question for you: does the mandatory set look right? Specifically:
A consequence of this split is that users cannot change the period or metricsets of the operator's built-in modules — they can only append new module entries alongside them. Happy to revisit the split if that's considered a blocker. |
96ef061 to
d342e02
Compare
d342e02 to
452e148
Compare
| // Merge order: baseline → user → userSecret → mandatory. | ||
| // User config can override anything in the baseline (e.g. sending_queue tuning). | ||
| // Mandatory config is rendered last so all operator-owned fields always win. | ||
| if err := baselineCfg.MergeWith(userCfg, userSecretCfg, mandatoryCfg); err != nil { |
There was a problem hiding this comment.
My only question is how well this merges things like:
receivers:
metricbeatreceiver:
metricbeat:
modules:
# Metrics
- module: autoops_es
hosts: ${env:AUTOOPS_ES_URL}
{{- template "ssl" . }}
period: 10s
metricsets:
- cat_shards
- cluster_health
- cluster_settings
- license
- node_stats
- tasks_management
# Templates
- module: autoops_es
hosts: ${env:AUTOOPS_ES_URL}
{{- template "ssl" . }}
period: 24h
metricsets:
- cat_template
- component_template
- index_templateIf I drop one of those - module blocks, will it continue to exist? If I want to override one, can I? What happens if I set (drops the # Templates section:
receivers:
metricbeatreceiver:
metricbeat:
modules:
# Metrics
- module: autoops_es
hosts: ${env:AUTOOPS_ES_URL}
{{- template "ssl" . }}
period: 10s
metricsets:
- cat_shards
- cluster_health
- cluster_settings
- license
- node_stats
- tasks_managementI think it will do the right thing and receivers.metricbeatreceiver.metricbeat.modules will be replaced with the new array, but I want to confirm.
There was a problem hiding this comment.
I missed in your comment this is covered:
A consequence of this split is that users cannot change the period or metricsets of the operator's built-in modules — they can only append new module entries alongside them. Happy to revisit the split if that's considered a blocker.
I don't think it's a showstopper, but it would be nice if we can make it so this can be overridden somehow. Specifically, some users may not want to share their templates for any reason and similarly may not want to share their tasks. In that scenario, I would like to be able to support them. I wonder if there's some simple mechanism where we can have an override flag that says "the config here is complete and you only need to provide mandatory changes" to it, where that would not include the metricsets.
There was a problem hiding this comment.
Hey @pickypg, thanks for taking a look here!
The main constraint I can think of is that allowing users to take full ownership of the config is risky - the healthcheckv2 extension on port 13133 is what the readiness probe hits, so if a user removes or remaps it the pod never becomes ready. Similarly, the OTLP endpoint/Authorization header and the metricbeatreceiver processor (token injection) are wiring the operator manages and that users shouldn't touch.
For the modules specifically though, I think there's a middle ground: if the user defines at least one autoops_es module in their spec.config or spec.configRef, the operator skips injecting its own default modules (Metrics + Templates) entirely, giving the user full control over which data is collected and at what schedule. If the user defines no autoops_es modules at all (or only adds non-autoops_es modules), the operator's defaults (Metrics + Templates) are injected as before.
Regardless of whether modules come from the user or the operator defaults, the operator always injects hosts, ssl.verification_mode, ssl.certificate_authorities, and (when mTLS is required) ssl.certificate/ssl.key into every autoops_es module post-merge. The user-supplied values for those fields are always overridden. This way users control what is collected but the operator always controls how the connection to ES is established.
Does this split sound reasonable to you?
There was a problem hiding this comment.
Thank you. This looks great (especially reviewing the tests).
Since users without ECK can override all of these fields, I don't really consider anything off limits for ECK. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
pebrc
left a comment
There was a problem hiding this comment.
LGTM in general. I am a bit worried about the complexity of the deep config inspection and manipulation. I and the 🤖 suggested a somewhat simpler approach inline. I think the viability depends on how common it is that users want to add new modules without touching the autops_es defaults or whether removing one of the autoops_es default modules is the main use case.
| assert.True(t, foundAutoOps, "operator autoops_es modules must be preserved") | ||
| assert.True(t, foundOther, "user module must be appended") |
There was a problem hiding this comment.
These assertions verify presence but not order. With AppendValues semantics the user's module (merged in an earlier layer) will always appear before the operator's defaults (merged later). A future change that reorders the merge constants would break this property without any test catching it. Consider pinning the order:
// user's module was merged before operator defaults, so it comes first
assert.Equal(t, "some_other_module", asMap(t, modules[0])["module"])
assert.Equal(t, "autoops_es", asMap(t, modules[1])["module"])There was a problem hiding this comment.
ok why does the order matter here?
There was a problem hiding this comment.
I was thinking along the lines of avoiding unnecessary restarts, and making sure this test fails if we do refactor this in a way that changes the order, so that the restart becomes a conscious choice. But maybe you are right and for AutoOps agents this is less of a concern.
| // buildAutoOpsESConfigMap builds the expected ConfigMap for autoops configuration. | ||
| // SSL is enabled based on the Elasticsearch CRD's spec.http.tls configuration. | ||
| func buildAutoOpsESConfigMap(policy autoopsv1alpha1.AutoOpsAgentPolicy, es esv1.Elasticsearch) (corev1.ConfigMap, error) { | ||
| // Merge order: baseline → userCfg → userSecretCfg → defaultModules → mandatory. |
There was a problem hiding this comment.
The code here is correct, but the PR description's Design section is now stale: it says the original template was "split into two" and shows merge order as baseline → userCfg → userSecretCfg → mandatory. The second commit introduced autoOpsDefaultModulesConfig as a third constant and updated the actual order. Worth updating the PR description to say "split into three" and reflect the defaultModules step, so future readers aren't confused.
There was a problem hiding this comment.
PR description updated
| // which autoops_es data is collected (e.g. to drop the Templates module). | ||
| // A user config that only adds non-autoops_es modules returns false and the operator still | ||
| // injects its own defaults alongside the user's entries. | ||
| func hasUserAutoOpsESModules(cfgs ...*settings.CanonicalConfig) bool { |
There was a problem hiding this comment.
This predicate introduces a pattern that doesn't exist anywhere else in ECK: unpacking config into map[string]any, walking a list, and matching entries by field value. For comparison, the codebase has three established patterns for combining user and operator config:
- Map-path layering (Beat, Agent): merge user config, then merge operator-owned settings at fixed map paths last (
beat/common/config.go). - Presence checks at fixed paths (EnterpriseSearch):
HasChildConfigto decide whether to inject a default (enterprisesearch/config.go:198). - Wholesale list ownership (Logstash pipelines): faced with the same "list-valued config that ucfg can't merge into" problem, Logstash chose
if userProvidedCfg != nil { return userProvidedCfg.Render() }— either the operator owns the list or the user does (logstash/pipeline.go).
To be clear about the split: injectModuleConnectionSettings below is justified — SSL paths are per-ES while one spec.config fans out to N clusters, so the operator must fix up connection settings post-merge; wholesale ownership can't solve that. But this predicate could be simplified to the established pattern 2:
const modulesPath = "receivers.metricbeatreceiver.metricbeat.modules"
hasUserModules := userCfg.HasChildConfig(modulesPath) || userSecretCfg.HasChildConfig(modulesPath)i.e. "if you touch the module list, you own the module list." That collapses this function to two calls of an existing helper, removes one of the two hand-rolled traversals over the same untyped structure (drift between them is a future bug class), and gives users a contract that's easy to state. The only behavioral difference — a user adding a non-autoops_es module also takes ownership of the defaults — is arguably more predictable, not less.
Whichever predicate wins, the module-ownership semantics need to be user-visible: the CRD field description currently only says connection details "are always injected", and the PR description still says changing period/metricsets of the built-in modules is "Not supported" — which d945645 made supported. The most surprising behavior in this PR currently exists only in code comments.
There was a problem hiding this comment.
what you say here, including the simplification proposal, is valid only if we accept the approach - if the user touches the modules then the user owns the modules list. To that end, if a user is able to fabricate an AutoOpsAgentPolicy configuration without any autoops_es module in it, what the point of this CRD? IMO, in the case of logstash, agent, beats yes the users should own everything because these CRDs need to be tailored according their needs. However, AutoOpsAgentPolicy CRD, is here to fulfil the need to run an AutoOps agent. In this PR we just want allow the users to configure any knobs they need to configure but this still has to be an autoops-oriented config. All that said, if you still think that we should change that, happy to do so, just let me know
There was a problem hiding this comment.
Fair point, you convinced me. Let's maybe make the behaviour clear in the config attribute godoc (and with that in our public API docs). Maybe something like:
supplying your own autoops_es module replaces the operator's built-in modules
pebrc
left a comment
There was a problem hiding this comment.
LGTM (not tested!). Let's document the module merging behaviour in the CRD.
|
@pickypg I would appreciate another review from you, to make sure that we provide enough flexibility but we don't break anything autoops-related, before I merge this |
|
buildkite test this -f p=gke,E2E_TAGS=autoops -m s=9.3.6,s=9.4.3-SNAPSHOT,s=9.5.0-SNAPSHOT |
|
I ran this branch through a manual test pass on a local kind cluster (operator image Test matrix
The module-ownership semantics and connection-settings injection both held up under adversarial input (scenario 4): user-supplied Findings
|
I don't like this transition from |
|
ok I found this which means that this transition doesn't affect us at all, @pickypg is my understanding correct? |
pickypg
left a comment
There was a problem hiding this comment.
LGTM. Thank you for making the metricsets overrideable.
If this runs an agent successfully, then we should be fine. It's noteworthy that the configuration in question here is for the Elastic Agent running in OTel mode, and not Beats. In the Elastic Agent, in OTel mode, we do not specify the classic Beats configuration (even though a part of it bleeds in under
https://github.com/elastic/elastic-agent/blob/main/internal/edot/samples/linux/autoops_es.yml |
|
thanks for the review @pickypg , ok this config here https://github.com/elastic/cloud-on-k8s/blob/main/pkg/controller/autoops/configmap_test.go#L21-L97 is essentially what ECK has as the default configuration of an AutoOps agent (this one predates this PR). Compared to the configuration you posted above I can see they are not fully the same. So the question is, should we update the default configuration of ECK to match this one https://github.com/elastic/elastic-agent/blob/main/internal/edot/samples/linux/autoops_es.yml? |
Summary
Adds
spec.configandspec.configReffields toAutoOpsAgentPolicy, giving users a supported way to tune the AutoOps agent's OpenTelemetry collector configuration.Closes #9395
Background
The AutoOps agent's OTel/metricbeat config was rendered from a single hardcoded Go template into a controller-owned ConfigMap. The reconciler overwrote
Dataon every loop, so any manual edit was silently reverted. Users who needed to tune operational knobs - queue sizing, additional processors, custom exporters - had no supported path.Every other ECK stack component already exposes a
config/configRefpair (Beats, Agent, Logstash, EnterpriseSearch, EPR). This PR brings AutoOps in line with that pattern.Design
The original config template is split into three:
autoOpsBaselineConfig- user-tunable defaults (currently thesending_queuesettings). Users can override any field here viaspec.configorspec.configRef.autoOpsDefaultModulesConfig- the operator's standardautoops_esmodule definitions (Metrics + Templates). Injected only when neitherspec.confignorspec.configRefdefines at least oneautoops_esmodule. When the user supplies their ownautoops_esmodules, this step is skipped entirely, giving them full control over which data is collected.autoOpsMandatoryConfig- operator-owned structural config (env-var references, OTLP exporter wiring, healthcheck extension). Applied last and always wins.Merge order:
baseline → userCfg → userSecretCfg → defaultModules → mandatoryThis means users can freely tune the baseline and extend it (add custom modules, exporters, pipelines), while the operator's critical wiring is always re-asserted at the end - no validation layer needed.
common.ParseConfigRefis always called unconditionally so the dynamic Secret watch lifecycle is managed correctly regardless of whetherconfigRefis set.What users can do
The merge uses
ucfg.AppendValuessemantics: scalar fields are overridden by the last write, and list fields accumulate entries. Because the mandatory config is merged last, users can extend but not replace operator-owned lists.Supported today:
When the user supplies at least one
autoops_esmodule, the operator's built-in Metrics and Templates modules are not injected - the user's list is used as-is. This allows changing theperiod,metricsets, or dropping individual modules entirely. Non-autoops_esmodules added by the user are appended alongside the operator's defaults (which are still injected in that case).Or via a Secret reference:
The following fields are always injected by the operator and cannot be overridden: Elasticsearch connection details (hosts, SSL paths, token), OTLP exporter endpoint and Authorization header, and the healthcheck extension on port 13133.