Decouple CA load from request lifecycle #50
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 93.05% 93.16% +0.10%
==========================================
Files 20 21 +1
Lines 1311 1433 +122
==========================================
+ Hits 1220 1335 +115
- Misses 55 60 +5
- Partials 36 38 +2
|
| // The duration between refreshes of the trusted certificate authority if `trusted_certificate_authority_file` is set. | ||
| // Unset or 0 means disables the refresh, useful is no rotation is expected. | ||
| // Default is unset, means disabled. | ||
| // Is a String that ends in `s` to indicate seconds and is preceded by the number of seconds, |
There was a problem hiding this comment.
I think it supports also minutes or hours?
Also, let's not mention nanos; no one will use such precision when configuring this interval
There was a problem hiding this comment.
That's what I thought, but this is not golang time.Duration, it's the protobuf one it says only seconds and nanos are supported: https://protobuf.dev/reference/protobuf/google.protobuf/#duration
There was a problem hiding this comment.
I think this is what happens when you marshal the duration into a JSON, that it always marshals it as seconds, but when unmarshaling you can use normal Golang Duration string values. Can you try it?
There was a problem hiding this comment.
I already tried it:
proto: (line 32:63): invalid google.protobuf.Duration value "60m"
But as the documentation says, it only accepts seconds with decimals for nanos.
Something that could make sense is to turn this field into a string representing the golang time.Duration format and validate that manually when we're loading the config.
|
Now that we have merged the PR to load the client-secret from a K8s secret, WDYT about adding an option to load the CA from a secret as well, and update the Secret loader accordingly? This can be done in another PR though. |
Co-authored-by: Ignasi Barrera <[email protected]>
yes, this was the intention, I thought I wrote it in the description 🤔 |
|
I'm merging this as I could raise a new PR if we need to change something about the Duration comments. |
Currently, the CA was loaded from the file (if configured) every time it was needed, meaning every request that matches this configuration.
This PR mainly creates a TLS pool that holds all requested TLS configs and serves them when necessary, so the file load only happens once, the first time a TLS config is requested.
It also allows configuring an asynchronous refresh of the CA, so the next time the same TLS config is requested it will contain new values from the file if that changed without restarting the authservice-go.
This code should be ready to add k8s secret support in the future. By implementing
internal.Readerinterface and adding a secret reference in thetrustedCAconfig.Note: I didn't want to add a dependency on
github.com/fsnotify/fsnotifyas I don't expect the CAs to rotate frequently, but it shouldn't be a big deal to add it in the future.