feat(ssl): Add per-certificate renewal and auto-renewal#116
Conversation
Certificates can now be renewed individually by domain, or in bulk for all domains belonging to a deployment. Each certificate tracks its own auto-renew preference, and a background worker automatically renews certificates approaching expiry. Certificates listed in the API are linked back to their owning deployment.
Code Review SummaryThis PR introduces individual certificate renewal, auto-renewal capabilities, and better API linking between certificates and deployments. It includes a background worker to handle certificates approaching expiration. 🚀 Key Improvements
💡 Minor Suggestions
|
| if s.config.Certbot.Enabled && s.config.Certbot.AutoRenewalEnabled { | ||
| s.certRenewer = ssl.NewRenewer( | ||
| s.proxyOrchestrator.SSLManager(), | ||
| s.config.Certbot.RenewalThresholdDays, | ||
| s.config.Certbot.RenewalCheckInterval, | ||
| func(domain string) { | ||
| if err := s.proxyOrchestrator.NginxManager().Reload(); err != nil { | ||
| log.Printf("auto-renew: failed to reload nginx after %s: %v", domain, err) | ||
| } | ||
| }, | ||
| ) | ||
| s.certRenewer.Start(context.Background()) |
There was a problem hiding this comment.
Starting a background worker in ListenAndServe (which is typically called in a goroutine or as a blocking call) is okay, but context.Background() here means the renewer isn't bound to the application's shutdown context. If the server is stopped, the loop might persist until the process exits.
| if s.config.Certbot.Enabled && s.config.Certbot.AutoRenewalEnabled { | |
| s.certRenewer = ssl.NewRenewer( | |
| s.proxyOrchestrator.SSLManager(), | |
| s.config.Certbot.RenewalThresholdDays, | |
| s.config.Certbot.RenewalCheckInterval, | |
| func(domain string) { | |
| if err := s.proxyOrchestrator.NginxManager().Reload(); err != nil { | |
| log.Printf("auto-renew: failed to reload nginx after %s: %v", domain, err) | |
| } | |
| }, | |
| ) | |
| s.certRenewer.Start(context.Background()) | |
| s.certRenewer.Start(context.Background()) // Consider passing a context that is cancelled on app shutdown |
| return nil | ||
| } | ||
|
|
||
| f, err := os.Create(marker) |
There was a problem hiding this comment.
When disabling auto-renew, the code creates an empty file. It is good practice to ensure the file is closed immediately to release the descriptor, which you are doing, but adding a check to see if it already exists before creating could avoid unnecessary IO.
| f, err := os.Create(marker) | |
| if _, err := os.Stat(marker); os.IsNotExist(err) { | |
| f, err := os.Create(marker) | |
| if err != nil { | |
| return fmt.Errorf("failed to disable auto-renew: %w", err) | |
| } | |
| return f.Close() | |
| } | |
| return nil |
Certificates can now be renewed individually by domain, or in bulk for all domains belonging to a deployment. Each certificate tracks its own auto-renew preference, and a background worker automatically renews certificates approaching expiry. Certificates listed in the API are linked back to their owning deployment.