[BUGFIX] use client config for tls dialer #2938
Conversation
📝 WalkthroughWalkthroughThe pull request updates the TLS configuration instantiation in several internal configuration files. In each Changes
Sequence Diagram(s)Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[WARNING:INTCFG] Changes in |
Deploying vald with
|
| Latest commit: |
e37f4d5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fda0de9f.vald.pages.dev |
| Branch Preview URL: | https://bugfix-internal-tls-use-clie.vald.pages.dev |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
area/internal
size/S
type/bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2938 +/- ##
=======================================
Coverage 17.79% 17.79%
=======================================
Files 124 124
Lines 14208 14208
=======================================
Hits 2528 2528
Misses 11388 11388
Partials 292 292 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kpango
force-pushed
the
bugfix/internal-tls/use-client-config-for-dialer
branch
from
5c904a3 to
c960b5f
Compare
Signed-off-by: kpango <kpango@vdaas.org>
kpango
force-pushed
the
bugfix/internal-tls/use-client-config-for-dialer
branch
from
c960b5f to
4a7b41c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tls/tls.go (1)
124-124: Added defensive nil check for certificate contentThe additional check for
c != nilprovides an extra safety layer, although there's already a check on line 116 that returns ifc == nil.Since there's already an error check at line 116 that returns if
c == nil, the redundant check could be removed to simplify the code:- if c != nil && !pool.AppendCertsFromPEM(c) { + if !pool.AppendCertsFromPEM(c) { err = errors.ErrCertificationFailed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/config/cassandra.go(1 hunks)internal/config/grpc.go(1 hunks)internal/config/mysql.go(1 hunks)internal/config/net.go(1 hunks)internal/config/redis.go(1 hunks)internal/tls/tls.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/net.go
- internal/config/redis.go
- internal/config/cassandra.go
- internal/config/grpc.go
- internal/config/mysql.go
⏰ Context from checks skipped due to timeout of 90000ms (59)
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: runner / go build
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (1)
internal/tls/tls.go (1)
81-110:✅ Verification successful
Verified that the client config function properly handles certificates
The
NewClientConfigfunction now handles both CA certificates and client certificates with proper error checking. This change aligns with the PR objective to fix TLS dialer configuration by using proper client configuration.Let's check how this function is being used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Check how the NewClientConfig function is being used in the codebase rg "tls\.NewClientConfig" --type goLength of output: 1186
Final Verification: The
NewClientConfigfunction correctly handles both CA and client certificates as intended.The updated implementation properly checks for errors and chooses between CA certificate and client certificate configuration. Additionally, our search confirms that the function is invoked in multiple parts of the codebase (e.g., in
internal/net/dialer_test.goand various configuration files), and there are no inconsistencies with its usage.
| } else if c.cert != "" { | ||
| pool, err := NewX509CertPool(c.cert) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c.cfg.RootCAs = pool |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhanced flexibility for TLS client configuration
This change allows using the certificate file as a root CA source when a specific CA file isn't provided. This is a good improvement for scenarios where self-signed certificates are used or when the same certificate serves multiple purposes.
Consider adding a comment explaining this fallback behavior to improve code maintainability:
} else if c.cert != "" {
+ // Fallback to using the cert file for RootCAs when CA file is not provided
pool, err := NewX509CertPool(c.cert)
if err != nil {
return nil, err
}
c.cfg.RootCAs = pool
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if c.cert != "" { | |
| pool, err := NewX509CertPool(c.cert) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.cfg.RootCAs = pool | |
| } else if c.cert != "" { | |
| // Fallback to using the cert file for RootCAs when CA file is not provided | |
| pool, err := NewX509CertPool(c.cert) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.cfg.RootCAs = pool | |
| } |
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit