make dynamic SSL mode default #3808
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/XS
ElvinEfendi
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
docs/user-guide/cli-arguments.md
Outdated
| @@ -14,7 +14,7 @@ They are set in the container spec of the `nginx-ingress-controller` Deployment | |||
| | `--default-server-port int` | When `default-backend-service` is not specified or specified service does not have any endpoint, a local endpoint with this port will be used to serve 404 page from inside Nginx. | | |||
| | `--default-ssl-certificate string` | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". | | |||
| | `--election-id string` | Election id to use for Ingress status updates. (default "ingress-controller-leader") | | |||
| | `--enable-dynamic-certificates` | Dynamically serves certificates instead of reloading NGINX when certificates are created, updated, or deleted. Currently does not support OCSP stapling, so --enable-ssl-chain-completion must be turned off. Assuming the certificate is generated with a 2048 bit RSA key/cert pair, this feature can store roughly 5000 certificates. This is an experiemental feature that currently is not ready for production use. Feature backed by OpenResty Lua libraries. (disabled by default) | | |||
| | `--enable-dynamic-certificates` | Dynamically serves certificates instead of reloading NGINX when certificates are created, updated, or deleted. Currently does not support OCSP stapling, so --enable-ssl-chain-completion must be turned off. Assuming the certificate is generated with a 2048 bit RSA key/cert pair, this feature can store roughly 5000 certificates. This is an experiemental feature that currently is not ready for production use. Feature backed by OpenResty Lua libraries. (enabled by default) | | |||
There was a problem hiding this comment.
Maybe remove "This is an experiemental feature that currently is not ready for production use." That would scare me considering we're making this the default.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@ElvinEfendi you need to change the e2e tests that check for the ssl real certificate name instead of the fake one |
k8s-ci-robot
added
size/M
|
okay, so I think client authentication and dynamic cert mode does not play nice together, a.k.a there's a bug in dynamic cert implementation, otherwise there's no fundamental issue why they can not be both enabled: when in dynamic cert mode, never gets executed. |
k8s-ci-robot
added
size/L
|
Recent changes should be enough functionality wise, but the package needs some love. I'll extract the actual bug fix into another PR and hopefully refactor the code a bit. |
k8s-ci-robot
added
size/M
|
I think we can not make dynamic SSL mode default before we implement OCSP stapling in Lua: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ocsp.md |
Why? We can switch to dynamic SSL mode with a note in the release, indicating how to return to normal mode. With this, we have at least one release cycle to get feedback and fix any potential issue (I've been using dynamic mode without problems for the last three releases) |
|
@aledbf in that case deprecating the non dynamic SSL mode does not make sense, because that'd indicate we have a concrete plan of when we are going to drop support for it. But we can not drop the support for it yet, because OCSP stapling is not supported in dynamic SSL mode. I'm going to make dynamic SSL mode the default in this PR but skip deprecating the flag until we implement OCSP stapling in dynamic SSL mode as well. |
I agree with that. |
ElvinEfendi
changed the title
|
@aledbf this is ready for review. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi 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 |
|
This flag breaks a pretty normal setup. |
What this PR does / why we need it:
Dynamic cert mode feature was introduced in
0.19.0and since then it has been used in production and also we have fixed few bugs since the initial implementation.This PR makes dynamic cert mode default. Currently the main missing feature of dynamic SSL mode is lack of OCSP support. Once that's implemented we can drop non dynamic SSL mode completely.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: