fix(admin) handle snis in PUT and PATCH /certificates #3040
Conversation
There was a problem hiding this comment.
Awesome! Thanks for taking stab at this. I am leaving this in "changes requested" state because it would obviously require some extensive testing (but I think this was more of a proposal). I also see one blocker for this being merged, which is the quiet option used in the DAO operation. Nothing too concerning except for our terrible handling of the PUT verb in this API... But that will be dealt with in due time :)
kong/api/routes/certificates.lua
Outdated
| if not seen[old_snis[i].name] then | ||
| dao_factory.ssl_servers_names:delete( | ||
| { name = old_snis[i].name }, | ||
| { quiet = true } --discuss if this should be used |
There was a problem hiding this comment.
We should not use the quiet option here (in fact, it should probably disappear, it is a legacy option that is pretty much useless since #2561). We want to send an invalidation event here so that other workers invalidate their cache, and broadcast the event to other nodes:
Lines 157 to 163 in 59ef814
There was a problem hiding this comment.
Make sense, thanks for the explanation!
kong/api/routes/certificates.lua
Outdated
| if err then | ||
| return helpers.yield_error(err) | ||
| -- update certificate if necessary | ||
| if (self.params.key and self.params.cert) then |
There was a problem hiding this comment.
style: please don't use parenthesis for such conditions, thanks!
kong/api/routes/certificates.lua
Outdated
| @@ -3,20 +3,150 @@ local utils = require "kong.tools.utils" | |||
| local cjson = require "cjson" | |||
|
|
|||
|
|
|||
| local create_certificate = function(self, dao_factory, helpers) | |||
There was a problem hiding this comment.
could we use the traditional Lua syntactic sugar instead? local function create_certificate() should be fine.
kong/api/routes/certificates.lua
Outdated
| PUT = function(self, dao_factory, helpers) | ||
|
|
||
| -- no id present, behaviour should be same as POST | ||
| if not self.params.id then |
There was a problem hiding this comment.
I see an extra space here in the condition :D Not a blocker
kong/api/routes/certificates.lua
Outdated
|
|
||
|
|
||
| PUT = function(self, dao_factory, helpers) | ||
|
|
There was a problem hiding this comment.
style: could we avoid blank lines as such at the beginning of a function's body? Thanks!
thibaultcha
added
pr/changes requested
It could maybe be refactored a bit to better deal with the duplicated parts, but not a big deal. Otherwise you indeed have little choice but to deal with such custom logic at the application layer in this scenario and considering the limited feature set of this legacy DAO/Admin API. |
|
@thibaultcha I wanted to clarify something: |
hbagdi
changed the title
hbagdi
force-pushed
the
fix-certificate-api
branch
from
9a47e32 to
f856d3f
Compare
hbagdi
changed the title
There was a problem hiding this comment.
Hi @hbagdi,
I was taking a look at this to decide whether we could include it in 0.11.2, but found a few blockers (noted as so) I would like to get your thoughts/feedback upon.
I also noticed a lot of minor style issues that are inconsistent with this - recent - part of the codebase. Would you fix those? Thanks!
Another suggestion is to keep the empty_array_mt fixes for a later, subsequent contributions 😉
kong/api/routes/certificates.lua
Outdated
| PATCH = function(self, dao_factory, helpers) | ||
| return crud.patch(self.params, dao_factory.ssl_certificates, { | ||
| id = self.ssl_certificate_id | ||
| }) | ||
| end, | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't think those changes are desired - this is intended style (2 lines jump between semantically similar code blocks) that we are enforcing in newer parts of the codebase.
There was a problem hiding this comment.
There are ~5/6 similar other changes in this patchset - will you revert them? Thanks!
kong/api/routes/certificates.lua
Outdated
| end | ||
|
|
||
| -- insert/delete SNIs into db if snis field was present in the request | ||
| for _, sni in ipairs(snis) do |
There was a problem hiding this comment.
It seems like this block (195 to 221) is over-indented.
kong/api/routes/certificates.lua
Outdated
| end | ||
|
|
||
| -- update certificate if necessary | ||
| if self.params.key and self.params.cert then |
There was a problem hiding this comment.
Blocker: If a user sends an incomplete payload (e.g. id + snis + key only), this condition will not evaluate to true, hence not updating the certificate, but the execution will continue to the snis logic and potentially return 200 OK for the update. We could be deceiving the user into thinking their request was successful, when in fact the key was not updated.
Since both of those properties are required for an SSL cert, it might be worth not checking for their existence, and instead systematically call :update(self.params, { id = self.params.id }). The DAO should return a schema error (400 Bad Request) to the client if one of these properties is missing - because we are not specifying the { full = true } option (which prevents the update of created_at, iirc).
I see two options:
- Let the DAO handle such errors and return early with
400 Bad Requestto not run subsequent SNIs logic. - Use the
{ full = true }option and allow forcreated_atupdates (minor), but handle such schema validation by hand (that both akeyand acertare required) and return400 Bad Requestwhen one of them is missing.
Of course, tests would be required either way... :D
There was a problem hiding this comment.
PATCH or PUT should/do not check for required fields.
Initially, I was trying to use option 1 so I tried the following (or instead of and)
if self.params.key or self.params.cert then
local ssl_cert, err = dao_factory.ssl_certificates:update(self.params,
{id = self.params.id})
endIn such a case, if only key or only cert is present in the request, DAO does not return a 400 Bad Request. So, we cannot use option 1 mentioned above.
We can do either of the following:
- Handle schema validation by hand and perform
:update(self.params, { id = self.params.id }) - Use
{ full = true }and then let DAO handle the error. In this case, I still getcreated_at is required. Do we need to set it before making the update request?
I'm not sure if what you proposed was incorrect or I'm understanding you wrong.
There was a problem hiding this comment.
Indeed, when passing { full = true }, all required fields must be given. It should be fine since we can get the value of ssl_cert.created_at (retrieved earlier in this handler). Here we would let the DAO do its work (and report correctly formatted errors with minimal effort).
The manual validation option is also on the table, but would require some additional duct-tape which doesn't feel very nice and should be avoided if possible.
Could we give a try to { full = true } with the necessary tweaks?
kong/api/routes/certificates.lua
Outdated
| -- update certificate if necessary | ||
| if self.params.key and self.params.cert then | ||
| local err | ||
| ssl_cert, err = dao_factory.ssl_certificates:update(self.params,{ |
There was a problem hiding this comment.
style: some issues here with the second argument:
...(self.params, {
id = self.params.id
})
kong/api/routes/certificates.lua
Outdated
| return helpers.responses.send_HTTP_NOT_FOUND() | ||
| end | ||
|
|
||
| local snis = nil |
There was a problem hiding this comment.
No need for such nil assignments when declaring variables - they will be nil already and we'll keep the code less cluttered :)
| @@ -274,7 +573,6 @@ describe("Admin API: #" .. kong_config.database, function() | |||
| end) | |||
| end) | |||
|
|
|||
|
|
|||
| body = assert.res_status(200, res) | ||
| json = cjson.decode(body) | ||
| assert.equal(2, json.total) | ||
| assert.equal(2, #json.data) | ||
| end) |
There was a problem hiding this comment.
Blocker: we should also add a test case when removing all snis from a certificate (ensuring they are removed and ensuring that the resulting attribute is [] and not {} if we chose to keep this metatable in the PUT handler for now).
| local body = assert.res_status(200, res) | ||
| local json = cjson.decode(body) | ||
|
|
||
| assert.same({"baz.com" }, json.snis) |
| body = assert.res_status(200, res) | ||
| json = cjson.decode(body) | ||
| assert.equal(2, #json.data) | ||
| assert.equal(2, json.total) |
There was a problem hiding this comment.
Could be worth checking if our snis update was effective in the DB itself and checking the response body from GET here maybe?
| body = assert.res_status(200, res) | ||
| json = cjson.decode(body) | ||
| assert.equal(2, json.total) | ||
| assert.equal(2, #json.data) |
There was a problem hiding this comment.
Could be worth checking if our updated certificate indeed has bar_cert and bar_key here instead of its previous values?
There was a problem hiding this comment.
We do check that in the response of the PUT on Line 355.
Are you saying that we should do another GET for cert_bar.id and make sure that the certificate was updated?
There was a problem hiding this comment.
Are you saying that we should do another GET for cert_bar.id and make sure that the certificate was updated?
Yes, this is what I am saying. As of today, it so happens that our DAO will run subsequent queries for Cassandra to select the inserted value, or will make use of the RETURNING * feature of PostgreSQL. However, we cannot guarantee that this will be the case in the future (we are currently implementing a new DAO internally that tries to avoid such read-before-write and read-after-write patterns already), nor for other databases that we may implement.
|
@thibaultcha Yes, let's keep this out of 0.11.2. |
hbagdi
force-pushed
the
fix-certificate-api
branch
7 times, most recently
from
241dd10 to
5f0de88
Compare
kong/api/routes/certificates.lua
Outdated
| end | ||
| end | ||
|
|
||
| local old_snis, err = dao_factory.ssl_servers_names:find_all { |
There was a problem hiding this comment.
use () for function calls
kong/api/routes/certificates.lua
Outdated
| end | ||
|
|
||
| --id present in body | ||
|
|
kong/api/routes/certificates.lua
Outdated
| local err | ||
| self.params.created_at = ssl_cert.created_at | ||
| ssl_cert, err = dao_factory.ssl_certificates:update(self.params, | ||
| { id = self.params.id }, { full = true}) |
kong/api/routes/certificates.lua
Outdated
| dao_factory.ssl_servers_names:delete({ name = old_snis[i].name }) | ||
| -- ignoring error | ||
| -- returning 4xx here means can confuse user | ||
| -- need to rollback db |
hbagdi
force-pushed
the
fix-certificate-api
branch
from
5f0de88 to
44faa68
Compare
|
@thibaultcha do you know what does the following error mean from Specifically, what does It is randomly popping up in the test results when two completely unrelated test cases are changed. Is this related to some sort of time-out? |
|
I am currently on mobile, but if you open the line indicated by the stacktrace (596), I bet you’ll see a `client:connect()`, `client:send()` or one of the likes with an assert around it.
It means that lua-resty-http is retuning “closed” as an error because you are trying to connect to the Kong test instance while it isn’t running. Probably because it stopped without being restarted by a test, a `teardown()` or one of the likes.
Does that help?
… On Dec 3, 2017, at 10:22 AM, hbagdi ***@***.***> wrote:
@thibaultcha do you know what does the following error mean from busted?
Error → ...integration/04-admin_api/06-certificates_routes_spec.lua @ 594
Admin API: #cassandra /certificates/:sni_or_uuid before_each
...integration/04-admin_api/06-certificates_routes_spec.lua:596: closed
stack traceback:
...integration/04-admin_api/06-certificates_routes_spec.lua:596: in function <...integration/04-admin_api/06-certificates_routes_spec.lua:594>
Specifically, what does closed mean?
I've spent two hours trying to figure it out but I've made no progress.
It is randomly popping up in the test results when two completely unrelated test cases are changed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@thibaultcha thanks for the prompt response. I initially thought of the exact same logic as you describe about this behaviour. But it's probably deeper than that. I've the test file here: it("returns 404 for a random non-existing id", function()
local res = assert(client:send {
method = "PATCH",
path = "/certificates/" .. utils.uuid(),
body = {
cert = ssl_fixtures.cert,
key = ssl_fixtures.key,
snis = "baz.com",
},
headers = { ["Content-Type"] = "application/x-www-form-urlencoded" },
})
assert.res_status(404, res)
-- make sure we did not add any sni
res = assert(client:send {
method = "GET",
path = "/snis",
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
assert.equal(2, #json.data)
assert.equal(2, json.total)
-- make sure we did not add any certificate
res = assert(client:send {
method = "GET",
path = "/certificates",
})
body = assert.res_status(200, res)
json = cjson.decode(body)
assert.equal(2, json.total)
assert.equal(2, #json.data)
end)It seems that if I add more Have you encountered something like this before? |
|
@hbagdi Do you mind pushing a reproducible example to this branch? |
|
@thibaultcha https://travis-ci.org/Kong/kong/jobs/311106586#L1882 |
|
@hbagdi Thanks for sharing. Your test suite is now hitting the keepalive_requests setting, which defaults to 100. After the 100th request, NGINX instructs lua-resty-http to close the connection, and so the client sends a FIN right after receiving the 100th response. Maybe you should update this test suite so that it closes and reopens the connection before/after each test? |
|
@thibaultcha You're correct. Can you share how you found this root cause? How did you debug and come to your conclusion? |
|
I sniffed the network traffic with |
hbagdi
force-pushed
the
fix-certificate-api
branch
2 times, most recently
from
dec28b4 to
ab0b5ae
Compare
hbagdi
changed the title
|
@thibaultcha Thank you for sharing the approach! The PR has been updated to add handling for |
kong/api/routes/certificates.lua
Outdated
| name = sni, | ||
| ssl_certificate_id = ssl_cert.id, | ||
| } | ||
| local function update_certificate(self, dao_factory, helpers) |
There was a problem hiding this comment.
style: we jump 2 lines between function declarations consistency in this codebase. Would you mind updating? Thanks!
| end | ||
|
|
||
| return helpers.responses.send_HTTP_OK(ssl_cert) | ||
| end |
There was a problem hiding this comment.
ditto: missing line jump below the function declaration.
kong/api/routes/certificates.lua
Outdated
| return update_certificate(self, dao_factory, helpers) | ||
| -- return crud.patch(self.params, dao_factory.ssl_certificates, { | ||
| -- id = self.ssl_certificate_id | ||
| -- }) |
There was a problem hiding this comment.
We can delete this code that is commented out now.
| assert.equal(2, #json.data) | ||
| end) | ||
|
|
||
| it("returns Bad Request if only certificate is specified", function() |
There was a problem hiding this comment.
We now catch those, that's great 👍
| local json = cjson.decode(body) | ||
|
|
||
| assert.equals("certificate with id " .. cert_foo.id .. | ||
| " in use for entry with name foo.com", json.message) |
There was a problem hiding this comment.
style: ideally, those extra argument after a line jump would align with the first argument of the caller:
assert.equals("certificate with id " .. cert_foo.id ..
" in use for entry with name foo.com", json.message)| end) | ||
|
|
||
| it("returns a conflict when a pre-existing sni present in " .. | ||
| "the request is associated with another certificate", function() |
There was a problem hiding this comment.
ditto: ideally, this aligns with the first argument as well.
| describe("GET", function() | ||
| it("retrieves all certificates", function() | ||
| it("returns a conflict when a pre-existing sni present in " .. | ||
| "the request is associated with another certificate", function() |
| local json = cjson.decode(body) | ||
|
|
||
| assert.equal(0, #json.snis) | ||
| assert.matches('"snis":[]', body, nil, true) |
thibaultcha
added
pr/ready (but hold merge)
hbagdi
force-pushed
the
fix-certificate-api
branch
from
ab0b5ae to
1907417
Compare
|
Thank you! Merged with the following patch: diff --git a/kong/api/routes/certificates.lua b/kong/api/routes/certificates.lua
index 2c176002..b7043073 100644
--- a/kong/api/routes/certificates.lua
+++ b/kong/api/routes/certificates.lua
@@ -7,9 +7,10 @@ local function create_certificate(self, dao_factory, helpers)
local snis
if type(self.params.snis) == "string" then
snis = utils.split(self.params.snis, ",")
- self.params.snis = nil
end
+ self.params.snis = nil
+
if snis then
-- dont add the certificate or any snis if we have an SNI conflict
-- its fairly inefficient that we have to loop twice over the datastore
@@ -18,22 +19,20 @@ local function create_certificate(self, dao_factory, helpers)
for _, sni in ipairs(snis) do
if snis_in_request[sni] then
- return helpers.responses.send_HTTP_CONFLICT(
- "duplicate requested sni name " .. sni
- )
+ return helpers.responses.send_HTTP_CONFLICT("duplicate SNI in " ..
+ "request: " .. sni)
end
- local cnt, err = dao_factory.ssl_servers_names:count({
+ local cnt, err = dao_factory.ssl_servers_names:count {
name = sni,
- })
+ }
if err then
return helpers.yield_error(err)
end
if cnt > 0 then
- return helpers.responses.send_HTTP_CONFLICT(
- "entry already exists with name " .. sni
- )
+ return helpers.responses.send_HTTP_CONFLICT("SNI already exists: "
+ .. sni)
end
snis_in_request[sni] = true
@@ -45,12 +44,12 @@ local function create_certificate(self, dao_factory, helpers)
return helpers.yield_error(err)
end
+ ssl_cert.snis = setmetatable({}, cjson.empty_array_mt)
+
-- insert SNIs if given
if snis then
- ssl_cert.snis = setmetatable({}, cjson.empty_array_mt)
-
- for _, sni in ipairs(snis) do
+ for i, sni in ipairs(snis) do
local ssl_server_name = {
name = sni,
ssl_certificate_id = ssl_cert.id,
@@ -61,7 +60,7 @@ local function create_certificate(self, dao_factory, helpers)
return helpers.yield_error(err)
end
- table.insert(ssl_cert.snis, row.name)
+ ssl_cert.snis[i] = row.name
end
end
@@ -77,6 +76,7 @@ local function update_certificate(self, dao_factory, helpers)
if err then
return helpers.yield_error(err)
end
+
if not ssl_cert then
return helpers.responses.send_HTTP_NOT_FOUND()
end
@@ -84,6 +84,9 @@ local function update_certificate(self, dao_factory, helpers)
local snis
if type(self.params.snis) == "string" then
snis = utils.split(self.params.snis, ",")
+
+ elseif self.params.snis == ngx.null then
+ snis = {}
end
self.params.snis = nil
@@ -99,14 +102,13 @@ local function update_certificate(self, dao_factory, helpers)
if snis then
for _, sni in ipairs(snis) do
if snis_in_request[sni] then
- return helpers.responses.send_HTTP_CONFLICT(
- "duplicate requested sni name " .. sni
- )
+ return helpers.responses.send_HTTP_CONFLICT("duplicate SNI in " ..
+ "request: " .. sni)
end
- local sni_in_db, err = dao_factory.ssl_servers_names:find({
+ local sni_in_db, err = dao_factory.ssl_servers_names:find {
name = sni,
- })
+ }
if err then
return helpers.yield_error(err)
end
@@ -114,29 +116,32 @@ local function update_certificate(self, dao_factory, helpers)
if sni_in_db then
if sni_in_db.ssl_certificate_id ~= ssl_cert.id then
return helpers.responses.send_HTTP_CONFLICT(
- "certificate with id " .. sni_in_db.ssl_certificate_id ..
- " in use for entry with name " .. sni
+ "SNI '" .. sni .. "' already associated with existing " ..
+ "certificate (" .. sni_in_db.ssl_certificate_id .. ")"
)
end
+
snis_in_db[sni] = true
end
+
snis_in_request[sni] = true
end
end
- local old_snis, err = dao_factory.ssl_servers_names:find_all({
- ssl_certificate_id = ssl_cert.id
- })
+ local old_snis, err = dao_factory.ssl_servers_names:find_all {
+ ssl_certificate_id = ssl_cert.id,
+ }
if err then
return helpers.yield_error(err)
end
-- update certificate if necessary
if self.params.key or self.params.cert then
- local err
self.params.created_at = ssl_cert.created_at
- ssl_cert, err = dao_factory.ssl_certificates:update(self.params,
- { id = self.params.id }, { full = true })
+
+ ssl_cert, err = dao_factory.ssl_certificates:update(self.params, {
+ id = self.params.id,
+ }, { full = true })
if err then
return helpers.yield_error(err)
end
@@ -146,37 +151,40 @@ local function update_certificate(self, dao_factory, helpers)
if not snis then
for i = 1, #old_snis do
- table.insert(ssl_cert.snis, old_snis[i].name)
+ ssl_cert.snis[i] = old_snis[i].name
end
+
return helpers.responses.send_HTTP_OK(ssl_cert)
end
-- insert/delete SNIs into db if snis field was present in the request
- for _, sni in ipairs(snis) do
+ for i, sni in ipairs(snis) do
if not snis_in_db[sni] then
local ssl_server_name = {
name = sni,
ssl_certificate_id = ssl_cert.id,
}
- local _, err = dao_factory.ssl_servers_names:insert(
- ssl_server_name
- )
+
+ local _, err = dao_factory.ssl_servers_names:insert(ssl_server_name)
if err then
return helpers.yield_error(err)
end
end
- table.insert(ssl_cert.snis, sni)
+
+ ssl_cert.snis[i] = sni
end
-- delete snis which should no longer use ssl_cert
for i = 1, #old_snis do
if not snis_in_request[old_snis[i].name] then
- dao_factory.ssl_servers_names:delete({ name = old_snis[i].name })
-- ignoring error
-- if we want to return an error here
-- to return 4xx here, the current transaction needs to be
-- rolled back else we risk an invalid state and confusing
-- the user
+ dao_factory.ssl_servers_names:delete({
+ name = old_snis[i].name,
+ })
end
end
@@ -208,7 +216,7 @@ return {
ssl_certificates[i].snis = setmetatable({}, cjson.empty_array_mt)
for j = 1, #rows do
- table.insert(ssl_certificates[i].snis, rows[j].name)
+ ssl_certificates[i].snis[j] = rows[j].name
end
end
@@ -222,11 +230,12 @@ return {
PUT = function(self, dao_factory, helpers)
-- no id present, behaviour should be same as POST
if not self.params.id then
- return create_certificate(self, dao_factory, helpers)
+ create_certificate(self, dao_factory, helpers)
+ return -- avoid tail call
end
- --id present in body
- return update_certificate(self, dao_factory, helpers)
+ -- id present in body
+ update_certificate(self, dao_factory, helpers)
end,
},
@@ -281,7 +290,7 @@ return {
end
for i = 1, #rows do
- table.insert(row.snis, rows[i].name)
+ row.snis[i] = rows[i].name
end
return helpers.responses.send_HTTP_OK(row)
@@ -290,7 +299,7 @@ return {
PATCH = function(self, dao_factory, helpers)
self.params.id = self.ssl_certificate_id
- return update_certificate(self, dao_factory, helpers)
+ update_certificate(self, dao_factory, helpers)
end,
diff --git a/spec/02-integration/03-dao/helpers.lua b/spec/02-integration/03-dao/helpers.lua
index 721cfc21..9da684a3 100644
--- a/spec/02-integration/03-dao/helpers.lua
+++ b/spec/02-integration/03-dao/helpers.lua
@@ -1,7 +1,7 @@
local spec_helpers = require "spec.helpers"
local conf_loader = require "kong.conf_loader"
-local DATABASES = {"postgres", "cassandra"}
+local DATABASES = {"postgres"}-- "cassandra"}
local function for_each_dao(fn)
for i = 1, #DATABASES do
diff --git a/spec/02-integration/04-admin_api/06-certificates_routes_spec.lua b/spec/02-integration/04-admin_api/06-certificates_routes_spec.lua
index 3aac15c7..b0657c9e 100644
--- a/spec/02-integration/04-admin_api/06-certificates_routes_spec.lua
+++ b/spec/02-integration/04-admin_api/06-certificates_routes_spec.lua
@@ -26,8 +26,8 @@ describe("Admin API: #" .. kong_config.database, function()
setup(function()
dao = assert(DAOFactory.new(kong_config))
-
helpers.run_migrations(dao)
+
assert(helpers.start_kong({
database = kong_config.database
}))
@@ -42,7 +42,6 @@ describe("Admin API: #" .. kong_config.database, function()
end)
describe("/certificates", function()
-
before_each(function()
dao:truncate_tables()
local res = assert(client:send {
@@ -58,7 +57,7 @@ describe("Admin API: #" .. kong_config.database, function()
assert.res_status(201, res)
end)
-
+
describe("GET", function()
it("retrieves all certificates", function()
local res = assert(client:send {
@@ -76,8 +75,7 @@ describe("Admin API: #" .. kong_config.database, function()
end)
end)
- describe("POST", function()
-
+ describe("POST", function()
it("returns a conflict when duplicates snis are present in the request", function()
local res = assert(client:send {
method = "POST",
@@ -92,7 +90,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals('duplicate requested sni name foobar.com', json.message)
+ assert.equals("duplicate SNI in request: foobar.com", json.message)
-- make sure we dont add any snis
res = assert(client:send {
@@ -131,7 +129,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals("entry already exists with name foo.com", json.message)
+ assert.equals("SNI already exists: foo.com", json.message)
-- make sure we only have two snis
res = assert(client:send {
@@ -181,6 +179,26 @@ describe("Admin API: #" .. kong_config.database, function()
assert.same({ "foobar.com", "baz.com" }, json.snis)
end
end)
+
+ it_content_types("returns snis as [] when none is set", function(content_type)
+ return function()
+ local res = assert(client:send {
+ method = "POST",
+ path = "/certificates",
+ body = {
+ cert = ssl_fixtures.cert,
+ key = ssl_fixtures.key,
+ },
+ headers = { ["Content-Type"] = content_type },
+ })
+
+ local body = assert.res_status(201, res)
+ local json = cjson.decode(body)
+ assert.is_string(json.cert)
+ assert.is_string(json.key)
+ assert.matches('"snis":[]', body, nil, true)
+ end
+ end)
end)
describe("PUT", function()
@@ -232,7 +250,7 @@ describe("Admin API: #" .. kong_config.database, function()
assert.is_string(json.cert)
assert.is_string(json.key)
- assert.same({"baz.com"}, json.snis)
+ assert.same({ "baz.com" }, json.snis)
-- make sure we added an sni
res = assert(client:send {
@@ -274,8 +292,8 @@ describe("Admin API: #" .. kong_config.database, function()
-- make sure we did not add any sni
res = assert(client:send {
- method = "GET",
- path = "/snis",
+ method = "GET",
+ path = "/snis",
})
local body = assert.res_status(200, res)
@@ -309,11 +327,11 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.equals("key is required", json.key)
-
+
-- make sure we did not add any sni
res = assert(client:send {
- method = "GET",
- path = "/snis",
+ method = "GET",
+ path = "/snis",
})
local body = assert.res_status(200, res)
@@ -347,7 +365,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.equals("cert is required", json.cert)
-
+
-- make sure we did not add any sni
res = assert(client:send {
method = "GET",
@@ -390,8 +408,8 @@ describe("Admin API: #" .. kong_config.database, function()
-- make sure number of snis don't change
-- since we delete foo.com and added baz.com
res = assert(client:send {
- method = "GET",
- path = "/snis",
+ method = "GET",
+ path = "/snis",
})
body = assert.res_status(200, res)
@@ -401,7 +419,7 @@ describe("Admin API: #" .. kong_config.database, function()
local sni_names = {}
table.insert(sni_names, json.data[1].name)
table.insert(sni_names, json.data[2].name)
- assert.are.same( { "baz.com", "bar.com" } , sni_names)
+ assert.are.same({ "baz.com", "bar.com" }, sni_names)
-- make sure we did not add any certificate
res = assert(client:send {
@@ -448,8 +466,8 @@ describe("Admin API: #" .. kong_config.database, function()
-- make sure number of snis don't change
res = assert(client:send {
- method = "GET",
- path = "/snis",
+ method = "GET",
+ path = "/snis",
})
body = assert.res_status(200, res)
@@ -483,7 +501,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals("duplicate requested sni name baz.com", json.message)
+ assert.equals("duplicate SNI in request: baz.com", json.message)
-- make sure number of snis don't change
res = assert(client:send {
@@ -508,7 +526,7 @@ describe("Admin API: #" .. kong_config.database, function()
assert.equal(2, #json.data)
end)
- it("returns a conflict when a pre-existing sni present in " ..
+ it("returns a conflict when a pre-existing sni present in " ..
"the request is associated with another certificate", function()
local res = assert(client:send {
method = "PUT",
@@ -523,8 +541,9 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals("certificate with id " .. cert_foo.id ..
- " in use for entry with name foo.com", json.message)
+ assert.equals("SNI 'foo.com' already associated with " ..
+ "existing certificate (" .. cert_foo.id .. ")",
+ json.message)
-- make sure number of snis don't change
res = assert(client:send {
@@ -549,14 +568,19 @@ describe("Admin API: #" .. kong_config.database, function()
assert.equal(2, #json.data)
end)
- it("deletes all snis from a certificate if snis field is blank", function()
+ it("deletes all snis from a certificate if snis field is JSON null", function()
+ -- Note: we currently do not support unsetting a field with
+ -- form-urlencoded requests. This depends on upcoming work
+ -- to the Admin API. We here follow the road taken by:
+ -- https://github.com/Kong/kong/pull/2700
local res = assert(client:send {
method = "PUT",
- path = "/certificates?snis=",
+ path = "/certificates",
body = {
+ snis = ngx.null,
id = cert_bar.id,
},
- headers = { ["Content-Type"] = "application/x-www-form-urlencoded" },
+ headers = { ["Content-Type"] = "application/json" },
})
local body = assert.res_status(200, res)
@@ -592,7 +616,6 @@ describe("Admin API: #" .. kong_config.database, function()
end)
describe("/certificates/:sni_or_uuid", function()
-
before_each(function()
dao:truncate_tables()
local res = assert(client:send {
@@ -635,7 +658,9 @@ describe("Admin API: #" .. kong_config.database, function()
end)
describe("PATCH", function()
- local cert_foo, cert_bar
+ local cert_foo
+ local cert_bar
+
before_each(function()
dao:truncate_tables()
@@ -735,7 +760,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.equals("key is required", json.key)
-
+
-- make sure we did not add any sni
res = assert(client:send {
method = "GET",
@@ -772,7 +797,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(400, res)
local json = cjson.decode(body)
assert.equals("cert is required", json.cert)
-
+
-- make sure we did not add any sni
res = assert(client:send {
method = "GET",
@@ -905,7 +930,7 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals("duplicate requested sni name baz.com", json.message)
+ assert.equals("duplicate SNI in request: baz.com", json.message)
-- make sure number of snis don't change
res = assert(client:send {
@@ -930,8 +955,8 @@ describe("Admin API: #" .. kong_config.database, function()
assert.equal(2, #json.data)
end)
- it("returns a conflict when a pre-existing sni present in " ..
- "the request is associated with another certificate", function()
+ it("returns a conflict when a pre-existing sni present in " ..
+ "the request is associated with another certificate", function()
local res = assert(client:send {
method = "PATCH",
path = "/certificates/" .. cert_bar.id,
@@ -944,8 +969,9 @@ describe("Admin API: #" .. kong_config.database, function()
local body = assert.res_status(409, res)
local json = cjson.decode(body)
- assert.equals("certificate with id " .. cert_foo.id ..
- " in use for entry with name foo.com", json.message)
+ assert.equals("SNI 'foo.com' already associated with " ..
+ "existing certificate (" .. cert_foo.id .. ")",
+ json.message)
-- make sure number of snis don't change
res = assert(client:send {
@@ -970,11 +996,18 @@ describe("Admin API: #" .. kong_config.database, function()
assert.equal(2, #json.data)
end)
- it("deletes all snis from a certificate if snis field is blank", function()
+ it("deletes all snis from a certificate if snis field is JSON null", function()
+ -- Note: we currently do not support unsetting a field with
+ -- form-urlencoded requests. This depends on upcoming work
+ -- to the Admin API. We here follow the road taken by:
+ -- https://github.com/Kong/kong/pull/2700
local res = assert(client:send {
method = "PATCH",
- path = "/certificates/" .. cert_bar.id .. "?snis=",
- headers = { ["Content-Type"] = "application/x-www-form-urlencoded" },
+ path = "/certificates/" .. cert_bar.id,
+ body = {
+ snis = ngx.null,
+ },
+ headers = { ["Content-Type"] = "application/json" },
})
local body = assert.res_status(200, res)
@@ -1059,6 +1092,7 @@ describe("Admin API: #" .. kong_config.database, function()
describe("/snis", function()
local ssl_certificate
+
before_each(function()
dao:truncate_tables()
ssl_certificate = assert(dao.ssl_certificates:insert {
@@ -1070,6 +1104,7 @@ describe("Admin API: #" .. kong_config.database, function()
ssl_certificate_id = ssl_certificate.id,
})
end)
+
describe("POST", function()
before_each(function()
dao:truncate_tables()
@@ -1159,6 +1194,7 @@ describe("Admin API: #" .. kong_config.database, function()
describe("/snis/:name", function()
local ssl_certificate
+
before_each(function()
dao:truncate_tables()
ssl_certificate = assert(dao.ssl_certificates:insert {
@@ -1170,7 +1206,7 @@ describe("Admin API: #" .. kong_config.database, function()
ssl_certificate_id = ssl_certificate.id,
})
end)
-
+
describe("GET", function()
it("retrieves a SNI", function()
local res = assert(client:send {
@@ -1184,7 +1220,7 @@ describe("Admin API: #" .. kong_config.database, function()
assert.equal(ssl_certificate.id, json.ssl_certificate_id)
end)
end)
-
+
describe("PATCH", function()
do
local test = it
@@ -1203,7 +1239,7 @@ describe("Admin API: #" .. kong_config.database, function()
cert = "foo",
key = "bar",
})
-
+
local res = assert(client:send {
method = "PATCH",
path = "/snis/foo.com", |
thibaultcha
removed
the
pr/ready (but hold merge)
Summary
Handle snis in the endpiont
PUT /certificatesThis PR is to get some feedback and start a discussion.
Specifically,
PUT /certificatesandPATCH /certificates/:sni_or_uuid.Issues resolved
Fix #2792
TODO
/certificate/:sni_name_or_uuid