-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Use GRPC interceptors for bearer token #6063
base: main
Are you sure you want to change the base?
Use GRPC interceptors for bearer token #6063
Conversation
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
I don't see any interceptors in this PR |
I have started working on it , will add in next commits . |
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
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.
where all these functions are going to be used? This should be part of the PR.
Do i need to write it in the comments of the code or mention it in the PR description ? |
the new functions need to be called from the real code. What's the point of having them otherwise? |
Got it 👍🏼 |
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6063 +/- ##
==========================================
+ Coverage 96.45% 96.48% +0.03%
==========================================
Files 352 353 +1
Lines 20007 20112 +105
==========================================
+ Hits 19297 19405 +108
+ Misses 526 523 -3
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
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.
please add tests and address all comments
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
@yurishkuro can you review the new changes , I have added tests and made changes to interceptor code . |
please address all comments |
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Please resolve conflicts |
Signed-off-by: chahat sagar <109112505+chahatsagarmain@users.noreply.github.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
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.
almost ready
|
||
// NewUnaryClientInterceptor injects the bearer token header into gRPC request metadata. | ||
func NewUnaryClientInterceptor() grpc.UnaryClientInterceptor { | ||
return grpc.UnaryClientInterceptor(func( |
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.
unnecessary wrapping
return grpc.UnaryClientInterceptor(func( | |
return func( |
invoker grpc.UnaryInvoker, | ||
opts ...grpc.CallOption, | ||
) error { | ||
var token string |
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 token string |
streamer grpc.Streamer, | ||
opts ...grpc.CallOption, | ||
) (grpc.ClientStream, error) { | ||
var token string |
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 token string |
require.ErrorContains(t, err, test.expectedErr) | ||
} | ||
|
||
// Stream interceptor test |
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.
instead of comments you can wrap into a subtest
t.Run("stream", func...)
}), | ||
expectedErr: "", | ||
wantToken: "valid-token", | ||
}, |
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.
missing no-context / no-metadata test
expectedErr: "", | ||
expectedMD: metadata.MD{Key: []string{"valid-token"}}, // Valid token setup | ||
}, | ||
} |
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.
missing no-context / no-metadata test
Which problem is this PR solving?
Resolves #6035
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test