dns: add a new RUST-based DNS resolver extension#44090
dns: add a new RUST-based DNS resolver extension#44090agrawroh wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
e720a35 to
fd827fd
Compare
|
/gemini review |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver extension, Hickory, which is a great addition. The implementation is comprehensive, including the core logic in Rust, the C++ shell, ABI definitions, build system changes, documentation, and tests.
My review has identified two main issues:
- A large, unrelated
TracerABI definition has been included insource/extensions/dynamic_modules/abi/abi.h. This should be removed. - There's a bug in the DNS-over-HTTPS configuration logic in the Rust implementation that prevents using hostnames in DoH URLs.
Apart from these points, the changes look solid. The C++ shell and Rust SDK are well-designed for thread safety and proper resource management.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver extension, Hickory, to Envoy. This is a substantial and well-executed feature, including a new dynamic module ABI for DNS resolvers, a Rust SDK for this ABI, the C++ shell for the extension, and the Rust implementation of the resolver logic. The code is well-structured, documented, and comes with comprehensive unit and integration tests. My review identified a critical issue in the new Rust SDK module where FFI functions are not panic-safe. Panics from user code could unwind across the FFI boundary, leading to undefined behavior and process crashes. I have provided comments with code suggestions to wrap the FFI calls with catch_unwind to ensure safety, consistent with other parts of the dynamic module SDK.
b5cc9d9 to
377e75f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Rust-based DNS resolver, Hickory, as a dynamic module. This is a significant and well-executed addition, providing modern DNS features like DNS-over-TLS, DNS-over-HTTPS, and DNSSEC. The implementation is thoughtfully split between a C++ shell and a Rust module, communicating via a new, well-documented DNS resolver ABI. The changes are extensive, touching API definitions, build systems, documentation, and adding new implementation and test files. The C++/Rust interaction, particularly the threading and shutdown logic, appears robust. The tests are comprehensive, covering both unit and integration scenarios. I have one suggestion to enhance configuration validation in the Rust module.
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Description
This PR adds a new RUST-based DNS resolver extension to Envoy.
Commit Message: dns: add a new RUST-based DNS resolver extension
Additional Description: Added a new RUST-based DNS resolver extension to Envoy.
Risk Level: Low
Testing: Added Tests
Docs Changes: Added
Release Notes: Added