fix: address review comments on telemetry TLS and endpoint config

Assisted-by: Claude <claude@anthropic.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>

Author: bkhizgiy

controller/cmd/telemetry/main.go

@@ -91,14 +91,16 @@ func main() {
 		Signer:   signer,
 	}
 
+	// Register signal handler before starting the service so no signal
+	// is missed in the window between goroutine start and Notify.
+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
+
 	errCh := make(chan error, 1)
 	go func() {
 		errCh <- svc.Start(ctx)
 	}()
 
-	sigs := make(chan os.Signal, 1)
-	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
-
 	select {
 	case sig := <-sigs:
 		logger.Info("received signal, shutting down", "signal", sig)

controller/internal/service/controller_service.go

@@ -330,6 +330,9 @@ func (s *ControllerService) GetServiceEndpoints(
 		// Prefer the explicit ConfigMap endpoint; fall back to GRPC_TELEMETRY_ENDPOINT
 		// so the operator can pass the address via env var without touching the ConfigMap.
 		ep := cmp.Or(s.TelemetryConfig.Endpoint, telemetryEndpoint())
+		if ep == "" {
+			return nil, status.Error(codes.Internal, "telemetry is enabled but no endpoint is configured; set telemetry.endpoint in the ConfigMap or GRPC_TELEMETRY_ENDPOINT on the controller pod")
+		}
 		resp.TelemetryEndpoints = append(resp.TelemetryEndpoints, &pb.TelemetryEndpoint{
 			Endpoint:    ep,
 			Certificate: s.TelemetryConfig.Certificate,

controller/internal/service/endpoints.go

@@ -22,7 +22,14 @@ func routerEndpoint() string {
 }
 
 func telemetryEndpoint() string {
-	return os.Getenv("GRPC_TELEMETRY_ENDPOINT")
+	ep := os.Getenv("GRPC_TELEMETRY_ENDPOINT")
+	if ep == "" {
+		return ""
+	}
+	if _, _, err := net.SplitHostPort(ep); err != nil {
+		return ""
+	}
+	return ep
 }
 
 func endpointToSAN(endpoint string) ([]string, []net.IP, error) {

controller/internal/service/telemetry_service.go

@@ -257,7 +257,10 @@ func (s *TelemetryService) loadTLSCredentials() (creds credentials.TransportCred
 			Bytes: cert.Certificate[0],
 		}))
 	}
-	return credentials.NewServerTLSFromCert(cert), selfSignedPEM, nil
+	return credentials.NewTLS(&tls.Config{
+		Certificates: []tls.Certificate{*cert},
+		MinVersion:   tls.VersionTLS12,
+	}), selfSignedPEM, nil
 }
 
 // Start starts the TelemetryService gRPC server and blocks until ctx is cancelled.
@@ -280,6 +283,7 @@ func (s *TelemetryService) Start(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("telemetry: listen %s: %w", s.BindAddr, err)
 	}
+	defer func() { _ = lis.Close() }()
 
 	srv := grpc.NewServer(grpc.Creds(creds))
 	pb.RegisterTelemetryServiceServer(srv, s)
@@ -300,6 +304,7 @@ func (s *TelemetryService) Start(ctx context.Context) error {
 		}
 		return nil
 	case err := <-errCh:
+		srv.Stop()
 		return err
 	}
 }

controller/internal/service/telemetry_service_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package service
 
 import (
+	"cmp"
 	"context"
 	"crypto/x509"
 	"encoding/pem"
@@ -28,7 +29,9 @@ import (
 	"github.com/jumpstarter-dev/jumpstarter/controller/internal/config"
 	"github.com/jumpstarter-dev/jumpstarter/controller/internal/oidc"
 	pb "github.com/jumpstarter-dev/jumpstarter/controller/internal/protocol/jumpstarter/v1"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/status"
 )
 
 // testSigner returns a deterministic Signer built from a fixed seed for use in tests.
@@ -85,51 +88,57 @@ func TestGetServiceEndpoints_RequiresAuthentication(t *testing.T) {
 	}
 }
 
-// buildTelemetryEndpointsResponse exercises the response-building logic
-// without going through the auth gate, for isolated unit testing.
-func buildTelemetryEndpointsResponse(cfg *config.Telemetry) *pb.GetServiceEndpointsResponse {
+// buildTelemetryEndpointsResponse mirrors the production GetServiceEndpoints
+// response-building logic without going through the auth gate.
+// It returns an error when no endpoint can be resolved, matching production.
+func buildTelemetryEndpointsResponse(cfg *config.Telemetry) (*pb.GetServiceEndpointsResponse, error) {
 	resp := &pb.GetServiceEndpointsResponse{}
 	if cfg != nil && cfg.Enabled {
-		minSev := cfg.Logging.Filter.MinSeverity
-		if minSev == "" {
-			minSev = "info"
-		}
-		ep := cfg.Endpoint
+		ep := cmp.Or(cfg.Endpoint, telemetryEndpoint())
 		if ep == "" {
-			ep = telemetryEndpoint()
+			return nil, status.Error(codes.Internal, "telemetry is enabled but no endpoint is configured")
 		}
 		resp.TelemetryEndpoints = append(resp.TelemetryEndpoints, &pb.TelemetryEndpoint{
 			Endpoint:    ep,
 			Certificate: cfg.Certificate,
-			MinSeverity: minSev,
+			MinSeverity: cmp.Or(cfg.Logging.Filter.MinSeverity, "info"),
 		})
 	}
-	return resp
+	return resp, nil
 }
 
 func TestGetServiceEndpoints_NilConfig_ReturnsEmptyList(t *testing.T) {
-	resp := buildTelemetryEndpointsResponse(nil)
+	resp, err := buildTelemetryEndpointsResponse(nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 	if len(resp.TelemetryEndpoints) != 0 {
 		t.Errorf("expected empty telemetry_endpoints, got %d", len(resp.TelemetryEndpoints))
 	}
 }
 
 func TestGetServiceEndpoints_DisabledConfig_ReturnsEmptyList(t *testing.T) {
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{Enabled: false, Endpoint: "telemetry:9093"})
+	resp, err := buildTelemetryEndpointsResponse(&config.Telemetry{Enabled: false, Endpoint: "telemetry:9093"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 	if len(resp.TelemetryEndpoints) != 0 {
 		t.Errorf("expected empty telemetry_endpoints when disabled, got %d", len(resp.TelemetryEndpoints))
 	}
 }
 
 func TestGetServiceEndpoints_WithEndpoint_ReturnsEndpoint(t *testing.T) {
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{
+	resp, err := buildTelemetryEndpointsResponse(&config.Telemetry{
 		Enabled:     true,
 		Endpoint:    "telemetry.jumpstarter.svc:9093",
 		Certificate: "--- PEM ---",
 		Logging: config.TelemetryLogging{
 			Filter: config.TelemetryLoggingFilter{MinSeverity: "warning"},
 		},
 	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 
 	if len(resp.TelemetryEndpoints) != 1 {
 		t.Fatalf("expected 1 telemetry endpoint, got %d", len(resp.TelemetryEndpoints))
@@ -147,10 +156,13 @@ func TestGetServiceEndpoints_WithEndpoint_ReturnsEndpoint(t *testing.T) {
 }
 
 func TestGetServiceEndpoints_DefaultsMinSeverityToInfo(t *testing.T) {
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{
+	resp, err := buildTelemetryEndpointsResponse(&config.Telemetry{
 		Enabled:  true,
 		Endpoint: "telemetry:9093",
 	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 
 	if len(resp.TelemetryEndpoints) != 1 {
 		t.Fatalf("expected 1 endpoint, got %d", len(resp.TelemetryEndpoints))
@@ -163,10 +175,13 @@ func TestGetServiceEndpoints_DefaultsMinSeverityToInfo(t *testing.T) {
 func TestGetServiceEndpoints_UsesEnvVarWhenEndpointEmpty(t *testing.T) {
 	t.Setenv("GRPC_TELEMETRY_ENDPOINT", "telemetry.jumpstarter.svc:9093")
 
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{
+	resp, err := buildTelemetryEndpointsResponse(&config.Telemetry{
 		Enabled: true,
 		// Endpoint intentionally left empty — should fall back to env var.
 	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 
 	if len(resp.TelemetryEndpoints) != 1 {
 		t.Fatalf("expected 1 endpoint, got %d", len(resp.TelemetryEndpoints))
@@ -176,30 +191,31 @@ func TestGetServiceEndpoints_UsesEnvVarWhenEndpointEmpty(t *testing.T) {
 	}
 }
 
-func TestGetServiceEndpoints_BothEndpointAndEnvVarEmpty_ReturnsEmptyEndpoint(t *testing.T) {
+func TestGetServiceEndpoints_BothEndpointAndEnvVarEmpty_ReturnsError(t *testing.T) {
 	t.Setenv("GRPC_TELEMETRY_ENDPOINT", "")
 
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{
+	_, err := buildTelemetryEndpointsResponse(&config.Telemetry{
 		Enabled: true,
 		// Both Endpoint and GRPC_TELEMETRY_ENDPOINT are empty.
 	})
-
-	if len(resp.TelemetryEndpoints) != 1 {
-		t.Fatalf("expected 1 endpoint entry, got %d", len(resp.TelemetryEndpoints))
+	if err == nil {
+		t.Fatal("expected error when no endpoint is configured, got nil")
 	}
-	// An empty endpoint is returned; the caller must handle this gracefully.
-	if resp.TelemetryEndpoints[0].Endpoint != "" {
-		t.Errorf("Endpoint = %q, want empty string when nothing is configured", resp.TelemetryEndpoints[0].Endpoint)
+	if status.Code(err) != codes.Internal {
+		t.Errorf("expected codes.Internal, got %v", status.Code(err))
 	}
 }
 
 func TestGetServiceEndpoints_ConfigEndpointTakesPrecedenceOverEnvVar(t *testing.T) {
 	t.Setenv("GRPC_TELEMETRY_ENDPOINT", "env-telemetry.svc:9093")
 
-	resp := buildTelemetryEndpointsResponse(&config.Telemetry{
+	resp, err := buildTelemetryEndpointsResponse(&config.Telemetry{
 		Enabled:  true,
 		Endpoint: "config-telemetry.svc:9093",
 	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 
 	if len(resp.TelemetryEndpoints) != 1 {
 		t.Fatalf("expected 1 endpoint, got %d", len(resp.TelemetryEndpoints))
@@ -242,33 +258,6 @@ func writeTLSPEMFiles(t *testing.T) (certPath, keyPath string) {
 	return certPath, keyPath
 }
 
-// selfSignedSANs generates a self-signed certificate with the same logic as
-// loadTLSCredentials for a given GRPC_TELEMETRY_ENDPOINT value, and returns
-// its DNS SANs for assertion.
-func selfSignedSANs(t *testing.T, advertised string) []string {
-	t.Helper()
-	var dnsnames []string
-	if advertised != "" {
-		dns, _, err := endpointToSAN(advertised)
-		if err != nil {
-			dnsnames = []string{"localhost"}
-		} else {
-			dnsnames = dns
-		}
-	} else {
-		dnsnames = []string{"localhost"}
-	}
-	cert, err := NewSelfSignedCertificate("test", dnsnames, nil)
-	if err != nil {
-		t.Fatalf("NewSelfSignedCertificate: %v", err)
-	}
-	leaf, err := x509.ParseCertificate(cert.Certificate[0])
-	if err != nil {
-		t.Fatalf("ParseCertificate: %v", err)
-	}
-	return leaf.DNSNames
-}
-
 func TestTelemetryService_LoadTLSCredentials_SelfSigned(t *testing.T) {
 	t.Setenv("EXTERNAL_CERT_PEM", "")
 	t.Setenv("EXTERNAL_KEY_PEM", "")
@@ -292,20 +281,56 @@ func TestTelemetryService_LoadTLSCredentials_SelfSigned(t *testing.T) {
 }
 
 func TestTelemetryService_LoadTLSCredentials_SelfSignedUsesAdvertisedEndpointForSAN(t *testing.T) {
-	// When GRPC_TELEMETRY_ENDPOINT is set, the self-signed cert SAN should derive
-	// from the advertised hostname — not from the bind address.
-	// We test the SAN derivation logic directly (same code path as loadTLSCredentials).
-	sans := selfSignedSANs(t, "telemetry.jumpstarter.svc:9093")
-	if len(sans) != 1 || sans[0] != "telemetry.jumpstarter.svc" {
-		t.Errorf("expected SAN [telemetry.jumpstarter.svc], got %v", sans)
+	// The self-signed cert SAN must match the advertised endpoint hostname so
+	// that TLS hostname verification succeeds when exporters connect.
+	t.Setenv("EXTERNAL_CERT_PEM", "")
+	t.Setenv("EXTERNAL_KEY_PEM", "")
+	t.Setenv("GRPC_TELEMETRY_ENDPOINT", "telemetry.jumpstarter.svc:9093")
+
+	svc := &TelemetryService{BindAddr: ":9093", Signer: testSigner(t)}
+	_, selfSignedPEM, err := svc.loadTLSCredentials()
+	if err != nil {
+		t.Fatalf("loadTLSCredentials() failed: %v", err)
+	}
+	if selfSignedPEM == "" {
+		t.Fatal("expected non-empty selfSignedPEM")
+	}
+
+	block, _ := pem.Decode([]byte(selfSignedPEM))
+	if block == nil {
+		t.Fatal("selfSignedPEM is not valid PEM")
+	}
+	leaf, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Fatalf("ParseCertificate: %v", err)
+	}
+	if len(leaf.DNSNames) != 1 || leaf.DNSNames[0] != "telemetry.jumpstarter.svc" {
+		t.Errorf("expected SAN [telemetry.jumpstarter.svc], got %v", leaf.DNSNames)
 	}
 }
 
 func TestTelemetryService_LoadTLSCredentials_SelfSignedFallsBackToLocalhostWhenNoEndpoint(t *testing.T) {
-	// When GRPC_TELEMETRY_ENDPOINT is empty, SAN defaults to "localhost".
-	sans := selfSignedSANs(t, "")
-	if len(sans) != 1 || sans[0] != "localhost" {
-		t.Errorf("expected SAN [localhost], got %v", sans)
+	// When GRPC_TELEMETRY_ENDPOINT is unset, the self-signed cert SAN defaults to "localhost".
+	t.Setenv("EXTERNAL_CERT_PEM", "")
+	t.Setenv("EXTERNAL_KEY_PEM", "")
+	t.Setenv("GRPC_TELEMETRY_ENDPOINT", "")
+
+	svc := &TelemetryService{BindAddr: ":9093", Signer: testSigner(t)}
+	_, selfSignedPEM, err := svc.loadTLSCredentials()
+	if err != nil {
+		t.Fatalf("loadTLSCredentials() failed: %v", err)
+	}
+
+	block, _ := pem.Decode([]byte(selfSignedPEM))
+	if block == nil {
+		t.Fatal("selfSignedPEM is not valid PEM")
+	}
+	leaf, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Fatalf("ParseCertificate: %v", err)
+	}
+	if len(leaf.DNSNames) != 1 || leaf.DNSNames[0] != "localhost" {
+		t.Errorf("expected SAN [localhost], got %v", leaf.DNSNames)
 	}
 }
 
@@ -384,6 +409,42 @@ func TestTelemetryService_LoadTLSCredentials_MissingCertFileReturnsError(t *test
 	}
 }
 
+func TestTelemetryService_LoadTLSCredentials_MissingKeyFileReturnsError(t *testing.T) {
+	certPath, _ := writeTLSPEMFiles(t)
+	t.Setenv("EXTERNAL_CERT_PEM", certPath)
+	t.Setenv("EXTERNAL_KEY_PEM", "/does/not/exist/tls.key")
+
+	svc := &TelemetryService{BindAddr: ":9093", Signer: testSigner(t)}
+	_, _, err := svc.loadTLSCredentials()
+	if err == nil {
+		t.Fatal("expected error reading missing key file")
+	}
+	if !strings.Contains(err.Error(), "key") {
+		t.Errorf("expected 'key' in error message, got: %v", err)
+	}
+}
+
+func TestTelemetryService_LoadTLSCredentials_ValidCertInvalidKeyReturnsError(t *testing.T) {
+	certPath, _ := writeTLSPEMFiles(t)
+
+	keyFile, err := os.CreateTemp(t.TempDir(), "tls-*.key")
+	if err != nil {
+		t.Fatalf("CreateTemp: %v", err)
+	}
+	if err := keyFile.Close(); err != nil {
+		t.Fatalf("close: %v", err)
+	}
+
+	t.Setenv("EXTERNAL_CERT_PEM", certPath)
+	t.Setenv("EXTERNAL_KEY_PEM", keyFile.Name())
+
+	svc := &TelemetryService{BindAddr: ":9093", Signer: testSigner(t)}
+	_, _, err = svc.loadTLSCredentials()
+	if err == nil {
+		t.Fatal("expected error parsing mismatched cert/key pair")
+	}
+}
+
 func TestTelemetryService_Start_FailsWhenExternalCertFileMissing(t *testing.T) {
 	t.Setenv("EXTERNAL_CERT_PEM", "/no/such/cert.pem")
 	t.Setenv("EXTERNAL_KEY_PEM", "/no/such/key.pem")