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

RethinkDB #53

Merged
merged 3 commits into from
Feb 18, 2016
Merged

RethinkDB #53

merged 3 commits into from
Feb 18, 2016

Conversation

leetal
Copy link
Contributor

@leetal leetal commented Feb 11, 2016

So, i have finally added RethinkDB. All tests pass (1. see below) locally and some changes have been made to the base implementation of Hydra (no BC Breaks?). I have for example introduced the new Context interface that makes it easier to extend Hydra in the future by adding more storage contexts. I also added the ability to select which storage backend to use via the new DATABASE environment variable (defaults to postgres). There is so much to write, but i think that a code review would definitely be in place now!

Since dockertest does not support rethinkdb, all rethinkdb tests require the environment variable RETHINKDB_URL to be set prior to testing (and obviously a rethinkdb instance up and running).

There are probably lots of improvements that can be made, but at least this is as far as I have gone.
To mention a few:

  • For example, i don't know if travis tests work at all right now since the code isn't merged to ory-am/hydra.
  • The Close() function in the Contexts is never called right now. We need to find a way to intercept shutdown and interrupt signals and call Close() from there.

1. Sadly i could not test the postgres version at all due to limitations on my macbook. So all Postgres-tests fails for me, but everything else passes. BUT, I have not touched the postgres parts at all, so they should probably work. So neither vagrant (won't work until after merge) nor postgres versions tested. (But hey as i said, they should work ;) )

Please see #39 for more discussions regarding this PR.

@leetal
Copy link
Contributor Author

leetal commented Feb 11, 2016

So, of course Ladon has to be merged for the tests to even run... ;)

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

Wow cool stuff, looks like a lot of work! I'll go over everything in detail.

@leetal
Copy link
Contributor Author

leetal commented Feb 11, 2016 via email

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

Man I hope I won't break too much of this once we go fosite...

@leetal
Copy link
Contributor Author

leetal commented Feb 11, 2016 via email

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

Ha! Yeah absolutely :)

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

Okay so priorities for me right now are:

  1. Add rethink db to dockertest
  2. Review and merge ladon PR
  3. Review and merge this PR

@leetal
Copy link
Contributor Author

leetal commented Feb 11, 2016

That should do it. BUT, I think that the tests actually will pass anyways
since I have added rethinkdb to travis. Ladon just passed and I think that
the same will go for hydra as soon as ladon is merged (Ladon´s tests all
pass now, just coverage that has decreased due to some errors being hard to
reproduce like connection errors to rethinkdb) :)

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

Ah I see :) I still prefer dockertest because I don't have to set up rethink db on my local environment to get tests passing :)

@leetal
Copy link
Contributor Author

leetal commented Feb 11, 2016 via email

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

I'll try to get that done tonight. Workload is kinda insane right now but these two PRs are high priority for me :)

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

ory/dockertest#23

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2016

dockertest is merged

@leetal
Copy link
Contributor Author

leetal commented Feb 12, 2016 via email

@leetal
Copy link
Contributor Author

leetal commented Feb 12, 2016

As soon as ladon is merged and usable, i'll rebase and squash the changes and rewrite the commits to a better format.

Storage/PostgreSQL: Updated some PostgreSQL tests.
Hydra: Fixed smaller bugs.

Signed-off-by: Alexander Widerberg <[email protected]>
@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2016

Hey, thanks for cleaning up! I will need a little time to go through all the changes. Are you planing on using Hydra in a production environment? If so, it would be very good if we two could get together for example on the gitter channel and discuss next steps, caveats and architecture.

There are still some areas and concepts in Hydra which I've grown to dislike and if you want to use Hydra in production we need a clear path so you won't get hit by unexpected changes or issues.

We will use Hydra at ORY in closed beta "production" starting first of march (1.3.2016) and hopefully get some insights into stability, scalability, security and so on.

@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2016

One major issue for me is upgrading / migrating the database - automating this is something where I have very little experience in. But I don't want to break your production system because I decided to rename some struct or add some functionality..

@leetal
Copy link
Contributor Author

leetal commented Feb 13, 2016 via email

@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2016

Okay very cool, glad we're on the same page then :) I will try to answer you in detail tomorrow

@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2016

Found a couple of free minutes... :)

I do for a living so I'm well aware of that things WILL break in hydra.

Perfect. :) We will have to find a good way to migrate databases but we could for example use a version table and have an array of incremental upgrade commands (ALTER TABLE, ..).

  1. Db migrations (only useful for sql)

See above. We need to find a good way to do this for both rethinkdb and sql

  1. Cluster functionality/scalability (ability to connect multiple hydra instances together and issue commands between them)

Not needed! Hydra is written 12-factor so it's completely stateless. Hydra relies on a highly available database cluster (rethink or postgres) - so if you have a good database cluster hydra will be a good cluster as well.

What I want to do is a CLI that uses the hydra RESTful api for managing clients, policies and accounts.

  1. Improved documentation ;) Right now I really don't know how to pass
    arbitrary data in the jwt so other services using the jwt can use that data
    in a scaleable way.

Absolutely, I won't be able to go into details now but this is definitely something. If the CLI was available, I however think that reading the code would already help a lot.

Regarding JWT - I will probably remove JWTs as access tokens and introduce OpenID Connect ID tokens instead. This is truly an area which must be covered.

One thing I've wanted to write for a while is a intra-communication system
that is plug and play for more or less all go services. And to this
intra-communication system write a dashboard where the status of the
service is shown, with ability to send commands to each instance (or group
of instances). I think a system like that in hydra would make hydra more
enterprise.

Sound like a cool idea, really glad you consider hydra :)

The following is a list to keep my train of thought. I will explain the things in there another time :)

I think these are the things that need to be done / talked about are:

  • implement fosite
  • implement OpenID Connect (where should the data come from? what claims/scopes to support?)
  • refactor login and consent logic. maybe login/consent page should be responsible for getting user data
  • consider to what extend account / profile management should be implemented
  • clean up messy tests (replace database integration tests in handlers with mocks)
  • implement better granted endpoint

@aeneasr
Copy link
Member

aeneasr commented Feb 13, 2016

I forgot to add something.

Cluster functionality/scalability (ability to connect multiple hydra instances together and issue commands between them)

Not needed! Hydra is written 12-factor so it's completely stateless. Hydra relies on a highly available database cluster (rethink or postgres) - so if you have a good database cluster hydra will be a good cluster as well.

Right now it is not anticipated that hydra will consist of multiple executables. I want Hydra to never get on a level of complexity, where it isn't considered "micro service" any more. This why I don't want Hydra to have any sort of HTML output nor a complex user management and it should not become a service for connecting legacy enterprise standards / software. I think there are better solutions already available for that! Instead, hydra should be a fast, easy, powerful and yet very secure service one can use for a new (web) app or micro service environment which will provide a secure experience to millions of users. This is my vision for hydra! :)

@@ -192,7 +192,8 @@ The CLI currently requires two environment variables:
| PORT | Which port to listen on | number | 443 |
| HOST | Which host to listen on | ip or hostname | empty (all) |
| HOST_URL | Hydra's host URL | url | "https://localhost:4443" |
| DATABASE_URL | PostgreSQL Database URL | `postgres://user:password@host:port/database` | empty |
| DATABASE_URL | Database URL | e.g: `postgres://user:password@host:port/database` | empty |
| DATABASE | Database Technology | string e.g: `postgres` or `rethinkdb` | postgres |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be done via:

  • postgres://user:password@host:port/database - postgres
  • rethinkdb://... - rethinkdb
  • mysql://...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could, no standard way of doing it was implemented so i just chose the easier way. I can fix this today. I think that prefixing it would me much more neat.

@aeneasr
Copy link
Member

aeneasr commented Feb 14, 2016

I've took a closer look at about 50% and everything looks really well. Give me a little bit more time to review everything in detail so I don't miss anything :)

@aeneasr
Copy link
Member

aeneasr commented Feb 15, 2016

Done! :) Everything else LGTM

Alexander Widerberg added 2 commits February 16, 2016 11:02
Storage/RethinkDB: Refactored smaller parts in conformance to discussions in #53.

Signed-off-by: Alexander Widerberg <[email protected]>
…ogy instead of using an extra environment variable DATABASE (as per discussions in #53).

Signed-off-by: Alexander Widerberg <[email protected]>
@aeneasr
Copy link
Member

aeneasr commented Feb 18, 2016

Thanks (y)

aeneasr pushed a commit that referenced this pull request Feb 18, 2016
@aeneasr aeneasr merged commit ca40af7 into ory:master Feb 18, 2016
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