search

canonical / istio-operators

Charmed Istio
2 stars 17 forks source link

feat: add `csr-domain-name` config option #381

Closed DnPlas closed 4 months ago

DnPlas commented 4 months ago

feat: enable csr-domain-name config option so istio-pilot can use it on CSRs

The istio-pilot charm already has a mechanism in place to discover the ingress gateway address from the Service, but it is limited to only returning IP addresses, which not all TLS certificate providers accept as a valid cert subject. Having the domain-name config option will allow users to specify the domain name they'd like to use when integrating with TLS certificate operators. This feature expands the support for integrating with TLS certificate providers that cannot issue signed certificates on a CSR that only contains an IP address (like we used to do). This commit also adds some test coverage to test the recently added code.

Fixes #379

DnPlas commented 4 months ago

I expect the CI to fail until we merge #382

jnsgruk commented 4 months ago

Can we line up the name of the config option with that of Traefik etc? (I think they use external_hostname or something like that?)

DnPlas commented 4 months ago

Can we line up the name of the config option with that of Traefik etc? (I think they use external_hostname or something like that?)

Hey @jnsgruk the reason why I did not want to use the same name as them is because traefik actually uses the external hostname to configure the traefik provider in a more explicit way, while in istio it is done more implicitly (by the LB configuration, for instance). I did not want users to assume this option was the same as we are limiting it to just use it for TLS configuration purposes.

I could see us having a more explicit configuration for the external hostname (the name users will reach on their browsers), but that seems out of scope of this PR. wdyt?

EDIT: or we could not have the option at all and use our "automatic" discovery and keep it implicit in istio. I will explore this option a bit more.

jnsgruk commented 4 months ago

EDIT: or we could not have the option at all and use our "automatic" discovery and keep it implicit in istio. I will explore this option a bit more.

This sounds like a better plan to me :)

DnPlas commented 4 months ago

EDIT: or we could not have the option at all and use our "automatic" discovery and keep it implicit in istio. I will explore this option a bit more.

This sounds like a better plan to me :)

Yeah, definitely. Will update the PR shortly.

DnPlas commented 4 months ago

Thanks @ca-scribner for the review, this is ready for another pass.