feat(monitoring): opt-in ServiceMonitor + PrometheusRule for the Mattermost CRD

[christopherfickess]

Summary

Proof-of-concept (draft) for SRE review — the "monitoring without Grafana dashboards" variant.

Adds an opt-in spec.monitoring block to the Mattermost CRD so the operator can create observability resources, each behind its own flag:

• ServiceMonitor targeting a dedicated internal headless metrics Service (<name>-metrics, port 8067). Using a dedicated Service means scraping works in every service mode — including useServiceLoadBalancer, where the app Service exposes only 80/443. Metrics stay internal; never published through the LB.
• PrometheusRule — a curated, Mattermost-focused alert set (server-down, crash-loop, CPU/mem vs limits, DB replica lag, HTTP 5xx rate, login-failure spike). Expressions are templated per-CR (__NAMESPACE__/__SERVICE__/__POD_SELECTOR__) so alerts scope to this install's pods by name/label — never a static IP/CIDR.
• clientMetrics (*bool) — explicit control of client/RUM + notification metrics; absent/empty leaves the server defaults untouched.
• callsMetrics — an optional second ServiceMonitor targeting a separately-deployed rtcd (Calls) Service, with label fallback so Prometheus still selects it.

Also: cleanup-on-disable (flipping a flag off deletes what it created, mirroring the Ingress pattern), a best-effort Enterprise-license warning (the /metrics endpoint is license-gated), CEL validation (rtcdServiceSelector required when callsMetrics.enabled), and RBAC/scheme wiring. Prometheus Operator CRDs are an optional dependency — creation degrades to a graceful skip when absent.

Validation: unit tests; envtest (real apiserver) for CEL rejection + reconcile create/delete; and a live kind + kube-prometheus-stack run confirming the PrometheusRule loads (valid PromQL, all 7 alerts) and the ServiceMonitor compiles into the scrape config with namespace/service/pod relabeling.

Companion PR (adds Grafana dashboards on top of this branch): the two are meant to be compared so SRE can adopt rules+ServiceMonitor independently of dashboards.

Ticket Link

Internal SRE monitoring POC — no public ticket.

Release Note

Add an opt-in `spec.monitoring` block to the Mattermost CRD: Prometheus Operator ServiceMonitor (scraping a dedicated internal metrics Service) and a Mattermost-focused, pod-scoped PrometheusRule, plus flags for client/RUM metrics and a Calls (rtcd) ServiceMonitor. Requires the Prometheus Operator CRDs; the /metrics endpoint requires an Enterprise license.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds opt-in Prometheus monitoring resources and configuration to Mattermost installations.

Changes:

• Adds ServiceMonitor, PrometheusRule, rtcd, and client metrics configuration.
• Reconciles monitoring resources with cleanup and optional-CRD handling.
• Adds alert rules, RBAC, documentation, and unit tests.

Reviewed changes

Copilot reviewed 14 out of 17 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
apis/mattermost/v1beta1/mattermost_types.go	Defines monitoring API types.
apis/mattermost/v1beta1/zz_generated.deepcopy.go	Adds generated deep-copy methods.
config/crd/bases/installation.mattermost.com_mattermosts.yaml	Adds monitoring CRD schema.
config/rbac/role.yaml	Grants monitoring resource permissions.
controllers/mattermost/mattermost/controller.go	Adds event recording support.
controllers/mattermost/mattermost/mattermost.go	Invokes monitoring reconciliation.
controllers/mattermost/mattermost/monitoring.go	Reconciles monitoring resources.
docs/examples/mattermost_monitoring.yaml	Provides a monitoring example.
go.mod	Adds Prometheus Operator dependencies.
go.sum	Updates dependency checksums.
main.go	Registers monitoring API types.
pkg/mattermost/mattermost_v1beta.go	Configures client metrics environment variables.
pkg/mattermost/monitoring.go	Generates monitoring resources and rules.
pkg/mattermost/monitoring_test.go	Tests monitoring generators.
pkg/mattermost/prometheusrules/mattermost.yaml	Defines Mattermost alert rules.
pkg/resources/create_resources.go	Adds monitoring resource CRUD helpers.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

pkg/mattermost/monitoring.go:118

• With spec.resourceLabels set, this map aliases the CR's map (mattermost_utils.go:299-304), so merging serviceMonitor.labels below also adds Prometheus discovery labels to the later rtcd/rule resources and Deployment pods. Work on a copy instead.

	labels := mattermost.MattermostLabels(mattermost.Name)

pkg/mattermost/monitoring.go:166

• This map can alias spec.resourceLabels, so applying calls-specific discovery labels at lines 174-175 mutates the CR and propagates those labels to the PrometheusRule and Deployment generated afterward. Clone the base labels first.

	labels := mattermost.MattermostLabels(mattermost.Name)

pkg/mattermost/monitoring.go:221

• When spec.resourceLabels is non-nil, this aliases that map, so merging prometheusRule.labels mutates the reconciled Mattermost object and causes rule-selector labels to appear on the Deployment/pods generated next. Clone the map before extending it.

	labels := mattermost.MattermostLabels(mattermost.Name)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[christopherfickess]

Thanks for the review — here's how each finding was handled.

Fixed (correctness):

• Service label mismatch — the ServiceMonitor scrapes the dedicated <name>-metrics Service, so the Prometheus Operator sets the service target label to <name>-metrics. Rules now scope by that name (previously service="<name>", which never matched up/mattermost_*).
• MattermostLabels map mutation — it now copies spec.resourceLabels instead of aliasing it, so adding labels never mutates the user's map.
• License warning scope — callsMetrics is excluded from the "requires Enterprise license" check, since rtcd metrics aren't gated by the Mattermost license.
• Stale zz_generated.openapi.go — regenerated so make generate is clean.

Also fixed to get CI green:

• gofmt alignment.
• Removed the CEL x-kubernetes-validations on callsMetrics — CI's e2e kind is K8s v1.22 (< 1.25), which rejects it. rtcdServiceSelector is still enforced gracefully at runtime (reconcile logs and skips when empty).
• Switched the deploy/install targets to server-side apply — the Mattermost CRD exceeds the 256KB client-side last-applied-configuration annotation limit.

Deferred as POC follow-ups (with rationale):

• Reconcile-level test coverage — a build-tagged envtest harness exists on the dashboards branch (feat(monitoring): ship Grafana dashboards as pod-scoped ConfigMaps #467); this branch keeps generator-level unit tests.
• MattermostServerDown not firing when the Service has zero endpoints — accepted for a conservative starter rule.
• Interval pattern validation and a deployment env-var behavior test — noted for a hardening pass.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 22 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

controllers/mattermost/mattermost/monitoring.go:31

• Removing spec.monitoring after resources were enabled leaves the metrics Service, ServiceMonitor, rtcd ServiceMonitor, and PrometheusRule behind indefinitely because this return bypasses every delete path. An omitted optional block should reconcile as all capabilities disabled, as the existing Ingress reconciliation does; only the never-configured case incurs harmless not-found checks.

	// Nothing to do — and nothing to clean up — for installations that never
	// declare a monitoring block. This avoids per-reconcile API calls for the
	// common case. Cleanup-on-disable still works via the `enabled: false` path,
	// which keeps the block present.
	if mattermost.Spec.Monitoring == nil {
		return nil

github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go:1

• This is a second copy of the generated v1beta1 OpenAPI package under a module-root github.com/... directory; the canonical generated file already exists at apis/mattermost/v1beta1/zz_generated.openapi.go. It creates an unintended extra Go package in go test ./... and is not part of the CI generation output. Remove this duplicate file/directory.
  apis/mattermost/v1beta1/mattermost_types.go:247
• The advertised rtcdServiceSelector requirement is not enforced: the generated CRD has neither a CEL rule nor another conditional validation, so {enabled: true} is accepted. Reconciliation then silently skips creation; worse, clearing a previously valid selector leaves the old rtcd ServiceMonitor untouched. Add the conditional API validation and the claimed apiserver rejection test.

	// RtcdServiceSelector are the labels identifying the rtcd Service to scrape.
	// Required when Enabled is true.
	// +optional
	RtcdServiceSelector map[string]string `json:"rtcdServiceSelector,omitempty"`

[Copilot]

Pull request overview

Copilot reviewed 18 out of 22 changed files in this pull request and generated 2 comments.

Files not reviewed (3)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (5)

pkg/resources/create_resources.go:209

• The cleanup path deletes a same-named PrometheusRule regardless of ownership. Thus adding an unrelated monitoring subfeature while prometheusRule is absent/disabled can remove a user-managed <name>-rules object, rather than only cleaning up resources created by this operator. Require a matching Mattermost controller owner reference before deleting.

	reqLogger.Info("Deleting prometheus rule", "name", key.Name)
	if err := r.client.Delete(context.TODO(), found); err != nil && !k8sErrors.IsNotFound(err) {

controllers/mattermost/mattermost/monitoring.go:162

• This message is inaccurate when only prometheusRule (or explicitly enabled client metrics) is configured, because no ServiceMonitor scrape target exists in those valid independent-flag configurations. Use wording that covers both unavailable metric-backed rules and down scrape targets without asserting that a target was created.

	const msg = "monitoring is enabled but spec.licenseSecret is empty; the Mattermost /metrics endpoint requires an Enterprise license, so scrape targets will stay down until a license is configured"

config/crd/bases/installation.mattermost.com_mattermosts.yaml:713

• The schema does not enforce the stated conditional requirement: callsMetrics: {enabled: true} is admitted because rtcdServiceSelector is neither required nor covered by an x-kubernetes-validations rule, after which reconciliation silently skips the ServiceMonitor. This also contradicts the PR's claimed CEL rejection. Add generated conditional validation plus an API-server test, or remove the requirement/claim if supported-cluster compatibility prevents CEL.

                      rtcdServiceSelector:
                        additionalProperties:
                          type: string
                        description: |-
                          RtcdServiceSelector are the labels identifying the rtcd Service to scrape.
                          Required when Enabled is true.
                        type: object

controllers/mattermost/mattermost/monitoring.go:194

• Explicitly enabling clientMetrics also depends on the Enterprise-gated Mattermost /metrics endpoint, but this predicate ignores it, so that configuration emits no promised license warning. Include only the explicit true case here; false and unset should remain warning-free.

func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)

github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go:1

• This duplicates apis/mattermost/v1beta1/zz_generated.openapi.go under a module-prefixed directory. Go treats it as the unrelated package github.com/mattermost/mattermost-operator/github.com/mattermost/..., and no code references that package, so it adds stray generated output to every ./... traversal. Remove this file and configure generation to write only to the canonical apis/mattermost/v1beta1 path.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 22 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (5)

apis/mattermost/v1beta1/mattermost_types.go:247

• rtcdServiceSelector is documented as required when enabled is true, and the PR description promises CEL rejection, but neither the type marker nor generated CRD contains conditional validation. The API therefore accepts callsMetrics: {enabled: true} and reconciliation silently skips the requested ServiceMonitor. Add conditional admission validation and regenerate the CRD, with the described API-server test.

	// RtcdServiceSelector are the labels identifying the rtcd Service to scrape.
	// Required when Enabled is true.
	// +optional
	RtcdServiceSelector map[string]string `json:"rtcdServiceSelector,omitempty"`

controllers/mattermost/mattermost/monitoring.go:235

• clientMetrics.enabled: true also enables metrics that the API documents as Enterprise-gated, but this predicate omits it. A client-metrics-only configuration with no license therefore receives no promised warning. Include explicitly enabled client metrics here while still excluding false and nil.

func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)

pkg/mattermost/monitoring.go:189

• Endpoint.TargetPort selects a port on the backing Pod, not a numeric port on the selected Service. Consequently, a valid Service with port: 8045 mapped to another target port will not be scraped as this API promises. Require a named Service port and assign endpoint.Port, or resolve a supplied numeric Service port to its name before creating the ServiceMonitor.

	// Accept either a numeric port (targetPort) or a named Service port.
	port := cm.Port
	if port == "" {
		port = defaultRtcdMetricsPort
	}
	if n, err := strconv.Atoi(port); err == nil {
		tp := intstr.FromInt(n)
		endpoint.TargetPort = &tp

github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go:11

• This generated file is under a duplicated module path and is not the canonical API artifact (apis/mattermost/v1beta1/zz_generated.openapi.go is). It adds an unintended github.com/mattermost/mattermost-operator/github.com/... package to ./... without being imported or used. Remove this artifact and fix the generation invocation/output path rather than committing both copies.
  apis/mattermost/v1beta1/mattermost_types.go:153
• This public API description advertises Grafana integration, but this PR intentionally contains only Prometheus resources and no Grafana capability. Describe this as optional Prometheus monitoring so the CRD/OpenAPI documentation matches the API actually exposed.

This issue also appears on line 244 of the same file.

	// Monitoring defines optional Prometheus/Grafana integration resources for
	// this installation. Each capability is gated by its own flag.
	// +optional
	Monitoring *Monitoring `json:"monitoring,omitempty"`

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cb231f1d-d395-42c7-864b-870c4d6bf9d2

📥 Commits

Reviewing files that changed from the base of the PR and between 57f9e73 and a9c761e.

📒 Files selected for processing (4)

• controllers/mattermost/mattermost/mattermost.go
• docs/mattermost-operator/mattermost-operator.yaml
• pkg/mattermost/monitoring.go
• pkg/mattermost/monitoring_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• controllers/mattermost/mattermost/mattermost.go
• pkg/mattermost/monitoring.go

---

📝 Walkthrough

Walkthrough

The PR adds optional Mattermost monitoring configuration, generates and reconciles monitoring resources, updates deployment metrics settings, registers Prometheus Operator types, updates RBAC and documentation, and changes Makefile installation to server-side apply.

Changes

Mattermost monitoring

Layer / File(s)	Summary
Monitoring API contract apis/mattermost/v1beta1/*, config/crd/bases/*, docs/mattermost-operator/*, main.go, config/rbac/role.yaml, go.mod	Adds monitoring configuration types, schemas, generated metadata, RBAC permissions, Prometheus Operator scheme registration, and defensive label copying.
Monitoring resource generation pkg/mattermost/monitoring.go, pkg/mattermost/prometheusrules/*, pkg/resources/create_resources.go, pkg/mattermost/monitoring_test.go	Generates the metrics Service, ServiceMonitors, and PrometheusRules. It loads embedded alert rules and tests defaults, selectors, ports, labels, and substitutions.
Monitoring reconciliation flow controllers/mattermost/mattermost/*, controllers/mattermost/mattermost/monitoring_reconcile_test.go	Reconciles owned monitoring resources, handles missing Prometheus Operator CRDs, removes disabled resources, emits licence warnings, and tests idempotence and cleanup.
Deployment metrics integration and example pkg/mattermost/mattermost_v1beta.go, pkg/mattermost/mattermost_v1beta_test.go, docs/examples/mattermost_monitoring.yaml	Applies explicitly configured client and notification metrics settings and documents the monitoring configuration lifecycle.

Server-side resource application

Layer / File(s)	Summary
Install and deploy targets Makefile	Adds the configurable FIELD_MANAGER value and switches install and deploy to server-side apply without forced conflict resolution.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MattermostReconciler
  participant MonitoringGenerators
  participant ResourceHelper
  participant KubernetesAPI
  MattermostReconciler->>MonitoringGenerators: Generate metrics Service, ServiceMonitor, and PrometheusRule
  MonitoringGenerators-->>MattermostReconciler: Return monitoring resources
  MattermostReconciler->>ResourceHelper: Reconcile owned resources
  ResourceHelper->>KubernetesAPI: Create, update, or delete resources

Loading

Possibly related PRs

• mattermost/mattermost-operator#467: Shares the monitoring configuration, reconciliation, generators, tests, RBAC, and deployment changes, with additional Grafana dashboard ConfigMaps.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main monitoring changes for the Mattermost CRD.
Description check	✅ Passed	The description directly explains the monitoring resources, configuration, reconciliation behaviour, validation, dependencies, and testing.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/operator-monitoring-poc

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apis/mattermost/v1beta1/mattermost_utils.go (1)

299-315: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Apply the same copy-not-alias fix to MattermostPodLabels and MattermostJobServerPodLabels.

This function now copies mm.Spec.ResourceLabels before mutating, so callers cannot corrupt the user's original map. The sibling functions MattermostPodLabels (line 322: l = mm.Spec.ResourceLabels) and MattermostJobServerPodLabels (line 346: l = mm.Spec.ResourceLabels) still alias the same map directly.

Because of the alias, every call to MattermostPodLabels (used in pkg/mattermost/mattermost_v1beta.go for the Deployment's pod template labels) permanently merges PodTemplate.ExtraLabels, the default resource labels, the cluster label, and the app label into mm.Spec.ResourceLabels itself. Any other code that reads mm.Spec.ResourceLabels afterward in the same reconcile — including the now-fixed MattermostLabels — sees these injected keys as if they were part of the user's original spec.

Copy the map in both functions the same way this fix does, for example:

🐛 Proposed fix for `MattermostPodLabels` and `MattermostJobServerPodLabels`

 func (mm *Mattermost) MattermostPodLabels(name string) map[string]string {
 	l := map[string]string{}
-	// Set resourceLabels ("global") as the initial labels
-	if mm.Spec.ResourceLabels != nil {
-		l = mm.Spec.ResourceLabels
-	}
+	// Copy resourceLabels ("global") as the initial labels, not alias, so this
+	// call never mutates the user's ResourceLabels map.
+	for k, v := range mm.Spec.ResourceLabels {
+		l[k] = v
+	}
 	if mm.Spec.PodTemplate != nil {

The same change applies to MattermostJobServerPodLabels.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apis/mattermost/v1beta1/mattermost_utils.go` around lines 299 - 315, Update
MattermostPodLabels and MattermostJobServerPodLabels to initialize a new map by
copying mm.Spec.ResourceLabels before applying extra, default, cluster, and app
labels. Remove the direct map aliasing so neither function mutates the
user-provided ResourceLabels map, matching MattermostLabels behavior.

🧹 Nitpick comments (2)

pkg/mattermost/monitoring.go (1)

183-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the deprecated intstr.FromInt with intstr.FromInt32.

intstr.FromInt(val int) in k8s.io/apimachinery/pkg/util/intstr is deprecated in favour of FromInt32(val int32). n here comes from strconv.Atoi(port), so it converts cleanly to int32.

♻️ Proposed fix

 	if n, err := strconv.Atoi(port); err == nil {
-		tp := intstr.FromInt(n)
+		tp := intstr.FromInt32(int32(n))
 		endpoint.TargetPort = &tp
 	} else {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/mattermost/monitoring.go` around lines 183 - 192, In the endpoint port
handling near endpoint.TargetPort, replace the deprecated intstr.FromInt call
with intstr.FromInt32 and convert the Atoi result n to int32 before passing it.
Preserve the existing fallback to endpoint.Port for non-numeric ports.

apis/mattermost/v1beta1/mattermost_types.go (1)

167-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider renaming to avoid confusion with prometheus-operator's own types.

Monitoring.ServiceMonitor and Monitoring.PrometheusRule are config wrapper types (only Enabled/Labels/Interval). They share the exact same names as monitoringv1.ServiceMonitor and monitoringv1.PrometheusRule, the actual Prometheus Operator CRD resource types generated in pkg/mattermost/monitoring.go. This does not cause a compile error (different packages), but it makes call sites and future contributors do extra work to keep the two apart. Rename these config types, for example ServiceMonitorConfig and PrometheusRuleConfig, before this ships to reduce ongoing confusion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apis/mattermost/v1beta1/mattermost_types.go` around lines 167 - 274, Rename
the configuration wrapper types Monitoring.ServiceMonitor and
Monitoring.PrometheusRule to distinct names such as ServiceMonitorConfig and
PrometheusRuleConfig, updating their fields, references, and serialization usage
consistently. Keep the Prometheus Operator monitoringv1.ServiceMonitor and
monitoringv1.PrometheusRule resource types unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@controllers/mattermost/mattermost/monitoring.go`:
- Around line 79-82: Update the nil-result branch after
GenerateRtcdServiceMonitorV1Beta so it deletes the existing owned <name>-rtcd
ServiceMonitor before returning. Reuse the controller’s established client and
ownership-identifying symbols, propagate or handle deletion errors consistently,
and retain the current skip behavior only after successful cleanup.

In `@Makefile`:
- Line 230: The Makefile apply command should stop using --force-conflicts in
routine install/deploy targets. Keep server-side apply with a stable
--field-manager value, and add a documented one-time migration target that
explicitly includes --force-conflicts for ownership conflicts.

In `@pkg/mattermost/monitoring.go`:
- Around line 112-152: Update GenerateServiceMonitorV1Beta,
GenerateRtcdServiceMonitorV1Beta, and GeneratePrometheusRuleV1Beta to handle nil
Spec.Monitoring without panicking, while preserving existing defaults when the
monitoring block is present or absent. Add unit tests that call each exported
generator with a nil monitoring configuration and verify the expected generated
resources.

---

Outside diff comments:
In `@apis/mattermost/v1beta1/mattermost_utils.go`:
- Around line 299-315: Update MattermostPodLabels and
MattermostJobServerPodLabels to initialize a new map by copying
mm.Spec.ResourceLabels before applying extra, default, cluster, and app labels.
Remove the direct map aliasing so neither function mutates the user-provided
ResourceLabels map, matching MattermostLabels behavior.

---

Nitpick comments:
In `@apis/mattermost/v1beta1/mattermost_types.go`:
- Around line 167-274: Rename the configuration wrapper types
Monitoring.ServiceMonitor and Monitoring.PrometheusRule to distinct names such
as ServiceMonitorConfig and PrometheusRuleConfig, updating their fields,
references, and serialization usage consistently. Keep the Prometheus Operator
monitoringv1.ServiceMonitor and monitoringv1.PrometheusRule resource types
unchanged.

In `@pkg/mattermost/monitoring.go`:
- Around line 183-192: In the endpoint port handling near endpoint.TargetPort,
replace the deprecated intstr.FromInt call with intstr.FromInt32 and convert the
Atoi result n to int32 before passing it. Preserve the existing fallback to
endpoint.Port for non-numeric ports.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4fc7127e-9d0d-42a1-9aff-f5fda03b9c3e

📥 Commits

Reviewing files that changed from the base of the PR and between 8ef3cfa and 27bed84.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (21)

• Makefile
• apis/mattermost/v1beta1/mattermost_types.go
• apis/mattermost/v1beta1/mattermost_utils.go
• apis/mattermost/v1beta1/zz_generated.deepcopy.go
• apis/mattermost/v1beta1/zz_generated.openapi.go
• config/crd/bases/installation.mattermost.com_mattermosts.yaml
• config/rbac/role.yaml
• controllers/mattermost/mattermost/controller.go
• controllers/mattermost/mattermost/mattermost.go
• controllers/mattermost/mattermost/monitoring.go
• controllers/mattermost/mattermost/monitoring_reconcile_test.go
• docs/examples/mattermost_monitoring.yaml
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go
• go.mod
• main.go
• pkg/mattermost/mattermost_v1beta.go
• pkg/mattermost/mattermost_v1beta_test.go
• pkg/mattermost/monitoring.go
• pkg/mattermost/monitoring_test.go
• pkg/mattermost/prometheusrules/mattermost.yaml
• pkg/resources/create_resources.go

[Copilot]

Pull request overview

Copilot reviewed 18 out of 22 changed files in this pull request and generated 1 comment.

Files not reviewed (3)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (5)

apis/mattermost/v1beta1/mattermost_types.go:247

• enabled: true is accepted without rtcdServiceSelector, despite the field contract and PR description saying it is required. The generator then returns nil, so a valid-looking CR silently creates no Calls ServiceMonitor. Add the conditional CRD validation and regenerate the manifests/tests so this is rejected at the Mattermost API boundary.

	// Enabled determines whether the Operator should create the rtcd ServiceMonitor.
	Enabled bool `json:"enabled"`

	// RtcdServiceSelector are the labels identifying the rtcd Service to scrape.
	// Required when Enabled is true.
	// +optional
	RtcdServiceSelector map[string]string `json:"rtcdServiceSelector,omitempty"`

controllers/mattermost/mattermost/monitoring.go:243

• This helper omits explicitly enabled clientMetrics, even though that capability is documented above as depending on the Enterprise-gated /metrics endpoint and only Calls is intentionally excluded. Consequently clientMetrics.enabled: true with no license produces no promised warning. Include the explicit-true client-metrics case.

// anyMonitoringEnabled reports whether any monitoring capability that depends on
// the (Enterprise-gated) Mattermost /metrics endpoint is turned on. callsMetrics
// is intentionally excluded: it targets a separately-deployed rtcd whose metrics
// are not gated by the Mattermost license.
func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)

pkg/mattermost/monitoring.go:96

• Appending -metrics can produce an invalid Service name for an otherwise valid existing installation. For example, a 60-character Mattermost name is a valid Service name, but the generated 68-character metrics Service exceeds the DNS-label 63-character limit and reconciliation fails whenever monitoring is enabled. Use a deterministic truncate-and-hash naming helper (and use that same result in rule placeholders).

func metricsServiceName(mattermostName string) string {
	return mattermostName + "-metrics"

pkg/mattermost/prometheusrules/mattermost.yaml:101

• The PR promises an HTTP 5xx-rate alert, but this expression divides the unfiltered mattermost_http_errors_total by all requests and contains no status-code restriction. It therefore measures the aggregate HTTP error rate (including non-5xx errors if represented by this metric), which can alert on expected client errors. Use a status-labelled request metric filtered to 5.., or rename/document the alert as an all-error-rate alert.

        # More than 5% of HTTP requests erroring for 5m.
        expr: |
          sum by (namespace, service) (rate(mattermost_http_errors_total{namespace="__NAMESPACE__", service="__SERVICE__"}[5m]))
          /
          sum by (namespace, service) (rate(mattermost_http_requests_total{namespace="__NAMESPACE__", service="__SERVICE__"}[5m])) > 0.05

github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go:1

• This is an accidental duplicate generated package under a nested github.com/... directory; the same generated output already exists at apis/mattermost/v1beta1/zz_generated.openapi.go, which is the path targeted by the Makefile and CI generation check. Remove this duplicate file so it is not treated as an extra Go package by ./... tooling.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file
• github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (5)

apis/mattermost/v1beta1/mattermost_types.go:247

• rtcdServiceSelector is only documented as conditionally required: the generated CRD has neither a CEL rule nor any other validation enforcing it. Consequently the API accepts callsMetrics.enabled: true without a selector, and reconciliation silently skips/removes the requested ServiceMonitor. Add admission validation for this contract (or revise the contract if the supported Kubernetes versions cannot express it), regenerate the CRD/docs, and cover rejection with an API-server test.

	// RtcdServiceSelector are the labels identifying the rtcd Service to scrape.
	// Required when Enabled is true.
	// +optional
	RtcdServiceSelector map[string]string `json:"rtcdServiceSelector,omitempty"`

controllers/mattermost/mattermost/controller.go:74

• Omitting watches for both monitoring resource types means they are not actually reconciled after the Mattermost becomes stable: deleting or editing an owned ServiceMonitor/PrometheusRule does not enqueue its owner, unlike the other owned resources above. Preserve optional-CRD startup behavior while adding conditional/dynamic watches when mappings exist, or schedule periodic reconciliation for enabled monitoring resources.

		// NOTE: ServiceMonitor is intentionally NOT added to Owns(). A watch on a
		// type whose CRD is absent would fail the manager at startup; the Prometheus
		// Operator CRDs are an optional dependency. GC still works via owner references.

controllers/mattermost/mattermost/monitoring.go:32

• The optional-CRD skip has no retry signal. After these branches return success, a stable Mattermost has no periodic requeue, and the controller intentionally does not watch these APIs; installing Prometheus Operator CRDs later therefore leaves enabled resources absent until an unrelated Mattermost event occurs. Propagate an unavailable result to the outer reconciler and schedule a bounded retry while monitoring remains enabled.

func (r *MattermostReconciler) checkMattermostMonitoring(mattermost *mmv1beta.Mattermost, reqLogger logr.Logger) error {
	// Nothing to do — and nothing to clean up — for installations that never
	// declare a monitoring block. This avoids per-reconcile API calls for the
	// common case. Cleanup-on-disable still works via the `enabled: false` path,
	// which keeps the block present.
	if mattermost.Spec.Monitoring == nil {

github.com/mattermost/mattermost-operator/apis/mattermost/v1beta1/zz_generated.openapi.go:1

• This generated file is an accidental duplicate under a nested github.com/mattermost/... directory. The canonical artifact is apis/mattermost/v1beta1/zz_generated.openapi.go, and the Makefile generation target writes to ./apis/mattermost/v1beta1; retaining this copy creates a redundant Go package with a bogus module-relative import path. Remove the nested file/directory.
  apis/mattermost/v1beta1/mattermost_types.go:151
• The public API description advertises “Prometheus/Grafana integration resources,” but this monitoring block contains only Prometheus resources and this PR is explicitly the no-Grafana variant. Change this to “Prometheus integration resources” and regenerate the OpenAPI/CRD documentation so users are not promised Grafana integration that the API cannot configure.

	// Monitoring defines optional Prometheus/Grafana integration resources for
	// this installation. Each capability is gated by its own flag.