From 0101b166b40940be0471e6f2432338457a7b54c2 Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Thu, 30 Mar 2023 14:02:09 -0700 Subject: [PATCH] `runsc metric-server`: Check that sandboxes don't export reserved metric names This change prevents sandboxes from exporting metrics that match the names reserved for process-level Prometheus metrics: https://prometheus.io/docs/instrumenting/writing_clientlibs/#process-metrics PiperOrigin-RevId: 520736834 --- pkg/prometheus/prometheus_test.go | 11 ++++ pkg/prometheus/prometheus_verify.go | 87 ++++++++++++++++++++++++++--- runsc/cmd/metric_server.go | 9 +-- 3 files changed, 92 insertions(+), 15 deletions(-) diff --git a/pkg/prometheus/prometheus_test.go b/pkg/prometheus/prometheus_test.go index 49e9bc0cc..8ae6b57fb 100644 --- a/pkg/prometheus/prometheus_test.go +++ b/pkg/prometheus/prometheus_test.go @@ -386,6 +386,17 @@ func TestVerifier(t *testing.T) { ), WantVerifierCreationErr: false, }, + { + Name: "Prometheus metric name matches reserved one", + Registration: newMetricRegistration(&metricMetadata{ + PB: &pb.MetricMetadata{ + Name: "doesNotMatter", + PrometheusName: ProcessStartTimeSeconds.Name, + Type: pb.MetricMetadata_TYPE_UINT64, + }}, + ), + WantVerifierCreationErr: true, + }, { Name: "no buckets", Registration: newMetricRegistration(&metricMetadata{ diff --git a/pkg/prometheus/prometheus_verify.go b/pkg/prometheus/prometheus_verify.go index 78204487f..881c8a00d 100644 --- a/pkg/prometheus/prometheus_verify.go +++ b/pkg/prometheus/prometheus_verify.go @@ -37,6 +37,71 @@ const ( MetaMetricPrefix = "meta_" ) +// Prometheus process-level metric names and definitions. +// These are not necessarily exported, but we enforce that sandboxes may not +// export metrics sharing the same names. +// https://prometheus.io/docs/instrumenting/writing_clientlibs/#process-metrics +var ( + ProcessCPUSecondsTotal = Metric{ + Name: "process_cpu_seconds_total", + Type: TypeGauge, + Help: "Total user and system CPU time spent in seconds.", + } + ProcessOpenFDs = Metric{ + Name: "process_open_fds", + Type: TypeGauge, + Help: "Number of open file descriptors.", + } + ProcessMaxFDs = Metric{ + Name: "process_max_fds", + Type: TypeGauge, + Help: "Maximum number of open file descriptors.", + } + ProcessVirtualMemoryBytes = Metric{ + Name: "process_virtual_memory_bytes", + Type: TypeGauge, + Help: "Virtual memory size in bytes.", + } + ProcessVirtualMemoryMaxBytes = Metric{ + Name: "process_virtual_memory_max_bytes", + Type: TypeGauge, + Help: "Maximum amount of virtual memory available in bytes.", + } + ProcessResidentMemoryBytes = Metric{ + Name: "process_resident_memory_bytes", + Type: TypeGauge, + Help: "Resident memory size in bytes.", + } + ProcessHeapBytes = Metric{ + Name: "process_heap_bytes", + Type: TypeGauge, + Help: "Process heap size in bytes.", + } + ProcessStartTimeSeconds = Metric{ + Name: "process_start_time_seconds", + Type: TypeGauge, + Help: "Start time of the process since unix epoch in seconds.", + } + ProcessThreads = Metric{ + Name: "process_threads", + Type: TypeGauge, + Help: "Number of OS threads in the process.", + } +) + +// processMetrics is the set of process-level metrics. +var processMetrics = [9]*Metric{ + &ProcessCPUSecondsTotal, + &ProcessOpenFDs, + &ProcessMaxFDs, + &ProcessVirtualMemoryBytes, + &ProcessVirtualMemoryMaxBytes, + &ProcessResidentMemoryBytes, + &ProcessHeapBytes, + &ProcessStartTimeSeconds, + &ProcessThreads, +} + // internedStringMap allows for interning strings. type internedStringMap map[string]*string @@ -262,18 +327,24 @@ type verifiableMetric struct { // newVerifiableMetric creates a new verifiableMetric that can verify the // values of a metric with the given metadata. func newVerifiableMetric(metadata *pb.MetricMetadata, verifier *Verifier) (*verifiableMetric, error) { - if metadata.GetName() == "" || metadata.GetPrometheusName() == "" { + promName := metadata.GetPrometheusName() + if metadata.GetName() == "" || promName == "" { return nil, errors.New("metric has no name") } - if strings.HasPrefix(metadata.GetPrometheusName(), MetaMetricPrefix) { - return nil, fmt.Errorf("metric name %q starts with %q which is a reserved prefix", metadata.GetPrometheusName(), "meta_") + for _, processMetric := range processMetrics { + if promName == processMetric.Name { + return nil, fmt.Errorf("metric name %q is reserved by Prometheus for process-level metrics", promName) + } } - if !unicode.IsLower(rune(metadata.GetPrometheusName()[0])) { - return nil, fmt.Errorf("invalid initial character in prometheus metric name: %q", metadata.GetPrometheusName()) + if strings.HasPrefix(promName, MetaMetricPrefix) { + return nil, fmt.Errorf("metric name %q starts with %q which is a reserved prefix", promName, "meta_") } - for _, r := range metadata.GetPrometheusName() { + if !unicode.IsLower(rune(promName[0])) { + return nil, fmt.Errorf("invalid initial character in prometheus metric name: %q", promName) + } + for _, r := range promName { if !unicode.IsLower(r) && !unicode.IsDigit(r) && r != '_' { - return nil, fmt.Errorf("invalid character %c in prometheus metric name %q", r, metadata.GetPrometheusName()) + return nil, fmt.Errorf("invalid character %c in prometheus metric name %q", r, promName) } } numFields := uint32(len(metadata.GetFields())) @@ -304,7 +375,7 @@ func newVerifiableMetric(metadata *pb.MetricMetadata, verifier *Verifier) (*veri metadata: metadata, verifier: verifier, wantMetric: Metric{ - Name: globalIntern(metadata.GetPrometheusName()), + Name: globalIntern(promName), Help: globalIntern(metadata.GetDescription()), }, numFields: numFields, diff --git a/runsc/cmd/metric_server.go b/runsc/cmd/metric_server.go index 7512775b2..955a9cb1e 100644 --- a/runsc/cmd/metric_server.go +++ b/runsc/cmd/metric_server.go @@ -530,11 +530,6 @@ var ( Type: prometheus.TypeCounter, Help: "Counter of sandboxes that have ever been started.", } - ProcessStartTimeMetric = prometheus.Metric{ - Name: "process_start_time_seconds", - Type: prometheus.TypeGauge, - Help: "Unix timestamp at which the process started. Used by Prometheus for counter resets.", - } ) // ServerMetrics is a list of metrics that the metric server generates. @@ -546,7 +541,7 @@ var ServerMetrics = []prometheus.Metric{ NumRunningSandboxesMetric, NumCannotExportSandboxesMetric, NumTotalSandboxesMetric, - ProcessStartTimeMetric, + prometheus.ProcessStartTimeSeconds, } // serveMetrics serves metrics requests. @@ -704,7 +699,7 @@ func (m *MetricServer) serveMetrics(w http.ResponseWriter, req *http.Request) ht ExporterPrefix: fmt.Sprintf("%s%s", m.exporterPrefix, prometheus.MetaMetricPrefix), } processMetrics := prometheus.NewSnapshot() - processMetrics.Add(prometheus.NewFloatData(&ProcessStartTimeMetric, float64(m.startTime.Unix())+(float64(m.startTime.Nanosecond())/1e9))) + processMetrics.Add(prometheus.NewFloatData(&prometheus.ProcessStartTimeSeconds, float64(m.startTime.Unix())+(float64(m.startTime.Nanosecond())/1e9))) snapshotsToOptions[processMetrics] = prometheus.SnapshotExportOptions{ // These metrics must be written without any prefix. }