🔥 feat: Add support for AutoTLS / ACME #3201
Conversation
|
Warning Rate limit exceeded@gaby has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces enhancements to the Fiber framework by adding support for automatic TLS certificate management through the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Can we add several test-cases using https://letsencrypt.org/tr/docs/staging-environment/? |
Of course, do you think this change will meet the demand? |
gaby
changed the title
I think it can. Otherwise it might be challenging to test the functionality. |
can't passed and not chack the file yet
|
|
After the PR is merged, I think we could also update gofiber/recipes under the autocert recipe to use this feature. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
listen_test.go(3 hunks)
🔇 Additional comments (3)
listen_test.go (3)
22-23: LGTM: Required imports for ACME functionality
The imports are correctly added and necessary for implementing AutoTLS support.
320-322: 🛠️ Refactor suggestion
Improve temporary directory handling in prefork test
Similar to the non-prefork test, use a system temp directory for better test isolation:
-dir, err := os.MkdirTemp(".", "certs")
+dir, err := os.MkdirTemp("", "fiber-acme-prefork-test-*")Likely invalid or redundant comment.
270-281: Add certificate verification
The test should verify that certificates are being properly managed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/api/fiber.md (1)
170-187: Enhance the AutoTLS documentation.While the example is good, it could be improved by:
- Adding error handling for the certificate manager setup
- Including staging environment configuration for testing
- Documenting common pitfalls and requirements (e.g., port 80 must be available for ACME HTTP challenge)
Here's a suggested enhancement to the documentation:
#### TLS AutoCert support (ACME / Let's Encrypt) Provides automatic access to certificates management from Let's Encrypt and any other ACME-based providers. + +:::note +Requirements: +- Port 80 must be available for the ACME HTTP challenge +- Domain must be publicly accessible +- Write permissions for the certificate cache directory +::: ```go title="Examples" +// For testing, use Let's Encrypt staging environment +staging := autocert.Manager{ + Prompt: autocert.AcceptTOS, + HostPolicy: autocert.HostWhitelist("example.com"), + Cache: autocert.DirCache("./certs"), + Client: &acme.Client{ + DirectoryURL: "https://acme-staging-v02.api.letsencrypt.org/directory", + }, +} + // Certificate manager certManager := &autocert.Manager{ Prompt: autocert.AcceptTOS, // Replace with your domain name HostPolicy: autocert.HostWhitelist("example.com"), // Folder to store the certificates Cache: autocert.DirCache("./certs"), } + +// Handle certificate manager errors +go func() { + log.Fatal(certManager.HTTPHandler(nil)) +}() app.Listen(":444", fiber.ListenConfig{ AutoCertManager: certManager, -})) +})</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2996935703b4e6f25bbfe174491d9de850c95955 and b594117e6a8f6e3fd748fcbdfe0e222e6f876801. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/api/fiber.md` (2 hunks) * `listen.go` (3 hunks) </details> <details> <summary>🔇 Additional comments (4)</summary> <details> <summary>listen.go (3)</summary> `26-26`: **LGTM!** The import of the `autocert` package is required for the new AutoTLS functionality. --- `74-78`: **LGTM!** The `AutoCertManager` field is well-documented and properly typed. The JSON tag is correctly set. --- `194-199`: **Consider adding support for custom TLS versions.** The TLS configuration is correct, but the hardcoded `tls.VersionTLS12` might be too restrictive for some users. Consider making this configurable. ```shell #!/bin/bash # Description: Check if there are any existing TLS version configurations in the codebase # Search for TLS version configurations rg -A 2 'MinVersion.*=.*tls\.Version'Note: I see this has been discussed in past review comments and issue #3239 has been created to track this enhancement.
docs/api/fiber.md (1)
117-117: LGTM!The new
OnShutdownSuccessandAutoCertManagerfields are properly documented with clear descriptions and default values.Also applies to: 119-119
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/whats_new.md (1)
923-923: Fix grammatical error in documentation.Change "It should not be invoke concurrently" to "It should not be invoked concurrently".
🧰 Tools
🪛 LanguageTool
[grammar] ~923-~923: Consider using either the past participle “invoked” or the present participle “invoking” here.
Context: ...y in development mode. It should not be invoke concurrently. ### 🧠 Context Fiber v3...(BEEN_PART_AGREEMENT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/whats_new.md(33 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md
[grammar] ~923-~923: Consider using either the past participle “invoked” or the present participle “invoking” here.
Context: ...y in development mode. It should not be invoke concurrently. ### 🧠 Context Fiber v3...
(BEEN_PART_AGREEMENT)
🔇 Additional comments (3)
docs/whats_new.md (3)
178-193: LGTM! Well-structured route chaining example.
The code example effectively demonstrates the new route chaining API with proper error handling and JSON responses.
212-213: LGTM! Clear middleware registration example.
The code example correctly demonstrates middleware registration for multiple prefixes.
464-477: LGTM! Well-documented generic functions examples.
The code examples for Convert, Params, Query, and GetReqHeader functions are clear, correct, and demonstrate proper error handling.
Also applies to: 543-554, 575-586, 608-619
|
I need to run the tests locally, something is crashing when we hit the staging server |
After testing multiple times, locally and in CI. That staging server is for testing with a domain you own. When running with Even the official google package doesn't have a test for this. We can't add one either. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3201 +/- ##
==========================================
- Coverage 82.84% 82.81% -0.03%
==========================================
Files 115 115
Lines 11295 11302 +7
==========================================
+ Hits 9357 9360 +3
- Misses 1539 1543 +4
Partials 399 399
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
listen.go (1)
74-78: Enhance field documentation with usage examplesWhile the documentation explains the purpose, it would be more helpful to include:
- A basic usage example
- Required configuration for Let's Encrypt
- Common pitfalls to avoid
Consider expanding the documentation like this:
// AutoCertManager manages TLS certificates automatically using the ACME protocol, // Enables integration with Let's Encrypt or other ACME-compatible providers. +// +// Example usage with Let's Encrypt: +// manager := &autocert.Manager{ +// Prompt: autocert.AcceptTOS, +// HostPolicy: autocert.HostWhitelist("example.com"), +// Cache: autocert.DirCache("certs"), +// } +// app.Listen(":443", fiber.ListenConfig{ +// AutoCertManager: manager, +// }) // // Default: nil AutoCertManager *autocert.Manager `json:"auto_cert_manager"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
listen.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
listen.go
[warning] 195-199: listen.go#L195-L199
Added lines #L195 - L199 were not covered by tests
🔇 Additional comments (3)
listen.go (3)
26-26: LGTM: Import of autocert package
The import of the standard Go ACME/autocert package is appropriate for implementing AutoTLS support.
194-199: Verify TLS configuration customization
The TLS configuration for AutoTLS should be customizable through the TLSConfigFunc. While the base configuration is correct, users might need to:
- Adjust security parameters
- Add custom protocols
- Configure cipher suites
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-199: listen.go#L195-L199
Added lines #L195 - L199 were not covered by tests
194-199:
Add test coverage for AutoTLS configuration
The AutoTLS configuration block lacks test coverage. Consider adding tests using Let's Encrypt's staging environment to verify:
- Certificate acquisition
- TLS configuration
- Protocol negotiation
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-199: listen.go#L195-L199
Added lines #L195 - L199 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
docs/whats_new.md (5)
197-212: Fix indentation in the route chaining example.The indentation of the chained methods is inconsistent with Go's standard formatting.
Apply this formatting:
app.Route("/api").Route("/user/:id?") - .Get(func(c fiber.Ctx) error { - // Get user - return c.JSON(fiber.Map{"message": "Get user", "id": c.Params("id")}) - }) - .Post(func(c fiber.Ctx) error { - // Create user - return c.JSON(fiber.Map{"message": "User created"}) - }) + .Get(func(c fiber.Ctx) error { + // Get user + return c.JSON(fiber.Map{"message": "Get user", "id": c.Params("id")}) + }) + .Post(func(c fiber.Ctx) error { + // Create user + return c.JSON(fiber.Map{"message": "User created"}) + })
264-271: Add error handling to the test configuration example.The example should demonstrate proper error handling.
Add error handling:
req := httptest.NewRequest(MethodGet, "/", nil) testConfig := fiber.TestConfig{ Timeout: 0, FailOnTimeout: false, } // Test the handler using the request and testConfig -resp, err := app.Test(req, testConfig) +resp, err := app.Test(req, testConfig) +if err != nil { + // Handle error + log.Fatal(err) +} +defer resp.Body.Close()
483-496: Enhance generic functions examples with error handling and type constraints.The examples could be improved to show best practices.
Here's an improved version of the Convert example:
app.Get("/convert", func(c fiber.Ctx) error { - value, err := Convert[string](c.Query("value"), strconv.Atoi, 0) + // Add type constraint + value, err := Convert[int, constraints.Integer]( + c.Query("value"), + strconv.Atoi, + 0, + ) if err != nil { - return c.Status(fiber.StatusBadRequest).SendString(err.Error()) + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{ + "error": "Invalid value: " + err.Error(), + }) } - return c.JSON(value) + return c.JSON(fiber.Map{"value": value}) })Also applies to: 562-571, 594-603, 627-638
Line range hint
922-942: Enhance RebuildTree documentation with production usage warnings.The documentation should emphasize the risks of using this feature in production.
Add these warnings:
Note: Use this method with caution. It is **not** thread-safe and can be very performance-intensive. Therefore, it should be used sparingly and primarily in development mode. It should not be invoke concurrently. + +**⚠️ Production Warning:** +- This method rebuilds the entire route tree, which can be CPU-intensive +- Avoid using this in production during high-traffic periods +- Consider implementing a maintenance window for route updates +- Use feature flags or environment-based conditions to control when rebuilding is allowed
942-942: Fix grammar in the RebuildTree note.The word "invoke" should be "invoked" in this context.
-It should not be invoke concurrently. +It should not be invoked concurrently.🧰 Tools
🪛 LanguageTool
[grammar] ~942-~942: Consider using either the past participle “invoked” or the present participle “invoking” here.
Context: ...y in development mode. It should not be invoke concurrently. ### 🧠 Context Fiber v3...(BEEN_PART_AGREEMENT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
docs/whats_new.md(34 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md
[grammar] ~942-~942: Consider using either the past participle “invoked” or the present participle “invoking” here.
Context: ...y in development mode. It should not be invoke concurrently. ### 🧠 Context Fiber v3...
(BEEN_PART_AGREEMENT)
🔇 Additional comments (1)
docs/whats_new.md (1)
231-232: LGTM!
The middleware registration example clearly demonstrates the new syntax for registering multiple prefixes and subapps.
Also applies to: 238-238
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Reduce the boilerplate of setting up AutoTLS / ACME by adding built-in support into Fiber. Whether this impl is good?
Fixes #3155
Changes introduced
Type of change
Please delete options that are not relevant.