Support gRPC-JSON transcoder - #9905
nfarhadian wants to merge 6 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
67bd275 to
f3d1aae
Compare
transformConfigMapData trims a cached ConfigMap's Data to cachedConfigMapKeys plus its lexicographically first key. LocalObjectKeyReference.Key is named by the SecurityPolicy, so no static allow-list can cover it and the policy was rejected for a key the API server still holds. Feeding referenced keys into the transform would race a ConfigMap cached and trimmed before the policy referencing it exists, so this read uses the uncached API reader instead. Offline it is the same client, which has no cache to bypass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Naeem Farhadian <n.f.azizi@gmail.com>
The offline loader only copied Data, so `egctl x translate` dropped any binaryData entry. Descriptors created with `kubectl create configmap --from-file` live there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Naeem Farhadian <n.f.azizi@gmail.com>
Extracted into addRouteFilterConfigMap so a second ValueRef on the same filter can reuse the fetch, dedup and resourceTree append. The error log gains a `field` key naming which ValueRef failed; DirectResponse is otherwise unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Naeem Farhadian <n.f.azizi@gmail.com>
f3d1aae to
9bd921a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd921afc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
9bd921a to
5c4aa79
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c4aa792f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Adds grpcJSONTranscoder to HTTPRouteFilter, referenced from an HTTPRoute rule via extensionRef. The proto descriptor comes from a ConfigMap in the route's namespace and is validated at translation time, so a bad descriptor becomes a route condition instead of a rejected listener. The filter is emitted once per config on the HCM, disabled by default, and enabled per route through typedPerFilterConfig, so the descriptor is not copied onto every route. Rejected where it cannot work: a backendRef filter has no route table to enable it on, and a GRPCRoute only carries traffic that is already gRPC. Both would otherwise be dropped silently. proto-descriptor joins the provider's cachedConfigMapKeys, or the informer transform drops it from any ConfigMap carrying more than one data entry. That transform also trims Data to its first key, so only a sole binaryData entry can stand in for the named key; a sole Data entry would mean something different in-cluster than it does to egctl. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Naeem Farhadian <n.f.azizi@gmail.com>
5c4aa79 to
b284757
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9905 +/- ##
==========================================
+ Coverage 81.33% 81.40% +0.07%
==========================================
Files 264 266 +2
Lines 40963 41181 +218
==========================================
+ Hits 33317 33524 +207
- Misses 7645 7656 +11
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
hey can the API be scoped to the fields needed for your use case ? |
|
Currently we enable convert_grpc_status, always_print_primitive_fields in print options and also service list. But having other fields does not cost much. I don't understand validateHTTPBindings you mentioned. Do you mean having request_validation_options in transcoder configuration or validating the descriptor bindings in runtime? |
| order = 305 | ||
| case isFilterType(filter, egv1a1.EnvoyFilterGRPCStats): | ||
| case isFilterType(filter, egv1a1.EnvoyFilterGRPCJSONTranscoder): | ||
| order = 306 |
There was a problem hiding this comment.
[P1] Prevent rematching from bypassing the destination route's authentication policy
The transcoder runs at order 306, after ext_authz (6), JWT authentication (10), and rate limiting. With the default matchIncomingRequestRoute: false, Envoy's transcoder rewrites the path and clears the route cache, but filters that already ran are not restarted.
For example, an HTTPRoute accepting JSON without authentication can rematch onto a GRPCRoute protected by a JWT SecurityPolicy. JWT authentication has already evaluated the original HTTPRoute, so the destination GRPCRoute's JWT requirement is not checked. The new documentation at site/content/en/latest/tasks/traffic/grpc-json-transcoder.md:125-127 explicitly suggests using rematching for policies attached to a GRPCRoute, which is unsafe for these policies with this ordering.
Please define and enforce safe policy semantics for rematching, correct the guidance, and add an e2e case where the destination GRPCRoute requires authentication. With the current ordering, authentication must protect the incoming HTTPRoute rather than only the destination GRPCRoute. Simply changing filter order also needs validation against route-scoped filter enablement.
Source references: Envoy transcoder path rewrite/cache clearing, JWT per-route verification in decodeHeaders.
| // listener when Envoy fails to build its own pool. Linking here makes it this route's | ||
| // problem instead. | ||
| func validateDescriptorPool(fds *descriptorpb.FileDescriptorSet) error { | ||
| if _, err := protodesc.NewFiles(fds); err != nil { |
There was a problem hiding this comment.
[P1] Validate HTTP bindings before publishing the descriptor on the listener
protodesc.NewFiles() validates protobuf definitions and references, but not the semantics of google.api.http annotations. A descriptor can link successfully while specifying, for example, body: "missing_field" when the request message has no such field.
Envoy's JsonTranscoderConfig constructor calls createMethodInfo() / resolveField() and rejects such a binding. Since this PR installs the descriptor in the listener-level HCM filter, the failure rejects the listener update rather than producing the promised route-local Accepted=False condition and HTTP 500. This can affect unrelated routes sharing the listener.
Please validate the selected services' HTTP bindings against Envoy's supported semantics before generating IR, and add negative coverage demonstrating that invalid bindings are surfaced on the route without rejecting the listener. Descriptor closure and protobuf linking alone do not provide that guarantee.
Source: Envoy JsonTranscoderConfig constructor and createMethodInfo.
This branch has not been deployed
3 participants
What this PR does / why we need it:
Adds
grpcJSONTranscodertoHTTPRouteFilter, referenced from an HTTPRoute rule viaextensionRef. REST/JSON clients can call a gRPC backend without a separate transcoding proxy.The proto descriptor comes from a ConfigMap in the route's namespace and is parsed and validated at translation time, so a bad descriptor becomes a route condition (
Accepted=False,UnsupportedValue) plus a 500, rather than a rejected listener. Descriptors must be built withprotoc --include_imports.The filter is emitted once per config on the HCM, disabled by default, and enabled per route through
typedPerFilterConfig, so the descriptor is not copied onto every route. Listeners with no transcoder are byte-identical to before.Rejected where it cannot work: a
backendReffilter has no route table to enable it on, and a GRPCRoute only carries traffic that is already gRPC.Worth reviewer attention:
matchIncomingRequestRoutedefaults to false (Envoy's default), so the rewrittengRPC path is re-matched and needs a route. Documented, with the single-rule form
shown first.
Which issue(s) this PR fixes:
Fixes #1776
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.