Skip to content

[amazon-cloudwatch-observability] feat: bundle ServiceMonitor/PodMonitor CRDs (zero-step install) - #331

Open
wenegiemepraise wants to merge 20 commits into
aws-observability:mainfrom
wenegiemepraise:helm-crd-bundling
Open

[amazon-cloudwatch-observability] feat: bundle ServiceMonitor/PodMonitor CRDs (zero-step install)#331
wenegiemepraise wants to merge 20 commits into
aws-observability:mainfrom
wenegiemepraise:helm-crd-bundling

Conversation

@wenegiemepraise

Copy link
Copy Markdown

Summary

Bundle the community monitoring.coreos.com ServiceMonitor/PodMonitor CRDs in the chart
as templated, gated resources — so customers who don't already run prometheus-operator get
them with no manual CRD prerequisite (matching the prometheus-operator experience). This
completes the "zero-step install" of the SM/PM scraping path.

Contents

  • templates/prometheus-crds/{servicemonitor,podmonitor}-crd.yaml — the CRD schemas
    (vendored/pinned), templated + gated, and skipped when an unmanaged copy already
    exists
    (Helm lookup) so a prometheus-operator-managed CRD is never clobbered. Annotated
    helm.sh/resource-policy: keep so uninstall does not cascade-delete the SM/PM CRs.
  • values.yamlprometheusCRDs.install (auto|always|never, default auto); auto
    bundles only when otelContainerInsights is enabled.
  • _helpers.tplprometheusCRDsEnabled gating helper.
  • tests/prometheus_crds_matrix.sh — render tests for the gating.

The Target Allocator CRD-watch RBAC is intentionally not here — it ships in #327.

Dependencies

Part of the SM/PM scraping stack; depends on #329 (v2 OTLP scraping path) and #330
(per-node). Until those merge, this PR's diff includes them. Reviewable bottom-up.

Testing

helm template:

  • otelContainerInsights.enabled=true2 CRD manifests render.
  • default (disabled) → 0 CRD manifests render (correctly gated).

(The skip-if-exists lookup is a no-op under helm template/--dry-run by design; it takes
effect on real install/upgrade.)

…path

Route Prometheus ServiceMonitor/PodMonitor metrics through the
otelContainerInsights v2 pipeline to CloudWatch using the Target Allocator's
prometheusCR discovery:

- _otel-container-insights-config.tpl: add the prometheus/cw_k8s_ci_v0_prometheuscr
  receiver (Target Allocator endpoint, mTLS) and the metrics/cw_k8s_ci_v0_prometheuscr
  pipeline (filter/scrape_metadata, metricstarttime, set_cluster_name,
  set_scope_prometheuscr, resourcedetection, batch) exported via otlphttp.
- _helpers.tpl: add the cloudwatch-agent.otelCIScrapeEnabled helper
  (otelContainerInsights.enabled AND targetAgent AND serviceMonitor/podMonitor on).
- cloudwatch-agent-custom-resource.yaml: render targetAllocator.enabled and
  prometheusCR.enabled on the AmazonCloudWatchAgent CR when otelCIScrape is on.
- target-allocator-clusterrole.yaml / -clusterrolebinding.yaml: grant the TA RBAC
  to get/list/watch podmonitors and servicemonitors when otelCIScrape is on.
- values.yaml: add otelContainerInsights.serviceMonitor.enabled and
  .podMonitor.enabled (default true).
- operator-deployment.yaml: supporting wiring.
…ategy

- CRD: allocationStrategy enum gains "per-node".
- cloudwatch-agent-custom-resource.yaml: set the TA allocationStrategy from
  values (defaults to per-node).
- values.yaml: prometheus.targetAllocator.allocationStrategy default per-node.
- _otel-container-insights-config.tpl: add set_node_name + promote_node_name
  transforms to the prometheuscr pipeline so each scraped series carries the
  scraping agent's node as @resource.k8s.node.name (from ${env:K8S_NODE_NAME}),
  enabling per-node verification and node enrichment.
…tep install

Bundle the community monitoring.coreos.com ServiceMonitor/PodMonitor CRDs as
templated, gated resources so the Target Allocator's prometheusCR scraping works
with no manual CRD prerequisite (matching the prometheus-operator experience).

- templates/prometheus-crds/{servicemonitor,podmonitor}-crd.yaml: templated + gated,
  skipped when an unmanaged copy already exists (Helm lookup) so a
  prometheus-operator-managed CRD is never clobbered; annotated
  helm.sh/resource-policy: keep so uninstall does not cascade-delete the CRs.
- values.yaml: prometheusCRDs.install (auto|always|never, default auto); auto
  bundles only when otelContainerInsights is enabled.
- _helpers.tpl: prometheusCRDsEnabled gating helper.
- tests/prometheus_crds_matrix.sh: helm template render tests for the gating.

The Target Allocator CRD-watch RBAC is deliberately not included here; it ships in
its own PR (aws-observability#327).
The prior comment attributed a $-collapsing pass to Helm. Helm passes the text
through unchanged; the two $$->$ collapses happen in the agent's OTel confmap
stack (legacy expandconverter + the resolver's built-in ${...} expansion), so
$$$1 lands as the literal $1 regex backreference. Comment-only; render unchanged.
- "--auto-instrumentation-nodejs-image={{ template "auto-instrumentation-nodejs.image" . }}"
- "--target-allocator-image={{ template "target-allocator.image" (merge .Values.agent.prometheus.targetAllocator.image (dict "region" $.Values.region)) }}"
- "--feature-gates=operator.autoinstrumentation.multiinstrumentation,operator.autoinstrumentation.multiinstrumentation.skipcontainervalidation"
- "--feature-gates=operator.autoinstrumentation.multi-instrumentation,operator.autoinstrumentation.multi-instrumentation.skip-container-validation"

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same --feature-gates issue as #329: hyphenated multi-instrumentation IDs the pinned operator doesn't register, so it CrashLoopBackOffs. Since it comes from #329, could we fix it there and rebase?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do

@@ -0,0 +1,1442 @@
{{- /*
ServiceMonitor CRD (monitoring.coreos.com/v1), pinned to prometheus-operator v0.91.0 to

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The header pins these CRDs to prometheus operator v0.91.0, but go.mod is actually on v0.92.0, so the API server prunes any field added between the two from a user's CR. Could we refetch both CRDs from the v0.92.0 tag (or fix the header)? Same in podmonitor-crd.yaml.

*/ -}}
{{- if eq (include "amazon-cloudwatch-observability.prometheusCRDsEnabled" .) "true" }}
{{- $crdName := "servicemonitors.monitoring.coreos.com" }}
{{- $existing := lookup "apiextensions.k8s.io/v1" "CustomResourceDefinition" "" $crdName }}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lookup is empty under helm template, so the bundled CRD always applies and can downgrade a GitOps user's newer schema, and the adoption check only reads meta.helm.sh/release-name. Could we add a clean opt out and also match meta.helm.sh/release-namespace so a cluster scoped CRD can't get cross adopted?


printf "\n${Y}== Target Allocator CRD RBAC ==${N}\n"
# The TA needs customresourcedefinitions get;list;watch on the prometheus-CR path.
expect_count "CRD RBAC present when otelCI on" "$CRD_RBAC" 1 --set otelContainerInsights.enabled=true

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The CRD RBAC assertion on line 73 expects customresourcedefinitions, but that RBAC lands in a separate PR so this branch renders zero, and set -euo pipefail fails the whole suite right here. Could we move those two assertions into the RBAC PR, or guard them until the rule exists?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The CRD_RBAC assertions are correct, they only fail because this branch hasn't re-synced onto #329 yet, where the CRD watch RBAC now lives

@@ -0,0 +1,78 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This new script uses bash only features but has no shebang, and the SM_CRD/PM_CRD grep patterns are unanchored with unescaped dots that can over count. Could we add #!/usr/bin/env bash and anchor the patterns with ^/$ and escaped dots?

Returns the string "true" when CRDs should be rendered, empty otherwise.
*/}}
{{- define "amazon-cloudwatch-observability.prometheusCRDsEnabled" -}}
{{- $install := "auto" -}}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

prometheusCRDsEnabled only matches always and never exactly, so anything else falls through and bundles the CRDs, meaning a typo like nevr silently bundles when otelCI is on. Could we add a fail branch for anything that isn't auto, always, or never, like the sibling key already does?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

split auto into its own branch and added a catch-all else that fails with prometheusCRDs.install must be one of "auto", "always", or "never", got: , matching the metricResolution sibling.

## annotated helm.sh/resource-policy: keep so `helm uninstall` does NOT delete
## them (which would cascade-delete every ServiceMonitor/PodMonitor in the
## cluster).
prometheusCRDs:

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This adds prometheusCRDs.install at the top level, but a later PR moves it under otelContainerInsights.prometheusScrape.crds.install with no alias, so someone who sets never now has it silently ignored after upgrading. Could we hold off calling it stable, or add an alias in that later PR?

@wenegiemepraise wenegiemepraise Jul 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll ship an alias when #332 relocates it: otelContainerInsights.prometheusScrape.crds.install will fall back to the legacy prometheusCRDs.install and emit a deprecation warning, so a user's never keeps working across the upgrade

- nonResourceURLs: ["/metrics"]
verbs: ["get"]
{{- if and (hasKey ($customAgent.prometheus.targetAllocator) "prometheusCR") $customAgent.prometheus.targetAllocator.prometheusCR.enabled }}
{{- if or (and (hasKey ($customAgent.prometheus) "targetAllocator") (hasKey ($customAgent.prometheus.targetAllocator) "prometheusCR") $customAgent.prometheus.targetAllocator.prometheusCR.enabled) $otelCIScrape }}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This branch's TA ClusterRole has the SM/PM rule but not a customresourcedefinitions rule, which lives in #327, so on its own the target-allocator can't list CRDs to start discovery. Worth making sure #327 lands alongside this so they gate together.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

handled on #329

The Target Allocator watches the ServiceMonitor/PodMonitor CRDs to start/stop
informers as CRDs appear or disappear. Under the otelContainerInsights scraping
path the TA ClusterRole granted list/watch on the monitors but not on
customresourcedefinitions, so the operator-side CRD watch was denied, the CRD
gate never opened, and SM/PM discovery failed quietly. Add the read-only
get/list/watch grant on customresourcedefinitions beside the monitoring.coreos.com
rule under the same $otelCIScrape gate. Comment-only render change; gated off by
default.
This branch had flipped the operator --feature-gates to the hyphenated
multi-instrumentation / multi-instrumentation.skip-container-validation form.
The pinned operator (cloudwatch-agent-operator:3.5.0) registers the
unhyphenated IDs (operator.autoinstrumentation.multiinstrumentation and
.multiinstrumentation.skipcontainervalidation), so the hyphenated forms are
unrecognized; the manager parses gates with ExitOnError and CrashLoopBackOffs,
which stops all CR reconciliation and breaks the whole chart on install/upgrade.
Out of scope for the scraping work; restore the unhyphenated IDs to match main.
…ment

The transform/cw_k8s_ci_v0_set_scope_prometheuscr processor was defined
unconditionally, while its receiver and pipeline are guarded by
serviceMonitor.enabled OR podMonitor.enabled. With otelContainerInsights
enabled but both monitors disabled, the processor rendered orphaned (declared,
referenced by no pipeline), which the collector warns about. Wrap it in the
same guard so the receiver, pipeline and processor stay in sync.
The TA ClusterRole and ClusterRoleBinding rendered inside range .Values.agents
with a static object name, so if more than one agent satisfied the gate (which
$otelCIScrape widens), two identically named objects rendered and Kubernetes
applied last-write-wins, silently clobbering one. All TAs share one
ServiceAccount (target-allocator-service-acct), so a single ClusterRole/Binding
suffices: pre-scan agents into needsTA/needsCR and render exactly once, with the
SM/PM + customresourcedefinitions rules included when any agent needs discovery.
Renders once even with multiple matching agents; verified 1 object each.
….92.0

The bundled ServiceMonitor/PodMonitor CRDs were pinned to v0.91.0 while the
operator's go.mod (the Target Allocator's prometheus-operator client) pins
v0.92.0. Refetched both from the v0.92.0 tag and updated the headers, preserving
the Helm gating (prometheusCRDsEnabled + skip-if-exists) and metadata edits
(helm.sh/resource-policy: keep + chart labels). v0.92.0 adds no fields to SM/PM,
so no admission-time pruning; the real delta is version annotations and a URL
validation (minLength -> pattern) that now matches the operator's client.
The bundled CRD skip-if-exists guard only re-rendered (adopted/upgraded) an
existing CRD when meta.helm.sh/release-name matched. CRDs are cluster-scoped, so
a same-named release in a different namespace could cross-adopt one. Also require
meta.helm.sh/release-namespace to match.

Document the GitOps caveat: lookup is empty under helm template, so the bundled
CRD always applies and can overwrite an externally managed CRD; set
prometheusCRDs.install: never to opt out (that opt-out already exists).
The prometheus_crds_matrix.sh uses bash-only features but had no shebang; add
#!/usr/bin/env bash and the exec bit to match flag_matrix.sh. The SM_CRD/PM_CRD
grep patterns were unanchored with unescaped regex dots that could over-count;
anchor them with ^[[:space:]]* ... $ and escaped dots for exact metadata.name
matches. Counts unchanged (verified).
prometheusCRDsEnabled matched "always"/"never" exactly and let any other
value fall through to the otelContainerInsights.enabled branch, so a mistyped
opt-out (e.g. "nevr") failed open and silently bundled the CRDs when otelCI was
on. Split "auto" into its own branch and add a catch-all else that fails with a
clear message (matching the metricResolution sibling), so unknown values are
rejected at render time regardless of otelCI state.
…bundling

Re-sync onto the updated scraping-path branch to inherit the feature-gate ID
fix, the co-gated customresourcedefinitions RBAC rule (render-once ClusterRole/
Binding), the $$$1 comment correction, and the prometheuscr scope-processor
guard. Resolves the CRD-RBAC matrix assertion (now 11/0) and the feature-gate
blocker on this branch.
allocationStrategy was emitted under $taEnabled with a hardcoded default of
per-node, so it also flipped classic static-config Target Allocators (enabled
without otelContainerInsights) from consistent-hashing to per-node -- beyond
this PR's scope. Default per-node only on the $otelCIScrape path and
consistent-hashing otherwise, while still honoring an explicit
allocationStrategy override. Drop the redundant | default "per-node" (values.yaml
no longer hardcodes it; the template now defaults context-aware).
…output

The per-node pipeline note claimed end-to-end verification via target_node
matching k8s.node.name, but the chart only stamps k8s.node.name (the scraping
agent's own node); target_node is not emitted by the chart. Reword the note so
target_node is described as a prerequisite the user supplies via SM/PM
relabeling, and note that k8s.node.name is stamped unconditionally.
The prometheusCR pipeline ran set_node_name/promote_node_name, unconditionally
overwriting k8s.node.name with the scraping agent's own node. That is only
correct when per-node placement succeeds; under the consistent-hashing fallback
(cross-node target) it mislabels the metric, and it clobbers any node label the
target/relabeling already set. Per-node placement is verified via the Target
Allocator logs, so the stamp is not needed here -- remove both transforms from
the prometheusCR pipeline. The shared transform definitions stay for the
node-level pipelines (cadvisor, node_exporter, etc.) where the agent's node is
the metric's node.
…node-allocation

Re-sync aws-observability#330 onto the updated scraping-path branch: inherit the feature-gate ID
fix, co-gated CRD RBAC render-once ClusterRole/Binding, the $$$1 comment
correction, and the prometheuscr scope-processor guard, alongside aws-observability#330's per-node
work.
…bundling

Re-sync aws-observability#331 onto aws-observability#330 so the stack is linear (aws-observability#329 -> aws-observability#330 -> aws-observability#331): inherit
the per-node allocationStrategy scoping/gating, the SM/PM node-stamping removal,
and the context-aware allocationStrategy default, on top of aws-observability#331's CRD bundling.
wenegiemepraise added a commit to wenegiemepraise/helm-charts that referenced this pull request Jul 23, 2026
…cture

Re-sync aws-observability#332 onto the updated stack (aws-observability#329->aws-observability#330->aws-observability#331). Resolutions favor the
settled older-PR fixes applied onto aws-observability#332's prometheusScrape.* restructure:
- prometheusCRDsEnabled: keep aws-observability#331's explicit auto branch + invalid-value fail,
  gated on aws-observability#332's otelContainerInsights.enabled && prometheusScrape.enabled.
- allocationStrategy: use aws-observability#332's precomputed $allocationStrategy (per-node on the
  otelCI path via prometheusScrape.allocationStrategy, consistent-hashing for
  direct-TA) -- supersedes aws-observability#330's ternary while preserving its intent.
- Fix a stale old-key guard (otelContainerInsights.serviceMonitor.enabled) left by
  auto-merge in the prometheuscr scope-processor to prometheusScrape.enabled.
- Keep aws-observability#330's SM/PM node-stamping removal; port aws-observability#331's GitOps opt-out caveat into
  the new prometheusScrape.crds doc.
Verified: lint clean, CRD matrix 21/0, flag matrix 8/8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants