add controller to regenerate service serving certs #12050
Conversation
|
Also, there should probably be a card on the platform board. |
|
[test] |
| // Run begins watching and syncing. | ||
| func (sc *ServiceServingCertUpdateController) Run(workers int, stopCh <-chan struct{}) { | ||
| defer utilruntime.HandleCrash() | ||
| defer glog.Infof("Shutting down service signing cert update controller") |
There was a problem hiding this comment.
should also have the "Starting" message?
| // syncedPollPeriod controls how often you look at the status of your sync funcs | ||
| const syncedPollPeriod = 100 * time.Millisecond | ||
|
|
||
| func waitForCacheSync(stopCh <-chan struct{}, cacheSyncs ...informerSynced) bool { |
There was a problem hiding this comment.
you mean this one is in kube right?
There was a problem hiding this comment.
@ncdc fyi, to be removed after 1.5 rebase
|
@php-coder or @enj PTAL |
|
@deads2k will create a card |
| @@ -41,6 +41,8 @@ const ( | |||
| // ServiceNameAnnotation is an annotation on a secret that indicates which service created it, by Name to allow reverse lookups on services | |||
| // for comparison against UIDs | |||
| ServiceNameAnnotation = "service.alpha.openshift.io/originating-service-name" | |||
| // ServingCertExpiryAnnotation is an annotation that holds the expiry time of the certificate | |||
| ServingCertExpiryAnnotation = "service.alpha.openshift.io/expiry" | |||
There was a problem hiding this comment.
should the annotation mention certificate-expiry or certificate-expiration-days ?
There was a problem hiding this comment.
Not days, but I could call it certificate-expiry
There was a problem hiding this comment.
Can you also document the format that is supported as a value?
| // syncedPollPeriod controls how often you look at the status of your sync funcs | ||
| const syncedPollPeriod = 100 * time.Millisecond | ||
|
|
||
| func waitForCacheSync(stopCh <-chan struct{}, cacheSyncs ...informerSynced) bool { |
There was a problem hiding this comment.
@ncdc fyi, to be removed after 1.5 rebase
| } | ||
|
|
||
| func (sc *ServiceServingCertUpdateController) runWorker() { | ||
| for sc.processNextWorkItem() { |
There was a problem hiding this comment.
I must admit I dislike this new form, but that doesn't matter here 😉 I find the body here being much more readable than almost empty for-loop with almost empty method calling another method.
| return true | ||
| } | ||
|
|
||
| if expiry.Add(sc.minTimeLeftForCert).After(time.Now()) { |
There was a problem hiding this comment.
Is this a correct calculation?
As far I understand certificate requires regeneration if now > (expiration date - 1h) IOW we'll update certificate if it expires in next hour. But here we're adding 1 hour instead of subtracting it. What I'm missing here?
Also: why 1 hour if we're checking each 20 minutes?
There was a problem hiding this comment.
Is this a correct calculation?
As far I understand certificate requires regeneration if now > (expiration date - 1h) IOW we'll update certificate if it expires in next hour. But here we're adding 1 hour instead of subtracting it. What I'm missing here?
Also: why 1 hour if we're checking each 20 minutes?
heh, backwards. I'll add @soltysh unit test and catch it. :)
The 1 hour is a place holder for something more reasonable driven off a "real" expiry time. I suspect we'll have an upper bound too to eliminate all long lived tokens.
| ca *crypto.CA | ||
| publicCert string | ||
| dnsSuffix string | ||
| minTimeLeftForCert time.Duration |
There was a problem hiding this comment.
This deserves its own comment
deads2k
force-pushed
the
serving-cert-01-refresh
branch
from
ff2658e to
75e1bd8
Compare
|
comments addressed. |
deads2k
force-pushed
the
serving-cert-01-refresh
branch
from
75e1bd8 to
663a5ef
Compare
|
Evaluated for origin test up to 663a5ef |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11785/) (Base Commit: 7ec2f67) |
|
I'm betting on a flake. Any other comments? |
|
LGTM |
|
[merge] |
|
Flake #11024. re-[merge] |
|
Evaluated for origin merge up to 663a5ef |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11858/) (Base Commit: ff5bd5a) (Image: devenv-rhel7_5456) |
Adds a controller to update service serving certs when they are about to expire.
@mfojtik for review distribution