feat: add CA certificate and IPv6 options to client configuration#836
Open
ciiiii wants to merge 7 commits intooxia-db:mainfrom
Open
feat: add CA certificate and IPv6 options to client configuration#836ciiiii wants to merge 7 commits intooxia-db:mainfrom
ciiiii wants to merge 7 commits intooxia-db:mainfrom
Conversation
ciiiii
requested review from
RobertIndie,
mattisonchao and
merlimat
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two new client configuration options to support custom CA certificates for TLS verification and the ability to disable IPv6 resolution for IPv4-only network environments.
Key Changes:
- Added
WithCACertFileoption to load and trust custom CA certificates for server authentication - Added
WithDisableIPv6option to force IPv4-only address resolution - Implemented custom dialer with manual DNS resolution and IP preference handling
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| oxia/options_client.go | Adds WithCACertFile and WithDisableIPv6 client options with TLS config and IPv6 flag support |
| oxia/options_delete_test.go | Comprehensive tests for new CA certificate and IPv6 disable options including error cases |
| oxia/async_client_impl.go | Updates async client to use new NewClientPoolWithOptions with DisableIPv6 support |
| common/rpc/client_pool.go | Implements custom DNS resolver with IPv4/IPv6 control, enhanced TLS configuration with CA cert support, and detailed connection/certificate logging |
| common/rpc/client_pool_test.go | Tests for new ClientPool options including DisableIPv6, custom CA certs, hostname extraction, and transport credentials |
| cmd/client/common/client.go | Integrates new CA cert and IPv6 options into CLI client configuration |
| cmd/client/cmd.go | Adds --ca-cert-file and --disable-ipv6 command-line flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Introduced `CACertFile` option for specifying a CA certificate file for TLS verification. - Added `DisableIPv6` option to allow users to disable IPv6 resolution, forcing the use of IPv4 addresses. - Updated client pool and related tests to support the new options. Signed-off-by: Yisheng Cai <yisheng.cai@streamnative.io>
… TLS verification - Refactored the connection handling in the client pool by introducing a new `createDialer` method to encapsulate IP resolution and connection logic. - Added detailed logging for DNS resolution and connection attempts, enhancing debugging capabilities. - Updated TLS configuration to enforce a minimum version of TLS 1.2 and improved certificate verification logging. - Renamed `getHostname` to `GetHostname` for consistency and accessibility. Signed-off-by: Yisheng Cai <yisheng.cai@streamnative.io>
- Introduced a new test `TestWithTLSAndWithCACertFile` to verify the behavior of the TLS configuration when using a CA certificate file. - The test checks that the CA certificate is correctly loaded, replaces previous TLS settings, and enforces the minimum TLS version. - Added logic to skip the test if the CA certificate file is not found. Signed-off-by: Yisheng Cai <yisheng.cai@streamnative.io>
- Introduced a new `defaultDialTimeout` constant for connection timeouts in the client pool. - Updated the `WithCACertFile` function to merge CA certificates into existing TLS configurations instead of replacing them, ensuring previous settings are preserved. - Adjusted unit tests to reflect the new behavior of merging TLS configurations when using CA certificates. Signed-off-by: Yisheng Cai <yisheng.cai@streamnative.io>
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
1 participant
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.
CACertFileoption for specifying a CA certificate file for TLS verification.DisableIPv6option to allow users to disable IPv6 resolution, forcing the use of IPv4 addresses.