From 9cc59c38aa1ee9235a88568b28257951f33235bd Mon Sep 17 00:00:00 2001
From: mattb18 <mattb18@users.noreply.github.com>
Date: Tue, 31 Dec 2024 18:26:19 +0000
Subject: [PATCH] Only register grpc TLS metrics on successful handshake
 (#1338)

In the current gRPC implementation, TLS metrics are registered before the handshake is complete. In cases where the probe fails, probeSSLEarliestCertExpiryGauge is set to the default time.Time value (far in the past), resulting in any alerts using probe_ssl_earliest_cert_expiry metric firing.

In the case of the TLS handshake failing, it makes more sense for TLS metrics not to be set. This is the way the http probe is currently configured. This should also solve https://github.com/prometheus/blackbox_exporter/issues/1120.

* only register grpc TLS metrics on success

* adds tests

---------

Signed-off-by: mattb18 <mattb18@users.noreply.github.com>
---
 prober/grpc.go       |  4 +---
 prober/grpc_test.go  | 31 +++++++++++++++++++++++++++++++
 prober/utils_test.go | 10 ++++++++++
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/prober/grpc.go b/prober/grpc.go
index 64c54dec7..cd43c7118 100644
--- a/prober/grpc.go
+++ b/prober/grpc.go
@@ -121,9 +121,6 @@ func ProbeGRPC(ctx context.Context, target string, module config.Module, registr
 	registry.MustRegister(isSSLGauge)
 	registry.MustRegister(statusCodeGauge)
 	registry.MustRegister(healthCheckResponseGaugeVec)
-	registry.MustRegister(probeSSLEarliestCertExpiryGauge)
-	registry.MustRegister(probeTLSVersion)
-	registry.MustRegister(probeSSLLastInformation)
 
 	if !strings.HasPrefix(target, "http://") && !strings.HasPrefix(target, "https://") {
 		target = "http://" + target
@@ -203,6 +200,7 @@ func ProbeGRPC(ctx context.Context, target string, module config.Module, registr
 	if serverPeer != nil {
 		tlsInfo, tlsOk := serverPeer.AuthInfo.(credentials.TLSInfo)
 		if tlsOk {
+			registry.MustRegister(probeSSLEarliestCertExpiryGauge, probeTLSVersion, probeSSLLastInformation)
 			isSSLGauge.Set(float64(1))
 			probeSSLEarliestCertExpiryGauge.Set(float64(getEarliestCertExpiry(&tlsInfo.State).Unix()))
 			probeTLSVersion.WithLabelValues(getTLSVersion(&tlsInfo.State)).Set(1)
diff --git a/prober/grpc_test.go b/prober/grpc_test.go
index 4185ccc17..2a2c4aeb6 100644
--- a/prober/grpc_test.go
+++ b/prober/grpc_test.go
@@ -432,3 +432,34 @@ func TestGRPCHealthCheckUnimplemented(t *testing.T) {
 
 	checkRegistryResults(expectedResults, mfs, t)
 }
+
+func TestGRPCAbsentFailedTLS(t *testing.T) {
+	testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	registry := prometheus.NewRegistry()
+
+	// probe and invalid port to trigger TCP/TLS error
+	result := ProbeGRPC(testCTX, "localhost:0",
+		config.Module{Timeout: time.Second, GRPC: config.GRPCProbe{
+			IPProtocolFallback: false,
+			Service:            "NonExistingService",
+		},
+		}, registry, promslog.NewNopLogger())
+
+	if result {
+		t.Fatalf("GRPC probe succeeded, should have failed")
+	}
+
+	mfs, err := registry.Gather()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	absentMetrics := []string{
+		"probe_ssl_earliest_cert_expiry",
+		"probe_tls_version_info",
+		"probe_ssl_last_chain_info",
+	}
+
+	checkAbsentMetrics(absentMetrics, mfs, t)
+}
diff --git a/prober/utils_test.go b/prober/utils_test.go
index c439790e9..cbc212108 100644
--- a/prober/utils_test.go
+++ b/prober/utils_test.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"math/big"
 	"net"
+	"slices"
 	"testing"
 	"time"
 
@@ -249,3 +250,12 @@ func checkMetrics(expected map[string]map[string]map[string]struct{}, mfs []*dto
 		}
 	}
 }
+
+func checkAbsentMetrics(absent []string, mfs []*dto.MetricFamily, t *testing.T) {
+	for _, v := range mfs {
+		name := v.GetName()
+		if slices.Contains(absent, name) {
+			t.Fatalf("metric %s was found but should be absent", name)
+		}
+	}
+}