feat(dao) move ssl certificates/snis to new DAO #3386

kikito · 2018-04-09T15:07:13Z

This PR continues #3315:

Migrations

ssl_certificates table renamed to certificates
ssl_servers_names table renamed to snis. Added a new field (id, UUID) and made that the PK. But also added indexes for the name field.
In postgresql these changes involved altering the tables. In Cassandra the changes involved copying all the records from the original tables to new tables (this was done with a custom migration helper which has its own PR).

Admin API

/snis is automatically generated by the new DAO admin api generator-from-schema.
/certificates is also generated, but it has some overrides, to deal with three "special" things:
- the server_names accepts a comma-separated string or an array of strings. It
  allows "setting the server names in one go" when creating (POST) or updating
  (PATCH) a certificate.
- the server_names attribute is automatically added to the certs when getting
  each one individually or when getting a list of them.
- a certificate can be referenced by its id or also by the name attribute of
  any of its associated server_names.
PUT is not allowed any more.
Deactivates some /snis endpoints that are not needed any more:
- GET /snis is not needed any more now that GET /certificates also returns the list of server names associated to every cert
- POST /snisis not needed sincePOST /certificates/id/server_names` is easier to use and does the same
- GET /snis/[name]/certificate was not needed since you can do GET /certificates/[name]` to get the same info
- The only active server names endpoints are DELETE /snis/x (to remove a server name quickly), PATCH /snis/x (to move server names quickly between certificates), GET /snis/x (for symmetry with DELETE and PATCH).

DAO

ssl_certificates and ssl_servers_names' schemas translated to new DAO's schemas (renamed to certificates and snis).
I had to include a read-before-delete in all the generated dao:delete* methods. This read was needed in order to propagate the structure through the worker events so it could be deleted properly (worker events can use any attribute
from the entitity, for example for the server names you need their name attribute. So if you delete a server name using its id you still need to read it in order to propagate an event with the name field).
certificates has a custom DAO to deal with the server_names pseudo-attribute and for "looking for a certificate using one of its server names' name"
server_names also has a custom DAO for "using the server names as if they were lists of strings", which is easier to do than "handling them as instances of the server_names schema".

Tests

the DAO:delete* methods now returns the entity which was erased instead of true, so some tests had to be changed from is_true to is_truthy.
the tests for PUT where just removed
I also removed the admin api tests from spec-old-api because they were a carbon copy of the new ones (there are no APIs, Services or Routes involved in SSL). I double-checked that we don't do anything "extra" on it before removing.
Added tests for the new admin API endpoints.

Related work

The migrations are tested in tests(0.13_0.14) test ssl cert & snis migration kong-upgrade-tests#5
Since we're doing the read-before-delete on the DAO we probably don't need the delete_by_field methods on the strategies (we can use the PK, we have it because of the "read"). We could remove those methods.

bungle

I think this is very invasive PR. It kinda points shortcomings in customizations. It is rhuge PR, KUDOS to that huge amount of work, also makes reviewing hard.

It adds new migrations (both code and sql/cql), and even new quite big migration tools
It renames entities
It adds custom daos for both entities it adds, and a new field named super
It adds custom routes for both entities it adds
It adds two new error codes in endpoints

I kinda think this is good first step, but we should really work on merging the certificates and server_names. And then I think introducing snis rename to server_names just makes another breaking change here. First step, just transfer entities to new, and try not to break too much if they were in old too. Next step, refactor, merge, redesign it. Is that needed for safer migrations or something like that?

bungle · 2018-04-11T17:08:42Z

kong/api/routes/certificates.lua

-local utils = require "kong.tools.utils"
-local cjson = require "cjson"
+local endpoints   = require "kong.api.endpoints"
+local utils       = require "kong.tools.utils"



One more new line here.

bungle · 2018-04-11T17:09:49Z

kong/api/routes/certificates.lua


+local function get_cert_by_server_name_or_id(self, db, helpers)
+  local id = self.params.certificates



New line maybe not needed, similar to pattern:

local ok, err = func() if not ok then end

E.g. where test follows immediately the declaration.

This is done

bungle · 2018-04-11T17:15:29Z

kong/api/routes/server_names.lua

+  -- GET / PATCH / DELETE /server_names/server_name are the only methods allowed
+
+  ["/server_names"] = {
+    before = method_not_allowed,


If they are not allowed, is 404 a better one? E.g. is there any endpoint /server_names that you can call with any method? If not with any, then I would go with 404. We could also add some support for autogenerator to remove endpoints, but not in this PR.

bungle · 2018-04-11T17:16:55Z

kong/api/routes/server_names.lua

+  },
+
+  ["/server_names/:server_names/certificate"] = {
+    before = method_not_allowed,


Same here, if this is the only:
/server_names/server_name

Endpoint, maybe we don't want to give reply that says that it is not allowed, users might try then other methods, and these seems no method allowed here. So I think it would be better to just have 404.

bungle · 2018-04-11T17:26:28Z

kong/dao/migrations/postgres.lua

+        local sql = fmt("UPDATE server_names SET id = '%s' WHERE name = '%s';",
+                        utils.uuid(),
+                        row.name)
+        local _, err = dao.db:query(sql)


Instead of executing these one-by-one, could up make up an transaction and do them in one query that is wrapped in transaction?

bungle · 2018-04-11T17:27:15Z

kong/db/dao/certificates.lua

+  if type(input) == "string" then
+    name_list = utils.split(input, ",")
+  elseif type(input) == "table" then
+    name_list = utils.shallow_copy(input)


new line after this

bungle · 2018-04-11T17:30:20Z

kong/db/dao/certificates.lua

+  if name_list then
+    local ok, err, err_t = db.server_names:insert_list({id = cert.id}, name_list)
+    if not ok then
+      return nil, err, err_t


This leads to interesting situations, like certificate added, but no server_names added. Do we have any concerns of that?

I would rather not have that situation, but I honestly didn't know what else to do (we don't have transactions at this level, because Cassandra).

This code reflects what the old DAO code did - it makes as much "checks" as possible before writing anything to the db.

kong/db/dao/init.lua

@@ -158,6 +166,7 @@ function _M.new(schema, strategy, errors)
    schema   = schema,
    strategy = strategy,
    errors   = errors,
+    super    = DAO, -- allows custom daos to do self.super.delete(self, ...)


bungle · 2018-04-11T17:51:30Z

kong/db/dao/server_names.lua

+      return nil, err, err_t
+    end
+    if row and row.certificate.id ~= cert_pk.id then
+      local msg = "Server Name '" .. row.name ..


On line 47 it says "Server name already exists and here Server Name . I think in general these messages should be all lower case (because they are injected usually in different places), comments on that?

bungle · 2018-04-11T17:58:05Z

kong/db/init.lua

      if not ok then
        return nil, fmt("schema of entity '%s' is invalid: %s", entity_name,
-                        err)
+                        pl_pretty.write(err_t))


Do we want this, or was this for debugging?

thibaultcha

First round of review for me!

thibaultcha · 2018-04-12T21:43:06Z

kong/db/dao/certificates.lua

+local function parse_name_list(input, errors)
+  local name_list
+  if type(input) == "string" then
+    name_list = utils.split(input, ",")


The whole point of the new Admin API endpoints is to get rid of comma-separated strings and use the new arg[]= notation instead (for form-encoded payloads). Why supporting comma-separated lists then?

thibaultcha · 2018-04-12T21:43:51Z

kong/db/dao/certificates.lua

+  end
+
+  cert.snis = nil
+  cert, err, err_t = assert(self:insert(cert))


assert() in a client-facing code path seems like this is a leftover or something?

thibaultcha · 2018-04-12T21:56:18Z

kong/db/errors.lua

+  INVALID_OFFSET        =  7, -- page(size, offset) is invalid
+  DATABASE_ERROR        =  8, -- connection refused or DB error (HTTP 500)
+  CONFLICTING_INPUT     =  9, -- user-provided data generated a conflict (HTTP 409)
+  INVALID_INPUT         = 10, -- user-provided data is not valid (HTTP 400)


This looks conflicting with the above SCHEMA_VIOLATION error (as in, it basically falls under a subcategory of schema violations). I also dislike that this error producer accepts any error message, thus can throw any type of error and is up to interpretation... All errors produced in this file (except the database-ones) have specific, precise error messages. The goal being to avoid the proliferation and possibly, duplication of similar error types.

Any thought on this @hishamhm?

This looks conflicting with the above SCHEMA_VIOLATION error

I have mixed feelings about reusing the error messages for errors detected via different sources.

On the one hand, for the user perspective I agree that the way bad input parameters were caught shouldn't affect the error message, it's an implementation detail. On the other hand, debugging gets harder when we get reports of SCHEMA_VIOLATION from the field and these don't come from the schema library.

I don't think it's the case for certificates, but if something like this starts happening in other parts of the code it could easily trip up plugin authors ("but my schema looks okay!"). But as you said, certs/snis are "abnormal" in that sense, so it's probably fine to make an exception for it in the name of better user-visible consistency.

In that case, the SCHEMA_VIOLATION should be formatted in a way consistent with those produced by the schema library (e.g. listing the offending fields, etc).

I also dislike that this error producer accepts any error message

Definitely agree with this. It shouldn't just forward the full error message, it should construct a message like the other functions in this module.

I have removed both my custom errors and used schema violations everywhere. All the errors happen on the context of the snis magical pseudo-attribute, so it does not look bad (all errors appear "scoped" by snis).

thibaultcha · 2018-04-12T22:04:14Z

kong/db/dao/snis.lua

+    end
+  end
+
+  return 1


Why choosing this return value (1) for the methods in this module instead of the conventional true?

thibaultcha · 2018-04-12T22:10:42Z

kong/api/routes/certificates.lua

    end,
  },

+  ["/certificates/:certificates"] = {


:certificates -> :certificate_id? Or even :certificate_id_or_sni if we allow querying by SNI as well

I think you need to use what @kikito used to override autogenerated.

I confirm that is the case. The auto-generated route is "certificates/certificates" so I must use the same name in order to override stuff.

That is exactly the reason. The auto-generated route is /certificates/:certificates.

We would have had to add a param called "singular_name" or something similar to the schema in order to be able to do fancy things like sometimes using singulars instead of plurals everywhere.

thibaultcha · 2018-04-12T23:10:22Z

kong/db/dao/certificates.lua

+
+  if not name_list then
+    return nil
+  end


The code would be simplified if we avoided returning nil from this function, and instead always return a table (empty if no names):

if type(input) == "table" then name_list = utils.shallow_copy(input) elseif input == ngx.null then name_list = {} else -- maybe throw an error here, looks like a programming error end -- ... return setmetatable(name_list, cjson.empty_array_mt)

This way, the below insert_with_name_list() method could do:

if #name_list > 0 then end -- ... cert.snis = name_list -- no need for cjson.empty_array fallback

The idea I was going for was this:

If you don't set the snis variable at all, it remains as it was (no programming error, maybe you where just missing a char on the key and are updating just that).

If you set the snis variable, it will create/delete whatever snis is necessary. This includes passing an empty table (all snis will be removed)

If you pass a json null, it is as if you passed an empty table.

If I do the change you propose, there are two drawbacks:

Either we consider that an empty table means "no changes" (which I think is confusing).

Or we require the user to pass the snis param every time they want to make a change on a cert, even for changing the cert or key (otherwise we set of an error).

If you think this is worth it, I will implement it, but I wanted to warn you about the costs first.

kong/db/dao/snis.lua

+  end
+
+  table.sort(name_list)
+  return name_list


thibaultcha · 2018-04-12T23:18:19Z

kong/db/dao/certificates.lua

+-- names associated to the cert, after being parsed by parse_name_list.
+-- Returns a certificate with the snis sorted alphabetically.
+function _Certificates:insert_with_name_list(cert)
+  local db = singletons.db


This is one of my biggest concerns with this PR. We need to find a way to avoid this at all costs.

I think the problem is that we have still not decided how to "refer to other tables from the DAO of one table" in the new DAO at least.

Quick solution: I think I can change kong.db.dao.init so that db is an attribute of self here, so we can do self.db.snis:whatever instead of singletons.db.snis:whatever. Would that be ok?

thibaultcha · 2018-04-12T23:20:58Z

kong/db/dao/init.lua

+    return nil, err, err_t
+  end
+  if not entity then
+    return 0


Our rationale was to not return the number of affected rows from delete() (because of the lack of support from Cassandra for this). Is the rationale behind this return value to indicate that no row was deleted?

Yes. With the read-before-delete you know which rows will (should) be affected by a delete. The 0 means "I could not find any rows". I can replace it with a true if you prefer.

thibaultcha · 2018-04-12T23:29:09Z

kong/db/errors.lua

+  NOT_FOUND             =  6, -- WHERE clause leads nowhere (HTTP 404)
+  INVALID_OFFSET        =  7, -- page(size, offset) is invalid
+  DATABASE_ERROR        =  8, -- connection refused or DB error (HTTP 500)
+  CONFLICTING_INPUT     =  9, -- user-provided data generated a conflict (HTTP 409)


Let's maybe make the description more precise: "a conflict with already inserted DB values"?

I recall discussing this with @bungle, and since this will result from UNIQUE constraints set in PostgreSQL, we wanted to name this value UNIQUE_VIOLATION to stay consistent with other _VIOLATION error types we already defined.

Since the certs/sni entities are already abnormal in the sense that they validate their own values and produce their own errors, I would rather avoid spreading that abnormality to the errors as well. So in other words, the conflict errors produced by certs/sni validations are not because of DB/schema UNIQUE constraints, but I don't think that matters and I think they can throw UNIQUE_VIOLATION which has the same semantics.

I have chosen to use schema violations instead. The errors always happen in the context of the snis magical meta-param, so it "fits" to just use schema violations for those (so they are scoped inside snis).

kikito · 2018-04-13T19:28:57Z

I made a change that makes kong/admin/routes/certificates.lua smaller: instead of "adding new methods" to the Certificates DAO (like Certificates:page_with_name_list or Certificates:insert_with_name_list) I just overrode the default (Certificates:page and Certificates:insert). These are the methods the generated admin api uses, so I didn't have to override those any more.

I did not dare changing Certificates:select because it is used in other places besides the Admin API, generating more queries for getting all the SNIS every time a cert was selected. So I still had to override one of the generated methods.

bungle

This is going to be merged on Wednesday, May 2nd. If you still want to comment, this is your final chance.

gszr

Left a few minor comments on style.

gszr · 2018-04-30T18:13:38Z

kong/dao/migrations/cassandra.lua

+      CREATE INDEX IF NOT EXISTS snis_name_idx ON snis(name);
+      CREATE INDEX IF NOT EXISTS snis_certificate_id_idx ON snis(certificate_id);
+    ]],
+    down = nil


a bit curious: assigning explicitly for style?

@Salazar I made other changes to this except this. I see it is always "defined" in other places, sometimes with empty function and sometimes with nil. Go figure. Didn't feel too bad for leaving them, :-)

gszr · 2018-04-30T18:14:06Z

kong/dao/migrations/cassandra.lua

+    ]],
+    down = nil
+  },
+


perhaps no new line for consistency?

gszr · 2018-04-30T18:14:31Z

kong/dao/migrations/cassandra.lua

+    down = nil
+  },
+
+  { name = "2018-03-26-234600_copy_records_to_new_ssl_tables",


new line after opening {

gszr · 2018-04-30T18:15:05Z

kong/dao/migrations/cassandra.lua

}
+

perhaps no new line for consistency?

gszr · 2018-04-30T18:15:51Z

kong/dao/migrations/helpers.lua

@@ -5,7 +5,6 @@ local utils = require "kong.tools.utils"

 local _M = {}

-


style: two blank lines

kong/dao/migrations/postgres.lua

@@ -1,3 +1,5 @@
+local utils = require "kong.tools.utils"
+


kong/db/dao/certificates.lua

@@ -0,0 +1,224 @@
+local cjson      = require "cjson"
+local utils      = require "kong.tools.utils"
+


thibaultcha · 2018-05-02T03:24:28Z

the tests for PUT where just removed

With PUT now back into the picture, they should probably be kept?

thibaultcha · 2018-05-02T03:31:59Z

kong/dao/migrations/helpers.lua

+  end
+
+end
+


This entire file has duplicate contents (open it locally on this branch) this absolutely needs to be fixed prior to the merge.

bungle · 2018-05-02T21:02:15Z

The PUT was moved to later work, and it is not blocking this merge.

Ehekatl · 2018-07-23T23:41:07Z

POST /snisis not needed sincePOST /certificates/id/server_names` is easier to use and does the same

where I can find the related doc?

kikito force-pushed the feat/certs-3-db branch from dcb1a7a to 6e8a368 Compare April 9, 2018 15:10

kikito changed the title ~~Feat/certs 3 db~~ feat(api) deactivate most /server_names endpoints. Add specs Apr 9, 2018

thibaultcha added the pr/please review label Apr 9, 2018

kikito mentioned this pull request Apr 11, 2018

feat(db) move ssl_certificates from dao to db #3315

Closed

kikito changed the title ~~feat(api) deactivate most /server_names endpoints. Add specs~~ feat(dao) move ssl certificates/snis to new DAO Apr 11, 2018

bungle reviewed Apr 11, 2018

View reviewed changes

kikito force-pushed the feat/certs-3-db branch 5 times, most recently from a888538 to db91399 Compare April 12, 2018 11:16

This comment has been minimized.

Sign in to view

kikito force-pushed the feat/certs-3-db branch 3 times, most recently from 39f7351 to 0e5802d Compare April 12, 2018 19:30

This comment has been minimized.

Sign in to view

thibaultcha reviewed Apr 12, 2018

View reviewed changes

kikito force-pushed the feat/certs-3-db branch 3 times, most recently from 534e33a to dc2a798 Compare April 13, 2018 19:11

kikito force-pushed the feat/certs-3-db branch from dc2a798 to d073aca Compare April 14, 2018 21:52

kikito force-pushed the feat/certs-3-db branch 6 times, most recently from 884e17a to 3e09b2e Compare April 28, 2018 01:58

bungle force-pushed the feat/certs-3-db branch from 3e09b2e to 030b102 Compare April 30, 2018 14:31

bungle approved these changes Apr 30, 2018

View reviewed changes

bungle added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 30, 2018

gszr reviewed Apr 30, 2018

View reviewed changes

bungle force-pushed the feat/certs-3-db branch from 030b102 to ef923c5 Compare April 30, 2018 18:53

thibaultcha reviewed May 2, 2018

View reviewed changes

feat(db) move ssl_cert schema to new db

977f8d9

bungle force-pushed the feat/certs-3-db branch from ef923c5 to 977f8d9 Compare May 2, 2018 21:00

bungle added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) labels May 2, 2018

bungle merged commit a6dee10 into next May 2, 2018

bungle deleted the feat/certs-3-db branch May 2, 2018 21:51

hbagdi mentioned this pull request Aug 4, 2018

Ingress controller crashes trying to create SSL Certificate Kong/kubernetes-ingress-controller#72

Closed


		local function get_cert_by_server_name_or_id(self, db, helpers)
		local id = self.params.certificates

		@@ -5,7 +5,6 @@ local utils = require "kong.tools.utils"

		local _M = {}

		@@ -0,0 +1,224 @@
		local cjson = require "cjson"
		local utils = require "kong.tools.utils"

feat(dao) move ssl certificates/snis to new DAO #3386

feat(dao) move ssl certificates/snis to new DAO #3386

Conversation

kikito commented Apr 9, 2018 • edited

Migrations

Admin API

DAO

Tests

Related work

bungle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

kikito Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikito commented Apr 13, 2018

bungle left a comment

Choose a reason for hiding this comment

gszr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

thibaultcha commented May 2, 2018

Choose a reason for hiding this comment

bungle commented May 2, 2018

Ehekatl commented Jul 23, 2018 • edited

kikito commented Apr 9, 2018 •

edited

kikito Apr 13, 2018 •

edited

Ehekatl commented Jul 23, 2018 •

edited