Add optional TLS support #340
Conversation
WalkthroughAdds two CLI flags (--tls-cert-file, --tls-key-file), enforces both-or-none, chooses default listen port (:8443 when TLS, :8080 otherwise), and conditionally starts the server with ListenAndServeTLS when both TLS files are provided; exits on TLS misconfiguration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as llama-swap (main)
participant S as HTTP(S) Server
U->>M: Run binary with flags
M->>M: Parse flags
alt Exactly one TLS flag provided
M->>U: Log error: both TLS flags required
M-->>U: Exit (fatal)
else Both TLS flags provided
M->>S: Start ListenAndServeTLS(certFile, keyFile) on :8443
M->>U: Log TLS URL (https://host:8443)
else No TLS flags
M->>S: Start ListenAndServe on :8080
M->>U: Log HTTP URL (http://host:8080)
end
note over S,M: Server runs until error or shutdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
llama-swap.go (1)
44-48: Consider validating file existence and readability.The XOR logic correctly ensures both TLS flags are provided together. However, consider adding validation that the certificate and key files exist and are readable before proceeding. This would fail fast with a clear error message rather than deferring the error until
ListenAndServeTLSis called.Apply this diff to add early file validation:
if (*certFile != "" && *keyFile == "") || (*certFile == "" && *keyFile != "") { fmt.Println("Error: Both --tls-cert-file and --tls-key-file must be provided for TLS.") os.Exit(1) } + + // Validate TLS files exist and are readable + if *certFile != "" { + if _, err := os.Stat(*certFile); err != nil { + fmt.Printf("Error: TLS certificate file not accessible: %v\n", err) + os.Exit(1) + } + if _, err := os.Stat(*keyFile); err != nil { + fmt.Printf("Error: TLS key file not accessible: %v\n", err) + os.Exit(1) + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
llama-swap.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
llama-swap.go
🔇 Additional comments (2)
llama-swap.go (2)
32-33: LGTM! Clean flag definitions.The TLS flag definitions follow the existing pattern and use appropriate defaults.
179-190: LGTM! Clean conditional TLS startup.The implementation correctly:
- Uses separate code paths for TLS and non-TLS startup
- Logs clear, protocol-appropriate URLs
- Handles errors properly by treating all errors except
http.ErrServerClosedas fatalThe redundant check at line 180 (given the earlier validation at lines 44-48) is harmless defensive programming.
|
I believe this works for the UI, but for I will update the PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
llama-swap.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
llama-swap.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (2)
llama-swap.go (2)
31-33: LGTM!The flag definitions follow Go conventions and appropriately use empty defaults since TLS usage is validated later.
190-198: Verify TLS configuration for llama-server component.The server startup logic is correct, but according to your PR comment, the llama-server component requires setting
srv.TLSConfig = &tls.Config{NextProtos: []string{"http/1.1"}}before callingListenAndServeTLS. Please confirm this has been addressed or is planned for the update you mentioned.If this configuration is still needed, apply this diff before line 194:
go func() { var err error if useTLS { + srv.TLSConfig = &tls.Config{NextProtos: []string{"http/1.1"}} fmt.Printf("llama-swap listening with TLS on https://%s\n", *listenStr) err = srv.ListenAndServeTLS(*certFile, *keyFile)
|
I am looking into using httputil.ReverseProxy. I'll continue this PR after I finish exploring that option. |
|
@dwrz anything that needs to be revisited in this PR? |
Introduce HTTPS support with net/http Server.ListenAndServeTLS. This should enable the option of serving via HTTPS without a reverse proxy. Add two flags: - tls-cert-file (path to the TLS certificate file) - tls-key-file (path to the TLS private key file) Both flags must be supplied together; otherwise exit with error. If both flags are present, call srv.ListenAndServeTLS. If not, fall back to the existing srv.ListenAndServe (HTTP); no changes to existing non‑TLS behavior.
If both tls-cert-file and tls-key-file are set, assume the intent to use TLS. If TLS is enabled, default to port 8443. Otherwise, 8080.
useTLS was set to true in the case that no certFile or keyFile was set.
|
@mostlygeek -- On the Either way -- thank you for your time and help with the issues. I hope other |
2 participants
Introduce HTTPS support with net/http Server.ListenAndServeTLS.
This should enable the option of serving via HTTPS without a reverse proxy.
Add two flags:
Both flags must be supplied together; otherwise exit with error.
If both flags are present, call srv.ListenAndServeTLS. If not, fall back to the existing srv.ListenAndServe (HTTP); no changes to existing non‑TLS behavior.
Summary by CodeRabbit
New Features
Bug Fixes