feat: TLS and env-based endpoint for jumpstarter-telemetry - #975
Open
bkhizgiy wants to merge 2 commits into
Open
feat: TLS and env-based endpoint for jumpstarter-telemetry#975bkhizgiy wants to merge 2 commits into
bkhizgiy wants to merge 2 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe telemetry service now resolves an explicit or environment-derived endpoint. It always starts with TLS. It uses external PEM files or a self-signed certificate with endpoint-derived SANs. Startup, shutdown, and error handling were updated. Tests cover endpoint and TLS behavior. ChangesTelemetry service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TelemetryService
participant Environment
participant TLSCredentials
participant GRPCServer
TelemetryService->>Environment: Resolve GRPC_TELEMETRY_ENDPOINT
TelemetryService->>TLSCredentials: Load external or self-signed credentials
TLSCredentials-->>TelemetryService: Return TLS credentials and certificate PEM
TelemetryService->>GRPCServer: Start with TLS credentials
GRPCServer-->>TelemetryService: Return serve error
TelemetryService->>GRPCServer: Stop server
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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>
bkhizgiy
marked this pull request as ready for review
August 5, 2026 13:42
Contributor
|
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. |
raballew
reviewed
Aug 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service/tls_credentials.go`:
- Around line 40-57: Update the external TLS configuration branch in the
certificate-loading function to detect when exactly one of EXTERNAL_CERT_PEM or
EXTERNAL_KEY_PEM is set and return an error instead of falling back to
self-signed certificate generation. Continue loading and parsing the external
certificate only when both paths are configured, and preserve the existing
behavior when neither is set.
🪄 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: 396acf93-3707-43b2-8d21-0fec2dbfb4f2
📒 Files selected for processing (6)
controller/internal/config/config.gocontroller/internal/service/controller_service.gocontroller/internal/service/router_service.gocontroller/internal/service/telemetry_service.gocontroller/internal/service/telemetry_service_test.gocontroller/internal/service/tls_credentials.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controller/internal/service/telemetry_service.go
- controller/internal/service/telemetry_service_test.go
Assisted-by: Claude <claude@anthropic.com> Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
raballew
requested changes
Aug 13, 2026
Comment on lines
+332
to
+334
| if s.TelemetryConfig.Endpoint == "" { | ||
| 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") | ||
| } |
Member
There was a problem hiding this comment.
The empty-endpoint guard at these lines is only reachable after authenticateExporter succeeds. All existing tests that call svc.GetServiceEndpoints use a failing authenticator.
Comment on lines
+280
to
+349
| func TestTelemetryEndpointResolution(t *testing.T) { | ||
| // This tests the resolution logic used by LoadConfiguration: | ||
| // cmp.Or(configMapValue, os.Getenv("GRPC_TELEMETRY_ENDPOINT")) | ||
| // followed by net.SplitHostPort validation. | ||
| tests := []struct { | ||
| name string | ||
| configValue string | ||
| envValue string | ||
| wantResult string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "ConfigMap value takes precedence", | ||
| configValue: "telemetry.ns.svc:9093", | ||
| envValue: "env-telemetry:9093", | ||
| wantResult: "telemetry.ns.svc:9093", | ||
| }, | ||
| { | ||
| name: "env var fallback when ConfigMap is empty", | ||
| envValue: "env-telemetry.svc:9093", | ||
| wantResult: "env-telemetry.svc:9093", | ||
| }, | ||
| { | ||
| name: "both empty yields empty (no error)", | ||
| wantResult: "", | ||
| }, | ||
| { | ||
| name: "malformed ConfigMap value is rejected", | ||
| configValue: "no-port", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "malformed env var is rejected", | ||
| envValue: "garbage-no-port", | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Setenv("GRPC_TELEMETRY_ENDPOINT", tt.envValue) | ||
|
|
||
| // Simulate the resolution logic from LoadConfiguration. | ||
| resolved := tt.configValue | ||
| if resolved == "" { | ||
| resolved = tt.envValue | ||
| } | ||
|
|
||
| var err error | ||
| if resolved != "" { | ||
| if _, _, splitErr := net.SplitHostPort(resolved); splitErr != nil { | ||
| err = splitErr | ||
| } | ||
| } | ||
|
|
||
| if tt.wantErr { | ||
| if err == nil { | ||
| t.Fatalf("expected validation error for resolved=%q, got nil", resolved) | ||
| } | ||
| return | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if resolved != tt.wantResult { | ||
| t.Errorf("resolved = %q, want %q", resolved, tt.wantResult) | ||
| } | ||
| }) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
TestTelemetryEndpointResolution simulates the LoadConfiguration logic instead of calling it.
Comment on lines
+332
to
+334
| if s.TelemetryConfig.Endpoint == "" { | ||
| 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") | ||
| } |
Member
There was a problem hiding this comment.
A missing endpoint is a known operator configuration gap, not an unexpected server failure. codes.Internal communicates unexpected server-side failure, whereas codes.FailedPrecondition better matches this situation per the gRPC status code semantics.
Comment on lines
39
to
41
| func endpointToSAN(endpoint string) ([]string, []net.IP, error) { | ||
| host, _, err := net.SplitHostPort(endpoint) | ||
| if err != nil { |
Member
There was a problem hiding this comment.
endpointToSAN generates a broken TLS certificate when given a port-only endpoint like :9093.
Comment on lines
+32
to
+33
| ctrl.Log.WithName("telemetry").Info("GRPC_TELEMETRY_ENDPOINT is not a valid host:port; ignoring", | ||
| "value", ep, "error", err) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.