feat: TLS and env-based endpoint for jumpstarter-telemetry - #975
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe telemetry service now uses an explicit or environment-derived endpoint. It always starts with TLS, using external PEM files when configured or a self-signed certificate with endpoint-derived SANs. Tests cover endpoint precedence and TLS credential failures. ChangesTelemetry service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerService
participant TelemetryService
participant CertificateFiles
participant GRPCServer
ControllerService->>TelemetryService: Advertise configured or environment endpoint
TelemetryService->>CertificateFiles: Load external PEM credentials
alt External credentials unavailable
TelemetryService->>TelemetryService: Generate self-signed certificate
end
TelemetryService->>GRPCServer: Start with TLS credentials
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/internal/config/types.go`:
- Around line 38-43: Update the Telemetry.Certificate documentation comment to
remove the claim that an empty value enables system CA verification for
self-signed certificates. State that self-signed mode requires an explicit
unverified-client policy, or that the operator must provide a stable CA
certificate and configure it for verification.
In `@controller/internal/service/controller_service.go`:
- Around line 330-334: The controller and telemetry server resolve different
endpoint sources, causing advertised endpoints to disagree with certificate
SANs. In controller/internal/service/controller_service.go:330-334,
controller/internal/service/telemetry_service.go:226-240, and
controller/cmd/telemetry/main.go:24-25, introduce and use one shared endpoint
configuration for both processes, derive self-signed SANs from that resolved
value, and advertise only that value; add an integration test verifying a TLS
client validates the certificate for the endpoint returned by
GetServiceEndpoints.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a1796af-2663-4c0b-9759-2c325f7c694a
📒 Files selected for processing (7)
controller/cmd/telemetry/main.gocontroller/internal/config/config.gocontroller/internal/config/types.gocontroller/internal/service/controller_service.gocontroller/internal/service/endpoints.gocontroller/internal/service/telemetry_service.gocontroller/internal/service/telemetry_service_test.go
💤 Files with no reviewable changes (1)
- controller/internal/config/config.go
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude Sonnet 4.5 <claude@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| 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()) | ||
| resp.TelemetryEndpoints = append(resp.TelemetryEndpoints, &pb.TelemetryEndpoint{ | ||
| Endpoint: s.TelemetryConfig.Endpoint, | ||
| Endpoint: ep, | ||
| Certificate: s.TelemetryConfig.Certificate, | ||
| MinSeverity: cmp.Or(s.TelemetryConfig.Logging.Filter.MinSeverity, "info"), | ||
| }) |
There was a problem hiding this comment.
When GRPC_TELEMETRY_ENDPOINT is unset and TelemetryConfig.Endpoint is also empty, cmp.Or("", "") returns "". The append highlighted here, runs unconditionally, so all exporters receive a TelemetryEndpoint{Endpoint: ""} and attempt to dial an empty string. I think you need to guard the append or return error.
| } | ||
|
|
||
| srv := grpc.NewServer() | ||
| srv := grpc.NewServer(grpc.Creds(creds)) |
There was a problem hiding this comment.
The errCh branch (case err := <-errCh: return err) in the select block below returns without stopping the gRPC server.
| certPEMPath := os.Getenv("EXTERNAL_CERT_PEM") | ||
| keyPEMPath := os.Getenv("EXTERNAL_KEY_PEM") | ||
|
|
||
| var cert *tls.Certificate | ||
| if certPEMPath != "" && keyPEMPath != "" { | ||
| certPEMBytes, readErr := os.ReadFile(certPEMPath) | ||
| if readErr != nil { | ||
| return nil, "", fmt.Errorf("failed to read external certificate file: %w", readErr) | ||
| } | ||
| keyPEMBytes, readErr := os.ReadFile(keyPEMPath) | ||
| if readErr != nil { | ||
| return nil, "", fmt.Errorf("failed to read external key file: %w", readErr) | ||
| } | ||
| parsedCert, parseErr := tls.X509KeyPair(certPEMBytes, keyPEMBytes) | ||
| if parseErr != nil { | ||
| return nil, "", fmt.Errorf("failed to parse external certificate: %w", parseErr) | ||
| } | ||
| cert = &parsedCert | ||
| } else { | ||
| // Derive the TLS SAN from the advertised endpoint (what clients connect to), | ||
| // not from the bind address (which is a local port like ":9093"). | ||
| // Same pattern as the router and controller services. | ||
| // IMPORTANT: GRPC_TELEMETRY_ENDPOINT must be set on the telemetry pod itself | ||
| // so the SAN matches the endpoint the controller advertises to exporters. | ||
| advertised := telemetryEndpoint() | ||
| var dnsnames []string | ||
| var ipaddresses []net.IP | ||
| if advertised != "" { | ||
| var sanErr error | ||
| dnsnames, ipaddresses, sanErr = endpointToSAN(advertised) | ||
| if sanErr != nil { | ||
| dnsnames = []string{"localhost"} | ||
| } | ||
| } else { | ||
| // No advertised endpoint configured — development/local mode. | ||
| dnsnames = []string{"localhost"} | ||
| } | ||
| var genErr error | ||
| cert, genErr = NewSelfSignedCertificate("jumpstarter telemetry", dnsnames, ipaddresses) | ||
| if genErr != nil { | ||
| return nil, "", genErr | ||
| } | ||
| // Encode the leaf cert as PEM so the caller can log it for the operator. | ||
| selfSignedPEM = string(pem.EncodeToMemory(&pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: cert.Certificate[0], | ||
| })) | ||
| } | ||
| return credentials.NewServerTLSFromCert(cert), selfSignedPEM, nil |
There was a problem hiding this comment.
TLS credential loading is duplicated between this block and controller_service.go:1206-1228 and I think they already diverged.
| if selfSignedPEM != "" { | ||
| // Log the self-signed cert so the operator can copy it into the controller | ||
| // ConfigMap's telemetry.certificate field. Exporters need this PEM to verify | ||
| // the TLS connection — a self-signed cert is not trusted by the system CA pool. | ||
| logger.Info("Using self-signed TLS certificate; copy certPEM into the controller ConfigMap telemetry.certificate so exporters can verify TLS", | ||
| "certPEM", selfSignedPEM) | ||
| } | ||
|
|
||
| lis, err := net.Listen("tcp", s.BindAddr) | ||
| if err != nil { | ||
| return fmt.Errorf("telemetry: listen %s: %w", s.BindAddr, err) | ||
| } | ||
|
|
||
| srv := grpc.NewServer() | ||
| srv := grpc.NewServer(grpc.Creds(creds)) | ||
| pb.RegisterTelemetryServiceServer(srv, s) | ||
| reflection.Register(srv) |
There was a problem hiding this comment.
The core behavioral change introduced by this PR, grpc.NewServer(grpc.Creds(creds)) mandating TLS for all connections, has no behavioral test.
| ep := cfg.Endpoint | ||
| if ep == "" { | ||
| ep = telemetryEndpoint() | ||
| } | ||
| resp.TelemetryEndpoints = append(resp.TelemetryEndpoints, &pb.TelemetryEndpoint{ | ||
| Endpoint: cfg.Endpoint, | ||
| Endpoint: ep, | ||
| Certificate: cfg.Certificate, | ||
| MinSeverity: minSev, | ||
| }) |
There was a problem hiding this comment.
The production line ep := cmp.Or(s.TelemetryConfig.Endpoint, telemetryEndpoint()) in controller_service.go has zero coverage.
| if err := config.Telemetry.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
| // Auto-derive the gRPC address when the operator has not overridden it. | ||
| // The well-known service name follows the same pattern as the controller | ||
| // and router: <service>.<namespace>.svc (in-cluster DNS). | ||
| if config.Telemetry.Endpoint == "" { | ||
| config.Telemetry.Endpoint = "jumpstarter-telemetry." + key.Namespace + ":9093" | ||
| } | ||
| telemetry = config.Telemetry | ||
| } |
There was a problem hiding this comment.
GRPC_TELEMETRY_ENDPOINT is read in the service layer (endpoints.go) rather than here in LoadConfiguration. As a result, LoadedConfig.Telemetry.Endpoint is always "" when the ConfigMap omits the field, even if the env var is set.
| func telemetryEndpoint() string { | ||
| return os.Getenv("GRPC_TELEMETRY_ENDPOINT") | ||
| } |
There was a problem hiding this comment.
The raw env var value is returned with no host:port format validation before it is distributed to all exporters.
| Bytes: cert.Certificate[0], | ||
| })) | ||
| } | ||
| return credentials.NewServerTLSFromCert(cert), selfSignedPEM, nil |
There was a problem hiding this comment.
Whats our required TLS min version?
| lis, err := net.Listen("tcp", s.BindAddr) | ||
| if err != nil { | ||
| return fmt.Errorf("telemetry: listen %s: %w", s.BindAddr, err) | ||
| } |
There was a problem hiding this comment.
After net.Listen succeeds there is no defer lis.Close() guard.
| // Endpoint: GRPC_TELEMETRY_ENDPOINT must be set on BOTH this pod and the controller | ||
| // pod to the same value (e.g. "jumpstarter-telemetry.jumpstarter.svc:9093"). | ||
| // The telemetry service uses it to generate the correct SAN in the self-signed | ||
| // certificate; the controller uses it to advertise the address to exporters via | ||
| // GetServiceEndpoints. A mismatch causes TLS hostname verification failures. | ||
| // | ||
| // Future phases will add direct Loki push and MetricsStream for reverse-scrape | ||
| // of exporter prometheus_client registries. | ||
| package main |
There was a problem hiding this comment.
In the main() function below, the goroutine calling svc.Start(ctx) is launched before signal.Notify is registered. In the narrow window between goroutine start and signal subscription, SIGINT and SIGTERM use Go's default disposition (immediate process termination). A signal arriving in this window bypasses the graceful shutdown path entirely.
2 participants
Follow-up to #930
jumpstarter-telemetry now always uses TLS.
Set
EXTERNAL_CERT_PEMandEXTERNAL_KEY_PEMto mount operator-provided certs,falls back to a self-signed cert when absent.
The controller now reads
GRPC_TELEMETRY_ENDPOINTfrom theenvironment instead of auto-deriving
jumpstarter-telemetry.<ns>:9093from the ConfigMap loader.