Use multiple shorter timeout requests to check for ssl cert#2140
Use multiple shorter timeout requests to check for ssl cert#2140edwardrf wants to merge 1 commit into
Conversation
To prevent cert verification timeout
📝 WalkthroughWalkthroughCheckTLSCert is refactored to resolve domain IPs and probe each separately using a new checkOne helper with per-attempt timeouts enforced at both context and HTTP client levels. Response bodies are drained to enable connection reuse. The fixed-IP transport gains a 5-second ResponseHeaderTimeout. Tests verify timeout firing, error propagation, IP pinning, and transport configuration. ChangesPer-Attempt Timeout Refactoring and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/cert/check_test.go`:
- Around line 114-123: The test TestCheckTLSCert_NoIPsReturnsNil and
CheckTLSCert behavior are wrong: an empty DNS A-record result must not be
treated as TLS-ready success. Update CheckTLSCert to add an explicit guard that
detects an empty IP list from the resolver and returns a non-nil error (e.g.,
"no IPs found" or a typed error) before attempting any probe; then update
TestCheckTLSCert_NoIPsReturnsNil to expect that error (i.e., assert non-nil)
rather than nil so waitForTLS callers won't prematurely stop. Ensure references
to the resolver response handling and the probe logic in CheckTLSCert are the
places you change.
In `@src/pkg/cert/check.go`:
- Around line 26-27: The error returns from calls to checkOne currently discard
domain/IP context (e.g. the if err := checkOne(ctx, domain, ip.String()); err !=
nil { return err } lines and similar blocks at the other locations), so change
those returns to wrap the original error with domain and IP information using
fmt.Errorf("... domain=%s ip=%s: %w", domain, ipStr, err) (or errors.Wrap
equivalent); update imports if needed and apply this pattern to all occurrences
(the checkOne calls at the shown site and the other two blocks referenced).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c665b822-1515-40e7-8cd9-b7f3db7762d4
📒 Files selected for processing (2)
src/pkg/cert/check.gosrc/pkg/cert/check_test.go
| func TestCheckTLSCert_NoIPsReturnsNil(t *testing.T) { | ||
| // Defensive: if the resolver returns no A records we should not loop, | ||
| // not error, and not panic. The cert command will retry on its 3s | ||
| // ticker and pick up the records once they propagate. | ||
| resolver := dns.MockResolver{Records: map[dns.DNSRequest]dns.DNSResponse{ | ||
| {Type: "A", Domain: "empty.example"}: {Records: nil}, | ||
| }} | ||
| if err := CheckTLSCert(context.Background(), "empty.example", resolver); err != nil { | ||
| t.Fatalf("expected nil for empty IP list, got %v", err) | ||
| } |
There was a problem hiding this comment.
NoIPs should not be treated as TLS-ready success.
Returning/expecting nil for an empty IP set can prematurely end readiness loops, because waitForTLS callers stop immediately on CheckTLSCert == nil.
Suggested update
-func TestCheckTLSCert_NoIPsReturnsNil(t *testing.T) {
+func TestCheckTLSCert_NoIPsReturnsError(t *testing.T) {
@@
- if err := CheckTLSCert(context.Background(), "empty.example", resolver); err != nil {
- t.Fatalf("expected nil for empty IP list, got %v", err)
- }
+ if err := CheckTLSCert(context.Background(), "empty.example", resolver); err == nil {
+ t.Fatal("expected error for empty IP list, got nil")
+ }
}And in src/pkg/cert/check.go, add an explicit empty-result guard before probing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/cert/check_test.go` around lines 114 - 123, The test
TestCheckTLSCert_NoIPsReturnsNil and CheckTLSCert behavior are wrong: an empty
DNS A-record result must not be treated as TLS-ready success. Update
CheckTLSCert to add an explicit guard that detects an empty IP list from the
resolver and returns a non-nil error (e.g., "no IPs found" or a typed error)
before attempting any probe; then update TestCheckTLSCert_NoIPsReturnsNil to
expect that error (i.e., assert non-nil) rather than nil so waitForTLS callers
won't prematurely stop. Ensure references to the resolver response handling and
the probe logic in CheckTLSCert are the places you change.
| if err := checkOne(ctx, domain, ip.String()); err != nil { | ||
| return err |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Wrap probe-path errors with domain/IP context.
These new network-path returns lose caller context (domain, ip) and make retry diagnostics much harder.
Suggested patch
import (
"context"
+ "fmt"
"io"
"net"
"net/http"
"time"
@@
func CheckTLSCert(ctx context.Context, domain string, resolver dns.Resolver) error {
ips, err := resolver.LookupIPAddr(ctx, domain)
if err != nil {
- return err
+ return fmt.Errorf("lookup A records for %q: %w", domain, err)
}
for _, ip := range ips {
if err := checkOne(ctx, domain, ip.String()); err != nil {
return err
}
@@
req, err := http.NewRequestWithContext(attemptCtx, http.MethodGet, "https://"+domain, nil)
if err != nil {
- return err
+ return fmt.Errorf("build TLS probe request for %q via %s: %w", domain, ip, err)
}
@@
resp, err := httpClient.Do(req)
if err != nil {
- return err
+ return fmt.Errorf("TLS probe failed for %q via %s: %w", domain, ip, err)
}As per coding guidelines: “Do not silently swallow real errors; return wrapped errors for filesystem, parsing, network, and provider failures”.
Also applies to: 37-39, 45-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pkg/cert/check.go` around lines 26 - 27, The error returns from calls to
checkOne currently discard domain/IP context (e.g. the if err := checkOne(ctx,
domain, ip.String()); err != nil { return err } lines and similar blocks at the
other locations), so change those returns to wrap the original error with domain
and IP information using fmt.Errorf("... domain=%s ip=%s: %w", domain, ipStr,
err) (or errors.Wrap equivalent); update imports if needed and apply this
pattern to all occurrences (the checkOne calls at the shown site and the other
two blocks referenced).
3 participants
To prevent cert verification timeout
Checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests