Delegate certificate reneawl to certapi, other fixes#50
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SSL certificate management system by replacing the internal SSL class with a RenewalManager from the certapi package and introducing a centralized certificate_backend factory. It also enhances the handling of injected Nginx directives by tracking them per backend, enabling precise cleanup when containers are removed. The VIRTUAL_HOST parsing logic was updated to support key=value syntax. Key feedback points include a missing reload mechanism in the background renewal process, the brittle use of protected internal methods, potential parsing conflicts with complex Nginx directives containing equal signs, and the use of Python 3.10+ type hint syntax which may break compatibility with older versions.
| self.renewal_manager = RenewalManager( | ||
| self.backend, | ||
| sync_watch_domains=self.sync_watch_domains, | ||
| renew_threshold_days=max(1, int(self.update_threshold_secs // (24 * 3600))), | ||
| batch_domains=self.certapi_batch_domains, | ||
| ) |
There was a problem hiding this comment.
The RenewalManager is initialized without a reload callback. In the previous implementation (SSL.py), the background thread explicitly called server.reload() after renewing certificates. Without a similar mechanism here (e.g., passing a reload callback to RenewalManager if supported by the certapi client), Nginx will not pick up renewed or newly issued certificates until an unrelated event triggers a reload. This could lead to expired certificates being served even after successful background renewal.
| register_self_signed = getattr(self.renewal_manager, "_register_self_signed", None) | ||
| if register_self_signed is None: | ||
| return | ||
| register_self_signed(domain) |
There was a problem hiding this comment.
Accessing the protected method _register_self_signed of RenewalManager via getattr is brittle and violates encapsulation. If certapi is an external dependency, this code may break if the internal implementation of RenewalManager changes. It is recommended to use a public API if available, or expose this functionality properly in the RenewalManager class.
| if "=" in directive: | ||
| key, value = directive.split("=", 1) | ||
| key = key.strip() | ||
| value = value.strip() | ||
| if key: | ||
| return key, value if value else None |
There was a problem hiding this comment.
Splitting directives by = in _parse_extra_directive can mangle complex Nginx directives that contain equal signs within their values (e.g., in set, map, or complex proxy_set_header directives). While this supports a convenient key=value syntax for simple cases, it risks breaking valid Nginx configurations. Consider a more robust parser or restricting this behavior to known safe keys.
| cert_manager: AcmeCertManager | None = None | ||
| certapi_client: CertManagerClient | None = None | ||
| challenge_store: NginxChallengeSolver | None = None |
There was a problem hiding this comment.
The use of the | operator for type unions (e.g., AcmeCertManager | None) requires Python 3.10+. The rest of the codebase (e.g., Location.py) uses typing.Union, suggesting compatibility with older Python versions is intended. If support for Python < 3.10 is required, these type hints should be updated to use Optional or Union from the typing module.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 59.87% 61.32% +1.45%
==========================================
Files 31 30 -1
Lines 2557 2384 -173
Branches 417 382 -35
==========================================
- Hits 1531 1462 -69
+ Misses 920 829 -91
+ Partials 106 93 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.