[DPE-8320] Fix backups with internal certificates #1162
Conversation
github-actions
bot
added
the
Libraries: Out of sync
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (64.51%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## 16/edge #1162 +/- ##
===========================================
- Coverage 64.57% 64.51% -0.07%
===========================================
Files 17 17
Lines 4328 4337 +9
Branches 669 671 +2
===========================================
+ Hits 2795 2798 +3
- Misses 1351 1357 +6
Partials 182 182 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dragomirp
force-pushed
the
dpe-8320-internal-peer-backups
branch
from
ff8127c to
847d51a
Compare
|
|
||
| if not self.get_secret(APP_SCOPE, "internal-ca"): | ||
| self.tls.generate_internal_peer_ca() | ||
| self.tls.generate_internal_peer_cert() |
There was a problem hiding this comment.
If the internal cert generates before the IP is set to the peer data, the hostname will be used as common name. It also causes issues on Juju 4.
| None, | ||
| None, | ||
| None, |
There was a problem hiding this comment.
Test on microceph with the internal certs, without using a TLS operator.
| def _get_peer_common_name(self) -> str: | ||
| return self.charm.unit_peer_data.get("database-peers-address") or self.host |
There was a problem hiding this comment.
I think we should use the peers address here, in case it's different from the relation address.
There was a problem hiding this comment.
Makes sense, as we might have different Juju spaces and for the purpose of using the certificates for backup on replicas, what makes sense is the peer address.
| try: | ||
| if raw_cert := self.get_secret(UNIT_SCOPE, "internal-cert"): | ||
| cert = load_pem_x509_certificate(raw_cert.encode()) | ||
| if ( | ||
| cert.subject.get_attributes_for_oid(NameOID.COMMON_NAME)[0].value | ||
| != self._unit_ip | ||
| ): | ||
| self.tls.generate_internal_peer_cert() | ||
| except Exception: | ||
| logger.exception("Unable to check or update internal cert") |
There was a problem hiding this comment.
Regenerate the cert if the common name is not the IP before the upgrade.
dragomirp
changed the title
dragomirp
requested review from
a team,
marceloneppel and
taurus-forever
and removed request for
a team
taurus-forever
left a comment
taurus-forever
left a comment
There was a problem hiding this comment.
LGTM, please add one more message as we are going to regenerate cert.
| cert.subject.get_attributes_for_oid(NameOID.COMMON_NAME)[0].value | ||
| != self._unit_ip | ||
| ): | ||
| self.tls.generate_internal_peer_cert() |
There was a problem hiding this comment.
Please add INFO-like message to log to see when cert is regenerated (to simplify production troubleshooting). Warning is also fine.
There was a problem hiding this comment.
Added info log inside generate_internal_peer_cert().
github-actions
bot
added
Libraries: OK
| def _get_peer_common_name(self) -> str: | ||
| return self.charm.unit_peer_data.get("database-peers-address") or self.host |
There was a problem hiding this comment.
Makes sense, as we might have different Juju spaces and for the purpose of using the certificates for backup on replicas, what makes sense is the peer address.
* Wait for ip to generate leader cert * Regenerate cert if common name is host * Add info message on internal cert generation
* Wait for ip to generate leader cert * Regenerate cert if common name is host * Add info message on internal cert generation
Checklist