diff --git a/README.md b/README.md index f6ed7e85..015db722 100644 --- a/README.md +++ b/README.md @@ -451,6 +451,92 @@ sequenceDiagram end ``` +## Metrics + +The controller serves Prometheus metrics on `:8443/metrics`. The endpoint is +protected by [controller-runtime][cr]'s built-in authentication and +authorization, so scrapers must present a bearer token belonging to a +ServiceAccount that has the `-metrics-reader` ClusterRole bound to it. + +Alongside the standard controller-runtime metrics (reconcile counts, webhook +latencies, workqueue depths), **Oz** exports metrics describing how it is being +*used*: + +| Metric | Type | Labels | Description | +| ------ | ---- | ------ | ----------- | +| `oz_access_request_created_total` | counter | `kind`, `namespace`, `template`, `user` | Access requests admitted by the validating webhook. | +| `oz_access_request_requested_duration_seconds` | histogram | `kind`, `namespace`, `template` | Duration explicitly asked for in `spec.duration`. Requests that inherit the template default are not observed. | +| `oz_access_request_ready_seconds` | histogram | `kind`, `namespace`, `template` | Time from creation to `Status.Ready`, ie how long the user waited before they could use their access. | +| `oz_access_request_terminated_total` | counter | `kind`, `namespace`, `template`, `reason` | Requests that reached a terminal state. `reason` is `expired` (the happy path), `duration_invalid` or `duration_too_long`. | +| `oz_access_request_condition_errors_total` | counter | `kind`, `namespace`, `template`, `condition` | Reconcile *attempts* that failed a verification step, labelled with the `Status.Condition` set to `False`. A permanently wedged request keeps incrementing this. | +| `oz_access_requests` | gauge | `kind`, `namespace`, `template`, `user`, `ready` | Access requests that exist right now. | +| `oz_access_templates` | gauge | `kind`, `namespace`, `name`, `ready` | Access templates that exist right now. | +| `oz_pod_exec_total` | counter | `namespace`, `subresource`, `user`, `interactive` | `exec`/`attach` operations seen by the Pod watcher webhook. Covers all such traffic in the cluster, not just Oz-managed Pods. | + +Some examples: + +```promql +# Which templates are being used, and by whom, over the last week? +sum by (template, user) (increase(oz_access_request_created_total[7d])) + +# Who currently holds access? Note the `max` rather than `sum` - see the +# caveat about replicas below. +max by (user, template) (oz_access_requests{ready="true"}) + +# Templates that nobody has requested access through in the last 30 days. +# Note the join is on (namespace, name) only - the `kind` label differs between +# the two metrics ("PodAccessTemplate" vs "PodAccessRequest"). +oz_access_templates + unless on (namespace, name) + label_replace( + sum by (namespace, template) (increase(oz_access_request_created_total[30d])) > 0, + "name", "$1", "template", "(.*)" + ) + +# 95th percentile wait for a Pod-based grant to become usable +histogram_quantile(0.95, sum by (le, template) (rate(oz_access_request_ready_seconds_bucket[1h]))) +``` + +The two gauges (`oz_access_requests` and `oz_access_templates`) are computed by +listing from the controller's cache at scrape time, so every replica reports the +same inventory. If you run `controllerManager.replicas > 1`, aggregate them with +`max by (...)` rather than `sum by (...)`, or the counts will be multiplied by +the replica count. The counters and histograms are not affected - only the +elected leader reconciles, and each webhook call is handled once. + +### Attributing usage to users + +The requesting user's identity is only visible inside an admission webhook, so +**Oz** stamps it onto each request as the `crds.wizardofoz.co/requested-by` +annotation during mutation. The annotation is set from authoritative API server +data on create (overwriting any client-supplied value) and is rejected from +being modified on update, so it is trustworthy for reporting - and it means you +can also audit access with plain `kubectl`: + +```bash +kubectl get podaccessrequests -A \ + -o custom-columns=NS:.metadata.namespace,NAME:.metadata.name,TEMPLATE:.spec.templateName,USER:'.metadata.annotations.crds\.wizardofoz\.co/requested-by' +``` + +The `user` **metric label** is opt-in, and reported as `redacted` until you turn +it on: + +```yaml +# values.yaml +metrics: + includeUserLabel: true + serviceMonitor: + create: true +``` + +It defaults to off for two reasons: usernames are frequently email addresses +(ie personally identifying, and metrics tend to be far more widely readable than +the cluster itself), and they multiply the series count of the affected metrics +by the number of distinct people using **Oz**. The annotation is always +populated regardless of this setting. + +[cr]: https://github.com/kubernetes-sigs/controller-runtime + ## License Copyright 2022 Matt Wise. diff --git a/charts/oz/README.md b/charts/oz/README.md index a3cfdeab..ed6b6508 100644 --- a/charts/oz/README.md +++ b/charts/oz/README.md @@ -42,6 +42,12 @@ Kubernetes: `>=1.26.0-0` | controllerManager.replicas | `int` | `1` | Number of Oz Controllers to run. If more than one is used, leader-election is used to ensure only one controller is operating at a time. | | controllerManager.tolerations | `[]map]` | `[]` | A list of Tolerations that will be applied to the controller-manager pods. See https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/. | | kubernetesClusterDomain | `string` | `"cluster.local"` | Configures the KUBERNETES_CLUSTER_DOMAIN environment variable. | +| metrics.includeUserLabel | `bool` | `false` | Report real Kubernetes usernames in the `user` label of the `oz_*` usage metrics. Without this, per-user attribution is unavailable and the label is reported as `redacted`. This is opt-in because usernames are frequently email addresses (ie personally identifying), and because they are considerably higher cardinality than the rest of the label set - the series count of the affected metrics is multiplied by the number of distinct people using Oz. | +| metrics.serviceMonitor.create | `bool` | `false` | Whether or not to create a Prometheus Operator `ServiceMonitor` resource pointing at the metrics service. Requires the Prometheus Operator CRDs to be installed in the cluster. If you scrape via some other mechanism, leave this off and point your scraper at the `-controller-manager-metrics-service` Service on port 8443. | +| metrics.serviceMonitor.interval | `string` | `nil` | How often Prometheus should scrape the controller. Defaults to the Prometheus global scrape interval when unset. | +| metrics.serviceMonitor.labels | `map` | `{}` | Additional labels to apply to the `ServiceMonitor`. Most Prometheus Operator installations use a label selector to decide which ServiceMonitors to act on (eg `release: kube-prometheus-stack`), in which case you must set the matching label here. | +| metrics.serviceMonitor.metricRelabelings | `[]map` | `[]` | Optional metric relabeling rules, applied by Prometheus after scraping. Useful for dropping high cardinality series you do not want to keep. | +| metrics.serviceMonitor.scrapeTimeout | `string` | `nil` | Per-scrape timeout. Defaults to the Prometheus global scrape timeout when unset. | | metricsService.ports[0].name | string | `"https"` | | | metricsService.ports[0].port | int | `8443` | | | metricsService.ports[0].protocol | string | `"TCP"` | | diff --git a/charts/oz/templates/deployment.yaml b/charts/oz/templates/deployment.yaml index 5fc206da..9671caed 100644 --- a/charts/oz/templates/deployment.yaml +++ b/charts/oz/templates/deployment.yaml @@ -62,6 +62,9 @@ spec: - --health-probe-bind-address=:8081 - --metrics-bind-address=:8443 - --leader-elect + {{- if .Values.metrics.includeUserLabel }} + - --metrics-include-user-label + {{- end }} securityContext: allowPrivilegeEscalation: false capabilities: diff --git a/charts/oz/templates/servicemonitor.yaml b/charts/oz/templates/servicemonitor.yaml new file mode 100644 index 00000000..452213d1 --- /dev/null +++ b/charts/oz/templates/servicemonitor.yaml @@ -0,0 +1,56 @@ +{{- if .Values.metrics.serviceMonitor.create }} +apiVersion: monitoring.coreos.com/v1 +kind: ServiceMonitor +metadata: + name: {{ include "oz.fullname" . }}-controller-manager-metrics-monitor + namespace: {{ .Release.Namespace }} + {{- /* + Only the labels that "oz.labels" does not already provide are listed here. + The sibling templates repeat several of them, but since "oz.labels" is + included last it wins, so the rendered output is unchanged and this avoids + emitting duplicate YAML keys. + */}} + labels: + app.kubernetes.io/component: metrics + control-plane: controller-manager + {{- include "oz.labels" . | nindent 4 }} + {{- with .Values.metrics.serviceMonitor.labels }} + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + namespaceSelector: + matchNames: + - {{ .Release.Namespace }} + selector: + matchLabels: + control-plane: controller-manager + {{- include "oz.selectorLabels" . | nindent 6 }} + endpoints: + - path: /metrics + port: https + scheme: https + {{- with .Values.metrics.serviceMonitor.interval }} + interval: {{ . }} + {{- end }} + {{- with .Values.metrics.serviceMonitor.scrapeTimeout }} + scrapeTimeout: {{ . }} + {{- end }} + {{- /* + The metrics endpoint is protected by controller-runtime's built-in + authn/authz (SecureServing + WithAuthenticationAndAuthorization), so the + scraper must present a bearer token. The ServiceAccount Prometheus runs + as needs the `-metrics-reader` ClusterRole bound to it, otherwise every + scrape returns 403. + */}} + bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token + {{- /* + controller-runtime generates a self-signed serving certificate on + startup, so there is no CA for Prometheus to verify against. + */}} + tlsConfig: + insecureSkipVerify: true + {{- with .Values.metrics.serviceMonitor.metricRelabelings }} + metricRelabelings: + {{- toYaml . | nindent 8 }} + {{- end }} +{{- end }} diff --git a/charts/oz/values.yaml b/charts/oz/values.yaml index 41e5b19d..571a7a58 100644 --- a/charts/oz/values.yaml +++ b/charts/oz/values.yaml @@ -40,6 +40,47 @@ metricsService: protocol: TCP targetPort: https +# Configuration for the Prometheus metrics that the Oz controller exports. In +# addition to the standard controller-runtime metrics (reconcile counts, +# webhook latencies, workqueue depths), Oz exports usage metrics that attribute +# access requests to templates and users - see the `oz_*` metrics. +metrics: + # -- (`bool`) Report real Kubernetes usernames in the `user` label of the + # `oz_*` usage metrics. Without this, per-user attribution is unavailable and + # the label is reported as `redacted`. This is opt-in because usernames are + # frequently email addresses (ie personally identifying), and because they + # are considerably higher cardinality than the rest of the label set - the + # series count of the affected metrics is multiplied by the number of + # distinct people using Oz. + includeUserLabel: false + + serviceMonitor: + # -- (`bool`) Whether or not to create a Prometheus Operator + # `ServiceMonitor` resource pointing at the metrics service. Requires the + # Prometheus Operator CRDs to be installed in the cluster. If you scrape + # via some other mechanism, leave this off and point your scraper at the + # `-controller-manager-metrics-service` Service on port 8443. + create: false + + # -- (`map`) Additional labels to apply to the `ServiceMonitor`. Most + # Prometheus Operator installations use a label selector to decide which + # ServiceMonitors to act on (eg `release: kube-prometheus-stack`), in which + # case you must set the matching label here. + labels: {} + + # -- (`string`) How often Prometheus should scrape the controller. Defaults + # to the Prometheus global scrape interval when unset. + interval: + + # -- (`string`) Per-scrape timeout. Defaults to the Prometheus global + # scrape timeout when unset. + scrapeTimeout: + + # -- (`[]map`) Optional metric relabeling rules, applied by Prometheus + # after scraping. Useful for dropping high cardinality series you do not + # want to keep. + metricRelabelings: [] + # Configuration for the oz-controller-manager-webhook-service, used for # handling ValidatingWebhookConfiguration and MutatingWebhookConfiguration # calls from the Kubernetes API. diff --git a/go.mod b/go.mod index bfb27d21..7f6e25f1 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/ivanpirog/coloredcobra v1.0.1 github.com/onsi/ginkgo/v2 v2.28.1 github.com/onsi/gomega v1.39.1 + github.com/prometheus/client_golang v1.23.2 github.com/spf13/cobra v1.10.2 go.uber.org/zap v1.27.1 k8s.io/api v0.35.4 @@ -56,6 +57,7 @@ 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/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect github.com/mailru/easyjson v0.9.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect @@ -67,7 +69,6 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/peterbourgon/diskv v2.0.1+incompatible // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect - github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.66.1 // indirect github.com/prometheus/procfs v0.16.1 // indirect diff --git a/internal/api/v1alpha1/exec_access_request_test.go b/internal/api/v1alpha1/exec_access_request_test.go index 635c040d..8173e44b 100644 --- a/internal/api/v1alpha1/exec_access_request_test.go +++ b/internal/api/v1alpha1/exec_access_request_test.go @@ -83,11 +83,41 @@ var _ = Describe("ExecAccessRequest", Ordered, func() { }, request) Expect(err).To(Not(HaveOccurred())) - // Update it and push it - request.SetAnnotations(map[string]string{"foo": "bar"}) + // Update it and push it. Note that we add an annotation rather + // than replacing the map wholesale - the requested-by annotation is + // immutable, so wiping it out is rejected (see below). + annotations := request.GetAnnotations() + annotations["foo"] = "bar" + request.SetAnnotations(annotations) err = k8sClient.Update(ctx, request) Expect(err).To(Not(HaveOccurred())) }) + + It("ExecAccessRequest records the requesting user", func() { + request := &ExecAccessRequest{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: requestName, + Namespace: template.Namespace, + }, request) + Expect(err).To(Not(HaveOccurred())) + Expect(GetRequestedBy(request)).To(Not(BeEmpty())) + }) + + It("ExecAccessRequest Update - Rejects a change to the requested-by annotation", func() { + request := &ExecAccessRequest{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: requestName, + Namespace: template.Namespace, + }, request) + Expect(err).To(Not(HaveOccurred())) + + // Wiping the annotations (or editing just this one) would let a + // user disown or misattribute their own access request. + request.SetAnnotations(map[string]string{"foo": "bar"}) + err = k8sClient.Update(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(AnnotationRequestedBy)) + }) It("ExecAccessRequest Delete - Passes Webhook ValidateUpdate() Call", func() { // Get the request first request := &ExecAccessRequest{ diff --git a/internal/api/v1alpha1/exec_access_request_webhook.go b/internal/api/v1alpha1/exec_access_request_webhook.go index 6fb93193..669b8094 100644 --- a/internal/api/v1alpha1/exec_access_request_webhook.go +++ b/internal/api/v1alpha1/exec_access_request_webhook.go @@ -51,7 +51,10 @@ func (r *ExecAccessRequest) SetupWebhookWithManager(mgr ctrl.Manager) error { var _ webhook.IContextuallyDefaultableObject = &ExecAccessRequest{} // Default implements webhook.Defaulter so a webhook will be registered for the type -func (r *ExecAccessRequest) Default(_ admission.Request) error { +func (r *ExecAccessRequest) Default(req admission.Request) error { + // Record who asked for this access, so that the reconciler and the metrics + // collector can attribute it later. + SetRequestedBy(r, req) return nil } @@ -72,11 +75,15 @@ func (r *ExecAccessRequest) ValidateCreate(req admission.Request) (admission.War warnings = append(warnings, w) execaccessrequestlog.Info(w) } + recordAccessRequestCreated(req, r) return warnings, nil } // ValidateUpdate prevents immutable updates to the ExecAccessRequest. -func (r *ExecAccessRequest) ValidateUpdate(_ admission.Request, old runtime.Object) (admission.Warnings, error) { +func (r *ExecAccessRequest) ValidateUpdate( + _ admission.Request, + old runtime.Object, +) (admission.Warnings, error) { execaccessrequestlog.Info("validate update", "name", r.Name) // https://stackoverflow.com/questions/70650677/manage-immutable-fields-in-kubebuilder-validating-webhook @@ -86,6 +93,13 @@ func (r *ExecAccessRequest) ValidateUpdate(_ admission.Request, old runtime.Obje "error - Spec.TargetPod is an immutable field, create a new PodAccessRequest instead", ) } + + // The requested-by annotation is the basis for all per-user reporting, so + // it must not be editable after creation. + if err := ValidateRequestedByUnchanged(oldRequest, r); err != nil { + return nil, err + } + return nil, nil } diff --git a/internal/api/v1alpha1/pod_access_request_test.go b/internal/api/v1alpha1/pod_access_request_test.go index a962d146..d6bb4848 100644 --- a/internal/api/v1alpha1/pod_access_request_test.go +++ b/internal/api/v1alpha1/pod_access_request_test.go @@ -83,11 +83,41 @@ var _ = Describe("PodAccessRequest", Ordered, func() { }, request) Expect(err).To(Not(HaveOccurred())) - // Update it and push it - request.SetAnnotations(map[string]string{"foo": "bar"}) + // Update it and push it. Note that we add an annotation rather + // than replacing the map wholesale - the requested-by annotation is + // immutable, so wiping it out is rejected (see below). + annotations := request.GetAnnotations() + annotations["foo"] = "bar" + request.SetAnnotations(annotations) err = k8sClient.Update(ctx, request) Expect(err).To(Not(HaveOccurred())) }) + + It("PodAccessRequest records the requesting user", func() { + request := &PodAccessRequest{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: requestName, + Namespace: template.Namespace, + }, request) + Expect(err).To(Not(HaveOccurred())) + Expect(GetRequestedBy(request)).To(Not(BeEmpty())) + }) + + It("PodAccessRequest Update - Rejects a change to the requested-by annotation", func() { + request := &PodAccessRequest{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: requestName, + Namespace: template.Namespace, + }, request) + Expect(err).To(Not(HaveOccurred())) + + // Wiping the annotations (or editing just this one) would let a + // user disown or misattribute their own access request. + request.SetAnnotations(map[string]string{"foo": "bar"}) + err = k8sClient.Update(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(AnnotationRequestedBy)) + }) It("PodAccessRequest Delete - Passes Webhook ValidateUpdate() Call", func() { // Get the request first request := &PodAccessRequest{ diff --git a/internal/api/v1alpha1/pod_access_request_webhook.go b/internal/api/v1alpha1/pod_access_request_webhook.go index c50bbf4f..78671f20 100644 --- a/internal/api/v1alpha1/pod_access_request_webhook.go +++ b/internal/api/v1alpha1/pod_access_request_webhook.go @@ -51,7 +51,10 @@ func (r *PodAccessRequest) SetupWebhookWithManager(mgr ctrl.Manager) error { var _ webhook.IContextuallyDefaultableObject = &PodAccessRequest{} // Default implements webhook.Defaulter so a webhook will be registered for the type -func (r *PodAccessRequest) Default(_ admission.Request) error { +func (r *PodAccessRequest) Default(req admission.Request) error { + // Record who asked for this access, so that the reconciler and the metrics + // collector can attribute it later. + SetRequestedBy(r, req) return nil } @@ -72,11 +75,15 @@ func (r *PodAccessRequest) ValidateCreate(req admission.Request) (admission.Warn warnings = append(warnings, w) podaccessrequestlog.Info(w) } + recordAccessRequestCreated(req, r) return warnings, nil } // ValidateUpdate implements webhook.IContextuallyValidatableObject so a webhook will be registered for the type -func (r *PodAccessRequest) ValidateUpdate(req admission.Request, _ runtime.Object) (admission.Warnings, error) { +func (r *PodAccessRequest) ValidateUpdate( + req admission.Request, + old runtime.Object, +) (admission.Warnings, error) { warnings := admission.Warnings{} if req.UserInfo.Username != "" { podaccessrequestlog.Info( @@ -88,6 +95,15 @@ func (r *PodAccessRequest) ValidateUpdate(req admission.Request, _ runtime.Objec warnings = append(warnings, w) podaccessrequestlog.Info(w) } + + // The requested-by annotation is the basis for all per-user reporting, so + // it must not be editable after creation. + if oldRequest, ok := old.(*PodAccessRequest); ok { + if err := ValidateRequestedByUnchanged(oldRequest, r); err != nil { + return warnings, err + } + } + return warnings, nil } diff --git a/internal/api/v1alpha1/requested_by.go b/internal/api/v1alpha1/requested_by.go new file mode 100644 index 00000000..15a0f946 --- /dev/null +++ b/internal/api/v1alpha1/requested_by.go @@ -0,0 +1,85 @@ +package v1alpha1 + +import ( + "fmt" + + admissionv1 "k8s.io/api/admission/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// AnnotationRequestedBy records the Kubernetes username that created an access +// request, as reported by the API server on the admission request. +// +// The identity is only ever known inside an admission webhook - by the time a +// request reaches the reconciler, the "who" has been lost. Stamping it onto the +// object during mutation is what makes per-user reporting (and after-the-fact +// auditing with plain kubectl) possible. +// +// Do not read this field as a security control on its own; it is set from +// authoritative API server data on create and is immutable thereafter, but it +// says who *asked* for access, not who was *granted* it. Access itself is +// granted to the template's allowedGroups. +const AnnotationRequestedBy = "crds.wizardofoz.co/requested-by" + +// SetRequestedBy stamps the AnnotationRequestedBy annotation onto an object +// from the authenticated user identity on an admission request. It is intended +// to be called from a mutating (defaulting) webhook. +// +// This only acts on CREATE operations, for two reasons: the annotation should +// record the original requester rather than whoever most recently touched the +// object, and the Oz reconciler itself issues Updates against these objects +// (to set owner references), which would otherwise rewrite the annotation to +// the controller's own service account. +// +// On create the annotation is set unconditionally, overwriting any value the +// client supplied, so that a user cannot attribute their request to somebody +// else. If the API server supplied no identity at all the annotation is +// removed rather than left at a client-supplied value. +func SetRequestedBy(obj metav1.Object, req admission.Request) { + if req.Operation != admissionv1.Create { + return + } + + annotations := obj.GetAnnotations() + + if req.UserInfo.Username == "" { + delete(annotations, AnnotationRequestedBy) + obj.SetAnnotations(annotations) + return + } + + if annotations == nil { + annotations = map[string]string{} + } + annotations[AnnotationRequestedBy] = req.UserInfo.Username + obj.SetAnnotations(annotations) +} + +// GetRequestedBy returns the username recorded by SetRequestedBy, or an empty +// string if the object predates this feature or was created without an +// identity. +func GetRequestedBy(obj metav1.Object) string { + return obj.GetAnnotations()[AnnotationRequestedBy] +} + +// ValidateRequestedByUnchanged returns an error if an update would modify the +// AnnotationRequestedBy annotation. +// +// SetRequestedBy deliberately ignores updates so that the controller's own +// writes do not clobber the requester. That leaves the annotation editable by +// anyone with update access to the object, which would make it worthless for +// reporting - so updates to it are rejected here instead. +func ValidateRequestedByUnchanged(old, updated metav1.Object) error { + oldValue := GetRequestedBy(old) + newValue := GetRequestedBy(updated) + if oldValue == newValue { + return nil + } + return fmt.Errorf( + "error - %s is an immutable annotation (%q), cannot update to %q", + AnnotationRequestedBy, + oldValue, + newValue, + ) +} diff --git a/internal/api/v1alpha1/requested_by_test.go b/internal/api/v1alpha1/requested_by_test.go new file mode 100644 index 00000000..81e2d350 --- /dev/null +++ b/internal/api/v1alpha1/requested_by_test.go @@ -0,0 +1,126 @@ +package v1alpha1 + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// admissionRequestFor builds a minimal admission.Request carrying an operation +// and an authenticated username. +func admissionRequestFor(op admissionv1.Operation, username string) admission.Request { + return admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: op, + UserInfo: authenticationv1.UserInfo{Username: username}, + }, + } +} + +var _ = Describe("RequestedBy annotation", func() { + Context("SetRequestedBy", func() { + It("stamps the authenticated user on create", func() { + request := &PodAccessRequest{} + SetRequestedBy(request, admissionRequestFor(admissionv1.Create, "dennis")) + Expect(GetRequestedBy(request)).To(Equal("dennis")) + }) + + It("preserves any annotations that were already present", func() { + request := &PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"unrelated": "value"}, + }, + } + SetRequestedBy(request, admissionRequestFor(admissionv1.Create, "dennis")) + Expect(GetRequestedBy(request)).To(Equal("dennis")) + Expect(request.GetAnnotations()).To(HaveKeyWithValue("unrelated", "value")) + }) + + It("overwrites a client-supplied value on create, so it cannot be spoofed", func() { + request := &PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{AnnotationRequestedBy: "somebody-else"}, + }, + } + SetRequestedBy(request, admissionRequestFor(admissionv1.Create, "dennis")) + Expect(GetRequestedBy(request)).To(Equal("dennis")) + }) + + It("removes a client-supplied value when the API server gave no identity", func() { + request := &PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{AnnotationRequestedBy: "somebody-else"}, + }, + } + SetRequestedBy(request, admissionRequestFor(admissionv1.Create, "")) + Expect(GetRequestedBy(request)).To(BeEmpty()) + }) + + // The reconciler issues Updates to set owner references. If those + // rewrote the annotation, every request would end up attributed to the + // Oz controller's own service account. + It("leaves the annotation alone on update", func() { + request := &PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{AnnotationRequestedBy: "dennis"}, + }, + } + SetRequestedBy(request, admissionRequestFor( + admissionv1.Update, "system:serviceaccount:oz-system:oz-controller-manager", + )) + Expect(GetRequestedBy(request)).To(Equal("dennis")) + }) + + It("does not create the annotation on update", func() { + request := &PodAccessRequest{} + SetRequestedBy(request, admissionRequestFor(admissionv1.Update, "dennis")) + Expect(GetRequestedBy(request)).To(BeEmpty()) + }) + + It("works for ExecAccessRequests too", func() { + request := &ExecAccessRequest{} + SetRequestedBy(request, admissionRequestFor(admissionv1.Create, "dennis")) + Expect(GetRequestedBy(request)).To(Equal("dennis")) + }) + }) + + Context("ValidateRequestedByUnchanged", func() { + withUser := func(username string) *PodAccessRequest { + return &PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{AnnotationRequestedBy: username}, + }, + } + } + + It("permits an update that does not touch the annotation", func() { + Expect(ValidateRequestedByUnchanged(withUser("dennis"), withUser("dennis"))). + To(Succeed()) + }) + + It("permits an update on an object that never had the annotation", func() { + Expect(ValidateRequestedByUnchanged(&PodAccessRequest{}, &PodAccessRequest{})). + To(Succeed()) + }) + + It("rejects an attempt to reassign the requester", func() { + err := ValidateRequestedByUnchanged(withUser("dennis"), withUser("somebody-else")) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(AnnotationRequestedBy)) + }) + + It("rejects an attempt to remove the annotation", func() { + err := ValidateRequestedByUnchanged(withUser("dennis"), &PodAccessRequest{}) + Expect(err).To(HaveOccurred()) + }) + + It("rejects an attempt to add the annotation after the fact", func() { + err := ValidateRequestedByUnchanged(&PodAccessRequest{}, withUser("dennis")) + Expect(err).To(HaveOccurred()) + }) + }) +}) diff --git a/internal/api/v1alpha1/webhook_metrics.go b/internal/api/v1alpha1/webhook_metrics.go new file mode 100644 index 00000000..e95c930a --- /dev/null +++ b/internal/api/v1alpha1/webhook_metrics.go @@ -0,0 +1,45 @@ +package v1alpha1 + +import ( + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/diranged/oz/internal/metrics" +) + +// recordAccessRequestCreated emits the create-time usage metrics for an access +// request. It is called from the validating webhook rather than the reconciler +// because the requesting user's identity is only available on the admission +// request. +// +// The namespace is taken from the admission request rather than the object, +// because the admission request is authoritative for namespaced resources even +// when the client omitted metadata.namespace. +// +// Dry-run admissions are skipped. The API server runs the full webhook chain +// for `kubectl apply --dry-run=server`, but nothing is persisted, so counting +// them would inflate usage with requests that were never actually granted. +func recordAccessRequestCreated(req admission.Request, obj IRequestResource) { + if req.DryRun != nil && *req.DryRun { + return + } + + kind := metrics.Kind(obj) + + metrics.AccessRequestCreatedTotal.WithLabelValues( + kind, + req.Namespace, + obj.GetTemplateName(), + metrics.UserLabel(req.UserInfo.Username), + ).Inc() + + // Only observe a duration the user actually asked for. An unset (or + // unparseable) spec.duration means the template's default applies, and we + // have no client here with which to go resolve the template. + if duration, err := obj.GetDuration(); err == nil && duration > 0 { + metrics.AccessRequestRequestedDurationSeconds.WithLabelValues( + kind, + req.Namespace, + obj.GetTemplateName(), + ).Observe(duration.Seconds()) + } +} diff --git a/internal/api/v1alpha1/webhook_metrics_test.go b/internal/api/v1alpha1/webhook_metrics_test.go new file mode 100644 index 00000000..3e531dba --- /dev/null +++ b/internal/api/v1alpha1/webhook_metrics_test.go @@ -0,0 +1,135 @@ +package v1alpha1 + +import ( + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/diranged/oz/internal/metrics" +) + +// histogramSumFor returns the observed sum for one label set of the requested +// duration histogram. testutil.ToFloat64 cannot be used here - it only handles +// single-value metrics, not histograms - so gather the vector into a scratch +// registry and pick out the matching series. +func histogramSumFor(labelValues ...string) float64 { + registry := prometheus.NewPedanticRegistry() + ExpectWithOffset(1, registry.Register(metrics.AccessRequestRequestedDurationSeconds)). + To(Succeed()) + + families, err := registry.Gather() + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + want := strings.Join(labelValues, "\x00") + for _, family := range families { + for _, metric := range family.GetMetric() { + got := []string{} + for _, label := range metric.GetLabel() { + got = append(got, label.GetValue()) + } + if strings.Join(got, "\x00") == want { + return metric.GetHistogram().GetSampleSum() + } + } + } + return -1 +} + +var _ = Describe("Webhook metrics", func() { + const ( + namespace = "metrics-test" + template = "webhook-metrics-template" + ) + + // createRequestFor builds the admission request the API server would send + // for a create, optionally flagged as a dry run. + createRequestFor := func(dryRun bool) admission.Request { + return admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Namespace: namespace, + DryRun: &dryRun, + UserInfo: authenticationv1.UserInfo{Username: "dennis"}, + }, + } + } + + created := func() float64 { + return testutil.ToFloat64(metrics.AccessRequestCreatedTotal.WithLabelValues( + "PodAccessRequest", namespace, template, metrics.UserRedacted, + )) + } + + It("counts a real create", func() { + before := created() + recordAccessRequestCreated( + createRequestFor(false), + &PodAccessRequest{Spec: PodAccessRequestSpec{TemplateName: template}}, + ) + Expect(created()).To(Equal(before + 1)) + }) + + // The API server runs the whole webhook chain for `kubectl apply + // --dry-run=server`, but persists nothing. Counting those would report + // access that was never granted. + It("does not count a dry-run create", func() { + before := created() + recordAccessRequestCreated( + createRequestFor(true), + &PodAccessRequest{Spec: PodAccessRequestSpec{TemplateName: template}}, + ) + Expect(created()).To(Equal(before)) + }) + + It("does not count a dry-run create for ExecAccessRequests either", func() { + labels := []string{"ExecAccessRequest", namespace, template, metrics.UserRedacted} + before := testutil.ToFloat64( + metrics.AccessRequestCreatedTotal.WithLabelValues(labels...), + ) + recordAccessRequestCreated( + createRequestFor(true), + &ExecAccessRequest{Spec: ExecAccessRequestSpec{TemplateName: template}}, + ) + Expect(testutil.ToFloat64( + metrics.AccessRequestCreatedTotal.WithLabelValues(labels...), + )).To(Equal(before)) + }) + + It("observes an explicitly requested duration", func() { + before := testutil.CollectAndCount(metrics.AccessRequestRequestedDurationSeconds) + + recordAccessRequestCreated( + createRequestFor(false), + &PodAccessRequest{Spec: PodAccessRequestSpec{ + TemplateName: "duration-template", + Duration: "4h", + }}, + ) + + Expect(testutil.CollectAndCount(metrics.AccessRequestRequestedDurationSeconds)). + To(BeNumerically(">", before)) + // Labels are gathered in alphabetical order: kind, namespace, template. + Expect(histogramSumFor("PodAccessRequest", namespace, "duration-template")). + To(Equal(float64(4 * 60 * 60))) + }) + + // An omitted spec.duration means "use the template default", which the + // webhook cannot resolve, so there is nothing meaningful to observe. + It("does not observe a request that inherits the template default", func() { + before := testutil.CollectAndCount(metrics.AccessRequestRequestedDurationSeconds) + recordAccessRequestCreated( + createRequestFor(false), + &PodAccessRequest{Spec: PodAccessRequestSpec{TemplateName: "no-duration-template"}}, + ) + Expect(testutil.CollectAndCount(metrics.AccessRequestRequestedDurationSeconds)). + To(Equal(before)) + }) +}) diff --git a/internal/cmd/manager/main.go b/internal/cmd/manager/main.go index 45c89ca0..d775c6e8 100644 --- a/internal/cmd/manager/main.go +++ b/internal/cmd/manager/main.go @@ -39,6 +39,8 @@ import ( "github.com/diranged/oz/internal/controllers/podwatcher" "github.com/diranged/oz/internal/controllers/requestcontroller" "github.com/diranged/oz/internal/controllers/templatecontroller" + ozmetrics "github.com/diranged/oz/internal/metrics" + metricscollector "github.com/diranged/oz/internal/metrics/collector" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" //+kubebuilder:scaffold:imports @@ -72,6 +74,7 @@ func Main() { var enableLeaderElection bool var requestReconciliationInterval int var templateReconciliationInterval int + var metricsIncludeUserLabel bool // Boilerplate flag.StringVar( @@ -95,6 +98,15 @@ func Main() { 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( + &metricsIncludeUserLabel, + "metrics-include-user-label", + false, + "Report real Kubernetes usernames in the 'user' label of the Oz usage metrics. "+ + "Off by default because usernames are frequently email addresses, and are "+ + "higher cardinality than the rest of the label set. When off, the label is "+ + "reported as 'redacted'.", + ) // Custom flag.IntVar( @@ -129,6 +141,10 @@ func Main() { rootLogger := zap.New(zap.UseFlagOptions(&opts)) ctrl.SetLogger(rootLogger) + // Must happen before the webhook server starts serving, because the + // webhook handlers consult this on every observation. + ozmetrics.SetIncludeUserLabel(metricsIncludeUserLabel) + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, Metrics: metricsserver.Options{ @@ -237,6 +253,12 @@ func Main() { //+kubebuilder:scaffold:builder + // Report point-in-time inventory of Oz resources (how many requests are + // live, for which templates, for whom) by listing from the manager's + // informer cache at scrape time. Registered after the reconcilers so that + // the caches it reads from are the ones they already populate. + metricscollector.MustRegister(mgr.GetClient()) + if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { setupLog.Error(err, "unable to set up health check") os.Exit(1) diff --git a/internal/controllers/podwatcher/handle_attach.go b/internal/controllers/podwatcher/handle_attach.go index 21e2d974..b868e5e9 100644 --- a/internal/controllers/podwatcher/handle_attach.go +++ b/internal/controllers/podwatcher/handle_attach.go @@ -41,6 +41,7 @@ func (w *PodWatcher) HandleAttach(ctx context.Context, req admission.Request) ad // Log and Record the event w.recorder.Eventf(pod, nil, "Normal", "PodAttach", "RecordedAttach", "%s", eventMsg) logger.Info(eventMsg) + recordPodConnect(req, opts.TTY) return admission.Allowed("") } diff --git a/internal/controllers/podwatcher/handle_exec.go b/internal/controllers/podwatcher/handle_exec.go index e4e109df..26bd7426 100644 --- a/internal/controllers/podwatcher/handle_exec.go +++ b/internal/controllers/podwatcher/handle_exec.go @@ -41,6 +41,7 @@ func (w *PodWatcher) HandleExec(ctx context.Context, req admission.Request) admi // Log and Record the event w.recorder.Eventf(pod, nil, "Normal", "PodExec", "RecordedExec", "%s", eventMsg) logger.Info(eventMsg) + recordPodConnect(req, opts.TTY) return admission.Allowed("") } diff --git a/internal/controllers/podwatcher/metrics.go b/internal/controllers/podwatcher/metrics.go new file mode 100644 index 00000000..8a68a9f5 --- /dev/null +++ b/internal/controllers/podwatcher/metrics.go @@ -0,0 +1,23 @@ +package podwatcher + +import ( + "strconv" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/diranged/oz/internal/metrics" +) + +// recordPodConnect counts an exec or attach operation against a Pod. +// +// The Pod name is deliberately not recorded as a label - Oz-created Pods have +// generated names, so it would be unbounded cardinality. Use the Kubernetes +// Events that this webhook also emits when you need to trace a specific Pod. +func recordPodConnect(req admission.Request, interactive bool) { + metrics.PodExecTotal.WithLabelValues( + req.Namespace, + req.SubResource, + metrics.UserLabel(req.UserInfo.Username), + strconv.FormatBool(interactive), + ).Inc() +} diff --git a/internal/controllers/requestcontroller/controller.go b/internal/controllers/requestcontroller/controller.go index 67379f16..2d64576f 100644 --- a/internal/controllers/requestcontroller/controller.go +++ b/internal/controllers/requestcontroller/controller.go @@ -110,10 +110,19 @@ func (r *RequestReconciler) reconcile(rctx *RequestContext) (ctrl.Result, error) // FINAL: Set Status.Ready state // // TODO: Implement on the ICoreStatus interface a "AreAllConditionsTrue" function and check that. + // + // Status.Ready is only ever written here, and UpdateStatus() refetches the + // object, so the value we read now is the one persisted by the previous + // reconcile. That makes this a reliable edge-trigger for observing how long + // the user waited for their access. + wasReady := rctx.obj.GetStatus().IsReady() err = status.SetReadyStatus(rctx, r, rctx.obj) if err != nil { return ctrl.Result{}, err } + if !wasReady && rctx.obj.GetStatus().IsReady() { + recordReady(rctx) + } // Exit Reconciliation Loop rctx.log.Info("Ending reconcile loop") diff --git a/internal/controllers/requestcontroller/is_access_expired.go b/internal/controllers/requestcontroller/is_access_expired.go index a9e084c0..8713676c 100644 --- a/internal/controllers/requestcontroller/is_access_expired.go +++ b/internal/controllers/requestcontroller/is_access_expired.go @@ -8,6 +8,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "github.com/diranged/oz/internal/api/v1alpha1" + "github.com/diranged/oz/internal/metrics" ) func (r *RequestReconciler) isAccessExpired( @@ -36,6 +37,9 @@ func (r *RequestReconciler) isAccessExpired( shouldEndReconcile = true result = ctrl.Result{} resultErr = r.Delete(rctx.Context, rctx.obj) + if resultErr == nil { + recordTerminated(rctx, metrics.ReasonExpired) + } } else { rctx.log.V(1).Info( fmt.Sprintf( diff --git a/internal/controllers/requestcontroller/metrics.go b/internal/controllers/requestcontroller/metrics.go new file mode 100644 index 00000000..1d768530 --- /dev/null +++ b/internal/controllers/requestcontroller/metrics.go @@ -0,0 +1,42 @@ +package requestcontroller + +import ( + "github.com/diranged/oz/internal/api/v1alpha1" + "github.com/diranged/oz/internal/metrics" +) + +// metricLabels returns the (kind, namespace, template) label triple shared by +// the access request metrics. Safe to call any time after fetchRequestObject +// has populated rctx.obj. +func (rctx *RequestContext) metricLabels() (kind, namespace, template string) { + return metrics.Kind(rctx.obj), rctx.obj.GetNamespace(), rctx.obj.GetTemplateName() +} + +// recordConditionError counts a reconcile attempt that failed a verification +// step. Called once per failing attempt, so a permanently wedged request keeps +// incrementing this on every reconciliation interval. +func recordConditionError(rctx *RequestContext, condition v1alpha1.IConditionType) { + kind, namespace, template := rctx.metricLabels() + metrics.AccessRequestConditionErrorsTotal. + WithLabelValues(kind, namespace, template, condition.String()). + Inc() +} + +// recordTerminated counts a request reaching a terminal state. See the +// metrics.Reason* constants. +func recordTerminated(rctx *RequestContext, reason string) { + kind, namespace, template := rctx.metricLabels() + metrics.AccessRequestTerminatedTotal. + WithLabelValues(kind, namespace, template, reason). + Inc() +} + +// recordReady observes how long the requesting user waited between creating +// their request and it becoming usable. Callers must only invoke this on the +// transition into the ready state, so that each request is observed once. +func recordReady(rctx *RequestContext) { + kind, namespace, template := rctx.metricLabels() + metrics.AccessRequestReadySeconds. + WithLabelValues(kind, namespace, template). + Observe(rctx.obj.GetUptime().Seconds()) +} diff --git a/internal/controllers/requestcontroller/verify_access_resources.go b/internal/controllers/requestcontroller/verify_access_resources.go index f20a8475..ada2fa90 100644 --- a/internal/controllers/requestcontroller/verify_access_resources.go +++ b/internal/controllers/requestcontroller/verify_access_resources.go @@ -26,6 +26,8 @@ func (r *RequestReconciler) verifyAccessResources( rctx.log.V(1).Info("Making sure Access Resources have been created") if statusStr, err = r.Builder.CreateAccessResources(rctx.Context, r.Client, rctx.obj, tmpl); err != nil { + recordConditionError(rctx, v1alpha1.ConditionAccessResourcesCreated) + // NOTE: Blindly ignoring the error return here because we are already // returning an error which will fail the reconciliation. _ = status.SetAccessResourcesNotCreated(rctx.Context, r, rctx.obj, err) @@ -39,12 +41,17 @@ func (r *RequestReconciler) verifyAccessResources( { // Check if the resources are ready rctx.log.V(1).Info("Checking if Access Resources are ready") if areReady, err := r.Builder.AccessResourcesAreReady(rctx.Context, r.Client, rctx.obj, tmpl); err != nil { + recordConditionError(rctx, v1alpha1.ConditionAccessResourcesReady) + // NOTE: Blindly ignoring the error return here because we are already // returning an error which will fail the reconciliation. _ = status.SetAccessResourcesNotReady(rctx.Context, r, rctx.obj, err) return true, result, err } else if !areReady { + // Not an error - the Pod is simply still coming up. Deliberately + // not counted as a condition error; the wait shows up in + // oz_access_request_ready_seconds instead. interval := r.getVerifyResourcesRequeueInterval() // NOTE: Blindly ignoring the error return here because we are already diff --git a/internal/controllers/requestcontroller/verify_duration.go b/internal/controllers/requestcontroller/verify_duration.go index e4469331..23f917f6 100644 --- a/internal/controllers/requestcontroller/verify_duration.go +++ b/internal/controllers/requestcontroller/verify_duration.go @@ -8,6 +8,7 @@ import ( "github.com/diranged/oz/internal/builders" "github.com/diranged/oz/internal/controllers/internal/ctrlrequeue" "github.com/diranged/oz/internal/controllers/internal/status" + "github.com/diranged/oz/internal/metrics" ctrl "sigs.k8s.io/controller-runtime" ) @@ -25,15 +26,19 @@ func (r *RequestReconciler) verifyDuration( // If an error is returned, determine whether its something wrong with the // user-supplied inputs, or whether it was transient. if err != nil { + recordConditionError(rctx, v1alpha1.ConditionRequestDurationsValid) + switch errors.Unwrap(err) { case builders.ErrRequestDurationInvalid: rctx.log.Error(err, "RequestDurationInvalid, will not requeue.") shouldEndReconcile = true result, resultErr = ctrlrequeue.NoRequeue() + recordTerminated(rctx, metrics.ReasonDurationInvalid) case builders.ErrRequestDurationTooLong: rctx.log.Error(err, "RequestDurationTooLong, will not requeue.") shouldEndReconcile = true result, resultErr = ctrlrequeue.NoRequeue() + recordTerminated(rctx, metrics.ReasonDurationTooLong) default: rctx.log.Error(err, "Unexpected error, will requeue") shouldEndReconcile = true diff --git a/internal/controllers/requestcontroller/verify_template.go b/internal/controllers/requestcontroller/verify_template.go index 5b00c103..aaed4ef6 100644 --- a/internal/controllers/requestcontroller/verify_template.go +++ b/internal/controllers/requestcontroller/verify_template.go @@ -17,6 +17,7 @@ func (r *RequestReconciler) verifyTemplate( tmpl, err := r.Builder.GetTemplate(rctx.Context, r.Client, rctx.obj) if err != nil { rctx.log.Error(err, "Unable to verify template") + recordConditionError(rctx, v1alpha1.ConditionTargetTemplateExists) // Update the condition. If that fails, return the error, otherwise // return nil which continues reconciliation. diff --git a/internal/metrics/collector/collector.go b/internal/metrics/collector/collector.go new file mode 100644 index 00000000..c94c609a --- /dev/null +++ b/internal/metrics/collector/collector.go @@ -0,0 +1,197 @@ +// Package collector implements a Prometheus collector that reports the current +// inventory of Oz resources in the cluster. +// +// Counters and histograms live in the parent metrics package, because they are +// incremented from the code path that the event happens on. Point-in-time +// gauges ("how many access requests are live right now, for which template, for +// whom") cannot be maintained that way: a controller never reliably observes the +// disappearance of a resource, so an incrementally-maintained GaugeVec +// accumulates stale label sets forever. +// +// Instead this collector lists the resources from the manager's informer cache +// at scrape time. The cache is already populated by the controllers' Watches, so +// this costs no API calls, and the reported series cannot go stale. +// +// This package is separate from the parent metrics package to avoid an import +// cycle: the v1alpha1 API package imports metrics from its webhooks, and this +// collector imports the v1alpha1 API package for its types. +package collector + +import ( + "context" + "strconv" + "time" + + "github.com/prometheus/client_golang/prometheus" + apimeta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" + + "github.com/diranged/oz/internal/api/v1alpha1" + "github.com/diranged/oz/internal/metrics" +) + +// collectTimeout bounds how long a single scrape may spend listing resources. +// Reads are served from the informer cache so they should be near-instant; this +// exists only so that a wedged cache degrades the scrape rather than hanging it. +const collectTimeout = 10 * time.Second + +var ( + requestsDesc = prometheus.NewDesc( + "oz_access_requests", + "Number of Oz access requests that currently exist, by kind, namespace, template, requesting user and readiness.", + []string{"kind", "namespace", "template", "user", "ready"}, + nil, + ) + + templatesDesc = prometheus.NewDesc( + "oz_access_templates", + "Number of Oz access templates that currently exist, by kind, namespace, name and readiness. Join against oz_access_requests to find templates nobody is using.", + []string{"kind", "namespace", "name", "ready"}, + nil, + ) +) + +// Collector reports the current inventory of Oz access requests and templates. +type Collector struct { + reader client.Reader +} + +var _ prometheus.Collector = &Collector{} + +// MustRegister builds a Collector backed by the supplied reader (normally +// mgr.GetClient(), so that reads hit the informer cache) and registers it +// against the controller-runtime metrics registry. +func MustRegister(reader client.Reader) { + crmetrics.Registry.MustRegister(&Collector{reader: reader}) +} + +// Describe implements prometheus.Collector. +func (c *Collector) Describe(ch chan<- *prometheus.Desc) { + ch <- requestsDesc + ch <- templatesDesc +} + +// Collect implements prometheus.Collector. +// +// Any listing failure is reported as an invalid metric, which surfaces to +// Prometheus as a scrape error rather than as a silent zero. This is the +// expected behavior if a scrape somehow arrives before the informer cache has +// synced. +func (c *Collector) Collect(ch chan<- prometheus.Metric) { + ctx, cancel := context.WithTimeout(context.Background(), collectTimeout) + defer cancel() + + c.collectRequests(ctx, ch, + &v1alpha1.PodAccessRequestList{}, + &v1alpha1.ExecAccessRequestList{}, + ) + c.collectTemplates(ctx, ch, + &v1alpha1.PodAccessTemplateList{}, + &v1alpha1.ExecAccessTemplateList{}, + ) +} + +// requestKey is the label set for the oz_access_requests gauge. +type requestKey struct { + kind string + namespace string + template string + user string + ready string +} + +func (c *Collector) collectRequests( + ctx context.Context, + ch chan<- prometheus.Metric, + lists ...client.ObjectList, +) { + counts := map[requestKey]int{} + + for _, list := range lists { + items, err := c.list(ctx, list) + if err != nil { + ch <- prometheus.NewInvalidMetric(requestsDesc, err) + return + } + for _, item := range items { + // Every request kind satisfies IRequestResource, which is where + // the template name lives. Anything that does not is not something + // we know how to report on. + req, ok := item.(v1alpha1.IRequestResource) + if !ok { + continue + } + counts[requestKey{ + kind: metrics.Kind(req), + namespace: req.GetNamespace(), + template: req.GetTemplateName(), + user: metrics.UserLabel(req.GetAnnotations()[v1alpha1.AnnotationRequestedBy]), + ready: strconv.FormatBool(req.GetStatus().IsReady()), + }]++ + } + } + + for key, count := range counts { + ch <- prometheus.MustNewConstMetric( + requestsDesc, + prometheus.GaugeValue, + float64(count), + key.kind, key.namespace, key.template, key.user, key.ready, + ) + } +} + +func (c *Collector) collectTemplates( + ctx context.Context, + ch chan<- prometheus.Metric, + lists ...client.ObjectList, +) { + // Templates are named and few, so unlike requests each one gets its own + // series rather than being aggregated into a count. They are still buffered + // until every list has succeeded, so that a failure partway through fails + // the scrape outright instead of reporting a subset as though it were + // everything. + templates := []v1alpha1.ITemplateResource{} + + for _, list := range lists { + items, err := c.list(ctx, list) + if err != nil { + ch <- prometheus.NewInvalidMetric(templatesDesc, err) + return + } + for _, item := range items { + tmpl, ok := item.(v1alpha1.ITemplateResource) + if !ok { + continue + } + templates = append(templates, tmpl) + } + } + + for _, tmpl := range templates { + ch <- prometheus.MustNewConstMetric( + templatesDesc, + prometheus.GaugeValue, + 1, + metrics.Kind(tmpl), + tmpl.GetNamespace(), + tmpl.GetName(), + strconv.FormatBool(tmpl.GetStatus().IsReady()), + ) + } +} + +// list fetches a resource list and flattens it into the individual items. +// ExtractList takes the address of each item, so the returned objects satisfy +// the pointer-receiver Oz resource interfaces. +func (c *Collector) list( + ctx context.Context, + list client.ObjectList, +) ([]runtime.Object, error) { + if err := c.reader.List(ctx, list); err != nil { + return nil, err + } + return apimeta.ExtractList(list) +} diff --git a/internal/metrics/collector/collector_test.go b/internal/metrics/collector/collector_test.go new file mode 100644 index 00000000..413a1836 --- /dev/null +++ b/internal/metrics/collector/collector_test.go @@ -0,0 +1,232 @@ +package collector + +import ( + "context" + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/prometheus/client_golang/prometheus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/diranged/oz/internal/api/v1alpha1" + "github.com/diranged/oz/internal/metrics" +) + +// erroringReader stands in for a client whose informer cache has not synced. +type erroringReader struct { + client.Reader +} + +func (erroringReader) List(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error { + return errors.New("the cache is not started, can not read objects") +} + +// series is one gathered sample, flattened into something convenient to assert +// against. +type series struct { + Name string + Labels map[string]string + Value float64 +} + +// collect performs a scrape of a Collector backed by the supplied reader, +// against an isolated registry so that these assertions are unaffected by the +// process-wide controller-runtime registry. +func collect(reader client.Reader) []series { + registry := prometheus.NewPedanticRegistry() + ExpectWithOffset(1, registry.Register(&Collector{reader: reader})).To(Succeed()) + + families, err := registry.Gather() + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + gathered := []series{} + for _, family := range families { + for _, metric := range family.GetMetric() { + labels := map[string]string{} + for _, label := range metric.GetLabel() { + labels[label.GetName()] = label.GetValue() + } + gathered = append(gathered, series{ + Name: family.GetName(), + Labels: labels, + Value: metric.GetGauge().GetValue(), + }) + } + } + return gathered +} + +// podRequest builds a PodAccessRequest as the collector would find it in the +// cluster - already stamped with a requester and with a settled readiness. +func podRequest(name, namespace, template, user string, ready bool) *v1alpha1.PodAccessRequest { + req := &v1alpha1.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{v1alpha1.AnnotationRequestedBy: user}, + }, + Spec: v1alpha1.PodAccessRequestSpec{TemplateName: template}, + } + req.Status.SetReady(ready) + return req +} + +func readerWith(objects ...client.Object) client.Reader { + scheme := runtime.NewScheme() + Expect(clientgoscheme.AddToScheme(scheme)).To(Succeed()) + Expect(v1alpha1.AddToScheme(scheme)).To(Succeed()) + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() +} + +var _ = Describe("Collector", func() { + AfterEach(func() { + metrics.SetIncludeUserLabel(false) + }) + + Context("with access requests in the cluster", func() { + It("aggregates requests that share a label set", func() { + metrics.SetIncludeUserLabel(true) + + gathered := collect(readerWith( + podRequest("a", "prod", "debug-web", "dennis", true), + podRequest("b", "prod", "debug-web", "dennis", true), + )) + + Expect(gathered).To(ConsistOf(series{ + Name: "oz_access_requests", + Labels: map[string]string{ + "kind": "PodAccessRequest", + "namespace": "prod", + "template": "debug-web", + "user": "dennis", + "ready": "true", + }, + Value: 2, + })) + }) + + It("separates requests by template, user and readiness", func() { + metrics.SetIncludeUserLabel(true) + + gathered := collect(readerWith( + podRequest("a", "prod", "debug-web", "dennis", true), + podRequest("b", "prod", "debug-web", "dennis", false), + podRequest("c", "prod", "debug-api", "dennis", true), + podRequest("d", "prod", "debug-web", "somebody-else", true), + )) + + Expect(gathered).To(HaveLen(4)) + for _, sample := range gathered { + Expect(sample.Value).To(Equal(float64(1))) + } + }) + + It("redacts the user label unless it has been enabled", func() { + gathered := collect(readerWith( + podRequest("a", "prod", "debug-web", "dennis", true), + )) + + Expect(gathered).To(HaveLen(1)) + Expect(gathered[0].Labels).To(HaveKeyWithValue("user", metrics.UserRedacted)) + }) + + It("collapses distinct users into one series when redacting", func() { + gathered := collect(readerWith( + podRequest("a", "prod", "debug-web", "dennis", true), + podRequest("b", "prod", "debug-web", "somebody-else", true), + )) + + Expect(gathered).To(HaveLen(1)) + Expect(gathered[0].Value).To(Equal(float64(2))) + }) + + It("labels each kind of request distinctly", func() { + gathered := collect(readerWith( + podRequest("a", "prod", "debug-web", "dennis", true), + &v1alpha1.ExecAccessRequest{ + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "prod"}, + Spec: v1alpha1.ExecAccessRequestSpec{TemplateName: "exec-web"}, + }, + )) + + kinds := []string{} + for _, sample := range gathered { + kinds = append(kinds, sample.Labels["kind"]) + } + Expect(kinds).To(ConsistOf("PodAccessRequest", "ExecAccessRequest")) + }) + + It("reports requests that were never stamped with a requester", func() { + metrics.SetIncludeUserLabel(true) + + gathered := collect(readerWith(&v1alpha1.PodAccessRequest{ + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "prod"}, + Spec: v1alpha1.PodAccessRequestSpec{TemplateName: "debug-web"}, + })) + + Expect(gathered).To(HaveLen(1)) + Expect(gathered[0].Labels).To(HaveKeyWithValue("user", metrics.UserUnknown)) + }) + }) + + Context("with access templates in the cluster", func() { + It("emits one series per template, named", func() { + gathered := collect(readerWith( + &v1alpha1.PodAccessTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "debug-web", Namespace: "prod"}, + }, + &v1alpha1.ExecAccessTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "exec-web", Namespace: "prod"}, + }, + )) + + Expect(gathered).To(ConsistOf( + series{ + Name: "oz_access_templates", + Labels: map[string]string{ + "kind": "PodAccessTemplate", + "namespace": "prod", + "name": "debug-web", + "ready": "false", + }, + Value: 1, + }, + series{ + Name: "oz_access_templates", + Labels: map[string]string{ + "kind": "ExecAccessTemplate", + "namespace": "prod", + "name": "exec-web", + "ready": "false", + }, + Value: 1, + }, + )) + }) + }) + + Context("with an empty cluster", func() { + It("reports no series rather than failing", func() { + Expect(collect(readerWith())).To(BeEmpty()) + }) + }) + + Context("when the cache cannot be read", func() { + // A silent zero here would look identical to "nobody is using Oz", + // which is exactly the wrong conclusion to let somebody draw. + It("surfaces the failure as a scrape error", func() { + registry := prometheus.NewPedanticRegistry() + Expect(registry.Register(&Collector{reader: erroringReader{}})).To(Succeed()) + + _, err := registry.Gather() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cache is not started")) + }) + }) +}) diff --git a/internal/metrics/collector/suite_test.go b/internal/metrics/collector/suite_test.go new file mode 100644 index 00000000..7af03ca3 --- /dev/null +++ b/internal/metrics/collector/suite_test.go @@ -0,0 +1,13 @@ +package collector + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestCollector(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Metrics Collector Suite") +} diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go new file mode 100644 index 00000000..563753e5 --- /dev/null +++ b/internal/metrics/metrics.go @@ -0,0 +1,217 @@ +// Package metrics defines the custom Prometheus metrics that Oz exports +// alongside the standard controller-runtime metrics (reconcile counts, webhook +// latencies, workqueue depths, etc). +// +// The controller-runtime metrics tell you how hard the operator is working. The +// metrics in this package tell you how the operator is being *used*: who is +// requesting access, which templates they are requesting it through, how long +// those grants take to become usable, and how much exec/attach traffic the +// resulting Pods actually see. +// +// All metrics here are registered against the controller-runtime metrics +// registry, so they are served on the existing (authenticated) metrics endpoint +// without any extra wiring. +// +// # A note on the `user` label +// +// Attributing usage to a person is the whole point of several of these metrics, +// but usernames are unbounded-ish cardinality and, depending on your +// authenticator, are personally identifying (OIDC usernames are frequently +// email addresses). The `user` label is therefore opt-in: unless +// SetIncludeUserLabel(true) is called, every user label is reported as +// "redacted" so the series collapses to one per template. See the +// --metrics-include-user-label flag. +package metrics + +import ( + "reflect" + "sync/atomic" + + "github.com/prometheus/client_golang/prometheus" + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +const ( + // UserRedacted is reported in place of a real username when the `user` + // label has not been explicitly enabled. + UserRedacted = "redacted" + + // UserUnknown is reported when the Kubernetes API server did not supply + // any user identity on the admission request. This should not happen in a + // healthy cluster, and a non-trivial rate of it is worth alerting on. + UserUnknown = "unknown" +) + +// includeUserLabel gates whether real usernames are attached to metrics. It is +// read on every metric observation from webhook handlers, which run +// concurrently, hence the atomic. +var includeUserLabel atomic.Bool + +// SetIncludeUserLabel enables or disables reporting of real usernames in the +// `user` label of the metrics in this package. It is expected to be called once +// during startup, before the manager begins serving webhooks. +func SetIncludeUserLabel(enabled bool) { + includeUserLabel.Store(enabled) +} + +// UserLabel maps a Kubernetes username onto a value safe to use as a metric +// label, honoring SetIncludeUserLabel. +// +// Note that a genuine user literally named "redacted" or "unknown" would be +// indistinguishable from the placeholders. This is not considered a problem in +// practice. +func UserLabel(username string) string { + if !includeUserLabel.Load() { + return UserRedacted + } + if username == "" { + return UserUnknown + } + return username +} + +// Kind returns the Kubernetes Kind for an Oz resource (eg +// "PodAccessRequest"), for use as a metric label. +// +// The concrete Go type name and the Kind are the same for every type in the +// v1alpha1 API, which lets callers label a metric without needing a runtime +// Scheme to do a GVK lookup. +func Kind(obj any) string { + t := reflect.TypeOf(obj) + for t != nil && t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t == nil { + return "Unknown" + } + return t.Name() +} + +// Reasons reported in the `reason` label of AccessRequestTerminatedTotal. +const ( + // ReasonExpired means the request outlived its access duration and was + // deleted by the reconciler. This is the normal, happy-path ending for an + // access request. + ReasonExpired = "expired" + + // ReasonDurationInvalid means the request asked for a duration that could + // not be parsed, and was abandoned without being retried. + ReasonDurationInvalid = "duration_invalid" + + // ReasonDurationTooLong means the request asked for more time than its + // template allows, and was abandoned without being retried. + ReasonDurationTooLong = "duration_too_long" +) + +var ( + // AccessRequestCreatedTotal counts access requests at the moment they are + // admitted by the validating webhook. This is the headline "who is using + // Oz, and through which template" metric. + // + // It counts admitted *attempts*: a request that is admitted here but then + // rejected by a later admission plugin, or that fails to reconcile, is + // still counted. Compare against AccessRequestReadySeconds's count to see + // how many requests actually turned into working access. + AccessRequestCreatedTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "oz_access_request_created_total", + Help: "Total number of Oz access requests admitted by the validating webhook, by kind, namespace, template and requesting user.", + }, + []string{"kind", "namespace", "template", "user"}, + ) + + // AccessRequestRequestedDurationSeconds observes the duration a user + // explicitly asked for on creation. + // + // Requests that omit spec.duration (and therefore inherit the template's + // default) are NOT observed here, because the webhook has no client with + // which to resolve the template. A large gap between this metric's count + // and AccessRequestCreatedTotal means most users are taking the default. + AccessRequestRequestedDurationSeconds = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "oz_access_request_requested_duration_seconds", + Help: "Access duration explicitly requested in spec.duration, in seconds. Requests that omit spec.duration and inherit the template default are not observed.", + Buckets: []float64{ + 900, // 15m + 1800, // 30m + 3600, // 1h + 7200, // 2h + 14400, // 4h + 28800, // 8h + 43200, // 12h + 86400, // 24h + 172800, // 48h + }, + }, + []string{"kind", "namespace", "template"}, + ) + + // AccessRequestReadySeconds observes how long it took a request to go from + // creation to Status.Ready, ie how long the user waited before they could + // actually use their access. For PodAccessRequests this includes cloning + // the target workload's PodSpec and waiting for the new Pod to start, so + // it is the metric to watch if users complain that Oz is slow. + // + // Observed once per request, on the transition to ready. + AccessRequestReadySeconds = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "oz_access_request_ready_seconds", + Help: "Time from access request creation to Status.Ready becoming true, in seconds.", + Buckets: prometheus.ExponentialBuckets(1, 2, 10), + }, + []string{"kind", "namespace", "template"}, + ) + + // AccessRequestTerminatedTotal counts requests that reached a terminal + // state, labelled by why. See the Reason* constants; `expired` is the + // happy path, everything else is a user or configuration error. + AccessRequestTerminatedTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "oz_access_request_terminated_total", + Help: "Total number of Oz access requests that reached a terminal state, by reason (expired, duration_invalid, duration_too_long).", + }, + []string{"kind", "namespace", "template", "reason"}, + ) + + // AccessRequestConditionErrorsTotal counts reconcile attempts that failed + // a verification step, labelled with the Status.Condition that was set to + // False. + // + // This is per *attempt*, not per request: a request wedged on a missing + // template increments this on every reconciliation interval. That is + // deliberate - it turns an otherwise silent requeue loop into a rate you + // can alert on. + AccessRequestConditionErrorsTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "oz_access_request_condition_errors_total", + Help: "Total number of access request reconcile attempts that failed a verification step, by the Status.Condition set to False.", + }, + []string{"kind", "namespace", "template", "condition"}, + ) + + // PodExecTotal counts exec and attach operations seen by the Oz pod + // watcher webhook. + // + // This is the "did the access actually get used" counter. Note that the + // webhook is registered for pods/exec and pods/attach cluster-wide, not + // just for Oz-managed Pods, so this metric covers all exec traffic in the + // cluster. The Pod name is deliberately not a label - it is unbounded. + PodExecTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "oz_pod_exec_total", + Help: "Total number of Pod exec/attach operations observed by the Oz pod watcher, by namespace, subresource, user and whether a TTY was requested.", + }, + []string{"namespace", "subresource", "user", "interactive"}, + ) +) + +func init() { + metrics.Registry.MustRegister( + AccessRequestCreatedTotal, + AccessRequestRequestedDurationSeconds, + AccessRequestReadySeconds, + AccessRequestTerminatedTotal, + AccessRequestConditionErrorsTotal, + PodExecTotal, + ) +} diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go new file mode 100644 index 00000000..55147543 --- /dev/null +++ b/internal/metrics/metrics_test.go @@ -0,0 +1,77 @@ +package metrics + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/prometheus/client_golang/prometheus/testutil" +) + +var _ = Describe("Metrics", func() { + // The user label gate is global state, so every test that flips it must put + // it back afterwards. + AfterEach(func() { + SetIncludeUserLabel(false) + }) + + Context("UserLabel", func() { + It("redacts usernames by default", func() { + Expect(UserLabel("dennis@example.com")).To(Equal(UserRedacted)) + }) + + It("redacts an empty username too, rather than leaking the distinction", func() { + Expect(UserLabel("")).To(Equal(UserRedacted)) + }) + + It("returns the real username once enabled", func() { + SetIncludeUserLabel(true) + Expect(UserLabel("dennis@example.com")).To(Equal("dennis@example.com")) + }) + + It("reports a missing identity as unknown once enabled", func() { + SetIncludeUserLabel(true) + Expect(UserLabel("")).To(Equal(UserUnknown)) + }) + }) + + Context("Kind", func() { + type somethingElse struct{} + + It("returns the type name for a pointer", func() { + Expect(Kind(&somethingElse{})).To(Equal("somethingElse")) + }) + + It("returns the type name for a value", func() { + Expect(Kind(somethingElse{})).To(Equal("somethingElse")) + }) + + It("handles a nil interface without panicking", func() { + Expect(Kind(nil)).To(Equal("Unknown")) + }) + }) + + Context("Registration", func() { + // A duplicate metric name, or a Help string that disagrees between two + // registrations, would panic at init() time. Confirm the metrics are + // actually usable and land where we expect. + It("registers the metrics against the controller-runtime registry", func() { + AccessRequestCreatedTotal. + WithLabelValues("PodAccessRequest", "ns", "tmpl", UserRedacted). + Inc() + + Expect(testutil.ToFloat64( + AccessRequestCreatedTotal.WithLabelValues( + "PodAccessRequest", "ns", "tmpl", UserRedacted, + ), + )).To(BeNumerically(">=", 1)) + }) + + It("exposes a histogram per template for grant latency", func() { + AccessRequestReadySeconds. + WithLabelValues("PodAccessRequest", "ns", "tmpl"). + Observe(12) + + Expect(testutil.CollectAndCount(AccessRequestReadySeconds)).To(BeNumerically(">=", 1)) + }) + }) +}) diff --git a/internal/metrics/suite_test.go b/internal/metrics/suite_test.go new file mode 100644 index 00000000..e8917cb4 --- /dev/null +++ b/internal/metrics/suite_test.go @@ -0,0 +1,13 @@ +package metrics + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMetrics(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Metrics Suite") +}