From 2ffa04a53a16c12e68a0fe89b99d01150244c630 Mon Sep 17 00:00:00 2001 From: ldornele Date: Tue, 1 Sep 2026 20:09:02 -0300 Subject: [PATCH 1/5] HYPERFLEET-1410 - feat: expose operator metrics, health probes, and ServiceMonitor --- README.md | 13 + cmd/main.go | 19 +- config/default/kustomization.yaml | 10 +- config/default/manager_metrics_patch.yaml | 4 - config/default/metrics_service.yaml | 6 +- config/manager/manager.yaml | 25 +- config/manifests/kustomization.yaml | 5 + .../network-policy/allow-metrics-traffic.yaml | 2 +- config/prometheus/monitor.yaml | 15 +- docs/metrics.md | 277 ++++++++++++++++++ go.mod | 5 +- .../controller/hyperfleetconfig_controller.go | 25 +- .../hyperfleetconfig_controller_test.go | 25 ++ internal/controller/observability.go | 165 +++++++++++ internal/metrics/metrics.go | 188 ++++++++++++ internal/metrics/metrics_test.go | 84 ++++++ internal/version/version.go | 78 +++++ test/e2e/e2e_test.go | 87 +----- 18 files changed, 922 insertions(+), 111 deletions(-) delete mode 100644 config/default/manager_metrics_patch.yaml create mode 100644 docs/metrics.md create mode 100644 internal/controller/observability.go create mode 100644 internal/metrics/metrics.go create mode 100644 internal/metrics/metrics_test.go create mode 100644 internal/version/version.go diff --git a/README.md b/README.md index da5a05e..7804f03 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,19 @@ kubectl apply -k config/samples/ >**NOTE**: Ensure that the samples has default values to test it out. +### Observability endpoints + +The manager exposes the standard HyperFleet observability endpoints (defaults): + +- **Liveness probe:** `http://localhost:8080/healthz` +- **Readiness probe:** `http://localhost:8080/readyz` +- **Metrics:** `http://localhost:9090/metrics` + +Metrics are served as plain HTTP under the `hyperfleet_operator_*` namespace. Ports +are configurable via `--health-probe-bind-address` and `--metrics-bind-address`. +See [docs/metrics.md](docs/metrics.md) for the full metric catalogue, labels, and +example PromQL queries. + ### To Uninstall **Delete the instances (CRs) from the cluster:** diff --git a/cmd/main.go b/cmd/main.go index e7bc2e0..4e6564c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -43,6 +43,7 @@ import ( hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/controller" + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" // +kubebuilder:scaffold:imports ) @@ -68,14 +69,18 @@ func main() { var secureMetrics bool var enableHTTP2 bool var tlsOpts []func(*tls.Config) - flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ - "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") - flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") + // Defaults follow the HyperFleet health-endpoints / metrics standards, matching + // the API, Sentinel and Adapter components: metrics on :9090 over plain HTTP at + // /metrics, health/readiness on :8080. Set 0 on metrics-bind-address to disable. + flag.StringVar(&metricsAddr, "metrics-bind-address", ":9090", "The address the metrics endpoint binds to. "+ + "Defaults to :9090 (HyperFleet standard). Set to 0 to disable the metrics service.") + flag.StringVar(&probeAddr, "health-probe-bind-address", ":8080", "The address the probe endpoint binds to.") flag.BoolVar(&enableLeaderElection, "leader-elect", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") - flag.BoolVar(&secureMetrics, "metrics-secure", true, - "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.") + flag.BoolVar(&secureMetrics, "metrics-secure", false, + "If set, the metrics endpoint is served securely via HTTPS with authn/authz. "+ + "The HyperFleet standard scrapes plain HTTP, so this defaults to false.") flag.StringVar(&webhookCertPath, "webhook-cert-path", "", "The directory that contains the webhook certificate.") flag.StringVar(&webhookCertName, "webhook-cert-name", "tls.crt", "The name of the webhook certificate file.") flag.StringVar(&webhookCertKey, "webhook-cert-key", "tls.key", "The name of the webhook key file.") @@ -93,6 +98,10 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) + // Publish the build-info and up metrics into controller-runtime's registry so + // they are exposed on the same /metrics endpoint as the reconcile metrics. + metrics.Init() + // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml index 95f3c02..adf8bdd 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -38,13 +38,9 @@ resources: # be able to communicate with the Webhook Server. #- ../network-policy -# Uncomment the patches line if you enable Metrics -patches: -# [METRICS] The following patch will enable the metrics endpoint using HTTPS and the port :8443. -# More info: https://book.kubebuilder.io/reference/metrics -- path: manager_metrics_patch.yaml - target: - kind: Deployment +# The metrics endpoint (:9090, plain HTTP, /metrics) and health probes (:8080) +# are configured directly on the manager Deployment (config/manager/manager.yaml) +# per the HyperFleet standard, so no metrics args patch is needed here. # Uncomment the patches line if you enable Metrics and CertManager # [METRICS-WITH-CERTS] To enable metrics protected with certManager, uncomment the following line. diff --git a/config/default/manager_metrics_patch.yaml b/config/default/manager_metrics_patch.yaml deleted file mode 100644 index 2aaef65..0000000 --- a/config/default/manager_metrics_patch.yaml +++ /dev/null @@ -1,4 +0,0 @@ -# This patch adds the args to allow exposing the metrics endpoint using HTTPS -- op: add - path: /spec/template/spec/containers/0/args/0 - value: --metrics-bind-address=:8443 diff --git a/config/default/metrics_service.yaml b/config/default/metrics_service.yaml index 6e301cc..a3faf56 100644 --- a/config/default/metrics_service.yaml +++ b/config/default/metrics_service.yaml @@ -9,10 +9,10 @@ metadata: namespace: system spec: ports: - - name: https - port: 8443 + - name: metrics + port: 9090 protocol: TCP - targetPort: 8443 + targetPort: metrics selector: control-plane: controller-manager app.kubernetes.io/name: hyperfleet-operator diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index a8dd370..0118f90 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -62,7 +62,11 @@ spec: - /manager args: - --leader-elect - - --health-probe-bind-address=:8081 + # HyperFleet standard: metrics on :9090 (plain HTTP, /metrics) and + # health/readiness on :8080, matching the other components. + - --health-probe-bind-address=:8080 + - --metrics-bind-address=:9090 + - --metrics-secure=false image: controller:latest name: manager env: @@ -72,24 +76,35 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - ports: [] + ports: + - name: metrics + containerPort: 9090 + protocol: TCP + - name: health + containerPort: 8080 + protocol: TCP securityContext: allowPrivilegeEscalation: false capabilities: drop: - "ALL" + # Probe timing follows the HyperFleet health-endpoints standard. livenessProbe: httpGet: path: /healthz - port: 8081 + port: 8080 initialDelaySeconds: 15 periodSeconds: 20 + timeoutSeconds: 5 + failureThreshold: 3 readinessProbe: httpGet: path: /readyz - port: 8081 + port: 8080 initialDelaySeconds: 5 periodSeconds: 10 + timeoutSeconds: 3 + failureThreshold: 3 # TODO(user): Configure the resources accordingly based on the project requirements. # More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ resources: @@ -102,4 +117,4 @@ spec: volumeMounts: [] volumes: [] serviceAccountName: controller-manager - terminationGracePeriodSeconds: 10 + terminationGracePeriodSeconds: 30 diff --git a/config/manifests/kustomization.yaml b/config/manifests/kustomization.yaml index bb7f880..7531189 100644 --- a/config/manifests/kustomization.yaml +++ b/config/manifests/kustomization.yaml @@ -5,6 +5,11 @@ resources: - ../default - ../samples - ../scorecard +# The Prometheus ServiceMonitor ships in the bundle so OLM wires up scraping of +# the operator's :9090 metrics endpoint. It lives here (bundle packaging) rather +# than in ../default so `make deploy`/kind runs do not require the Prometheus +# Operator CRDs to be present on the target cluster. +- ../prometheus # [WEBHOOK] To enable webhooks, uncomment all the sections with [WEBHOOK] prefix. # Do NOT uncomment sections with prefix [CERTMANAGER], as OLM does not support cert-manager. diff --git a/config/network-policy/allow-metrics-traffic.yaml b/config/network-policy/allow-metrics-traffic.yaml index 0820272..7d36201 100644 --- a/config/network-policy/allow-metrics-traffic.yaml +++ b/config/network-policy/allow-metrics-traffic.yaml @@ -23,5 +23,5 @@ spec: matchLabels: metrics: enabled # Only from namespaces with this label ports: - - port: 8443 + - port: 9090 protocol: TCP diff --git a/config/prometheus/monitor.yaml b/config/prometheus/monitor.yaml index 1c880a7..6a812b2 100644 --- a/config/prometheus/monitor.yaml +++ b/config/prometheus/monitor.yaml @@ -1,4 +1,6 @@ # Prometheus Monitor Service (Metrics) +# Scrapes the operator's plain-HTTP metrics endpoint on :9090, per the HyperFleet +# metrics standard (same exposition as the API, Sentinel and Adapter components). apiVersion: monitoring.coreos.com/v1 kind: ServiceMonitor metadata: @@ -11,16 +13,9 @@ metadata: spec: endpoints: - path: /metrics - port: https # Ensure this is the name of the port that exposes HTTPS metrics - scheme: https - bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token - tlsConfig: - # TODO(user): The option insecureSkipVerify: true is not recommended for production since it disables - # certificate verification, exposing the system to potential man-in-the-middle attacks. - # For production environments, it is recommended to use cert-manager for automatic TLS certificate management. - # To apply this configuration, enable cert-manager and use the patch located at config/prometheus/servicemonitor_tls_patch.yaml, - # which securely references the certificate from the 'metrics-server-cert' secret. - insecureSkipVerify: true + port: metrics # matches the metrics Service port name + scheme: http + interval: 30s selector: matchLabels: control-plane: controller-manager diff --git a/docs/metrics.md b/docs/metrics.md new file mode 100644 index 0000000..f46fae5 --- /dev/null +++ b/docs/metrics.md @@ -0,0 +1,277 @@ +# Metrics Documentation + +This document describes the Prometheus metrics exposed by the HyperFleet +operator, including their meaning, labels, and example queries for common +investigations. It follows the HyperFleet metrics standard, matching the naming +and label conventions used by the other components (API, Sentinel, Adapters). + +## Metrics Endpoint + +Metrics are exposed at: +- **Endpoint**: `/metrics` +- **Port**: 9090 (default, configurable via `--metrics-bind-address`; set `0` to disable) +- **Protocol**: plain HTTP (`--metrics-secure=false` by default) +- **Format**: OpenMetrics/Prometheus text format + +The operator's custom collectors register into controller-runtime's registry, so +they are served on the **same** `/metrics` endpoint as the built-in +`controller_runtime_*` metrics — there is no second metrics server. + +All operator-defined series share the `hyperfleet_operator_` prefix (Prometheus +`Namespace` `hyperfleet` + `Subsystem` `operator`) and carry the standard +`component` and `version` const labels. + +## Application Metrics + +### Build Info + +#### `hyperfleet_operator_build_info` + +**Type:** Gauge (always 1) + +**Description:** Build information for the HyperFleet operator component. The +value is always `1`; the identity is carried in the labels. + +**Labels:** + +| Label | Description | Example Values | +|-------|-------------|----------------| +| `component` | Component name (const) | `operator` | +| `version` | Application version (const) | `v1.2.3`, `dev` | +| `commit` | Git commit SHA (short) | `abc1234` | +| `go_version` | Go runtime version | `go1.26.0` | + +**Example output:** + +```text +hyperfleet_operator_build_info{component="operator",version="v1.2.3",commit="abc1234",go_version="go1.26.0"} 1 +``` + +> Version and commit are injected at build time via `-ldflags -X` (see +> `internal/version`). Under a plain `go build`/`make run` they fall back to the +> module version and VCS revision from the binary's build info, so the metric is +> still populated (`version="dev"` when unavailable). + +### Process Liveness + +#### `hyperfleet_operator_up` + +**Type:** Gauge + +**Description:** `1` while the operator process is running. Set once at startup. +Distinct from the Prometheus scrape-generated `up` series. + +**Labels:** `component`, `version` (const) + +**Example output:** + +```text +hyperfleet_operator_up{component="operator",version="v1.2.3"} 1 +``` + +### Reconcile Metrics + +These metrics track the `HyperFleetConfig` reconcile loop. + +#### `hyperfleet_operator_reconcile_duration_seconds` + +**Type:** Histogram + +**Description:** Wall-clock duration of a full reconcile, recorded regardless of +outcome. + +**Labels:** `component`, `version` (const) + +**Buckets:** `0.005s`, `0.01s`, `0.025s`, `0.05s`, `0.1s`, `0.25s`, `0.5s`, `1s`, `2.5s`, `5s`, `10s` + +**Derived metrics:** +- `hyperfleet_operator_reconcile_duration_seconds_sum` +- `hyperfleet_operator_reconcile_duration_seconds_count` +- `hyperfleet_operator_reconcile_duration_seconds_bucket` + +#### `hyperfleet_operator_reconcile_errors_total` + +**Type:** Counter + +**Description:** Total number of reconcile errors, labeled by the stage that +failed, so error rate can be broken down by cause. + +**Labels:** + +| Label | Description | Example Values | +|-------|-------------|----------------| +| `component` | Component name (const) | `operator` | +| `version` | Application version (const) | `v1.2.3` | +| `reason` | Reconcile stage that failed | `get`, `render`, `apply` | + +**Example output:** + +```text +hyperfleet_operator_reconcile_errors_total{component="operator",version="v1.2.3",reason="apply"} 3 +``` + +### Operand Metrics + +These metrics track the operands the operator manages (one per component in the +resolved bundle, e.g. `api`). + +#### `hyperfleet_operator_operand_ready` + +**Type:** Gauge + +**Description:** Readiness of each operand workload: `1` when the Deployment +reports `Available=True`, `0` otherwise. Published from the freshly applied state +each reconcile. + +**Labels:** + +| Label | Description | Example Values | +|-------|-------------|----------------| +| `component` | Component name (const) | `operator` | +| `version` | Application version (const) | `v1.2.3` | +| `operand` | Operand component name | `api` | + +**Example output:** + +```text +hyperfleet_operator_operand_ready{component="operator",version="v1.2.3",operand="api"} 1 +``` + +#### `hyperfleet_operator_operand_rollouts_total` + +**Type:** Counter + +**Description:** Total operand workload rollouts, labeled by operand and the +trigger that caused the rollout. Detected before applying, by comparing the live +pod template against the desired one via a template-hash annotation. + +**Labels:** + +| Label | Description | Example Values | +|-------|-------------|----------------| +| `component` | Component name (const) | `operator` | +| `version` | Application version (const) | `v1.2.3` | +| `operand` | Operand component name | `api` | +| `trigger` | What caused the rollout | `create`, `image`, `config` | + +**Trigger values:** +- `create` — the operand's workload did not exist and was created. +- `image` — a rollout caused by a container image change. +- `config` — a rollout caused by any other pod-template change (env, resources, ...). + +**Example output:** + +```text +hyperfleet_operator_operand_rollouts_total{component="operator",version="v1.2.3",operand="api",trigger="create"} 1 +hyperfleet_operator_operand_rollouts_total{component="operator",version="v1.2.3",operand="api",trigger="image"} 4 +``` + +### Applied Config + +#### `hyperfleet_operator_applied_config_info` + +**Type:** Gauge (info-style, always 1) + +**Description:** Info metric whose `hash` label is the digest of the currently +applied `HyperFleetConfig` spec. Exactly **one** series exists at a time: the +collector is reset before each set, so a spec change replaces the previous series +rather than accumulating cardinality. + +**Labels:** + +| Label | Description | Example Values | +|-------|-------------|----------------| +| `component` | Component name (const) | `operator` | +| `version` | Application version (const) | `v1.2.3` | +| `hash` | 12-char SHA-256 digest of the applied spec | `9f2a1c4b7d3e` | + +**Example output:** + +```text +hyperfleet_operator_applied_config_info{component="operator",version="v1.2.3",hash="9f2a1c4b7d3e"} 1 +``` + +## Controller-Runtime Metrics + +Because the operator's collectors share controller-runtime's registry, the +standard controller-runtime metrics are exposed on the same endpoint, including: + +| Metric | Type | Description | +|--------|------|-------------| +| `controller_runtime_reconcile_total` | Counter | Total reconciles per controller, by result (`success`, `error`, `requeue`). | +| `controller_runtime_reconcile_errors_total` | Counter | Total reconcile errors per controller. | +| `controller_runtime_reconcile_time_seconds` | Histogram | Reconcile latency per controller. | +| `controller_runtime_active_workers` | Gauge | Number of active reconcile workers per controller. | +| `workqueue_depth` | Gauge | Current depth of the reconcile work queue. | + +## Go Runtime and Process Metrics + +The Prometheus Go client library automatically exposes process and runtime +metrics on the same endpoint, e.g. `process_cpu_seconds_total`, +`process_resident_memory_bytes`, `process_start_time_seconds`, `go_goroutines`, +and the `go_memstats_*` family. Use `process_start_time_seconds` (rather than a +custom build-time label) to answer "when did this binary start". + +## Example PromQL Queries + +### Reconcile Health + +```promql +# Reconcile rate (per second) +rate(hyperfleet_operator_reconcile_duration_seconds_count[5m]) + +# Reconcile error rate by failing stage +sum by (reason) (rate(hyperfleet_operator_reconcile_errors_total[5m])) + +# P99 reconcile latency +histogram_quantile(0.99, + sum(rate(hyperfleet_operator_reconcile_duration_seconds_bucket[5m])) by (le)) + +# Average reconcile duration +rate(hyperfleet_operator_reconcile_duration_seconds_sum[10m]) / +rate(hyperfleet_operator_reconcile_duration_seconds_count[10m]) +``` + +### Operand Health + +```promql +# Operands that are not ready +hyperfleet_operator_operand_ready == 0 + +# Rollout rate by operand and trigger +sum by (operand, trigger) (rate(hyperfleet_operator_operand_rollouts_total[15m])) + +# Image-triggered rollouts in the last hour +increase(hyperfleet_operator_operand_rollouts_total{trigger="image"}[1h]) +``` + +### Fleet State + +```promql +# Currently applied config digest (the single active series) +hyperfleet_operator_applied_config_info + +# Operator build identity +hyperfleet_operator_build_info + +# Is the operator up? +hyperfleet_operator_up +``` + +## Prometheus Operator Integration + +A `ServiceMonitor` ships in the OLM bundle so Prometheus scrapes the operator's +`:9090` endpoint over plain HTTP once the operator is installed via OLM. It is +packaged under `config/prometheus` and wired into the bundle via +`config/manifests`. + +It is intentionally **not** part of `config/default`, so `make deploy` and the +kind-based e2e do not require the Prometheus Operator CRDs to be present on the +target cluster. To scrape the operator on a cluster without OLM, apply +`config/prometheus/monitor.yaml` manually (requires the `ServiceMonitor` CRD). + +## Related Documentation + +- [README](../README.md#observability-endpoints) — observability endpoints quick reference +- [HyperFleet metrics standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/metrics.md) +- [HyperFleet health-endpoints standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/health-endpoints.md) diff --git a/go.mod b/go.mod index bd443e0..08a1a10 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,8 @@ go 1.26.0 require ( github.com/onsi/ginkgo/v2 v2.22.0 github.com/onsi/gomega v1.36.1 + github.com/prometheus/client_golang v1.22.0 + github.com/prometheus/client_model v0.6.1 k8s.io/api v0.33.0 k8s.io/apimachinery v0.33.0 k8s.io/client-go v0.33.0 @@ -44,13 +46,12 @@ require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/kylelemons/godebug v1.1.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect - github.com/prometheus/client_golang v1.22.0 // indirect - github.com/prometheus/client_model v0.6.1 // indirect github.com/prometheus/common v0.62.0 // indirect github.com/prometheus/procfs v0.15.1 // indirect github.com/spf13/cobra v1.8.1 // indirect diff --git a/internal/controller/hyperfleetconfig_controller.go b/internal/controller/hyperfleetconfig_controller.go index ece5d87..dde5eb6 100644 --- a/internal/controller/hyperfleetconfig_controller.go +++ b/internal/controller/hyperfleetconfig_controller.go @@ -21,10 +21,12 @@ import ( "fmt" "net/http" "sync" + "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -38,6 +40,7 @@ import ( hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/apply" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/bundle" + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" ) // HyperFleetConfigReconciler reconciles a HyperFleetConfig object. It is the @@ -109,11 +112,20 @@ type HyperFleetConfigReconciler struct { func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logf.FromContext(ctx) + // Record reconcile latency regardless of outcome, and error rate by the stage + // that failed, so both are observable via hyperfleet_operator_reconcile_*. + start := time.Now() + defer func() { metrics.ObserveReconcile(time.Since(start)) }() + cr := &hyperfleetv1alpha1.HyperFleetConfig{} if err := r.Get(ctx, req.NamespacedName, cr); err != nil { // The CR is gone: its operands carry controller owner references, so the // built-in garbage collector removes them. No finalizer is required. - return ctrl.Result{}, client.IgnoreNotFound(err) + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + metrics.IncReconcileError("get") + return ctrl.Result{}, fmt.Errorf("get HyperFleetConfig %q: %w", req.NamespacedName, err) } // Resolve the JWKS URL. When auth is on and the CR pins neither a JWKS URL nor @@ -147,17 +159,28 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req for _, component := range components { objs, err := component.Render(ctx, cr) if err != nil { + metrics.IncReconcileError("render") return ctrl.Result{}, fmt.Errorf("render component %q: %w", component.Name(), err) } // Stamp the config-hash on the component's Deployment pod template so a // config or secret-value change rolls the pods. For components without a // ConfigMap+Deployment pair (none today besides the API) this is a no-op. stampConfigHash(objs, secretData) + // Detect (and count) an imminent operand rollout before applying, while the + // live object still reflects the previous desired state. Runs after + // stampConfigHash so the desired template it hashes is the final one. + r.recordRollouts(ctx, component.Name(), objs) if err := apply.Objects(ctx, r.Client, cr, r.Scheme, objs); err != nil { + metrics.IncReconcileError("apply") return ctrl.Result{}, fmt.Errorf("apply component %q: %w", component.Name(), err) } + // Publish operand readiness from the freshly applied state. + r.recordReadiness(ctx, component.Name(), objs) } + // Publish the digest of the config we just applied. + metrics.SetAppliedConfigHash(hashConfig(cr.Spec)) + // TODO(HYPERFLEET-1409): roll each component's Conditions up into // status.conditions and set status.observedGeneration. // TODO(HYPERFLEET-1512): enforce that referenced Secrets exist in diff --git a/internal/controller/hyperfleetconfig_controller_test.go b/internal/controller/hyperfleetconfig_controller_test.go index da294a9..2006c58 100644 --- a/internal/controller/hyperfleetconfig_controller_test.go +++ b/internal/controller/hyperfleetconfig_controller_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/prometheus/client_golang/prometheus/testutil" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -32,6 +33,7 @@ import ( hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" apicomponent "github.com/openshift-hyperfleet/hyperfleet-operator/internal/component/api" + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" ) // Reconciler behavior specs (HYPERFLEET-1407). These run against envtest, which @@ -243,6 +245,29 @@ var _ = Describe("HyperFleetConfig Controller", func() { Expect(dep.Spec.Template.Annotations[configHashAnnotation]).NotTo(Equal(firstHash)) }) + It("records observability metrics for the reconcile and its operands", func() { + By("reconciling to create the operands") + doReconcile() + + By("publishing the applied-config digest as a single info series") + // SetAppliedConfigHash resets before setting, so exactly one series exists + // regardless of how many times the suite has reconciled. + Expect(testutil.CollectAndCount(metrics.AppliedConfig)).To(Equal(1)) + + By("publishing operand readiness for the api component") + // envtest is apiserver + etcd only: no deployment controller runs, so the + // operand never reports Available. The gauge must still be published, at 0. + Expect(testutil.ToFloat64( + metrics.OperandReady.WithLabelValues(apicomponent.ComponentName))).To(Equal(0.0)) + + By("counting a create-triggered rollout for the api operand") + // The Deployment did not exist before this reconcile (BeforeEach starts + // clean), so the reconcile records at least one create rollout. + Expect(testutil.ToFloat64( + metrics.OperandRollouts.WithLabelValues(apicomponent.ComponentName, metrics.TriggerCreate))). + To(BeNumerically(">=", 1)) + }) + It("returns without error when the CR is absent (deletion path)", func() { By("deleting the singleton before it is reconciled") deleteSingletonAndWait(ctx) diff --git a/internal/controller/observability.go b/internal/controller/observability.go new file mode 100644 index 0000000..c58f960 --- /dev/null +++ b/internal/controller/observability.go @@ -0,0 +1,165 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" + + appsv1 "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// templateHashAnnotation records the digest of the pod template the operator last +// applied to an operand Deployment. It is stored in the Deployment's metadata (not +// its pod template), so writing it never itself triggers a rollout; it exists only +// so the next reconcile can tell whether the desired pod template changed and thus +// whether an apply will roll the workload. HYPERFLEET-1408 layers spec-derived +// content into the template; this annotation already accounts for it. +const templateHashAnnotation = "hyperfleet.redhat.com/template-hash" + +// hashConfig returns a short, stable digest of the applied spec. json.Marshal of a +// Go struct is field-ordered and deterministic, so equal specs hash equally across +// reconciles and process restarts. +func hashConfig(spec hyperfleetv1alpha1.HyperFleetConfigSpec) string { + b, err := json.Marshal(spec) + if err != nil { + // A spec that cannot be marshaled is not something the caller can act on; + // fall back to a sentinel so the metric still publishes a single series. + return "unmarshalable" + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:])[:12] +} + +// hashPodTemplate returns a short, stable digest of a Deployment's pod template. +func hashPodTemplate(dep *appsv1.Deployment) string { + b, err := json.Marshal(dep.Spec.Template) + if err != nil { + return "unmarshalable" + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:])[:12] +} + +// stampTemplateHash records the desired pod-template digest on the Deployment so +// the apply persists it for the next reconcile's rollout comparison. +func stampTemplateHash(dep *appsv1.Deployment, hash string) { + if dep.Annotations == nil { + dep.Annotations = map[string]string{} + } + dep.Annotations[templateHashAnnotation] = hash +} + +// recordRollouts inspects each Deployment a component wants to apply and, when the +// apply will roll the workload, increments the rollout counter with the trigger. +// It must run BEFORE apply, while the live object still holds the previous state. +// It also stamps the desired template hash onto the object so the apply persists +// it. Metrics are best-effort: a read error here is logged and skipped, never +// surfaced as a reconcile failure. +func (r *HyperFleetConfigReconciler) recordRollouts(ctx context.Context, component string, objs []client.Object) { + log := logf.FromContext(ctx) + for _, o := range objs { + dep, ok := o.(*appsv1.Deployment) + if !ok { + continue + } + desired := hashPodTemplate(dep) + stampTemplateHash(dep, desired) + + live := &appsv1.Deployment{} + err := r.Get(ctx, client.ObjectKeyFromObject(dep), live) + switch { + case apierrors.IsNotFound(err): + metrics.IncOperandRollout(component, metrics.TriggerCreate) + case err != nil: + log.V(1).Info("skipping rollout metric: could not read live operand", + "component", component, "deployment", dep.Name, "error", err.Error()) + default: + prev := live.Annotations[templateHashAnnotation] + // prev == "" means we have never stamped this Deployment (e.g. first + // reconcile after upgrading to this operator version): adopt the hash + // silently rather than count a rollout we cannot attribute. + if prev != "" && prev != desired { + metrics.IncOperandRollout(component, rolloutTrigger(live, dep)) + } + } + } +} + +// rolloutTrigger classifies why a rollout is happening: an image change if any +// container image differs, otherwise a config/template change. +func rolloutTrigger(live, desired *appsv1.Deployment) string { + if !sameContainerImages(live, desired) { + return metrics.TriggerImage + } + return metrics.TriggerConfig +} + +// sameContainerImages reports whether both Deployments have the same container +// images in the same order. +func sameContainerImages(a, b *appsv1.Deployment) bool { + ac, bc := a.Spec.Template.Spec.Containers, b.Spec.Template.Spec.Containers + if len(ac) != len(bc) { + return false + } + for i := range ac { + if ac[i].Image != bc[i].Image { + return false + } + } + return true +} + +// recordReadiness reads the live status of each of a component's Deployments after +// apply and publishes the operand readiness gauge. A Deployment is ready when it +// reports the Available condition True. Best-effort: read errors are logged and the +// gauge is left untouched rather than failing the reconcile. +func (r *HyperFleetConfigReconciler) recordReadiness(ctx context.Context, component string, objs []client.Object) { + log := logf.FromContext(ctx) + for _, o := range objs { + dep, ok := o.(*appsv1.Deployment) + if !ok { + continue + } + live := &appsv1.Deployment{} + if err := r.Get(ctx, client.ObjectKeyFromObject(dep), live); err != nil { + log.V(1).Info("skipping readiness metric: could not read live operand", + "component", component, "deployment", dep.Name, "error", err.Error()) + continue + } + metrics.SetOperandReady(component, deploymentAvailable(live)) + } +} + +// deploymentAvailable reports whether a Deployment carries the Available=True +// condition. +func deploymentAvailable(dep *appsv1.Deployment) bool { + for _, c := range dep.Status.Conditions { + if c.Type == appsv1.DeploymentAvailable { + return c.Status == "True" + } + } + return false +} diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 0000000..440e57b --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,188 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package metrics defines the operator's custom Prometheus collectors and the +// helpers the reconciler uses to record them. It follows the HyperFleet metrics +// standard: +// +// - names are hyperfleet_operator__ (Namespace "hyperfleet", +// Subsystem "operator"); +// - every series carries the standard component/version const labels; +// - durations are histograms in seconds, counters end in _total. +// +// Collectors register into controller-runtime's global registry, so they are +// served on the same /metrics endpoint (and through the same ServiceMonitor) as +// the built-in controller_runtime_* metrics — no second metrics server. +package metrics + +import ( + "time" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" + ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" + + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/version" +) + +const ( + // namespace and subsystem compose the hyperfleet_operator_ prefix mandated by + // the metrics standard for this component. + namespace = "hyperfleet" + subsystem = "operator" + + // Component is the value of the standard "component" label for the operator. + Component = "operator" +) + +// Rollout trigger label values for OperandRollouts. Kept as constants so the set +// stays closed and low-cardinality. +const ( + // TriggerCreate is recorded the first time an operand's workload is created. + TriggerCreate = "create" + // TriggerImage is recorded when a rollout is caused by an image change. + TriggerImage = "image" + // TriggerConfig is recorded when a rollout is caused by any other pod-template + // change (config, env, resources, ...). + TriggerConfig = "config" +) + +// commonLabels are the standard labels every HyperFleet metric must carry. They +// are constant for the lifetime of the process, so they are attached as const +// labels rather than passed at each observation. +func commonLabels() prometheus.Labels { + return prometheus.Labels{ + "component": Component, + "version": version.Version(), + } +} + +// reconcileBuckets covers the expected reconcile latency spread: sub-millisecond +// server-side-apply no-ops up to multi-second reconciles that touch the API +// server repeatedly. Matches the standard's general-purpose bucket guidance. +var reconcileBuckets = []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10} + +// Collectors. Exported so tests can assert on them via prometheus/testutil and so +// call sites read explicitly; prefer the helper functions below for recording. +var ( + // ReconcileDuration measures wall-clock time of a full reconcile, regardless + // of outcome. + ReconcileDuration = promauto.With(ctrlmetrics.Registry).NewHistogram(prometheus.HistogramOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "reconcile_duration_seconds", + Help: "Duration of HyperFleetConfig reconciles in seconds.", + Buckets: reconcileBuckets, + ConstLabels: commonLabels(), + }) + + // ReconcileErrors counts reconcile failures by the stage that failed + // (get/render/apply/...), so error rate can be broken down by reason. + ReconcileErrors = promauto.With(ctrlmetrics.Registry).NewCounterVec(prometheus.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "reconcile_errors_total", + Help: "Total number of reconcile errors, labeled by the failing stage.", + ConstLabels: commonLabels(), + }, []string{"reason"}) + + // OperandReady reports, per operand component, whether its workload is + // currently Available (1) or not (0). + OperandReady = promauto.With(ctrlmetrics.Registry).NewGaugeVec(prometheus.GaugeOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "operand_ready", + Help: "Readiness of each operand workload (1 = Available, 0 = not).", + ConstLabels: commonLabels(), + }, []string{"operand"}) + + // OperandRollouts counts operand workload rollouts by component and trigger. + OperandRollouts = promauto.With(ctrlmetrics.Registry).NewCounterVec(prometheus.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "operand_rollouts_total", + Help: "Total operand workload rollouts, labeled by operand and trigger.", + ConstLabels: commonLabels(), + }, []string{"operand", "trigger"}) + + // AppliedConfig is an info-style gauge whose "hash" label carries the digest + // of the currently applied HyperFleetConfig spec. Only ever one series exists + // at a time (see SetAppliedConfigHash). + AppliedConfig = promauto.With(ctrlmetrics.Registry).NewGaugeVec(prometheus.GaugeOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "applied_config_info", + Help: "Info metric whose hash label is the digest of the applied HyperFleetConfig spec.", + ConstLabels: commonLabels(), + }, []string{"hash"}) + + // buildInfo is set once at startup; its labels carry the build identity. + buildInfo = promauto.With(ctrlmetrics.Registry).NewGaugeVec(prometheus.GaugeOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "build_info", + Help: "Build information; always 1, identity carried in labels.", + ConstLabels: commonLabels(), + }, []string{"commit", "go_version"}) + + // up is 1 while the operator process is running. + up = promauto.With(ctrlmetrics.Registry).NewGauge(prometheus.GaugeOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "up", + Help: "1 while the operator is running.", + ConstLabels: commonLabels(), + }) +) + +// Init records the process-lifetime metrics (build info and up). Call it once at +// startup, after flags are parsed. +func Init() { + buildInfo.WithLabelValues(version.Commit(), version.GoVersion()).Set(1) + up.Set(1) +} + +// ObserveReconcile records the duration of one reconcile. +func ObserveReconcile(d time.Duration) { + ReconcileDuration.Observe(d.Seconds()) +} + +// IncReconcileError increments the error counter for the given failing stage. +func IncReconcileError(reason string) { + ReconcileErrors.WithLabelValues(reason).Inc() +} + +// SetOperandReady sets the readiness gauge for an operand component. +func SetOperandReady(operand string, ready bool) { + v := 0.0 + if ready { + v = 1.0 + } + OperandReady.WithLabelValues(operand).Set(v) +} + +// IncOperandRollout records a rollout of an operand's workload. +func IncOperandRollout(operand, trigger string) { + OperandRollouts.WithLabelValues(operand, trigger).Inc() +} + +// SetAppliedConfigHash publishes the applied-config digest as the only series of +// the AppliedConfig gauge. It resets first so the previous hash's series does not +// linger and inflate cardinality over the operator's lifetime. +func SetAppliedConfigHash(hash string) { + AppliedConfig.Reset() + AppliedConfig.WithLabelValues(hash).Set(1) +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go new file mode 100644 index 0000000..99aba17 --- /dev/null +++ b/internal/metrics/metrics_test.go @@ -0,0 +1,84 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package metrics + +import ( + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus/testutil" + dto "github.com/prometheus/client_model/go" +) + +func TestObserveReconcileRecordsASample(t *testing.T) { + ObserveReconcile(150 * time.Millisecond) + + var m dto.Metric + if err := ReconcileDuration.Write(&m); err != nil { + t.Fatalf("writing histogram: %v", err) + } + if got := m.GetHistogram().GetSampleCount(); got < 1 { + t.Errorf("reconcile duration sample count = %d, want >= 1", got) + } +} + +func TestIncReconcileErrorCountsByReason(t *testing.T) { + // Unique reason so the assertion is independent of other tests in this package. + const reason = "test-reason" + IncReconcileError(reason) + + if got := testutil.ToFloat64(ReconcileErrors.WithLabelValues(reason)); got != 1 { + t.Errorf("reconcile errors{reason=%q} = %v, want 1", reason, got) + } +} + +func TestSetOperandReadyTogglesGauge(t *testing.T) { + const operand = "test-ready" + + SetOperandReady(operand, true) + if got := testutil.ToFloat64(OperandReady.WithLabelValues(operand)); got != 1 { + t.Errorf("operand_ready{operand=%q} = %v, want 1", operand, got) + } + + SetOperandReady(operand, false) + if got := testutil.ToFloat64(OperandReady.WithLabelValues(operand)); got != 0 { + t.Errorf("operand_ready{operand=%q} = %v, want 0", operand, got) + } +} + +func TestIncOperandRolloutCountsByTrigger(t *testing.T) { + const operand = "test-rollout" + IncOperandRollout(operand, TriggerImage) + + if got := testutil.ToFloat64(OperandRollouts.WithLabelValues(operand, TriggerImage)); got != 1 { + t.Errorf("operand_rollouts{operand=%q,trigger=%q} = %v, want 1", operand, TriggerImage, got) + } +} + +func TestSetAppliedConfigHashKeepsASingleSeries(t *testing.T) { + SetAppliedConfigHash("hash-one") + SetAppliedConfigHash("hash-two") + + // The reset in SetAppliedConfigHash must drop the previous hash's series so the + // gauge never accumulates stale series over the operator's lifetime. + if got := testutil.CollectAndCount(AppliedConfig); got != 1 { + t.Errorf("applied_config_info series count = %d, want 1", got) + } + if got := testutil.ToFloat64(AppliedConfig.WithLabelValues("hash-two")); got != 1 { + t.Errorf("applied_config_info{hash=hash-two} = %v, want 1", got) + } +} diff --git a/internal/version/version.go b/internal/version/version.go new file mode 100644 index 0000000..b9122b0 --- /dev/null +++ b/internal/version/version.go @@ -0,0 +1,78 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package version exposes the operator's build identity (version, commit, Go +// runtime) so it can be surfaced in logs and in the hyperfleet_operator_build_info +// metric, per the HyperFleet metrics standard. +// +// The values are intended to be injected at build time via -ldflags -X, e.g. +// +// -X github.com/openshift-hyperfleet/hyperfleet-operator/internal/version.version=v1.2.3 +// -X github.com/openshift-hyperfleet/hyperfleet-operator/internal/version.commit=abc1234 +// +// When they are not injected (plain `go build`, `make run`, tests) the accessors +// fall back to the module version and VCS revision recorded in the binary's +// build info, so the metric is still populated with something meaningful. +package version + +import ( + "runtime" + "runtime/debug" +) + +// These are overridden at build time via -ldflags -X. Keep them unexported and +// read through the accessors so the build-info fallback below always applies. +var ( + version = "" + commit = "" +) + +// Version returns the operator version, e.g. "v1.2.3" or "dev-abc1234". It +// prefers the ldflags-injected value, then the module version from build info, +// and finally "dev". +func Version() string { + if version != "" { + return version + } + if info, ok := debug.ReadBuildInfo(); ok && info.Main.Version != "" && info.Main.Version != "(devel)" { + return info.Main.Version + } + return "dev" +} + +// Commit returns the short VCS revision the binary was built from, or "unknown" +// when it cannot be determined. +func Commit() string { + if commit != "" { + return commit + } + if info, ok := debug.ReadBuildInfo(); ok { + for _, s := range info.Settings { + if s.Key == "vcs.revision" { + if len(s.Value) > 7 { + return s.Value[:7] + } + return s.Value + } + } + } + return "unknown" +} + +// GoVersion returns the Go runtime version the binary was compiled with. +func GoVersion() string { + return runtime.Version() +} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 05a2844..0494c51 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -17,11 +17,8 @@ limitations under the License. package e2e import ( - "encoding/json" "fmt" - "os" "os/exec" - "path/filepath" "time" . "github.com/onsi/ginkgo/v2" @@ -39,8 +36,9 @@ const serviceAccountName = "hyperfleet-operator-controller-manager" // metricsServiceName is the name of the metrics service of the project const metricsServiceName = "hyperfleet-operator-controller-manager-metrics-service" -// metricsRoleBindingName is the name of the RBAC that will be created to allow get the metrics data -const metricsRoleBindingName = "hyperfleet-operator-metrics-binding" +// metricsPort is the plain-HTTP port the operator serves /metrics on, per the +// HyperFleet metrics standard. +const metricsPort = "9090" var _ = Describe("Manager", Ordered, func() { var controllerPodName string @@ -171,30 +169,17 @@ var _ = Describe("Manager", Ordered, func() { }) It("should ensure the metrics endpoint is serving metrics", func() { - By("creating a ClusterRoleBinding for the service account to allow access to metrics") - cmd := exec.Command("kubectl", "create", "clusterrolebinding", metricsRoleBindingName, - "--clusterrole=hyperfleet-operator-metrics-reader", - fmt.Sprintf("--serviceaccount=%s:%s", namespace, serviceAccountName), - ) - _, err := utils.Run(cmd) - Expect(err).NotTo(HaveOccurred(), "Failed to create ClusterRoleBinding") - By("validating that the metrics service is available") - cmd = exec.Command("kubectl", "get", "service", metricsServiceName, "-n", namespace) - _, err = utils.Run(cmd) + cmd := exec.Command("kubectl", "get", "service", metricsServiceName, "-n", namespace) + _, err := utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Metrics service should exist") - By("getting the service account token") - token, err := serviceAccountToken() - Expect(err).NotTo(HaveOccurred()) - Expect(token).NotTo(BeEmpty()) - By("waiting for the metrics endpoint to be ready") verifyMetricsEndpointReady := func(g Gomega) { cmd := exec.Command("kubectl", "get", "endpoints", metricsServiceName, "-n", namespace) output, err := utils.Run(cmd) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(output).To(ContainSubstring("8443"), "Metrics endpoint is not ready") + g.Expect(output).To(ContainSubstring(metricsPort), "Metrics endpoint is not ready") } Eventually(verifyMetricsEndpointReady).Should(Succeed()) @@ -208,7 +193,7 @@ var _ = Describe("Manager", Ordered, func() { } Eventually(verifyMetricsServerStarted).Should(Succeed()) - By("creating the curl-metrics pod to access the metrics endpoint") + By("creating the curl-metrics pod to access the plain-HTTP metrics endpoint") cmd = exec.Command("kubectl", "run", "curl-metrics", "--restart=Never", "--namespace", namespace, "--image=curlimages/curl:latest", @@ -219,7 +204,7 @@ var _ = Describe("Manager", Ordered, func() { "name": "curl", "image": "curlimages/curl:latest", "command": ["/bin/sh", "-c"], - "args": ["curl -v -k -H 'Authorization: Bearer %s' https://%s.%s.svc.cluster.local:8443/metrics"], + "args": ["curl -v http://%s.%s.svc.cluster.local:%s/metrics"], "securityContext": { "allowPrivilegeEscalation": false, "capabilities": { @@ -234,7 +219,7 @@ var _ = Describe("Manager", Ordered, func() { }], "serviceAccount": "%s" } - }`, token, metricsServiceName, namespace, serviceAccountName)) + }`, metricsServiceName, namespace, metricsPort, serviceAccountName)) _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed to create curl-metrics pod") @@ -251,9 +236,14 @@ var _ = Describe("Manager", Ordered, func() { By("getting the metrics by checking curl-metrics logs") metricsOutput := getMetricsOutput() + By("verifying the built-in controller-runtime metrics are exposed") Expect(metricsOutput).To(ContainSubstring( "controller_runtime_reconcile_total", )) + By("verifying the operator's custom HyperFleet metrics are exposed") + Expect(metricsOutput).To(ContainSubstring( + "hyperfleet_operator_up", + )) }) // +kubebuilder:scaffold:e2e-webhooks-checks @@ -269,47 +259,6 @@ var _ = Describe("Manager", Ordered, func() { }) }) -// serviceAccountToken returns a token for the specified service account in the given namespace. -// It uses the Kubernetes TokenRequest API to generate a token by directly sending a request -// and parsing the resulting token from the API response. -func serviceAccountToken() (string, error) { - const tokenRequestRawString = `{ - "apiVersion": "authentication.k8s.io/v1", - "kind": "TokenRequest" - }` - - // Temporary file to store the token request - secretName := fmt.Sprintf("%s-token-request", serviceAccountName) - tokenRequestFile := filepath.Join("/tmp", secretName) - err := os.WriteFile(tokenRequestFile, []byte(tokenRequestRawString), os.FileMode(0o644)) - if err != nil { - return "", err - } - - var out string - verifyTokenCreation := func(g Gomega) { - // Execute kubectl command to create the token - cmd := exec.Command("kubectl", "create", "--raw", fmt.Sprintf( - "/api/v1/namespaces/%s/serviceaccounts/%s/token", - namespace, - serviceAccountName, - ), "-f", tokenRequestFile) - - output, err := cmd.CombinedOutput() - g.Expect(err).NotTo(HaveOccurred()) - - // Parse the JSON output to extract the token - var token tokenRequest - err = json.Unmarshal(output, &token) - g.Expect(err).NotTo(HaveOccurred()) - - out = token.Status.Token - } - Eventually(verifyTokenCreation).Should(Succeed()) - - return out, err -} - // getMetricsOutput retrieves and returns the logs from the curl pod used to access the metrics endpoint. func getMetricsOutput() string { By("getting the curl-metrics logs") @@ -319,11 +268,3 @@ func getMetricsOutput() string { Expect(metricsOutput).To(ContainSubstring("< HTTP/1.1 200 OK")) return metricsOutput } - -// tokenRequest is a simplified representation of the Kubernetes TokenRequest API response, -// containing only the token field that we need to extract. -type tokenRequest struct { - Status struct { - Token string `json:"token"` - } `json:"status"` -} From 4794660a02bba0484db029e7e65136b05b1362b1 Mon Sep 17 00:00:00 2001 From: ldornele Date: Tue, 1 Sep 2026 22:00:58 -0300 Subject: [PATCH 2/5] HYPERFLEET-1410 - fix: create ServiceMonitor at runtime instead of bundling it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OLM applies a bundle's arbitrary manifests but does not install the CRDs they depend on, so shipping the ServiceMonitor (monitoring.coreos.com/v1) in the OLM bundle failed the InstallPlan — blocking the entire operator install — on any cluster without the Prometheus Operator CRD. HyperFleet targets generic Kubernetes, not only OpenShift where that CRD is guaranteed. The operator now creates its own ServiceMonitor at runtime via a leader-only manager Runnable that first probes the discovery API for monitoring.coreos.com/v1 and skips (logging) when the API is absent, so metrics stay served on :9090 and installs never break. A cluster that installs the Prometheus Operator later picks the ServiceMonitor up on the operator's next restart. - remove ../prometheus from the bundle kustomization (config/manifests) - add internal/servicemonitor with CRD detection + server-side apply + unit tests - add a namespaced servicemonitors RBAC grant (covered by the existing binding) - rewrite the Prometheus Operator Integration section in docs/metrics.md Co-Authored-By: Claude Opus 4.8 --- cmd/main.go | 14 ++ config/manifests/kustomization.yaml | 13 +- config/rbac/role.yaml | 11 ++ docs/metrics.md | 27 ++- internal/servicemonitor/servicemonitor.go | 186 ++++++++++++++++++ .../servicemonitor/servicemonitor_test.go | 98 +++++++++ 6 files changed, 335 insertions(+), 14 deletions(-) create mode 100644 internal/servicemonitor/servicemonitor.go create mode 100644 internal/servicemonitor/servicemonitor_test.go diff --git a/cmd/main.go b/cmd/main.go index 4e6564c..d1665e1 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -44,6 +44,7 @@ import ( hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/controller" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/servicemonitor" // +kubebuilder:scaffold:imports ) @@ -265,6 +266,19 @@ func main() { } // +kubebuilder:scaffold:builder + // Create the operator's own ServiceMonitor at runtime, but only when the + // Prometheus Operator API is present. Shipping it in the OLM bundle would fail + // the InstallPlan on clusters without the monitoring.coreos.com CRD and block + // the operator install; this bootstrapper degrades gracefully instead. See the + // servicemonitor package doc for the full rationale. + if err := mgr.Add(&servicemonitor.Bootstrapper{ + Config: mgr.GetConfig(), + Namespace: operatorNamespace, + }); err != nil { + setupLog.Error(err, "unable to add ServiceMonitor bootstrapper") + os.Exit(1) + } + if metricsCertWatcher != nil { setupLog.Info("Adding metrics certificate watcher to manager") if err := mgr.Add(metricsCertWatcher); err != nil { diff --git a/config/manifests/kustomization.yaml b/config/manifests/kustomization.yaml index 7531189..29fe55f 100644 --- a/config/manifests/kustomization.yaml +++ b/config/manifests/kustomization.yaml @@ -5,11 +5,14 @@ resources: - ../default - ../samples - ../scorecard -# The Prometheus ServiceMonitor ships in the bundle so OLM wires up scraping of -# the operator's :9090 metrics endpoint. It lives here (bundle packaging) rather -# than in ../default so `make deploy`/kind runs do not require the Prometheus -# Operator CRDs to be present on the target cluster. -- ../prometheus +# The Prometheus ServiceMonitor is deliberately NOT included in the bundle. OLM +# applies a bundle's arbitrary manifests but does not install the CRDs they need, +# so bundling the ServiceMonitor would fail the InstallPlan — and block the whole +# operator install — on clusters without the Prometheus Operator CRD. HyperFleet +# targets generic Kubernetes, so the operator creates the ServiceMonitor itself at +# runtime only when the monitoring.coreos.com/v1 API is present (see the +# internal/servicemonitor package). config/prometheus remains an optional GitOps +# overlay for users who prefer to apply it statically. # [WEBHOOK] To enable webhooks, uncomment all the sections with [WEBHOOK] prefix. # Do NOT uncomment sections with prefix [CERTMANAGER], as OLM does not support cert-manager. diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 302b2ce..92c1a5b 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -81,3 +81,14 @@ rules: - get - list - watch +- apiGroups: + - monitoring.coreos.com + resources: + - servicemonitors + verbs: + - create + - get + - list + - patch + - update + - watch diff --git a/docs/metrics.md b/docs/metrics.md index f46fae5..e783ffe 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -260,15 +260,24 @@ hyperfleet_operator_up ## Prometheus Operator Integration -A `ServiceMonitor` ships in the OLM bundle so Prometheus scrapes the operator's -`:9090` endpoint over plain HTTP once the operator is installed via OLM. It is -packaged under `config/prometheus` and wired into the bundle via -`config/manifests`. - -It is intentionally **not** part of `config/default`, so `make deploy` and the -kind-based e2e do not require the Prometheus Operator CRDs to be present on the -target cluster. To scrape the operator on a cluster without OLM, apply -`config/prometheus/monitor.yaml` manually (requires the `ServiceMonitor` CRD). +The operator creates its own `ServiceMonitor` (`controller-manager-metrics-monitor`) +at runtime so Prometheus scrapes the `:9090` endpoint over plain HTTP. The +bootstrap is conditional: it runs only when the cluster serves the Prometheus +Operator API (`monitoring.coreos.com/v1`), and is skipped — with a log line — when +that CRD is absent. Metrics remain available on `:9090` either way. See the +`internal/servicemonitor` package. + +The `ServiceMonitor` is intentionally **not** shipped in the OLM bundle. OLM +applies a bundle's arbitrary manifests but does not install the CRDs they depend +on, so bundling it would fail the InstallPlan — and block the entire operator +install — on any cluster without the Prometheus Operator CRD. Because HyperFleet +targets generic Kubernetes (not only OpenShift, where that CRD is guaranteed), the +runtime bootstrap above degrades gracefully instead. + +A cluster that installs the Prometheus Operator *after* the operator started picks +the `ServiceMonitor` up on the operator's next restart. For GitOps, the equivalent +static manifest remains available at `config/prometheus/monitor.yaml` and can be +applied directly (requires the `ServiceMonitor` CRD). ## Related Documentation diff --git a/internal/servicemonitor/servicemonitor.go b/internal/servicemonitor/servicemonitor.go new file mode 100644 index 0000000..2631dd6 --- /dev/null +++ b/internal/servicemonitor/servicemonitor.go @@ -0,0 +1,186 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package servicemonitor bootstraps the operator's own Prometheus ServiceMonitor +// at runtime, but only when the Prometheus Operator API is present on the cluster. +// +// The ServiceMonitor (monitoring.coreos.com/v1) is deliberately NOT shipped in the +// OLM bundle: OLM applies a bundle's arbitrary manifests via an InstallPlan but does +// not install the CRD they depend on, so bundling the ServiceMonitor would fail the +// InstallPlan — and block the entire operator install — on any cluster without the +// Prometheus Operator CRD. Since HyperFleet targets generic Kubernetes (not only +// OpenShift, where that CRD is guaranteed), the operator instead creates the +// ServiceMonitor itself when, and only when, the API is available, and degrades +// gracefully (metrics still served on :9090) when it is not. +package servicemonitor + +import ( + "context" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +// The operator creates and maintains its own ServiceMonitor in its own namespace +// when the Prometheus Operator API is present. Namespaced (not a ClusterRole rule): +// the ServiceMonitor only ever lives in the operator's namespace, matching the +// least-privilege scoping used for the secrets grant. The namespace literal must +// match config/default/kustomization.yaml's namespace transformer. +// +kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors,verbs=get;list;watch;create;update;patch,namespace=hyperfleet-system + +const ( + // serviceMonitorName is the operator's own ServiceMonitor. It matches the static + // manifest historically shipped in config/prometheus/monitor.yaml so GitOps users + // who apply that overlay and clusters relying on this runtime path converge on the + // same object. + serviceMonitorName = "controller-manager-metrics-monitor" + + // appName is the operator's app.kubernetes.io/name label value and its + // server-side-apply field-manager identity. + appName = "hyperfleet-operator" + // controlPlane is the control-plane label value shared by the operator's + // Deployment, its metrics Service and this ServiceMonitor. + controlPlane = "controller-manager" + + smGroup = "monitoring.coreos.com" + smVersion = "v1" + smKind = "ServiceMonitor" +) + +// Bootstrapper is a manager Runnable that ensures the operator's ServiceMonitor +// exists. It runs once, after the manager wins leader election, so only the active +// instance writes the object. +type Bootstrapper struct { + // Config is the rest.Config used to probe API availability and apply the object. + Config *rest.Config + // Namespace is the operator's own namespace, where the ServiceMonitor is created. + Namespace string +} + +// NeedLeaderElection makes the bootstrap run only on the leader, so a multi-replica +// rollout does not race to create the same object. +func (b *Bootstrapper) NeedLeaderElection() bool { return true } + +// Start ensures the ServiceMonitor exists when the Prometheus Operator API is +// available. It is best-effort: every failure is logged and swallowed so metrics +// bootstrap never crashes the manager or blocks reconciliation. A cluster that +// installs the Prometheus Operator after the operator started picks the +// ServiceMonitor up on the operator's next restart. +func (b *Bootstrapper) Start(ctx context.Context) error { + log := logf.FromContext(ctx).WithName("servicemonitor") + + available, err := serviceMonitorAvailable(b.Config) + if err != nil { + // Discovery failed (e.g. a transient API server error). Skip rather than + // crash: metrics are still exposed on :9090 and a restart retries. + log.Error(err, "could not determine ServiceMonitor API availability; skipping ServiceMonitor bootstrap") + return nil + } + if !available { + log.Info("Prometheus Operator API (monitoring.coreos.com/v1 ServiceMonitor) not present; " + + "skipping ServiceMonitor creation. Install the Prometheus Operator and restart the operator to enable " + + "scraping, or apply config/prometheus manually.") + return nil + } + + cl, err := client.New(b.Config, client.Options{}) + if err != nil { + log.Error(err, "could not build client for ServiceMonitor bootstrap") + return nil + } + + sm := buildServiceMonitor(b.Namespace) + if err := cl.Patch(ctx, sm, client.Apply, client.FieldOwner(appName), client.ForceOwnership); err != nil { + log.Error(err, "failed to apply operator ServiceMonitor", + "name", serviceMonitorName, "namespace", b.Namespace) + return nil + } + log.Info("ensured operator ServiceMonitor", "name", serviceMonitorName, "namespace", b.Namespace) + return nil +} + +// serviceMonitorAvailable reports whether the cluster serves the ServiceMonitor +// API. A cluster without the Prometheus Operator returns NotFound for the group +// version, which is a clean "not available" rather than an error. +func serviceMonitorAvailable(cfg *rest.Config) (bool, error) { + dc, err := discovery.NewDiscoveryClientForConfig(cfg) + if err != nil { + return false, err + } + list, err := dc.ServerResourcesForGroupVersion(smGroup + "/" + smVersion) + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return hasServiceMonitorKind(list), nil +} + +// hasServiceMonitorKind reports whether the API resource list advertises the +// ServiceMonitor kind. Split out from serviceMonitorAvailable so the matching +// logic is unit-testable without a live discovery client. +func hasServiceMonitorKind(list *metav1.APIResourceList) bool { + if list == nil { + return false + } + for _, r := range list.APIResources { + if r.Kind == smKind { + return true + } + } + return false +} + +// buildServiceMonitor renders the operator's ServiceMonitor as an unstructured +// object. Unstructured avoids taking a compile-time dependency on the Prometheus +// Operator API module for a single fixed object. The selector must match the +// labels the operator's metrics Service carries (config/default/metrics_service.yaml) +// or Prometheus scrapes nothing. +func buildServiceMonitor(namespace string) *unstructured.Unstructured { + sm := &unstructured.Unstructured{} + sm.SetGroupVersionKind(schema.GroupVersionKind{Group: smGroup, Version: smVersion, Kind: smKind}) + sm.SetName(serviceMonitorName) + sm.SetNamespace(namespace) + sm.SetLabels(map[string]string{ + "control-plane": controlPlane, + "app.kubernetes.io/name": appName, + "app.kubernetes.io/managed-by": appName, + }) + sm.Object["spec"] = map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ + "control-plane": controlPlane, + "app.kubernetes.io/name": appName, + }, + }, + "endpoints": []any{ + map[string]any{ + "path": "/metrics", + "port": "metrics", // matches the metrics Service port name + "scheme": "http", + "interval": "30s", + }, + }, + } + return sm +} diff --git a/internal/servicemonitor/servicemonitor_test.go b/internal/servicemonitor/servicemonitor_test.go new file mode 100644 index 0000000..cbaa194 --- /dev/null +++ b/internal/servicemonitor/servicemonitor_test.go @@ -0,0 +1,98 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package servicemonitor + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestBuildServiceMonitor(t *testing.T) { + sm := buildServiceMonitor("hyperfleet-system") + + if got := sm.GetName(); got != serviceMonitorName { + t.Errorf("name = %q, want %q", got, serviceMonitorName) + } + if got := sm.GetNamespace(); got != "hyperfleet-system" { + t.Errorf("namespace = %q, want %q", got, "hyperfleet-system") + } + if gvk := sm.GroupVersionKind(); gvk.Group != smGroup || gvk.Version != smVersion || gvk.Kind != smKind { + t.Errorf("gvk = %v, want %s/%s %s", gvk, smGroup, smVersion, smKind) + } + + // The ServiceMonitor selector must match the labels the operator's metrics + // Service carries, or Prometheus discovers no target to scrape. + sel, found, err := unstructured.NestedStringMap(sm.Object, "spec", "selector", "matchLabels") + if err != nil || !found { + t.Fatalf("spec.selector.matchLabels missing: found=%v err=%v", found, err) + } + if sel["control-plane"] != controlPlane || sel["app.kubernetes.io/name"] != appName { + t.Errorf("selector.matchLabels = %v", sel) + } + + // Exactly one endpoint, scraping the plain-HTTP :9090 metrics port by name. + endpoints, found, err := unstructured.NestedSlice(sm.Object, "spec", "endpoints") + if err != nil || !found { + t.Fatalf("spec.endpoints missing: found=%v err=%v", found, err) + } + if len(endpoints) != 1 { + t.Fatalf("len(spec.endpoints) = %d, want 1", len(endpoints)) + } + ep, ok := endpoints[0].(map[string]any) + if !ok { + t.Fatalf("endpoints[0] type = %T, want map[string]any", endpoints[0]) + } + if ep["port"] != "metrics" || ep["path"] != "/metrics" || ep["scheme"] != "http" { + t.Errorf("endpoint = %v, want port=metrics path=/metrics scheme=http", ep) + } +} + +func TestHasServiceMonitorKind(t *testing.T) { + tests := []struct { + name string + list *metav1.APIResourceList + want bool + }{ + { + name: "nil list (group absent)", + list: nil, + want: false, + }, + { + name: "group present without ServiceMonitor", + list: &metav1.APIResourceList{APIResources: []metav1.APIResource{{Kind: "PrometheusRule"}}}, + want: false, + }, + { + name: "ServiceMonitor advertised", + list: &metav1.APIResourceList{APIResources: []metav1.APIResource{ + {Kind: "PrometheusRule"}, + {Kind: smKind}, + }}, + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := hasServiceMonitorKind(tc.list); got != tc.want { + t.Errorf("hasServiceMonitorKind() = %v, want %v", got, tc.want) + } + }) + } +} From 08fd6addd481f0f177e24fe1bc852848473d004f Mon Sep 17 00:00:00 2001 From: ldornele Date: Tue, 1 Sep 2026 22:22:15 -0300 Subject: [PATCH 3/5] HYPERFLEET-1410 - fix: wrap discovery errors in serviceMonitorAvailable Wrap the errors from discovery client creation and ServerResourcesForGroupVersion with operation context (including the monitoring.coreos.com/v1 group version) before returning them, so the ServiceMonitor bootstrapper logs a descriptive message instead of a bare client-go error. Co-Authored-By: Claude Opus 4.8 --- internal/servicemonitor/servicemonitor.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/servicemonitor/servicemonitor.go b/internal/servicemonitor/servicemonitor.go index 2631dd6..249160c 100644 --- a/internal/servicemonitor/servicemonitor.go +++ b/internal/servicemonitor/servicemonitor.go @@ -29,6 +29,7 @@ package servicemonitor import ( "context" + "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -124,14 +125,15 @@ func (b *Bootstrapper) Start(ctx context.Context) error { func serviceMonitorAvailable(cfg *rest.Config) (bool, error) { dc, err := discovery.NewDiscoveryClientForConfig(cfg) if err != nil { - return false, err + return false, fmt.Errorf("create discovery client: %w", err) } - list, err := dc.ServerResourcesForGroupVersion(smGroup + "/" + smVersion) + groupVersion := smGroup + "/" + smVersion + list, err := dc.ServerResourcesForGroupVersion(groupVersion) if err != nil { if apierrors.IsNotFound(err) { return false, nil } - return false, err + return false, fmt.Errorf("list server resources for %s: %w", groupVersion, err) } return hasServiceMonitorKind(list), nil } From 80fbd2b40fd6b1f74f96e120a87983e87fb3822a Mon Sep 17 00:00:00 2001 From: ldornele Date: Tue, 1 Sep 2026 22:49:59 -0300 Subject: [PATCH 4/5] HYPERFLEET-1410 - fix: count reconcile errors per stage and block CGNAT discovery targets Address CodeRabbit review findings on PR #9: - Reconcile now increments hyperfleet_operator_reconcile_errors_total on the JWKS-discovery, referenced-secret and bundle-resolution failure paths (labels discovery/secrets/bundle), so the error metric no longer under-reports; docs updated with the new reason values. - Harden isDisallowedDiscoveryTarget against CGNAT (100.64.0.0/10) and other non-public IANA special-purpose ranges that net.IP.IsPrivate does not classify, closing an SSRF gap on the partner-controlled OIDC issuer; tests extended. - Add docstrings to the operator's metrics and servicemonitor unit tests. Co-Authored-By: Claude Opus 4.8 --- docs/metrics.md | 2 +- .../controller/hyperfleetconfig_controller.go | 3 + .../controller/hyperfleetconfig_rollout.go | 50 ++++++++++++++-- .../hyperfleetconfig_rollout_test.go | 18 ++++-- internal/controller/observability.go | 3 +- internal/controller/observability_test.go | 58 +++++++++++++++++++ internal/metrics/metrics_test.go | 10 ++++ .../servicemonitor/servicemonitor_test.go | 4 ++ internal/version/version_test.go | 58 +++++++++++++++++++ 9 files changed, 195 insertions(+), 11 deletions(-) create mode 100644 internal/controller/observability_test.go create mode 100644 internal/version/version_test.go diff --git a/docs/metrics.md b/docs/metrics.md index e783ffe..ca8071b 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -102,7 +102,7 @@ failed, so error rate can be broken down by cause. |-------|-------------|----------------| | `component` | Component name (const) | `operator` | | `version` | Application version (const) | `v1.2.3` | -| `reason` | Reconcile stage that failed | `get`, `render`, `apply` | +| `reason` | Reconcile stage that failed | `get`, `discovery`, `secrets`, `bundle`, `render`, `apply` | **Example output:** diff --git a/internal/controller/hyperfleetconfig_controller.go b/internal/controller/hyperfleetconfig_controller.go index dde5eb6..b134f26 100644 --- a/internal/controller/hyperfleetconfig_controller.go +++ b/internal/controller/hyperfleetconfig_controller.go @@ -133,6 +133,7 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req // here rather than in the pure renderer. Empty otherwise. jwksURL, err := r.resolveJWKSURL(ctx, cr) if err != nil { + metrics.IncReconcileError("discovery") return ctrl.Result{}, fmt.Errorf("resolve JWKS URL: %w", err) } @@ -144,6 +145,7 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req // pods roll once the Secret appears. secretData, err := r.referencedSecretData(ctx, cr) if err != nil { + metrics.IncReconcileError("secrets") return ctrl.Result{}, fmt.Errorf("read referenced secrets: %w", err) } @@ -153,6 +155,7 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req ResolvedJWKSURL: jwksURL, }) if err != nil { + metrics.IncReconcileError("bundle") return ctrl.Result{}, fmt.Errorf("resolve components: %w", err) } diff --git a/internal/controller/hyperfleetconfig_rollout.go b/internal/controller/hyperfleetconfig_rollout.go index b753ea8..b5abb03 100644 --- a/internal/controller/hyperfleetconfig_rollout.go +++ b/internal/controller/hyperfleetconfig_rollout.go @@ -114,13 +114,53 @@ func blockDiscoveryDial(_, address string, _ syscall.RawConn) error { return nil } +// reservedDiscoveryCIDRs are non-public destination ranges the net.IP.IsXxx +// helpers do not classify. net.IP.IsPrivate covers RFC1918 and IPv6 ULA +// (fc00::/7) but not, notably, the shared CGNAT space (100.64.0.0/10, RFC 6598) +// a partner-controlled issuer could use to pivot into a carrier- or +// cloud-internal host. The remainder are IANA special-purpose ranges that are +// never a legitimate public IdP, so blocking them costs nothing and closes the +// gap left by relying on IsPrivate alone. +var reservedDiscoveryCIDRs = []*net.IPNet{ + mustCIDR("0.0.0.0/8"), // "this host on this network" (RFC 1122) + mustCIDR("100.64.0.0/10"), // shared address space / CGNAT (RFC 6598) + mustCIDR("192.0.0.0/24"), // IETF protocol assignments (RFC 6890) + mustCIDR("192.0.2.0/24"), // documentation TEST-NET-1 (RFC 5737) + mustCIDR("198.18.0.0/15"), // benchmarking (RFC 2544) + mustCIDR("198.51.100.0/24"), // documentation TEST-NET-2 (RFC 5737) + mustCIDR("203.0.113.0/24"), // documentation TEST-NET-3 (RFC 5737) + mustCIDR("240.0.0.0/4"), // reserved / former class E (RFC 1112) + mustCIDR("100::/64"), // discard-only (RFC 6666) + mustCIDR("2001:db8::/32"), // documentation (RFC 3849) +} + +// mustCIDR parses a CIDR literal that is a compile-time constant; a parse error +// can only be a programming error, so it panics rather than returning one. +func mustCIDR(s string) *net.IPNet { + _, n, err := net.ParseCIDR(s) + if err != nil { + panic(fmt.Sprintf("reserved discovery CIDR %q: %v", s, err)) + } + return n +} + // isDisallowedDiscoveryTarget reports whether ip is a loopback, private, -// link-local, unspecified, or multicast address — the set of destinations an -// outbound OIDC discovery request must never reach, since spec.api.auth.issuer -// is partner-controlled. +// link-local, unspecified, or multicast address, or falls in one of the +// reserved ranges above (CGNAT and other non-public IANA special-purpose +// blocks) — the set of destinations an outbound OIDC discovery request must +// never reach, since spec.api.auth.issuer is partner-controlled. IsPrivate +// alone is insufficient: it does not cover CGNAT (100.64.0.0/10). func isDisallowedDiscoveryTarget(ip net.IP) bool { - return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || - ip.IsLinkLocalMulticast() || ip.IsUnspecified() || ip.IsMulticast() + if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || + ip.IsLinkLocalMulticast() || ip.IsUnspecified() || ip.IsMulticast() { + return true + } + for _, n := range reservedDiscoveryCIDRs { + if n.Contains(ip) { + return true + } + } + return false } // resolveJWKSURL returns the JWKS URL the renderer should write into config.yaml, diff --git a/internal/controller/hyperfleetconfig_rollout_test.go b/internal/controller/hyperfleetconfig_rollout_test.go index 4f4b4b8..cbc2d4e 100644 --- a/internal/controller/hyperfleetconfig_rollout_test.go +++ b/internal/controller/hyperfleetconfig_rollout_test.go @@ -267,15 +267,25 @@ func TestIsDisallowedDiscoveryTarget(t *testing.T) { "127.0.0.1", "::1", // loopback "10.0.0.5", "172.16.0.5", "192.168.1.5", // RFC1918 private "169.254.169.254", "169.254.1.1", // link-local, incl. cloud metadata - "0.0.0.0", // unspecified - "224.0.0.1", // multicast - "fc00::1", // IPv6 unique local + "0.0.0.0", // unspecified + "224.0.0.1", // multicast + "fc00::1", // IPv6 unique local + "100.64.0.1", // CGNAT / shared address space (RFC 6598) + "100.127.255.1", // CGNAT upper bound + "0.1.2.3", // "this host on this network" (RFC 1122) + "192.0.0.1", // IETF protocol assignments + "198.18.0.1", // benchmarking + "240.0.0.1", // reserved / former class E + "2001:db8::1", // IPv6 documentation } for _, s := range disallowed { g.Expect(isDisallowedDiscoveryTarget(net.ParseIP(s))).To(BeTrue(), s) } - allowed := []string{"8.8.8.8", "1.1.1.1", "2001:4860:4860::8888"} + allowed := []string{ + "8.8.8.8", "1.1.1.1", "2001:4860:4860::8888", + "100.63.255.255", "100.128.0.0", // just outside the CGNAT block, still public + } for _, s := range allowed { g.Expect(isDisallowedDiscoveryTarget(net.ParseIP(s))).To(BeFalse(), s) } diff --git a/internal/controller/observability.go b/internal/controller/observability.go index c58f960..60ad25a 100644 --- a/internal/controller/observability.go +++ b/internal/controller/observability.go @@ -23,6 +23,7 @@ import ( "encoding/json" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -158,7 +159,7 @@ func (r *HyperFleetConfigReconciler) recordReadiness(ctx context.Context, compon func deploymentAvailable(dep *appsv1.Deployment) bool { for _, c := range dep.Status.Conditions { if c.Type == appsv1.DeploymentAvailable { - return c.Status == "True" + return c.Status == corev1.ConditionTrue } } return false diff --git a/internal/controller/observability_test.go b/internal/controller/observability_test.go new file mode 100644 index 0000000..6b8a791 --- /dev/null +++ b/internal/controller/observability_test.go @@ -0,0 +1,58 @@ +package controller + +import ( + "testing" + + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + + "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" +) + +// depWithImages builds a Deployment whose pod template carries the given +// container images, in order, so the rollout-trigger classification can be +// exercised without an API server. +func depWithImages(images ...string) *appsv1.Deployment { + containers := make([]corev1.Container, len(images)) + for i, img := range images { + containers[i] = corev1.Container{Image: img} + } + dep := &appsv1.Deployment{} + dep.Spec.Template.Spec.Containers = containers + return dep +} + +// TestRolloutTrigger verifies rolloutTrigger classifies an image change as +// TriggerImage and any other pod-template change as TriggerConfig. +func TestRolloutTrigger(t *testing.T) { + g := NewWithT(t) + + g.Expect(rolloutTrigger(depWithImages("api:v1"), depWithImages("api:v2"))). + To(Equal(metrics.TriggerImage), "an image change is an image-triggered rollout") + + g.Expect(rolloutTrigger(depWithImages("api:v1"), depWithImages("api:v1"))). + To(Equal(metrics.TriggerConfig), "same images means a config-triggered rollout") + + // A change in the number of containers is an image-set change, not a config one. + g.Expect(rolloutTrigger(depWithImages("api:v1"), depWithImages("api:v1", "sidecar:v1"))). + To(Equal(metrics.TriggerImage), "a container-count change is treated as an image change") +} + +// TestSameContainerImages verifies sameContainerImages compares images by value +// and order, and treats a differing container count as not-equal. +func TestSameContainerImages(t *testing.T) { + g := NewWithT(t) + + g.Expect(sameContainerImages(depWithImages("a:1", "b:1"), depWithImages("a:1", "b:1"))). + To(BeTrue(), "identical image lists are equal") + + g.Expect(sameContainerImages(depWithImages("a:1"), depWithImages("a:2"))). + To(BeFalse(), "a differing image is not equal") + + g.Expect(sameContainerImages(depWithImages("a:1", "b:1"), depWithImages("b:1", "a:1"))). + To(BeFalse(), "order matters") + + g.Expect(sameContainerImages(depWithImages("a:1"), depWithImages("a:1", "b:1"))). + To(BeFalse(), "a differing container count is not equal") +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index 99aba17..3a858a4 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -24,6 +24,8 @@ import ( dto "github.com/prometheus/client_model/go" ) +// TestObserveReconcileRecordsASample verifies ObserveReconcile records a +// duration observation into the reconcile histogram. func TestObserveReconcileRecordsASample(t *testing.T) { ObserveReconcile(150 * time.Millisecond) @@ -36,6 +38,8 @@ func TestObserveReconcileRecordsASample(t *testing.T) { } } +// TestIncReconcileErrorCountsByReason verifies IncReconcileError increments the +// error counter for the series identified by the given reason label. func TestIncReconcileErrorCountsByReason(t *testing.T) { // Unique reason so the assertion is independent of other tests in this package. const reason = "test-reason" @@ -46,6 +50,8 @@ func TestIncReconcileErrorCountsByReason(t *testing.T) { } } +// TestSetOperandReadyTogglesGauge verifies SetOperandReady drives the operand +// readiness gauge to 1 when ready and back to 0 when not. func TestSetOperandReadyTogglesGauge(t *testing.T) { const operand = "test-ready" @@ -60,6 +66,8 @@ func TestSetOperandReadyTogglesGauge(t *testing.T) { } } +// TestIncOperandRolloutCountsByTrigger verifies IncOperandRollout increments the +// rollout counter for the series keyed by operand and trigger. func TestIncOperandRolloutCountsByTrigger(t *testing.T) { const operand = "test-rollout" IncOperandRollout(operand, TriggerImage) @@ -69,6 +77,8 @@ func TestIncOperandRolloutCountsByTrigger(t *testing.T) { } } +// TestSetAppliedConfigHashKeepsASingleSeries verifies SetAppliedConfigHash resets +// the info gauge so only the latest hash's series exists at any time. func TestSetAppliedConfigHashKeepsASingleSeries(t *testing.T) { SetAppliedConfigHash("hash-one") SetAppliedConfigHash("hash-two") diff --git a/internal/servicemonitor/servicemonitor_test.go b/internal/servicemonitor/servicemonitor_test.go index cbaa194..f815c2b 100644 --- a/internal/servicemonitor/servicemonitor_test.go +++ b/internal/servicemonitor/servicemonitor_test.go @@ -23,6 +23,8 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +// TestBuildServiceMonitor verifies buildServiceMonitor renders the expected +// name, namespace, GVK, metrics Service selector and single scrape endpoint. func TestBuildServiceMonitor(t *testing.T) { sm := buildServiceMonitor("hyperfleet-system") @@ -63,6 +65,8 @@ func TestBuildServiceMonitor(t *testing.T) { } } +// TestHasServiceMonitorKind verifies hasServiceMonitorKind detects the +// ServiceMonitor kind in a discovery list and handles a nil/absent group. func TestHasServiceMonitorKind(t *testing.T) { tests := []struct { name string diff --git a/internal/version/version_test.go b/internal/version/version_test.go new file mode 100644 index 0000000..f51e38b --- /dev/null +++ b/internal/version/version_test.go @@ -0,0 +1,58 @@ +package version + +import "testing" + +// TestVersionPrefersInjectedValue verifies Version returns the ldflags-injected +// value when one is set. +func TestVersionPrefersInjectedValue(t *testing.T) { + orig := version + t.Cleanup(func() { version = orig }) + + version = "v1.2.3" + if got := Version(); got != "v1.2.3" { + t.Errorf("Version() = %q, want v1.2.3", got) + } +} + +// TestVersionFallsBackToNonEmpty verifies Version never returns empty: with no +// injected value it falls back to the module version or the "dev" sentinel. +func TestVersionFallsBackToNonEmpty(t *testing.T) { + orig := version + t.Cleanup(func() { version = orig }) + + version = "" + if got := Version(); got == "" { + t.Error("Version() returned empty; want module version or \"dev\"") + } +} + +// TestCommitPrefersInjectedValue verifies Commit returns the ldflags-injected +// value when one is set. +func TestCommitPrefersInjectedValue(t *testing.T) { + orig := commit + t.Cleanup(func() { commit = orig }) + + commit = "abc1234" + if got := Commit(); got != "abc1234" { + t.Errorf("Commit() = %q, want abc1234", got) + } +} + +// TestCommitFallsBackToNonEmpty verifies Commit never returns empty: with no +// injected value it falls back to the VCS revision or the "unknown" sentinel. +func TestCommitFallsBackToNonEmpty(t *testing.T) { + orig := commit + t.Cleanup(func() { commit = orig }) + + commit = "" + if got := Commit(); got == "" { + t.Error("Commit() returned empty; want vcs.revision or \"unknown\"") + } +} + +// TestGoVersionIsPopulated verifies GoVersion reports the runtime version. +func TestGoVersionIsPopulated(t *testing.T) { + if GoVersion() == "" { + t.Error("GoVersion() returned empty") + } +} From 2bbfed3d0574793f2c23c887ed8a7a5885a0c7af Mon Sep 17 00:00:00 2001 From: Super User Date: Fri, 4 Sep 2026 00:24:54 -0300 Subject: [PATCH 5/5] HYPERFLEET-1410 - fix: address metrics/rollout PR review comments Regenerates the OLM bundle (stale since the 9090/8080 metrics port and runtime-ServiceMonitor changes) so the CSV deployment, metrics Service, and namespaced servicemonitors RBAC match config/. Defers the operand rollout counter increment until after apply succeeds, so a failed apply retried on the next reconcile is no longer double-counted. Folds each component's config-rollout hash (rendered config + referenced-Secret resourceVersions) into the applied-config metric so a Secret rotation or resolved-value drift (e.g. OIDC JWKS discovery) is reflected there too, not just a CR spec change. Co-Authored-By: Claude Sonnet 5 --- ...er-manager-metrics-service_v1_service.yaml | 6 +- ...rfleet-operator.clusterserviceversion.yaml | 35 ++++++++-- docs/metrics.md | 13 ++-- .../controller/hyperfleetconfig_controller.go | 26 ++++--- .../hyperfleetconfig_controller_test.go | 13 ++++ .../controller/hyperfleetconfig_rollout.go | 10 ++- internal/controller/observability.go | 68 ++++++++++++++----- internal/controller/observability_test.go | 22 ++++++ 8 files changed, 152 insertions(+), 41 deletions(-) diff --git a/bundle/manifests/hyperfleet-operator-controller-manager-metrics-service_v1_service.yaml b/bundle/manifests/hyperfleet-operator-controller-manager-metrics-service_v1_service.yaml index b042de1..74029b9 100644 --- a/bundle/manifests/hyperfleet-operator-controller-manager-metrics-service_v1_service.yaml +++ b/bundle/manifests/hyperfleet-operator-controller-manager-metrics-service_v1_service.yaml @@ -9,10 +9,10 @@ metadata: name: hyperfleet-operator-controller-manager-metrics-service spec: ports: - - name: https - port: 8443 + - name: metrics + port: 9090 protocol: TCP - targetPort: 8443 + targetPort: metrics selector: app.kubernetes.io/name: hyperfleet-operator control-plane: controller-manager diff --git a/bundle/manifests/hyperfleet-operator.clusterserviceversion.yaml b/bundle/manifests/hyperfleet-operator.clusterserviceversion.yaml index 2654e73..a27b5c4 100644 --- a/bundle/manifests/hyperfleet-operator.clusterserviceversion.yaml +++ b/bundle/manifests/hyperfleet-operator.clusterserviceversion.yaml @@ -33,7 +33,7 @@ metadata: } ] capabilities: Basic Install - createdAt: "2026-09-01T18:11:57Z" + createdAt: "2026-09-03T22:06:02Z" operators.operatorframework.io/builder: operator-sdk-v1.42.3 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 name: hyperfleet-operator.v0.0.1 @@ -158,9 +158,10 @@ spec: spec: containers: - args: - - --metrics-bind-address=:8443 - --leader-elect - - --health-probe-bind-address=:8081 + - --health-probe-bind-address=:8080 + - --metrics-bind-address=:9090 + - --metrics-secure=false command: - /manager env: @@ -172,18 +173,29 @@ spec: value: quay.io/redhat-services-prod/hyperfleet-tenant/hyperfleet/hyperfleet-api:latest image: quay.io/redhat-services-prod/hyperfleet-tenant/hyperfleet/hyperfleet-operator:latest livenessProbe: + failureThreshold: 3 httpGet: path: /healthz - port: 8081 + port: 8080 initialDelaySeconds: 15 periodSeconds: 20 + timeoutSeconds: 5 name: manager + ports: + - containerPort: 9090 + name: metrics + protocol: TCP + - containerPort: 8080 + name: health + protocol: TCP readinessProbe: + failureThreshold: 3 httpGet: path: /readyz - port: 8081 + port: 8080 initialDelaySeconds: 5 periodSeconds: 10 + timeoutSeconds: 3 resources: limits: cpu: 500m @@ -201,7 +213,7 @@ spec: seccompProfile: type: RuntimeDefault serviceAccountName: hyperfleet-operator-controller-manager - terminationGracePeriodSeconds: 10 + terminationGracePeriodSeconds: 30 permissions: - rules: - apiGroups: @@ -243,6 +255,17 @@ spec: - get - list - watch + - apiGroups: + - monitoring.coreos.com + resources: + - servicemonitors + verbs: + - create + - get + - list + - patch + - update + - watch serviceAccountName: hyperfleet-operator-controller-manager strategy: deployment installModes: diff --git a/docs/metrics.md b/docs/metrics.md index ca8071b..7379d52 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -173,9 +173,14 @@ hyperfleet_operator_operand_rollouts_total{component="operator",version="v1.2.3" **Type:** Gauge (info-style, always 1) **Description:** Info metric whose `hash` label is the digest of the currently -applied `HyperFleetConfig` spec. Exactly **one** series exists at a time: the -collector is reset before each set, so a spec change replaces the previous series -rather than accumulating cardinality. +applied configuration: the `HyperFleetConfig` spec plus every component's +config-rollout hash (rendered config content and referenced-Secret +resourceVersions). Covering only the spec would miss a Secret rotation or a +resolved value that never lands in the CR (e.g. OIDC JWKS discovery), either of +which changes what is actually applied to an operand without changing the spec +itself. Exactly **one** series exists at a time: the collector is reset before +each set, so a change replaces the previous series rather than accumulating +cardinality. **Labels:** @@ -183,7 +188,7 @@ rather than accumulating cardinality. |-------|-------------|----------------| | `component` | Component name (const) | `operator` | | `version` | Application version (const) | `v1.2.3` | -| `hash` | 12-char SHA-256 digest of the applied spec | `9f2a1c4b7d3e` | +| `hash` | 12-char SHA-256 digest of the applied spec and component config hashes | `9f2a1c4b7d3e` | **Example output:** diff --git a/internal/controller/hyperfleetconfig_controller.go b/internal/controller/hyperfleetconfig_controller.go index b134f26..94e36ad 100644 --- a/internal/controller/hyperfleetconfig_controller.go +++ b/internal/controller/hyperfleetconfig_controller.go @@ -159,6 +159,7 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, fmt.Errorf("resolve components: %w", err) } + componentConfigHashes := make([]string, 0, len(components)) for _, component := range components { objs, err := component.Render(ctx, cr) if err != nil { @@ -167,22 +168,31 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req } // Stamp the config-hash on the component's Deployment pod template so a // config or secret-value change rolls the pods. For components without a - // ConfigMap+Deployment pair (none today besides the API) this is a no-op. - stampConfigHash(objs, secretData) - // Detect (and count) an imminent operand rollout before applying, while the - // live object still reflects the previous desired state. Runs after - // stampConfigHash so the desired template it hashes is the final one. - r.recordRollouts(ctx, component.Name(), objs) + // ConfigMap+Deployment pair (none today besides the API) this is a no-op + // beyond the returned hash. The hash is also folded into the applied-config + // metric below, so it reflects secret/resolved-value changes too. + componentConfigHashes = append(componentConfigHashes, stampConfigHash(objs, secretData)) + // Detect an imminent operand rollout before applying, while the live object + // still reflects the previous desired state. Runs after stampConfigHash so + // the desired template it hashes is the final one. The rollout counter + // itself is only incremented once the apply below succeeds, so a failed + // apply — retried on the next reconcile — is not counted as a rollout that + // never happened. + rollouts := r.detectRollouts(ctx, component.Name(), objs) if err := apply.Objects(ctx, r.Client, cr, r.Scheme, objs); err != nil { metrics.IncReconcileError("apply") return ctrl.Result{}, fmt.Errorf("apply component %q: %w", component.Name(), err) } + commitRollouts(rollouts) // Publish operand readiness from the freshly applied state. r.recordReadiness(ctx, component.Name(), objs) } - // Publish the digest of the config we just applied. - metrics.SetAppliedConfigHash(hashConfig(cr.Spec)) + // Publish the digest of the config we just applied: the spec plus every + // component's config-rollout hash, so a Secret rotation or resolved-value + // drift (e.g. OIDC discovery) shows up here even when the spec itself did not + // change. + metrics.SetAppliedConfigHash(hashConfig(cr.Spec, componentConfigHashes)) // TODO(HYPERFLEET-1409): roll each component's Conditions up into // status.conditions and set status.observedGeneration. diff --git a/internal/controller/hyperfleetconfig_controller_test.go b/internal/controller/hyperfleetconfig_controller_test.go index 2006c58..6c4ca82 100644 --- a/internal/controller/hyperfleetconfig_controller_test.go +++ b/internal/controller/hyperfleetconfig_controller_test.go @@ -291,8 +291,21 @@ var _ = Describe("HyperFleetConfig Controller", func() { OperatorNamespace: "does-not-exist", APIImage: apiImage, } + + // The Deployment among the rendered objects would detect as a create-triggered + // rollout, but the apply above fails before anything is persisted. The rollout + // counter must not advance for a rollout that never happened — otherwise a + // reconcile retried on every failed apply would keep double-counting it. + before := testutil.ToFloat64( + metrics.OperandRollouts.WithLabelValues(apicomponent.ComponentName, metrics.TriggerCreate)) + _, err := badReconciler.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("apply component")) + + By("not counting a rollout for the failed apply") + Expect(testutil.ToFloat64( + metrics.OperandRollouts.WithLabelValues(apicomponent.ComponentName, metrics.TriggerCreate))). + To(Equal(before)) }) }) diff --git a/internal/controller/hyperfleetconfig_rollout.go b/internal/controller/hyperfleetconfig_rollout.go index b5abb03..c710291 100644 --- a/internal/controller/hyperfleetconfig_rollout.go +++ b/internal/controller/hyperfleetconfig_rollout.go @@ -408,9 +408,12 @@ func computeConfigHash(configYAML string, entries []hashEntry) string { // ConfigMap plus the referenced-secret entries and writes it onto the component's // Deployment pod-template annotations. It matches operands by the API component's // well-known names, so for a component without that ConfigMap+Deployment pair it -// is a no-op. Mutating the Deployment in place is safe: the object was just -// rendered and has not yet been applied. -func stampConfigHash(objs []client.Object, entries []hashEntry) { +// is a no-op beyond returning the (ConfigMap-less) hash. Mutating the Deployment +// in place is safe: the object was just rendered and has not yet been applied. +// The returned hash is also folded into the applied-config metric (see +// hashConfig) so a Secret rotation or resolved-value drift shows up there too, +// not just as a pod-template rollout. +func stampConfigHash(objs []client.Object, entries []hashEntry) string { var configYAML string for _, o := range objs { if cm, ok := o.(*corev1.ConfigMap); ok && cm.Name == apicomponent.ConfigMapName { @@ -431,4 +434,5 @@ func stampConfigHash(objs []client.Object, entries []hashEntry) { } dep.Spec.Template.Annotations[configHashAnnotation] = hash } + return hash } diff --git a/internal/controller/observability.go b/internal/controller/observability.go index 60ad25a..49ee0ca 100644 --- a/internal/controller/observability.go +++ b/internal/controller/observability.go @@ -21,6 +21,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "io" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -40,18 +41,28 @@ import ( // content into the template; this annotation already accounts for it. const templateHashAnnotation = "hyperfleet.redhat.com/template-hash" -// hashConfig returns a short, stable digest of the applied spec. json.Marshal of a -// Go struct is field-ordered and deterministic, so equal specs hash equally across -// reconciles and process restarts. -func hashConfig(spec hyperfleetv1alpha1.HyperFleetConfigSpec) string { +// hashConfig returns a short, stable digest of the fully-applied state: the CR +// spec plus every rendered component's config-rollout digest (see +// computeConfigHash, returned by stampConfigHash). Spec alone is not enough — a +// referenced Secret rotation, or a resolved value that never lands in the CR +// (e.g. resolveJWKSURL's OIDC discovery), can change what is actually applied to +// an operand without the spec itself changing, and this metric must reflect +// that too. json.Marshal of a Go struct is field-ordered and deterministic, so +// equal inputs hash equally across reconciles and process restarts. +func hashConfig(spec hyperfleetv1alpha1.HyperFleetConfigSpec, componentConfigHashes []string) string { b, err := json.Marshal(spec) if err != nil { // A spec that cannot be marshaled is not something the caller can act on; - // fall back to a sentinel so the metric still publishes a single series. - return "unmarshalable" + // fall back to a sentinel rather than fail the metric outright. + b = []byte("unmarshalable") } - sum := sha256.Sum256(b) - return hex.EncodeToString(sum[:])[:12] + h := sha256.New() + _, _ = h.Write(b) + for _, ch := range componentConfigHashes { + _, _ = h.Write([]byte{0}) + _, _ = io.WriteString(h, ch) + } + return hex.EncodeToString(h.Sum(nil))[:12] } // hashPodTemplate returns a short, stable digest of a Deployment's pod template. @@ -73,14 +84,26 @@ func stampTemplateHash(dep *appsv1.Deployment, hash string) { dep.Annotations[templateHashAnnotation] = hash } -// recordRollouts inspects each Deployment a component wants to apply and, when the -// apply will roll the workload, increments the rollout counter with the trigger. -// It must run BEFORE apply, while the live object still holds the previous state. -// It also stamps the desired template hash onto the object so the apply persists -// it. Metrics are best-effort: a read error here is logged and skipped, never -// surfaced as a reconcile failure. -func (r *HyperFleetConfigReconciler) recordRollouts(ctx context.Context, component string, objs []client.Object) { +// rolloutEvent is a rollout detected by detectRollouts but not yet reported to +// the rollout counter. Splitting detection from reporting lets the caller defer +// metrics.IncOperandRollout until apply has actually succeeded, so a failed +// apply — retried on the next reconcile — is not counted as a rollout that +// never happened. +type rolloutEvent struct { + component string + trigger string +} + +// detectRollouts inspects each Deployment a component wants to apply and reports +// which ones the apply will roll, and why. It must run BEFORE apply, while the +// live object still holds the previous state. It also stamps the desired +// template hash onto the object so the apply persists it. Callers must pass the +// returned events to commitRollouts only after apply succeeds — see that +// function's doc comment. Detection itself is best-effort: a read error here is +// logged and skipped, never surfaced as a reconcile failure. +func (r *HyperFleetConfigReconciler) detectRollouts(ctx context.Context, component string, objs []client.Object) []rolloutEvent { log := logf.FromContext(ctx) + var events []rolloutEvent for _, o := range objs { dep, ok := o.(*appsv1.Deployment) if !ok { @@ -93,7 +116,7 @@ func (r *HyperFleetConfigReconciler) recordRollouts(ctx context.Context, compone err := r.Get(ctx, client.ObjectKeyFromObject(dep), live) switch { case apierrors.IsNotFound(err): - metrics.IncOperandRollout(component, metrics.TriggerCreate) + events = append(events, rolloutEvent{component: component, trigger: metrics.TriggerCreate}) case err != nil: log.V(1).Info("skipping rollout metric: could not read live operand", "component", component, "deployment", dep.Name, "error", err.Error()) @@ -103,10 +126,21 @@ func (r *HyperFleetConfigReconciler) recordRollouts(ctx context.Context, compone // reconcile after upgrading to this operator version): adopt the hash // silently rather than count a rollout we cannot attribute. if prev != "" && prev != desired { - metrics.IncOperandRollout(component, rolloutTrigger(live, dep)) + events = append(events, rolloutEvent{component: component, trigger: rolloutTrigger(live, dep)}) } } } + return events +} + +// commitRollouts reports previously-detected rollout events to the rollout +// counter. Call it only after the apply that carries the stamped template hash +// has succeeded, so a failed apply (which leaves the live object's annotation +// unchanged, and is retried on the next reconcile) is never counted. +func commitRollouts(events []rolloutEvent) { + for _, e := range events { + metrics.IncOperandRollout(e.component, e.trigger) + } } // rolloutTrigger classifies why a rollout is happening: an image change if any diff --git a/internal/controller/observability_test.go b/internal/controller/observability_test.go index 6b8a791..bebe8d1 100644 --- a/internal/controller/observability_test.go +++ b/internal/controller/observability_test.go @@ -7,6 +7,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" "github.com/openshift-hyperfleet/hyperfleet-operator/internal/metrics" ) @@ -56,3 +57,24 @@ func TestSameContainerImages(t *testing.T) { g.Expect(sameContainerImages(depWithImages("a:1"), depWithImages("a:1", "b:1"))). To(BeFalse(), "a differing container count is not equal") } + +// TestHashConfigCoversComponentHashesNotJustSpec verifies the applied-config +// digest changes when a component's config-rollout hash changes (e.g. a +// referenced Secret rotation or resolved-value drift), even though the CR spec +// itself is unchanged. Spec-only hashing would miss exactly this case — see the +// PR review this addresses. +func TestHashConfigCoversComponentHashesNotJustSpec(t *testing.T) { + g := NewWithT(t) + + spec := hyperfleetv1alpha1.HyperFleetConfigSpec{Bundle: hyperfleetv1alpha1.BundleCloudCAPI} + + same := hashConfig(spec, []string{"component-hash-v1"}) + g.Expect(hashConfig(spec, []string{"component-hash-v1"})).To(Equal(same), + "identical spec and component hashes must hash equally") + + g.Expect(hashConfig(spec, []string{"component-hash-v2"})).NotTo(Equal(same), + "a changed component config hash (e.g. from a Secret rotation) must change the digest even though the spec did not") + + g.Expect(hashConfig(spec, nil)).NotTo(Equal(same), + "a missing component hash must not collide with a present one") +}