[DPE-7899] 8.4 - Migrate to TLS v4 (I)#248
[DPE-7899] 8.4 - Migrate to TLS v4 (I)#248sinclert-canonical wants to merge 9 commits into8.4/edgefrom
Conversation
7ae46b1 to
aa07e3e
Compare
| except RuntimeError: | ||
| logger.warning("Unit DNS domain name is not propagated yet") | ||
| return "" |
There was a problem hiding this comment.
This is required so that the TLS class initialization does not crash when the charm does not has any of the expected relations (database, database-peers, replication, replication-offer). This usually happens at the very early stages of a charm deployment, or during unit tests.
There was a problem hiding this comment.
Should we defer instead. I not sure how cert request behave with a "" SAN
There was a problem hiding this comment.
I am not entirely sure how to do so: the function does not received any event to defer upon 🤔
Regardless, these host-names will only be used when the group-replication encryption mode is set to VERIFY_IDENTITY (see docs), which we are far from using. The code is here to mimic PostgreSQL way of dealing with certificate generation.
There was a problem hiding this comment.
Good point. I'd say then to not include the empty str in the SAN set.
There was a problem hiding this comment.
What about moving this try...except one level up then? Not a fan of that return ""
436a30b to
f4c48c0
Compare
paulomach
left a comment
There was a problem hiding this comment.
Nice. Couple open questions, minor catches
| except RuntimeError: | ||
| logger.warning("Unit DNS domain name is not propagated yet") | ||
| return "" |
There was a problem hiding this comment.
Should we defer instead. I not sure how cert request behave with a "" SAN
d4512d1 to
267e79f
Compare
5d69742 to
5a87f5b
Compare
5a87f5b to
5758b15
Compare
|
🗞️ PR branch rebased from |
paulomach
left a comment
There was a problem hiding this comment.
No blocker from my side! Nice work
astrojuanlu
left a comment
There was a problem hiding this comment.
Left a few nits, leaving those to your discretion. Otherwise LGTM!
| — CA file should have a full chain. | ||
| — Key file should have private key. | ||
| — Certificate file should have certificate without certificate chain. |
There was a problem hiding this comment.
| — CA file should have a full chain. | |
| — Key file should have private key. | |
| — Certificate file should have certificate without certificate chain. | |
| - CA file should have a full chain. | |
| - Key file should have private key. | |
| - Certificate file should have certificate without certificate chain. |
?
| except MySQLTLSSetupError: | ||
| logger.error("Failed to enable TLS configuration.") |
There was a problem hiding this comment.
Any problem with retaining the original context for debugging?
| except MySQLTLSSetupError: | |
| logger.error("Failed to enable TLS configuration.") | |
| except MySQLTLSSetupError as exc: | |
| logger.error(f"Failed to enable TLS configuration: {exc}") |
There was a problem hiding this comment.
Or even logger.exception to show the full traceback
| except MySQLTLSSetupError: | ||
| logger.error("Failed to disable TLS configuration.") |
There was a problem hiding this comment.
Same as above:
| except MySQLTLSSetupError: | |
| logger.error("Failed to disable TLS configuration.") | |
| except MySQLTLSSetupError as exc: | |
| logger.error(f"Failed to disable TLS configuration: {exc}") |
This PR migrates the handling of TLS certificates to use version 4 of the
tls-certificatesinterface.The library used to deal with version 4 of such interface was initially published as a charmlib, but then ported to a pure Python package within the charmlibs repository, to be published and consumed via PyPi. Despite the package current version (
1.X.Y), it is shipping version 4 of the interface.Approach:
The new interface version allow us to simplify the current logic by skipping any certificate expiration event handling. This is now managed by the provider automatically, so the only event we need to handle is
CertificateAvailable.Future work: