search

canonical / glauth-k8s-operator

A Charmed Operator for running GLAuth on Kubernetes
https://charmhub.io/glauth-k8s
Apache License 2.0
0 stars 0 forks source link

feat: tls-certificates & certificate-transfer interface integrations #20

Closed wood-push-melon closed 9 months ago

wood-push-melon commented 9 months ago

This pull request tries to implement the integration with tls-certificates interface and certificate-transfer interface.

natalian98 commented 9 months ago

Shall we add this piece of config to the template?

wood-push-melon commented 9 months ago
  1. On config_changed you need to update the CertHandler config with the new hostname
  2. I have mixed feelings about the integrations.py file. It makes sense for removing some of the logic from the charm.py file, but at the same time it feels like it beats the purpose of having a lib. Libs should provide as much functionality as possible only through config. For example, if CertificateTransferProvides provided a function for updating all the relations with one call, you wouldn't need to wrap it. In addition, it makes each charm implement different patterns (especially across teams) and I think that we should strive for a uniform approach across charms. Having said that this is a nitpick and I have no problem with moving on with the current implementation, until we can better evaluate this approach.

I am confused with the first one. The CertHandler constructor is fed with the most recent juju config value.

For the second, the integrations.py's purpose is not to replace/overlap the juju libraries. It's to wrap part of extra logic involved with juju libraries and expose some APIs for charms to use so that the charm.py does not need to worry too much about the logic which it does not need to. In some case that there isn't much logic to do when dealing with the integrations, simply using the API provided by juju library is good enough.

wood-push-melon commented 9 months ago

Shall we add this piece of config to the template?

This task is in the scope of https://warthogs.atlassian.net/browse/IAM-621. It needs us to decide if we want the StartTLS in mandatory. I will leave some comments on the ticket.

wood-push-melon commented 9 months ago

Another question: Shouldn't the glauth config be updated to use the cert after it is recieved?

I think this is related to another ticket's task: https://warthogs.atlassian.net/browse/IAM-621. And it needs us to make some decisions.