Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kikito
Copy link
Member

@kikito kikito commented Mar 19, 2018

Migrations

  • ssl_certificates table renamed to certificates
  • ssl_servers_names table renamed to server_names. 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 now /server_names and 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.

DAO

  • ssl_certificates and ssl_servers_names' schemas translated to new DAO's schemas (renamed to certificates and server_names).
  • 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.

Future work

  • The tests on this PR are the equivalent to the ones that existed before (and were not removed like the PUT ones). However, the auto-generated APIs include more endpoints which will also need to be tested. That will be done in a separate PR.
  • I'm almost certain that the /server_names endpoint is not needed any more (it's useful for counting the total number of server names in some tests, but that's all). I will also explore the possibility of not including this endpoint on the other PR.
  • And of course the migrations need to be tested as well. That'll be yet another PR.
  • 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.

@thibaultcha thibaultcha added pr/wip A work in progress PR opened to receive feedback pr/please review labels Mar 20, 2018
@kikito kikito force-pushed the feat/certs-2-db branch 3 times, most recently from 1a79648 to d95ae4e Compare April 5, 2018 15:16
@kikito kikito force-pushed the feat/certs-2-db branch from d95ae4e to 25c7205 Compare April 5, 2018 22:26
@kikito kikito changed the title [WIP] - feat(db) move ssl_certificates from dao to db feat(db) move ssl_certificates from dao to db Apr 5, 2018
@kikito kikito removed the pr/wip A work in progress PR opened to receive feedback label Apr 5, 2018
@kikito
Copy link
Member Author

kikito commented Apr 11, 2018

In order to simplify things, I'm going to close this in favor of #3386 (which contains all the commits on this + more tests)

@kikito kikito closed this Apr 11, 2018
@kikito kikito deleted the feat/certs-2-db branch April 11, 2018 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants