feature: support custom interceptors#143
Merged
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
📝 WalkthroughWalkthroughAdds support for configured unary gRPC interceptor plugins: schema and config additions, server-side validated chaining of named interceptors, two test interceptor implementations with e2e tests, adjustments to tests/util dialing, a Dockerfile/go.mod update, and a CI workflow to run the e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as gRPC Client
participant Server as gRPC Server
participant Inter1 as Interceptor1
participant Inter2 as Interceptor2
participant Handler as Request Handler
Client->>Server: Send Ping request
Server->>Inter1: UnaryServerInterceptor (configured order)
Inter1->>Inter1: Create marker, inject into context
Inter1->>Inter2: Call next interceptor with context
Inter2->>Inter2: Read marker from context, log/check
Inter2->>Handler: Invoke final handler
Handler-->>Inter2: Return response
Inter2-->>Inter1: Return response
Inter1-->>Server: Return response
Server-->>Client: Send Ping response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom unary server interceptors to the gRPC plugin, allowing users to register and chain custom interceptors for cross-cutting concerns like authentication, rate limiting, and logging.
Changes:
- Added interceptor configuration support with ordered execution
- Implemented test interceptors demonstrating context propagation between chained interceptors
- Added comprehensive end-to-end test suite with GitHub Actions workflow
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server.go | Implements custom interceptor chaining with configuration-based ordering |
| config.go | Adds Interceptors field to support list of interceptor names |
| schema.json | Adds JSON schema documentation for the interceptors configuration |
| tests/interceptor1/interceptor1.go | Test interceptor that creates and propagates context markers |
| tests/interceptor2/interceptor2.go | Test interceptor that validates context propagation |
| tests/grpc_interceptors_test.go | End-to-end test validating interceptor chaining and ordering |
| tests/configs/.rr-grpc-rq-interceptors.yaml | Test configuration demonstrating interceptor setup |
| .github/workflows/grpc-interceptors.yml | CI workflow for automated testing |
| tests/mock/doc.go | Documentation for mock logger package |
| tests/doc.go | Documentation for test package |
| tests/interceptor1/doc.go | Documentation for interceptor1 test package |
| tests/interceptor2/doc.go | Documentation for interceptor2 test package |
| protoc_plugins/Dockerfile | Updates Docker build for improved cross-platform support |
| protoc_plugins/go.mod | Updates Go version specification |
| protoc_plugins/go.sum | Dependency checksum updates |
| tests/grpc_plugin_test.go | Minor improvement to use context-aware dialer |
| go.work.sum | Workspace dependency updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.go (1)
32-58:⚠️ Potential issue | 🔴 Critical
p.interceptor(metrics + logging) is silently dropped for all servers without custom interceptors — critical regression.
grpc.ChainUnaryInterceptor(unaryInterceptors...)is only appended tooptsinside theifblock. When no custom interceptors are configured (the common case for all existing deployments),p.interceptoris never passed togrpc.NewServer. This means Prometheus metrics (rr_grpc_request_duration_seconds,rr_grpc_request_total,rr_grpc_requests_queue) and per-request structured logs are silently lost.TestGRPCMetricswill fail against the unmodified metrics config.The AI summary confirms this was previously applied unconditionally outside the block and was removed in this PR.
Fix: always apply the interceptor chain; only conditionally append custom interceptors:
🐛 Proposed fix
unaryInterceptors := []grpc.UnaryServerInterceptor{ grpc.UnaryServerInterceptor(p.interceptor), } - // if we have interceptors in the config, we need to chain them with our interceptor, and add them to the server options - if len(p.config.Interceptors) > 0 && len(interceptors) > 0 { - // we need to loop backwards, since the first interceptor in the list should be the last one to execute, and the last one should be the first to execute - for i := len(p.config.Interceptors) - 1; i >= 0; i-- { - name := p.config.Interceptors[i] - if _, ok := interceptors[name]; !ok { - // we should raise an error here, since we may silently ignore let's say auth interceptor, which is critical for security - return nil, errors.E(op, errors.Str(fmt.Sprintf("interceptor %s is not registered", name))) - } - - unaryInterceptors = append( - unaryInterceptors, - interceptors[name].UnaryServerInterceptor(), - ) - } - - opts = append( - opts, - grpc.ChainUnaryInterceptor( - unaryInterceptors..., - ), - ) - } + if len(p.config.Interceptors) > 0 { + for _, name := range p.config.Interceptors { + if _, ok := interceptors[name]; !ok { + return nil, errors.E(op, errors.Str(fmt.Sprintf("interceptor %s is not registered", name))) + } + unaryInterceptors = append(unaryInterceptors, interceptors[name].UnaryServerInterceptor()) + } + } + + opts = append(opts, grpc.ChainUnaryInterceptor(unaryInterceptors...))Update the test YAML to list interceptors in execution order:
- interceptors: - - "interceptor2" - - "interceptor1" + interceptors: + - "interceptor1" + - "interceptor2"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.go` around lines 32 - 58, The p.interceptor (metrics+logging) is currently only added when p.config.Interceptors is non-empty, causing it to be dropped for the common case; fix by always building grpc.ChainUnaryInterceptor with p.interceptor included and only conditionally append additional interceptors from p.config.Interceptors: ensure unaryInterceptors is initialized with grpc.UnaryServerInterceptor(p.interceptor), then if len(p.config.Interceptors)>0 iterate the config (reverse order) and append interceptors[name].UnaryServerInterceptor() after validating presence in the interceptors map, and finally always append opts with grpc.ChainUnaryInterceptor(unaryInterceptors...) before grpc.NewServer so p.interceptor is applied even when no custom interceptors are configured.
🧹 Nitpick comments (1)
config.go (1)
32-33: LGTM — newInterceptorsfield is clean.The field follows the same pattern as other config fields. Schema-level validation (non-empty strings) combined with runtime resolution in
server.goprovides adequate coverage.Consider adding a brief doc comment for the field (similar to
Env's comment on line 31) to describe what the interceptor names reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 32 - 33, Add a short doc comment above the Interceptors field in the Config struct to explain that Interceptors is a list of interceptor names (strings) that will be resolved at runtime in server.go, similar to the existing comment for Env; reference the Interceptors field and Env field so readers know the pattern and where names are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protoc_plugins/Dockerfile`:
- Around line 15-21: The Dockerfile incorrectly strips the leading "v" from
TARGETVARIANT when setting GOAMD64; update the variable assignment so that for
the amd64 branch you preserve the "v" prefix (set goamd64="${TARGETVARIANT}"
when TARGETARCH=amd64 and TARGETVARIANT is non-empty) while keeping the existing
stripping behavior for GOARM (goarm="${TARGETVARIANT#v}"); ensure the
GOOS/GOARCH/GOARM/GOAMD64 export line continues to use GOAMD64 so builds with
platforms like linux/amd64/v3 produce GOAMD64="v3".
In `@schema.json`:
- Around line 37-53: The "interceptors" array schema allows duplicate entries
which causes duplicate registration in server.go; update the "interceptors"
property in schema.json to include "uniqueItems": true so JSON Schema validation
rejects duplicate interceptor names (keep the existing "type", "items", and
"examples" unchanged and only add the uniqueItems constraint for the
"interceptors" definition).
In `@server.go`:
- Around line 37-50: The reverse loop over p.config.Interceptors causes the
configured order to be inverted (first config entry executes last); change the
loop in the block that builds unaryInterceptors to iterate forward from i := 0
to len(p.config.Interceptors)-1, keep the existing missing-interceptor check
(the errors.E call using op) and append
interceptors[name].UnaryServerInterceptor() in that forward order so config
order matches execution; also update any tests/YAML (e.g., the test that used
[interceptor2, interceptor1]) to use the natural order [interceptor1,
interceptor2].
In `@tests/configs/.rr-grpc-rq-interceptors.yaml`:
- Around line 21-23: The YAML test config currently lists interceptors in
reversed order ("interceptor2", "interceptor1") as a temporary workaround for
the server.go reverse-iteration bug; after you fix the iteration in server.go,
update the interceptors array to ["interceptor1", "interceptor2"] so declaration
and runtime execution order match. Locate the interceptors list in the config
and flip the two entries to the natural declaration order once the
reverse-iteration in server.go is corrected.
In `@tests/interceptor2/interceptor2.go`:
- Around line 30-40: In UnaryServerInterceptor (method
Plugin.UnaryServerInterceptor) replace the panic when
interceptor1.MarkerFromContext(ctx) returns false with returning a gRPC status
error (e.g., status.Errorf with an appropriate codes.* value) instead of calling
panic; update the error return path so the interceptor logs the failure via
p.logger() and returns (nil, status.Error(...)) (or similar) so the RPC
goroutine does not crash, and ensure the status and codes packages are
imported/used in the interceptor code.
---
Outside diff comments:
In `@server.go`:
- Around line 32-58: The p.interceptor (metrics+logging) is currently only added
when p.config.Interceptors is non-empty, causing it to be dropped for the common
case; fix by always building grpc.ChainUnaryInterceptor with p.interceptor
included and only conditionally append additional interceptors from
p.config.Interceptors: ensure unaryInterceptors is initialized with
grpc.UnaryServerInterceptor(p.interceptor), then if len(p.config.Interceptors)>0
iterate the config (reverse order) and append
interceptors[name].UnaryServerInterceptor() after validating presence in the
interceptors map, and finally always append opts with
grpc.ChainUnaryInterceptor(unaryInterceptors...) before grpc.NewServer so
p.interceptor is applied even when no custom interceptors are configured.
---
Nitpick comments:
In `@config.go`:
- Around line 32-33: Add a short doc comment above the Interceptors field in the
Config struct to explain that Interceptors is a list of interceptor names
(strings) that will be resolved at runtime in server.go, similar to the existing
comment for Env; reference the Interceptors field and Env field so readers know
the pattern and where names are resolved.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server.go (1)
43-43:errors.Str(fmt.Sprintf(...))is redundant.
fmt.Sprintfalready returns astring; wrapping it inerrors.Strjust re-converts the same value. Usefmt.Errorf(orerrors.Errorfif the package exposes one) directly.♻️ Proposed simplification
- return nil, errors.E(op, errors.Str(fmt.Sprintf("interceptor %s is not registered", name))) + return nil, errors.E(op, fmt.Errorf("interceptor %s is not registered", name))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.go` at line 43, The return builds an error with errors.E(op, errors.Str(fmt.Sprintf("interceptor %s is not registered", name))) which redundantly wraps a fmt.Sprintf string in errors.Str; replace the inner expression with an actual error value (e.g. fmt.Errorf("interceptor %s is not registered", name) or errors.Errorf(...) if the errors package provides it) so the call becomes errors.E(op, fmt.Errorf(...)) (or errors.E(op, errors.Errorf(...))) and remove errors.Str and fmt.Sprintf wrapping.protoc_plugins/Dockerfile (1)
3-9: Consider documenting the BuildKit requirement.The
TARGETPLATFORMARG is only auto-populated by BuildKit. Without BuildKit (DOCKER_BUILDKIT=0), the ARG is empty and theteston Line 9 will fail immediately with the platform error message rather than a clear "BuildKit required" message. This is a minor edge case since BuildKit has been the default since Docker 20.10, and the guard is intentional per the buf custom-plugin constraint (the linked comment explains this). No action strictly required, but a brief comment at Line 3 or a more specific error message would aid debugging.♻️ Suggested improvement to error message
-RUN test "${TARGETPLATFORM}" = "linux/amd64" || (echo "buf plugin image must be built for linux/amd64" && exit 1) +RUN test "${TARGETPLATFORM}" = "linux/amd64" || \ + (echo "ERROR: buf plugin image must be built for linux/amd64 (got '${TARGETPLATFORM}'). Ensure Docker BuildKit is enabled and use --platform=linux/amd64." && exit 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protoc_plugins/Dockerfile` around lines 3 - 9, The Dockerfile's check using ARG TARGETPLATFORM and the RUN test (the line that tests "${TARGETPLATFORM}" = "linux/amd64") can fail silently when BuildKit is not enabled because TARGETPLATFORM is only set by BuildKit; update the Dockerfile to either (a) add a short comment above ARG TARGETPLATFORM stating "Requires BuildKit (DOCKER_BUILDKIT=1) because TARGETPLATFORM is auto-populated by BuildKit", or (b) replace the current failure message produced by the RUN test with a more specific error text that mentions BuildKit is required (referencing TARGETPLATFORM and the RUN test condition) so users get a clear hint when TARGETPLATFORM is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protoc_plugins/Dockerfile`:
- Line 18: The final image uses "FROM scratch" so the binary runs as root;
replace that final-stage base with a non-root distroless image (e.g.
"gcr.io/distroless/static-debian12:nonroot") or, if you must keep scratch, add a
numeric non-root user and switch to it (add a USER instruction with a fixed
UID/GID and ensure the built binary and any runtime dirs are chowned to that
UID/GID). Update the Dockerfile's final stage where "FROM scratch" appears and
ensure the binary file ownership and permissions match the chosen non-root UID
before the USER switch.
- Around line 18-31: ARG BUILD_TIME and ARG APP_VERSION default to empty strings
which yields empty OCI labels; change their defaults to a non-empty sentinel
(for example ARG BUILD_TIME="unknown" and ARG APP_VERSION="unknown") and keep
the LABEL org.opencontainers.image.created="${BUILD_TIME}" and
org.opencontainers.image.version="${APP_VERSION}" as-is, and add a short note
(or CI/build-doc) to supply real values via --build-arg BUILD_TIME="$(date -u
+%Y-%m-%dT%H:%M:%SZ)" and --build-arg APP_VERSION="…" when building so the
LABELs are never blank.
In `@server.go`:
- Line 37: Remove the redundant guard "len(interceptors) > 0" from the
conditional so the check runs whenever p.config.Interceptors is non-empty;
iterate over p.config.Interceptors and for each name perform the existing lookup
"if _, ok := interceptors[name]; !ok" to return the missing-interceptor error
(this works correctly for nil/empty maps because map lookups return (zero,
false)). In short, change the if that currently reads "if
len(p.config.Interceptors) > 0 && len(interceptors) > 0" to only check "if
len(p.config.Interceptors) > 0" and keep the inner per-name lookup and
error-return logic intact (referencing p.config.Interceptors and interceptors).
---
Duplicate comments:
In `@server.go`:
- Around line 36-51: Keep the forward-order iteration and explicit error
handling introduced in server.go: iterate p.config.Interceptors from i=0 to
len-1, validate each name exists in the interceptors map and return the error
via errors.E(op, errors.Str(fmt.Sprintf("interceptor %s is not registered",
name))) if missing, and append interceptors[name].UnaryServerInterceptor() into
unaryInterceptors so execution order matches configuration (symbols:
p.config.Interceptors, interceptors, unaryInterceptors, UnaryServerInterceptor,
errors.E, op).
---
Nitpick comments:
In `@protoc_plugins/Dockerfile`:
- Around line 3-9: The Dockerfile's check using ARG TARGETPLATFORM and the RUN
test (the line that tests "${TARGETPLATFORM}" = "linux/amd64") can fail silently
when BuildKit is not enabled because TARGETPLATFORM is only set by BuildKit;
update the Dockerfile to either (a) add a short comment above ARG TARGETPLATFORM
stating "Requires BuildKit (DOCKER_BUILDKIT=1) because TARGETPLATFORM is
auto-populated by BuildKit", or (b) replace the current failure message produced
by the RUN test with a more specific error text that mentions BuildKit is
required (referencing TARGETPLATFORM and the RUN test condition) so users get a
clear hint when TARGETPLATFORM is empty.
In `@server.go`:
- Line 43: The return builds an error with errors.E(op,
errors.Str(fmt.Sprintf("interceptor %s is not registered", name))) which
redundantly wraps a fmt.Sprintf string in errors.Str; replace the inner
expression with an actual error value (e.g. fmt.Errorf("interceptor %s is not
registered", name) or errors.Errorf(...) if the errors package provides it) so
the call becomes errors.E(op, fmt.Errorf(...)) (or errors.E(op,
errors.Errorf(...))) and remove errors.Str and fmt.Sprintf wrapping.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #143 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Tests
Chores