[wip]: add labels for prometheus monitoring#199
[wip]: add labels for prometheus monitoring#199pratapalakshmi wants to merge 2 commits intomasterfrom
Conversation
- Updated values.yaml to switch ingressClass from 'nginx' to 'traefik-external'. - Added new ingress-traefik.yaml template for Traefik IngressRoute configuration. - Introduced traefik-middleware.yaml for request body size limiting middleware. - Modified existing ingress.yaml to conditionally render based on the selected ingressClass.
- Introduced a new monitoring section in values.yaml to enable Prometheus scraping. - Added Prometheus annotations to service templates for various components (admin, api, live, minio, postgres, rabbitmq, redis, silo, space, web) to facilitate metrics collection. - Updated ingress-traefik.yaml with additional labels for better resource management.
WalkthroughThe pull request adds Prometheus monitoring support and Traefik ingress routing capabilities to the Plane Enterprise Helm chart. It introduces standard Kubernetes labeling conventions across workload resources, switches the default ingress controller configuration, and enriches metadata with conditional Prometheus scraping annotations. Changes
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pratapalakshmi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@charts/plane-enterprise/templates/ingress-traefik.yaml`:
- Line 3: The chart declares apiVersion: traefik.io/v1alpha1 which requires
Traefik v2.10+ (or v3); update the Helm chart metadata and docs to prevent
installs on older Traefik: add a clear minimum Traefik version statement (e.g.,
"Traefik >= v2.10") to Chart.yaml and the chart README, and mention that this
template uses traefik.io/v1alpha1 so older Traefik (traefik.containo.us/v1alpha1
for v2.0–v2.9) is incompatible; ensure the README and Chart.yaml's
notes/description reference the minimum Traefik version.
In `@charts/plane-enterprise/templates/workloads/postgres.stateful.yaml`:
- Around line 8-11: The Prometheus scrape annotations added by the include
"plane.prometheusAnnotations" with port (.Values.services.postgres.servicePort)
are incorrect for the vanilla postgres image (no /metrics); either remove those
annotations for the Postgres StatefulSet/Service or add a postgres_exporter
sidecar and change the scrape port to the exporter (e.g., 9187). Update the
postgres.stateful.yaml template to stop including plane.prometheusAnnotations
when targeting the database container, or add a postgres_exporter container and
expose its port, then adjust .Values.services.postgres.servicePort (or pass a
different port into plane.prometheusAnnotations) so Prometheus scrapes the
exporter's endpoint instead of 5432.
In `@charts/plane-enterprise/templates/workloads/redis.stateful.yaml`:
- Around line 49-53: The label value using {{ .Values.planeVersion }} is
unquoted and may be YAML-coerced to a number; update every workload template
that sets the app.kubernetes.io/version label (e.g., the redis StatefulSet label
block that currently contains app.kubernetes.io/version: {{ .Values.planeVersion
}}) to use a quoted Helm template expression (i.e., wrap the template in double
quotes) so the rendered value is preserved as a string; apply the same change
anywhere else the bare {{ .Values.planeVersion }} appears in labels across
workload templates.
In `@charts/plane-enterprise/values.yaml`:
- Around line 28-30: The default Ingress class was switched to Traefik causing
silent, breaking upgrades; revert the default in values.yaml by setting
ingressClass back to 'nginx' and keep the nginx-specific ingress_annotations
("nginx.ingress.kubernetes.io/proxy-body-size": "5m"), and add a clear comment
near the ingressClass and ingress_annotations keys explaining how to opt into
Traefik (e.g., set ingressClass: 'traefik-external' and remove/override nginx
annotations) so users won’t be migrated unexpectedly; update any
README/changelog references if present to reflect that Traefik is opt-in.
🧹 Nitpick comments (2)
charts/plane-enterprise/templates/_helpers.tpl (1)
30-37: The internal guard in the helper duplicates the check at every call site.Every caller already wraps the
includein{{- if and .Values.monitoring .Values.monitoring.enabled }}. Since the helper also checks the same condition, one of the two is redundant.Consider removing the guard from either the helper (relying on call-site checks) or from all call sites (relying on the helper to self-guard and emit nothing). The latter is cleaner — it would eliminate the repeated
ifblocks across ~10 workload templates.charts/plane-enterprise/templates/ingress-traefik.yaml (1)
19-19: Misleading comment: Traefik IngressRoute uses rule length for priority, not YAML order.Traefik calculates route priority from rule length by default (longer rules = higher priority), so the YAML ordering doesn't actually affect evaluation. The comment "specific paths FIRST" may confuse future maintainers into thinking reordering would change behavior. Consider updating it to clarify priority semantics, or simply remove it.
| @@ -0,0 +1,89 @@ | |||
| {{- if and .Values.ingress.enabled (eq .Values.ingress.ingressClass "traefik-external") .Values.license.licenseDomain }} | |||
|
|
|||
| apiVersion: traefik.io/v1alpha1 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any references to the old Traefik API group in the chart
rg -n "traefik.containo.us" --type yaml
rg -n "traefik.io" --type yamlRepository: makeplane/helm-charts
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Check Chart.yaml for the plane-enterprise chart
cat -n charts/plane-enterprise/Chart.yaml
# Check if there's a values.yaml
ls -la charts/plane-enterprise/values.yaml
# Search for any Traefik version references or documentation
rg -i "traefik.*version|version.*traefik" charts/plane-enterprise/ --type yaml -A 2 -B 2Repository: makeplane/helm-charts
Length of output: 1440
🏁 Script executed:
#!/bin/bash
# Check values.yaml for Traefik-related configuration
rg -i "traefik" charts/plane-enterprise/values.yaml -B 2 -A 2Repository: makeplane/helm-charts
Length of output: 210
🌐 Web query:
Traefik v1alpha1 API group traefik.io vs traefik.containo.us version history
💡 Result:
API group history (Kubernetes CRDs) — traefik.containo.us vs traefik.io
-
Traefik v2.0 → v2.9: Traefik’s CRDs used the API group
traefik.containo.us(e.g.,traefik.containo.us/v1alpha1). This is shown in the original Traefik v2 CRD examples/docs. [1] [2] -
Traefik v2.10: Traefik introduced the new CRD API group
traefik.ioand deprecatedtraefik.containo.us(with the stated intention to remove it in v3). In v2.10, the CRD provider still supports bothtraefik.io/v1alpha1andtraefik.containo.us/v1alpha1during the transition. [3] [4] -
Traefik v3.x: The
traefik.containo.usAPI group is removed; you must usetraefik.io(e.g.,traefik.io/v1alpha1). [5]
Practical takeaway:
If you’re on v2.10.x, you can run with both installed while you migrate manifests, but v3 requires traefik.io/* only. [4] [5]
Sources
[1] https://traefik.io/blog/back-to-traefik-2-0-2f9aa17be305/
[2] https://doc.traefik.io/traefik/v2.4/reference/dynamic-configuration/kubernetes-crd/
[3] https://traefik.io/blog/announcing-traefik-proxy-2-10
[4] https://doc.traefik.io/traefik/v3.3/migration/v2/
[5] https://doc.traefik.io/traefik/master/migration/v2-to-v3-details/
Document the minimum Traefik version requirement.
The chart uses traefik.io/v1alpha1, which requires Traefik v2.10+ or v3. Older versions (v2.0–v2.9) use traefik.containo.us/v1alpha1 and are incompatible. Add a documented minimum version constraint to the Helm chart (Chart.yaml or README) to prevent silent failures on unsupported Traefik installations.
🤖 Prompt for AI Agents
In `@charts/plane-enterprise/templates/ingress-traefik.yaml` at line 3, The chart
declares apiVersion: traefik.io/v1alpha1 which requires Traefik v2.10+ (or v3);
update the Helm chart metadata and docs to prevent installs on older Traefik:
add a clear minimum Traefik version statement (e.g., "Traefik >= v2.10") to
Chart.yaml and the chart README, and mention that this template uses
traefik.io/v1alpha1 so older Traefik (traefik.containo.us/v1alpha1 for
v2.0–v2.9) is incompatible; ensure the README and Chart.yaml's notes/description
reference the minimum Traefik version.
| {{- if and .Values.monitoring .Values.monitoring.enabled }} | ||
| annotations: | ||
| {{- include "plane.prometheusAnnotations" (dict "port" (.Values.services.postgres.servicePort | toString) "root" .) | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Prometheus scrape annotations on PostgreSQL will fail — vanilla Postgres doesn't expose /metrics.
The standard postgres:15.7-alpine image doesn't serve a /metrics endpoint on port 5432. Prometheus will continuously fail to scrape this target, generating noise in logs and alerts.
Either:
- Remove Prometheus annotations from the Postgres Service (and similarly for Redis, which uses
valkey/valkeyand also doesn't expose/metrics). - Or add a
postgres_exportersidecar container and point the scrape port to the exporter's port (typically 9187).
🤖 Prompt for AI Agents
In `@charts/plane-enterprise/templates/workloads/postgres.stateful.yaml` around
lines 8 - 11, The Prometheus scrape annotations added by the include
"plane.prometheusAnnotations" with port (.Values.services.postgres.servicePort)
are incorrect for the vanilla postgres image (no /metrics); either remove those
annotations for the Postgres StatefulSet/Service or add a postgres_exporter
sidecar and change the scrape port to the exporter (e.g., 9187). Update the
postgres.stateful.yaml template to stop including plane.prometheusAnnotations
when targeting the database container, or add a postgres_exporter container and
expose its port, then adjust .Values.services.postgres.servicePort (or pass a
different port into plane.prometheusAnnotations) so Prometheus scrapes the
exporter's endpoint instead of 5432.
| app.kubernetes.io/name: plane-redis | ||
| app.kubernetes.io/component: redis | ||
| app.kubernetes.io/part-of: plane-enterprise | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| app.kubernetes.io/version: {{ .Values.planeVersion }} |
There was a problem hiding this comment.
Quote planeVersion in labels to avoid YAML type coercion.
If planeVersion ever holds a numeric-looking value (e.g., 1.0), YAML will parse it as a float and Helm will render it as 1, silently losing the intended version string. This applies to all workload templates in this PR that use {{ .Values.planeVersion }} in labels.
Proposed fix
- app.kubernetes.io/version: {{ .Values.planeVersion }}
+ app.kubernetes.io/version: {{ .Values.planeVersion | quote }}Apply the same change in every workload template where this label appears.
🤖 Prompt for AI Agents
In `@charts/plane-enterprise/templates/workloads/redis.stateful.yaml` around lines
49 - 53, The label value using {{ .Values.planeVersion }} is unquoted and may be
YAML-coerced to a number; update every workload template that sets the
app.kubernetes.io/version label (e.g., the redis StatefulSet label block that
currently contains app.kubernetes.io/version: {{ .Values.planeVersion }}) to use
a quoted Helm template expression (i.e., wrap the template in double quotes) so
the rendered value is preserved as a string; apply the same change anywhere else
the bare {{ .Values.planeVersion }} appears in labels across workload templates.
| # ingressClass: 'nginx' | ||
| ingress_annotations: { "nginx.ingress.kubernetes.io/proxy-body-size": "5m" } | ||
| ingressClass: 'traefik-external' |
There was a problem hiding this comment.
Breaking change: default ingressClass switched from nginx to traefik-external.
Existing users upgrading the chart will lose their nginx Ingress resource and instead get a Traefik IngressRoute, which requires Traefik CRDs and a Traefik controller to be installed. This is a silent, disruptive change with no migration path.
Additionally, line 29 still ships the nginx-specific annotation (nginx.ingress.kubernetes.io/proxy-body-size), which is meaningless for Traefik and will confuse users.
Consider one of:
- Keep
nginxas the default and let users opt in totraefik-external. - If Traefik is intentional as the new default, bump the chart's major version, update the annotation default to an empty map
{}, and document the migration in the changelog/README.
🤖 Prompt for AI Agents
In `@charts/plane-enterprise/values.yaml` around lines 28 - 30, The default
Ingress class was switched to Traefik causing silent, breaking upgrades; revert
the default in values.yaml by setting ingressClass back to 'nginx' and keep the
nginx-specific ingress_annotations
("nginx.ingress.kubernetes.io/proxy-body-size": "5m"), and add a clear comment
near the ingressClass and ingress_annotations keys explaining how to opt into
Traefik (e.g., set ingressClass: 'traefik-external' and remove/override nginx
annotations) so users won’t be migrated unexpectedly; update any
README/changelog references if present to reflect that Traefik is opt-in.
1 participant
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Chores