decouples middleware modifications #1391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/CLI
vroldanbet
force-pushed
the
decouple-middleware-modifications
branch
from
c8a1c7a
to
9c06e87
Compare
vroldanbet
force-pushed
the
decouple-middleware-modifications
branch
from
50b46d5
to
efc36e3
Compare
vroldanbet
force-pushed
the
decouple-middleware-modifications
branch
2 times, most recently
from
cd93f12
to
783cbf5
Compare
until now middleware modifications composed both unary and streaming middlewares. This worked under the assumption that we wanted the same order in each type of API method. SpiceDB relies on `grpc-ecosystem/go-grpc-middleware` to build [chains of interceptors](https://github.com/grpc-ecosystem/go-grpc-middleware/blob/bfde8086e68d6a129363b063ffd6518ab568467d/doc.go), which is not natively supported by the go gRPC SDK. The order of execution is, assuming a slice of interceptors, from left to right. Because of the design of gRPC Streaming APIs, composed of stream creation and actual streaming of messages, while the chain of interceptors will follow the expected middleware chain order, the actual interception of messages (`RecvMsg` and `SendMsg`) will happen in the opposite order. Having middlewares depend on each other is a not good practice, but there are circumstances where at the very least you need a middleware to be the first or the last in the chain, think middlewares for auditing purposes should happen earlier, otherwise, it wouldn't catch requests that fail earlier in the chain. This can be addressed by reversing the order of the middleware to obtain the expected order, but this hasn't been possible because the `MiddlewareModification` API had both unary and streaming coupled. This commit separates modifications to unary and streaming middlewares, generalized using Go generics. The generated options also rely on a modification to `ecordell/optgen` in order to support generics. See ecordell/optgen#9
vroldanbet
force-pushed
the
decouple-middleware-modifications
branch
from
783cbf5
to
3983967
Compare
I did a whole thing asserting order in #1395 |
See dependency addressed in 21b87a4
tools/analyzers/go.work.sum
Outdated
There was a problem hiding this comment.
Should this file be in the PR?
There was a problem hiding this comment.
The file predated this PR, it just got updated, which makes me think it should have been updated in a previous PR but wasn't. We have some actions in CI to fail if the repository is dirty, but somehow ignored this file.
The file was introduced in 43678a7#diff-583a688fd5bc869dc63c64d19cb1dab06bfbde70115e13ad54f7e280ca57794eR1-R2. I'm not sure there is a reason to check it in git. If not, I can open a follow-up PR to address it.
@ecordell mind clarifying whether having it in git was intentional or not?☝🏻
There was a problem hiding this comment.
Yes it was intentional. Normally we wouldn't commit go.work like this, but it's just to make the tools find all of the right dependencies.
grpc middleware order assertion
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
until now middleware modifications composed both unary and streaming middlewares. This worked under the assumption that we wanted the same order in each type of API method.
SpiceDB relies on
grpc-ecosystem/go-grpc-middlewareto build chains of interceptors, which is not nativelysupported by the go gRPC SDK. The order of execution is, assuming a slice of interceptors, from left to right.
Because of the design of gRPC Streaming APIs, composed of stream creation and actual streaming of messages, while the chain of interceptors will follow the expected middleware chain order, the actual interception of messages (
RecvMsgandSendMsg) will happen in the opposite order.Having middlewares depend on each other is a not good practice, but there are circumstances
where at the very least you need a middleware to be the first or the last in the chain, think
middlewares for auditing purposes should happen earlier, otherwise, it wouldn't catch requests that
fail earlier in the chain.
This can be addressed by reversing the order of the middleware to obtain the expected order, but this hasn't been possible because the
MiddlewareModificationAPI had both unary and streaming coupled.This commit separates modifications to unary and streaming middlewares, generalized using Go generics.
ℹ️ The generated options rely on a modification to
ecordell/optgenin order to support generics. See ecordell/optgen#9