Add /dbg certs command #3799
Add /dbg certs command #3799
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/approve |
k8s-ci-robot
added
the
approved
Why not? I think we should publish the plugin in 0.23 as an alpha feature providing the krew command to install the plugin from github without opening a PR in the krew repository. |
| @@ -127,6 +157,11 @@ function _M.call() | |||
| return | |||
| end | |||
|
|
|||
| if starts_with(ngx.var.request_uri, "/configuration/certs") then | |||
There was a problem hiding this comment.
Maybe use ngx.var.uri instead of using starts_with?
| end | ||
|
|
||
| local key = _M.get_pem_cert_key(query["hostname"]) | ||
| if key and key ~= "" then |
There was a problem hiding this comment.
key ~= ""
I don't think this should be "not found" - it's found, just empty.
There was a problem hiding this comment.
So if the queried host isn't configured at all, this is nil. If the host exists in the ingress definition at all but doesn't have a certificate, this is the empty string. I feel like we should return 404 for both of these cases.
There was a problem hiding this comment.
Based on some in-person discussion, I've made this change.
There was a problem hiding this comment.
tldr; returning an empty content in that case reveals more information than returning 404.
Sure, that seems like a good idea! |
|
@ElvinEfendi good idea |
|
@alexkursell let's simplify the RP then by removing support for static certs. |
Done. |
| @@ -88,15 +89,39 @@ func NewPostStatusRequest(path, contentType string, data interface{}) (int, []by | |||
| return res.StatusCode, body, nil | |||
| } | |||
|
|
|||
| // GetServerBlock takes an nginx.conf file and a host and tries to find the server block for that host | |||
| func GetServerBlock(conf string, host string) (string, error) { | |||
There was a problem hiding this comment.
Did you leave this here because it'll be needed later on when we add more commands to dbg?
There was a problem hiding this comment.
Maybe. I'm also making use of it in the actual plugin, and I feel that it's good to keep it next to the other common nginx.conf reading stuff.
alexkursell
force-pushed
the
dbg-certs
branch
from
b5994e7
to
b3a011b
Compare
alexkursell
force-pushed
the
dbg-certs
branch
from
b3a011b
to
c96eae3
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, alexkursell, 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 |
What this PR does / why we need it: While #3779 isn't going to be part of the 0.23 release, it would be nice however, to have all of the
/dbgcommands that the plugin relies on existing to be present in the 0.23 release so that the plugin is completely usable with 0.23 ingress-nginx controllers.This PR separates out just the additions to the
/dbgcommand so that they can be merged before #3779cc: @ElvinEfendi