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
✨ new feat: selector middleware #511
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks cool, but I think it's missing some things:
- Where are the tests? We should include some tests of this new functionality.
- Could you include an
selector_example_test.go
file with an example of how to use this? It will automatically be shown as part of the documentation. See https://go.dev/blog/examples.
Yes, that's how it should be. I'll take care of it. |
Codecov ReportBase: 84.01% // Head: 58.74% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v2 #511 +/- ##
===========================================
- Coverage 84.01% 58.74% -25.27%
===========================================
Files 30 31 +1
Lines 932 1583 +651
===========================================
+ Hits 783 930 +147
- Misses 110 590 +480
- Partials 39 63 +24
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests, just a few suggestions
interceptors/selector/doc.go
Outdated
`selector` a generic server-side selector middleware for gRPC. | ||
|
||
# Server Side Selector Middleware | ||
It allows to set check rules to whitelist or blacklist middleware such as Auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, do you mind changing these phrases to allowlist
and blocklist
consistently through this PR?
It allows to set check rules to whitelist or blacklist middleware such as Auth | |
It allows to set check rules to allowlist or blocklist middleware such as Auth |
interceptors/selector/selector.go
Outdated
|
||
type Option func(o *options) | ||
|
||
// WithMatch ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment needs improving
// WithMatch ... | |
// WithMatch configures what condition should trigger the selector to run the interceptor. | |
// If used multiple times, the last one specified will be used. |
interceptors/selector/selector.go
Outdated
return optCopy | ||
} | ||
|
||
// AlwaysMatch is the default implementation, always selected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// AlwaysMatch is the default implementation, always selected | |
// AlwaysMatch can be used to always run the interceptor. This is the default behavior. |
interceptors/selector/selector.go
Outdated
return true | ||
} | ||
|
||
// UnaryServerInterceptor returns a new unary server interceptors that performs per-request selector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// UnaryServerInterceptor returns a new unary server interceptors that performs per-request selector. | |
// UnaryServerInterceptor returns a new unary server interceptor that will decide whether to call the interceptor on the behavior of the MatchFunc. |
interceptors/selector/selector.go
Outdated
} | ||
|
||
// UnaryServerInterceptor returns a new unary server interceptors that performs per-request selector. | ||
func UnaryServerInterceptor(interceptors grpc.UnaryServerInterceptor, opts ...Option) grpc.UnaryServerInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards removing MatchFunc
as an option and making it a required parameter. This solves two problems:
- If you accidentally use the selector without a match func, you gain no utility from it.
- If you accidentally specify two match func options, you might be confused about which to use.
That simplifies things generally, no need for the AlwaysMatch
function, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I readjusted it, can you help me in seeing what is wrong?
interceptors/selector/doc.go
Outdated
|
||
# Server Side Selector Middleware | ||
It allows to set check rules to whitelist or blacklist middleware such as Auth | ||
Requests that match the rules will go to the next level of middleware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requests that match the rules will go to the next level of middleware | |
interceptors to toggle behavior on or off based on the request path. |
} | ||
|
||
func loginSkip(ctx context.Context, fullMethod string) bool { | ||
return fullMethod != "login" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use a realistic looking path, like you are doing in the tests
return fullMethod != "login" | |
return fullMethod != "/auth.v1.AuthService/Login" |
e3b9f5e
to
d3ce045
Compare
interceptors/selector/selector.go
Outdated
// Allow After the method is matched, the interceptor is run | ||
func Allow(methods []string) MatchFunc { | ||
return func(ctx context.Context, fullMethod string) bool { | ||
for _, s := range methods { | ||
if s == fullMethod { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
} | ||
|
||
// Block the interceptor will not run after the method matches | ||
func Block(methods []string) MatchFunc { | ||
allow := Allow(methods) | ||
return func(ctx context.Context, fullMethod string) bool { | ||
return !allow(ctx, fullMethod) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these, to be honest. Users can easily implement these themselves.
"testing" | ||
) | ||
|
||
var whiteList = []string{"/auth.v1beta1.AuthService/Login"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var whiteList = []string{"/auth.v1beta1.AuthService/Login"} | |
var allowList = []string{"/auth.v1beta1.AuthService/Login"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adjusted
testing/testpb/test.pb.go
Outdated
@@ -1,18 +1,16 @@ | |||
// Code generated by protoc-gen-go. DO NOT EDIT. | |||
// versions: | |||
// protoc-gen-go v1.25.0 | |||
// protoc-gen-go v1.28.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert these changes, I don't think these should need to be changed?
Looks like there is a formatting issue, can you run |
9add81a
to
948425e
Compare
Signed-off-by: aimuz <mr.imuz@gmail.com>
done |
Thank you for your contribution! |
It allows to set check rules to whitelist or blacklist middleware such as Auth
Requests that match the rules will go to the next level of middleware