Add dynamic certificate serving feature to controller #2923
Conversation
k8s-ci-robot
added
cncf-cla: yes
aledbf
added
the
do-not-merge/hold
Codecov Report
@@ Coverage Diff @@
## master #2923 +/- ##
==========================================
- Coverage 47.58% 47.56% -0.02%
==========================================
Files 77 77
Lines 5533 5632 +99
==========================================
+ Hits 2633 2679 +46
- Misses 2560 2600 +40
- Partials 340 353 +13
Continue to review full report at Codecov.
|
aledbf
removed
the
do-not-merge/hold
|
@hnrytrn my only question is related to OCSP. From the ssl.md link I see this https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ocsp.md Edit: this can be done in a different PR |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
lgtm
|
@aledbf Hi, yes I was planning on adding OCSP functionality using Open Resty's OCSP module in the next iteration of this feature. |
| if !n.IsDynamicConfigurationEnough(newConfig) { | ||
| t.Errorf("Expected to be dynamically configurable when backend and SSLCert changes") | ||
| } | ||
|
|
There was a problem hiding this comment.
can you also add a test when dynamic certificate configuration is enabled and a server in new configuration has a change other than certificate.
| } | ||
|
|
||
| if strings.Index(body, "service") != -1 { | ||
| t.Errorf("unexpected service reference in JSON content: %v", body) |
There was a problem hiding this comment.
Why do you have these two assertions?
| c := certutil.EncodeCertPEM(cert.Cert) | ||
| k := certutil.EncodePrivateKeyPEM(cert.Key) | ||
|
|
||
| ngxCert, err := CreateSSLCert(name, c, k, []byte{}) |
There was a problem hiding this comment.
I don't see how are you asserting that c and k are properly formatted and bundled into ngxCert, am I missing something?
k8s-ci-robot
removed
the
lgtm
|
/hold cancel |
k8s-ci-robot
added
lgtm
hnrytrn
force-pushed
the
dynamic-certs-controller
branch
from
c358afd to
566ffd3
Compare
k8s-ci-robot
removed
the
lgtm
hnrytrn
force-pushed
the
dynamic-certs-controller
branch
from
566ffd3 to
d081687
Compare
hnrytrn
force-pushed
the
dynamic-certs-controller
branch
from
d081687 to
7faf089
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi, hnrytrn 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 |
What this PR does / why we need it:
This PR is a part of adding dynamic certificate serving functionality. It adds a new flag
enable-dynamic-certificates. When enabled, the controller will POST certificates to a Lua endpoint (#2889), avoid reloading NGINX when certificates are updated, and avoid writing certificates on disk. There will be another PR following this to serve the certificates that are POSTed.Special notes for your reviewer:
This feature currently does not support OCSP because the future PR that will serve the certificates will use Open Resty's ngx.ssl module (https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ssl.md), which does not support OCSP.