Support gRPC-JSON transcoder - #9905
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".
| // When false (the default), a request carrying an unmappable parameter is rejected by | ||
| // the gateway with 415 and never reaches the backend. |
There was a problem hiding this comment.
The revised text provides fresh evidence beyond the prior comment by newly hard-coding status 415. An unmappable query parameter is reported by the transcoder as an invalid argument, which Envoy returns as 400 Bad Request; 415 denotes an unsupported media type. Publishing 415 in the API and generated CRD documentation will send users toward content-type debugging, so state 400 or omit the numeric status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Original wording was correct, it reaches the backend and since it does not have content-type backend responds with 415
Here is the envoy logic which passes the request:
https://github.com/envoyproxy/envoy/blob/v1.39.1/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L514-L521
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.34% 81.36% +0.01%
==========================================
Files 264 266 +2
Lines 40967 41185 +218
==========================================
+ Hits 33326 33511 +185
- Misses 7640 7673 +33
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? |
2 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/.