Add app.server.certificateDuration #469
Conversation
Signed-off-by: Volodymyr Kornylyuk <[email protected]>
cert-manager-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
bot
added
the
needs-ok-to-test
|
Hi @volkorny. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cert-manager-prow
bot
added
the
size/M
There was a problem hiding this comment.
@volkorny Could you please describe a little better what you are trying to do here? Is ValidityDuration not given in the request which results in a default of 24h being applied? Or is it set, but you want all certificates to have the same ValidityDuration? With your proposed changes it seems like the latter. It is important to understand this to get good naming. I would suggest either DefaultClientCertificateDuration or FixedClientCertificateDuration - depending on what you are try to achieve.
It is also a bit harder to introduce a new field when we already have MaximumClientCertificateDuration. It should be documented how they relate to each other - depending on the implemented logic.
| duration = s.opts.ClientCertificateDuration | ||
| } | ||
|
|
||
| log.V(2).Info("Setting certificate duration", "duration", duration) |
There was a problem hiding this comment.
If logging this, this should only be logged when overriding the duration. Why was log level 2 chosen? I would expect log level 3 after scanning through the code in this project.
|
|
||
| if s.opts.ClientCertificateDuration > 0 { | ||
| duration = s.opts.ClientCertificateDuration | ||
| } |
There was a problem hiding this comment.
I get a bit confused by this code that may modify duration twice (max and override)., and wonder if a shared switch statement would make this clearer.
Add app.server.certificateDuration
In our service mesh, we aim to increase the certificate duration beyond 24 hours. I noticed a similar request in #176