fix: add TLS credentials support to gRPC server #20
Conversation
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
Why: gRPC TLS tests were timing out because gRPC servers require TLS credentials configured via grpc.Creds(), not just a TLS listener Approach: - Added variadic grpc.ServerOption to NewGrpcServer signature - Updated NewServer to accept optional *tls.Config and convert to grpc.Creds() - Modified test harness to pass TLS config to gRPC servers, use plain listeners - HTTP servers continue using TLS listeners (different architecture) Learned: gRPC and HTTP handle TLS differently - gRPC needs server credentials, HTTP uses TLS at listener level Next: Create PR, merge, then rebase logs branch Fixes #17 Co-Authored-By: Claude <[email protected]>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes TLS support for gRPC servers by properly configuring server credentials. Previously, two TLS-related gRPC tests were timing out because gRPC requires TLS to be configured via grpc.Creds() rather than just using a TLS listener.
Key changes:
- Modified
NewGrpcServerto accept variadicgrpc.ServerOptionparameters for TLS and other configurations - Updated
NewServerto accept optional*tls.Configand convert it to gRPC credentials for gRPC servers - Adjusted test harness to pass TLS config to gRPC servers and use plain listeners with server credentials
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| otlpserver/server.go | Added optional TLS config parameter to NewServer and converts it to gRPC credentials for gRPC protocol |
| otlpserver/grpcserver.go | Modified NewGrpcServer to accept variadic server options for TLS configuration |
| main_test.go | Updated test harness to prepare TLS config upfront and pass it to gRPC servers while using TLS listener only for HTTP |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes the TLS gRPC test timeouts by properly configuring gRPC server credentials.
Problem
Two TLS-related gRPC tests were timing out:
minimum configuration (tls, no-verify, recording, grpc)minimum configuration (tls, client cert auth, recording, grpc)The root cause: gRPC requires TLS credentials to be configured in the server itself via
grpc.Creds(), not just passed through a TLS listener. HTTP and gRPC handle TLS fundamentally differently.Solution
Updated
NewGrpcServer:grpc.ServerOptionparameter for TLS and other configurationsUpdated
NewServer:*tls.Configparametergrpc.Creds()for gRPC serversUpdated test harness:
ServerTLSEnabledis trueTest Results
$ go test PASS ok github.com/tobert/otel-cli 1.019sBoth previously-failing TLS gRPC tests now pass! ✅
Breaking Changes
None - all changes are backwards compatible.
Fixes #17
Co-Authored-By: Claude [email protected]