Certificate Rotatation in APIServer #1154
Conversation
|
Thanks for your PR. The following commands are available:
|
MatthewHinton56
force-pushed
the
Update-Serving-Certificate
branch
from
6b92482 to
c69be3d
Compare
There was a problem hiding this comment.
LGTM, but I'll defer to @tnqn for the review. I wonder if we should improve testing for rotation, since the original tests were not enough to detect this issue (the test should verify that the apiserver switches to the new certificate). But I don't want to delay this PR either...
Thanks @MatthewHinton56 for realizing there was an issue in the first place.
| @@ -124,15 +127,14 @@ func generateSelfSignedCertificate(secureServing *options.SecureServingOptionsWi | |||
| var caContentProvider dynamiccertificates.CAContentProvider | |||
|
|
|||
| // Set the PairName but leave certificate directory blank to generate in-memory by default. | |||
| secureServing.ServerCert.CertDirectory = "" | |||
| secureServing.ServerCert.CertDirectory = selfSignedCertDir | |||
There was a problem hiding this comment.
The above comment should be updated too.
| // UpdateCertificate updates the certificate to a new one. Used to rotate statically signed certificates before they | ||
| // expire. | ||
| func (c *CACertController) UpdateCertificate(caContentProvider dynamiccertificates.CAContentProvider) { | ||
| c.mutex.Lock() |
| if err != nil { | ||
| klog.Errorf("error generating new cert: %v", err) | ||
| return | ||
| } | ||
| c.UpdateCertificate(caContentProvider) | ||
| c.RunOnce() |
There was a problem hiding this comment.
c.RunOnce calls caContentProvider.RunOnce() and c.syncCACert(), while the former will trigger the latter too, then it will be executed twice.
I think it could just call caContentProvider.RunOnce(), the first argument of this function could be *DynamicFileCAContent.
MatthewHinton56
force-pushed
the
Update-Serving-Certificate
branch
from
c69be3d to
3c23287
Compare
There was a problem hiding this comment.
One remaining comment about unit test message, otherwise LGTM.
@antoninbas please go ahead merging this once the comment is addressed.
BTW, the attached PDF is not the log. But I have verified this PR too.
| } else { | ||
| assert.Nil(t, secureServing.ServerCert.GeneratedCert) | ||
| assert.NotEqual(t, genericoptions.CertKey{CertFile: selfSignedCertDir + "/antrea-controller.crt", KeyFile: selfSignedCertDir + "/antrea-controller.key"}, secureServing.ServerCert.CertKey, "CertKey doesn't match") |
There was a problem hiding this comment.
The message "CertKey doesn't match" is wrong if it asserts they are not equal? Perhaps you could assert it equals to the expected CertKey of user provided case.
|
I updated the log, sorry about that. |
MatthewHinton56
force-pushed
the
Update-Serving-Certificate
branch
from
3c23287 to
588dcdd
Compare
|
/test-all |
|
/test-windows-networkpolicy |
|
Seems that there is an unrelated issue with the |
Self signed certificates are now stored in files rather than memory. This allows for the use of dynamic certificate objects. That will automatically process the update The caCertController is updated immediately through RunOnce, whereas the DynamicServingContent for the serving certificate can take up to one minute Fixes antrea-io#1152
Self signed certificates are now stored in files rather than memory. This allows for the use of dynamic certificate objects. That will automatically process the update The caCertController is updated immediately through RunOnce, whereas the DynamicServingContent for the serving certificate can take up to one minute Fixes #1152
Self signed certificates are now stored in files rather than memory. This allows for the use of dynamic certificate objects. That will automatically process the update The caCertController is updated immediately through RunOnce, whereas the DynamicServingContent for the serving certificate can take up to one minute Fixes antrea-io#1152
fixes: #1152
Self signed certificates are now stored in files rather than memory. This allows for the use of dynamic certificate objects. That will automatically process the update
The caCertController is updated immediately through RunOnce, whereas the DynamicServingContent for the serving certificate can take up to one minute
Attached is a log for an antrea controller with a fixed certificate rotation every 3 minutes. It shows that it can take up to 1 minute for the certificate to be detected, but in production, the rotation period will be closer to months.
antrea-contoller-info.txt