-
Notifications
You must be signed in to change notification settings - Fork 12
fix!: flagd, wire up TLS support for gRPC connections #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add `tls: bool` parameter to `UpstreamConfig::new()` to control `http://` vs `https://` scheme selection - Update `GrpcStreamConnector` to store and propagate TLS setting - Update all callers (RPC resolver, in-process resolver, connector) - Fix tonic TLS features for 0.14: use `tls-ring` + `tls-webpki-roots` (no OpenSSL dependency) - Add unit tests for TLS scheme selection The `FlagdOptions.tls` field was documented but not wired up. This change ensures that when `tls: true`, gRPC connections use HTTPS. Signed-off-by: Eren Atas <[email protected]>
Summary of ChangesHello @erenatas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces robust TLS support for gRPC connections within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly wires up TLS support for gRPC connections, which was a documented but unimplemented feature. The changes involve updating UpstreamConfig to handle http vs https schemes based on a tls flag, and propagating this setting through the RPC and in-process resolvers. The addition of tls-ring and tls-webpki-roots features for tonic is a good choice for enabling TLS without an OpenSSL dependency. The new unit tests for UpstreamConfig are comprehensive and cover the new logic well.
I've identified a couple of issues. First, a high-severity pre-existing bug that makes the in-process resolver's Unix socket support non-functional, which is affected by the changes in this PR. Second, a medium-severity inconsistency in how URL schemes are handled between the RPC and in-process resolvers. My feedback includes a suggestion to align their behavior for better predictability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly wires up TLS support for gRPC connections, which was a missing feature. The changes include updating the tonic dependency to use tls-ring and tls-webpki-roots for TLS without an OpenSSL dependency, and modifying UpstreamConfig to handle https schemes. The addition of unit tests for the new TLS logic is also a great improvement.
My review has identified a few areas for improvement:
- There's some code duplication in the TLS configuration logic that could be refactored.
- A test contains a redundant assertion that could be removed for clarity.
- There's duplicated logic for handling
envoy://URIs in the RPC resolver, which should be consolidated to use the new capabilities ofUpstreamConfig.
Overall, this is a valuable fix, and addressing these points will improve the code's maintainability.
|
Hey @erenatas. Great work! What about self-signed certs? |
Signed-off-by: Eren Atas <[email protected]>
Thank you and nice catch! I missed handling cert files, can you have a look now? I also made sure rest implementation is consistent by utilizing |
8012c87 to
7af3815
Compare
This PR
tls: boolparameter toUpstreamConfig::new()to controlhttp://vshttps://scheme selectionGrpcStreamConnectorto store and propagate TLS settingtls-ring+tls-webpki-roots(no OpenSSL dependency)The
FlagdOptions.tlsfield was documented but not wired up. This change ensures that whentls: true, gRPC connections use HTTPS.