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/config/config.go

@@ -1,8 +1,11 @@
 package config
 
 import (
+	"cmp"
 	"context"
 	"fmt"
+	"net"
+	"os"
 	"time"
 
 	"github.com/jumpstarter-dev/jumpstarter/controller/internal/oidc"
@@ -43,6 +46,34 @@ func LoadRouterConfiguration(
 	return serverOptions, nil
 }
 
+// resolveTelemetryConfig validates and resolves the telemetry endpoint for a
+// Telemetry config block. It prefers the explicit ConfigMap endpoint and falls
+// back to GRPC_TELEMETRY_ENDPOINT, matching the behaviour previously inlined in
+// LoadConfiguration. Returns nil when t is nil or disabled.
+func resolveTelemetryConfig(t *Telemetry) (*Telemetry, error) {
+	if t == nil || !t.Enabled {
+		return nil, nil
+	}
+	if err := t.Validate(); err != nil {
+		return nil, err
+	}
+	// Prefer the explicit ConfigMap value; fall back to the env var so the
+	// operator can set the endpoint without touching the ConfigMap. Resolving
+	// here ensures LoadedConfig.Telemetry.Endpoint is always the complete value
+	// — callers don't need to re-check the env var.
+	t.Endpoint = cmp.Or(t.Endpoint, os.Getenv("GRPC_TELEMETRY_ENDPOINT"))
+	if ep := t.Endpoint; ep != "" {
+		host, _, err := net.SplitHostPort(ep)
+		if err != nil {
+			return nil, fmt.Errorf("telemetry endpoint %q is not a valid host:port: %w", ep, err)
+		}
+		if host == "" {
+			return nil, fmt.Errorf("telemetry endpoint %q has no host", ep)
+		}
+	}
+	return t, nil
+}
+
 func LoadConfiguration(
 	ctx context.Context,
 	client client.Reader,
@@ -122,12 +153,9 @@ func LoadConfiguration(
 		return nil, err
 	}
 
-	var telemetry *Telemetry
-	if config.Telemetry != nil && config.Telemetry.Enabled {
-		if err := config.Telemetry.Validate(); err != nil {
-			return nil, err
-		}
-		telemetry = config.Telemetry
+	telemetry, err := resolveTelemetryConfig(config.Telemetry)
+	if err != nil {
+		return nil, err
 	}
 
 	return &LoadedConfig{

controller/internal/config/types_test.go

@@ -276,6 +276,78 @@ func TestDeprecatedLabelsOmitEmpty(t *testing.T) {
 	}
 }
 
+func TestTelemetryEndpointResolution(t *testing.T) {
+	tests := []struct {
+		name         string
+		configValue  string
+		envValue     string
+		wantEndpoint string
+		wantErr      bool
+	}{
+		{
+			name:         "ConfigMap value takes precedence",
+			configValue:  "telemetry.ns.svc:9093",
+			envValue:     "env-telemetry:9093",
+			wantEndpoint: "telemetry.ns.svc:9093",
+		},
+		{
+			name:         "env var fallback when ConfigMap is empty",
+			envValue:     "env-telemetry.svc:9093",
+			wantEndpoint: "env-telemetry.svc:9093",
+		},
+		{
+			name:         "both empty yields empty endpoint (no error)",
+			wantEndpoint: "",
+		},
+		{
+			name:        "malformed ConfigMap value is rejected",
+			configValue: "no-port",
+			wantErr:     true,
+		},
+		{
+			name:     "malformed env var is rejected",
+			envValue: "garbage-no-port",
+			wantErr:  true,
+		},
+		{
+			name:        "port-only ConfigMap value is rejected",
+			configValue: ":9093",
+			wantErr:     true,
+		},
+		{
+			name:     "port-only env var is rejected",
+			envValue: ":9093",
+			wantErr:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("GRPC_TELEMETRY_ENDPOINT", tt.envValue)
+
+			cfg := &Telemetry{Enabled: true, Endpoint: tt.configValue}
+			resolved, err := resolveTelemetryConfig(cfg)
+
+			if tt.wantErr {
+				if err == nil {
+					t.Fatalf("expected validation error, got nil (resolved=%+v)", resolved)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			var gotEndpoint string
+			if resolved != nil {
+				gotEndpoint = resolved.Endpoint
+			}
+			if gotEndpoint != tt.wantEndpoint {
+				t.Errorf("resolved.Endpoint = %q, want %q", gotEndpoint, tt.wantEndpoint)
+			}
+		})
+	}
+}
+
 func TestParseDuration(t *testing.T) {
 	tests := []struct {
 		input    string

controller/internal/service/controller_service.go

@@ -327,11 +327,13 @@ func (s *ControllerService) GetServiceEndpoints(
 	resp := &pb.GetServiceEndpointsResponse{}
 
 	if s.TelemetryConfig != nil && s.TelemetryConfig.Enabled {
-		// 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())
+		// Endpoint is resolved at config-load time (ConfigMap value or GRPC_TELEMETRY_ENDPOINT
+		// env var fallback), so TelemetryConfig.Endpoint is always the complete value here.
+		if s.TelemetryConfig.Endpoint == "" {
+			return nil, status.Error(codes.FailedPrecondition, "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,
+			Endpoint:    s.TelemetryConfig.Endpoint,
 			Certificate: s.TelemetryConfig.Certificate,
 			MinSeverity: cmp.Or(s.TelemetryConfig.Logging.Filter.MinSeverity, "info"),
 		})
@@ -1199,32 +1201,9 @@ func (s *ControllerService) Start(ctx context.Context) error {
 		return err
 	}
 
-	// Load external certificate if provided via environment variables.
-	// Environment variables EXTERNAL_CERT_PEM and EXTERNAL_KEY_PEM should contain the PEM-encoded
-	// certificate and private key respectively. If both are set, they are used; otherwise
-	// a self-signed certificate is generated.
-	var cert *tls.Certificate
-	certPEMPath := os.Getenv("EXTERNAL_CERT_PEM")
-	keyPEMPath := os.Getenv("EXTERNAL_KEY_PEM")
-	if certPEMPath != "" && keyPEMPath != "" {
-		certPEMBytes, err := os.ReadFile(certPEMPath)
-		if err != nil {
-			return fmt.Errorf("failed to read external certificate file: %w", err)
-		}
-		keyPEMBytes, err := os.ReadFile(keyPEMPath)
-		if err != nil {
-			return fmt.Errorf("failed to read external key file: %w", err)
-		}
-		parsedCert, err := tls.X509KeyPair(certPEMBytes, keyPEMBytes)
-		if err != nil {
-			return fmt.Errorf("failed to parse external certificate: %w", err)
-		}
-		cert = &parsedCert
-	} else {
-		cert, err = NewSelfSignedCertificate("jumpstarter controller", dnsnames, ipaddresses)
-		if err != nil {
-			return err
-		}
+	cert, _, err := LoadTLSCertificate("jumpstarter controller", dnsnames, ipaddresses)
+	if err != nil {
+		return err
 	}
 
 	opts := append(s.ServerOptions,
@@ -1267,8 +1246,11 @@ func (s *ControllerService) Start(ctx context.Context) error {
 	// Register gRPC gateway
 	gwmux := gwruntime.NewServeMux()
 
+	// The controller multiplexes gRPC (h2) and REST (http/1.1) on a single port,
+	// so it needs NextProtos — which LoadTLSCredentials doesn't expose.
 	listener, err := tls.Listen("tcp", ":8082", &tls.Config{
 		Certificates: []tls.Certificate{*cert},
+		MinVersion:   tls.VersionTLS12,
 		NextProtos:   []string{"http/1.1", "h2"},
 	})
 	if err != nil {

controller/internal/service/controller_service_test.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/golang-jwt/jwt/v5"
 	jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/api/v1alpha1"
+	"github.com/jumpstarter-dev/jumpstarter/controller/internal/config"
 	jlog "github.com/jumpstarter-dev/jumpstarter/controller/internal/log"
 	pb "github.com/jumpstarter-dev/jumpstarter/controller/internal/protocol/jumpstarter/v1"
 	"google.golang.org/grpc"
@@ -39,11 +40,13 @@ import (
 	"google.golang.org/grpc/status"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	ctrlzap "sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 const testRouterToken = "tok"
@@ -2061,6 +2064,57 @@ func (noopAuthorizer) Authorize(_ context.Context, _ authorizer.Attributes) (aut
 	return authorizer.DecisionNoOpinion, "", nil
 }
 
+// passingAuthenticator always authenticates successfully with a fixed user name.
+type passingAuthenticator struct{ userName string }
+
+func (p *passingAuthenticator) AuthenticateContext(_ context.Context) (*authenticator.Response, bool, error) {
+	return &authenticator.Response{User: &user.DefaultInfo{Name: p.userName}}, true, nil
+}
+
+// exporterAttributesGetter returns attributes that identify a fixed Exporter object.
+type exporterAttributesGetter struct{ namespace, name string }
+
+func (e *exporterAttributesGetter) ContextAttributes(_ context.Context, u user.Info) (authorizer.Attributes, error) {
+	return authorizer.AttributesRecord{
+		User:      u,
+		Namespace: e.namespace,
+		Resource:  "Exporter",
+		Name:      e.name,
+	}, nil
+}
+
+// passingAuthorizer always allows.
+type passingAuthorizer struct{}
+
+func (passingAuthorizer) Authorize(_ context.Context, _ authorizer.Attributes) (authorizer.Decision, string, error) {
+	return authorizer.DecisionAllow, "", nil
+}
+
+// authSuccessServiceCtx builds a ControllerService whose authentication always
+// succeeds. A pre-populated Exporter object is stored in the fake client so
+// that VerifyExporterObjectToken can fetch it.
+func authSuccessServiceCtx(t *testing.T, cfg *config.Telemetry) (*ControllerService, context.Context) {
+	t.Helper()
+
+	scheme := k8sruntime.NewScheme()
+	if err := jumpstarterdevv1alpha1.AddToScheme(scheme); err != nil {
+		t.Fatalf("failed to add scheme: %v", err)
+	}
+	exporter := &jumpstarterdevv1alpha1.Exporter{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-exporter", Namespace: "default"},
+	}
+	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(exporter).Build()
+
+	svc := &ControllerService{
+		Client:          fakeClient,
+		Authn:           &passingAuthenticator{userName: "test-user"},
+		Authz:           passingAuthorizer{},
+		Attr:            &exporterAttributesGetter{namespace: "default", name: "test-exporter"},
+		TelemetryConfig: cfg,
+	}
+	return svc, context.Background()
+}
+
 // authFailureServiceCtx builds a ControllerService whose authentication always
 // fails, plus a context carrying a peer address, a captured logger, and the
 // jlog.LogContext enrichment applied by the gRPC interceptors in production.

controller/internal/service/endpoints.go

@@ -1,8 +1,11 @@
 package service
 
 import (
+	"fmt"
 	"net"
 	"os"
+
+	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 func controllerEndpoint() string {
@@ -22,14 +25,35 @@ func routerEndpoint() string {
 }
 
 func telemetryEndpoint() string {
-	return os.Getenv("GRPC_TELEMETRY_ENDPOINT")
+	ep := os.Getenv("GRPC_TELEMETRY_ENDPOINT")
+	if ep == "" {
+		return ""
+	}
+	if err := validateHostPort(ep); err != nil {
+		ctrl.Log.WithName("telemetry").Error(err, "GRPC_TELEMETRY_ENDPOINT is not a valid host:port; ignoring",
+			"value", ep)
+		return ""
+	}
+	return ep
 }
 
-func endpointToSAN(endpoint string) ([]string, []net.IP, error) {
-	host, _, err := net.SplitHostPort(endpoint)
+// validateHostPort checks that s is a valid "host:port" with a non-empty host.
+func validateHostPort(s string) error {
+	host, _, err := net.SplitHostPort(s)
 	if err != nil {
+		return err
+	}
+	if host == "" {
+		return fmt.Errorf("endpoint %q has no host", s)
+	}
+	return nil
+}
+
+func endpointToSAN(endpoint string) ([]string, []net.IP, error) {
+	if err := validateHostPort(endpoint); err != nil {
 		return nil, nil, err
 	}
+	host, _, _ := net.SplitHostPort(endpoint)
 	ip := net.ParseIP(host)
 	if ip != nil {
 		return []string{}, []net.IP{ip}, nil