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: unified APIs with REST, gRPC and Connect RPC #1659

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

hperl
Copy link
Collaborator

@hperl hperl commented Dec 18, 2024

This PR unifies the API service implementations between REST and gRPC, and adds with the support for Connect RPC a third protocol, all via Vanguard.

The advantages are as follows:

  • Unified, fully typed service implementations powered by gRPC. No more manual request parsing, no more duplicated code between the REST and gRPC implementations.
  • Unified middleware stack: We can use the Oathkeeper gRPC middleware to protect all API requests.
  • Support for Connect RPC, which offers modern SDKs for languages that the OpenAPI client struggles with (Kotlin, Swift).
  • Protobuf as the IDL, no more go-swagger

Requests flow through the system as follows:

  1. All services land on a handler that first checks if tthe path has been registered directly, for example for the Ory-wide health and version paths, or other paths that are REST specific. If so, the request is forwarded to that route's handler. This is always an escape hatch for REST APIs to remain backwards compatibility.
  2. Next, for REST and Connect RPC, the request is transcoded into gRPC.
  3. Next, the gRPC service implementation is called.
  4. Finally, for REST, a middleware converts the gRPC error (if any) into a Herodot error, and optionally sets the response status, and location header to one specified by the service implementation

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@hperl hperl requested a review from jonas-jonas December 18, 2024 07:56
@hperl hperl self-assigned this Dec 18, 2024
@aeneasr
Copy link
Member

aeneasr commented Dec 18, 2024

The API specs all remain the same?

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Seems like there are a few breaking changes in the OpenAPI spec, but not too many

spec/api.json Outdated
"operationId": "patchRelationships",
"requestBody": {
"content": {
"application/json": {
"schema": {
"items": {
"$ref": "#/components/schemas/relationshipPatch"
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!

spec/api.json Outdated
@@ -517,33 +504,12 @@
}
},
"description": "errorGeneric"
},
"404": {
Copy link
Member

Choose a reason for hiding this comment

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

Missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@hperl hperl mentioned this pull request Dec 20, 2024
7 tasks
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