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

propagate context through to the sql store #1030

Merged
merged 8 commits into from
Sep 18, 2018

Conversation

aaslamin
Copy link
Contributor

@aaslamin aaslamin commented Sep 12, 2018

With the introduction of #1019 we will need to start propagating the request context down to sql store(s) so we can instrument our database calls.

Notes:

  • Creating the spans will be done in a follow-up PR.
  • Same thing will need to be done for the other handlers.

@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 12, 2018

Unit tests pass for me locally.

@aeneasr
Copy link
Member

aeneasr commented Sep 14, 2018

Would it make sense to propagate context down to other stores too?

@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 14, 2018

Yes, it definitely does. Would you like me to do this in this PR @aeneasr?

Edit: I'll just jump on it!

…integration

Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
…t and propagate context

Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
@aaslamin aaslamin force-pushed the propagate-context-through-to-sql-store branch from dde0593 to e5cf6b8 Compare September 15, 2018 01:05
@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 15, 2018

ℹ️ Update:

• Client manager and handler have been updated to pass along context
• All consumers have also been updated accordingly

Remaining handlers (will check these on Monday):

• Consent
• JWK

@aaslamin aaslamin changed the title oauth2: propagate context through to the sql store propagate context through to the sql store Sep 15, 2018
@aaslamin
Copy link
Contributor Author

@aeneasr at this point I am a bit concerned that the PR is getting too large 😬. Could you review this as is and let me know if you want me to proceed with the Consent and JWK handlers in here or as a separate PR.

They can be 🚀'ed independently.

@aeneasr
Copy link
Member

aeneasr commented Sep 15, 2018

It's definitely a bit larger, but that's fine as there's no functional change so it's very easy to review. So far this looks good to me!

@aeneasr
Copy link
Member

aeneasr commented Sep 16, 2018

Do you want to address the rest of the managers in this PR as well? You can also create another one - it doesn't really matter to me :) Maybe it's best to have the context in one commit (the one I'll squash when merging this)

@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 17, 2018

@aeneasr it's all good, i'll do it all in this PR.

Tackling through this now - I shall keep posting updates here as I make progress or find out new things that need doing.

Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 17, 2018

Update:

• JWK manager and all of it's clients have been updated ✅

Coming up:

• Updating the JWTStrategy to pass along context where appropriate - (See comment below)
• Updating Consent Manager (this is a biggie 😨) ✅

@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 17, 2018

@aeneasr I believe the JWTStrategy interface in fosite needs to be updated to take in a context.

Why?

The implementation of this interface in Hydra makes calls to the db to fetch the latest keys:

The oauth handler consumes the Generate method.

Could you please confirm if this makes sense before I proceed with the plumbing? If so I believe this would require:

  • a PR in fosite to update the interface and any tests that break as a result
  • a PR in Hydra to pull in the new version of fosite and update the JWTStrategy interface as necessary

…t and update all consumers

Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
@aaslamin
Copy link
Contributor Author

aaslamin commented Sep 18, 2018

Update:

Consent Manager and all of its clients have been updated to pass along context. All that is left is the JWT Strategy which I have outlined a plan for above. If that makes sense, I can jump on it.

@aeneasr ready for another review 🙏

@aeneasr
Copy link
Member

aeneasr commented Sep 18, 2018

Ah yeah, that makes sense regarding JWT Strategy. It will also improve the API contract in fosite, which is using context almost everywhere anyways (well, except there and in bcrypt).

@aeneasr
Copy link
Member

aeneasr commented Sep 18, 2018

Wow this is massive! But looks good to me so far!

@aeneasr
Copy link
Member

aeneasr commented Sep 18, 2018

Thank you for your contribution!

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