diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md new file mode 100644 index 0000000..23f945d --- /dev/null +++ b/PR_DESCRIPTION.md @@ -0,0 +1,191 @@ +# Make ServiceMonitor labels configurable for Prometheus discovery + +Closes #97. + +## Summary + +The chart's `ServiceMonitor` resource hardcoded its `metadata.labels` to the +five chart-managed labels emitted by the `livekit-server.labels` helper. +Prometheus operators that use a non-empty `serviceMonitorSelector` (notably +`kube-prometheus-stack`, which defaults to +`spec.serviceMonitorSelector.matchLabels.release: `) could not +discover the ServiceMonitor because the operator-required selector label was +missing. + +This PR adds a single `serviceMonitor.labels` field (an empty map by default) +that is merged into `metadata.labels` alongside the chart-managed labels. The +field is opt-in: operators who do not set it see byte-identical output to the +prior chart. + +## Changes + +- `livekit-server/values.yaml`: new `serviceMonitor.labels: {}` field with + comment documenting the kube-prometheus-stack `release: prometheus` + example and pointing operators at their Prometheus + `serviceMonitorSelector` configuration. +- `livekit-server/templates/servicemonitor.yaml`: added a + `{{- with .Values.serviceMonitor.labels }}{{- toYaml . | nindent 4 }}{{- end }}` + block immediately after the chart-managed labels, mirroring the existing + `annotations` pattern lower in the same file. +- `livekit-server/tests/servicemonitor_labels_test.yaml` (new): helm-unittest + suite covering five scenarios. Details in the Testing section below. +- `livekit-server/tests/upgrade_compatibility_test.yaml` (new): helm-unittest + suite that asserts byte-identical default rendering. + +## Multi-chart consistency + +Only `livekit-server` ships a ServiceMonitor template +(`livekit-server/templates/servicemonitor.yaml`). The `egress` and `ingress` +charts have no ServiceMonitor, no `serviceMonitor` block in their +`values.yaml`, and no Prometheus-specific configuration at all (verified by +`grep -rl 'ServiceMonitor\|serviceMonitor' egress/ ingress/` returning no +matches). The fix is therefore applied only where it has a target. + +## Default behavior vs prior chart + +| Scenario | Before | After | +|-------------------------------------------------------------------------|-----------------|-----------------| +| `serviceMonitor.create: false` (chart default) | no ServiceMonitor | no ServiceMonitor | +| `serviceMonitor.create: true` and `serviceMonitor.labels: {}` (default) | 5 chart labels | 5 chart labels (byte-identical) | +| `serviceMonitor.create: true` with `labels.release: prometheus` | 5 chart labels (Prometheus selector misses) | 5 chart labels + `release: prometheus` (Prometheus selector matches) | + +The diff between prior chart and this branch with default values is +**empty**, verified by `helm template ... | diff` and asserted in +`tests/upgrade_compatibility_test.yaml`. + +## Backwards compatibility + +- No values keys removed or renamed. +- One opt-in field added. +- Default rendering is byte-identical (zero diff) confirmed by `helm template` + comparison and locked in by a unit test. +- **Edge case**: any existing user who already had `serviceMonitor.labels: {...}` + set in their values (a no-op before this change because the template + ignored it) will see those labels start applying after upgrade. This is + "configuration that previously was silently ignored now does what the + user clearly intended," but operators in this position should review their + values.yaml to confirm the labels they wrote are still what they want + applied. + +## Testing + +Tests are written for the [helm-unittest][hu] plugin +(`helm plugin install https://github.com/helm-unittest/helm-unittest`). + +[hu]: https://github.com/helm-unittest/helm-unittest + +### Scenarios covered + +`tests/servicemonitor_labels_test.yaml` (7 tests): + +- **Scenario A - default values** (3 tests): A1 confirms no ServiceMonitor + renders at chart defaults (`create=false`); A2 confirms chart-managed + labels are intact when ServiceMonitor is enabled; A3 deep-equals the + full `metadata.labels` map against the five chart-managed entries to + guarantee no accidental leakage from the new code path. +- **Scenario B - single label override** (1 test): setting + `serviceMonitor.labels.release: prometheus` adds the label and preserves + all five chart-managed labels. +- **Scenario C - multiple label override** (1 test, 7 assertions): three + custom labels render alongside the five chart-managed labels with no + overwrites or losses; uses a fixture file + (`tests/fixtures/labels_three.yaml`). +- **Scenario D - kube-prometheus-stack realistic config** (1 test): + asserts the rendered ServiceMonitor would be discovered by a Prometheus + instance whose `serviceMonitorSelector` matches `release: prometheus`, + and that the spec block (apiVersion, endpoints, selector) is valid for + scraping. +- **Scenario E - empty override** (1 test): explicitly setting + `serviceMonitor.labels: {}` produces output identical to the default + case (no malformed/empty labels block); uses a fixture file + (`tests/fixtures/labels_empty.yaml`). + +`tests/upgrade_compatibility_test.yaml` (3 tests): asserts no ServiceMonitor +renders at chart defaults, asserts deep-equality on `metadata.labels` for +the enabled-with-default-labels case (catches any leakage), and asserts the +spec block is unchanged. The suite header documents the manual reproduction +recipe and the captured diff (empty). + +### Running the tests + +``` +$ helm unittest livekit-server/ # full suite +$ helm unittest -f 'tests/servicemonitor_labels_test.yaml' livekit-server/ # just labels +$ helm unittest -f 'tests/upgrade_compatibility_test.yaml' livekit-server/ # just upgrade +``` + +Test names are scenario-prefixed (`A1.`, `A2.`, `A3.`, `B1.`, `C1.`, `D1.`, +`E1.`) so a failure line points at the exact scenario. + +### Coverage gaps (out of scope for this PR) + +- **No live-cluster install test of label discovery**. The repo's existing + `ct install` matrix runs `helm install` on microk8s but does not stand up + a Prometheus instance and verify scrape target discovery. The unit test + asserts the rendered ServiceMonitor would be selected by a + `release: prometheus` selector, but does not deploy Prometheus to confirm. +- **No CI step yet runs `helm unittest`** in this repo. The current CI + runs `ct lint` and `ct install` only. To wire these tests into CI, a + maintainer would add: + + ```yaml + - name: Install helm-unittest plugin + run: helm plugin install https://github.com/helm-unittest/helm-unittest + + - name: Run helm unittest + run: helm unittest livekit-server/ + ``` + + after the existing "Set up Helm" step (CI uses Helm v3.11.2; Helm v4 + users locally may need `--verify=false` on the plugin install). I have + intentionally not modified the workflow file in this PR to keep the + chart-change diff focused; happy to add the CI step in this PR or a + follow-up if maintainers prefer. +- **No test for label-key collisions** between user-supplied labels and + chart-managed labels. With user labels rendered after chart-managed + labels, a user setting (for example) `serviceMonitor.labels.app.kubernetes.io/name` + would shadow the chart-managed value. This matches the convention used + by widely-deployed charts but is documented here rather than tested. +- **No annotations changes**. Issue #97 is scoped to labels; + `serviceMonitor.annotations` already exists and is unchanged. + +### Local verification + +``` +$ helm lint livekit-server/ +==> Linting livekit-server/ +[INFO] Chart.yaml: icon is recommended +1 chart(s) linted, 0 chart(s) failed + +$ helm unittest livekit-server/ +### Chart [ livekit-server ] livekit-server/ + PASS ServiceMonitor configurable labels livekit-server/tests/servicemonitor_labels_test.yaml + PASS upgrade compatibility - ServiceMonitor render is byte-identical when serviceMonitor.labels is left at default livekit-server/tests/upgrade_compatibility_test.yaml + +Charts: 1 passed, 1 total +Test Suites: 2 passed, 2 total +Tests: 10 passed, 10 total +Snapshot: 0 passed, 0 total + +$ diff <(git show master:livekit-server/templates/servicemonitor.yaml | helm template lk-test livekit-server/ -f - --set serviceMonitor.create=true --set livekit.prometheus_port=6789) \ + <(helm template lk-test livekit-server/ --set serviceMonitor.create=true --set livekit.prometheus_port=6789) +[empty: zero diff with default labels] +``` + +## Path Considered But Not Taken + +This PR adds a single `labels` field to the existing `serviceMonitor` block. +An alternative approach considered was a more comprehensive metadata block +(annotations, labels, name overrides) which would address potential future +configuration needs preemptively. That approach was not taken because: + +- Issue #97 is scoped specifically to the labels problem, not broader + metadata configuration. +- Adding fields without a current concrete user need expands the chart's + API surface unnecessarily. +- Future issues asking for annotations or other metadata can be addressed + in follow-up PRs with the same pattern. The existing + `serviceMonitor.annotations` field is already in place; further metadata + fields would slot in alongside without a structural change. + +The current scope is the minimum change that resolves #97 cleanly. diff --git a/livekit-server/templates/servicemonitor.yaml b/livekit-server/templates/servicemonitor.yaml index 73b62a9..80c35e9 100644 --- a/livekit-server/templates/servicemonitor.yaml +++ b/livekit-server/templates/servicemonitor.yaml @@ -5,6 +5,9 @@ metadata: name: {{ include "livekit-server.serviceMonitorName" . }} labels: {{- include "livekit-server.labels" . | nindent 4 }} + {{- with .Values.serviceMonitor.labels }} + {{- toYaml . | nindent 4 }} + {{- end }} {{- with .Values.serviceMonitor.annotations }} annotations: {{- toYaml . | nindent 4 }} diff --git a/livekit-server/tests/fixtures/labels_empty.yaml b/livekit-server/tests/fixtures/labels_empty.yaml new file mode 100644 index 0000000..2a8a59b --- /dev/null +++ b/livekit-server/tests/fixtures/labels_empty.yaml @@ -0,0 +1,5 @@ +serviceMonitor: + create: true + labels: {} +livekit: + prometheus_port: 6789 diff --git a/livekit-server/tests/fixtures/labels_three.yaml b/livekit-server/tests/fixtures/labels_three.yaml new file mode 100644 index 0000000..bac49ae --- /dev/null +++ b/livekit-server/tests/fixtures/labels_three.yaml @@ -0,0 +1,8 @@ +serviceMonitor: + create: true + labels: + release: prometheus + team: platform + environment: production +livekit: + prometheus_port: 6789 diff --git a/livekit-server/tests/servicemonitor_labels_test.yaml b/livekit-server/tests/servicemonitor_labels_test.yaml new file mode 100644 index 0000000..95d49f8 --- /dev/null +++ b/livekit-server/tests/servicemonitor_labels_test.yaml @@ -0,0 +1,181 @@ +suite: ServiceMonitor configurable labels +release: + name: lk-test + namespace: livekit +tests: + # --------------------------------------------------------------------------- + # Scenario A: default values + # --------------------------------------------------------------------------- + - it: A1. ServiceMonitor is NOT rendered when serviceMonitor.create is false (chart default) + template: servicemonitor.yaml + asserts: + # The template guards on `if and .Values.serviceMonitor.create + # .Values.livekit.prometheus_port`. With both at chart defaults + # (create=false, prometheus_port unset) nothing should render. + - hasDocuments: + count: 0 + + - it: A2. ServiceMonitor renders with chart-managed labels intact when enabled with no custom labels + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + asserts: + - hasDocuments: + count: 1 + - isKind: + of: ServiceMonitor + - equal: + path: metadata.labels["app.kubernetes.io/name"] + value: livekit-server + - equal: + path: metadata.labels["app.kubernetes.io/instance"] + value: lk-test + - equal: + path: metadata.labels["app.kubernetes.io/managed-by"] + value: Helm + - matchRegex: + path: metadata.labels["helm.sh/chart"] + pattern: ^livekit-server-.+$ + - exists: + path: metadata.labels["app.kubernetes.io/version"] + + - it: A3. with default serviceMonitor.labels (empty map), no labels appear beyond chart-managed five + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + asserts: + # The five chart-managed label keys are: helm.sh/chart, + # app.kubernetes.io/name, app.kubernetes.io/instance, + # app.kubernetes.io/version, app.kubernetes.io/managed-by. + # If the new field accidentally injected anything when empty, this + # test would catch it. + - equal: + path: metadata.labels + value: + helm.sh/chart: livekit-server-1.11.0 + app.kubernetes.io/name: livekit-server + app.kubernetes.io/instance: lk-test + app.kubernetes.io/version: v1.11.0 + app.kubernetes.io/managed-by: Helm + + # --------------------------------------------------------------------------- + # Scenario B: single label override + # --------------------------------------------------------------------------- + - it: B1. setting serviceMonitor.labels.release adds the label without dropping chart-managed labels + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + serviceMonitor.labels.release: prometheus + asserts: + - equal: + path: metadata.labels.release + value: prometheus + # Chart-managed labels still present + - equal: + path: metadata.labels["app.kubernetes.io/name"] + value: livekit-server + - equal: + path: metadata.labels["app.kubernetes.io/instance"] + value: lk-test + - equal: + path: metadata.labels["app.kubernetes.io/managed-by"] + value: Helm + - exists: + path: metadata.labels["helm.sh/chart"] + - exists: + path: metadata.labels["app.kubernetes.io/version"] + + # --------------------------------------------------------------------------- + # Scenario C: multiple label override + # --------------------------------------------------------------------------- + - it: C1. three custom labels all render alongside chart-managed labels with no overwrites + template: servicemonitor.yaml + values: + - ./fixtures/labels_three.yaml + asserts: + # Three custom labels present + - equal: + path: metadata.labels.release + value: prometheus + - equal: + path: metadata.labels.team + value: platform + - equal: + path: metadata.labels.environment + value: production + # Chart-managed labels still intact and unchanged + - equal: + path: metadata.labels["app.kubernetes.io/name"] + value: livekit-server + - equal: + path: metadata.labels["app.kubernetes.io/instance"] + value: lk-test + - equal: + path: metadata.labels["app.kubernetes.io/managed-by"] + value: Helm + - matchRegex: + path: metadata.labels["helm.sh/chart"] + pattern: ^livekit-server-.+$ + + # --------------------------------------------------------------------------- + # Scenario D: kube-prometheus-stack realistic config + # --------------------------------------------------------------------------- + - it: D1. rendered ServiceMonitor matches kube-prometheus-stack default selector (release=prometheus) + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + serviceMonitor.labels.release: prometheus + asserts: + # kube-prometheus-stack installs Prometheus with + # spec.serviceMonitorSelector.matchLabels.release = + # The default helm release name is "prometheus", giving a selector of + # release=prometheus. A ServiceMonitor with metadata.labels.release=prometheus + # will be discovered. + - equal: + path: metadata.labels.release + value: prometheus + - isKind: + of: ServiceMonitor + - equal: + path: apiVersion + value: monitoring.coreos.com/v1 + # Spec must remain valid for Prometheus to actually scrape + - equal: + path: spec.endpoints[0].port + value: metrics + - equal: + path: spec.endpoints[0].path + value: / + - equal: + path: spec.endpoints[0].interval + value: 30s + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/name"] + value: livekit-server + + # --------------------------------------------------------------------------- + # Scenario E: empty override + # --------------------------------------------------------------------------- + - it: E1. explicitly empty serviceMonitor.labels produces output identical to default (no malformed labels block) + template: servicemonitor.yaml + values: + - ./fixtures/labels_empty.yaml + asserts: + # Same five labels as Scenario A3, no more, no less + - equal: + path: metadata.labels + value: + helm.sh/chart: livekit-server-1.11.0 + app.kubernetes.io/name: livekit-server + app.kubernetes.io/instance: lk-test + app.kubernetes.io/version: v1.11.0 + app.kubernetes.io/managed-by: Helm + # Sanity: rendered doc is structurally a ServiceMonitor, not malformed + - isKind: + of: ServiceMonitor + - exists: + path: spec.endpoints diff --git a/livekit-server/tests/upgrade_compatibility_test.yaml b/livekit-server/tests/upgrade_compatibility_test.yaml new file mode 100644 index 0000000..09e9128 --- /dev/null +++ b/livekit-server/tests/upgrade_compatibility_test.yaml @@ -0,0 +1,76 @@ +suite: upgrade compatibility - ServiceMonitor render is byte-identical when serviceMonitor.labels is left at default +# +# Purpose +# ------- +# Issue #97 added an opt-in field. Operators who do NOT set +# serviceMonitor.labels must see no change in the rendered ServiceMonitor on +# upgrade. This suite locks that contract in. +# +# How to reproduce the underlying before/after diff manually +# ---------------------------------------------------------- +# git checkout master -- livekit-server/templates/servicemonitor.yaml \ +# livekit-server/values.yaml +# helm template lk-test livekit-server/ \ +# --set serviceMonitor.create=true \ +# --set livekit.prometheus_port=6789 > /tmp/before.yaml +# git checkout HEAD -- livekit-server/templates/servicemonitor.yaml \ +# livekit-server/values.yaml +# helm template lk-test livekit-server/ \ +# --set serviceMonitor.create=true \ +# --set livekit.prometheus_port=6789 > /tmp/after.yaml +# diff -u /tmp/before.yaml /tmp/after.yaml +# +# Captured at commit time: empty diff. The new default serviceMonitor.labels +# is an empty map and the {{- with .Values.serviceMonitor.labels }} guard +# produces zero bytes when the map is empty, so the rendered output is +# byte-identical to the prior chart. + +release: + name: lk-test + namespace: livekit +tests: + - it: with serviceMonitor disabled (chart default), no ServiceMonitor renders + template: servicemonitor.yaml + asserts: + - hasDocuments: + count: 0 + + - it: with serviceMonitor enabled and labels at default, rendered metadata.labels equals exactly the chart-managed five + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + asserts: + # Asserting deep-equality on metadata.labels guarantees: exactly five + # keys, no more, no fewer. If the new code path leaks anything when + # labels is the default empty map, this assertion fails. + - equal: + path: metadata.labels + value: + helm.sh/chart: livekit-server-1.11.0 + app.kubernetes.io/name: livekit-server + app.kubernetes.io/instance: lk-test + app.kubernetes.io/version: v1.11.0 + app.kubernetes.io/managed-by: Helm + + - it: spec block is unchanged from prior chart for default configuration + template: servicemonitor.yaml + set: + serviceMonitor.create: true + livekit.prometheus_port: 6789 + asserts: + - equal: + path: spec.endpoints[0].port + value: metrics + - equal: + path: spec.endpoints[0].path + value: / + - equal: + path: spec.endpoints[0].interval + value: 30s + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/name"] + value: livekit-server + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/instance"] + value: lk-test diff --git a/livekit-server/values.yaml b/livekit-server/values.yaml index bcf6d51..31bbb75 100644 --- a/livekit-server/values.yaml +++ b/livekit-server/values.yaml @@ -145,6 +145,11 @@ serviceMonitor: create: false # Annotations to add to the service monitor annotations: {} + # Additional labels for the ServiceMonitor resource. Required by some + # Prometheus deployments. For example, kube-prometheus-stack expects + # 'release: prometheus' to discover ServiceMonitors. Set per your + # Prometheus serviceMonitorSelector configuration. + labels: {} # The name of the service monitor to use. # If not set and create is true, a name is generated using the fullname template name: ""