Harden tests, deduplicate cert/key file writes, and update CI/linting#20
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughBumps CI action versions and golangci-lint; centralizes file-write behavior and nil-safety in testcerts core; modernizes integration tests to use dynamic loopback listeners and ServeTLS with explicit shutdown; updates README examples and a GoDoc comment. ChangesTestcerts Library Improvements and Infrastructure
Sequence DiagramsequenceDiagram
participant TestFullFlow
participant LoopbackListener
participant TLSServer
participant HTTPClient
TestFullFlow->>LoopbackListener: create listener on 127.0.0.1:0
TestFullFlow->>TLSServer: start server.ServeTLS(listener) in goroutine (serverErrCh)
TestFullFlow->>HTTPClient: build Transport with DialContext -> listener address
HTTPClient->>LoopbackListener: DialContext connects
LoopbackListener->>TLSServer: connection handed to ServeTLS
TLSServer-->>HTTPClient: TLS handshake + response
TestFullFlow->>TLSServer: server.Close()
TLSServer-->>TestFullFlow: serverErrCh reports (allow http.ErrServerClosed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 70.81% 73.85% +3.03%
==========================================
Files 3 3
Lines 257 218 -39
==========================================
- Hits 182 161 -21
+ Misses 51 30 -21
- Partials 24 27 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testcerts_test.go (1)
617-655:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a pre-bound ephemeral listener in the example.
Line 617 still binds a fixed port, so this example can fail whenever
127.0.0.1:8443is already occupied. After removing the readiness wait, Line 651 can also race server startup becauseListenAndServeTLSperforms its own bind and serve asynchronously. Reusing thenet.Listen("tcp", "127.0.0.1:0")+ServeTLSpattern fromTestFullFlowwould make the example deterministic and avoid port-collision flakes.Proposed fix
- server := &http.Server{ - Addr: "127.0.0.1:8443", + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + fmt.Printf("Error creating listener - %s", err) + return + } + defer func() { + _ = listener.Close() + }() + + server := &http.Server{ Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if _, writeErr := w.Write([]byte("Hello, World!")); writeErr != nil { fmt.Printf("Error writing response - %s", writeErr) } }), TLSConfig: serverTLSConfig, } defer func() { _ = server.Close() }() go func() { - // Start HTTP Listener - err = server.ListenAndServeTLS(cert.Name(), key.Name()) - if err != nil && err != http.ErrServerClosed { - fmt.Printf("Listener returned error - %s", err) + if serveErr := server.ServeTLS(listener, cert.Name(), key.Name()); serveErr != nil && serveErr != http.ErrServerClosed { + fmt.Printf("Listener returned error - %s", serveErr) } }() @@ client := &http.Client{ Transport: &http.Transport{ TLSClientConfig: clientTLSConfig, + DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) { + var d net.Dialer + return d.DialContext(ctx, network, listener.Addr().String()) + }, }, } // Make an HTTPS request - rsp, err := client.Get("https://localhost:8443") + rsp, err := client.Get("https://localhost")🤖 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 `@testcerts_test.go` around lines 617 - 655, Replace the hard-coded bind and ListenAndServeTLS usage with a pre-bound ephemeral listener: create a net.Listen("tcp","127.0.0.1:0") before starting the server, set server.TLSConfig as before, then start the server using go server.ServeTLS(listener, cert.Name(), key.Name()) (so the OS picks an available port) and defer server.Close(); finally construct the client URL from listener.Addr().String() (e.g. "https://"+listener.Addr().String()) instead of "https://localhost:8443" so client.Get targets the ephemeral port; keep certs.ConfigureTLSConfig and ca.GenerateTLSConfig logic unchanged.
🧹 Nitpick comments (1)
README.md (1)
49-53: ⚡ Quick winAvoid writing to outer
errfrom the goroutine in the README example.The goroutine mutates the outer
err, which is a race-prone pattern in sample code. Prefer a local variable inside the goroutine.Suggested tweak
go func() { - err = http.ListenAndServeTLS("localhost:443", "/tmp/cert", "/tmp/key", someHandler) - if err != nil { + if serveErr := http.ListenAndServeTLS("localhost:443", "/tmp/cert", "/tmp/key", someHandler); serveErr != nil { // do something } }()🤖 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 `@README.md` around lines 49 - 53, The README example spawns a goroutine that writes to the outer variable err, causing a race; update the goroutine to use a local variable (e.g., localErr) for the result of http.ListenAndServeTLS("localhost:443", "/tmp/cert", "/tmp/key", someHandler) and handle/log localErr inside the goroutine instead of assigning to the outer err so no shared mutable state is modified concurrently.
🤖 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 `@kpconfig.go`:
- Line 55: The doc comment for IPNetAddresses uses the incorrect type casing
"Net.IP"; update the comment to use the correct Go type name "net.IP" so it
matches the actual package/type and Go documentation conventions for the
IPNetAddresses function.
In `@testcerts.go`:
- Around line 229-243: The writePairToFiles function currently writes empty
cert/key files silently; update it to validate inputs before writing: check that
certData and keyData are non-empty and contain valid PEM blocks (e.g., parse
pem.Decode on certData and keyData and ensure returned block != nil), return a
clear error if validation fails, and then proceed to os.WriteFile; also ensure
CertificateAuthority.ToFile (which calls writePairToFiles with PublicKey() and
PrivateKey()) propagates these validation errors so callers don't get silent
success when PEM data is missing or invalid.
---
Outside diff comments:
In `@testcerts_test.go`:
- Around line 617-655: Replace the hard-coded bind and ListenAndServeTLS usage
with a pre-bound ephemeral listener: create a net.Listen("tcp","127.0.0.1:0")
before starting the server, set server.TLSConfig as before, then start the
server using go server.ServeTLS(listener, cert.Name(), key.Name()) (so the OS
picks an available port) and defer server.Close(); finally construct the client
URL from listener.Addr().String() (e.g. "https://"+listener.Addr().String())
instead of "https://localhost:8443" so client.Get targets the ephemeral port;
keep certs.ConfigureTLSConfig and ca.GenerateTLSConfig logic unchanged.
---
Nitpick comments:
In `@README.md`:
- Around line 49-53: The README example spawns a goroutine that writes to the
outer variable err, causing a race; update the goroutine to use a local variable
(e.g., localErr) for the result of http.ListenAndServeTLS("localhost:443",
"/tmp/cert", "/tmp/key", someHandler) and handle/log localErr inside the
goroutine instead of assigning to the outer err so no shared mutable state is
modified concurrently.
🪄 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: 5175527d-f4a0-4acb-9101-120851336cb9
📒 Files selected for processing (8)
.github/workflows/go.yaml.github/workflows/lint.yml.golangci.ymlREADME.mdgencerts_test.gokpconfig.gotestcerts.gotestcerts_test.go
There was a problem hiding this comment.
Pull request overview
This PR hardens TLS integration tests against flakiness, deduplicates cert/key file-writing logic, tightens public-API nil safety, refreshes README/example correctness, and updates CI action versions while pinning the linter via a checked-in .golangci.yml.
Changes:
- Test hardening: replace fixed port
0.0.0.0:8443+ 3s sleep with an ephemeral127.0.0.1:0listener,ServeTLS, buffered server error channel, and a customDialContext(with IPv6 bracket handling) for client requests. - API/refactor: extract
writePairToFiles+fileModeconstant, fix the key-file error message, add nil guards onPrivateKey/PublicKey, and letConfigureTLSConfigaccept a nil*tls.Config. - CI/docs: bump
setup-go@v5,checkout@v4,codecov@v5, pingolangci-lint-action@v8tov2.1.6with a new.golangci.yml, renameIPAddressesdoc toIPNetAddresses, and fix README'sGenerateCertsToTempFilename +ConfigureTLSConfigusage and example error handling.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| testcerts.go | Adds fileMode constant, dedupes file writes via writePairToFiles, adds nil guards on PEM getters, and makes ConfigureTLSConfig tolerate a nil tls.Config. |
| testcerts_test.go | Removes fixed listen address / readiness sleep, uses an ephemeral listener with ServeTLS, IPv6-aware URL host, server error channel, and cleans up the now-misnamed example. |
| README.md | Corrects function name, error-check, and ConfigureTLSConfig multi-return usage in the example. |
| kpconfig.go | Renames doc comment for IPNetAddresses to satisfy revive. |
| gencerts_test.go | Skips permission negative tests when running as root on non-Windows hosts. |
| .golangci.yml | New pinned linter config enabling misspell and revive with v2 format. |
| .github/workflows/lint.yml | Bumps checkout to v4 and golangci-lint-action to v8 pinned to v2.1.6. |
| .github/workflows/go.yaml | Bumps setup-go to v5, checkout to v4, and codecov-action to v5. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
623
to
645
| @@ -630,7 +645,11 @@ | |||
| rsp, err := client.Get("https://localhost:8443") | |||
Comment on lines
623
to
629
| go func() { | ||
| // Start HTTP Listener | ||
| err = server.ListenAndServeTLS(cert.Name(), key.Name()) | ||
| if err != nil && err != http.ErrServerClosed { | ||
| fmt.Printf("Listener returned error - %s", err) | ||
| } | ||
| }() |
Comment on lines
+645
to
+652
| rsp, err := client.Get("https://localhost:8443") | ||
| if err != nil { | ||
| fmt.Printf("Client returned error - %s", err) | ||
| return | ||
| } | ||
| defer func() { | ||
| _ = rsp.Body.Close() | ||
| }() |
Comment on lines
+229
to
237
| func writePairToFiles(certData []byte, certFile string, keyData []byte, keyFile string) error { | ||
| if err := os.WriteFile(certFile, certData, fileMode); err != nil { | ||
| return fmt.Errorf("unable to create certificate file - %w", err) | ||
| } | ||
|
|
||
| // Write Key | ||
| err = os.WriteFile(keyFile, ca.PrivateKey(), 0640) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to create certificate file - %w", err) | ||
| if err := os.WriteFile(keyFile, keyData, fileMode); err != nil { | ||
| return fmt.Errorf("unable to create key file - %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
Comment on lines
+352
to
+357
| // ConfigureTLSConfig will configure the tls.Config with the KeyPair certificate and private key. | ||
| // The returned tls.Config can be used for a server or client. | ||
| func (kp *KeyPair) ConfigureTLSConfig(tlsConfig *tls.Config) (*tls.Config, error) { | ||
| if tlsConfig == nil { | ||
| tlsConfig = &tls.Config{} | ||
| } |
Comment on lines
354
to
358
| func (kp *KeyPair) ConfigureTLSConfig(tlsConfig *tls.Config) (*tls.Config, error) { | ||
| if tlsConfig == nil { | ||
| tlsConfig = &tls.Config{} | ||
| } | ||
| cert, err := tls.X509KeyPair(kp.PublicKey(), kp.PrivateKey()) |
| @@ -109,6 +110,9 @@ func TestGeneratingCertsToFile(t *testing.T) { | |||
| }) | |||
|
|
|||
| t.Run("Testing the unhappy path for insufficient permissions", func(t *testing.T) { | |||
| } | ||
|
|
||
| // IPAddresses returns a list of IP addresses in Net.IP format. | ||
| // IPNetAddresses returns a list of IP addresses in Net.IP format. |
Validate cert/key PEM before writing files, clean up orphaned certs on key write failure, and make ExampleNewCA avoid listener races.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
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.
Motivation
Description
127.0.0.1:0, serve withServeTLSon the pre-created listener, avoid sharederrstate, and route client connections to the listener via a customDialContextwhile preserving TLS host/SAN validation and IPv6 bracket handling.time.Aftersleep with listener-driven serve and a buffered server error channel to synchronize lifecycle and capture serve errors.writePairToFileshelper, introducedfileModeconstant (keeps0640), and corrected key-write error text to reportkey filefailures.PrivateKey/PublicKey) and madeConfigureTLSConfigacceptnilinput safely.ExampleNewCAto return on intermediate errors and to close response body to avoid nil derefs and errcheck failures.GenerateCertsToTempFile, proper error checks, TLS config handling).setup-go@v5,checkout@v4,codecov@v5) and pinnedgolangci-lintusage plus added a checked-in.golangci.yml.IPAddressesdoc toIPNetAddressesto satisfy revive.Testing
go test ./...and all unit tests passed.go vet ./...with no issues.go test -race ./...and the race-sensitive tests now pass.make tests(coverage + race) which completed successfully and reported coverage81.5%.make lint(golangci-lint) and no issues were reported.Codex Task
Summary by CodeRabbit
Chores
Refactor
Documentation
Tests