Add lua endpoint to handle certificates in dynamic configuration mode #56
Conversation
hnrytrn
force-pushed
the
master
branch
3 times, most recently
from
5618b93 to
cc61952
Compare
| @@ -196,6 +196,8 @@ func buildLuaSharedDictionaries(s interface{}, dynamicConfigurationEnabled bool, | |||
| "lua_shared_dict balancer_ewma 1M", | |||
| "lua_shared_dict balancer_ewma_last_touched_at 1M", | |||
| "lua_shared_dict sticky_sessions 1M", | |||
| "lua_shared_dict cert_data 10M", | |||
| "lua_shared_dict key_data 10M", | |||
There was a problem hiding this comment.
Why not use configuration_data for keeping certificates? My initial plan was to keep every config piece in there. Any reason to not use single dictionary for all?
There was a problem hiding this comment.
Sure, I just thought it was cleaner to have different dictionaries, but I can update it to just use the configuration_data dictionary
| end | ||
| local success, err = configuration_data:set(cert.HostName .. cert_ext, cert.FullChainCert) | ||
| if not success then | ||
| ngx.log(ngx.ERR, "certificate dynamic-configuration: error setting certificate: " .. tostring(err)) |
There was a problem hiding this comment.
Could we dump cert.Hostname here as well? Or is that composite in the err that would be thrown here?
| @@ -1,6 +1,11 @@ | |||
| local json = require("cjson") | |||
|
|
|||
| -- this is the Lua representation of Configuration struct in internal/ingress/types.go | |||
| local configuration_data = ngx.shared.configuration_data | |||
There was a problem hiding this comment.
We're attempting to make configuration_data's structure mirror this: https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/types.go#L52
I feel like that breaks in this PR. Maybe we want a servers key within configuration_data?
On the same vein; in addition, in your corresponding controller PR - we already have an ingress.Server type, which we could use instead of your new Certificate type in your configureCerts method. What do you think?
E.g.
servers := make([]*ingress.Server, len(pcfg.Servers))
for i, server := range pcfg.Servers {
luaServer := &ingress.Server{
...
}
There was a problem hiding this comment.
Good point, I agree. I will update the PRs.
There was a problem hiding this comment.
I talked to Elvin who wrote this comment and he said that this didn't have to mirror the Configuration struct in internal/ingress/types.go perfectly. I reverted the commit that used servers as a key, so we don't have to JSON decode the servers data and iterate through each server for every request.
| @@ -58,7 +110,7 @@ function _M.call() | |||
| ngx.log(ngx.ERR, "dynamic-configuration: error updating configuration: " .. tostring(err)) | |||
| ngx.status = ngx.HTTP_BAD_REQUEST | |||
| return | |||
| end | |||
| end | |||
There was a problem hiding this comment.
redundant whitespace in the end of line
| function _M.call() | ||
| if ngx.var.request_method ~= "POST" and ngx.var.request_method ~= "GET" then | ||
| ngx.status = ngx.HTTP_BAD_REQUEST | ||
| ngx.print("Only POST and GET requests are allowed!") | ||
| return | ||
| end | ||
|
|
||
| if ngx.var.request_uri == "/configuration/servers" then | ||
| handle_server_request(); | ||
| return; |
| return | ||
| end | ||
|
|
||
| -- Handler for the /configuration/servers endpoint |
There was a problem hiding this comment.
do you think it is hard for someone to figure out that this function is handler for that requests?
| @@ -27,13 +29,63 @@ local function fetch_request_body() | |||
| return body | |||
| end | |||
|
|
|||
| -- Returns the certificate for a given host | |||
| function _M.get_cert(host_name) | |||
There was a problem hiding this comment.
the convention is to not separate host and name, use hostname instead
hnrytrn
force-pushed
the
master
branch
2 times, most recently
from
39fb157 to
07d6fc2
Compare
hnrytrn
force-pushed
the
master
branch
3 times, most recently
from
abda1c4 to
c21fcb4
Compare
| if not success then | ||
| ngx.log(ngx.ERR, "certificate dynamic-configuration: error setting certificate: " | ||
| .. tostring(err), cert.hostname) | ||
| ngx.status = ngx.HTTP_BAD_REQUEST |
There was a problem hiding this comment.
what is this a bad request? should it not be internal server error (status 500)?
| ngx.log(ngx.ERR, "certificate dynamic-configuration: error setting certificate: " | ||
| .. tostring(err), cert.hostname) | ||
| ngx.status = ngx.HTTP_BAD_REQUEST | ||
| return |
There was a problem hiding this comment.
You are short circuiting when we fail to store one cert and never try the rest. I suggest we go through the whole list and try to store all of them and collect the errors. In the end check if there was any error and if there was any then return internal server error and log
| end | ||
|
|
||
| ngx.status = ngx.HTTP_CREATED | ||
| return |
|
|
||
| -- Update certificates and private keys for each host | ||
| for _, cert in pairs(certs) do | ||
| if cert.hostname and cert.sslCert.pemCertKey then |
There was a problem hiding this comment.
let's also log a warning when this does not hold, this will help with potential debugging in future
| return | ||
| end | ||
|
|
||
| local cert_str = fetch_request_body() |
| local ok, certs = pcall(json.decode, raw_certs) | ||
| if not ok then | ||
| ngx.log(ngx.ERR, "could not parse certificate: " .. tostring(certs)) | ||
| return |
| return | ||
| end | ||
|
|
||
| if not certs then |
|
|
||
| local err_buf = {} | ||
| -- Update certificates and private keys for each host | ||
| for _, cert in pairs(certs) do |
There was a problem hiding this comment.
for an array it's recommended to use ipairs.
pairs for when it is a dictionary
| return configuration_data:get(hostname) | ||
| end | ||
|
|
||
| local function handle_cert_request() |
There was a problem hiding this comment.
since this is handling requests to /configuration/servers, let's call it handle_servers, and rename the variables accordingly
| "error setting certificate for %s: %s\n", cert.hostname, tostring(err)) | ||
| end | ||
| else | ||
| ngx.log(ngx.WARN, "certificate dynamic-configuration: hostname and pemCertKey are not present") |
There was a problem hiding this comment.
no need for log prefixes.
--
hostname OR pemCertKey
| -- Update certificates and private keys for each host | ||
| for _, cert in pairs(certs) do | ||
| if cert.hostname and cert.sslCert.pemCertKey then | ||
| local success, err = configuration_data:set(cert.hostname, cert.sslCert.pemCertKey) |
There was a problem hiding this comment.
let's introduce a new certificates_data dict to store certs.
| if cert.hostname and cert.sslCert.pemCertKey then | ||
| local success, err = configuration_data:set(cert.hostname, cert.sslCert.pemCertKey) | ||
| if not success then | ||
| err_buf[#err_buf + 1] = string.format("certificate dynamic-configuration: " .. |
There was a problem hiding this comment.
Let's treat the array as array. You can use table.insert() function to add a new element to err_buf array.
| @@ -27,13 +29,67 @@ local function fetch_request_body() | |||
| return body | |||
| end | |||
|
|
|||
| -- Returns the certificate and key for a given host | |||
There was a problem hiding this comment.
does not add much value given the function name
| @@ -27,13 +29,67 @@ local function fetch_request_body() | |||
| return body | |||
| end | |||
|
|
|||
| -- Returns the certificate and key for a given host | |||
| function _M.get_cert_key(hostname) | |||
There was a problem hiding this comment.
get_pem_cert_key to be consistent with controller and https://github.com/Shopify/ingress/pull/60/files
| return configuration_data:get(hostname) | ||
| end | ||
|
|
||
| local function handle_cert_request() |
There was a problem hiding this comment.
let's export this when TEST variable is set and write unit test for it
This reverts commit 07d6fc2e407eb2c3b7c8b63d223dc688d753c7f8.
hnrytrn
force-pushed
the
master
branch
2 times, most recently
from
84f121b to
f9aae61
Compare
| -- Update certificates and private keys for each host | ||
| for _, server in ipairs(servers) do | ||
| if server.hostname and server.sslCert.pemCertKey then | ||
| local success, err = certificate_data:set(server.hostname, server.sslCert.pemCertKey) |
There was a problem hiding this comment.
When the dictionary is full Nginx will remove least used item to spare some space for the new one and try to fit the new item. In that case success will still be true. But IMO this is unacceptable for what we are trying to do. That would mean we delete the certificate of app1 so that we can make app2 work.
I think we should use safe_set (https://github.com/openresty/lua-nginx-module#ngxshareddictsafe_set) and when the error message is "no memory" immediately short-circuit from the loop and return 502, Internal Server Error and log the message.
| -- this is the Lua representation of Configuration struct in internal/ingress/types.go | ||
| local configuration_data = ngx.shared.configuration_data | ||
| local certificate_data = ngx.shared.certificate_data |
There was a problem hiding this comment.
you need to declare this dictionary in nginx.tmpl. Also what size do you think of configuring for it? Given you configure it to be 60MB, how many certificates can we fit?
There was a problem hiding this comment.
Assuming the certificate is generated with a 2048 bit RSA key/cert pair, the PEM encoded certificate and private key would ~3.2kB. So if we have a dictionary of 60mB we would be able to store 18 750 certificates. With that, I think setting it to 16MB would be good enough (5000 certificates). What do you think?
There was a problem hiding this comment.
Later we should add this info into docs.
| HTTP_CREATED = 201, | ||
| HTTP_INTERNAL_SERVER_ERROR = 500, | ||
| ERR = nil, | ||
| req = { |
There was a problem hiding this comment.
would not just mocking ngx.req be enough?
_G.ngx.req = {
...
}
Also please make sure you unmock it after your test finishes otherwise you're leaking a state.
There was a problem hiding this comment.
Yes, will unmock after.
Unless I'm missing something just mocking ngx.req wouldn't be enough. The tests require access to ngx.status, which wouldn't be accessible unless ngx was mocked. By mocking ngx I had to add the other mocked functions and variables that are used by configuration.lua.
|
The current tests cases look good! Once #56 (comment) is addressed we will need a test for that too. |
| ngx.req.get_body_data = function() return mock_servers end | ||
|
|
||
| assert.has_no.errors(configuration.handle_servers) | ||
| assert.same(ngx.status, ngx.HTTP_CREATED) |
There was a problem hiding this comment.
you should also assert that the data is stored in the shared dictionary - that's the most important outcome of calling handle_servers
| sslCert = { | ||
| pemCertKey = "pemCertKey" | ||
| } | ||
| } |
There was a problem hiding this comment.
can you add more than one host to make sure the error collection logic works?
| pemCertKey = "pemCertKey" | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
can you have more than one server and return "no memory" on only i.e second of them and assert that the loop gets broken and function does not try to store the next one
|
Moved PR upstream kubernetes#2889 |
What this PR does / why we need it:
This PR is a part of adding dynamic certificate serving functionality (https://github.com/Shopify/edgescale/issues/515). This adds a Lua endpoint that stores certificates and keys of a host in a shared dictionary and allows Lua directives to retrieve them.
@Shopify/edgescale