Add watcher for upstreams TLS certificates #716
Conversation
rubenvp8510
force-pushed
the
refresh_tls
branch
2 times, most recently
from
4fea880 to
44abbae
Compare
|
Why only the traces endpoint? Can't we make it available but opt-in to every signal? |
|
Yes! I will extend this to all endpoints :) |
rubenvp8510
force-pushed
the
refresh_tls
branch
4 times, most recently
from
412c763 to
917d08c
Compare
rubenvp8510
force-pushed
the
refresh_tls
branch
3 times, most recently
from
9d6176f to
5b99c12
Compare
Signed-off-by: Ruben Vargas <[email protected]>
rubenvp8510
force-pushed
the
refresh_tls
branch
3 times, most recently
from
de6c05e to
a1957e6
Compare
Signed-off-by: Ruben Vargas <[email protected]>
There was a problem hiding this comment.
Thanks a lot for your work, @rubenvp8510.
This mostly looks good to me, I just have left a few minor comments regarding some documentation in the codebase. With them handled I think we'll be good to merge, imho.
Signed-off-by: Ruben Vargas <[email protected]>
|
@douglascamata Sorry for the late response. this is ready for review again. Thank you very much! |
| type UpstreamOptions struct { | ||
| cert *stdtls.Certificate | ||
| ca []byte | ||
| certReloader *rbacproxytls.CertReloader |
There was a problem hiding this comment.
I think we could improve the name here to try to make things easier to understand. *rbacproxytls.CertReloader is a certificate reloader for the tenants MTLS certs. So maybe UpstreamOptions.certReloader could be named UpstreamOptions.tenantsCertReloader.
Without this it makes me wonder: why there is a certReloader and a caReloader here? Both are certs! This makes me also confused about the separate startCertReloader and startCAReloader.
This brings me to wonder about the name of the struct itself. What is "upstream" in UpstreamOptions? 🤔
There was a problem hiding this comment.
why tenantsCertReloader? I don't see where the tenants are involved here. Upstream means all upstream services for logs/metrics/traces. At least is the terminology that is used in all the gateway. for instance:
File containing the TLS client key to authenticate against upstream logs servers. Leave blank to disable mTLS.
There was a problem hiding this comment.
Humm, I see. So I got it wrong. Probably we could clarify this. There are many certs everywhere, and reloaders for them, so things are confusing.
Signed-off-by: Ruben Vargas <[email protected]>
douglascamata
changed the title
This PR adds a watcher for the certificates so in case of rotation, the certificates will be reloaded.
Added flags to disable/enable this per signal, and in the general server