Dynamic serving certificates #84200
Dynamic serving certificates #84200
Conversation
|
Hi @jackkleeman. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
|
/assign @deads2k |
| } | ||
|
|
||
| // check to see if we have a change. If the values are the same, do nothing. | ||
| existing, ok := c.caBundle.Load().(*certKeyContent) |
There was a problem hiding this comment.
as in the other pull. Why doesn't this panic when nil is stored?
There was a problem hiding this comment.
There was a problem hiding this comment.
It shouldn't because of the ok, I thought?
There was a problem hiding this comment.
a7b9e87 to
cc90aba
Compare
staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go
Outdated
Show resolved
Hide resolved
Reload certificate cert and key file from disk every minute and notify the dynamic certificate controller when they change, allowing serving tls config to be updated.
f286865 to
4e99b5d
Compare
| } | ||
|
|
||
| // check to see if we have a change. If the values are the same, do nothing. | ||
| existing, ok := c.servingCert.Load().(*certKeyContent) |
There was a problem hiding this comment.
@sttts are you happy with this? I refactored the other one, but this may work.
@jackkleeman you can match the other one and be golden.
|
I'm convinced by the playground test. /lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jackkleeman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
| if err != nil { | ||
| return err | ||
| } | ||
| if len(cert) == 0 || len(key) == 0 { |
There was a problem hiding this comment.
It seems the cert length check can be performed prior to reading keyFile.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR means that apiserver serving certificates are reloaded off of disk every minute, and notify the dynamic certificate controller when they change, leading to an updated tls.Config being served. SNI certs will be handled in a later PR
Which issue(s) this PR fixes:
Fixes #66448
Does this PR introduce a user-facing change?: